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

ServiceBus SDK: Rbac support #6393

Merged
merged 7 commits into from
Jun 12, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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 @@ -992,13 +992,16 @@ private static ITokenProvider CreateTokenProvider(ServiceBusConnectionStringBuil
{
return new SharedAccessSignatureTokenProvider(builder.SasToken);
}

if (builder.SasKeyName != null && builder.SasKey != null)
else if (builder.SasKeyName != null && builder.SasKey != null)
{
return new SharedAccessSignatureTokenProvider(builder.SasKeyName, builder.SasKey);
}
else if (String.Equals(builder.Authentication, ServiceBusConnectionStringBuilder.AuthenticationType.ManagedIdentity, StringComparison.OrdinalIgnoreCase))
{
return new ManagedIdentityTokenProvider();
}

throw new Exception("Could not create token provider. Either ITokenProvider has to be passed into constructor or connection string should contain information such as SAS token / SAS key name and SAS key.");
throw new Exception("Could not create token provider. Either ITokenProvider has to be passed into constructor or connection string should contain SAS token OR SAS key name and SAS key OR Authentication = Managed Identity.");
}

private Task<string> GetToken(Uri requestUri)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,59 +5,28 @@ namespace Microsoft.Azure.ServiceBus.Primitives
{
using System;
using System.Threading.Tasks;
using Microsoft.IdentityModel.Clients.ActiveDirectory;

/// <summary>
/// Represents the Azure Active Directory token provider for the Service Bus.
/// </summary>
public class AzureActiveDirectoryTokenProvider : TokenProvider
{
readonly AuthenticationContext authContext;
readonly ClientCredential clientCredential;
#if !UAP10_0
readonly ClientAssertionCertificate clientAssertionCertificate;
#endif
readonly string clientId;
readonly Uri redirectUri;
readonly IPlatformParameters platformParameters;
readonly UserIdentifier userIdentifier;

enum AuthType
{
ClientCredential,
UserPasswordCredential,
ClientAssertionCertificate,
InteractiveUserLogin
}
/// <summary>
/// Common authority for Azure Active Directory.
/// </summary>
public const string CommonAuthority = "https://login.microsoftonline.com/common";

readonly AuthType authType;
readonly string authority;
readonly object authCallbackState;
event AuthenticationCallback AuthCallback;

internal AzureActiveDirectoryTokenProvider(AuthenticationContext authContext, ClientCredential credential)
{
this.clientCredential = credential;
this.authContext = authContext;
this.authType = AuthType.ClientCredential;
this.clientId = clientCredential.ClientId;
}
public delegate Task<string> AuthenticationCallback(string audience, string authority, object state);

#if !UAP10_0
internal AzureActiveDirectoryTokenProvider(AuthenticationContext authContext, ClientAssertionCertificate clientAssertionCertificate)
public AzureActiveDirectoryTokenProvider(AuthenticationCallback authenticationCallback, string authority, object state)
{
this.clientAssertionCertificate = clientAssertionCertificate;
this.authContext = authContext;
this.authType = AuthType.ClientAssertionCertificate;
this.clientId = clientAssertionCertificate.ClientId;
}
#endif

internal AzureActiveDirectoryTokenProvider(AuthenticationContext authContext, string clientId, Uri redirectUri, IPlatformParameters platformParameters, UserIdentifier userIdentifier)
{
this.authContext = authContext;
this.clientId = clientId;
this.redirectUri = redirectUri;
this.platformParameters = platformParameters;
this.userIdentifier = userIdentifier;
this.authType = AuthType.InteractiveUserLogin;
this.AuthCallback = authenticationCallback ?? throw Fx.Exception.ArgumentNull(nameof(authenticationCallback));
this.authority = authority ?? CommonAuthority;
this.authCallbackState = state;
}

/// <summary>
Expand All @@ -68,29 +37,8 @@ internal AzureActiveDirectoryTokenProvider(AuthenticationContext authContext, st
/// <returns><see cref="SecurityToken"/></returns>
public override async Task<SecurityToken> GetTokenAsync(string appliesTo, TimeSpan timeout)
{
AuthenticationResult authResult;

switch (this.authType)
{
case AuthType.ClientCredential:
authResult = await this.authContext.AcquireTokenAsync(Constants.AadServiceBusAudience, this.clientCredential);
break;

#if !UAP10_0
case AuthType.ClientAssertionCertificate:
authResult = await this.authContext.AcquireTokenAsync(Constants.AadServiceBusAudience, this.clientAssertionCertificate);
break;
#endif

case AuthType.InteractiveUserLogin:
authResult = await this.authContext.AcquireTokenAsync(Constants.AadServiceBusAudience, this.clientId, this.redirectUri, this.platformParameters, this.userIdentifier);
break;

default:
throw new NotSupportedException();
}

return new JsonSecurityToken(authResult.AccessToken, appliesTo);
var tokenString = await this.AuthCallback(appliesTo, this.authority, this.authCallbackState).ConfigureAwait(false);
return new JsonSecurityToken(tokenString, appliesTo);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@ namespace Microsoft.Azure.ServiceBus.Primitives
using Azure.Services.AppAuthentication;

/// <summary>
/// Represents the Azure Active Directory token provider for Azure Managed Service Identity integration.
/// Represents the Azure Active Directory token provider for Azure Managed Identity integration.
/// </summary>
public class ManagedServiceIdentityTokenProvider : TokenProvider
public class ManagedIdentityTokenProvider : TokenProvider
Copy link
Contributor

@nemakam nemakam May 22, 2019

Choose a reason for hiding this comment

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

ManagedIdentityTokenProvider [](start = 17, length = 28)

Do we need this rename? If we rename the public method then it becomes a breaking change.

If there's enough justification for the rename, lets update the major version in the csproj. #Closed

{
static AzureServiceTokenProvider azureServiceTokenProvider = new AzureServiceTokenProvider();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ namespace Microsoft.Azure.ServiceBus.Primitives
{
using System;
using System.Threading.Tasks;
using Microsoft.IdentityModel.Clients.ActiveDirectory;

/// <summary>
/// This abstract base class can be extended to implement additional token providers.
Expand Down Expand Up @@ -74,88 +73,28 @@ public static TokenProvider CreateSharedAccessSignatureTokenProvider(string keyN
return new SharedAccessSignatureTokenProvider(keyName, sharedAccessKey, tokenTimeToLive, tokenScope);
}

/// <summary>Creates an Azure Active Directory token provider.</summary>
/// <param name="authContext">AuthenticationContext for AAD.</param>
/// <param name="clientCredential">The app credential.</param>
/// <returns>The <see cref="TokenProvider" /> for returning Json web token.</returns>
public static TokenProvider CreateAadTokenProvider(AuthenticationContext authContext, ClientCredential clientCredential)
{
if (authContext == null)
{
throw new ArgumentNullException(nameof(authContext));
}

if (clientCredential == null)
{
throw new ArgumentNullException(nameof(clientCredential));
}

return new AzureActiveDirectoryTokenProvider(authContext, clientCredential);
}

/// <summary>Creates an Azure Active Directory token provider.</summary>
/// <param name="authContext">AuthenticationContext for AAD.</param>
/// <param name="clientId">ClientId for AAD.</param>
/// <param name="redirectUri">The redirectUri on Client App.</param>
/// <param name="platformParameters">Platform parameters</param>
/// <param name="userIdentifier">User Identifier</param>
/// <returns>The <see cref="TokenProvider" /> for returning Json web token.</returns>
/// <param name="authCallback">The user defined authentication delegate to provide access token.</param>
/// <param name="authority">URL of the Azure Active Directory instance to issue token.</param>
/// <param name="state">Custom parameters that may be passed into the authentication delegate.</param>
/// <returns>The <see cref="Microsoft.ServiceBus.TokenProvider" /> for returning Json web token.</returns>
public static TokenProvider CreateAadTokenProvider(
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest we consider avoiding the abbreviation here and go with something like CreateAzureActiveDirectoryTokenProvider for clarity. This class and method are public and, therefore, discoverable/usable by consumers directly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at #6393 (comment) wouldn't this change be handled in a separate (follow up) PR, @jsquire?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but I've referred back to this one in the issue that I opened to track, since there are several comments that were not addressed due to time constraints. I wanted to take one last pass and keep thoughts consolidated so that they're all in one place for future consideration.

Copy link
Contributor

@SeanFeldman SeanFeldman Jun 13, 2019

Choose a reason for hiding this comment

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

Ok. Would expect to see these comments in the new PR with a reference to this PR as a starting point. Otherwise it looks like a call to action on the PR that has been approved 🙂

AuthenticationContext authContext,
string clientId,
Uri redirectUri,
IPlatformParameters platformParameters,
UserIdentifier userIdentifier = null)
{
if (authContext == null)
{
throw new ArgumentNullException(nameof(authContext));
}

if (string.IsNullOrEmpty(clientId))
{
throw new ArgumentNullException(nameof(clientId));
}

if (redirectUri == null)
{
throw new ArgumentNullException(nameof(redirectUri));
}

if (platformParameters == null)
{
throw new ArgumentNullException(nameof(platformParameters));
}

return new AzureActiveDirectoryTokenProvider(authContext, clientId, redirectUri, platformParameters, userIdentifier);
}

#if !UAP10_0
/// <summary>Creates an Azure Active Directory token provider.</summary>
/// <param name="authContext">AuthenticationContext for AAD.</param>
/// <param name="clientAssertionCertificate">The client assertion certificate credential.</param>
/// <returns>The <see cref="TokenProvider" /> for returning Json web token.</returns>
public static TokenProvider CreateAadTokenProvider(AuthenticationContext authContext, ClientAssertionCertificate clientAssertionCertificate)
AzureActiveDirectoryTokenProvider.AuthenticationCallback authCallback,
string authority = AzureActiveDirectoryTokenProvider.CommonAuthority,
object state = null)
{
if (authContext == null)
{
throw new ArgumentNullException(nameof(authContext));
}

if (clientAssertionCertificate == null)
if (authCallback == null)
{
throw new ArgumentNullException(nameof(clientAssertionCertificate));
throw new ArgumentNullException(nameof(authCallback));
}

return new AzureActiveDirectoryTokenProvider(authContext, clientAssertionCertificate);
return new AzureActiveDirectoryTokenProvider(authCallback, authority, state);
}
#endif

/// <summary>Creates Azure Managed Service Identity token provider.</summary>
/// <summary>Creates Azure Managed Identity token provider.</summary>
/// <returns>The <see cref="TokenProvider" /> for returning Json web token.</returns>
public static TokenProvider CreateManagedServiceIdentityTokenProvider()
public static TokenProvider CreateManagedIdentityTokenProvider()
{
return new ManagedServiceIdentityTokenProvider();
return new ManagedIdentityTokenProvider();
}

/// <summary>
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions sdk/servicebus/Microsoft.Azure.ServiceBus/src/Resources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -216,4 +216,7 @@
<data name="TimeoutMustBePositiveNonZero" xml:space="preserve">
<value>Argument {0} must be a positive non-zero timeout value. The provided value was {1}.</value>
</data>
<data name="ArgumentInvalidCombination" xml:space="preserve">
<value>The following arguments must all be provided or none at all: {0}.</value>
</data>
</root>
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,10 @@ void InitializeConnection(ServiceBusConnectionStringBuilder builder)
{
this.TokenProvider = new SharedAccessSignatureTokenProvider(builder.SasKeyName, builder.SasKey);
}
else if (String.Equals(builder.Authentication, ServiceBusConnectionStringBuilder.AuthenticationType.ManagedIdentity, StringComparison.OrdinalIgnoreCase))
{
this.TokenProvider = new ManagedIdentityTokenProvider();
}

this.OperationTimeout = builder.OperationTimeout;
this.TransportType = builder.TransportType;
Expand Down
Loading