Query the Microsoft Graph API to handle Group overage scenarios in token claims.#55
Conversation
|
|
||
| namespace Octopus.Server.Extensibility.Authentication.AzureAD.GraphApi | ||
| { | ||
| internal class GraphApiClient |
There was a problem hiding this comment.
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, clientId and clientSecret are runtime config.
|
@pstephenson02 Regarding this line: "I chose to leave this alone as well because I could not think of a good way technically to handle storing refresh tokens that would be required to retrieve new access tokens for graph API authorization." Maybe I'm unclear what a refresh token is. But couldn't you use the new Client Key value that was added to the AzureAD Configuration page with this PR (and my fork). The Client Key is the secret key generated in the Azure App Registration to query the Graph API. Or is that configuration value not available where/how one would implement IExternalGroupRetriever? |
|
@StephenShamakian good question. Let me just brain dump here a bit since I was thinking more about this on Friday:
{
"token_type": "Bearer",
"scope": "https://graph.microsoft.com/user.read",
"expires_in": 3269,
"ext_expires_in": 0,
"access_token": "eyJ0eXAiO....",
"refresh_token": "OAQABAAA..."
}These access tokens expire fairly quickly, and so it's my understanding that this refresh token is a convenient way for us to go back and get another access token without the need for the original ID token that came from the login flow. If we don't store the access token and/or refresh tokens, then the only way that we can access the Graph API is by using a client credentials grant (which would be using the client secret you referred to in your comment). So technically this just means that Octopus'es authorization is no longer delegated by the AzureAD user. Ultimately I really don't even know whether this is important or not. The MSDN docs suggest there are a few different ways you can authorize apps for this purpose. But using a delegated flow during login only to subsequently use a client credentials grant feels wrong and just not in the spirit of OAuth. But okay, even if we decide we want to always use delegated authorization, there are more questions. When using the delegated flow, these tokens that come back are unique to the user that allowed access right? I can't access someone else's AzureAD groups with a token from a separate user. So does this mean that we have to be in the business of storing access and/or refresh tokens for all users in Octopus? What's the best way to do that? That would have to be done in shared storage (SQL Server). It doesn't seem like these auth provider extensions were meant for doing something like that effectively. This feels like a dead end. Let's say that we concede then and decide that given the limitations, we want to just go ahead and use a client credentials grant to resync groups every hour (to implement public IResultFromExtension<ExternalGroupResult> Read(IUser user, CancellationToken cancellationToken);The sync job runs once an hour and as you can guess from the method, runs for every Octopus user with an appropriate identity that supports group synchronization. In the case of the on prem Active Directory extension, the software pulls the samAccountName from the user's claims and basically constructs a new In our case with AzureAD, for every user we would need to first go retrieve an access token (there is no way to re-use an access token from a previous request given the current interface definition). Then we'd have to go make a call to the Graph API. For a couple dozen users this may not be a problem, but for hundreds or even thousands of users I think network latency and even Graph API rate limiting could become a real problem. So yea.. I'm not really sure how to solve these problems yet. I hope this helps someone at least when we pick this back up though down the road. |
|
@pstephenson02 Thanks for the brain dump! That helps a ton in understanding some of the limitations of implementing IExternalGroupRetriever when talking to an external provider like AzureAD. I understand the hesitancy on not wanting to use the client secret method for refresh, and use the on be half of method instead. As yes, it is more in the nature of OAuth to do that sort of thing. But you bring up some really good limitations for doing that (especially with a SQL Session State DB). My 2cents on the matter (take it for what you will), I'm looking for solutions more than works of art :) (The phrase don't let "perfect" become the enemy of "good" comes to mind). So if its not elegant I get it, to a point that is. I do hear you on the amount of API calls to refresh those groups for each user. As you would need to at least make one call before the loop to get a token based off of the client key. Then once you have that loop through each user getting their group memberships. If you have 3,000+ users (like we do) then that's 3,000+ Graph API calls every one hour... I can see where that would be a limitation for our size too! And this same API hit rate would still effect the on behalf of auth model as well (at least how its implemented in Octopus right now, a System Server Task) But with all of that said, just to be super clear (I know Phil you and I talked about this already) implementing IExternalGroupRetriever is a bonus not the requirement (at least for the immediate issue). We need the ability to follow the Graph API on user login not on entitlement refresh. So users logging in would see Octopus Project they are meant to see. In fact Jenkins and even GitHub Enterprise have this same limitation where if a membership changes in AzureAD the user wont see those updates until they sign in/out. Now I can't say if GitHub/Jenkins have infinite user session cookies (pretty sure GitHub might...). But the main goal here is getting past the group limit in the OpenID tokens on user login (authentication) and following the Graph API that Microsoft returns to get the groups in the event of an overage. Which is what my fork and this PR accomplishes. Your PR is just much more elegant than my fork :) |
| 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"); |
There was a problem hiding this comment.
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.
Good point! It does look like there may be some differences in URLs. See these references:
https://docs.microsoft.com/en-us/azure/china/resources-developer-guide
https://tipsfordev.com/authenticating-user-to-use-the-graph-api-in-the-german-national-cloud
https://docs.microsoft.com/en-us/graph/deployments
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.
@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):
Error: Unable to load one or more of the requested types. Error: Could not load type 'Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Configuration.OpenIdConnectConfigurationSettings3' from assembly 'Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common, Version=11.1.163.0, Culture=neutral, PublicKeyToken=null'.
Error: Could not load type 'Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Configuration.OpenIdConnectConfigurationWithRoleStore1' from assembly 'Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common, Version=11.1.163.0, Culture=neutral, PublicKeyToken=null'. Error: Could not load type 'Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Configuration.OpenIdConnectConfigureCommands1' from assembly 'Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common, Version=11.1.163.0, Culture=neutral, PublicKeyToken=null'.
Error: System.Reflection.ReflectionTypeLoadException: Unable to load one or more of the requested types.
Error: Could not load type 'Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Configuration.OpenIdConnectConfigurationSettings3' from assembly 'Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common, Version=11.1.163.0, Culture=neutral, PublicKeyToken=null'. Error: Could not load type 'Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Configuration.OpenIdConnectConfigurationWithRoleStore1' from assembly 'Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common, Version=11.1.163.0, Culture=neutral, PublicKeyToken=null'.
Error: Could not load type 'Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Configuration.OpenIdConnectConfigureCommands1' from assembly 'Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common, Version=11.1.163.0, Culture=neutral, PublicKeyToken=null'. Error: at System.Reflection.RuntimeModule.GetTypes(RuntimeModule module) Error: at System.Reflection.Assembly.GetTypes() Error: at Octopus.Core.Startup.PluginsModule.LoadExtension(ContainerBuilder builder, HashSet1 loadedExtensions, Boolean isLoadingCustomExtensions, String file) in ./source/Octopus.Core/Startup/PluginsModule.cs:line 130
Error: at Octopus.Core.Startup.PluginsModule.LoadExtensions(ContainerBuilder builder, String path, HashSet1 loadedExtensions, Boolean isLoadingCustomExtensions) in ./source/Octopus.Core/Startup/PluginsModule.cs:line 107 Error: at Octopus.Core.Startup.PluginsModule.LoadCustomExtensions(ContainerBuilder builder, HashSet1 loadedExtensions) in ./source/Octopus.Core/Startup/PluginsModule.cs:line 61
Error: at Octopus.Core.Startup.PluginsModule.Load(ContainerBuilder builder) in ./source/Octopus.Core/Startup/PluginsModule.cs:line 45
Error: at Autofac.Module.Configure(IComponentRegistry componentRegistry)
Error: at Autofac.ContainerBuilder.Build(IComponentRegistry componentRegistry, Boolean excludeDefaultModules)
Error: at Autofac.ContainerBuilder.Build(ContainerBuildOptions options)
Error: at Octopus.Core.Startup.OctopusServerProgram.RegisterPlugins(ContainerBuilder containerBuilder, ILogFileOnlyLogger logger) in ./source/Octopus.Core/Startup/OctopusServerProgram.cs:line 97
Error: at Octopus.Server.Program.RegisterPlugins(ContainerBuilder containerBuilder, ILogFileOnlyLogger logger) in ./source/Octopus.Server/Program.cs:line 331
Error: at Octopus.Core.Startup.OctopusServerProgram.BuildContainer(StartUpInstanceRequest startupRequest) in ./source/Octopus.Core/Startup/OctopusServerProgram.cs:line 68
Error: at Octopus.Shared.Startup.OctopusProgram.Run() in ./source/Octopus.Shared/Startup/OctopusProgram.cs:line 130
Error: System.TypeLoadException: Could not load type 'Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Configuration.OpenIdConnectConfigurationSettings3' from assembly 'Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common, Version=11.1.163.0, Culture=neutral, PublicKeyToken=null'. Error: System.TypeLoadException: Could not load type 'Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Configuration.OpenIdConnectConfigurationWithRoleStore1' from assembly 'Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common, Version=11.1.163.0, Culture=neutral, PublicKeyToken=null'.
Error: System.TypeLoadException: Could not load type 'Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Configuration.OpenIdConnectConfigureCommands1' from assembly 'Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common, Version=11.1.163.0, Culture=neutral, PublicKeyToken=null'. Error: Could not load type 'Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Configuration.OpenIdConnectConfigurationSettings3' from assembly 'Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common, Version=11.1.163.0, Culture=neutral, PublicKeyToken=null'.
Error: System.TypeLoadException: Could not load type 'Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Configuration.OpenIdConnectConfigurationSettings3' from assembly 'Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common, Version=11.1.163.0, Culture=neutral, PublicKeyToken=null'. Error: Could not load type 'Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Configuration.OpenIdConnectConfigurationWithRoleStore1' from assembly 'Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common, Version=11.1.163.0, Culture=neutral, PublicKeyToken=null'.
Error: System.TypeLoadException: Could not load type 'Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Configuration.OpenIdConnectConfigurationWithRoleStore1' from assembly 'Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common, Version=11.1.163.0, Culture=neutral, PublicKeyToken=null'. Error: Could not load type 'Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Configuration.OpenIdConnectConfigureCommands1' from assembly 'Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common, Version=11.1.163.0, Culture=neutral, PublicKeyToken=null'.
Error: System.TypeLoadException: Could not load type 'Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Configuration.OpenIdConnectConfigureCommands1' from assembly 'Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common, Version=11.1.163.0, Culture=neutral, PublicKeyToken=null'. Process E:\Program Files\Octopus Deploy\Octopus\Octopus.Server.exe in E:\ exited with code 43 Error: The previous command returned a non-zero exit code of: 43 Error: The command that failed was: "E:\Program Files\Octopus Deploy\Octopus\Octopus.Server.exe" database --instance "OctopusServer" --upgrade
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Octopus.Server.Extensibility.Authentication - 11.1.126
Octopus.Server.MessageContracts - 3.0.592
Unfortunately, as 2022.2 is an LTS version, we're not in a position to bump these dependencies there.
There are two options to get you going again -
- Downgrade these versions to the following in
Server.OpenIDConnect.Common.csproj
<PackageReference Include="Octopus.Server.Extensibility" Version="14.3.328" />
<PackageReference Include="Octopus.Server.Extensibility.Authentication" Version="11.1.124" />
...
<PackageReference Include="Octopus.Server.MessageContracts" Version="3.0.566" />
- Update your Update-OctopusReferences.ps1 script to copy
Octopus.Server.ExtensibilityandOctopus.Server.MessageContractsfromOctopusServerBinaryLocation
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.
@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
There was a problem hiding this comment.
That's great news! I'm glad you're able to upgrade 🙂
5 participants
This is a very primitive attempt to solve the problem described in this internal Slack thread.
The purpose of this Draft PR is really to start a conversation. I've worked on this on behalf of one of my Octopus customers and that customer has a vested interest in seeing this feature merged back into the official repo.
A few notes about this implementation:
This solution implements the OAuth 2.0 On Behalf-Of flow described here. However, it does not actually present users a screen to explicitly delegate access to their group membership like you'd normally expect. This was essentially just a compromise because the ability to manage/revoke delegated authorization requests was outside the scope of this change.
In its current form, this query to the Graph API only happens when a user signs in. For companies/orgs with large Azure AD tenants with many groups, this at least gets them over the line to allow their users to actually login using their AAD identities. I spoke with @slewis74 about this, and ideally this AAD extension would also implement
IExternalGroupRetriever, similar to the AD extension. Octopus would then periodically query for groups and keep the User Principal up to date. I chose to leave this alone as well because I could not think of a good way technically to handle storing refresh tokens that would be required to retrieve new access tokens for graph api authorization.Ideally for this, we'd be able to use the Microsoft Graph SDK and Azure.Identity. However, both of these packages have many transitive dependencies (DLLs). Some of those DLLs already come bundled with Octopus Server and the bundled ones are different versions. When I tried including these 2 packages in the Server.AzureAD project and then loading the extension using Octopus Server it would just complain about missing DLLs or types/versions. Maybe someone else knows if this is possible, but in lieu of not being able to include external packages, I just wrote primitive API client code using HttpClient. While it does handle paging when necessary, there is no retry logic in case of transient issues, but users can always just try their login again.
You'll probably notice: The code doesn't have tests. Big TODO here.