Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Azure identity second try #394

Merged
merged 30 commits into from
Nov 26, 2024
Merged
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
dfdba78
Initial attempt
AsafMah May 22, 2024
25cadac
Initial attempt
AsafMah May 22, 2024
e571e2e
Progress
AsafMah Jun 6, 2024
e928509
Merge branch 'refs/heads/master' into azure-identity
AsafMah Jun 6, 2024
e349dac
Formatting + Warnings
AsafMah Jun 6, 2024
c46551e
test
AsafMah Jun 9, 2024
6437224
retry e2e
AsafMah Jun 9, 2024
4ddc1a1
Merge branch 'master' into azure-identity
AsafMah Nov 4, 2024
e6f67d4
Fix test
AsafMah Nov 4, 2024
f592cca
Fix test
AsafMah Nov 4, 2024
65844de
Async cloud info (should be own PR maybe?)
AsafMah Nov 4, 2024
2d880fb
Async cloud info (should be own PR maybe?)
AsafMah Nov 5, 2024
3f5ce2f
Async cloud info (should be own PR maybe?)
AsafMah Nov 5, 2024
42d8ade
Async cloud info (should be own PR maybe?)
AsafMah Nov 5, 2024
a3b89ee
I hate
AsafMah Nov 12, 2024
cddae73
Revert "I hate"
AsafMah Nov 12, 2024
09c3d29
Revert "Async cloud info (should be own PR maybe?)"
AsafMah Nov 12, 2024
fa3c67d
Revert "Async cloud info (should be own PR maybe?)"
AsafMah Nov 12, 2024
0449a67
Revert "Async cloud info (should be own PR maybe?)"
AsafMah Nov 12, 2024
e30452d
Revert "Async cloud info (should be own PR maybe?)"
AsafMah Nov 12, 2024
2984872
Merge branch 'master' into azure-identity02
AsafMah Nov 12, 2024
3650f99
I hate
AsafMah Nov 12, 2024
a4c59bf
TODO
AsafMah Nov 12, 2024
d8864a8
Added custom credential
AsafMah Nov 12, 2024
237580a
Try getting cloud data at oncej
AsafMah Nov 14, 2024
f596aa1
PR Fixes
AsafMah Nov 19, 2024
58c68fe
Merge branch 'master' into azure-identity02
AsafMah Nov 19, 2024
dc03ccd
PR Fixes
AsafMah Nov 20, 2024
2f723df
PR Fixes + tests
AsafMah Nov 20, 2024
9faae10
PR Fixes + tests
AsafMah Nov 21, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -394,7 +394,7 @@ private long determineTimeout(ClientRequestProperties properties, CommandType co

private String getAuthorizationHeaderValue() throws DataServiceException, DataClientException {
if (aadAuthenticationHelper != null) {
return String.format("Bearer %s", aadAuthenticationHelper.acquireAccessToken());
return String.format("Bearer %s", aadAuthenticationHelper.acquireAccessToken().block());
}
return null;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@
import java.net.URISyntaxException;

import org.jetbrains.annotations.NotNull;
import reactor.core.publisher.Mono;

public class AccessTokenTokenProvider extends TokenProviderBase {
public static final String ACCESS_TOKEN_TOKEN_PROVIDER = "AccessTokenTokenProvider";
private final String accessToken;

AccessTokenTokenProvider(@NotNull String clusterUrl, @NotNull String accessToken) throws URISyntaxException {
Expand All @@ -17,12 +17,7 @@ public class AccessTokenTokenProvider extends TokenProviderBase {
}

@Override
protected String acquireAccessTokenImpl() {
return accessToken;
}

@Override
protected String getAuthMethod() {
return ACCESS_TOKEN_TOKEN_PROVIDER;
protected Mono<String> acquireAccessTokenImpl() {
return Mono.just(accessToken);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,9 @@
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;

// Azure identity doesn't provide a solution for all certificate types, so for now we still use MSAL for this.
ohadbitt marked this conversation as resolved.
Show resolved Hide resolved

public class ApplicationCertificateTokenProvider extends ConfidentialAppTokenProviderBase {
public static final String APPLICATION_CERTIFICATE_TOKEN_PROVIDER = "ApplicationCertificateTokenProvider";
private final IClientCertificate clientCertificate;

ApplicationCertificateTokenProvider(@NotNull String clusterUrl, @NotNull String applicationClientId, @NotNull IClientCertificate clientCertificate,
Expand All @@ -34,9 +35,4 @@ protected IConfidentialClientApplication getClientApplication() throws Malformed
return clientApplication = builder
.build();
}

@Override
protected String getAuthMethod() {
return APPLICATION_CERTIFICATE_TOKEN_PROVIDER;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,38 +3,34 @@

package com.microsoft.azure.kusto.data.auth;

import com.microsoft.aad.msal4j.ConfidentialClientApplication;
import com.microsoft.aad.msal4j.IClientSecret;
import com.microsoft.aad.msal4j.IConfidentialClientApplication;
import java.net.MalformedURLException;
import com.azure.core.credential.TokenCredential;
import com.azure.core.http.HttpClient;
import com.azure.identity.ClientSecretCredentialBuilder;
import com.azure.identity.CredentialBuilderBase;

import java.net.URISyntaxException;

import com.azure.core.http.HttpClient;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;

public class ApplicationKeyTokenProvider extends ConfidentialAppTokenProviderBase {
private final IClientSecret clientSecret;
public static final String APPLICATION_KEY_TOKEN_PROVIDER = "ApplicationKeyTokenProvider";
public class ApplicationKeyTokenProvider extends AzureIdentityTokenProvider {
private final String clientSecret;

ApplicationKeyTokenProvider(@NotNull String clusterUrl, @NotNull String applicationClientId, @NotNull IClientSecret clientSecret,
String authorityId, @Nullable HttpClient httpClient) throws URISyntaxException {
super(clusterUrl, applicationClientId, authorityId, httpClient);
public ApplicationKeyTokenProvider(@NotNull String clusterUrl, String clientId, String clientSecret, String tenantId,
@Nullable HttpClient httpClient) throws URISyntaxException {
super(clusterUrl, tenantId, clientId, httpClient);
this.clientSecret = clientSecret;
}

@Override
protected IConfidentialClientApplication getClientApplication() throws MalformedURLException {
ConfidentialClientApplication.Builder authority = ConfidentialClientApplication.builder(applicationClientId, clientSecret)
.authority(aadAuthorityUrl);
if (httpClient != null) {
authority.httpClient(new HttpClientWrapper(httpClient));
}
return authority.build();
protected TokenCredential createTokenCredential(CredentialBuilderBase<?> builder) {
return new ClientSecretCredentialBuilder()
.clientSecret(clientSecret)
.build();
AsafMah marked this conversation as resolved.
Show resolved Hide resolved
}

@Override
protected String getAuthMethod() {
return APPLICATION_KEY_TOKEN_PROVIDER;
protected CredentialBuilderBase<?> initBuilder() {
return new ClientSecretCredentialBuilder();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
package com.microsoft.azure.kusto.data.auth;

import com.microsoft.azure.kusto.data.exceptions.DataClientException;
import com.microsoft.azure.kusto.data.exceptions.ExceptionsUtils;
import org.jetbrains.annotations.NotNull;
import reactor.core.publisher.Mono;

import java.net.URISyntaxException;

public class AsyncCallbackTokenProvider extends TokenProviderBase {
public static final String CALLBACK_TOKEN_PROVIDER = "CallbackTokenProvider";
private final Mono<String> tokenProvider;

AsyncCallbackTokenProvider(@NotNull String clusterUrl, @NotNull Mono<String> tokenProvider) throws URISyntaxException {
super(clusterUrl, null);
this.tokenProvider = tokenProvider;
}

@Override
protected Mono<String> acquireAccessTokenImpl() {
return tokenProvider
.onErrorMap(e -> {
if (e instanceof Exception) {
Exception ex = (Exception) e;
return new DataClientException(clusterUrl, ExceptionsUtils.getMessageEx(ex), ex);
} else {
return new DataClientException(clusterUrl, e.toString(), null);
}
});
}

@Override
protected String getAuthMethod() {
return CALLBACK_TOKEN_PROVIDER;
}
}
Original file line number Diff line number Diff line change
@@ -1,51 +1,26 @@
package com.microsoft.azure.kusto.data.auth;

import com.azure.core.credential.AccessToken;
import com.azure.core.credential.TokenRequestContext;
import com.azure.core.credential.TokenCredential;
import com.azure.core.http.HttpClient;
import com.azure.identity.AzureCliCredential;
import com.azure.identity.AzureCliCredentialBuilder;
import com.microsoft.azure.kusto.data.exceptions.DataClientException;
import com.microsoft.azure.kusto.data.exceptions.DataServiceException;
import com.azure.identity.CredentialBuilderBase;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;

import java.net.URISyntaxException;

public class AzureCliTokenProvider extends CloudDependentTokenProviderBase {
public static final String AZURE_CLI_TOKEN_PROVIDER = "AzureCliTokenProvider";
private TokenRequestContext tokenRequestContext;
private AzureCliCredential azureCliCredential;

public class AzureCliTokenProvider extends AzureIdentityTokenProvider {
public AzureCliTokenProvider(@NotNull String clusterUrl, @Nullable HttpClient httpClient) throws URISyntaxException {
super(clusterUrl, httpClient);

}

@Override
protected void initializeWithCloudInfo(CloudInfo cloudInfo) throws DataServiceException, DataClientException {
super.initializeWithCloudInfo(cloudInfo);
AzureCliCredentialBuilder builder = new AzureCliCredentialBuilder();

if (httpClient != null) {
builder = builder.httpClient(new HttpClientWrapper(httpClient));
}

this.azureCliCredential = builder.build();
tokenRequestContext = new TokenRequestContext().addScopes(scopes.toArray(new String[0]));
}

@Override
protected String acquireAccessTokenImpl() throws DataServiceException {
AccessToken accessToken = azureCliCredential.getToken(tokenRequestContext).block();
if (accessToken == null) {
throw new DataServiceException(clusterUrl, "Couldn't get token from Azure Identity", true);
}
return accessToken.getToken();
protected CredentialBuilderBase<?> initBuilder() {
return new AzureCliCredentialBuilder();
}

@Override
protected String getAuthMethod() {
return AZURE_CLI_TOKEN_PROVIDER;
protected TokenCredential createTokenCredential(CredentialBuilderBase<?> builder) {
return ((AzureCliCredentialBuilder) builder).build();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
package com.microsoft.azure.kusto.data.auth;

import com.azure.core.credential.AccessToken;
import com.azure.core.credential.TokenCredential;
import com.azure.core.credential.TokenRequestContext;
import com.azure.core.http.HttpClient;
import com.azure.identity.AadCredentialBuilderBase;
import com.azure.identity.CredentialBuilderBase;
import com.microsoft.azure.kusto.data.UriUtils;
import com.microsoft.azure.kusto.data.exceptions.DataClientException;
import com.microsoft.azure.kusto.data.exceptions.DataServiceException;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
import reactor.core.publisher.Mono;

import java.net.URISyntaxException;

public abstract class AzureIdentityTokenProvider extends CloudDependentTokenProviderBase {
private String clientId;
private final String tenantId;
TokenCredential cred;
TokenRequestContext tokenRequestContext;

protected static final String ORGANIZATION_URI_SUFFIX = "organizations";
protected static final String ERROR_INVALID_AUTHORITY_URL = "Error acquiring ApplicationAccessToken due to invalid Authority URL";

private String determineAadAuthorityUrl(CloudInfo cloudInfo) throws DataClientException {
String aadAuthorityUrlFromEnv = System.getenv("AadAuthorityUri");
String authorityIdToUse = tenantId != null ? tenantId : ORGANIZATION_URI_SUFFIX;
try {
return UriUtils.setPathForUri(aadAuthorityUrlFromEnv == null ? cloudInfo.getLoginEndpoint() : aadAuthorityUrlFromEnv, authorityIdToUse, true);
} catch (URISyntaxException e) {
throw new DataClientException(clusterUrl, ERROR_INVALID_AUTHORITY_URL, e);
}
}

AzureIdentityTokenProvider(@NotNull String clusterUrl, @Nullable String tenantId, @Nullable String clientId,
@Nullable HttpClient httpClient) throws URISyntaxException {
super(clusterUrl, httpClient);
this.tenantId = tenantId;
this.clientId = clientId;
}

AzureIdentityTokenProvider(@NotNull String clusterUrl, @Nullable HttpClient httpClient) throws URISyntaxException {
this(clusterUrl, null, null, httpClient);
}

@Override
protected final Mono<String> acquireAccessTokenImpl() {
return cred.getToken(tokenRequestContext).map(AccessToken::getToken);
}

@Override
protected final void initializeWithCloudInfo(CloudInfo cloudInfo) throws DataClientException, DataServiceException {
super.initializeWithCloudInfo(cloudInfo);
CredentialBuilderBase<?> builder = initBuilder();
if (httpClient != null) {
builder.httpClient(new HttpClientWrapper(httpClient));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did we forget to remove the wrapper ?
it should work without it ..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only need it to go one way now, I'll fixj

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the certificate, which still uses the old msal (because they removed some options from the new one if you remember), we'll still need it

}
if (builder instanceof AadCredentialBuilderBase<?>) {
AadCredentialBuilderBase<?> aadBuilder = (AadCredentialBuilderBase<?>) builder;
if (tenantId != null)
aadBuilder.tenantId(tenantId);

aadBuilder.authorityHost(determineAadAuthorityUrl(cloudInfo));

if (clientId == null) {
clientId = cloudInfo.getKustoClientAppId();
}

aadBuilder.clientId(clientId);

// TODO: do we need to port this code?
// if (account.homeAccountId() != null && account.homeAccountId().endsWith(PERSONAL_TENANT_IDV2_AAD)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this code handles some issue they have with msa users - i only encountered it trying to use device auth - so i dont think its a big deal anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll remove it. Worst case we'll add it again

// authorityUrl = firstPartyAuthorityUrl;
// }

}

cred = createTokenCredential(builder);
ohadbitt marked this conversation as resolved.
Show resolved Hide resolved
tokenRequestContext = new TokenRequestContext().addScopes(scopes.toArray(new String[0]));
}

protected abstract CredentialBuilderBase<?> initBuilder();

protected abstract TokenCredential createTokenCredential(CredentialBuilderBase<?> builder);
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import com.microsoft.azure.kusto.data.exceptions.ExceptionsUtils;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
import reactor.core.publisher.Mono;

import java.net.URISyntaxException;
import java.util.concurrent.Callable;
Expand All @@ -29,12 +30,17 @@ public class CallbackTokenProvider extends TokenProviderBase {
}

@Override
protected String acquireAccessTokenImpl() throws DataClientException {
try {
return tokenProvider.apply(httpClient);
} catch (Exception e) {
throw new DataClientException(clusterUrl, ExceptionsUtils.getMessageEx(e), e);
}
protected Mono<String> acquireAccessTokenImpl() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its not yours but all this callback thing is really weird here - we create a tokenProvider that accepts the httpclient and then dont pass it to the underline method - we dont need this - the user should use his client anyway ..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean if we break it anyway, sure..

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i see no reason to to use it like that

return Mono.fromCallable(() -> tokenProvider.apply(httpClient))
// TODO - is this a better way?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is a better way ?
put the onErrorMap logic in common plcae

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish there was a more better generic way than this wrapping/unwrapping madness (it got even worse when I tried to make cloudinfo async)

For now I'll do that

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you remove the http thing and make it a Callable it will be nicer - you just pass it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I did, make sure you're on the latest version

.onErrorMap(e -> {
if (e instanceof Exception) {
Exception ex = (Exception) e;
return new DataClientException(clusterUrl, ExceptionsUtils.getMessageEx(ex), ex);
} else {
return new DataClientException(clusterUrl, e.toString(), null);
}
});
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
import reactor.core.publisher.Mono;

public abstract class CloudDependentTokenProviderBase extends TokenProviderBase {
private static final String ERROR_INVALID_SERVICE_RESOURCE_URL = "Error determining scope due to invalid Kusto Service Resource URL";
Expand All @@ -28,17 +29,29 @@ public abstract class CloudDependentTokenProviderBase extends TokenProviderBase
}

@Override
synchronized void initialize() throws DataClientException, DataServiceException {
if (initialized) {
return;
}
// trace retrieveCloudInfo
cloudInfo = MonitoredActivity.invoke(
(SupplierTwoExceptions<CloudInfo, DataClientException, DataServiceException>) () -> CloudInfo.retrieveCloudInfoForCluster(clusterUrl,
httpClient),
"CloudDependentTokenProviderBase.retrieveCloudInfo", getTracingAttributes());
initializeWithCloudInfo(cloudInfo);
initialized = true;
final Mono<Void> initialize() {
return Mono.fromCallable(() -> initialized)
.flatMap(initialized -> {
if (initialized) {
return Mono.empty();
}
// trace retrieveCloudInfo
return MonitoredActivity.wrap(
CloudInfo.retrieveCloudInfoForClusterAsync(clusterUrl,
httpClient),
"CloudDependentTokenProviderBase.retrieveCloudInfo", getTracingAttributes())
.flatMap(cloudInfo -> {
this.cloudInfo = cloudInfo;

try {
initializeWithCloudInfo(cloudInfo);
} catch (DataClientException | DataServiceException e) {
return Mono.error(e);
}
this.initialized = true;
return Mono.empty();
});
});
}

protected void initializeWithCloudInfo(CloudInfo cloudInfo) throws DataClientException, DataServiceException {
Expand Down
Loading
Loading