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 27 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 @@ -8,6 +8,7 @@
import com.microsoft.azure.kusto.data.http.CloseParentResourcesStream;
import com.microsoft.azure.kusto.data.http.HttpRequestBuilder;
import com.microsoft.azure.kusto.data.http.HttpStatus;
import com.microsoft.azure.kusto.data.req.RequestUtils;
import org.apache.commons.lang3.StringUtils;
import org.apache.http.conn.EofSensorInputStream;
import org.slf4j.Logger;
Expand All @@ -23,6 +24,7 @@

public abstract class BaseClient implements Client, StreamingClient {

// TODO - this is never used?
private static final int MAX_REDIRECT_COUNT = 1;
private static final int EXTRA_TIMEOUT_FOR_CLIENT_SIDE = (int) TimeUnit.SECONDS.toMillis(30);

Expand Down Expand Up @@ -152,8 +154,7 @@ public static DataServiceException createExceptionFromResponse(String url, HttpR
private static Context getContextTimeout(long timeoutMs) {
int requestTimeout = timeoutMs > Integer.MAX_VALUE ? Integer.MAX_VALUE : Math.toIntExact(timeoutMs) + EXTRA_TIMEOUT_FOR_CLIENT_SIDE;

// See https://github.com/Azure/azure-sdk-for-java/blob/azure-core-http-netty_1.10.2/sdk/core/azure-core-http-netty/CHANGELOG.md#features-added
return Context.NONE.addData("azure-response-timeout", Duration.ofMillis(requestTimeout));
return RequestUtils.contextWithTimeout(Duration.ofMillis(requestTimeout));
}

private static void closeResourcesIfNeeded(boolean returnInputStream, HttpResponse httpResponse) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -397,7 +397,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 ((ClientSecretCredentialBuilder)builder)
.clientSecret(clientSecret)
.build();
}

@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,27 @@
package com.microsoft.azure.kusto.data.auth;

import com.microsoft.azure.kusto.data.exceptions.DataClientException;
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 -> DataClientException.unwrapThrowable(clusterUrl, e));
}

@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,82 @@
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(httpClient);
}
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);

}

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 @@ -3,38 +3,26 @@

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

import com.azure.core.http.HttpClient;
import com.microsoft.azure.kusto.data.exceptions.DataClientException;

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;

public class CallbackTokenProvider extends TokenProviderBase {
public static final String CALLBACK_TOKEN_PROVIDER = "CallbackTokenProvider";
private final CallbackTokenProviderFunction tokenProvider;
private final @NotNull Callable<String> tokenProvider;

CallbackTokenProvider(@NotNull String clusterUrl, @NotNull Callable<String> tokenProvider) throws URISyntaxException {
super(clusterUrl, null);
this.tokenProvider = (httpClient) -> tokenProvider.call();
}

CallbackTokenProvider(@NotNull String clusterUrl, @NotNull CallbackTokenProviderFunction tokenProvider,
@Nullable HttpClient httpClient) throws URISyntaxException {
super(clusterUrl, httpClient);
this.tokenProvider = tokenProvider;
}

@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).onErrorMap(e -> DataClientException.unwrapThrowable(clusterUrl, e));
}

@Override
Expand Down

This file was deleted.

Loading
Loading