-
Notifications
You must be signed in to change notification settings - Fork 1
Query the Microsoft Graph API to handle Group overage scenarios in token claims. #55
base: main
Are you sure you want to change the base?
Changes from all commits
7249022
9d3fe02
24ac883
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,11 @@ | ||
| using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Configuration; | ||
| using Octopus.Data.Model; | ||
| using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Configuration; | ||
|
|
||
| namespace Octopus.Server.Extensibility.Authentication.AzureAD.Configuration | ||
| { | ||
| interface IAzureADConfigurationStore : IOpenIDConnectConfigurationWithRoleStore<AzureADConfiguration> | ||
| { | ||
| SensitiveString? GetClientSecret(); | ||
| void SetClientSecret(SensitiveString? clientSecret); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| using Newtonsoft.Json; | ||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Linq; | ||
| using System.Net.Http; | ||
| using System.Net.Http.Headers; | ||
| using System.Threading.Tasks; | ||
|
|
||
| namespace Octopus.Server.Extensibility.Authentication.AzureAD.GraphApi | ||
| { | ||
| internal class GraphApiClient | ||
| { | ||
| private readonly HttpClient httpClient; | ||
| private readonly Uri tokenUri; | ||
| private readonly Guid clientId; | ||
| private readonly string clientSecret; | ||
|
|
||
| private const string scope = "https://graph.microsoft.com/groupmember.read.all"; | ||
| private const string grantType = "urn:ietf:params:oauth:grant-type:jwt-bearer"; | ||
| private const string requestedTokenUse = "on_behalf_of"; | ||
| private const string graphQuerySelect = "$select=id,displayName,onPremisesNetBiosName,onPremisesDomainName,onPremisesSamAccountNameonPremisesSecurityIdentifier"; | ||
|
|
||
| public GraphApiClient(HttpClient httpClient, Guid tenantId, Guid clientId, string? clientSecret) | ||
| { | ||
| this.httpClient = httpClient; | ||
| tokenUri = new Uri("https://login.microsoftonline.com/" + tenantId.ToString() + "/oauth2/v2.0/token"); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Be careful here, we've been caught before. Some of these URLs can change per AzureEnvironment (Germany, China, US Gov etc). Graph API may be global and you don't have to worry about it, but it'd be worth checking. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point! It does look like there may be some differences in URLs. See these references: Is there a way you guys handled that in the past? Maybe a configuration dropdown in the AzureAD Configuration Settings page? As this implementation also adds an Azure App Registration key to that page as well. In fact if that key is missing this code in theory shouldn't run (which makes this code invisible to users that may not run into this issue). At least that's what I did in my fork of this repo: https://github.com/StephenShamakian/OpenIDConnectAuthenticationProviders/blob/b0d237b40be9a3ec8e9bcd1ccd9e0942a4c62a19/source/Server.AzureAD/Tokens/AzureADAuthTokenHandler.cs#L212 Another point to mention; which only really applies to the graph endpoint as I didn't think about differences in the login.microsoftonline.com endpoint. The only reason we are hard coding the graph endpoint is it seems like Microsoft's own developers didn't get the memo that https://graph.windows.net will be out of support soon. As the JWT token (returned by Microsoft) will return the exact endpoint you need if there is a group overage in the token. But it returns it as windows.net vs. microsoft.com. Now I haven't tested this, but maybe the endpoint the JWT token returns is the correct value for German, China & Government versions of windows.com. But that still leaves the problem on relying on an old endpoint. I made note of this in my readme on my fork as well: https://github.com/StephenShamakian/OpenIDConnectAuthenticationProviders#microsoft-graphapi-vs-windows-graphapi FYI @pstephenson02 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @slewis74 Any additional thoughts or ETA in adding Graph API lookups for AzureAD group overages? We just tried to perform an Octopus upgrade from v2021.3.12132 in Dev but were greeted to this error after copying over the necessary .dll to make our modified AzureAD authentication provider work I think I'm going to need to update my fork (https://github.com/StephenShamakian/OpenIDConnectAuthenticationProviders):
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @slewis74 I'm really struggling here... I can't seem to get the self-compiled .dlls to match the same version of the .dlls on the Octopus server for v2022.2.7462. Every time I re-compile my fork'ed repo with my modifications plus the latest merged modifications from Octopus I get dll versions of v11.1.7.0. But the Octopus dll versions on the server after upgrading to v2022.2.7462 are at v11.1.163.0 At this point in time we can never upgrade our Octopus Server instances as users will no longer be able to login. That's not good.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @StephenShamakian, I'm one of the engineers of the team that has ownership of the authentication providers. I'm actively looking into this issue, and I will reach out soon once I have a fix.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @StephenShamakian, I think I found the issue, we seem to have had a breaking change here and also versions for dependencies are higher than what is in Octopus Server 2022.2. I see that you've updated your fork 👍 So currently, the challenge is these dependencies versions, on the provider, the versions are as follows Octopus.Server.Extensibility - 14.3.338 Unfortunately, as 2022.2 is an LTS version, we're not in a position to bump these dependencies there.
The good news going forward is that we intend to try and get the changes in this PR into the provider as soon as possible, so you won't have to manage your own fork and deal with issues like this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ankithkonda Thank you so much!!! I went with option one (the special combination of package versions) to get this working again! I really appreciate it! I look forward to the official release of this functionality in the future! :) Also between the weekend and now v2022.2.7580 came out. I also tested upgrading to that version instead of v2022.2.7462 and it still worked! So were going to move ahead with our Prod instance in the next week or so using v2022.2.7580
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's great news! I'm glad you're able to upgrade 🙂 |
||
| this.clientId = clientId; | ||
| this.clientSecret = clientSecret ?? throw new ArgumentNullException(nameof(clientSecret)); | ||
| } | ||
|
|
||
| public async Task<string> GetAccessTokenOnBehalfOfUser(string assertion) | ||
| { | ||
| var requestBody = new FormUrlEncodedContent(new[] | ||
| { | ||
| new KeyValuePair<string, string>("grant_type", grantType), | ||
| new KeyValuePair<string, string>("client_id", clientId.ToString()), | ||
| new KeyValuePair<string, string>("client_secret", clientSecret), | ||
| new KeyValuePair<string, string>("assertion", assertion), | ||
| new KeyValuePair<string, string>("scope", scope), | ||
| new KeyValuePair<string, string>("requested_token_use", requestedTokenUse) | ||
| }); | ||
|
|
||
| var response = await httpClient.PostAsync(tokenUri, requestBody); | ||
| response.EnsureSuccessStatusCode(); | ||
| var responseBody = await response.Content.ReadAsStringAsync(); | ||
| var model = JsonConvert.DeserializeObject<TokenResponse>(responseBody); | ||
|
|
||
| return model.AccessToken; | ||
| } | ||
|
|
||
| public async Task<string[]> GetGroupMembershipIds(string accessToken) | ||
| { | ||
| var groups = new HashSet<string>(); | ||
| string? nextLink = null; | ||
| do | ||
| { | ||
| var uri = string.IsNullOrEmpty(nextLink) ? ("https://graph.microsoft.com/v1.0/me/memberOf/microsoft.graph.group?" + graphQuerySelect) : nextLink; | ||
| // The nextLink will contain all other query parameters from original request: https://docs.microsoft.com/en-us/graph/paging?context=graph%2Fapi%2F1.0&view=graph-rest-1.0 | ||
| using (var request = new HttpRequestMessage(HttpMethod.Get, uri)) | ||
| { | ||
| request.Headers.Authorization = new AuthenticationHeaderValue("Bearer", accessToken); | ||
|
|
||
| var response = await httpClient.SendAsync(request); | ||
| response.EnsureSuccessStatusCode(); | ||
| var responseBody = await response.Content.ReadAsStringAsync(); | ||
| var graphObjects = JsonConvert.DeserializeObject<GraphResponse>(responseBody); | ||
| nextLink = graphObjects.NextLink; | ||
|
|
||
| groups.UnionWith(graphObjects.Value.Select(m => m.Id)); | ||
| } | ||
| } while (!string.IsNullOrEmpty(nextLink)); | ||
|
|
||
| return groups.ToArray(); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| using Newtonsoft.Json; | ||
|
|
||
| namespace Octopus.Server.Extensibility.Authentication.AzureAD.GraphApi | ||
| { | ||
| internal class GraphResponse | ||
| { | ||
| [JsonProperty("@odata.context")] | ||
| public string? Context { get; set; } | ||
| [JsonProperty("@odata.nextLink")] | ||
| public string? NextLink { get; set; } | ||
| [JsonProperty("value")] | ||
| public MembershipEntity[]? Value { get; set; } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| using Newtonsoft.Json; | ||
| using System; | ||
|
|
||
| namespace Octopus.Server.Extensibility.Authentication.AzureAD.GraphApi | ||
| { | ||
| internal class MembershipEntity | ||
| { | ||
| [JsonProperty("id")] | ||
| public string Id { get; set; } = string.Empty; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| using Newtonsoft.Json; | ||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Text; | ||
|
|
||
| namespace Octopus.Server.Extensibility.Authentication.AzureAD.GraphApi | ||
| { | ||
| internal class TokenResponse | ||
| { | ||
| [JsonProperty("token_type")] | ||
| public string TokenType { get; set; } = string.Empty; | ||
| [JsonProperty("scope")] | ||
| public string Scope { get; set; } = string.Empty; | ||
| [JsonProperty("expires_in")] | ||
| public int ExpiresIn { get; set; } | ||
| [JsonProperty("ext_expires_in")] | ||
| public int ExtExpiresIn { get; set; } | ||
| [JsonProperty("access_token")] | ||
| public string AccessToken { get; set; } = string.Empty; | ||
| [JsonProperty("refresh_token")] | ||
| public string RefreshToken { get; set; } = string.Empty; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,15 +1,55 @@ | ||
| using Octopus.Diagnostics; | ||
| using Octopus.Server.Extensibility.Authentication.AzureAD.Configuration; | ||
| using Octopus.Server.Extensibility.Authentication.AzureAD.GraphApi; | ||
| using Octopus.Server.Extensibility.Authentication.AzureAD.Issuer; | ||
| using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Issuer; | ||
| using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Tokens; | ||
| using System; | ||
| using System.Linq; | ||
| using System.Net.Http; | ||
| using System.Security.Claims; | ||
| using System.Threading.Tasks; | ||
|
|
||
| namespace Octopus.Server.Extensibility.Authentication.AzureAD.Tokens | ||
| { | ||
| class AzureADAuthTokenHandler : OpenIDConnectAuthTokenWithRolesHandler<IAzureADConfigurationStore, IAzureADKeyRetriever, IIdentityProviderConfigDiscoverer>, IAzureADAuthTokenHandler | ||
| { | ||
| private readonly IAzureADConfigurationStore configurationStore; | ||
|
|
||
| public AzureADAuthTokenHandler(ISystemLog log, IAzureADConfigurationStore configurationStore, IIdentityProviderConfigDiscoverer identityProviderConfigDiscoverer, IAzureADKeyRetriever keyRetriever) : base(log, configurationStore, identityProviderConfigDiscoverer, keyRetriever) | ||
| { | ||
| this.configurationStore = configurationStore; | ||
| } | ||
|
|
||
| protected override string[] GetProviderGroupIds(ClaimsPrincipal principal, string? assertion = null) | ||
| => (SupportsHandlingOverages() && HasOverageOccurred(principal)) | ||
| ? GetProviderGroupIdsAsync(principal, assertion).Result | ||
| : base.GetProviderGroupIds(principal); | ||
|
|
||
| private async Task<string[]> GetProviderGroupIdsAsync(ClaimsPrincipal principal, string? idToken) | ||
| { | ||
| using (var httpClient = new HttpClient()) | ||
| { | ||
| var graphClient = new GraphApiClient( | ||
| httpClient, | ||
| GetTenantIdFromIssuer(configurationStore.GetIssuer()), | ||
| Guid.Parse(configurationStore.GetClientId()), | ||
| configurationStore.GetClientSecret()?.Value | ||
| ); | ||
|
|
||
| var bearerToken = await graphClient.GetAccessTokenOnBehalfOfUser(idToken!); | ||
| return await graphClient.GetGroupMembershipIds(bearerToken); | ||
| } | ||
| } | ||
|
|
||
| private bool SupportsHandlingOverages() => !string.IsNullOrEmpty(configurationStore.GetClientSecret()?.Value); | ||
|
|
||
| private static bool HasOverageOccurred(ClaimsPrincipal identity) => identity.Claims.Any(x => x.Type == "hasgroups" || (x.Type == "_claim_names" && x.Value == "{\"groups\":\"src1\"}")); | ||
|
|
||
| private static Guid GetTenantIdFromIssuer(string? issuer) | ||
| { | ||
| var uri = new Uri(issuer); | ||
| return Guid.Parse(uri.Segments.Last()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should arguably be its own project/package, but see my comment on the PR about not knowing how to include other DLLs
Also, I'm not very familiar with Autofac, and I couldn't figure out a good way to register this type such that I could use constructor dependency injection. Especially since the parameters
tenantId,clientIdandclientSecretare runtime config.