From 2b87d22f6c57ce84fa80ceca75ba179355bd23c7 Mon Sep 17 00:00:00 2001 From: Nelson Susanto Date: Thu, 17 Mar 2022 09:19:32 +1100 Subject: [PATCH 01/27] Add very basic pkce endpoints and related actions on google auth --- source/Server.GoogleApps/GoogleAppsApi.cs | 2 + .../Server.GoogleApps/GoogleAppsExtension.cs | 2 + ...ogleAppsAuthorizationEndpointUrlBuilder.cs | 4 +- .../GoogleAppsUserAuthenticatedPkceAction.cs | 33 +++ .../GoogleAppsUserAuthenticationPkceAction.cs | 24 ++ .../OpenIDConnectConfiguration.cs | 3 +- .../Issuer/AuthorizationEndpointUrlBuilder.cs | 14 +- .../IAuthorizationEndpointUrlBuilder.cs | 2 +- .../OpenIDConnectAuthenticationProvider.cs | 2 + .../Server.OpenIDConnect.Common.csproj | 4 +- .../Web/UserAuthenticatedPkceAction.cs | 274 ++++++++++++++++++ .../Web/UserAuthenticationPkceAction.cs | 94 ++++++ 12 files changed, 448 insertions(+), 10 deletions(-) create mode 100644 source/Server.GoogleApps/Web/GoogleAppsUserAuthenticatedPkceAction.cs create mode 100644 source/Server.GoogleApps/Web/GoogleAppsUserAuthenticationPkceAction.cs create mode 100644 source/Server.OpenIDConnect.Common/Web/UserAuthenticatedPkceAction.cs create mode 100644 source/Server.OpenIDConnect.Common/Web/UserAuthenticationPkceAction.cs diff --git a/source/Server.GoogleApps/GoogleAppsApi.cs b/source/Server.GoogleApps/GoogleAppsApi.cs index 5b54bef..a9e62d1 100644 --- a/source/Server.GoogleApps/GoogleAppsApi.cs +++ b/source/Server.GoogleApps/GoogleAppsApi.cs @@ -14,8 +14,10 @@ public GoogleAppsApi( IGoogleAppsConfigurationStore configurationStore, GoogleAppsAuthenticationProvider authenticationProvider) : base(configurationStore, authenticationProvider) { + Add("POST", authenticationProvider.AuthenticatePkceUri, RouteCategory.Raw, new AnonymousWhenEnabledEndpointInvocation(), null, "OpenIDConnect"); Add("POST", authenticationProvider.AuthenticateUri, RouteCategory.Raw, new AnonymousWhenEnabledEndpointInvocation(), null, "OpenIDConnect"); Add("POST", configurationStore.RedirectUri, RouteCategory.Raw, new AnonymousWhenEnabledEndpointInvocation(), null, "OpenIDConnect"); + Add("GET", configurationStore.RedirectUri, RouteCategory.Raw, new AnonymousWhenEnabledEndpointInvocation(), null, "OpenIDConnect"); } } } \ No newline at end of file diff --git a/source/Server.GoogleApps/GoogleAppsExtension.cs b/source/Server.GoogleApps/GoogleAppsExtension.cs index 8c1cbd1..82f6c85 100644 --- a/source/Server.GoogleApps/GoogleAppsExtension.cs +++ b/source/Server.GoogleApps/GoogleAppsExtension.cs @@ -57,7 +57,9 @@ public override void Load(ContainerBuilder builder) builder.RegisterType().As().SingleInstance(); builder.RegisterType().As().SingleInstance(); + builder.RegisterType().AsSelf().InstancePerDependency(); builder.RegisterType().AsSelf().InstancePerDependency(); + builder.RegisterType().AsSelf().InstancePerDependency(); builder.RegisterType().AsSelf().InstancePerDependency(); builder.RegisterType() diff --git a/source/Server.GoogleApps/Issuer/GoogleAppsAuthorizationEndpointUrlBuilder.cs b/source/Server.GoogleApps/Issuer/GoogleAppsAuthorizationEndpointUrlBuilder.cs index cbca793..a71d3ff 100644 --- a/source/Server.GoogleApps/Issuer/GoogleAppsAuthorizationEndpointUrlBuilder.cs +++ b/source/Server.GoogleApps/Issuer/GoogleAppsAuthorizationEndpointUrlBuilder.cs @@ -11,9 +11,9 @@ public GoogleAppsAuthorizationEndpointUrlBuilder(IGoogleAppsConfigurationStore c { } - public override string Build(string requestDirectoryPath, IssuerConfiguration issuerConfiguration, string nonce, string? state = null) + public override string Build(string requestDirectoryPath, IssuerConfiguration issuerConfiguration, string? nonce = null, string? state = null, bool pkce = false) { - var url = base.Build(requestDirectoryPath, issuerConfiguration, nonce, state); + var url = base.Build(requestDirectoryPath, issuerConfiguration, nonce, state, pkce); var hd = ConfigurationStore.GetHostedDomain(); if (!string.IsNullOrWhiteSpace(hd)) diff --git a/source/Server.GoogleApps/Web/GoogleAppsUserAuthenticatedPkceAction.cs b/source/Server.GoogleApps/Web/GoogleAppsUserAuthenticatedPkceAction.cs new file mode 100644 index 0000000..3a61558 --- /dev/null +++ b/source/Server.GoogleApps/Web/GoogleAppsUserAuthenticatedPkceAction.cs @@ -0,0 +1,33 @@ +using Octopus.Diagnostics; +using Octopus.Server.Extensibility.Authentication.GoogleApps.Configuration; +using Octopus.Server.Extensibility.Authentication.GoogleApps.Identities; +using Octopus.Server.Extensibility.Authentication.GoogleApps.Tokens; +using Octopus.Server.Extensibility.Authentication.HostServices; +using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Infrastructure; +using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Web; +using Octopus.Server.Extensibility.HostServices.Web; +using Octopus.Time; + +namespace Octopus.Server.Extensibility.Authentication.GoogleApps.Web +{ + class GoogleAppsUserAuthenticatedPkceAction + : UserAuthenticatedPkceAction + { + public GoogleAppsUserAuthenticatedPkceAction( + ISystemLog log, + IGoogleAuthTokenHandler authTokenHandler, + IPrincipalToUserResourceMapper principalToUserResourceMapper, + IUpdateableUserStore userStore, + IGoogleAppsConfigurationStore configurationStore, + IAuthCookieCreator authCookieCreator, + IInvalidLoginTracker loginTracker, + ISleep sleep, + IGoogleAppsIdentityCreator identityCreator, + IClock clock, IUrlEncoder encoder) + : base(log, authTokenHandler, principalToUserResourceMapper, userStore, configurationStore, authCookieCreator, loginTracker, sleep, identityCreator, clock, encoder) + { + } + + protected override string ProviderName => GoogleAppsAuthenticationProvider.ProviderName; + } +} \ No newline at end of file diff --git a/source/Server.GoogleApps/Web/GoogleAppsUserAuthenticationPkceAction.cs b/source/Server.GoogleApps/Web/GoogleAppsUserAuthenticationPkceAction.cs new file mode 100644 index 0000000..7d7b27d --- /dev/null +++ b/source/Server.GoogleApps/Web/GoogleAppsUserAuthenticationPkceAction.cs @@ -0,0 +1,24 @@ +using Octopus.Diagnostics; +using Octopus.Server.Extensibility.Authentication.GoogleApps.Configuration; +using Octopus.Server.Extensibility.Authentication.GoogleApps.Issuer; +using Octopus.Server.Extensibility.Authentication.HostServices; +using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Issuer; +using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Web; +using Octopus.Server.Extensibility.Extensions.Infrastructure.Web.Api; + +namespace Octopus.Server.Extensibility.Authentication.GoogleApps.Web +{ + class GoogleAppsUserAuthenticationPkceAction : UserAuthenticationPkceAction + { + public GoogleAppsUserAuthenticationPkceAction( + ISystemLog log, + IGoogleAppsConfigurationStore configurationStore, + IIdentityProviderConfigDiscoverer identityProviderConfigDiscoverer, + IGoogleAppsAuthorizationEndpointUrlBuilder urlBuilder, + IApiActionModelBinder modelBinder, + IAuthenticationConfigurationStore authenticationConfigurationStore) + : base(log, configurationStore, identityProviderConfigDiscoverer, urlBuilder, modelBinder, authenticationConfigurationStore) + { + } + } +} \ No newline at end of file diff --git a/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfiguration.cs b/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfiguration.cs index d3dce86..1b90c9e 100644 --- a/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfiguration.cs +++ b/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfiguration.cs @@ -4,7 +4,8 @@ namespace Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Confi { public abstract class OpenIDConnectConfiguration : ExtensionConfigurationDocument, IOpenIDConnectConfiguration { - public const string DefaultResponseType = "code+id_token"; + public const string AuthCodeResponseType = "code"; + public const string HybridResponseType = "code+id_token"; public const string DefaultResponseMode = "form_post"; public const string DefaultScope = "openid%20profile%20email"; public const string DefaultNameClaimType = "name"; diff --git a/source/Server.OpenIDConnect.Common/Issuer/AuthorizationEndpointUrlBuilder.cs b/source/Server.OpenIDConnect.Common/Issuer/AuthorizationEndpointUrlBuilder.cs index 4d4c1fa..39b4142 100644 --- a/source/Server.OpenIDConnect.Common/Issuer/AuthorizationEndpointUrlBuilder.cs +++ b/source/Server.OpenIDConnect.Common/Issuer/AuthorizationEndpointUrlBuilder.cs @@ -16,10 +16,11 @@ protected AuthorizationEndpointUrlBuilder(TStore configurationStore, IUrlEncoder this.urlEncoder = urlEncoder; } - protected virtual string ResponseType => OpenIDConnectConfiguration.DefaultResponseType; + protected virtual string ResponseType => OpenIDConnectConfiguration.HybridResponseType; + protected virtual string PkceResponseType => OpenIDConnectConfiguration.AuthCodeResponseType; protected virtual string ResponseMode => OpenIDConnectConfiguration.DefaultResponseMode; - public virtual string Build(string requestDirectoryPath, IssuerConfiguration issuerConfiguration, string nonce, string? state = null) + public virtual string Build(string requestDirectoryPath, IssuerConfiguration issuerConfiguration, string? nonce = null, string? state = null, bool pkce = false) { if (issuerConfiguration == null) throw new ArgumentException("issuerConfiguration is required", nameof(issuerConfiguration)); @@ -27,17 +28,22 @@ public virtual string Build(string requestDirectoryPath, IssuerConfiguration iss var issuerEndpoint = issuerConfiguration.AuthorizationEndpoint; var clientId = ConfigurationStore.GetClientId(); var scope = ConfigurationStore.GetScope(); - var responseType = ResponseType; + var responseType = pkce ? PkceResponseType : ResponseType; var responseMode = ResponseMode; var redirectUri = requestDirectoryPath.Trim('/') + ConfigurationStore.RedirectUri; - var url = $"{issuerEndpoint}?client_id={clientId}&scope={scope}&response_type={responseType}&response_mode={responseMode}&nonce={nonce}&redirect_uri={redirectUri}"; + var url = $"{issuerEndpoint}?client_id={clientId}&scope={scope}&response_type={responseType}&response_mode={responseMode}&redirect_uri={redirectUri}"; if (!string.IsNullOrWhiteSpace(state)) { url += $"&state={urlEncoder.UrlEncode(state)}"; } + if (!string.IsNullOrWhiteSpace(nonce)) + { + url += $"&nonce={nonce}"; + } + return url; } } diff --git a/source/Server.OpenIDConnect.Common/Issuer/IAuthorizationEndpointUrlBuilder.cs b/source/Server.OpenIDConnect.Common/Issuer/IAuthorizationEndpointUrlBuilder.cs index 81404ae..bdc88c0 100644 --- a/source/Server.OpenIDConnect.Common/Issuer/IAuthorizationEndpointUrlBuilder.cs +++ b/source/Server.OpenIDConnect.Common/Issuer/IAuthorizationEndpointUrlBuilder.cs @@ -2,6 +2,6 @@ { public interface IAuthorizationEndpointUrlBuilder { - string Build(string requestDirectoryPath, IssuerConfiguration issuerConfiguration, string nonce, string? state = null); + string Build(string requestDirectoryPath, IssuerConfiguration issuerConfiguration, string? nonce = null, string? state = null, bool pkce = false); } } \ No newline at end of file diff --git a/source/Server.OpenIDConnect.Common/OpenIDConnectAuthenticationProvider.cs b/source/Server.OpenIDConnect.Common/OpenIDConnectAuthenticationProvider.cs index 27cac18..76bc446 100644 --- a/source/Server.OpenIDConnect.Common/OpenIDConnectAuthenticationProvider.cs +++ b/source/Server.OpenIDConnect.Common/OpenIDConnectAuthenticationProvider.cs @@ -48,6 +48,8 @@ bool IsProviderConfigComplete() public string AuthenticateUri => $"/users/authenticate/{ConfigurationStore.ConfigurationSettingsName}"; + public string AuthenticatePkceUri => $"/users/authenticate-pkce/{ConfigurationStore.ConfigurationSettingsName}"; + public AuthenticationProviderElement GetAuthenticationProviderElement() { var authenticationProviderElement = new AuthenticationProviderElement diff --git a/source/Server.OpenIDConnect.Common/Server.OpenIDConnect.Common.csproj b/source/Server.OpenIDConnect.Common/Server.OpenIDConnect.Common.csproj index 6c779fe..5eadb42 100644 --- a/source/Server.OpenIDConnect.Common/Server.OpenIDConnect.Common.csproj +++ b/source/Server.OpenIDConnect.Common/Server.OpenIDConnect.Common.csproj @@ -26,7 +26,7 @@ - - + + \ No newline at end of file diff --git a/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedPkceAction.cs b/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedPkceAction.cs new file mode 100644 index 0000000..10196f6 --- /dev/null +++ b/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedPkceAction.cs @@ -0,0 +1,274 @@ +using System; +using System.Collections.Generic; +using System.Globalization; +using System.Linq; +using System.Net.Http; +using System.Threading; +using System.Threading.Tasks; +using Newtonsoft.Json; +using Octopus.Data; +using Octopus.Data.Model.User; +using Octopus.Data.Storage.User; +using Octopus.Diagnostics; +using Octopus.Server.Extensibility.Authentication.HostServices; +using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Configuration; +using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Identities; +using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Infrastructure; +using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Tokens; +using Octopus.Server.Extensibility.Authentication.Resources; +using Octopus.Server.Extensibility.Authentication.Resources.Identities; +using Octopus.Server.Extensibility.Extensions.Infrastructure.Web.Api; +using Octopus.Server.Extensibility.HostServices.Web; +using Octopus.Server.Extensibility.Results; +using Octopus.Time; + +namespace Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Web +{ + public abstract class UserAuthenticatedPkceAction : IAsyncApiAction + where TStore : IOpenIDConnectConfigurationStore + where TAuthTokenHandler : IAuthTokenHandler + where TIdentityCreator : IIdentityCreator + { + static readonly BadRequestRegistration LoginFailed = new BadRequestRegistration("User login failed"); + static readonly RedirectRegistration Redirect = new RedirectRegistration("Redirects back to the Octopus portal"); + + static readonly RequiredQueryParameterProperty Code = new("code", "Authorization code provided by the identity provider"); + static readonly OptionalQueryParameterProperty State = new("state", "The state value associated with the authentication session"); + + readonly ISystemLog log; + readonly TAuthTokenHandler authTokenHandler; + readonly IPrincipalToUserResourceMapper principalToUserResourceMapper; + readonly IUpdateableUserStore userStore; + readonly IAuthCookieCreator authCookieCreator; + readonly IInvalidLoginTracker loginTracker; + readonly ISleep sleep; + readonly TIdentityCreator identityCreator; + readonly IClock clock; + readonly IUrlEncoder encoder; + + protected readonly TStore ConfigurationStore; + + protected UserAuthenticatedPkceAction( + ISystemLog log, + TAuthTokenHandler authTokenHandler, + IPrincipalToUserResourceMapper principalToUserResourceMapper, + IUpdateableUserStore userStore, + TStore configurationStore, + IAuthCookieCreator authCookieCreator, + IInvalidLoginTracker loginTracker, + ISleep sleep, + TIdentityCreator identityCreator, + IClock clock, + IUrlEncoder encoder) + { + this.log = log; + this.authTokenHandler = authTokenHandler; + this.principalToUserResourceMapper = principalToUserResourceMapper; + this.userStore = userStore; + ConfigurationStore = configurationStore; + this.authCookieCreator = authCookieCreator; + this.loginTracker = loginTracker; + this.sleep = sleep; + this.identityCreator = identityCreator; + this.clock = clock; + this.encoder = encoder; + } + + protected abstract string ProviderName { get; } + + public async Task ExecuteAsync(IOctoRequest request) + { + return await request.HandleAsync(Code, State, (code, state) => Handle(code, state, request)); + } + + async Task> RequestAuthToken(string code, string redirectUri) + { + var issuerConfig = await identityProviderConfigDiscoverer.GetConfigurationAsync(ConfigurationStore.GetIssuer() ?? string.Empty); + using var client = new HttpClient(); + var request = new HttpRequestMessage(HttpMethod.Post, issuerConfig.TokenEndpoint); + var formValues = new Dictionary + { + ["grant_type"] = "authorization_code", + ["code"] = code, + ["redirect_uri"] = redirectUri, + ["client_id"] = ConfigurationStore.GetClientId()!, + ["client_secret"] = ConfigurationStore.GetClientSecret()!.Value + }; + request.Content = new FormUrlEncodedContent(formValues); + var response = await client.SendAsync(request, HttpCompletionOption.ResponseContentRead); + var body = await response.Content.ReadAsStringAsync(); + return JsonConvert.DeserializeObject>(body); + } + + async Task Handle(string code, string state, IOctoRequest request) + { + // Step 1: Try and get all of the details from the request making sure there are no errors passed back from the external identity provider + var principalContainer = await authTokenHandler.GetPrincipalAsync(request.Form.ToDictionary(pair => pair.Key, pair => (string?)pair.Value), out var stateStringFromRequest); + var principal = principalContainer.Principal; + if (principal == null || !string.IsNullOrEmpty(principalContainer.Error)) + { + return BadRequest($"The response from the external identity provider contained an error: {principalContainer.Error}"); + } + + // Step 2: Validate the state object we passed wasn't tampered with + const string stateDescription = "As a security precaution, Octopus ensures the state object returned from the external identity provider matches what it expected."; + var expectedStateHash = string.Empty; + if (request.Cookies.ContainsKey(UserAuthConstants.OctopusStateCookieName)) + expectedStateHash = encoder.UrlDecode(request.Cookies[UserAuthConstants.OctopusStateCookieName]); + if (string.IsNullOrWhiteSpace(expectedStateHash)) + { + return BadRequest($"User login failed: Missing State Hash Cookie. {stateDescription} In this case the Cookie containing the SHA256 hash of the state object is missing from the request."); + } + + var stateFromRequestHash = State.Protect(stateStringFromRequest); + if (stateFromRequestHash != expectedStateHash) + { + return BadRequest($"User login failed: Tampered State. {stateDescription} In this case the state object looks like it has been tampered with. The state object is '{stateStringFromRequest}'. The SHA256 hash of the state was expected to be '{expectedStateHash}' but was '{stateFromRequestHash}'."); + } + + var stateFromRequest = JsonConvert.DeserializeObject(stateStringFromRequest ?? string.Empty); + + // Step 3: Validate the nonce is as we expected to prevent replay attacks + const string nonceDescription = "As a security precaution to prevent replay attacks, Octopus ensures the nonce returned in the claims from the external identity provider matches what it expected."; + + var expectedNonceHash = string.Empty; + if (request.Cookies.ContainsKey(UserAuthConstants.OctopusNonceCookieName)) + expectedNonceHash = encoder.UrlDecode(request.Cookies[UserAuthConstants.OctopusNonceCookieName]); + + if (string.IsNullOrWhiteSpace(expectedNonceHash)) + { + return BadRequest($"User login failed: Missing Nonce Hash Cookie. {nonceDescription} In this case the Cookie containing the SHA256 hash of the nonce is missing from the request."); + } + + var nonceFromClaims = principal.Claims.FirstOrDefault(c => c.Type == "nonce"); + if (nonceFromClaims == null) + { + return BadRequest($"User login failed: Missing Nonce Claim. {nonceDescription} In this case the 'nonce' claim is missing from the security token."); + } + + var nonceFromClaimsHash = Nonce.Protect(nonceFromClaims.Value); + if (nonceFromClaimsHash != expectedNonceHash) + { + return BadRequest($"User login failed: Tampered Nonce. {nonceDescription} In this case the nonce looks like it has been tampered with or reused. The nonce is '{nonceFromClaims}'. The SHA256 hash of the state was expected to be '{expectedNonceHash}' but was '{nonceFromClaimsHash}'."); + } + + // Step 4: Now the integrity of the request has been validated we can figure out which Octopus User this represents + var authenticationCandidate = principalToUserResourceMapper.MapToUserResource(principal); + if (authenticationCandidate.Username == null) + { + return BadRequest("Unable to determine username."); + } + + // Step 4a: Check if this authentication attempt is already being banned + var action = loginTracker.BeforeAttempt(authenticationCandidate.Username, request.Host); + if (action == InvalidLoginAction.Ban) + { + return BadRequest("You have had too many failed login attempts in a short period of time. Please try again later."); + } + + using (var cts = new CancellationTokenSource(TimeSpan.FromMinutes(1))) + { + // Step 4b: Try to get or create a the Octopus User this external identity represents + var userResult = GetOrCreateUser(authenticationCandidate, principalContainer.ExternalGroupIds, cts.Token); + if (userResult is ISuccessResult successResult) + { + loginTracker.RecordSucess(authenticationCandidate.Username, request.Host); + + if (!successResult.Value.IsActive) + { + return BadRequest($"The Octopus User Account '{authenticationCandidate.Username}' has been disabled by an Administrator. If you believe this to be a mistake, please contact your Octopus Administrator to have your account re-enabled."); + } + + if (successResult.Value.IsService) + { + return BadRequest($"The Octopus User Account '{authenticationCandidate.Username}' is a Service Account, which are prevented from using Octopus interactively. Service Accounts are designed to authorize external systems to access the Octopus API using an API Key."); + } + + var octoResponse = Redirect.Response(stateFromRequest.RedirectAfterLoginTo) + .WithHeader("Expires", new[] {DateTime.UtcNow.AddYears(1).ToString("R", DateTimeFormatInfo.InvariantInfo)}) + .WithCookie(new OctoCookie(UserAuthConstants.OctopusStateCookieName, Guid.NewGuid().ToString()) {HttpOnly = true, Secure = false, Expires = DateTimeOffset.MinValue}) + .WithCookie(new OctoCookie(UserAuthConstants.OctopusNonceCookieName, Guid.NewGuid().ToString()) {HttpOnly = true, Secure = false, Expires = DateTimeOffset.MinValue}); + + var authCookies = authCookieCreator.CreateAuthCookies(successResult.Value.IdentificationToken, TimeSpan.FromDays(20), request.IsHttps, stateFromRequest.UsingSecureConnection); + + foreach (var cookie in authCookies) + { + octoResponse = octoResponse.WithCookie(cookie); + } + + return octoResponse; + } + + // Step 5: Handle other types of failures + loginTracker.RecordFailure(authenticationCandidate.Username, request.Host); + + // Step 5a: Slow this potential attacker down a bit since they seem to keep failing + if (action == InvalidLoginAction.Slow) + { + sleep.For(1000); + } + + return BadRequest($"User login failed: {((IFailureResult) userResult).ErrorString}"); + } + } + + IOctoResponseProvider BadRequest(string message) + { + log.Error(message); + return LoginFailed.Response(message); + } + + IResultFromExtension GetOrCreateUser(UserResource userResource, string[] groups, CancellationToken cancellationToken) + { + var identityToMatch = NewIdentity(userResource); + + var matchingUsers = userStore.GetByIdentity(identityToMatch); + if (matchingUsers.Count() > 1) + throw new Exception("There are multiple users with this identity. OpenID Connect identity providers do not support users with duplicate email addresses. Please remove any duplicate users, or make the email addresses unique."); + var user = matchingUsers.SingleOrDefault(); + + if (user != null) + { + userStore.SetSecurityGroupIds(ProviderName, user.Id, groups, clock.GetUtcTime()); + + var identity = user.Identities.FirstOrDefault(x => MatchesProviderAndExternalId(userResource, x)); + if (identity != null) + { + return ResultFromExtension.Success(user); + } + + identity = user.Identities.FirstOrDefault(x => x.IdentityProviderName == ProviderName && x.Claims[ClaimDescriptor.EmailClaimType].Value == userResource.EmailAddress); + if (identity != null) + { + return ResultFromExtension.Success(userStore.UpdateIdentity(user.Id, identityToMatch, cancellationToken)); + } + + return ResultFromExtension.Success(userStore.AddIdentity(user.Id, identityToMatch, cancellationToken)); + } + + if (!ConfigurationStore.GetAllowAutoUserCreation()) + return ResultFromExtension.Failed("User could not be located and auto user creation is not enabled."); + + var userResult = userStore.Create( + userResource.Username ?? string.Empty, + userResource.DisplayName ?? string.Empty, + userResource.EmailAddress ?? string.Empty, + cancellationToken, + new ProviderUserGroups { IdentityProviderName = ProviderName, GroupIds = groups }, + new[] { identityToMatch }); + if (userResult is IFailureResult failureResult) + return ResultFromExtension.Failed(failureResult.Errors); + return ResultFromExtension.Success(((ISuccessResult)userResult).Value); + } + + bool MatchesProviderAndExternalId(UserResource userResource, Identity x) + { + return x.IdentityProviderName == ProviderName && x.Claims.ContainsKey(IdentityCreator.ExternalIdClaimType) && x.Claims[IdentityCreator.ExternalIdClaimType].Value == userResource.ExternalId; + } + + Identity NewIdentity(UserResource userResource) + { + return identityCreator.Create(userResource); + } + } +} diff --git a/source/Server.OpenIDConnect.Common/Web/UserAuthenticationPkceAction.cs b/source/Server.OpenIDConnect.Common/Web/UserAuthenticationPkceAction.cs new file mode 100644 index 0000000..5322c35 --- /dev/null +++ b/source/Server.OpenIDConnect.Common/Web/UserAuthenticationPkceAction.cs @@ -0,0 +1,94 @@ +using System; +using System.Threading.Tasks; +using Newtonsoft.Json; +using Octopus.Diagnostics; +using Octopus.Server.Extensibility.Authentication.HostServices; +using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Configuration; +using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Infrastructure; +using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Issuer; +using Octopus.Server.Extensibility.Authentication.Resources; +using Octopus.Server.Extensibility.Extensions.Infrastructure.Web.Api; + +namespace Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Web +{ + public abstract class UserAuthenticationPkceAction : IAsyncApiAction + where TStore : IOpenIDConnectConfigurationStore + { + static readonly BadRequestRegistration Disabled = new BadRequestRegistration("This authentication provider is disabled."); + static readonly BadRequestRegistration PotentialOpenDirect = new BadRequestRegistration("Request not allowed, due to potential Open Redirection attack"); + static readonly BadRequestRegistration LoginFailed = new BadRequestRegistration("Login failed. Please see the Octopus Server logs for more details."); + static readonly OctopusJsonRegistration Result = new OctopusJsonRegistration(); + + readonly ISystemLog log; + readonly IIdentityProviderConfigDiscoverer identityProviderConfigDiscoverer; + readonly IAuthorizationEndpointUrlBuilder urlBuilder; + + protected readonly TStore ConfigurationStore; + readonly IApiActionModelBinder modelBinder; + readonly IAuthenticationConfigurationStore authenticationConfigurationStore; + + protected UserAuthenticationPkceAction( + ISystemLog log, + TStore configurationStore, + IIdentityProviderConfigDiscoverer identityProviderConfigDiscoverer, + IAuthorizationEndpointUrlBuilder urlBuilder, + IApiActionModelBinder modelBinder, + IAuthenticationConfigurationStore authenticationConfigurationStore) + { + this.log = log; + this.modelBinder = modelBinder; + this.authenticationConfigurationStore = authenticationConfigurationStore; + ConfigurationStore = configurationStore; + this.identityProviderConfigDiscoverer = identityProviderConfigDiscoverer; + this.urlBuilder = urlBuilder; + } + + public async Task ExecuteAsync(IOctoRequest request) + { + if (ConfigurationStore.GetIsEnabled() == false) + { + log.Warn($"{ConfigurationStore.ConfigurationSettingsName} user authentication API was called while the provider was disabled."); + return Disabled.Response(); + } + + var model = modelBinder.Bind(); + + // If the login state object was passed use it, otherwise fall back to a safe default: + // 1. Redirecting to the root of the local web site + // 2. Setting the Cookie.Secure flag according to the current request + var state = model.State ?? new LoginState{RedirectAfterLoginTo = "/", UsingSecureConnection = request.IsHttps}; + + // Prevent Open Redirection attacks by ensuring the redirect after successful login is somewhere we trust (local origin or a trusted remote origin) + var whitelist = authenticationConfigurationStore.GetTrustedRedirectUrls(); + if (!Requests.IsLocalUrl(state.RedirectAfterLoginTo, whitelist)) + { + log.WarnFormat("Prevented potential Open Redirection attack on an authentication request, to the non-local url {0}", state.RedirectAfterLoginTo); + return PotentialOpenDirect.Response(); + } + + // Finally, provide the client with the information it requires to initiate the redirect to the external identity provider + try + { + var issuer = ConfigurationStore.GetIssuer() ?? string.Empty; + var issuerConfig = await identityProviderConfigDiscoverer.GetConfigurationAsync(issuer); + + var stateString = JsonConvert.SerializeObject(state); + var url = urlBuilder.Build(model.ApiAbsUrl, issuerConfig, state: stateString, pkce: true); + + // These cookies are used to validate the data returned from the external identity provider - this prevents tampering + return Result.Response(new LoginRedirectLinkResponseModel {ExternalAuthenticationUrl = url}) + .WithCookie(new OctoCookie(UserAuthConstants.OctopusStateCookieName, State.Protect(stateString)) { HttpOnly = true, Secure = state.UsingSecureConnection, Expires = DateTimeOffset.UtcNow.AddMinutes(20) }); + } + catch (ArgumentException ex) + { + log.Error(ex); + return LoginFailed.Response(ex.Message); + } + catch (Exception ex) + { + log.Error(ex); + return LoginFailed.Response(); + } + } + } +} \ No newline at end of file From 4719713523a1088c02b9b91464b5dc260afce9a8 Mon Sep 17 00:00:00 2001 From: Nelson Susanto Date: Thu, 17 Mar 2022 09:19:48 +1100 Subject: [PATCH 02/27] First version of code verifier and code challenge --- .../Infrastructure/CodeChallenge.cs | 17 +++++++++++++++++ .../Infrastructure/CodeVerifier.cs | 18 ++++++++++++++++++ 2 files changed, 35 insertions(+) create mode 100644 source/Server.OpenIDConnect.Common/Infrastructure/CodeChallenge.cs create mode 100644 source/Server.OpenIDConnect.Common/Infrastructure/CodeVerifier.cs diff --git a/source/Server.OpenIDConnect.Common/Infrastructure/CodeChallenge.cs b/source/Server.OpenIDConnect.Common/Infrastructure/CodeChallenge.cs new file mode 100644 index 0000000..f3af124 --- /dev/null +++ b/source/Server.OpenIDConnect.Common/Infrastructure/CodeChallenge.cs @@ -0,0 +1,17 @@ +using System; +using System.Security.Cryptography; +using System.Text; + +namespace Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Infrastructure +{ + public static class CodeChallenge + { + public static string CreateS256CodeChallenge(string codeVerifier) + { + using var sha = SHA256.Create(); + var codeChallenge = Convert.ToBase64String(sha.ComputeHash(Encoding.UTF8.GetBytes(codeVerifier))) + .TrimEnd('=').Replace("/", string.Empty).Replace("+", string.Empty); + return codeChallenge; + } + } +} \ No newline at end of file diff --git a/source/Server.OpenIDConnect.Common/Infrastructure/CodeVerifier.cs b/source/Server.OpenIDConnect.Common/Infrastructure/CodeVerifier.cs new file mode 100644 index 0000000..1a01b3e --- /dev/null +++ b/source/Server.OpenIDConnect.Common/Infrastructure/CodeVerifier.cs @@ -0,0 +1,18 @@ +using System; +using System.Security.Cryptography; + +namespace Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Infrastructure +{ + public static class CodeVerifier + { + static readonly RandomNumberGenerator Rng = RandomNumberGenerator.Create(); + + public static string GenerateUrlSafeCodeVerifier() + { + var data = new byte[32]; + Rng.GetBytes(data); + var codeVerifier = Convert.ToBase64String(data).TrimEnd('=').Replace("/", string.Empty).Replace("+", string.Empty); + return codeVerifier; + } + } +} \ No newline at end of file From f7440f255ee573c9d5b18e940b6958f97869fbc2 Mon Sep 17 00:00:00 2001 From: Nelson Susanto Date: Thu, 17 Mar 2022 11:44:38 +1100 Subject: [PATCH 03/27] More pkce stuff --- ...ogleAppsAuthorizationEndpointUrlBuilder.cs | 4 ++-- .../GoogleAppsUserAuthenticatedPkceAction.cs | 5 +++-- .../Infrastructure/CodeVerifier.cs | 2 ++ .../Issuer/AuthorizationEndpointUrlBuilder.cs | 19 ++++++++++++++---- .../IAuthorizationEndpointUrlBuilder.cs | 2 +- .../Issuer/IssuerConfiguration.cs | 3 +++ .../Web/UserAuthenticatedPkceAction.cs | 20 +++++++++++++------ .../Web/UserAuthenticationPkceAction.cs | 3 +++ 8 files changed, 43 insertions(+), 15 deletions(-) diff --git a/source/Server.GoogleApps/Issuer/GoogleAppsAuthorizationEndpointUrlBuilder.cs b/source/Server.GoogleApps/Issuer/GoogleAppsAuthorizationEndpointUrlBuilder.cs index a71d3ff..210f6da 100644 --- a/source/Server.GoogleApps/Issuer/GoogleAppsAuthorizationEndpointUrlBuilder.cs +++ b/source/Server.GoogleApps/Issuer/GoogleAppsAuthorizationEndpointUrlBuilder.cs @@ -11,9 +11,9 @@ public GoogleAppsAuthorizationEndpointUrlBuilder(IGoogleAppsConfigurationStore c { } - public override string Build(string requestDirectoryPath, IssuerConfiguration issuerConfiguration, string? nonce = null, string? state = null, bool pkce = false) + public override string Build(string requestDirectoryPath, IssuerConfiguration issuerConfiguration, string? nonce = null, string? state = null, bool pkce = false, string? codeChallenge = null) { - var url = base.Build(requestDirectoryPath, issuerConfiguration, nonce, state, pkce); + var url = base.Build(requestDirectoryPath, issuerConfiguration, nonce, state, pkce, codeChallenge); var hd = ConfigurationStore.GetHostedDomain(); if (!string.IsNullOrWhiteSpace(hd)) diff --git a/source/Server.GoogleApps/Web/GoogleAppsUserAuthenticatedPkceAction.cs b/source/Server.GoogleApps/Web/GoogleAppsUserAuthenticatedPkceAction.cs index 3a61558..1139960 100644 --- a/source/Server.GoogleApps/Web/GoogleAppsUserAuthenticatedPkceAction.cs +++ b/source/Server.GoogleApps/Web/GoogleAppsUserAuthenticatedPkceAction.cs @@ -4,6 +4,7 @@ using Octopus.Server.Extensibility.Authentication.GoogleApps.Tokens; using Octopus.Server.Extensibility.Authentication.HostServices; using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Infrastructure; +using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Issuer; using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Web; using Octopus.Server.Extensibility.HostServices.Web; using Octopus.Time; @@ -23,8 +24,8 @@ public GoogleAppsUserAuthenticatedPkceAction( IInvalidLoginTracker loginTracker, ISleep sleep, IGoogleAppsIdentityCreator identityCreator, - IClock clock, IUrlEncoder encoder) - : base(log, authTokenHandler, principalToUserResourceMapper, userStore, configurationStore, authCookieCreator, loginTracker, sleep, identityCreator, clock, encoder) + IClock clock, IUrlEncoder encoder, IIdentityProviderConfigDiscoverer identityProviderConfigDiscoverer) + : base(log, authTokenHandler, principalToUserResourceMapper, userStore, configurationStore, authCookieCreator, loginTracker, sleep, identityCreator, clock, encoder, identityProviderConfigDiscoverer) { } diff --git a/source/Server.OpenIDConnect.Common/Infrastructure/CodeVerifier.cs b/source/Server.OpenIDConnect.Common/Infrastructure/CodeVerifier.cs index 1a01b3e..da6c2f8 100644 --- a/source/Server.OpenIDConnect.Common/Infrastructure/CodeVerifier.cs +++ b/source/Server.OpenIDConnect.Common/Infrastructure/CodeVerifier.cs @@ -5,6 +5,7 @@ namespace Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Infra { public static class CodeVerifier { + public static string? InMemoryCodeVerifier { get; private set; } static readonly RandomNumberGenerator Rng = RandomNumberGenerator.Create(); public static string GenerateUrlSafeCodeVerifier() @@ -12,6 +13,7 @@ public static string GenerateUrlSafeCodeVerifier() var data = new byte[32]; Rng.GetBytes(data); var codeVerifier = Convert.ToBase64String(data).TrimEnd('=').Replace("/", string.Empty).Replace("+", string.Empty); + InMemoryCodeVerifier = codeVerifier; return codeVerifier; } } diff --git a/source/Server.OpenIDConnect.Common/Issuer/AuthorizationEndpointUrlBuilder.cs b/source/Server.OpenIDConnect.Common/Issuer/AuthorizationEndpointUrlBuilder.cs index 39b4142..23eb841 100644 --- a/source/Server.OpenIDConnect.Common/Issuer/AuthorizationEndpointUrlBuilder.cs +++ b/source/Server.OpenIDConnect.Common/Issuer/AuthorizationEndpointUrlBuilder.cs @@ -7,7 +7,7 @@ namespace Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Issue public abstract class AuthorizationEndpointUrlBuilder : IAuthorizationEndpointUrlBuilder where TStore : IOpenIDConnectConfigurationStore { - protected readonly TStore ConfigurationStore; + protected TStore ConfigurationStore { get; } readonly IUrlEncoder urlEncoder; protected AuthorizationEndpointUrlBuilder(TStore configurationStore, IUrlEncoder urlEncoder) @@ -20,7 +20,7 @@ protected AuthorizationEndpointUrlBuilder(TStore configurationStore, IUrlEncoder protected virtual string PkceResponseType => OpenIDConnectConfiguration.AuthCodeResponseType; protected virtual string ResponseMode => OpenIDConnectConfiguration.DefaultResponseMode; - public virtual string Build(string requestDirectoryPath, IssuerConfiguration issuerConfiguration, string? nonce = null, string? state = null, bool pkce = false) + public virtual string Build(string requestDirectoryPath, IssuerConfiguration issuerConfiguration, string? nonce = null, string? state = null, bool pkce = false, string? codeChallenge = null) { if (issuerConfiguration == null) throw new ArgumentException("issuerConfiguration is required", nameof(issuerConfiguration)); @@ -32,8 +32,14 @@ public virtual string Build(string requestDirectoryPath, IssuerConfiguration iss var responseMode = ResponseMode; var redirectUri = requestDirectoryPath.Trim('/') + ConfigurationStore.RedirectUri; - var url = $"{issuerEndpoint}?client_id={clientId}&scope={scope}&response_type={responseType}&response_mode={responseMode}&redirect_uri={redirectUri}"; - + var url = $"{issuerEndpoint}?client_id={clientId}&scope={scope}&response_type={responseType}&redirect_uri={redirectUri}"; + + if (!pkce) + { + url += $"&response_mode={responseMode}"; + + } + if (!string.IsNullOrWhiteSpace(state)) { url += $"&state={urlEncoder.UrlEncode(state)}"; @@ -43,6 +49,11 @@ public virtual string Build(string requestDirectoryPath, IssuerConfiguration iss { url += $"&nonce={nonce}"; } + if (!string.IsNullOrWhiteSpace(codeChallenge)) + { + url += $"&code_challenge={urlEncoder.UrlEncode(codeChallenge)}"; + url += $"&code_challenge_method=S256"; + } return url; } diff --git a/source/Server.OpenIDConnect.Common/Issuer/IAuthorizationEndpointUrlBuilder.cs b/source/Server.OpenIDConnect.Common/Issuer/IAuthorizationEndpointUrlBuilder.cs index bdc88c0..2d61499 100644 --- a/source/Server.OpenIDConnect.Common/Issuer/IAuthorizationEndpointUrlBuilder.cs +++ b/source/Server.OpenIDConnect.Common/Issuer/IAuthorizationEndpointUrlBuilder.cs @@ -2,6 +2,6 @@ { public interface IAuthorizationEndpointUrlBuilder { - string Build(string requestDirectoryPath, IssuerConfiguration issuerConfiguration, string? nonce = null, string? state = null, bool pkce = false); + string Build(string requestDirectoryPath, IssuerConfiguration issuerConfiguration, string? nonce = null, string? state = null, bool pkce = false, string? codeChallenge = null); } } \ No newline at end of file diff --git a/source/Server.OpenIDConnect.Common/Issuer/IssuerConfiguration.cs b/source/Server.OpenIDConnect.Common/Issuer/IssuerConfiguration.cs index ad6d597..5586572 100644 --- a/source/Server.OpenIDConnect.Common/Issuer/IssuerConfiguration.cs +++ b/source/Server.OpenIDConnect.Common/Issuer/IssuerConfiguration.cs @@ -11,5 +11,8 @@ public class IssuerConfiguration [JsonProperty("authorization_endpoint")] public string AuthorizationEndpoint { get; set; } = string.Empty; + + [JsonProperty("token_endpoint")] + public string TokenEndpoint { get; set; } = string.Empty; } } \ No newline at end of file diff --git a/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedPkceAction.cs b/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedPkceAction.cs index 10196f6..7925fec 100644 --- a/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedPkceAction.cs +++ b/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedPkceAction.cs @@ -14,6 +14,7 @@ using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Configuration; using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Identities; using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Infrastructure; +using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Issuer; using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Tokens; using Octopus.Server.Extensibility.Authentication.Resources; using Octopus.Server.Extensibility.Authentication.Resources.Identities; @@ -45,8 +46,9 @@ public abstract class UserAuthenticatedPkceAction ExecuteAsync(IOctoRequest request) { var issuerConfig = await identityProviderConfigDiscoverer.GetConfigurationAsync(ConfigurationStore.GetIssuer() ?? string.Empty); using var client = new HttpClient(); - var request = new HttpRequestMessage(HttpMethod.Post, issuerConfig.TokenEndpoint); + var request = new HttpRequestMessage(HttpMethod.Get, issuerConfig.TokenEndpoint); var formValues = new Dictionary { ["grant_type"] = "authorization_code", ["code"] = code, ["redirect_uri"] = redirectUri, ["client_id"] = ConfigurationStore.GetClientId()!, - ["client_secret"] = ConfigurationStore.GetClientSecret()!.Value + ["code_verifier"] = CodeVerifier.InMemoryCodeVerifier! }; request.Content = new FormUrlEncodedContent(formValues); var response = await client.SendAsync(request, HttpCompletionOption.ResponseContentRead); @@ -102,8 +106,11 @@ public async Task ExecuteAsync(IOctoRequest request) async Task Handle(string code, string state, IOctoRequest request) { + var redirectUri = $"{request.Scheme}://{request.Host}{ConfigurationStore.RedirectUri}"; + var response = await RequestAuthToken(code, redirectUri); + // Step 1: Try and get all of the details from the request making sure there are no errors passed back from the external identity provider - var principalContainer = await authTokenHandler.GetPrincipalAsync(request.Form.ToDictionary(pair => pair.Key, pair => (string?)pair.Value), out var stateStringFromRequest); + var principalContainer = await authTokenHandler.GetPrincipalAsync(response, out var stateStringFromRequest); var principal = principalContainer.Principal; if (principal == null || !string.IsNullOrEmpty(principalContainer.Error)) { @@ -113,6 +120,7 @@ async Task Handle(string code, string state, IOctoRequest // Step 2: Validate the state object we passed wasn't tampered with const string stateDescription = "As a security precaution, Octopus ensures the state object returned from the external identity provider matches what it expected."; var expectedStateHash = string.Empty; + stateStringFromRequest ??= state; if (request.Cookies.ContainsKey(UserAuthConstants.OctopusStateCookieName)) expectedStateHash = encoder.UrlDecode(request.Cookies[UserAuthConstants.OctopusStateCookieName]); if (string.IsNullOrWhiteSpace(expectedStateHash)) @@ -120,7 +128,7 @@ async Task Handle(string code, string state, IOctoRequest return BadRequest($"User login failed: Missing State Hash Cookie. {stateDescription} In this case the Cookie containing the SHA256 hash of the state object is missing from the request."); } - var stateFromRequestHash = State.Protect(stateStringFromRequest); + var stateFromRequestHash = Infrastructure.State.Protect(stateStringFromRequest); if (stateFromRequestHash != expectedStateHash) { return BadRequest($"User login failed: Tampered State. {stateDescription} In this case the state object looks like it has been tampered with. The state object is '{stateStringFromRequest}'. The SHA256 hash of the state was expected to be '{expectedStateHash}' but was '{stateFromRequestHash}'."); diff --git a/source/Server.OpenIDConnect.Common/Web/UserAuthenticationPkceAction.cs b/source/Server.OpenIDConnect.Common/Web/UserAuthenticationPkceAction.cs index 5322c35..db7f68d 100644 --- a/source/Server.OpenIDConnect.Common/Web/UserAuthenticationPkceAction.cs +++ b/source/Server.OpenIDConnect.Common/Web/UserAuthenticationPkceAction.cs @@ -72,6 +72,9 @@ public async Task ExecuteAsync(IOctoRequest request) var issuer = ConfigurationStore.GetIssuer() ?? string.Empty; var issuerConfig = await identityProviderConfigDiscoverer.GetConfigurationAsync(issuer); + var codeVerifier = CodeVerifier.GenerateUrlSafeCodeVerifier(); + var codeChallenge = CodeChallenge.CreateS256CodeChallenge(codeVerifier); + var stateString = JsonConvert.SerializeObject(state); var url = urlBuilder.Build(model.ApiAbsUrl, issuerConfig, state: stateString, pkce: true); From 94121e99faa8af07d017f164464d69bcfbc47087 Mon Sep 17 00:00:00 2001 From: Nelson Susanto Date: Thu, 17 Mar 2022 11:46:43 +1100 Subject: [PATCH 04/27] Add client secret --- .../IOpenIDConnectConfigurationStore.cs | 3 +++ .../Configuration/OpenIDConnectConfiguration.cs | 5 ++++- .../OpenIDConnectConfigurationResource.cs | 6 ++++++ .../Configuration/OpenIDConnectConfigurationStore.cs | 12 ++++++++++++ .../Configuration/OpenIDConnectConfigureCommands.cs | 5 +++++ 5 files changed, 30 insertions(+), 1 deletion(-) diff --git a/source/Server.OpenIDConnect.Common/Configuration/IOpenIDConnectConfigurationStore.cs b/source/Server.OpenIDConnect.Common/Configuration/IOpenIDConnectConfigurationStore.cs index caac390..527a295 100644 --- a/source/Server.OpenIDConnect.Common/Configuration/IOpenIDConnectConfigurationStore.cs +++ b/source/Server.OpenIDConnect.Common/Configuration/IOpenIDConnectConfigurationStore.cs @@ -17,6 +17,9 @@ public interface IOpenIDConnectConfigurationStore : IExtensionConfigurationStore string? GetClientId(); void SetClientId(string? clientId); + SensitiveString? GetClientSecret(); + void SetClientSecret(SensitiveString? clientSecret); + bool HasClientSecret { get; } string? GetScope(); void SetScope(string? scope); diff --git a/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfiguration.cs b/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfiguration.cs index 1b90c9e..0d67dba 100644 --- a/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfiguration.cs +++ b/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfiguration.cs @@ -1,4 +1,5 @@ -using Octopus.Server.Extensibility.Extensions.Infrastructure.Configuration; +using Octopus.Data.Model; +using Octopus.Server.Extensibility.Extensions.Infrastructure.Configuration; namespace Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Configuration { @@ -28,6 +29,8 @@ protected OpenIDConnectConfiguration(string id, string name, string author, stri public string? ClientId { get; set; } + public SensitiveString? ClientSecret { get; set; } + public string? Scope { get; set; } public string? NameClaimType { get; set; } diff --git a/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfigurationResource.cs b/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfigurationResource.cs index 9b5120d..647d0b3 100644 --- a/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfigurationResource.cs +++ b/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfigurationResource.cs @@ -1,5 +1,6 @@ using System.ComponentModel; using Octopus.Server.Extensibility.Extensions.Infrastructure.Configuration; +using Octopus.Server.MessageContracts; using Octopus.Server.MessageContracts.Attributes; namespace Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Configuration @@ -15,6 +16,11 @@ public class OpenIDConnectConfigurationResource : ExtensionConfigurationResource [Writeable] public virtual string? ClientId { get; set; } + [DisplayName("Client Secret")] + [Description("Shared secret for validating the authentication tokens")] + [Writeable] + public virtual SensitiveValue? ClientSecret { get; set; } + [DisplayName("Allow Auto User Creation")] [Description("Tell Octopus to automatically create a user account when a person signs in for the first time with this identity provider")] [Writeable] diff --git a/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfigurationStore.cs b/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfigurationStore.cs index 1087aa0..ef4e591 100644 --- a/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfigurationStore.cs +++ b/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfigurationStore.cs @@ -40,6 +40,18 @@ public void SetClientId(string? clientId) SetProperty(doc => doc.ClientId = clientId); } + public SensitiveString? GetClientSecret() + { + return GetProperty(doc => doc.ClientSecret); + } + + public void SetClientSecret(SensitiveString? clientSecret) + { + SetProperty(doc => doc.ClientSecret = clientSecret); + } + + public bool HasClientSecret => GetClientSecret() != null; + public string? GetScope() { return GetProperty(doc => doc.Scope); diff --git a/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfigureCommands.cs b/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfigureCommands.cs index cd30ada..1c3201c 100644 --- a/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfigureCommands.cs +++ b/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfigureCommands.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Generic; using System.Linq; +using Octopus.Data.Model; using Octopus.Diagnostics; using Octopus.Server.Extensibility.Extensions.Infrastructure.Configuration; using Octopus.Server.Extensibility.HostServices.Web; @@ -61,6 +62,10 @@ protected IEnumerable GetCoreOptions(bool hide) ConfigurationStore.Value.SetClientId(v); Log.Info($"{ConfigurationSettingsName} ClientId set to: {v}"); }, hide: hide); + yield return new ConfigureCommandOption($"{ConfigurationSettingsName}ClientSecret=", $"Follow our documentation to find the Client Secret for {ConfigurationSettingsName}.", v => + { + ConfigurationStore.Value.SetClientSecret(v.ToSensitiveString()); + }, hide: hide); } public virtual IEnumerable GetOptions() From 54b03c4b031c8ab93214cce5259bf759403d8543 Mon Sep 17 00:00:00 2001 From: Nelson Susanto Date: Thu, 17 Mar 2022 13:25:51 +1100 Subject: [PATCH 05/27] use HasClientSecret in AuthTokenHandler.cs --- source/Server.OpenIDConnect.Common/Tokens/AuthTokenHandler.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/Server.OpenIDConnect.Common/Tokens/AuthTokenHandler.cs b/source/Server.OpenIDConnect.Common/Tokens/AuthTokenHandler.cs index 5383a15..dbae003 100644 --- a/source/Server.OpenIDConnect.Common/Tokens/AuthTokenHandler.cs +++ b/source/Server.OpenIDConnect.Common/Tokens/AuthTokenHandler.cs @@ -88,9 +88,9 @@ protected async Task GetPrincipalFromToken(string? acc ClaimsPrincipal ValidateUsingSharedSecret(TokenValidationParameters validationParameters, string? tokenToValidate) { - if (ConfigurationStore is IOpenIDConnectWithClientSecretConfigurationStore clientSecretStore) + if (ConfigurationStore.HasClientSecret) { - var clientSecret = clientSecretStore.GetClientSecret(); + var clientSecret = ConfigurationStore.GetClientSecret(); if (clientSecret == null) throw new ArgumentException("Client secret is not configured."); validationParameters.IssuerSigningKey = new SymmetricSecurityKey(Encoding.UTF8.GetBytes(clientSecret.Value)); From 13934caf3d362b67880fdfe28e74e3a9fbbbeda2 Mon Sep 17 00:00:00 2001 From: Nelson Susanto Date: Thu, 17 Mar 2022 13:26:50 +1100 Subject: [PATCH 06/27] Move Pkce stuff into existing AuthenticationAction --- .../Web/UserAuthenticationAction.cs | 37 +++++-- .../Web/UserAuthenticationPkceAction.cs | 97 ------------------- 2 files changed, 28 insertions(+), 106 deletions(-) delete mode 100644 source/Server.OpenIDConnect.Common/Web/UserAuthenticationPkceAction.cs diff --git a/source/Server.OpenIDConnect.Common/Web/UserAuthenticationAction.cs b/source/Server.OpenIDConnect.Common/Web/UserAuthenticationAction.cs index 409e204..6ac44d0 100644 --- a/source/Server.OpenIDConnect.Common/Web/UserAuthenticationAction.cs +++ b/source/Server.OpenIDConnect.Common/Web/UserAuthenticationAction.cs @@ -72,16 +72,11 @@ public async Task ExecuteAsync(IOctoRequest request) var issuer = ConfigurationStore.GetIssuer() ?? string.Empty; var issuerConfig = await identityProviderConfigDiscoverer.GetConfigurationAsync(issuer); - // Use a non-deterministic nonce to prevent replay attacks - var nonce = Nonce.GenerateUrlSafeNonce(); + var response = ConfigurationStore.HasClientSecret + ? BuildAuthorizationCodePkceResponse(model, state, issuerConfig) + : BuildHybridResponse(model, state, issuerConfig); - var stateString = JsonConvert.SerializeObject(state); - var url = urlBuilder.Build(model.ApiAbsUrl, issuerConfig, nonce, stateString); - - // These cookies are used to validate the data returned from the external identity provider - this prevents tampering - return Result.Response(new LoginRedirectLinkResponseModel {ExternalAuthenticationUrl = url}) - .WithCookie(new OctoCookie(UserAuthConstants.OctopusStateCookieName, State.Protect(stateString)) { HttpOnly = true, Secure = state.UsingSecureConnection, Expires = DateTimeOffset.UtcNow.AddMinutes(20) }) - .WithCookie(new OctoCookie(UserAuthConstants.OctopusNonceCookieName, Nonce.Protect(nonce)) { HttpOnly = true, Secure = state.UsingSecureConnection, Expires = DateTimeOffset.UtcNow.AddMinutes(20) }); + return response; } catch (ArgumentException ex) { @@ -94,5 +89,29 @@ public async Task ExecuteAsync(IOctoRequest request) return LoginFailed.Response(); } } + + IOctoResponseProvider BuildAuthorizationCodePkceResponse(LoginRedirectLinkRequestModel model, LoginState state, IssuerConfiguration issuerConfig) + { + var codeVerifier = CodeVerifier.GenerateUrlSafeCodeVerifier(); + var codeChallenge = CodeChallenge.CreateS256CodeChallenge(codeVerifier); + + var stateString = JsonConvert.SerializeObject(state); + var url = urlBuilder.Build(model.ApiAbsUrl, issuerConfig, state: stateString, codeChallenge: codeChallenge); + var response = Result.Response(new LoginRedirectLinkResponseModel {ExternalAuthenticationUrl = url}) + .WithCookie(new OctoCookie(UserAuthConstants.OctopusStateCookieName, State.Protect(stateString)) {HttpOnly = true, Secure = state.UsingSecureConnection, Expires = DateTimeOffset.UtcNow.AddMinutes(20)}); + return response; + } + + IOctoResponseProvider BuildHybridResponse(LoginRedirectLinkRequestModel model, LoginState state, IssuerConfiguration issuerConfig) + { + var nonce = Nonce.GenerateUrlSafeNonce(); + var stateString = JsonConvert.SerializeObject(state); + var url = urlBuilder.Build(model.ApiAbsUrl, issuerConfig, nonce, stateString); + // These cookies are used to validate the data returned from the external identity provider - this prevents tampering + var response = Result.Response(new LoginRedirectLinkResponseModel {ExternalAuthenticationUrl = url}) + .WithCookie(new OctoCookie(UserAuthConstants.OctopusStateCookieName, State.Protect(stateString)) {HttpOnly = true, Secure = state.UsingSecureConnection, Expires = DateTimeOffset.UtcNow.AddMinutes(20)}) + .WithCookie(new OctoCookie(UserAuthConstants.OctopusNonceCookieName, Nonce.Protect(nonce)) {HttpOnly = true, Secure = state.UsingSecureConnection, Expires = DateTimeOffset.UtcNow.AddMinutes(20)}); + return response; + } } } \ No newline at end of file diff --git a/source/Server.OpenIDConnect.Common/Web/UserAuthenticationPkceAction.cs b/source/Server.OpenIDConnect.Common/Web/UserAuthenticationPkceAction.cs deleted file mode 100644 index db7f68d..0000000 --- a/source/Server.OpenIDConnect.Common/Web/UserAuthenticationPkceAction.cs +++ /dev/null @@ -1,97 +0,0 @@ -using System; -using System.Threading.Tasks; -using Newtonsoft.Json; -using Octopus.Diagnostics; -using Octopus.Server.Extensibility.Authentication.HostServices; -using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Configuration; -using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Infrastructure; -using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Issuer; -using Octopus.Server.Extensibility.Authentication.Resources; -using Octopus.Server.Extensibility.Extensions.Infrastructure.Web.Api; - -namespace Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Web -{ - public abstract class UserAuthenticationPkceAction : IAsyncApiAction - where TStore : IOpenIDConnectConfigurationStore - { - static readonly BadRequestRegistration Disabled = new BadRequestRegistration("This authentication provider is disabled."); - static readonly BadRequestRegistration PotentialOpenDirect = new BadRequestRegistration("Request not allowed, due to potential Open Redirection attack"); - static readonly BadRequestRegistration LoginFailed = new BadRequestRegistration("Login failed. Please see the Octopus Server logs for more details."); - static readonly OctopusJsonRegistration Result = new OctopusJsonRegistration(); - - readonly ISystemLog log; - readonly IIdentityProviderConfigDiscoverer identityProviderConfigDiscoverer; - readonly IAuthorizationEndpointUrlBuilder urlBuilder; - - protected readonly TStore ConfigurationStore; - readonly IApiActionModelBinder modelBinder; - readonly IAuthenticationConfigurationStore authenticationConfigurationStore; - - protected UserAuthenticationPkceAction( - ISystemLog log, - TStore configurationStore, - IIdentityProviderConfigDiscoverer identityProviderConfigDiscoverer, - IAuthorizationEndpointUrlBuilder urlBuilder, - IApiActionModelBinder modelBinder, - IAuthenticationConfigurationStore authenticationConfigurationStore) - { - this.log = log; - this.modelBinder = modelBinder; - this.authenticationConfigurationStore = authenticationConfigurationStore; - ConfigurationStore = configurationStore; - this.identityProviderConfigDiscoverer = identityProviderConfigDiscoverer; - this.urlBuilder = urlBuilder; - } - - public async Task ExecuteAsync(IOctoRequest request) - { - if (ConfigurationStore.GetIsEnabled() == false) - { - log.Warn($"{ConfigurationStore.ConfigurationSettingsName} user authentication API was called while the provider was disabled."); - return Disabled.Response(); - } - - var model = modelBinder.Bind(); - - // If the login state object was passed use it, otherwise fall back to a safe default: - // 1. Redirecting to the root of the local web site - // 2. Setting the Cookie.Secure flag according to the current request - var state = model.State ?? new LoginState{RedirectAfterLoginTo = "/", UsingSecureConnection = request.IsHttps}; - - // Prevent Open Redirection attacks by ensuring the redirect after successful login is somewhere we trust (local origin or a trusted remote origin) - var whitelist = authenticationConfigurationStore.GetTrustedRedirectUrls(); - if (!Requests.IsLocalUrl(state.RedirectAfterLoginTo, whitelist)) - { - log.WarnFormat("Prevented potential Open Redirection attack on an authentication request, to the non-local url {0}", state.RedirectAfterLoginTo); - return PotentialOpenDirect.Response(); - } - - // Finally, provide the client with the information it requires to initiate the redirect to the external identity provider - try - { - var issuer = ConfigurationStore.GetIssuer() ?? string.Empty; - var issuerConfig = await identityProviderConfigDiscoverer.GetConfigurationAsync(issuer); - - var codeVerifier = CodeVerifier.GenerateUrlSafeCodeVerifier(); - var codeChallenge = CodeChallenge.CreateS256CodeChallenge(codeVerifier); - - var stateString = JsonConvert.SerializeObject(state); - var url = urlBuilder.Build(model.ApiAbsUrl, issuerConfig, state: stateString, pkce: true); - - // These cookies are used to validate the data returned from the external identity provider - this prevents tampering - return Result.Response(new LoginRedirectLinkResponseModel {ExternalAuthenticationUrl = url}) - .WithCookie(new OctoCookie(UserAuthConstants.OctopusStateCookieName, State.Protect(stateString)) { HttpOnly = true, Secure = state.UsingSecureConnection, Expires = DateTimeOffset.UtcNow.AddMinutes(20) }); - } - catch (ArgumentException ex) - { - log.Error(ex); - return LoginFailed.Response(ex.Message); - } - catch (Exception ex) - { - log.Error(ex); - return LoginFailed.Response(); - } - } - } -} \ No newline at end of file From 437406e0359226da2e34a43b5fcbd7d4fc7b09d4 Mon Sep 17 00:00:00 2001 From: Nelson Susanto Date: Thu, 17 Mar 2022 13:33:26 +1100 Subject: [PATCH 07/27] Add client_secret to authenticated action --- .../Web/UserAuthenticatedPkceAction.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedPkceAction.cs b/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedPkceAction.cs index 7925fec..5a2998c 100644 --- a/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedPkceAction.cs +++ b/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedPkceAction.cs @@ -89,13 +89,14 @@ public async Task ExecuteAsync(IOctoRequest request) { var issuerConfig = await identityProviderConfigDiscoverer.GetConfigurationAsync(ConfigurationStore.GetIssuer() ?? string.Empty); using var client = new HttpClient(); - var request = new HttpRequestMessage(HttpMethod.Get, issuerConfig.TokenEndpoint); + var request = new HttpRequestMessage(HttpMethod.Post, issuerConfig.TokenEndpoint); var formValues = new Dictionary { ["grant_type"] = "authorization_code", ["code"] = code, ["redirect_uri"] = redirectUri, ["client_id"] = ConfigurationStore.GetClientId()!, + ["client_secret"] = ConfigurationStore.GetClientSecret()!.Value, ["code_verifier"] = CodeVerifier.InMemoryCodeVerifier! }; request.Content = new FormUrlEncodedContent(formValues); From c2e3310a9740b8c6080d05735671e7de88a73824 Mon Sep 17 00:00:00 2001 From: Nelson Susanto Date: Thu, 17 Mar 2022 13:38:00 +1100 Subject: [PATCH 08/27] Use HasClientSecret for everything --- source/Server.GoogleApps/GoogleAppsApi.cs | 1 - .../Server.GoogleApps/GoogleAppsExtension.cs | 1 - ...ogleAppsAuthorizationEndpointUrlBuilder.cs | 4 ++-- .../GoogleAppsUserAuthenticationPkceAction.cs | 24 ------------------- .../Issuer/AuthorizationEndpointUrlBuilder.cs | 10 ++++---- .../IAuthorizationEndpointUrlBuilder.cs | 2 +- 6 files changed, 8 insertions(+), 34 deletions(-) delete mode 100644 source/Server.GoogleApps/Web/GoogleAppsUserAuthenticationPkceAction.cs diff --git a/source/Server.GoogleApps/GoogleAppsApi.cs b/source/Server.GoogleApps/GoogleAppsApi.cs index a9e62d1..28368e0 100644 --- a/source/Server.GoogleApps/GoogleAppsApi.cs +++ b/source/Server.GoogleApps/GoogleAppsApi.cs @@ -14,7 +14,6 @@ public GoogleAppsApi( IGoogleAppsConfigurationStore configurationStore, GoogleAppsAuthenticationProvider authenticationProvider) : base(configurationStore, authenticationProvider) { - Add("POST", authenticationProvider.AuthenticatePkceUri, RouteCategory.Raw, new AnonymousWhenEnabledEndpointInvocation(), null, "OpenIDConnect"); Add("POST", authenticationProvider.AuthenticateUri, RouteCategory.Raw, new AnonymousWhenEnabledEndpointInvocation(), null, "OpenIDConnect"); Add("POST", configurationStore.RedirectUri, RouteCategory.Raw, new AnonymousWhenEnabledEndpointInvocation(), null, "OpenIDConnect"); Add("GET", configurationStore.RedirectUri, RouteCategory.Raw, new AnonymousWhenEnabledEndpointInvocation(), null, "OpenIDConnect"); diff --git a/source/Server.GoogleApps/GoogleAppsExtension.cs b/source/Server.GoogleApps/GoogleAppsExtension.cs index 82f6c85..a6c64e0 100644 --- a/source/Server.GoogleApps/GoogleAppsExtension.cs +++ b/source/Server.GoogleApps/GoogleAppsExtension.cs @@ -57,7 +57,6 @@ public override void Load(ContainerBuilder builder) builder.RegisterType().As().SingleInstance(); builder.RegisterType().As().SingleInstance(); - builder.RegisterType().AsSelf().InstancePerDependency(); builder.RegisterType().AsSelf().InstancePerDependency(); builder.RegisterType().AsSelf().InstancePerDependency(); builder.RegisterType().AsSelf().InstancePerDependency(); diff --git a/source/Server.GoogleApps/Issuer/GoogleAppsAuthorizationEndpointUrlBuilder.cs b/source/Server.GoogleApps/Issuer/GoogleAppsAuthorizationEndpointUrlBuilder.cs index 210f6da..ad14329 100644 --- a/source/Server.GoogleApps/Issuer/GoogleAppsAuthorizationEndpointUrlBuilder.cs +++ b/source/Server.GoogleApps/Issuer/GoogleAppsAuthorizationEndpointUrlBuilder.cs @@ -11,9 +11,9 @@ public GoogleAppsAuthorizationEndpointUrlBuilder(IGoogleAppsConfigurationStore c { } - public override string Build(string requestDirectoryPath, IssuerConfiguration issuerConfiguration, string? nonce = null, string? state = null, bool pkce = false, string? codeChallenge = null) + public override string Build(string requestDirectoryPath, IssuerConfiguration issuerConfiguration, string? nonce = null, string? state = null, string? codeChallenge = null) { - var url = base.Build(requestDirectoryPath, issuerConfiguration, nonce, state, pkce, codeChallenge); + var url = base.Build(requestDirectoryPath, issuerConfiguration, nonce, state, codeChallenge); var hd = ConfigurationStore.GetHostedDomain(); if (!string.IsNullOrWhiteSpace(hd)) diff --git a/source/Server.GoogleApps/Web/GoogleAppsUserAuthenticationPkceAction.cs b/source/Server.GoogleApps/Web/GoogleAppsUserAuthenticationPkceAction.cs deleted file mode 100644 index 7d7b27d..0000000 --- a/source/Server.GoogleApps/Web/GoogleAppsUserAuthenticationPkceAction.cs +++ /dev/null @@ -1,24 +0,0 @@ -using Octopus.Diagnostics; -using Octopus.Server.Extensibility.Authentication.GoogleApps.Configuration; -using Octopus.Server.Extensibility.Authentication.GoogleApps.Issuer; -using Octopus.Server.Extensibility.Authentication.HostServices; -using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Issuer; -using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Web; -using Octopus.Server.Extensibility.Extensions.Infrastructure.Web.Api; - -namespace Octopus.Server.Extensibility.Authentication.GoogleApps.Web -{ - class GoogleAppsUserAuthenticationPkceAction : UserAuthenticationPkceAction - { - public GoogleAppsUserAuthenticationPkceAction( - ISystemLog log, - IGoogleAppsConfigurationStore configurationStore, - IIdentityProviderConfigDiscoverer identityProviderConfigDiscoverer, - IGoogleAppsAuthorizationEndpointUrlBuilder urlBuilder, - IApiActionModelBinder modelBinder, - IAuthenticationConfigurationStore authenticationConfigurationStore) - : base(log, configurationStore, identityProviderConfigDiscoverer, urlBuilder, modelBinder, authenticationConfigurationStore) - { - } - } -} \ No newline at end of file diff --git a/source/Server.OpenIDConnect.Common/Issuer/AuthorizationEndpointUrlBuilder.cs b/source/Server.OpenIDConnect.Common/Issuer/AuthorizationEndpointUrlBuilder.cs index 23eb841..83380b9 100644 --- a/source/Server.OpenIDConnect.Common/Issuer/AuthorizationEndpointUrlBuilder.cs +++ b/source/Server.OpenIDConnect.Common/Issuer/AuthorizationEndpointUrlBuilder.cs @@ -16,11 +16,10 @@ protected AuthorizationEndpointUrlBuilder(TStore configurationStore, IUrlEncoder this.urlEncoder = urlEncoder; } - protected virtual string ResponseType => OpenIDConnectConfiguration.HybridResponseType; - protected virtual string PkceResponseType => OpenIDConnectConfiguration.AuthCodeResponseType; + protected virtual string ResponseType => ConfigurationStore.HasClientSecret ? OpenIDConnectConfiguration.AuthCodeResponseType : OpenIDConnectConfiguration.HybridResponseType; protected virtual string ResponseMode => OpenIDConnectConfiguration.DefaultResponseMode; - public virtual string Build(string requestDirectoryPath, IssuerConfiguration issuerConfiguration, string? nonce = null, string? state = null, bool pkce = false, string? codeChallenge = null) + public virtual string Build(string requestDirectoryPath, IssuerConfiguration issuerConfiguration, string? nonce = null, string? state = null, string? codeChallenge = null) { if (issuerConfiguration == null) throw new ArgumentException("issuerConfiguration is required", nameof(issuerConfiguration)); @@ -28,13 +27,13 @@ public virtual string Build(string requestDirectoryPath, IssuerConfiguration iss var issuerEndpoint = issuerConfiguration.AuthorizationEndpoint; var clientId = ConfigurationStore.GetClientId(); var scope = ConfigurationStore.GetScope(); - var responseType = pkce ? PkceResponseType : ResponseType; + var responseType = ResponseType; var responseMode = ResponseMode; var redirectUri = requestDirectoryPath.Trim('/') + ConfigurationStore.RedirectUri; var url = $"{issuerEndpoint}?client_id={clientId}&scope={scope}&response_type={responseType}&redirect_uri={redirectUri}"; - if (!pkce) + if (!ConfigurationStore.HasClientSecret) { url += $"&response_mode={responseMode}"; @@ -49,6 +48,7 @@ public virtual string Build(string requestDirectoryPath, IssuerConfiguration iss { url += $"&nonce={nonce}"; } + if (!string.IsNullOrWhiteSpace(codeChallenge)) { url += $"&code_challenge={urlEncoder.UrlEncode(codeChallenge)}"; diff --git a/source/Server.OpenIDConnect.Common/Issuer/IAuthorizationEndpointUrlBuilder.cs b/source/Server.OpenIDConnect.Common/Issuer/IAuthorizationEndpointUrlBuilder.cs index 2d61499..410e910 100644 --- a/source/Server.OpenIDConnect.Common/Issuer/IAuthorizationEndpointUrlBuilder.cs +++ b/source/Server.OpenIDConnect.Common/Issuer/IAuthorizationEndpointUrlBuilder.cs @@ -2,6 +2,6 @@ { public interface IAuthorizationEndpointUrlBuilder { - string Build(string requestDirectoryPath, IssuerConfiguration issuerConfiguration, string? nonce = null, string? state = null, bool pkce = false, string? codeChallenge = null); + string Build(string requestDirectoryPath, IssuerConfiguration issuerConfiguration, string? nonce = null, string? state = null, string? codeChallenge = null); } } \ No newline at end of file From 57a4e3d57dfa5ebb8dd37580ede7aebe2ac8c0fa Mon Sep 17 00:00:00 2001 From: Nelson Susanto Date: Thu, 17 Mar 2022 13:47:49 +1100 Subject: [PATCH 09/27] A lot of renaming and remove client secret stuff from OctopusID --- .../Configuration/AzureADConfigurationSettings.cs | 2 +- .../Configuration/AzureADConfigurationStore.cs | 2 +- .../Configuration/AzureADConfigureCommands.cs | 2 +- .../Configuration/GoogleAppsConfigurationSettings.cs | 2 +- .../Configuration/GoogleAppsConfigurationStore.cs | 2 +- .../Configuration/GoogleAppsConfigureCommands.cs | 2 +- .../Configuration/IOctopusIDConfigurationStore.cs | 3 +-- .../Configuration/OctopusIDConfiguration.cs | 2 +- .../Configuration/OctopusIDConfigurationResource.cs | 2 +- .../Configuration/OctopusIDConfigurationSettings.cs | 2 +- .../Configuration/OctopusIDConfigurationStore.cs | 2 +- .../Configuration/OctopusIDConfigureCommands.cs | 7 +------ .../Server.Okta/Configuration/OktaConfigurationSettings.cs | 2 +- source/Server.Okta/Configuration/OktaConfigurationStore.cs | 2 +- source/Server.Okta/Configuration/OktaConfigureCommands.cs | 2 +- ...onSettings.cs => OpenIDConnectConfigurationSettings.cs} | 4 ++-- .../Configuration/OpenIDConnectConfigurationStore.cs | 4 ++-- .../OpenIDConnectConfigurationWithRoleStore.cs | 4 ++-- .../Configuration/OpenIDConnectConfigureCommands.cs | 4 ++-- .../OpenIDConnectWithClientSecretConfigurationStore.cs | 2 +- 20 files changed, 24 insertions(+), 30 deletions(-) rename source/Server.OpenIDConnect.Common/Configuration/{OpenIdConnectConfigurationSettings.cs => OpenIDConnectConfigurationSettings.cs} (95%) diff --git a/source/Server.AzureAD/Configuration/AzureADConfigurationSettings.cs b/source/Server.AzureAD/Configuration/AzureADConfigurationSettings.cs index 934f728..9f05b52 100644 --- a/source/Server.AzureAD/Configuration/AzureADConfigurationSettings.cs +++ b/source/Server.AzureAD/Configuration/AzureADConfigurationSettings.cs @@ -4,7 +4,7 @@ namespace Octopus.Server.Extensibility.Authentication.AzureAD.Configuration { - class AzureADConfigurationSettings : OpenIdConnectConfigurationSettings, IAzureADConfigurationSettings + class AzureADConfigurationSettings : OpenIDConnectConfigurationSettings, IAzureADConfigurationSettings { public AzureADConfigurationSettings(IAzureADConfigurationStore configurationDocumentStore) : base(configurationDocumentStore) { diff --git a/source/Server.AzureAD/Configuration/AzureADConfigurationStore.cs b/source/Server.AzureAD/Configuration/AzureADConfigurationStore.cs index fc5de7d..5c666d6 100644 --- a/source/Server.AzureAD/Configuration/AzureADConfigurationStore.cs +++ b/source/Server.AzureAD/Configuration/AzureADConfigurationStore.cs @@ -3,7 +3,7 @@ namespace Octopus.Server.Extensibility.Authentication.AzureAD.Configuration { - class AzureADConfigurationStore : OpenIdConnectConfigurationWithRoleStore, IAzureADConfigurationStore + class AzureADConfigurationStore : OpenIDConnectConfigurationWithRoleStore, IAzureADConfigurationStore { public const string SingletonId = "authentication-aad"; diff --git a/source/Server.AzureAD/Configuration/AzureADConfigureCommands.cs b/source/Server.AzureAD/Configuration/AzureADConfigureCommands.cs index d16971b..a494459 100644 --- a/source/Server.AzureAD/Configuration/AzureADConfigureCommands.cs +++ b/source/Server.AzureAD/Configuration/AzureADConfigureCommands.cs @@ -7,7 +7,7 @@ namespace Octopus.Server.Extensibility.Authentication.AzureAD.Configuration { - class AzureADConfigureCommands : OpenIdConnectConfigureCommands + class AzureADConfigureCommands : OpenIDConnectConfigureCommands { public AzureADConfigureCommands( ISystemLog log, diff --git a/source/Server.GoogleApps/Configuration/GoogleAppsConfigurationSettings.cs b/source/Server.GoogleApps/Configuration/GoogleAppsConfigurationSettings.cs index edeaadd..94b0fe0 100644 --- a/source/Server.GoogleApps/Configuration/GoogleAppsConfigurationSettings.cs +++ b/source/Server.GoogleApps/Configuration/GoogleAppsConfigurationSettings.cs @@ -4,7 +4,7 @@ namespace Octopus.Server.Extensibility.Authentication.GoogleApps.Configuration { - class GoogleAppsConfigurationSettings : OpenIdConnectConfigurationSettings, IGoogleAppsConfigurationSettings + class GoogleAppsConfigurationSettings : OpenIDConnectConfigurationSettings, IGoogleAppsConfigurationSettings { public GoogleAppsConfigurationSettings(IGoogleAppsConfigurationStore configurationDocumentStore) : base(configurationDocumentStore) { diff --git a/source/Server.GoogleApps/Configuration/GoogleAppsConfigurationStore.cs b/source/Server.GoogleApps/Configuration/GoogleAppsConfigurationStore.cs index 3dfd0f7..db08fde 100644 --- a/source/Server.GoogleApps/Configuration/GoogleAppsConfigurationStore.cs +++ b/source/Server.GoogleApps/Configuration/GoogleAppsConfigurationStore.cs @@ -3,7 +3,7 @@ namespace Octopus.Server.Extensibility.Authentication.GoogleApps.Configuration { - class GoogleAppsConfigurationStore : OpenIdConnectConfigurationStore, IGoogleAppsConfigurationStore + class GoogleAppsConfigurationStore : OpenIDConnectConfigurationStore, IGoogleAppsConfigurationStore { public const string SingletonId = "authentication-googleapps"; diff --git a/source/Server.GoogleApps/Configuration/GoogleAppsConfigureCommands.cs b/source/Server.GoogleApps/Configuration/GoogleAppsConfigureCommands.cs index 2ec9c13..a061948 100644 --- a/source/Server.GoogleApps/Configuration/GoogleAppsConfigureCommands.cs +++ b/source/Server.GoogleApps/Configuration/GoogleAppsConfigureCommands.cs @@ -7,7 +7,7 @@ namespace Octopus.Server.Extensibility.Authentication.GoogleApps.Configuration { - class GoogleAppsConfigureCommands : OpenIdConnectConfigureCommands + class GoogleAppsConfigureCommands : OpenIDConnectConfigureCommands { public GoogleAppsConfigureCommands( ISystemLog log, diff --git a/source/Server.OctopusID/Configuration/IOctopusIDConfigurationStore.cs b/source/Server.OctopusID/Configuration/IOctopusIDConfigurationStore.cs index 4b33730..89121ee 100644 --- a/source/Server.OctopusID/Configuration/IOctopusIDConfigurationStore.cs +++ b/source/Server.OctopusID/Configuration/IOctopusIDConfigurationStore.cs @@ -2,8 +2,7 @@ namespace Octopus.Server.Extensibility.Authentication.OctopusID.Configuration { - interface IOctopusIDConfigurationStore : IOpenIDConnectWithClientSecretConfigurationStore, - IOpenIDConnectConfigurationWithRoleStore + interface IOctopusIDConfigurationStore : IOpenIDConnectConfigurationWithRoleStore { } } \ No newline at end of file diff --git a/source/Server.OctopusID/Configuration/OctopusIDConfiguration.cs b/source/Server.OctopusID/Configuration/OctopusIDConfiguration.cs index fce2918..16214a9 100644 --- a/source/Server.OctopusID/Configuration/OctopusIDConfiguration.cs +++ b/source/Server.OctopusID/Configuration/OctopusIDConfiguration.cs @@ -2,7 +2,7 @@ namespace Octopus.Server.Extensibility.Authentication.OctopusID.Configuration { - class OctopusIDConfiguration : OpenIDConnectConfigurationWithClientSecret, IOpenIDConnectConfigurationWithRole + class OctopusIDConfiguration : OpenIDConnectConfiguration, IOpenIDConnectConfigurationWithRole { public static string DefaultRoleClaimType = "roles"; diff --git a/source/Server.OctopusID/Configuration/OctopusIDConfigurationResource.cs b/source/Server.OctopusID/Configuration/OctopusIDConfigurationResource.cs index 82a4739..ff2b0bb 100644 --- a/source/Server.OctopusID/Configuration/OctopusIDConfigurationResource.cs +++ b/source/Server.OctopusID/Configuration/OctopusIDConfigurationResource.cs @@ -6,7 +6,7 @@ namespace Octopus.Server.Extensibility.Authentication.OctopusID.Configuration { [Description("Sign in to your Octopus Server with an Octopus ID. [Learn more](https://g.octopushq.com/AuthOctopusID).")] - class OctopusIDConfigurationResource : OpenIDConnectConfigurationWithClientSecretResource + class OctopusIDConfigurationResource : OpenIDConnectConfigurationResource { [Writeable] public override string? Issuer { get; set; } diff --git a/source/Server.OctopusID/Configuration/OctopusIDConfigurationSettings.cs b/source/Server.OctopusID/Configuration/OctopusIDConfigurationSettings.cs index 4ffa024..b54504a 100644 --- a/source/Server.OctopusID/Configuration/OctopusIDConfigurationSettings.cs +++ b/source/Server.OctopusID/Configuration/OctopusIDConfigurationSettings.cs @@ -5,7 +5,7 @@ namespace Octopus.Server.Extensibility.Authentication.OctopusID.Configuration { - class OctopusIDConfigurationSettings : OpenIdConnectConfigurationSettings, IOctopusIDConfigurationSettings + class OctopusIDConfigurationSettings : OpenIDConnectConfigurationSettings, IOctopusIDConfigurationSettings { public OctopusIDConfigurationSettings(IOctopusIDConfigurationStore configurationDocumentStore) : base(configurationDocumentStore) { diff --git a/source/Server.OctopusID/Configuration/OctopusIDConfigurationStore.cs b/source/Server.OctopusID/Configuration/OctopusIDConfigurationStore.cs index d2e211a..a2a3cfc 100644 --- a/source/Server.OctopusID/Configuration/OctopusIDConfigurationStore.cs +++ b/source/Server.OctopusID/Configuration/OctopusIDConfigurationStore.cs @@ -3,7 +3,7 @@ namespace Octopus.Server.Extensibility.Authentication.OctopusID.Configuration { - class OctopusIDConfigurationStore : OpenIDConnectWithClientSecretConfigurationStore, IOctopusIDConfigurationStore + class OctopusIDConfigurationStore : OpenIDConnectConfigurationStore, IOctopusIDConfigurationStore { public const string SingletonId = "authentication-octopusid"; diff --git a/source/Server.OctopusID/Configuration/OctopusIDConfigureCommands.cs b/source/Server.OctopusID/Configuration/OctopusIDConfigureCommands.cs index 2cd406c..0a7e8b8 100644 --- a/source/Server.OctopusID/Configuration/OctopusIDConfigureCommands.cs +++ b/source/Server.OctopusID/Configuration/OctopusIDConfigureCommands.cs @@ -8,7 +8,7 @@ namespace Octopus.Server.Extensibility.Authentication.OctopusID.Configuration { - class OctopusIDConfigureCommands : OpenIdConnectConfigureCommands + class OctopusIDConfigureCommands : OpenIDConnectConfigureCommands { public OctopusIDConfigureCommands( ISystemLog log, @@ -26,11 +26,6 @@ public override IEnumerable GetOptions() { yield return option; } - yield return new ConfigureCommandOption($"{ConfigurationSettingsName}ClientSecret=", "Tell Octopus the shared secret to use for Octopus ID authentication requests.", v => - { - ConfigurationStore.Value.SetClientSecret(v.ToSensitiveString()); - Log.Info($"{ConfigurationSettingsName} ClientSecret set"); - }, hide: true); } } } \ No newline at end of file diff --git a/source/Server.Okta/Configuration/OktaConfigurationSettings.cs b/source/Server.Okta/Configuration/OktaConfigurationSettings.cs index cb341d5..74b0c80 100644 --- a/source/Server.Okta/Configuration/OktaConfigurationSettings.cs +++ b/source/Server.Okta/Configuration/OktaConfigurationSettings.cs @@ -4,7 +4,7 @@ namespace Octopus.Server.Extensibility.Authentication.Okta.Configuration { - class OktaConfigurationSettings : OpenIdConnectConfigurationSettings, IOktaConfigurationSettings + class OktaConfigurationSettings : OpenIDConnectConfigurationSettings, IOktaConfigurationSettings { public OktaConfigurationSettings(IOktaConfigurationStore configurationDocumentStore) : base(configurationDocumentStore) { diff --git a/source/Server.Okta/Configuration/OktaConfigurationStore.cs b/source/Server.Okta/Configuration/OktaConfigurationStore.cs index 4253391..1667150 100644 --- a/source/Server.Okta/Configuration/OktaConfigurationStore.cs +++ b/source/Server.Okta/Configuration/OktaConfigurationStore.cs @@ -3,7 +3,7 @@ namespace Octopus.Server.Extensibility.Authentication.Okta.Configuration { - class OktaConfigurationStore : OpenIdConnectConfigurationWithRoleStore, IOktaConfigurationStore + class OktaConfigurationStore : OpenIDConnectConfigurationWithRoleStore, IOktaConfigurationStore { public const string SingletonId = "authentication-od"; diff --git a/source/Server.Okta/Configuration/OktaConfigureCommands.cs b/source/Server.Okta/Configuration/OktaConfigureCommands.cs index 7858d20..2f9e9dc 100644 --- a/source/Server.Okta/Configuration/OktaConfigureCommands.cs +++ b/source/Server.Okta/Configuration/OktaConfigureCommands.cs @@ -7,7 +7,7 @@ namespace Octopus.Server.Extensibility.Authentication.Okta.Configuration { - class OktaConfigureCommands : OpenIdConnectConfigureCommands + class OktaConfigureCommands : OpenIDConnectConfigureCommands { public OktaConfigureCommands( ISystemLog log, diff --git a/source/Server.OpenIDConnect.Common/Configuration/OpenIdConnectConfigurationSettings.cs b/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfigurationSettings.cs similarity index 95% rename from source/Server.OpenIDConnect.Common/Configuration/OpenIdConnectConfigurationSettings.cs rename to source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfigurationSettings.cs index fbe23fd..4cc6c26 100644 --- a/source/Server.OpenIDConnect.Common/Configuration/OpenIdConnectConfigurationSettings.cs +++ b/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfigurationSettings.cs @@ -5,12 +5,12 @@ namespace Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Configuration { - public abstract class OpenIdConnectConfigurationSettings : ExtensionConfigurationSettings + public abstract class OpenIDConnectConfigurationSettings : ExtensionConfigurationSettings where TConfiguration : OpenIDConnectConfiguration, new() where TResource : ExtensionConfigurationResource where TDocumentStore : IOpenIDConnectConfigurationStore { - protected OpenIdConnectConfigurationSettings(TDocumentStore configurationDocumentStore) : base(configurationDocumentStore) + protected OpenIDConnectConfigurationSettings(TDocumentStore configurationDocumentStore) : base(configurationDocumentStore) { } diff --git a/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfigurationStore.cs b/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfigurationStore.cs index ef4e591..b8d1fa5 100644 --- a/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfigurationStore.cs +++ b/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfigurationStore.cs @@ -5,12 +5,12 @@ namespace Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Configuration { - public abstract class OpenIdConnectConfigurationStore : ExtensionConfigurationStore, IOpenIDConnectConfigurationStore + public abstract class OpenIDConnectConfigurationStore : ExtensionConfigurationStore, IOpenIDConnectConfigurationStore where TConfiguration : OpenIDConnectConfiguration, IId, new() { public abstract string ConfigurationSettingsName { get; } - protected OpenIdConnectConfigurationStore( + protected OpenIDConnectConfigurationStore( IConfigurationStore configurationStore) : base(configurationStore) { } diff --git a/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfigurationWithRoleStore.cs b/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfigurationWithRoleStore.cs index 820cd57..1eded21 100644 --- a/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfigurationWithRoleStore.cs +++ b/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfigurationWithRoleStore.cs @@ -3,10 +3,10 @@ namespace Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Configuration { - public abstract class OpenIdConnectConfigurationWithRoleStore : OpenIdConnectConfigurationStore, IOpenIDConnectConfigurationWithRoleStore + public abstract class OpenIDConnectConfigurationWithRoleStore : OpenIDConnectConfigurationStore, IOpenIDConnectConfigurationWithRoleStore where TConfiguration : OpenIDConnectConfigurationWithRole, IId, new() { - protected OpenIdConnectConfigurationWithRoleStore(IConfigurationStore configurationStore) : base(configurationStore) + protected OpenIDConnectConfigurationWithRoleStore(IConfigurationStore configurationStore) : base(configurationStore) { } diff --git a/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfigureCommands.cs b/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfigureCommands.cs index 1c3201c..534275c 100644 --- a/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfigureCommands.cs +++ b/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfigureCommands.cs @@ -8,14 +8,14 @@ namespace Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Configuration { - public abstract class OpenIdConnectConfigureCommands : IContributeToConfigureCommand + public abstract class OpenIDConnectConfigureCommands : IContributeToConfigureCommand where TStore : IOpenIDConnectConfigurationStore { protected readonly ISystemLog Log; protected readonly Lazy ConfigurationStore; readonly Lazy webPortalConfigurationStore; - protected OpenIdConnectConfigureCommands( + protected OpenIDConnectConfigureCommands( ISystemLog log, Lazy configurationStore, Lazy webPortalConfigurationStore) diff --git a/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectWithClientSecretConfigurationStore.cs b/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectWithClientSecretConfigurationStore.cs index d479094..469010d 100644 --- a/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectWithClientSecretConfigurationStore.cs +++ b/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectWithClientSecretConfigurationStore.cs @@ -3,7 +3,7 @@ namespace Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Configuration { - public abstract class OpenIDConnectWithClientSecretConfigurationStore : OpenIdConnectConfigurationStore, IOpenIDConnectWithClientSecretConfigurationStore + public abstract class OpenIDConnectWithClientSecretConfigurationStore : OpenIDConnectConfigurationStore, IOpenIDConnectWithClientSecretConfigurationStore where TConfiguration : OpenIDConnectConfigurationWithClientSecret, IId, new() { protected OpenIDConnectWithClientSecretConfigurationStore(IConfigurationStore configurationStore) : base(configurationStore) From b54f2d02e2cae06b48ab30f15cc8fa7c393a18f0 Mon Sep 17 00:00:00 2001 From: Nelson Susanto Date: Thu, 17 Mar 2022 16:52:23 +1100 Subject: [PATCH 10/27] code challenge/verifier improvements --- .../Infrastructure/CodeChallenge.cs | 8 ++++---- .../Infrastructure/CodeVerifier.cs | 18 ++++++++++++++---- .../Issuer/AuthorizationEndpointUrlBuilder.cs | 4 ++-- .../Web/UserAuthenticationAction.cs | 4 ++-- 4 files changed, 22 insertions(+), 12 deletions(-) diff --git a/source/Server.OpenIDConnect.Common/Infrastructure/CodeChallenge.cs b/source/Server.OpenIDConnect.Common/Infrastructure/CodeChallenge.cs index f3af124..012aad7 100644 --- a/source/Server.OpenIDConnect.Common/Infrastructure/CodeChallenge.cs +++ b/source/Server.OpenIDConnect.Common/Infrastructure/CodeChallenge.cs @@ -1,17 +1,17 @@ using System; using System.Security.Cryptography; using System.Text; +using Microsoft.IdentityModel.Tokens; namespace Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Infrastructure { public static class CodeChallenge { - public static string CreateS256CodeChallenge(string codeVerifier) + public static string GenerateCodeChallenge(string codeVerifier) { using var sha = SHA256.Create(); - var codeChallenge = Convert.ToBase64String(sha.ComputeHash(Encoding.UTF8.GetBytes(codeVerifier))) - .TrimEnd('=').Replace("/", string.Empty).Replace("+", string.Empty); - return codeChallenge; + var challengeBytes = sha.ComputeHash(Encoding.UTF8.GetBytes(codeVerifier)); + return Base64UrlEncoder.Encode(challengeBytes); } } } \ No newline at end of file diff --git a/source/Server.OpenIDConnect.Common/Infrastructure/CodeVerifier.cs b/source/Server.OpenIDConnect.Common/Infrastructure/CodeVerifier.cs index da6c2f8..2f6885a 100644 --- a/source/Server.OpenIDConnect.Common/Infrastructure/CodeVerifier.cs +++ b/source/Server.OpenIDConnect.Common/Infrastructure/CodeVerifier.cs @@ -8,11 +8,21 @@ public static class CodeVerifier public static string? InMemoryCodeVerifier { get; private set; } static readonly RandomNumberGenerator Rng = RandomNumberGenerator.Create(); - public static string GenerateUrlSafeCodeVerifier() + public static string GenerateCodeVerifier(int size = 128) { - var data = new byte[32]; - Rng.GetBytes(data); - var codeVerifier = Convert.ToBase64String(data).TrimEnd('=').Replace("/", string.Empty).Replace("+", string.Empty); + if (size is < 43 or > 128) + size = 128; + + const string unreservedCharacters = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-._~"; + Random random = new(); + char[] highEntropyCryptograph = new char[size]; + + for (var i = 0; i < highEntropyCryptograph.Length; i++) + { + highEntropyCryptograph[i] = unreservedCharacters[random.Next(unreservedCharacters.Length)]; + } + + var codeVerifier = new string(highEntropyCryptograph); InMemoryCodeVerifier = codeVerifier; return codeVerifier; } diff --git a/source/Server.OpenIDConnect.Common/Issuer/AuthorizationEndpointUrlBuilder.cs b/source/Server.OpenIDConnect.Common/Issuer/AuthorizationEndpointUrlBuilder.cs index 83380b9..8ad1473 100644 --- a/source/Server.OpenIDConnect.Common/Issuer/AuthorizationEndpointUrlBuilder.cs +++ b/source/Server.OpenIDConnect.Common/Issuer/AuthorizationEndpointUrlBuilder.cs @@ -51,8 +51,8 @@ public virtual string Build(string requestDirectoryPath, IssuerConfiguration iss if (!string.IsNullOrWhiteSpace(codeChallenge)) { - url += $"&code_challenge={urlEncoder.UrlEncode(codeChallenge)}"; - url += $"&code_challenge_method=S256"; + url += $"&code_challenge={codeChallenge}"; + url += "&code_challenge_method=S256"; } return url; diff --git a/source/Server.OpenIDConnect.Common/Web/UserAuthenticationAction.cs b/source/Server.OpenIDConnect.Common/Web/UserAuthenticationAction.cs index 6ac44d0..c6c4554 100644 --- a/source/Server.OpenIDConnect.Common/Web/UserAuthenticationAction.cs +++ b/source/Server.OpenIDConnect.Common/Web/UserAuthenticationAction.cs @@ -92,8 +92,8 @@ public async Task ExecuteAsync(IOctoRequest request) IOctoResponseProvider BuildAuthorizationCodePkceResponse(LoginRedirectLinkRequestModel model, LoginState state, IssuerConfiguration issuerConfig) { - var codeVerifier = CodeVerifier.GenerateUrlSafeCodeVerifier(); - var codeChallenge = CodeChallenge.CreateS256CodeChallenge(codeVerifier); + var codeVerifier = CodeVerifier.GenerateCodeVerifier(); + var codeChallenge = CodeChallenge.GenerateCodeChallenge(codeVerifier); var stateString = JsonConvert.SerializeObject(state); var url = urlBuilder.Build(model.ApiAbsUrl, issuerConfig, state: stateString, codeChallenge: codeChallenge); From 8806827492bc4a5efab68e3611784cf7d6e24a68 Mon Sep 17 00:00:00 2001 From: Nelson Susanto Date: Thu, 17 Mar 2022 16:52:40 +1100 Subject: [PATCH 11/27] Remove nonce comparison from Pkce action --- .../Web/UserAuthenticatedPkceAction.cs | 37 ++++--------------- 1 file changed, 7 insertions(+), 30 deletions(-) diff --git a/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedPkceAction.cs b/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedPkceAction.cs index 5a2998c..d55d740 100644 --- a/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedPkceAction.cs +++ b/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedPkceAction.cs @@ -107,7 +107,8 @@ public async Task ExecuteAsync(IOctoRequest request) async Task Handle(string code, string state, IOctoRequest request) { - var redirectUri = $"{request.Scheme}://{request.Host}{ConfigurationStore.RedirectUri}"; + var host = request.Headers.ContainsKey("Host") ? request.Headers["Host"].Single() : request.Host; + var redirectUri = $"{request.Scheme}://{host}{ConfigurationStore.RedirectUri}"; var response = await RequestAuthToken(code, redirectUri); // Step 1: Try and get all of the details from the request making sure there are no errors passed back from the external identity provider @@ -137,38 +138,14 @@ async Task Handle(string code, string state, IOctoRequest var stateFromRequest = JsonConvert.DeserializeObject(stateStringFromRequest ?? string.Empty); - // Step 3: Validate the nonce is as we expected to prevent replay attacks - const string nonceDescription = "As a security precaution to prevent replay attacks, Octopus ensures the nonce returned in the claims from the external identity provider matches what it expected."; - - var expectedNonceHash = string.Empty; - if (request.Cookies.ContainsKey(UserAuthConstants.OctopusNonceCookieName)) - expectedNonceHash = encoder.UrlDecode(request.Cookies[UserAuthConstants.OctopusNonceCookieName]); - - if (string.IsNullOrWhiteSpace(expectedNonceHash)) - { - return BadRequest($"User login failed: Missing Nonce Hash Cookie. {nonceDescription} In this case the Cookie containing the SHA256 hash of the nonce is missing from the request."); - } - - var nonceFromClaims = principal.Claims.FirstOrDefault(c => c.Type == "nonce"); - if (nonceFromClaims == null) - { - return BadRequest($"User login failed: Missing Nonce Claim. {nonceDescription} In this case the 'nonce' claim is missing from the security token."); - } - - var nonceFromClaimsHash = Nonce.Protect(nonceFromClaims.Value); - if (nonceFromClaimsHash != expectedNonceHash) - { - return BadRequest($"User login failed: Tampered Nonce. {nonceDescription} In this case the nonce looks like it has been tampered with or reused. The nonce is '{nonceFromClaims}'. The SHA256 hash of the state was expected to be '{expectedNonceHash}' but was '{nonceFromClaimsHash}'."); - } - - // Step 4: Now the integrity of the request has been validated we can figure out which Octopus User this represents + // Step 3: Now the integrity of the request has been validated we can figure out which Octopus User this represents var authenticationCandidate = principalToUserResourceMapper.MapToUserResource(principal); if (authenticationCandidate.Username == null) { return BadRequest("Unable to determine username."); } - // Step 4a: Check if this authentication attempt is already being banned + // Step 3a: Check if this authentication attempt is already being banned var action = loginTracker.BeforeAttempt(authenticationCandidate.Username, request.Host); if (action == InvalidLoginAction.Ban) { @@ -177,7 +154,7 @@ async Task Handle(string code, string state, IOctoRequest using (var cts = new CancellationTokenSource(TimeSpan.FromMinutes(1))) { - // Step 4b: Try to get or create a the Octopus User this external identity represents + // Step 3b: Try to get or create a the Octopus User this external identity represents var userResult = GetOrCreateUser(authenticationCandidate, principalContainer.ExternalGroupIds, cts.Token); if (userResult is ISuccessResult successResult) { @@ -208,10 +185,10 @@ async Task Handle(string code, string state, IOctoRequest return octoResponse; } - // Step 5: Handle other types of failures + // Step 4: Handle other types of failures loginTracker.RecordFailure(authenticationCandidate.Username, request.Host); - // Step 5a: Slow this potential attacker down a bit since they seem to keep failing + // Step 4a: Slow this potential attacker down a bit since they seem to keep failing if (action == InvalidLoginAction.Slow) { sleep.For(1000); From 6ba54d3b0b5065d7a503f3e685a42b63ee08bd0e Mon Sep 17 00:00:00 2001 From: Nelson Susanto Date: Thu, 17 Mar 2022 16:52:47 +1100 Subject: [PATCH 12/27] Check if state exists --- .../Tokens/OpenIDConnectAuthTokenHandler.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/source/Server.OpenIDConnect.Common/Tokens/OpenIDConnectAuthTokenHandler.cs b/source/Server.OpenIDConnect.Common/Tokens/OpenIDConnectAuthTokenHandler.cs index e27db25..02ddfdd 100644 --- a/source/Server.OpenIDConnect.Common/Tokens/OpenIDConnectAuthTokenHandler.cs +++ b/source/Server.OpenIDConnect.Common/Tokens/OpenIDConnectAuthTokenHandler.cs @@ -43,7 +43,10 @@ public Task GetPrincipalAsync(IDictionary Date: Thu, 17 Mar 2022 16:52:57 +1100 Subject: [PATCH 13/27] Validate on id token --- source/Server.OpenIDConnect.Common/Tokens/AuthTokenHandler.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/Server.OpenIDConnect.Common/Tokens/AuthTokenHandler.cs b/source/Server.OpenIDConnect.Common/Tokens/AuthTokenHandler.cs index dbae003..68f1101 100644 --- a/source/Server.OpenIDConnect.Common/Tokens/AuthTokenHandler.cs +++ b/source/Server.OpenIDConnect.Common/Tokens/AuthTokenHandler.cs @@ -47,7 +47,7 @@ protected async Task GetPrincipalFromToken(string? acc { ValidateActor = true, ValidateAudience = true, - ValidAudience = issuer + "/resources", + ValidAudience = ConfigurationStore.GetClientId(), ValidateIssuer = true, ValidIssuer = issuerConfig.Issuer, ValidateIssuerSigningKey = true @@ -56,7 +56,7 @@ protected async Task GetPrincipalFromToken(string? acc if (!string.IsNullOrWhiteSpace(ConfigurationStore.GetNameClaimType())) validationParameters.NameClaimType = ConfigurationStore.GetNameClaimType(); - var tokenToValidate = accessToken; + var tokenToValidate = idToken; if (string.IsNullOrWhiteSpace(tokenToValidate)) { // if we're validating the id_token then the audience is based on the client_id, not the issuer/resource like access_token From 919df033b2fda1836c0ddfed06aad6d1c6ae10e3 Mon Sep 17 00:00:00 2001 From: Nelson Susanto Date: Fri, 18 Mar 2022 11:06:21 +1100 Subject: [PATCH 14/27] Clean up --- .../IOpenIDConnectConfigurationStore.cs | 1 + ...nIDConnectConfigurationWithClientSecret.cs | 17 -------------- ...ctConfigurationWithClientSecretResource.cs | 14 ----------- ...nnectWithClientSecretConfigurationStore.cs | 23 ------------------- .../Infrastructure/CodeChallenge.cs | 17 -------------- .../{CodeVerifier.cs => Pkce.cs} | 16 +++++++++---- .../Issuer/AuthorizationEndpointUrlBuilder.cs | 4 ---- .../OpenIDConnectAuthenticationProvider.cs | 2 -- .../Tokens/AuthTokenHandler.cs | 20 ++++------------ .../Web/UserAuthenticatedPkceAction.cs | 18 +++++++-------- .../Web/UserAuthenticationAction.cs | 5 ++-- 11 files changed, 29 insertions(+), 108 deletions(-) delete mode 100644 source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfigurationWithClientSecret.cs delete mode 100644 source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfigurationWithClientSecretResource.cs delete mode 100644 source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectWithClientSecretConfigurationStore.cs delete mode 100644 source/Server.OpenIDConnect.Common/Infrastructure/CodeChallenge.cs rename source/Server.OpenIDConnect.Common/Infrastructure/{CodeVerifier.cs => Pkce.cs} (55%) diff --git a/source/Server.OpenIDConnect.Common/Configuration/IOpenIDConnectConfigurationStore.cs b/source/Server.OpenIDConnect.Common/Configuration/IOpenIDConnectConfigurationStore.cs index 527a295..87b5289 100644 --- a/source/Server.OpenIDConnect.Common/Configuration/IOpenIDConnectConfigurationStore.cs +++ b/source/Server.OpenIDConnect.Common/Configuration/IOpenIDConnectConfigurationStore.cs @@ -17,6 +17,7 @@ public interface IOpenIDConnectConfigurationStore : IExtensionConfigurationStore string? GetClientId(); void SetClientId(string? clientId); + SensitiveString? GetClientSecret(); void SetClientSecret(SensitiveString? clientSecret); bool HasClientSecret { get; } diff --git a/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfigurationWithClientSecret.cs b/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfigurationWithClientSecret.cs deleted file mode 100644 index 51ff55c..0000000 --- a/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfigurationWithClientSecret.cs +++ /dev/null @@ -1,17 +0,0 @@ -using Octopus.Data.Model; - -namespace Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Configuration -{ - public abstract class OpenIDConnectConfigurationWithClientSecret : OpenIDConnectConfiguration, IOpenIDConnectConfigurationWithClientSecret - { - protected OpenIDConnectConfigurationWithClientSecret(string id) : base(id) - { - } - - protected OpenIDConnectConfigurationWithClientSecret(string id, string name, string author, string configurationSchemaVersion) : base(id, name, author, configurationSchemaVersion) - { - } - - public SensitiveString? ClientSecret { get; set; } - } -} \ No newline at end of file diff --git a/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfigurationWithClientSecretResource.cs b/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfigurationWithClientSecretResource.cs deleted file mode 100644 index 5f53df3..0000000 --- a/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfigurationWithClientSecretResource.cs +++ /dev/null @@ -1,14 +0,0 @@ -using System.ComponentModel; -using Octopus.Server.MessageContracts; -using Octopus.Server.MessageContracts.Attributes; - -namespace Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Configuration -{ - public class OpenIDConnectConfigurationWithClientSecretResource : OpenIDConnectConfigurationResource - { - [DisplayName("Client Secret")] - [Description("Shared secret for validating the authentication tokens")] - [Writeable] - public virtual SensitiveValue? ClientSecret { get; set; } - } -} \ No newline at end of file diff --git a/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectWithClientSecretConfigurationStore.cs b/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectWithClientSecretConfigurationStore.cs deleted file mode 100644 index 469010d..0000000 --- a/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectWithClientSecretConfigurationStore.cs +++ /dev/null @@ -1,23 +0,0 @@ -using Octopus.Data.Model; -using Octopus.Data.Storage.Configuration; - -namespace Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Configuration -{ - public abstract class OpenIDConnectWithClientSecretConfigurationStore : OpenIDConnectConfigurationStore, IOpenIDConnectWithClientSecretConfigurationStore - where TConfiguration : OpenIDConnectConfigurationWithClientSecret, IId, new() - { - protected OpenIDConnectWithClientSecretConfigurationStore(IConfigurationStore configurationStore) : base(configurationStore) - { - } - - public SensitiveString? GetClientSecret() - { - return GetProperty(doc => doc.ClientSecret); - } - - public void SetClientSecret(SensitiveString? clientSecret) - { - SetProperty(doc => doc.ClientSecret = clientSecret); - } - } -} \ No newline at end of file diff --git a/source/Server.OpenIDConnect.Common/Infrastructure/CodeChallenge.cs b/source/Server.OpenIDConnect.Common/Infrastructure/CodeChallenge.cs deleted file mode 100644 index 012aad7..0000000 --- a/source/Server.OpenIDConnect.Common/Infrastructure/CodeChallenge.cs +++ /dev/null @@ -1,17 +0,0 @@ -using System; -using System.Security.Cryptography; -using System.Text; -using Microsoft.IdentityModel.Tokens; - -namespace Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Infrastructure -{ - public static class CodeChallenge - { - public static string GenerateCodeChallenge(string codeVerifier) - { - using var sha = SHA256.Create(); - var challengeBytes = sha.ComputeHash(Encoding.UTF8.GetBytes(codeVerifier)); - return Base64UrlEncoder.Encode(challengeBytes); - } - } -} \ No newline at end of file diff --git a/source/Server.OpenIDConnect.Common/Infrastructure/CodeVerifier.cs b/source/Server.OpenIDConnect.Common/Infrastructure/Pkce.cs similarity index 55% rename from source/Server.OpenIDConnect.Common/Infrastructure/CodeVerifier.cs rename to source/Server.OpenIDConnect.Common/Infrastructure/Pkce.cs index 2f6885a..cb1694c 100644 --- a/source/Server.OpenIDConnect.Common/Infrastructure/CodeVerifier.cs +++ b/source/Server.OpenIDConnect.Common/Infrastructure/Pkce.cs @@ -1,30 +1,38 @@ using System; using System.Security.Cryptography; +using System.Text; +using Microsoft.IdentityModel.Tokens; namespace Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Infrastructure { - public static class CodeVerifier + public static class Pkce { + const string UnreservedCharacters = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-._~"; public static string? InMemoryCodeVerifier { get; private set; } - static readonly RandomNumberGenerator Rng = RandomNumberGenerator.Create(); public static string GenerateCodeVerifier(int size = 128) { if (size is < 43 or > 128) size = 128; - const string unreservedCharacters = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-._~"; Random random = new(); char[] highEntropyCryptograph = new char[size]; for (var i = 0; i < highEntropyCryptograph.Length; i++) { - highEntropyCryptograph[i] = unreservedCharacters[random.Next(unreservedCharacters.Length)]; + highEntropyCryptograph[i] = UnreservedCharacters[random.Next(UnreservedCharacters.Length)]; } var codeVerifier = new string(highEntropyCryptograph); InMemoryCodeVerifier = codeVerifier; return codeVerifier; } + + public static string GenerateCodeChallenge(string codeVerifier) + { + using var sha = SHA256.Create(); + var challengeBytes = sha.ComputeHash(Encoding.UTF8.GetBytes(codeVerifier)); + return Base64UrlEncoder.Encode(challengeBytes); + } } } \ No newline at end of file diff --git a/source/Server.OpenIDConnect.Common/Issuer/AuthorizationEndpointUrlBuilder.cs b/source/Server.OpenIDConnect.Common/Issuer/AuthorizationEndpointUrlBuilder.cs index 8ad1473..7d0cf67 100644 --- a/source/Server.OpenIDConnect.Common/Issuer/AuthorizationEndpointUrlBuilder.cs +++ b/source/Server.OpenIDConnect.Common/Issuer/AuthorizationEndpointUrlBuilder.cs @@ -36,19 +36,15 @@ public virtual string Build(string requestDirectoryPath, IssuerConfiguration iss if (!ConfigurationStore.HasClientSecret) { url += $"&response_mode={responseMode}"; - } - if (!string.IsNullOrWhiteSpace(state)) { url += $"&state={urlEncoder.UrlEncode(state)}"; } - if (!string.IsNullOrWhiteSpace(nonce)) { url += $"&nonce={nonce}"; } - if (!string.IsNullOrWhiteSpace(codeChallenge)) { url += $"&code_challenge={codeChallenge}"; diff --git a/source/Server.OpenIDConnect.Common/OpenIDConnectAuthenticationProvider.cs b/source/Server.OpenIDConnect.Common/OpenIDConnectAuthenticationProvider.cs index 76bc446..27cac18 100644 --- a/source/Server.OpenIDConnect.Common/OpenIDConnectAuthenticationProvider.cs +++ b/source/Server.OpenIDConnect.Common/OpenIDConnectAuthenticationProvider.cs @@ -48,8 +48,6 @@ bool IsProviderConfigComplete() public string AuthenticateUri => $"/users/authenticate/{ConfigurationStore.ConfigurationSettingsName}"; - public string AuthenticatePkceUri => $"/users/authenticate-pkce/{ConfigurationStore.ConfigurationSettingsName}"; - public AuthenticationProviderElement GetAuthenticationProviderElement() { var authenticationProviderElement = new AuthenticationProviderElement diff --git a/source/Server.OpenIDConnect.Common/Tokens/AuthTokenHandler.cs b/source/Server.OpenIDConnect.Common/Tokens/AuthTokenHandler.cs index 68f1101..89c0297 100644 --- a/source/Server.OpenIDConnect.Common/Tokens/AuthTokenHandler.cs +++ b/source/Server.OpenIDConnect.Common/Tokens/AuthTokenHandler.cs @@ -56,34 +56,22 @@ protected async Task GetPrincipalFromToken(string? acc if (!string.IsNullOrWhiteSpace(ConfigurationStore.GetNameClaimType())) validationParameters.NameClaimType = ConfigurationStore.GetNameClaimType(); - var tokenToValidate = idToken; - if (string.IsNullOrWhiteSpace(tokenToValidate)) - { - // if we're validating the id_token then the audience is based on the client_id, not the issuer/resource like access_token - tokenToValidate = idToken; - validationParameters.ValidAudience = ConfigurationStore.GetClientId(); - } - SetIssuerSpecificTokenValidationParameters(validationParameters); var jwt = new JwtSecurityToken(idToken); if (hmacAlgorithms.Contains(jwt.Header.Alg)) { - principal = ValidateUsingSharedSecret(validationParameters, tokenToValidate); + principal = ValidateUsingSharedSecret(validationParameters, idToken); } else { - principal = await ValidateUsingIssuerCertificate(validationParameters, tokenToValidate, issuerConfig); + principal = await ValidateUsingIssuerCertificate(validationParameters, idToken, issuerConfig); } - var error = string.Empty; - DoIssuerSpecificClaimsValidation(principal, out error); - - if (string.IsNullOrWhiteSpace(error)) - return new ClaimsPrincipleContainer(principal, GetProviderGroupIds(principal)); + DoIssuerSpecificClaimsValidation(principal, out string error); - return new ClaimsPrincipleContainer(error); + return string.IsNullOrWhiteSpace(error) ? new ClaimsPrincipleContainer(principal, GetProviderGroupIds(principal)) : new ClaimsPrincipleContainer(error); } ClaimsPrincipal ValidateUsingSharedSecret(TokenValidationParameters validationParameters, string? tokenToValidate) diff --git a/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedPkceAction.cs b/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedPkceAction.cs index d55d740..d96ac9c 100644 --- a/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedPkceAction.cs +++ b/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedPkceAction.cs @@ -30,11 +30,11 @@ public abstract class UserAuthenticatedPkceAction Code = new("code", "Authorization code provided by the identity provider"); - static readonly OptionalQueryParameterProperty State = new("state", "The state value associated with the authentication session"); + readonly RequiredQueryParameterProperty codeParameter = new("code", "Authorization code provided by the identity provider"); + readonly OptionalQueryParameterProperty stateParameter = new("state", "The state value associated with the authentication session"); readonly ISystemLog log; readonly TAuthTokenHandler authTokenHandler; @@ -82,7 +82,7 @@ protected UserAuthenticatedPkceAction( public async Task ExecuteAsync(IOctoRequest request) { - return await request.HandleAsync(Code, State, (code, state) => Handle(code, state, request)); + return await request.HandleAsync(codeParameter, stateParameter, (code, state) => Handle(code, state, request)); } async Task> RequestAuthToken(string code, string redirectUri) @@ -97,12 +97,12 @@ public async Task ExecuteAsync(IOctoRequest request) ["redirect_uri"] = redirectUri, ["client_id"] = ConfigurationStore.GetClientId()!, ["client_secret"] = ConfigurationStore.GetClientSecret()!.Value, - ["code_verifier"] = CodeVerifier.InMemoryCodeVerifier! + ["code_verifier"] = Pkce.InMemoryCodeVerifier! }; request.Content = new FormUrlEncodedContent(formValues); var response = await client.SendAsync(request, HttpCompletionOption.ResponseContentRead); var body = await response.Content.ReadAsStringAsync(); - return JsonConvert.DeserializeObject>(body); + return JsonConvert.DeserializeObject>(body)!; } async Task Handle(string code, string state, IOctoRequest request) @@ -170,7 +170,7 @@ async Task Handle(string code, string state, IOctoRequest return BadRequest($"The Octopus User Account '{authenticationCandidate.Username}' is a Service Account, which are prevented from using Octopus interactively. Service Accounts are designed to authorize external systems to access the Octopus API using an API Key."); } - var octoResponse = Redirect.Response(stateFromRequest.RedirectAfterLoginTo) + var octoResponse = redirect.Response(stateFromRequest.RedirectAfterLoginTo) .WithHeader("Expires", new[] {DateTime.UtcNow.AddYears(1).ToString("R", DateTimeFormatInfo.InvariantInfo)}) .WithCookie(new OctoCookie(UserAuthConstants.OctopusStateCookieName, Guid.NewGuid().ToString()) {HttpOnly = true, Secure = false, Expires = DateTimeOffset.MinValue}) .WithCookie(new OctoCookie(UserAuthConstants.OctopusNonceCookieName, Guid.NewGuid().ToString()) {HttpOnly = true, Secure = false, Expires = DateTimeOffset.MinValue}); @@ -201,7 +201,7 @@ async Task Handle(string code, string state, IOctoRequest IOctoResponseProvider BadRequest(string message) { log.Error(message); - return LoginFailed.Response(message); + return loginFailed.Response(message); } IResultFromExtension GetOrCreateUser(UserResource userResource, string[] groups, CancellationToken cancellationToken) diff --git a/source/Server.OpenIDConnect.Common/Web/UserAuthenticationAction.cs b/source/Server.OpenIDConnect.Common/Web/UserAuthenticationAction.cs index c6c4554..4b1a660 100644 --- a/source/Server.OpenIDConnect.Common/Web/UserAuthenticationAction.cs +++ b/source/Server.OpenIDConnect.Common/Web/UserAuthenticationAction.cs @@ -92,8 +92,8 @@ public async Task ExecuteAsync(IOctoRequest request) IOctoResponseProvider BuildAuthorizationCodePkceResponse(LoginRedirectLinkRequestModel model, LoginState state, IssuerConfiguration issuerConfig) { - var codeVerifier = CodeVerifier.GenerateCodeVerifier(); - var codeChallenge = CodeChallenge.GenerateCodeChallenge(codeVerifier); + var codeVerifier = Pkce.GenerateCodeVerifier(); + var codeChallenge = Pkce.GenerateCodeChallenge(codeVerifier); var stateString = JsonConvert.SerializeObject(state); var url = urlBuilder.Build(model.ApiAbsUrl, issuerConfig, state: stateString, codeChallenge: codeChallenge); @@ -104,6 +104,7 @@ IOctoResponseProvider BuildAuthorizationCodePkceResponse(LoginRedirectLinkReques IOctoResponseProvider BuildHybridResponse(LoginRedirectLinkRequestModel model, LoginState state, IssuerConfiguration issuerConfig) { + // Use a non-deterministic nonce to prevent replay attacks var nonce = Nonce.GenerateUrlSafeNonce(); var stateString = JsonConvert.SerializeObject(state); var url = urlBuilder.Build(model.ApiAbsUrl, issuerConfig, nonce, stateString); From 5df8a3726dc4608a3cd8ec6bb5e888b5b3d4834d Mon Sep 17 00:00:00 2001 From: Nelson Susanto Date: Fri, 18 Mar 2022 16:33:36 +1100 Subject: [PATCH 15/27] Updates for Okta --- source/Server.Okta/OktaApi.cs | 5 ++- source/Server.Okta/OktaExtension.cs | 1 + .../Web/OktaUserAuthenticatedPkceAction.cs | 34 +++++++++++++++++++ 3 files changed, 37 insertions(+), 3 deletions(-) create mode 100644 source/Server.Okta/Web/OktaUserAuthenticatedPkceAction.cs diff --git a/source/Server.Okta/OktaApi.cs b/source/Server.Okta/OktaApi.cs index 83b7392..33830b8 100644 --- a/source/Server.Okta/OktaApi.cs +++ b/source/Server.Okta/OktaApi.cs @@ -1,9 +1,7 @@ -using System; -using Octopus.Server.Extensibility.Authentication.Okta.Configuration; +using Octopus.Server.Extensibility.Authentication.Okta.Configuration; using Octopus.Server.Extensibility.Authentication.Okta.Identities; using Octopus.Server.Extensibility.Authentication.Okta.Tokens; using Octopus.Server.Extensibility.Authentication.Okta.Web; -using Octopus.Server.Extensibility.Authentication.OpenIDConnect; using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common; using Octopus.Server.Extensibility.Extensions.Infrastructure.Web.Api; @@ -16,6 +14,7 @@ public OktaApi(IOktaConfigurationStore configurationStore, OktaAuthenticationPro { Add("POST", authenticationProvider.AuthenticateUri, RouteCategory.Raw, new AnonymousWhenEnabledEndpointInvocation(), null, "OpenIDConnect"); Add("POST", configurationStore.RedirectUri, RouteCategory.Raw, new AnonymousWhenEnabledEndpointInvocation(), null, "OpenIDConnect"); + Add("GET", configurationStore.RedirectUri, RouteCategory.Raw, new AnonymousWhenEnabledEndpointInvocation(), null, "OpenIDConnect"); } } } \ No newline at end of file diff --git a/source/Server.Okta/OktaExtension.cs b/source/Server.Okta/OktaExtension.cs index 17f0f93..0c3c1b8 100644 --- a/source/Server.Okta/OktaExtension.cs +++ b/source/Server.Okta/OktaExtension.cs @@ -62,6 +62,7 @@ public override void Load(ContainerBuilder builder) builder.RegisterType().As().InstancePerDependency(); builder.RegisterType().AsSelf().InstancePerDependency(); + builder.RegisterType().AsSelf().InstancePerDependency(); builder.RegisterType().AsSelf().InstancePerDependency(); builder.RegisterType() diff --git a/source/Server.Okta/Web/OktaUserAuthenticatedPkceAction.cs b/source/Server.Okta/Web/OktaUserAuthenticatedPkceAction.cs new file mode 100644 index 0000000..4ef22bd --- /dev/null +++ b/source/Server.Okta/Web/OktaUserAuthenticatedPkceAction.cs @@ -0,0 +1,34 @@ +using Octopus.Diagnostics; +using Octopus.Server.Extensibility.Authentication.HostServices; +using Octopus.Server.Extensibility.Authentication.Okta.Configuration; +using Octopus.Server.Extensibility.Authentication.Okta.Identities; +using Octopus.Server.Extensibility.Authentication.Okta.Tokens; +using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Infrastructure; +using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Issuer; +using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Web; +using Octopus.Server.Extensibility.HostServices.Web; +using Octopus.Time; + +namespace Octopus.Server.Extensibility.Authentication.Okta.Web +{ + class OktaUserAuthenticatedPkceAction + : UserAuthenticatedPkceAction + { + public OktaUserAuthenticatedPkceAction( + ISystemLog log, + IOktaAuthTokenHandler authTokenHandler, + IPrincipalToUserResourceMapper principalToUserResourceMapper, + IUpdateableUserStore userStore, + IOktaConfigurationStore configurationStore, + IAuthCookieCreator authCookieCreator, + IInvalidLoginTracker loginTracker, + ISleep sleep, + IOktaIdentityCreator identityCreator, + IClock clock, IUrlEncoder encoder, IIdentityProviderConfigDiscoverer identityProviderConfigDiscoverer) + : base(log, authTokenHandler, principalToUserResourceMapper, userStore, configurationStore, authCookieCreator, loginTracker, sleep, identityCreator, clock, encoder, identityProviderConfigDiscoverer) + { + } + + protected override string ProviderName => OktaAuthenticationProvider.ProviderName; + } +} \ No newline at end of file From 8e1543687dc67afdf72fc8638ec65ebca0fa090e Mon Sep 17 00:00:00 2001 From: Nelson Susanto Date: Tue, 22 Mar 2022 14:12:34 +1100 Subject: [PATCH 16/27] AAD and OctopusID classes --- source/Server.AzureAD/AzureADApi.cs | 1 + source/Server.AzureAD/AzureADExtension.cs | 1 + source/Server.OctopusID/OctopusIDApi.cs | 1 + source/Server.OctopusID/OctopusIDExtension.cs | 1 + 4 files changed, 4 insertions(+) diff --git a/source/Server.AzureAD/AzureADApi.cs b/source/Server.AzureAD/AzureADApi.cs index 5cc1a15..91ca208 100644 --- a/source/Server.AzureAD/AzureADApi.cs +++ b/source/Server.AzureAD/AzureADApi.cs @@ -16,6 +16,7 @@ public AzureADApi( { Add("POST", authenticationProvider.AuthenticateUri, RouteCategory.Raw, new AnonymousWhenEnabledEndpointInvocation(), null, "OpenIDConnect"); Add("POST", configurationStore.RedirectUri, RouteCategory.Raw, new AnonymousWhenEnabledEndpointInvocation(), null, "OpenIDConnect"); + Add("GET", configurationStore.RedirectUri, RouteCategory.Raw, new AnonymousWhenEnabledEndpointInvocation(), null, "OpenIDConnect"); } } } \ No newline at end of file diff --git a/source/Server.AzureAD/AzureADExtension.cs b/source/Server.AzureAD/AzureADExtension.cs index 6ade59d..71bf19d 100644 --- a/source/Server.AzureAD/AzureADExtension.cs +++ b/source/Server.AzureAD/AzureADExtension.cs @@ -60,6 +60,7 @@ public override void Load(ContainerBuilder builder) builder.RegisterType().As().InstancePerDependency(); builder.RegisterType().AsSelf().InstancePerDependency(); + builder.RegisterType().AsSelf().InstancePerDependency(); builder.RegisterType().AsSelf().InstancePerDependency(); builder.RegisterType() diff --git a/source/Server.OctopusID/OctopusIDApi.cs b/source/Server.OctopusID/OctopusIDApi.cs index 16e2ac0..be10818 100644 --- a/source/Server.OctopusID/OctopusIDApi.cs +++ b/source/Server.OctopusID/OctopusIDApi.cs @@ -16,6 +16,7 @@ public OctopusIDApi( { Add("POST", authenticationProvider.AuthenticateUri, RouteCategory.Raw, new AnonymousWhenEnabledEndpointInvocation(), null, "OpenIDConnect"); Add("POST", configurationStore.RedirectUri, RouteCategory.Raw, new AnonymousWhenEnabledEndpointInvocation(), null, "OpenIDConnect"); + Add("GET", configurationStore.RedirectUri, RouteCategory.Raw, new AnonymousWhenEnabledEndpointInvocation(), null, "OpenIDConnect"); } } } \ No newline at end of file diff --git a/source/Server.OctopusID/OctopusIDExtension.cs b/source/Server.OctopusID/OctopusIDExtension.cs index 34a6086..dadc5eb 100644 --- a/source/Server.OctopusID/OctopusIDExtension.cs +++ b/source/Server.OctopusID/OctopusIDExtension.cs @@ -61,6 +61,7 @@ public override void Load(ContainerBuilder builder) builder.RegisterType().As().InstancePerDependency(); builder.RegisterType().AsSelf().InstancePerDependency(); + builder.RegisterType().AsSelf().InstancePerDependency(); builder.RegisterType().AsSelf().InstancePerDependency(); builder.RegisterType() From 32740b12e040abda9eba6e77306a4c45f6b9d757 Mon Sep 17 00:00:00 2001 From: Nelson Susanto Date: Tue, 22 Mar 2022 14:31:06 +1100 Subject: [PATCH 17/27] remove old store --- .../IOpenIDConnectConfigurationWithClientSecret.cs | 9 --------- 1 file changed, 9 deletions(-) delete mode 100644 source/Server.OpenIDConnect.Common/Configuration/IOpenIDConnectConfigurationWithClientSecret.cs diff --git a/source/Server.OpenIDConnect.Common/Configuration/IOpenIDConnectConfigurationWithClientSecret.cs b/source/Server.OpenIDConnect.Common/Configuration/IOpenIDConnectConfigurationWithClientSecret.cs deleted file mode 100644 index 4bd96d7..0000000 --- a/source/Server.OpenIDConnect.Common/Configuration/IOpenIDConnectConfigurationWithClientSecret.cs +++ /dev/null @@ -1,9 +0,0 @@ -using Octopus.Data.Model; - -namespace Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Configuration -{ - public interface IOpenIDConnectConfigurationWithClientSecret : IOpenIDConnectConfiguration - { - SensitiveString? ClientSecret { get; set; } - } -} \ No newline at end of file From a415e81c7de7a98d9f9d08ee126df6193a79b70c Mon Sep 17 00:00:00 2001 From: Nelson Susanto Date: Tue, 22 Mar 2022 14:38:29 +1100 Subject: [PATCH 18/27] Add actions --- .../Web/AzureADUserAuthenticatedPkceAction.cs | 47 +++++++++++++++++++ .../OctopusIDUserAuthenticatedPkceAction.cs | 34 ++++++++++++++ 2 files changed, 81 insertions(+) create mode 100644 source/Server.AzureAD/Web/AzureADUserAuthenticatedPkceAction.cs create mode 100644 source/Server.OctopusID/Web/OctopusIDUserAuthenticatedPkceAction.cs diff --git a/source/Server.AzureAD/Web/AzureADUserAuthenticatedPkceAction.cs b/source/Server.AzureAD/Web/AzureADUserAuthenticatedPkceAction.cs new file mode 100644 index 0000000..90caceb --- /dev/null +++ b/source/Server.AzureAD/Web/AzureADUserAuthenticatedPkceAction.cs @@ -0,0 +1,47 @@ +using Octopus.Diagnostics; +using Octopus.Server.Extensibility.Authentication.AzureAD.Configuration; +using Octopus.Server.Extensibility.Authentication.AzureAD.Identities; +using Octopus.Server.Extensibility.Authentication.AzureAD.Infrastructure; +using Octopus.Server.Extensibility.Authentication.AzureAD.Tokens; +using Octopus.Server.Extensibility.Authentication.HostServices; +using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Issuer; +using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Web; +using Octopus.Server.Extensibility.HostServices.Web; +using Octopus.Time; + +namespace Octopus.Server.Extensibility.Authentication.AzureAD.Web +{ + class AzureADUserAuthenticatedPkceAction : UserAuthenticatedPkceAction + { + public AzureADUserAuthenticatedPkceAction( + ISystemLog log, + IAzureADAuthTokenHandler authTokenHandler, + IAzureADPrincipalToUserResourceMapper principalToUserResourceMapper, + IUpdateableUserStore userStore, + IAzureADConfigurationStore configurationStore, + IAuthCookieCreator authCookieCreator, + IInvalidLoginTracker loginTracker, + ISleep sleep, + IAzureADIdentityCreator identityCreator, + IClock clock, + IUrlEncoder encoder, + IIdentityProviderConfigDiscoverer identityProviderConfigDiscoverer) : + base( + log, + authTokenHandler, + principalToUserResourceMapper, + userStore, + configurationStore, + authCookieCreator, + loginTracker, + sleep, + identityCreator, + clock, + encoder, + identityProviderConfigDiscoverer) + { + } + + protected override string ProviderName => AzureADAuthenticationProvider.ProviderName; + } +} \ No newline at end of file diff --git a/source/Server.OctopusID/Web/OctopusIDUserAuthenticatedPkceAction.cs b/source/Server.OctopusID/Web/OctopusIDUserAuthenticatedPkceAction.cs new file mode 100644 index 0000000..b2bea7f --- /dev/null +++ b/source/Server.OctopusID/Web/OctopusIDUserAuthenticatedPkceAction.cs @@ -0,0 +1,34 @@ +using Octopus.Diagnostics; +using Octopus.Server.Extensibility.Authentication.HostServices; +using Octopus.Server.Extensibility.Authentication.OctopusID.Configuration; +using Octopus.Server.Extensibility.Authentication.OctopusID.Identities; +using Octopus.Server.Extensibility.Authentication.OctopusID.Tokens; +using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Infrastructure; +using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Issuer; +using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Web; +using Octopus.Server.Extensibility.HostServices.Web; +using Octopus.Time; + +namespace Octopus.Server.Extensibility.Authentication.OctopusID.Web +{ + class OctopusIDUserAuthenticatedPkceAction + : UserAuthenticatedPkceAction + { + public OctopusIDUserAuthenticatedPkceAction( + ISystemLog log, + IOctopusIDAuthTokenHandler authTokenHandler, + IPrincipalToUserResourceMapper principalToUserResourceMapper, + IUpdateableUserStore userStore, + IOctopusIDConfigurationStore configurationStore, + IAuthCookieCreator authCookieCreator, + IInvalidLoginTracker loginTracker, + ISleep sleep, + IOctopusIDIdentityCreator identityCreator, + IClock clock, IUrlEncoder encoder, IIdentityProviderConfigDiscoverer identityProviderConfigDiscoverer) + : base(log, authTokenHandler, principalToUserResourceMapper, userStore, configurationStore, authCookieCreator, loginTracker, sleep, identityCreator, clock, encoder, identityProviderConfigDiscoverer) + { + } + + protected override string ProviderName => OctopusIDAuthenticationProvider.ProviderName; + } +} \ No newline at end of file From 096f66c78ed95c5237f79e2dec7b18c09580d8ae Mon Sep 17 00:00:00 2001 From: Nelson Susanto Date: Tue, 29 Mar 2022 11:23:57 +1100 Subject: [PATCH 19/27] Use blob storage to store code verifier --- .../OctopusIDConfigurationResource.cs | 2 +- .../Client.OpenIDConnect.csproj | 4 +- .../OpenIDConnectConfigurationResource.cs | 6 +++ ...ctConfigurationWithClientSecretResource.cs | 15 -------- .../Web/AzureADUserAuthenticatedPkceAction.cs | 6 ++- .../Web/AzureADUserAuthenticationAction.cs | 4 +- .../GoogleAppsUserAuthenticatedPkceAction.cs | 5 ++- .../Web/GoogleAppsUserAuthenticationAction.cs | 6 ++- .../OctopusIDUserAuthenticatedPkceAction.cs | 5 ++- .../Web/OctopusIDUserAuthenticationAction.cs | 4 +- .../Web/OktaUserAuthenticatedPkceAction.cs | 5 ++- .../Web/OktaUserAuthenticationAction.cs | 4 +- ...nnectWithClientSecretConfigurationStore.cs | 15 -------- .../Infrastructure/Pkce.cs | 2 - .../Infrastructure/PkceBlob.cs | 18 +++++++++ .../Server.OpenIDConnect.Common.csproj | 1 + .../Web/LoginStateWithSessionId.cs | 17 +++++++++ .../Web/UserAuthenticatedPkceAction.cs | 38 +++++++++++++------ .../Web/UserAuthenticationAction.cs | 20 +++++++--- 19 files changed, 112 insertions(+), 65 deletions(-) delete mode 100644 source/Client.OpenIDConnect/Configuration/OpenIDConnectConfigurationWithClientSecretResource.cs delete mode 100644 source/Server.OpenIDConnect.Common/Configuration/IOpenIDConnectWithClientSecretConfigurationStore.cs create mode 100644 source/Server.OpenIDConnect.Common/Infrastructure/PkceBlob.cs create mode 100644 source/Server.OpenIDConnect.Common/Web/LoginStateWithSessionId.cs diff --git a/source/Client.OctopusID/Configuration/OctopusIDConfigurationResource.cs b/source/Client.OctopusID/Configuration/OctopusIDConfigurationResource.cs index 75d3da0..2df8795 100644 --- a/source/Client.OctopusID/Configuration/OctopusIDConfigurationResource.cs +++ b/source/Client.OctopusID/Configuration/OctopusIDConfigurationResource.cs @@ -5,7 +5,7 @@ namespace Octopus.Client.Extensibility.Authentication.OctopusID.Configuration { [Description("Sign in to your Octopus Server with your Octopus ID. [Learn more](https://g.octopushq.com/AuthOctopusID).")] - public class OctopusIDConfigurationResource : OpenIDConnectConfigurationWithClientSecretResource + public class OctopusIDConfigurationResource : OpenIDConnectConfigurationResource { public OctopusIDConfigurationResource() { diff --git a/source/Client.OpenIDConnect/Client.OpenIDConnect.csproj b/source/Client.OpenIDConnect/Client.OpenIDConnect.csproj index 48274b0..88580ae 100644 --- a/source/Client.OpenIDConnect/Client.OpenIDConnect.csproj +++ b/source/Client.OpenIDConnect/Client.OpenIDConnect.csproj @@ -20,8 +20,8 @@ - - + + diff --git a/source/Client.OpenIDConnect/Configuration/OpenIDConnectConfigurationResource.cs b/source/Client.OpenIDConnect/Configuration/OpenIDConnectConfigurationResource.cs index 7119f0f..fc2c309 100644 --- a/source/Client.OpenIDConnect/Configuration/OpenIDConnectConfigurationResource.cs +++ b/source/Client.OpenIDConnect/Configuration/OpenIDConnectConfigurationResource.cs @@ -1,6 +1,7 @@ using System.ComponentModel; using Octopus.Client.Extensibility.Attributes; using Octopus.Client.Extensibility.Extensions.Infrastructure.Configuration; +using Octopus.Client.Model; namespace Octopus.Client.Extensibility.Authentication.OpenIDConnect.Configuration { @@ -15,6 +16,11 @@ public class OpenIDConnectConfigurationResource : ExtensionConfigurationResource [Writeable] public string ClientId { get; set; } + [DisplayName("Client Secret")] + [Description("Follow our documentation to find the Client Secret for your identity provider")] + [Writeable] + public SensitiveValue ClientSecret { get; set; } + [Writeable] [Description("Only change this if you need to change the OpenID Connect scope requested by Octopus")] public string Scope { get; set; } diff --git a/source/Client.OpenIDConnect/Configuration/OpenIDConnectConfigurationWithClientSecretResource.cs b/source/Client.OpenIDConnect/Configuration/OpenIDConnectConfigurationWithClientSecretResource.cs deleted file mode 100644 index 230f2d3..0000000 --- a/source/Client.OpenIDConnect/Configuration/OpenIDConnectConfigurationWithClientSecretResource.cs +++ /dev/null @@ -1,15 +0,0 @@ -using System.ComponentModel; -using Octopus.Client.Extensibility.Attributes; -using Octopus.Client.Model; - -namespace Octopus.Client.Extensibility.Authentication.OpenIDConnect.Configuration -{ - public class OpenIDConnectConfigurationWithClientSecretResource : OpenIDConnectConfigurationResource - { - [DisplayName("Client Secret")] - [Description("Follow our documentation to find the Client Secret for your identity provider")] - [Writeable] - public SensitiveValue ClientSecret { get; set; } - - } -} \ No newline at end of file diff --git a/source/Server.AzureAD/Web/AzureADUserAuthenticatedPkceAction.cs b/source/Server.AzureAD/Web/AzureADUserAuthenticatedPkceAction.cs index 90caceb..6de3588 100644 --- a/source/Server.AzureAD/Web/AzureADUserAuthenticatedPkceAction.cs +++ b/source/Server.AzureAD/Web/AzureADUserAuthenticatedPkceAction.cs @@ -7,6 +7,7 @@ using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Issuer; using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Web; using Octopus.Server.Extensibility.HostServices.Web; +using Octopus.Server.Extensibility.Mediator; using Octopus.Time; namespace Octopus.Server.Extensibility.Authentication.AzureAD.Web @@ -25,7 +26,8 @@ public AzureADUserAuthenticatedPkceAction( IAzureADIdentityCreator identityCreator, IClock clock, IUrlEncoder encoder, - IIdentityProviderConfigDiscoverer identityProviderConfigDiscoverer) : + IIdentityProviderConfigDiscoverer identityProviderConfigDiscoverer, + IMediator mediator) : base( log, authTokenHandler, @@ -38,7 +40,7 @@ public AzureADUserAuthenticatedPkceAction( identityCreator, clock, encoder, - identityProviderConfigDiscoverer) + identityProviderConfigDiscoverer, mediator) { } diff --git a/source/Server.AzureAD/Web/AzureADUserAuthenticationAction.cs b/source/Server.AzureAD/Web/AzureADUserAuthenticationAction.cs index c3e2933..8ce4eb1 100644 --- a/source/Server.AzureAD/Web/AzureADUserAuthenticationAction.cs +++ b/source/Server.AzureAD/Web/AzureADUserAuthenticationAction.cs @@ -5,6 +5,7 @@ using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Issuer; using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Web; using Octopus.Server.Extensibility.Extensions.Infrastructure.Web.Api; +using Octopus.Server.Extensibility.Mediator; namespace Octopus.Server.Extensibility.Authentication.AzureAD.Web { @@ -16,7 +17,8 @@ public AzureADUserAuthenticationAction( IIdentityProviderConfigDiscoverer identityProviderConfigDiscoverer, IAzureADAuthorizationEndpointUrlBuilder urlBuilder, IApiActionModelBinder modelBinder, - IAuthenticationConfigurationStore authenticationConfigurationStore) : base(log, configurationStore, identityProviderConfigDiscoverer, urlBuilder, modelBinder, authenticationConfigurationStore) + IAuthenticationConfigurationStore authenticationConfigurationStore, + IMediator mediator) : base(log, configurationStore, identityProviderConfigDiscoverer, urlBuilder, modelBinder, authenticationConfigurationStore, mediator) { } } diff --git a/source/Server.GoogleApps/Web/GoogleAppsUserAuthenticatedPkceAction.cs b/source/Server.GoogleApps/Web/GoogleAppsUserAuthenticatedPkceAction.cs index 1139960..f4c4401 100644 --- a/source/Server.GoogleApps/Web/GoogleAppsUserAuthenticatedPkceAction.cs +++ b/source/Server.GoogleApps/Web/GoogleAppsUserAuthenticatedPkceAction.cs @@ -7,6 +7,7 @@ using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Issuer; using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Web; using Octopus.Server.Extensibility.HostServices.Web; +using Octopus.Server.Extensibility.Mediator; using Octopus.Time; namespace Octopus.Server.Extensibility.Authentication.GoogleApps.Web @@ -24,8 +25,8 @@ public GoogleAppsUserAuthenticatedPkceAction( IInvalidLoginTracker loginTracker, ISleep sleep, IGoogleAppsIdentityCreator identityCreator, - IClock clock, IUrlEncoder encoder, IIdentityProviderConfigDiscoverer identityProviderConfigDiscoverer) - : base(log, authTokenHandler, principalToUserResourceMapper, userStore, configurationStore, authCookieCreator, loginTracker, sleep, identityCreator, clock, encoder, identityProviderConfigDiscoverer) + IClock clock, IUrlEncoder encoder, IIdentityProviderConfigDiscoverer identityProviderConfigDiscoverer, IMediator mediator) + : base(log, authTokenHandler, principalToUserResourceMapper, userStore, configurationStore, authCookieCreator, loginTracker, sleep, identityCreator, clock, encoder, identityProviderConfigDiscoverer, mediator) { } diff --git a/source/Server.GoogleApps/Web/GoogleAppsUserAuthenticationAction.cs b/source/Server.GoogleApps/Web/GoogleAppsUserAuthenticationAction.cs index b69a5c5..b1d99f0 100644 --- a/source/Server.GoogleApps/Web/GoogleAppsUserAuthenticationAction.cs +++ b/source/Server.GoogleApps/Web/GoogleAppsUserAuthenticationAction.cs @@ -5,6 +5,7 @@ using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Issuer; using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Web; using Octopus.Server.Extensibility.Extensions.Infrastructure.Web.Api; +using Octopus.Server.Extensibility.Mediator; namespace Octopus.Server.Extensibility.Authentication.GoogleApps.Web { @@ -16,8 +17,9 @@ public GoogleAppsUserAuthenticationAction( IIdentityProviderConfigDiscoverer identityProviderConfigDiscoverer, IGoogleAppsAuthorizationEndpointUrlBuilder urlBuilder, IApiActionModelBinder modelBinder, - IAuthenticationConfigurationStore authenticationConfigurationStore) - : base(log, configurationStore, identityProviderConfigDiscoverer, urlBuilder, modelBinder, authenticationConfigurationStore) + IAuthenticationConfigurationStore authenticationConfigurationStore, + IMediator mediator) + : base(log, configurationStore, identityProviderConfigDiscoverer, urlBuilder, modelBinder, authenticationConfigurationStore, mediator) { } } diff --git a/source/Server.OctopusID/Web/OctopusIDUserAuthenticatedPkceAction.cs b/source/Server.OctopusID/Web/OctopusIDUserAuthenticatedPkceAction.cs index b2bea7f..ed6a9a8 100644 --- a/source/Server.OctopusID/Web/OctopusIDUserAuthenticatedPkceAction.cs +++ b/source/Server.OctopusID/Web/OctopusIDUserAuthenticatedPkceAction.cs @@ -7,6 +7,7 @@ using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Issuer; using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Web; using Octopus.Server.Extensibility.HostServices.Web; +using Octopus.Server.Extensibility.Mediator; using Octopus.Time; namespace Octopus.Server.Extensibility.Authentication.OctopusID.Web @@ -24,8 +25,8 @@ public OctopusIDUserAuthenticatedPkceAction( IInvalidLoginTracker loginTracker, ISleep sleep, IOctopusIDIdentityCreator identityCreator, - IClock clock, IUrlEncoder encoder, IIdentityProviderConfigDiscoverer identityProviderConfigDiscoverer) - : base(log, authTokenHandler, principalToUserResourceMapper, userStore, configurationStore, authCookieCreator, loginTracker, sleep, identityCreator, clock, encoder, identityProviderConfigDiscoverer) + IClock clock, IUrlEncoder encoder, IIdentityProviderConfigDiscoverer identityProviderConfigDiscoverer, IMediator mediator) + : base(log, authTokenHandler, principalToUserResourceMapper, userStore, configurationStore, authCookieCreator, loginTracker, sleep, identityCreator, clock, encoder, identityProviderConfigDiscoverer, mediator) { } diff --git a/source/Server.OctopusID/Web/OctopusIDUserAuthenticationAction.cs b/source/Server.OctopusID/Web/OctopusIDUserAuthenticationAction.cs index f010cbe..dc21594 100644 --- a/source/Server.OctopusID/Web/OctopusIDUserAuthenticationAction.cs +++ b/source/Server.OctopusID/Web/OctopusIDUserAuthenticationAction.cs @@ -4,6 +4,7 @@ using Octopus.Server.Extensibility.Authentication.OctopusID.Issuer; using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Web; using Octopus.Server.Extensibility.Extensions.Infrastructure.Web.Api; +using Octopus.Server.Extensibility.Mediator; namespace Octopus.Server.Extensibility.Authentication.OctopusID.Web { @@ -15,7 +16,8 @@ public OctopusIDUserAuthenticationAction( IOctopusIdentityProviderConfigDiscoverer identityProviderConfigDiscoverer, IOctopusIDAuthorizationEndpointUrlBuilder urlBuilder, IApiActionModelBinder modelBinder, - IAuthenticationConfigurationStore authenticationConfigurationStore) : base(log, configurationStore, identityProviderConfigDiscoverer, urlBuilder, modelBinder, authenticationConfigurationStore) + IAuthenticationConfigurationStore authenticationConfigurationStore, + IMediator mediator) : base(log, configurationStore, identityProviderConfigDiscoverer, urlBuilder, modelBinder, authenticationConfigurationStore, mediator) { } } diff --git a/source/Server.Okta/Web/OktaUserAuthenticatedPkceAction.cs b/source/Server.Okta/Web/OktaUserAuthenticatedPkceAction.cs index 4ef22bd..94ca612 100644 --- a/source/Server.Okta/Web/OktaUserAuthenticatedPkceAction.cs +++ b/source/Server.Okta/Web/OktaUserAuthenticatedPkceAction.cs @@ -7,6 +7,7 @@ using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Issuer; using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Web; using Octopus.Server.Extensibility.HostServices.Web; +using Octopus.Server.Extensibility.Mediator; using Octopus.Time; namespace Octopus.Server.Extensibility.Authentication.Okta.Web @@ -24,8 +25,8 @@ public OktaUserAuthenticatedPkceAction( IInvalidLoginTracker loginTracker, ISleep sleep, IOktaIdentityCreator identityCreator, - IClock clock, IUrlEncoder encoder, IIdentityProviderConfigDiscoverer identityProviderConfigDiscoverer) - : base(log, authTokenHandler, principalToUserResourceMapper, userStore, configurationStore, authCookieCreator, loginTracker, sleep, identityCreator, clock, encoder, identityProviderConfigDiscoverer) + IClock clock, IUrlEncoder encoder, IIdentityProviderConfigDiscoverer identityProviderConfigDiscoverer, IMediator mediator) + : base(log, authTokenHandler, principalToUserResourceMapper, userStore, configurationStore, authCookieCreator, loginTracker, sleep, identityCreator, clock, encoder, identityProviderConfigDiscoverer, mediator) { } diff --git a/source/Server.Okta/Web/OktaUserAuthenticationAction.cs b/source/Server.Okta/Web/OktaUserAuthenticationAction.cs index 7b5631b..2d980e9 100644 --- a/source/Server.Okta/Web/OktaUserAuthenticationAction.cs +++ b/source/Server.Okta/Web/OktaUserAuthenticationAction.cs @@ -5,6 +5,7 @@ using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Issuer; using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Web; using Octopus.Server.Extensibility.Extensions.Infrastructure.Web.Api; +using Octopus.Server.Extensibility.Mediator; namespace Octopus.Server.Extensibility.Authentication.Okta.Web { @@ -16,7 +17,8 @@ public OktaUserAuthenticationAction( IIdentityProviderConfigDiscoverer identityProviderConfigDiscoverer, IOktaAuthorizationEndpointUrlBuilder urlBuilder, IApiActionModelBinder modelBinder, - IAuthenticationConfigurationStore authenticationConfigurationStore) : base(log, configurationStore, identityProviderConfigDiscoverer, urlBuilder, modelBinder, authenticationConfigurationStore) + IAuthenticationConfigurationStore authenticationConfigurationStore, + IMediator mediator) : base(log, configurationStore, identityProviderConfigDiscoverer, urlBuilder, modelBinder, authenticationConfigurationStore, mediator) { } } diff --git a/source/Server.OpenIDConnect.Common/Configuration/IOpenIDConnectWithClientSecretConfigurationStore.cs b/source/Server.OpenIDConnect.Common/Configuration/IOpenIDConnectWithClientSecretConfigurationStore.cs deleted file mode 100644 index 4b79213..0000000 --- a/source/Server.OpenIDConnect.Common/Configuration/IOpenIDConnectWithClientSecretConfigurationStore.cs +++ /dev/null @@ -1,15 +0,0 @@ -using Octopus.Data.Model; - -namespace Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Configuration -{ - public interface IOpenIDConnectWithClientSecretConfigurationStore : IOpenIDConnectConfigurationStore, IOpenIDConnectWithClientSecretConfigurationStore - where TConfiguration : OpenIDConnectConfiguration, IId, new() - { - } - - public interface IOpenIDConnectWithClientSecretConfigurationStore : IOpenIDConnectConfigurationStore - { - SensitiveString? GetClientSecret(); - void SetClientSecret(SensitiveString? clientSecret); - } -} \ No newline at end of file diff --git a/source/Server.OpenIDConnect.Common/Infrastructure/Pkce.cs b/source/Server.OpenIDConnect.Common/Infrastructure/Pkce.cs index cb1694c..6a80129 100644 --- a/source/Server.OpenIDConnect.Common/Infrastructure/Pkce.cs +++ b/source/Server.OpenIDConnect.Common/Infrastructure/Pkce.cs @@ -8,7 +8,6 @@ namespace Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Infra public static class Pkce { const string UnreservedCharacters = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-._~"; - public static string? InMemoryCodeVerifier { get; private set; } public static string GenerateCodeVerifier(int size = 128) { @@ -24,7 +23,6 @@ public static string GenerateCodeVerifier(int size = 128) } var codeVerifier = new string(highEntropyCryptograph); - InMemoryCodeVerifier = codeVerifier; return codeVerifier; } diff --git a/source/Server.OpenIDConnect.Common/Infrastructure/PkceBlob.cs b/source/Server.OpenIDConnect.Common/Infrastructure/PkceBlob.cs new file mode 100644 index 0000000..151274a --- /dev/null +++ b/source/Server.OpenIDConnect.Common/Infrastructure/PkceBlob.cs @@ -0,0 +1,18 @@ +using System; + +namespace Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Infrastructure +{ + public class PkceBlob + { + public Guid SessionId { get; } + public string CodeVerifier { get; } + public DateTimeOffset TimeStamp { get; } + + public PkceBlob(Guid sessionId, string codeVerifier, DateTimeOffset timeStamp) + { + SessionId = sessionId; + CodeVerifier = codeVerifier; + TimeStamp = timeStamp; + } + } +} \ No newline at end of file diff --git a/source/Server.OpenIDConnect.Common/Server.OpenIDConnect.Common.csproj b/source/Server.OpenIDConnect.Common/Server.OpenIDConnect.Common.csproj index 5eadb42..ce081c7 100644 --- a/source/Server.OpenIDConnect.Common/Server.OpenIDConnect.Common.csproj +++ b/source/Server.OpenIDConnect.Common/Server.OpenIDConnect.Common.csproj @@ -19,6 +19,7 @@ + diff --git a/source/Server.OpenIDConnect.Common/Web/LoginStateWithSessionId.cs b/source/Server.OpenIDConnect.Common/Web/LoginStateWithSessionId.cs new file mode 100644 index 0000000..6648cc3 --- /dev/null +++ b/source/Server.OpenIDConnect.Common/Web/LoginStateWithSessionId.cs @@ -0,0 +1,17 @@ +using System; +using Octopus.Server.Extensibility.Authentication.Resources; + +namespace Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Web +{ + class LoginStateWithSessionId : LoginState + { + public Guid SessionId { get; set; } + + public LoginStateWithSessionId(string redirectAfterLoginTo, bool usingSecureConnection, Guid sessionId) + { + RedirectAfterLoginTo = redirectAfterLoginTo; + UsingSecureConnection = usingSecureConnection; + SessionId = sessionId; + } + } +} \ No newline at end of file diff --git a/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedPkceAction.cs b/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedPkceAction.cs index d96ac9c..8b164d6 100644 --- a/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedPkceAction.cs +++ b/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedPkceAction.cs @@ -3,6 +3,7 @@ using System.Globalization; using System.Linq; using System.Net.Http; +using System.Text; using System.Threading; using System.Threading.Tasks; using Newtonsoft.Json; @@ -16,11 +17,12 @@ using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Infrastructure; using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Issuer; using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Tokens; -using Octopus.Server.Extensibility.Authentication.Resources; using Octopus.Server.Extensibility.Authentication.Resources.Identities; using Octopus.Server.Extensibility.Extensions.Infrastructure.Web.Api; using Octopus.Server.Extensibility.HostServices.Web; +using Octopus.Server.Extensibility.Mediator; using Octopus.Server.Extensibility.Results; +using Octopus.Server.MessageContracts.Features.BlobStorage; using Octopus.Time; namespace Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Web @@ -47,11 +49,11 @@ public abstract class UserAuthenticatedPkceAction ExecuteAsync(IOctoRequest request) return await request.HandleAsync(codeParameter, stateParameter, (code, state) => Handle(code, state, request)); } - async Task> RequestAuthToken(string code, string redirectUri) + async Task> RequestAuthToken(string code, string redirectUri, Guid sessionId) { var issuerConfig = await identityProviderConfigDiscoverer.GetConfigurationAsync(ConfigurationStore.GetIssuer() ?? string.Empty); using var client = new HttpClient(); var request = new HttpRequestMessage(HttpMethod.Post, issuerConfig.TokenEndpoint); + + var allBlobsBelongingToExtension = await mediator.Request(new GetAllBlobsRequest(ConfigurationStore.ConfigurationSettingsName), new CancellationToken()); + var blobs = allBlobsBelongingToExtension.Blobs.Select(b => JsonConvert.DeserializeObject(Encoding.UTF8.GetString(b))!).ToList(); + foreach (var blob in blobs) + { + // Any expired blobs should be removed as well - currently using a 30 second timer + if (blob.SessionId == sessionId || DateTimeOffset.UtcNow.Subtract(blob.TimeStamp).TotalSeconds > 30) + { + await mediator.Do(new DeleteBlobCommand(ConfigurationStore.ConfigurationSettingsName, blob.SessionId.ToString()), new CancellationToken()); + } + } + var formValues = new Dictionary { ["grant_type"] = "authorization_code", @@ -97,9 +112,9 @@ public async Task ExecuteAsync(IOctoRequest request) ["redirect_uri"] = redirectUri, ["client_id"] = ConfigurationStore.GetClientId()!, ["client_secret"] = ConfigurationStore.GetClientSecret()!.Value, - ["code_verifier"] = Pkce.InMemoryCodeVerifier! + ["code_verifier"] = blobs.Single(b => b.SessionId == sessionId).CodeVerifier }; - request.Content = new FormUrlEncodedContent(formValues); + request.Content = new FormUrlEncodedContent(formValues!); var response = await client.SendAsync(request, HttpCompletionOption.ResponseContentRead); var body = await response.Content.ReadAsStringAsync(); return JsonConvert.DeserializeObject>(body)!; @@ -109,7 +124,8 @@ async Task Handle(string code, string state, IOctoRequest { var host = request.Headers.ContainsKey("Host") ? request.Headers["Host"].Single() : request.Host; var redirectUri = $"{request.Scheme}://{host}{ConfigurationStore.RedirectUri}"; - var response = await RequestAuthToken(code, redirectUri); + var stateFromRequest = JsonConvert.DeserializeObject(state); + var response = await RequestAuthToken(code, redirectUri, stateFromRequest!.SessionId); // Step 1: Try and get all of the details from the request making sure there are no errors passed back from the external identity provider var principalContainer = await authTokenHandler.GetPrincipalAsync(response, out var stateStringFromRequest); @@ -130,14 +146,12 @@ async Task Handle(string code, string state, IOctoRequest return BadRequest($"User login failed: Missing State Hash Cookie. {stateDescription} In this case the Cookie containing the SHA256 hash of the state object is missing from the request."); } - var stateFromRequestHash = Infrastructure.State.Protect(stateStringFromRequest); + var stateFromRequestHash = State.Protect(stateStringFromRequest); + if (stateFromRequestHash != expectedStateHash) { return BadRequest($"User login failed: Tampered State. {stateDescription} In this case the state object looks like it has been tampered with. The state object is '{stateStringFromRequest}'. The SHA256 hash of the state was expected to be '{expectedStateHash}' but was '{stateFromRequestHash}'."); } - - var stateFromRequest = JsonConvert.DeserializeObject(stateStringFromRequest ?? string.Empty); - // Step 3: Now the integrity of the request has been validated we can figure out which Octopus User this represents var authenticationCandidate = principalToUserResourceMapper.MapToUserResource(principal); if (authenticationCandidate.Username == null) diff --git a/source/Server.OpenIDConnect.Common/Web/UserAuthenticationAction.cs b/source/Server.OpenIDConnect.Common/Web/UserAuthenticationAction.cs index 4b1a660..d1ffb52 100644 --- a/source/Server.OpenIDConnect.Common/Web/UserAuthenticationAction.cs +++ b/source/Server.OpenIDConnect.Common/Web/UserAuthenticationAction.cs @@ -1,4 +1,5 @@ using System; +using System.Threading; using System.Threading.Tasks; using Newtonsoft.Json; using Octopus.Diagnostics; @@ -8,10 +9,13 @@ using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Issuer; using Octopus.Server.Extensibility.Authentication.Resources; using Octopus.Server.Extensibility.Extensions.Infrastructure.Web.Api; +using Octopus.Server.Extensibility.Mediator; +using Octopus.Server.MessageContracts.Features.BlobStorage; +using JsonSerializer = System.Text.Json.JsonSerializer; namespace Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Web { - public abstract class UserAuthenticationAction : IAsyncApiAction + public abstract partial class UserAuthenticationAction : IAsyncApiAction where TStore : IOpenIDConnectConfigurationStore { static readonly BadRequestRegistration Disabled = new BadRequestRegistration("This authentication provider is disabled."); @@ -26,6 +30,7 @@ public abstract class UserAuthenticationAction : IAsyncApiAction protected readonly TStore ConfigurationStore; readonly IApiActionModelBinder modelBinder; readonly IAuthenticationConfigurationStore authenticationConfigurationStore; + readonly IMediator mediator; protected UserAuthenticationAction( ISystemLog log, @@ -33,11 +38,13 @@ protected UserAuthenticationAction( IIdentityProviderConfigDiscoverer identityProviderConfigDiscoverer, IAuthorizationEndpointUrlBuilder urlBuilder, IApiActionModelBinder modelBinder, - IAuthenticationConfigurationStore authenticationConfigurationStore) + IAuthenticationConfigurationStore authenticationConfigurationStore, + IMediator mediator) { this.log = log; this.modelBinder = modelBinder; this.authenticationConfigurationStore = authenticationConfigurationStore; + this.mediator = mediator; ConfigurationStore = configurationStore; this.identityProviderConfigDiscoverer = identityProviderConfigDiscoverer; this.urlBuilder = urlBuilder; @@ -73,7 +80,7 @@ public async Task ExecuteAsync(IOctoRequest request) var issuerConfig = await identityProviderConfigDiscoverer.GetConfigurationAsync(issuer); var response = ConfigurationStore.HasClientSecret - ? BuildAuthorizationCodePkceResponse(model, state, issuerConfig) + ? await BuildAuthorizationCodePkceResponse(model, new LoginStateWithSessionId(state.RedirectAfterLoginTo, state.UsingSecureConnection, Guid.NewGuid()), issuerConfig) : BuildHybridResponse(model, state, issuerConfig); return response; @@ -90,18 +97,21 @@ public async Task ExecuteAsync(IOctoRequest request) } } - IOctoResponseProvider BuildAuthorizationCodePkceResponse(LoginRedirectLinkRequestModel model, LoginState state, IssuerConfiguration issuerConfig) + async Task BuildAuthorizationCodePkceResponse(LoginRedirectLinkRequestModel model, LoginStateWithSessionId state, IssuerConfiguration issuerConfig) { var codeVerifier = Pkce.GenerateCodeVerifier(); - var codeChallenge = Pkce.GenerateCodeChallenge(codeVerifier); + var pkceBlob = new PkceBlob(state.SessionId, codeVerifier, DateTimeOffset.UtcNow); + await mediator.Do(new PutBlobCommand(ConfigurationStore.ConfigurationSettingsName, state.SessionId.ToString(), JsonSerializer.SerializeToUtf8Bytes(pkceBlob)), new CancellationToken()); var stateString = JsonConvert.SerializeObject(state); + var codeChallenge = Pkce.GenerateCodeChallenge(codeVerifier); var url = urlBuilder.Build(model.ApiAbsUrl, issuerConfig, state: stateString, codeChallenge: codeChallenge); var response = Result.Response(new LoginRedirectLinkResponseModel {ExternalAuthenticationUrl = url}) .WithCookie(new OctoCookie(UserAuthConstants.OctopusStateCookieName, State.Protect(stateString)) {HttpOnly = true, Secure = state.UsingSecureConnection, Expires = DateTimeOffset.UtcNow.AddMinutes(20)}); return response; } + // For backwards compatibility - if no client secret is specified then the implicit flow will attempt to be used IOctoResponseProvider BuildHybridResponse(LoginRedirectLinkRequestModel model, LoginState state, IssuerConfiguration issuerConfig) { // Use a non-deterministic nonce to prevent replay attacks From e79f046fac1791820aa19b24e6609f0acd2be06b Mon Sep 17 00:00:00 2001 From: Nelson Susanto Date: Wed, 30 Mar 2022 11:03:26 +1100 Subject: [PATCH 20/27] Update code verifier generation --- .../GoogleAppsUserAuthenticatedPkceAction.cs | 5 ++- .../OctopusIDUserAuthenticatedPkceAction.cs | 5 ++- .../Web/OktaUserAuthenticatedPkceAction.cs | 5 ++- .../OpenIDConnectConfigureCommands.cs | 1 + .../Infrastructure/Pkce.cs | 44 ++++++++++++------- .../Web/UserAuthenticationAction.cs | 2 +- 6 files changed, 42 insertions(+), 20 deletions(-) diff --git a/source/Server.GoogleApps/Web/GoogleAppsUserAuthenticatedPkceAction.cs b/source/Server.GoogleApps/Web/GoogleAppsUserAuthenticatedPkceAction.cs index f4c4401..3c8de41 100644 --- a/source/Server.GoogleApps/Web/GoogleAppsUserAuthenticatedPkceAction.cs +++ b/source/Server.GoogleApps/Web/GoogleAppsUserAuthenticatedPkceAction.cs @@ -25,7 +25,10 @@ public GoogleAppsUserAuthenticatedPkceAction( IInvalidLoginTracker loginTracker, ISleep sleep, IGoogleAppsIdentityCreator identityCreator, - IClock clock, IUrlEncoder encoder, IIdentityProviderConfigDiscoverer identityProviderConfigDiscoverer, IMediator mediator) + IClock clock, + IUrlEncoder encoder, + IIdentityProviderConfigDiscoverer identityProviderConfigDiscoverer, + IMediator mediator) : base(log, authTokenHandler, principalToUserResourceMapper, userStore, configurationStore, authCookieCreator, loginTracker, sleep, identityCreator, clock, encoder, identityProviderConfigDiscoverer, mediator) { } diff --git a/source/Server.OctopusID/Web/OctopusIDUserAuthenticatedPkceAction.cs b/source/Server.OctopusID/Web/OctopusIDUserAuthenticatedPkceAction.cs index ed6a9a8..ef1abe2 100644 --- a/source/Server.OctopusID/Web/OctopusIDUserAuthenticatedPkceAction.cs +++ b/source/Server.OctopusID/Web/OctopusIDUserAuthenticatedPkceAction.cs @@ -25,7 +25,10 @@ public OctopusIDUserAuthenticatedPkceAction( IInvalidLoginTracker loginTracker, ISleep sleep, IOctopusIDIdentityCreator identityCreator, - IClock clock, IUrlEncoder encoder, IIdentityProviderConfigDiscoverer identityProviderConfigDiscoverer, IMediator mediator) + IClock clock, + IUrlEncoder encoder, + IIdentityProviderConfigDiscoverer identityProviderConfigDiscoverer, + IMediator mediator) : base(log, authTokenHandler, principalToUserResourceMapper, userStore, configurationStore, authCookieCreator, loginTracker, sleep, identityCreator, clock, encoder, identityProviderConfigDiscoverer, mediator) { } diff --git a/source/Server.Okta/Web/OktaUserAuthenticatedPkceAction.cs b/source/Server.Okta/Web/OktaUserAuthenticatedPkceAction.cs index 94ca612..5616089 100644 --- a/source/Server.Okta/Web/OktaUserAuthenticatedPkceAction.cs +++ b/source/Server.Okta/Web/OktaUserAuthenticatedPkceAction.cs @@ -25,7 +25,10 @@ public OktaUserAuthenticatedPkceAction( IInvalidLoginTracker loginTracker, ISleep sleep, IOktaIdentityCreator identityCreator, - IClock clock, IUrlEncoder encoder, IIdentityProviderConfigDiscoverer identityProviderConfigDiscoverer, IMediator mediator) + IClock clock, + IUrlEncoder encoder, + IIdentityProviderConfigDiscoverer identityProviderConfigDiscoverer, + IMediator mediator) : base(log, authTokenHandler, principalToUserResourceMapper, userStore, configurationStore, authCookieCreator, loginTracker, sleep, identityCreator, clock, encoder, identityProviderConfigDiscoverer, mediator) { } diff --git a/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfigureCommands.cs b/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfigureCommands.cs index 534275c..caac939 100644 --- a/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfigureCommands.cs +++ b/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfigureCommands.cs @@ -65,6 +65,7 @@ protected IEnumerable GetCoreOptions(bool hide) yield return new ConfigureCommandOption($"{ConfigurationSettingsName}ClientSecret=", $"Follow our documentation to find the Client Secret for {ConfigurationSettingsName}.", v => { ConfigurationStore.Value.SetClientSecret(v.ToSensitiveString()); + Log.Info($"{ConfigurationSettingsName} ClientSecret set to: {v}"); }, hide: hide); } diff --git a/source/Server.OpenIDConnect.Common/Infrastructure/Pkce.cs b/source/Server.OpenIDConnect.Common/Infrastructure/Pkce.cs index 6a80129..b831dca 100644 --- a/source/Server.OpenIDConnect.Common/Infrastructure/Pkce.cs +++ b/source/Server.OpenIDConnect.Common/Infrastructure/Pkce.cs @@ -7,25 +7,37 @@ namespace Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Infra { public static class Pkce { - const string UnreservedCharacters = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-._~"; - - public static string GenerateCodeVerifier(int size = 128) + /* + * https://datatracker.ietf.org/doc/html/rfc7636#section-4.1 + * code_verifier = high-entropy cryptographic random STRING using the + * unreserved characters [A-Z] / [a-z] / [0-9] / "-" / "." / "_" / "~" + * from Section 2.3 of [RFC3986], with a minimum length of 43 characters + * and a maximum length of 128 characters. + * + * ABNF for "code_verifier" is as follows. + * + * code-verifier = 43*128unreserved + * unreserved = ALPHA / DIGIT / "-" / "." / "_" / "~" + * ALPHA = %x41-5A / %x61-7A + * DIGIT = %x30-39 + * + * NOTE: The code verifier SHOULD have enough entropy to make it + * impractical to guess the value. It is RECOMMENDED that the output of + * a suitable random number generator be used to create a 32-octet + * sequence. The octet sequence is then base64url-encoded to produce a + * 43-octet URL safe string to use as the code verifier. + */ + public static string GenerateCodeVerifier() { - if (size is < 43 or > 128) - size = 128; - - Random random = new(); - char[] highEntropyCryptograph = new char[size]; - - for (var i = 0; i < highEntropyCryptograph.Length; i++) - { - highEntropyCryptograph[i] = UnreservedCharacters[random.Next(UnreservedCharacters.Length)]; - } - - var codeVerifier = new string(highEntropyCryptograph); - return codeVerifier; + var data = new byte[32]; + RandomNumberGenerator.Fill(data); + return Base64UrlEncoder.Encode(data); } + /* + * https://datatracker.ietf.org/doc/html/rfc7636#section-4.2 + * code_challenge = BASE64URL-ENCODE(SHA256(ASCII(code_verifier))) + */ public static string GenerateCodeChallenge(string codeVerifier) { using var sha = SHA256.Create(); diff --git a/source/Server.OpenIDConnect.Common/Web/UserAuthenticationAction.cs b/source/Server.OpenIDConnect.Common/Web/UserAuthenticationAction.cs index d1ffb52..1407691 100644 --- a/source/Server.OpenIDConnect.Common/Web/UserAuthenticationAction.cs +++ b/source/Server.OpenIDConnect.Common/Web/UserAuthenticationAction.cs @@ -15,7 +15,7 @@ namespace Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Web { - public abstract partial class UserAuthenticationAction : IAsyncApiAction + public abstract class UserAuthenticationAction : IAsyncApiAction where TStore : IOpenIDConnectConfigurationStore { static readonly BadRequestRegistration Disabled = new BadRequestRegistration("This authentication provider is disabled."); From 28ffa70606b8d38f8a525a1ad6db49117c6fb00c Mon Sep 17 00:00:00 2001 From: Nelson Susanto Date: Mon, 11 Apr 2022 14:47:34 +1000 Subject: [PATCH 21/27] Clean up --- .../Web/AzureADUserAuthenticatedAction.cs | 10 +- .../Web/AzureADUserAuthenticatedPkceAction.cs | 14 +- .../Web/GoogleAppsUserAuthenticatedAction.cs | 16 +- .../GoogleAppsUserAuthenticatedPkceAction.cs | 18 +- .../Web/OctopusIDUserAuthenticatedAction.cs | 9 +- .../OctopusIDUserAuthenticatedPkceAction.cs | 18 +- .../Web/OktaUserAuthenticatedAction.cs | 9 +- .../Web/OktaUserAuthenticatedPkceAction.cs | 18 +- .../Infrastructure/PkceBlob.cs | 6 +- .../OpenIDConnectExtension.cs | 2 + .../Tokens/AuthTokenHandler.cs | 4 +- ...ntainer.cs => ClaimsPrincipalContainer.cs} | 6 +- .../Tokens/IAuthTokenHandler.cs | 2 +- .../Tokens/OpenIDConnectAuthTokenHandler.cs | 4 +- .../Web/FailedAuthenticationException.cs | 11 + .../Web/IUserService.cs | 13 + ...essionId.cs => LoginStateWithRequestId.cs} | 8 +- .../Web/UserAuthenticatedAction.cs | 181 ++++---------- .../Web/UserAuthenticatedPkceAction.cs | 224 +++++++----------- .../Web/UserAuthenticatedValidator.cs | 104 ++++++++ .../Web/UserAuthenticationAction.cs | 8 +- .../Web/UserService.cs | 86 +++++++ 22 files changed, 434 insertions(+), 337 deletions(-) rename source/Server.OpenIDConnect.Common/Tokens/{ClaimsPrincipleContainer.cs => ClaimsPrincipalContainer.cs} (79%) create mode 100644 source/Server.OpenIDConnect.Common/Web/FailedAuthenticationException.cs create mode 100644 source/Server.OpenIDConnect.Common/Web/IUserService.cs rename source/Server.OpenIDConnect.Common/Web/{LoginStateWithSessionId.cs => LoginStateWithRequestId.cs} (56%) create mode 100644 source/Server.OpenIDConnect.Common/Web/UserAuthenticatedValidator.cs create mode 100644 source/Server.OpenIDConnect.Common/Web/UserService.cs diff --git a/source/Server.AzureAD/Web/AzureADUserAuthenticatedAction.cs b/source/Server.AzureAD/Web/AzureADUserAuthenticatedAction.cs index f575196..6f9248d 100644 --- a/source/Server.AzureAD/Web/AzureADUserAuthenticatedAction.cs +++ b/source/Server.AzureAD/Web/AzureADUserAuthenticatedAction.cs @@ -16,26 +16,24 @@ public AzureADUserAuthenticatedAction( ISystemLog log, IAzureADAuthTokenHandler authTokenHandler, IAzureADPrincipalToUserResourceMapper principalToUserResourceMapper, - IUpdateableUserStore userStore, IAzureADConfigurationStore configurationStore, IAuthCookieCreator authCookieCreator, IInvalidLoginTracker loginTracker, ISleep sleep, IAzureADIdentityCreator identityCreator, - IClock clock, - IUrlEncoder encoder) : + IUrlEncoder encoder, + IUserService userService) : base( log, authTokenHandler, principalToUserResourceMapper, - userStore, configurationStore, authCookieCreator, loginTracker, sleep, identityCreator, - clock, - encoder) + encoder, + userService) { } diff --git a/source/Server.AzureAD/Web/AzureADUserAuthenticatedPkceAction.cs b/source/Server.AzureAD/Web/AzureADUserAuthenticatedPkceAction.cs index 6de3588..a8c5ab7 100644 --- a/source/Server.AzureAD/Web/AzureADUserAuthenticatedPkceAction.cs +++ b/source/Server.AzureAD/Web/AzureADUserAuthenticatedPkceAction.cs @@ -18,29 +18,27 @@ public AzureADUserAuthenticatedPkceAction( ISystemLog log, IAzureADAuthTokenHandler authTokenHandler, IAzureADPrincipalToUserResourceMapper principalToUserResourceMapper, - IUpdateableUserStore userStore, IAzureADConfigurationStore configurationStore, IAuthCookieCreator authCookieCreator, IInvalidLoginTracker loginTracker, ISleep sleep, IAzureADIdentityCreator identityCreator, - IClock clock, IUrlEncoder encoder, IIdentityProviderConfigDiscoverer identityProviderConfigDiscoverer, - IMediator mediator) : - base( - log, + IMediator mediator, + IUserService service) + : base(log, authTokenHandler, principalToUserResourceMapper, - userStore, configurationStore, authCookieCreator, loginTracker, sleep, identityCreator, - clock, encoder, - identityProviderConfigDiscoverer, mediator) + identityProviderConfigDiscoverer, + mediator, + service) { } diff --git a/source/Server.GoogleApps/Web/GoogleAppsUserAuthenticatedAction.cs b/source/Server.GoogleApps/Web/GoogleAppsUserAuthenticatedAction.cs index a0b0a0e..0707a5e 100644 --- a/source/Server.GoogleApps/Web/GoogleAppsUserAuthenticatedAction.cs +++ b/source/Server.GoogleApps/Web/GoogleAppsUserAuthenticatedAction.cs @@ -17,14 +17,24 @@ public GoogleAppsUserAuthenticatedAction( ISystemLog log, IGoogleAuthTokenHandler authTokenHandler, IPrincipalToUserResourceMapper principalToUserResourceMapper, - IUpdateableUserStore userStore, IGoogleAppsConfigurationStore configurationStore, IAuthCookieCreator authCookieCreator, IInvalidLoginTracker loginTracker, ISleep sleep, IGoogleAppsIdentityCreator identityCreator, - IClock clock, IUrlEncoder encoder) - : base(log, authTokenHandler, principalToUserResourceMapper, userStore, configurationStore, authCookieCreator, loginTracker, sleep, identityCreator, clock, encoder) + IUrlEncoder encoder, + IUserService userService) : + base( + log, + authTokenHandler, + principalToUserResourceMapper, + configurationStore, + authCookieCreator, + loginTracker, + sleep, + identityCreator, + encoder, + userService) { } diff --git a/source/Server.GoogleApps/Web/GoogleAppsUserAuthenticatedPkceAction.cs b/source/Server.GoogleApps/Web/GoogleAppsUserAuthenticatedPkceAction.cs index 3c8de41..71cf361 100644 --- a/source/Server.GoogleApps/Web/GoogleAppsUserAuthenticatedPkceAction.cs +++ b/source/Server.GoogleApps/Web/GoogleAppsUserAuthenticatedPkceAction.cs @@ -19,17 +19,27 @@ public GoogleAppsUserAuthenticatedPkceAction( ISystemLog log, IGoogleAuthTokenHandler authTokenHandler, IPrincipalToUserResourceMapper principalToUserResourceMapper, - IUpdateableUserStore userStore, IGoogleAppsConfigurationStore configurationStore, IAuthCookieCreator authCookieCreator, IInvalidLoginTracker loginTracker, ISleep sleep, IGoogleAppsIdentityCreator identityCreator, - IClock clock, IUrlEncoder encoder, IIdentityProviderConfigDiscoverer identityProviderConfigDiscoverer, - IMediator mediator) - : base(log, authTokenHandler, principalToUserResourceMapper, userStore, configurationStore, authCookieCreator, loginTracker, sleep, identityCreator, clock, encoder, identityProviderConfigDiscoverer, mediator) + IMediator mediator, + IUserService service) + : base(log, + authTokenHandler, + principalToUserResourceMapper, + configurationStore, + authCookieCreator, + loginTracker, + sleep, + identityCreator, + encoder, + identityProviderConfigDiscoverer, + mediator, + service) { } diff --git a/source/Server.OctopusID/Web/OctopusIDUserAuthenticatedAction.cs b/source/Server.OctopusID/Web/OctopusIDUserAuthenticatedAction.cs index a0945af..f085484 100644 --- a/source/Server.OctopusID/Web/OctopusIDUserAuthenticatedAction.cs +++ b/source/Server.OctopusID/Web/OctopusIDUserAuthenticatedAction.cs @@ -16,25 +16,24 @@ public OctopusIDUserAuthenticatedAction( ISystemLog log, IOctopusIDAuthTokenHandler authTokenHandler, IOctopusIDPrincipalToUserResourceMapper principalToUserResourceMapper, - IUpdateableUserStore userStore, IOctopusIDConfigurationStore configurationStore, IAuthCookieCreator authCookieCreator, IInvalidLoginTracker loginTracker, ISleep sleep, IOctopusIDIdentityCreator identityCreator, - IClock clock, IUrlEncoder encoder) : + IUrlEncoder encoder, + IUserService userService) : base( log, authTokenHandler, principalToUserResourceMapper, - userStore, configurationStore, authCookieCreator, loginTracker, sleep, identityCreator, - clock, - encoder) + encoder, + userService) { } diff --git a/source/Server.OctopusID/Web/OctopusIDUserAuthenticatedPkceAction.cs b/source/Server.OctopusID/Web/OctopusIDUserAuthenticatedPkceAction.cs index ef1abe2..9a3d218 100644 --- a/source/Server.OctopusID/Web/OctopusIDUserAuthenticatedPkceAction.cs +++ b/source/Server.OctopusID/Web/OctopusIDUserAuthenticatedPkceAction.cs @@ -19,17 +19,27 @@ public OctopusIDUserAuthenticatedPkceAction( ISystemLog log, IOctopusIDAuthTokenHandler authTokenHandler, IPrincipalToUserResourceMapper principalToUserResourceMapper, - IUpdateableUserStore userStore, IOctopusIDConfigurationStore configurationStore, IAuthCookieCreator authCookieCreator, IInvalidLoginTracker loginTracker, ISleep sleep, IOctopusIDIdentityCreator identityCreator, - IClock clock, IUrlEncoder encoder, IIdentityProviderConfigDiscoverer identityProviderConfigDiscoverer, - IMediator mediator) - : base(log, authTokenHandler, principalToUserResourceMapper, userStore, configurationStore, authCookieCreator, loginTracker, sleep, identityCreator, clock, encoder, identityProviderConfigDiscoverer, mediator) + IMediator mediator, + IUserService service) + : base(log, + authTokenHandler, + principalToUserResourceMapper, + configurationStore, + authCookieCreator, + loginTracker, + sleep, + identityCreator, + encoder, + identityProviderConfigDiscoverer, + mediator, + service) { } diff --git a/source/Server.Okta/Web/OktaUserAuthenticatedAction.cs b/source/Server.Okta/Web/OktaUserAuthenticatedAction.cs index fa87682..5872ee4 100644 --- a/source/Server.Okta/Web/OktaUserAuthenticatedAction.cs +++ b/source/Server.Okta/Web/OktaUserAuthenticatedAction.cs @@ -17,25 +17,24 @@ public OktaUserAuthenticatedAction( ISystemLog log, IOktaAuthTokenHandler authTokenHandler, IOktaPrincipalToUserResourceMapper principalToUserResourceMapper, - IUpdateableUserStore userStore, IOktaConfigurationStore configurationStore, IAuthCookieCreator authCookieCreator, IInvalidLoginTracker loginTracker, ISleep sleep, IOktaIdentityCreator identityCreator, - IClock clock, IUrlEncoder encoder) : + IUrlEncoder encoder, + IUserService userService) : base( log, authTokenHandler, principalToUserResourceMapper, - userStore, configurationStore, authCookieCreator, loginTracker, sleep, identityCreator, - clock, - encoder) + encoder, + userService) { } diff --git a/source/Server.Okta/Web/OktaUserAuthenticatedPkceAction.cs b/source/Server.Okta/Web/OktaUserAuthenticatedPkceAction.cs index 5616089..813af03 100644 --- a/source/Server.Okta/Web/OktaUserAuthenticatedPkceAction.cs +++ b/source/Server.Okta/Web/OktaUserAuthenticatedPkceAction.cs @@ -19,17 +19,27 @@ public OktaUserAuthenticatedPkceAction( ISystemLog log, IOktaAuthTokenHandler authTokenHandler, IPrincipalToUserResourceMapper principalToUserResourceMapper, - IUpdateableUserStore userStore, IOktaConfigurationStore configurationStore, IAuthCookieCreator authCookieCreator, IInvalidLoginTracker loginTracker, ISleep sleep, IOktaIdentityCreator identityCreator, - IClock clock, IUrlEncoder encoder, IIdentityProviderConfigDiscoverer identityProviderConfigDiscoverer, - IMediator mediator) - : base(log, authTokenHandler, principalToUserResourceMapper, userStore, configurationStore, authCookieCreator, loginTracker, sleep, identityCreator, clock, encoder, identityProviderConfigDiscoverer, mediator) + IMediator mediator, + IUserService service) + : base(log, + authTokenHandler, + principalToUserResourceMapper, + configurationStore, + authCookieCreator, + loginTracker, + sleep, + identityCreator, + encoder, + identityProviderConfigDiscoverer, + mediator, + service) { } diff --git a/source/Server.OpenIDConnect.Common/Infrastructure/PkceBlob.cs b/source/Server.OpenIDConnect.Common/Infrastructure/PkceBlob.cs index 151274a..26c6b17 100644 --- a/source/Server.OpenIDConnect.Common/Infrastructure/PkceBlob.cs +++ b/source/Server.OpenIDConnect.Common/Infrastructure/PkceBlob.cs @@ -4,13 +4,13 @@ namespace Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Infra { public class PkceBlob { - public Guid SessionId { get; } + public Guid RequestId { get; } public string CodeVerifier { get; } public DateTimeOffset TimeStamp { get; } - public PkceBlob(Guid sessionId, string codeVerifier, DateTimeOffset timeStamp) + public PkceBlob(Guid requestId, string codeVerifier, DateTimeOffset timeStamp) { - SessionId = sessionId; + RequestId = requestId; CodeVerifier = codeVerifier; TimeStamp = timeStamp; } diff --git a/source/Server.OpenIDConnect.Common/OpenIDConnectExtension.cs b/source/Server.OpenIDConnect.Common/OpenIDConnectExtension.cs index 6879319..3275f57 100644 --- a/source/Server.OpenIDConnect.Common/OpenIDConnectExtension.cs +++ b/source/Server.OpenIDConnect.Common/OpenIDConnectExtension.cs @@ -1,5 +1,6 @@ using Autofac; using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Infrastructure; +using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Web; namespace Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common { @@ -8,6 +9,7 @@ public abstract class OpenIDConnectExtension public virtual void Load(ContainerBuilder builder) { builder.RegisterType().As().InstancePerDependency(); + builder.RegisterType().As().InstancePerDependency(); } } } \ No newline at end of file diff --git a/source/Server.OpenIDConnect.Common/Tokens/AuthTokenHandler.cs b/source/Server.OpenIDConnect.Common/Tokens/AuthTokenHandler.cs index 89c0297..f47b434 100644 --- a/source/Server.OpenIDConnect.Common/Tokens/AuthTokenHandler.cs +++ b/source/Server.OpenIDConnect.Common/Tokens/AuthTokenHandler.cs @@ -36,7 +36,7 @@ protected AuthTokenHandler(ISystemLog log, this.keyRetriever = keyRetriever; } - protected async Task GetPrincipalFromToken(string? accessToken, string? idToken) + protected async Task GetPrincipalFromToken(string? accessToken, string? idToken) { ClaimsPrincipal? principal = null; @@ -71,7 +71,7 @@ protected async Task GetPrincipalFromToken(string? acc DoIssuerSpecificClaimsValidation(principal, out string error); - return string.IsNullOrWhiteSpace(error) ? new ClaimsPrincipleContainer(principal, GetProviderGroupIds(principal)) : new ClaimsPrincipleContainer(error); + return string.IsNullOrWhiteSpace(error) ? new ClaimsPrincipalContainer(principal, GetProviderGroupIds(principal)) : new ClaimsPrincipalContainer(error); } ClaimsPrincipal ValidateUsingSharedSecret(TokenValidationParameters validationParameters, string? tokenToValidate) diff --git a/source/Server.OpenIDConnect.Common/Tokens/ClaimsPrincipleContainer.cs b/source/Server.OpenIDConnect.Common/Tokens/ClaimsPrincipalContainer.cs similarity index 79% rename from source/Server.OpenIDConnect.Common/Tokens/ClaimsPrincipleContainer.cs rename to source/Server.OpenIDConnect.Common/Tokens/ClaimsPrincipalContainer.cs index 51046a0..39242f9 100644 --- a/source/Server.OpenIDConnect.Common/Tokens/ClaimsPrincipleContainer.cs +++ b/source/Server.OpenIDConnect.Common/Tokens/ClaimsPrincipalContainer.cs @@ -3,14 +3,14 @@ namespace Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Tokens { - public class ClaimsPrincipleContainer + public class ClaimsPrincipalContainer { - public ClaimsPrincipleContainer(string error) + public ClaimsPrincipalContainer(string error) { this.Error = error; ExternalGroupIds = Array.Empty(); } - public ClaimsPrincipleContainer(ClaimsPrincipal principal, string[] externalGroupIds) + public ClaimsPrincipalContainer(ClaimsPrincipal principal, string[] externalGroupIds) { this.Principal = principal; ExternalGroupIds = externalGroupIds; diff --git a/source/Server.OpenIDConnect.Common/Tokens/IAuthTokenHandler.cs b/source/Server.OpenIDConnect.Common/Tokens/IAuthTokenHandler.cs index a7f3ac4..5c4f3d3 100644 --- a/source/Server.OpenIDConnect.Common/Tokens/IAuthTokenHandler.cs +++ b/source/Server.OpenIDConnect.Common/Tokens/IAuthTokenHandler.cs @@ -5,6 +5,6 @@ namespace Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Token { public interface IAuthTokenHandler { - Task GetPrincipalAsync(IDictionary requestForm, out string? stateString); + Task GetPrincipalAsync(IDictionary requestForm, out string? stateString); } } \ No newline at end of file diff --git a/source/Server.OpenIDConnect.Common/Tokens/OpenIDConnectAuthTokenHandler.cs b/source/Server.OpenIDConnect.Common/Tokens/OpenIDConnectAuthTokenHandler.cs index 02ddfdd..ec0fbea 100644 --- a/source/Server.OpenIDConnect.Common/Tokens/OpenIDConnectAuthTokenHandler.cs +++ b/source/Server.OpenIDConnect.Common/Tokens/OpenIDConnectAuthTokenHandler.cs @@ -20,7 +20,7 @@ protected OpenIDConnectAuthTokenHandler( { } - public Task GetPrincipalAsync(IDictionary requestForm, out string? stateString) + public Task GetPrincipalAsync(IDictionary requestForm, out string? stateString) { stateString = null; @@ -28,7 +28,7 @@ public Task GetPrincipalAsync(IDictionary GetOrCreateUser(UserResource userResource, string[] groups, string providerName, IIdentityCreator identityCreator, bool allowAutoUserCreation, CancellationToken cancellationToken); + } +} \ No newline at end of file diff --git a/source/Server.OpenIDConnect.Common/Web/LoginStateWithSessionId.cs b/source/Server.OpenIDConnect.Common/Web/LoginStateWithRequestId.cs similarity index 56% rename from source/Server.OpenIDConnect.Common/Web/LoginStateWithSessionId.cs rename to source/Server.OpenIDConnect.Common/Web/LoginStateWithRequestId.cs index 6648cc3..7c98704 100644 --- a/source/Server.OpenIDConnect.Common/Web/LoginStateWithSessionId.cs +++ b/source/Server.OpenIDConnect.Common/Web/LoginStateWithRequestId.cs @@ -3,15 +3,15 @@ namespace Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Web { - class LoginStateWithSessionId : LoginState + internal class LoginStateWithRequestId : LoginState { - public Guid SessionId { get; set; } + public Guid RequestId { get; } - public LoginStateWithSessionId(string redirectAfterLoginTo, bool usingSecureConnection, Guid sessionId) + public LoginStateWithRequestId(string redirectAfterLoginTo, bool usingSecureConnection, Guid requestId) { RedirectAfterLoginTo = redirectAfterLoginTo; UsingSecureConnection = usingSecureConnection; - SessionId = sessionId; + RequestId = requestId; } } } \ No newline at end of file diff --git a/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedAction.cs b/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedAction.cs index 2004cc3..20aab28 100644 --- a/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedAction.cs +++ b/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedAction.cs @@ -6,7 +6,6 @@ using Newtonsoft.Json; using Octopus.Data; using Octopus.Data.Model.User; -using Octopus.Data.Storage.User; using Octopus.Diagnostics; using Octopus.Server.Extensibility.Authentication.HostServices; using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Configuration; @@ -14,10 +13,8 @@ using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Infrastructure; using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Tokens; using Octopus.Server.Extensibility.Authentication.Resources; -using Octopus.Server.Extensibility.Authentication.Resources.Identities; using Octopus.Server.Extensibility.Extensions.Infrastructure.Web.Api; using Octopus.Server.Extensibility.HostServices.Web; -using Octopus.Server.Extensibility.Results; using Octopus.Time; namespace Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Web @@ -27,135 +24,96 @@ public abstract class UserAuthenticatedAction ExecuteAsync(IOctoRequest request) { - // Step 1: Try and get all of the details from the request making sure there are no errors passed back from the external identity provider - var principalContainer = await authTokenHandler.GetPrincipalAsync(request.Form.ToDictionary(pair => pair.Key, pair => (string?)pair.Value), out var stateStringFromRequest); - var principal = principalContainer.Principal; - if (principal == null || !string.IsNullOrEmpty(principalContainer.Error)) + try { - return BadRequest($"The response from the external identity provider contained an error: {principalContainer.Error}"); - } + // Step 1: Try and get all of the details from the request making sure there are no errors passed back from the external identity provider + var principalContainer = await authTokenHandler.GetPrincipalAsync(request.Form.ToDictionary(pair => pair.Key, pair => (string?)pair.Value), out var stateStringFromRequest); + var principal = principalContainer.Principal; + UserAuthenticatedValidator.ValidatePrincipalContainer(principal, principalContainer); - // Step 2: Validate the state object we passed wasn't tampered with - const string stateDescription = "As a security precaution, Octopus ensures the state object returned from the external identity provider matches what it expected."; - var expectedStateHash = string.Empty; - if (request.Cookies.ContainsKey(UserAuthConstants.OctopusStateCookieName)) - expectedStateHash = encoder.UrlDecode(request.Cookies[UserAuthConstants.OctopusStateCookieName]); - if (string.IsNullOrWhiteSpace(expectedStateHash)) - { - return BadRequest($"User login failed: Missing State Hash Cookie. {stateDescription} In this case the Cookie containing the SHA256 hash of the state object is missing from the request."); - } + // Step 2: Validate the state object we passed wasn't tampered with + var expectedStateHash = string.Empty; + if (request.Cookies.ContainsKey(UserAuthConstants.OctopusStateCookieName)) + expectedStateHash = encoder.UrlDecode(request.Cookies[UserAuthConstants.OctopusStateCookieName]); + UserAuthenticatedValidator.ValidateExpectedStateHashIsNotEmpty(expectedStateHash); - var stateFromRequestHash = State.Protect(stateStringFromRequest); - if (stateFromRequestHash != expectedStateHash) - { - return BadRequest($"User login failed: Tampered State. {stateDescription} In this case the state object looks like it has been tampered with. The state object is '{stateStringFromRequest}'. The SHA256 hash of the state was expected to be '{expectedStateHash}' but was '{stateFromRequestHash}'."); - } + var stateFromRequestHash = State.Protect(stateStringFromRequest); + UserAuthenticatedValidator.ValidateReceivedStateIsEqualToExpectedState(stateFromRequestHash, expectedStateHash, stateStringFromRequest); - var stateFromRequest = JsonConvert.DeserializeObject(stateStringFromRequest ?? string.Empty); + var stateFromRequest = JsonConvert.DeserializeObject(stateStringFromRequest ?? string.Empty)!; - // Step 3: Validate the nonce is as we expected to prevent replay attacks - const string nonceDescription = "As a security precaution to prevent replay attacks, Octopus ensures the nonce returned in the claims from the external identity provider matches what it expected."; + // Step 3: Validate the nonce is as we expected to prevent replay attacks + var expectedNonceHash = string.Empty; + if (request.Cookies.ContainsKey(UserAuthConstants.OctopusNonceCookieName)) + expectedNonceHash = encoder.UrlDecode(request.Cookies[UserAuthConstants.OctopusNonceCookieName]); - var expectedNonceHash = string.Empty; - if (request.Cookies.ContainsKey(UserAuthConstants.OctopusNonceCookieName)) - expectedNonceHash = encoder.UrlDecode(request.Cookies[UserAuthConstants.OctopusNonceCookieName]); + UserAuthenticatedValidator.ValidateExpectedNonceHashIsNotEmpty(expectedNonceHash); - if (string.IsNullOrWhiteSpace(expectedNonceHash)) - { - return BadRequest($"User login failed: Missing Nonce Hash Cookie. {nonceDescription} In this case the Cookie containing the SHA256 hash of the nonce is missing from the request."); - } + var nonceFromClaims = principal!.Claims.FirstOrDefault(c => c.Type == "nonce"); + UserAuthenticatedValidator.ValidateNonceFromClaimsIsNotEmpty(nonceFromClaims); - var nonceFromClaims = principal.Claims.FirstOrDefault(c => c.Type == "nonce"); - if (nonceFromClaims == null) - { - return BadRequest($"User login failed: Missing Nonce Claim. {nonceDescription} In this case the 'nonce' claim is missing from the security token."); - } + var nonceFromClaimsHash = Nonce.Protect(nonceFromClaims!.Value); + UserAuthenticatedValidator.ValidateNonceFromClaimsHashIsEqualToExpectedNonce(expectedNonceHash, nonceFromClaimsHash, nonceFromClaims); - var nonceFromClaimsHash = Nonce.Protect(nonceFromClaims.Value); - if (nonceFromClaimsHash != expectedNonceHash) - { - return BadRequest($"User login failed: Tampered Nonce. {nonceDescription} In this case the nonce looks like it has been tampered with or reused. The nonce is '{nonceFromClaims}'. The SHA256 hash of the state was expected to be '{expectedNonceHash}' but was '{nonceFromClaimsHash}'."); - } - - // Step 4: Now the integrity of the request has been validated we can figure out which Octopus User this represents - var authenticationCandidate = principalToUserResourceMapper.MapToUserResource(principal); - if (authenticationCandidate.Username == null) - { - return BadRequest("Unable to determine username."); - } + // Step 4: Now the integrity of the request has been validated we can figure out which Octopus User this represents + var authenticationCandidate = principalToUserResourceMapper.MapToUserResource(principal); + UserAuthenticatedValidator.ValidateUsername(authenticationCandidate.Username); - // Step 4a: Check if this authentication attempt is already being banned - var action = loginTracker.BeforeAttempt(authenticationCandidate.Username, request.Host); - if (action == InvalidLoginAction.Ban) - { - return BadRequest("You have had too many failed login attempts in a short period of time. Please try again later."); - } + // Step 4a: Check if this authentication attempt is already being banned + var action = loginTracker.BeforeAttempt(authenticationCandidate.Username, request.Host); + UserAuthenticatedValidator.ValidateUserIsNotBanned(action); - using (var cts = new CancellationTokenSource(TimeSpan.FromMinutes(1))) - { + using var cts = new CancellationTokenSource(TimeSpan.FromMinutes(1)); // Step 4b: Try to get or create a the Octopus User this external identity represents - var userResult = GetOrCreateUser(authenticationCandidate, principalContainer.ExternalGroupIds, cts.Token); + var userResult = userService.GetOrCreateUser(authenticationCandidate, principalContainer.ExternalGroupIds, ProviderName, identityCreator, configurationStore.GetAllowAutoUserCreation(), cts.Token); if (userResult is ISuccessResult successResult) { loginTracker.RecordSucess(authenticationCandidate.Username, request.Host); - if (!successResult.Value.IsActive) - { - return BadRequest($"The Octopus User Account '{authenticationCandidate.Username}' has been disabled by an Administrator. If you believe this to be a mistake, please contact your Octopus Administrator to have your account re-enabled."); - } + UserAuthenticatedValidator.ValidateUserIsActive(successResult.Value.IsActive, authenticationCandidate.Username); - if (successResult.Value.IsService) - { - return BadRequest($"The Octopus User Account '{authenticationCandidate.Username}' is a Service Account, which are prevented from using Octopus interactively. Service Accounts are designed to authorize external systems to access the Octopus API using an API Key."); - } + UserAuthenticatedValidator.ValidateUserIsNotServiceAccount(successResult.Value.IsService, authenticationCandidate.Username); - var octoResponse = Redirect.Response(stateFromRequest.RedirectAfterLoginTo) + var octoResponse = UserAuthenticatedValidator.Redirect.Response(stateFromRequest.RedirectAfterLoginTo) .WithHeader("Expires", new[] {DateTime.UtcNow.AddYears(1).ToString("R", DateTimeFormatInfo.InvariantInfo)}) .WithCookie(new OctoCookie(UserAuthConstants.OctopusStateCookieName, Guid.NewGuid().ToString()) {HttpOnly = true, Secure = false, Expires = DateTimeOffset.MinValue}) .WithCookie(new OctoCookie(UserAuthConstants.OctopusNonceCookieName, Guid.NewGuid().ToString()) {HttpOnly = true, Secure = false, Expires = DateTimeOffset.MinValue}); @@ -179,67 +137,12 @@ public async Task ExecuteAsync(IOctoRequest request) sleep.For(1000); } - return BadRequest($"User login failed: {((IFailureResult) userResult).ErrorString}"); + throw new FailedAuthenticationException($"User login failed: {((IFailureResult) userResult).ErrorString}"); } - } - - IOctoResponseProvider BadRequest(string message) - { - log.Error(message); - return LoginFailed.Response(message); - } - - IResultFromExtension GetOrCreateUser(UserResource userResource, string[] groups, CancellationToken cancellationToken) - { - var identityToMatch = NewIdentity(userResource); - - var matchingUsers = userStore.GetByIdentity(identityToMatch); - if (matchingUsers.Count() > 1) - throw new Exception("There are multiple users with this identity. OpenID Connect identity providers do not support users with duplicate email addresses. Please remove any duplicate users, or make the email addresses unique."); - var user = matchingUsers.SingleOrDefault(); - - if (user != null) + catch (FailedAuthenticationException e) { - userStore.SetSecurityGroupIds(ProviderName, user.Id, groups, clock.GetUtcTime()); - - var identity = user.Identities.FirstOrDefault(x => MatchesProviderAndExternalId(userResource, x)); - if (identity != null) - { - return ResultFromExtension.Success(user); - } - - identity = user.Identities.FirstOrDefault(x => x.IdentityProviderName == ProviderName && x.Claims[ClaimDescriptor.EmailClaimType].Value == userResource.EmailAddress); - if (identity != null) - { - return ResultFromExtension.Success(userStore.UpdateIdentity(user.Id, identityToMatch, cancellationToken)); - } - - return ResultFromExtension.Success(userStore.AddIdentity(user.Id, identityToMatch, cancellationToken)); + return UserAuthenticatedValidator.BadRequest(log, e.Message); } - - if (!ConfigurationStore.GetAllowAutoUserCreation()) - return ResultFromExtension.Failed("User could not be located and auto user creation is not enabled."); - - var userResult = userStore.Create( - userResource.Username ?? string.Empty, - userResource.DisplayName ?? string.Empty, - userResource.EmailAddress ?? string.Empty, - cancellationToken, - new ProviderUserGroups { IdentityProviderName = ProviderName, GroupIds = groups }, - new[] { identityToMatch }); - if (userResult is IFailureResult failureResult) - return ResultFromExtension.Failed(failureResult.Errors); - return ResultFromExtension.Success(((ISuccessResult)userResult).Value); - } - - bool MatchesProviderAndExternalId(UserResource userResource, Identity x) - { - return x.IdentityProviderName == ProviderName && x.Claims.ContainsKey(IdentityCreator.ExternalIdClaimType) && x.Claims[IdentityCreator.ExternalIdClaimType].Value == userResource.ExternalId; - } - - Identity NewIdentity(UserResource userResource) - { - return identityCreator.Create(userResource); } } } diff --git a/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedPkceAction.cs b/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedPkceAction.cs index 8b164d6..f695ed7 100644 --- a/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedPkceAction.cs +++ b/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedPkceAction.cs @@ -3,6 +3,7 @@ using System.Globalization; using System.Linq; using System.Net.Http; +using System.Security.Claims; using System.Text; using System.Threading; using System.Threading.Tasks; @@ -32,53 +33,47 @@ public abstract class UserAuthenticatedPkceAction codeParameter = new("code", "Authorization code provided by the identity provider"); readonly OptionalQueryParameterProperty stateParameter = new("state", "The state value associated with the authentication session"); readonly ISystemLog log; readonly TAuthTokenHandler authTokenHandler; readonly IPrincipalToUserResourceMapper principalToUserResourceMapper; - readonly IUpdateableUserStore userStore; + readonly TStore configurationStore; readonly IAuthCookieCreator authCookieCreator; readonly IInvalidLoginTracker loginTracker; readonly ISleep sleep; readonly TIdentityCreator identityCreator; - readonly IClock clock; readonly IUrlEncoder encoder; readonly IIdentityProviderConfigDiscoverer identityProviderConfigDiscoverer; readonly IMediator mediator; - - TStore ConfigurationStore { get; } + readonly IUserService userService; protected UserAuthenticatedPkceAction(ISystemLog log, TAuthTokenHandler authTokenHandler, IPrincipalToUserResourceMapper principalToUserResourceMapper, - IUpdateableUserStore userStore, TStore configurationStore, IAuthCookieCreator authCookieCreator, IInvalidLoginTracker loginTracker, ISleep sleep, TIdentityCreator identityCreator, - IClock clock, IUrlEncoder encoder, - IIdentityProviderConfigDiscoverer identityProviderConfigDiscoverer, IMediator mediator) + IIdentityProviderConfigDiscoverer identityProviderConfigDiscoverer, + IMediator mediator, + IUserService userService) { this.log = log; this.authTokenHandler = authTokenHandler; this.principalToUserResourceMapper = principalToUserResourceMapper; - this.userStore = userStore; - ConfigurationStore = configurationStore; + this.configurationStore = configurationStore; this.authCookieCreator = authCookieCreator; this.loginTracker = loginTracker; this.sleep = sleep; this.identityCreator = identityCreator; - this.clock = clock; this.encoder = encoder; this.identityProviderConfigDiscoverer = identityProviderConfigDiscoverer; this.mediator = mediator; + this.userService = userService; } protected abstract string ProviderName { get; } @@ -88,103 +83,55 @@ public async Task ExecuteAsync(IOctoRequest request) return await request.HandleAsync(codeParameter, stateParameter, (code, state) => Handle(code, state, request)); } - async Task> RequestAuthToken(string code, string redirectUri, Guid sessionId) - { - var issuerConfig = await identityProviderConfigDiscoverer.GetConfigurationAsync(ConfigurationStore.GetIssuer() ?? string.Empty); - using var client = new HttpClient(); - var request = new HttpRequestMessage(HttpMethod.Post, issuerConfig.TokenEndpoint); - - var allBlobsBelongingToExtension = await mediator.Request(new GetAllBlobsRequest(ConfigurationStore.ConfigurationSettingsName), new CancellationToken()); - var blobs = allBlobsBelongingToExtension.Blobs.Select(b => JsonConvert.DeserializeObject(Encoding.UTF8.GetString(b))!).ToList(); - foreach (var blob in blobs) - { - // Any expired blobs should be removed as well - currently using a 30 second timer - if (blob.SessionId == sessionId || DateTimeOffset.UtcNow.Subtract(blob.TimeStamp).TotalSeconds > 30) - { - await mediator.Do(new DeleteBlobCommand(ConfigurationStore.ConfigurationSettingsName, blob.SessionId.ToString()), new CancellationToken()); - } - } - - var formValues = new Dictionary - { - ["grant_type"] = "authorization_code", - ["code"] = code, - ["redirect_uri"] = redirectUri, - ["client_id"] = ConfigurationStore.GetClientId()!, - ["client_secret"] = ConfigurationStore.GetClientSecret()!.Value, - ["code_verifier"] = blobs.Single(b => b.SessionId == sessionId).CodeVerifier - }; - request.Content = new FormUrlEncodedContent(formValues!); - var response = await client.SendAsync(request, HttpCompletionOption.ResponseContentRead); - var body = await response.Content.ReadAsStringAsync(); - return JsonConvert.DeserializeObject>(body)!; - } - async Task Handle(string code, string state, IOctoRequest request) { - var host = request.Headers.ContainsKey("Host") ? request.Headers["Host"].Single() : request.Host; - var redirectUri = $"{request.Scheme}://{host}{ConfigurationStore.RedirectUri}"; - var stateFromRequest = JsonConvert.DeserializeObject(state); - var response = await RequestAuthToken(code, redirectUri, stateFromRequest!.SessionId); + var stateFromRequest = JsonConvert.DeserializeObject(state)!; - // Step 1: Try and get all of the details from the request making sure there are no errors passed back from the external identity provider - var principalContainer = await authTokenHandler.GetPrincipalAsync(response, out var stateStringFromRequest); - var principal = principalContainer.Principal; - if (principal == null || !string.IsNullOrEmpty(principalContainer.Error)) - { - return BadRequest($"The response from the external identity provider contained an error: {principalContainer.Error}"); - } + var blobs = await GetAllPkceBlobsBelongingToExtension(); + var blobFromOriginalRequest = blobs.Single(b => b.RequestId == stateFromRequest.RequestId); + await RemoveBlob(blobFromOriginalRequest); + await RemoveExpiredBlobs(blobs); - // Step 2: Validate the state object we passed wasn't tampered with - const string stateDescription = "As a security precaution, Octopus ensures the state object returned from the external identity provider matches what it expected."; - var expectedStateHash = string.Empty; - stateStringFromRequest ??= state; - if (request.Cookies.ContainsKey(UserAuthConstants.OctopusStateCookieName)) - expectedStateHash = encoder.UrlDecode(request.Cookies[UserAuthConstants.OctopusStateCookieName]); - if (string.IsNullOrWhiteSpace(expectedStateHash)) - { - return BadRequest($"User login failed: Missing State Hash Cookie. {stateDescription} In this case the Cookie containing the SHA256 hash of the state object is missing from the request."); - } - - var stateFromRequestHash = State.Protect(stateStringFromRequest); - - if (stateFromRequestHash != expectedStateHash) - { - return BadRequest($"User login failed: Tampered State. {stateDescription} In this case the state object looks like it has been tampered with. The state object is '{stateStringFromRequest}'. The SHA256 hash of the state was expected to be '{expectedStateHash}' but was '{stateFromRequestHash}'."); - } - // Step 3: Now the integrity of the request has been validated we can figure out which Octopus User this represents - var authenticationCandidate = principalToUserResourceMapper.MapToUserResource(principal); - if (authenticationCandidate.Username == null) - { - return BadRequest("Unable to determine username."); - } - - // Step 3a: Check if this authentication attempt is already being banned - var action = loginTracker.BeforeAttempt(authenticationCandidate.Username, request.Host); - if (action == InvalidLoginAction.Ban) - { - return BadRequest("You have had too many failed login attempts in a short period of time. Please try again later."); - } + var host = request.Headers.ContainsKey("Host") ? request.Headers["Host"].Single() : request.Host; + var redirectUri = $"{request.Scheme}://{host}{configurationStore.RedirectUri}"; + var response = await RequestAuthToken(code, redirectUri, blobFromOriginalRequest.CodeVerifier); - using (var cts = new CancellationTokenSource(TimeSpan.FromMinutes(1))) + try { + // Step 1: Try and get all of the details from the request making sure there are no errors passed back from the external identity provider + var principalContainer = await authTokenHandler.GetPrincipalAsync(response, out var stateStringFromRequest); + var principal = principalContainer.Principal; + UserAuthenticatedValidator.ValidatePrincipalContainer(principal, principalContainer); + + // Step 2: Validate the state object we passed wasn't tampered with + var expectedStateHash = string.Empty; + stateStringFromRequest ??= state; + if (request.Cookies.ContainsKey(UserAuthConstants.OctopusStateCookieName)) + expectedStateHash = encoder.UrlDecode(request.Cookies[UserAuthConstants.OctopusStateCookieName]); + UserAuthenticatedValidator.ValidateExpectedStateHashIsNotEmpty(expectedStateHash); + + var stateFromRequestHash = State.Protect(stateStringFromRequest); + UserAuthenticatedValidator.ValidateReceivedStateIsEqualToExpectedState(stateFromRequestHash, expectedStateHash, stateStringFromRequest); + + // Step 3: Now the integrity of the request has been validated we can figure out which Octopus User this represents + var authenticationCandidate = principalToUserResourceMapper.MapToUserResource(principal!); + UserAuthenticatedValidator.ValidateUsername(authenticationCandidate.Username); + + // Step 3a: Check if this authentication attempt is already being banned + var action = loginTracker.BeforeAttempt(authenticationCandidate.Username, request.Host); + UserAuthenticatedValidator.ValidateUserIsNotBanned(action); + + using var cts = new CancellationTokenSource(TimeSpan.FromMinutes(1)); // Step 3b: Try to get or create a the Octopus User this external identity represents - var userResult = GetOrCreateUser(authenticationCandidate, principalContainer.ExternalGroupIds, cts.Token); + var userResult = userService.GetOrCreateUser(authenticationCandidate, principalContainer.ExternalGroupIds, ProviderName, identityCreator, configurationStore.GetAllowAutoUserCreation(), cts.Token); if (userResult is ISuccessResult successResult) { loginTracker.RecordSucess(authenticationCandidate.Username, request.Host); - if (!successResult.Value.IsActive) - { - return BadRequest($"The Octopus User Account '{authenticationCandidate.Username}' has been disabled by an Administrator. If you believe this to be a mistake, please contact your Octopus Administrator to have your account re-enabled."); - } - - if (successResult.Value.IsService) - { - return BadRequest($"The Octopus User Account '{authenticationCandidate.Username}' is a Service Account, which are prevented from using Octopus interactively. Service Accounts are designed to authorize external systems to access the Octopus API using an API Key."); - } + UserAuthenticatedValidator.ValidateUserIsActive(successResult.Value.IsActive, authenticationCandidate.Username); + UserAuthenticatedValidator.ValidateUserIsNotServiceAccount(successResult.Value.IsService, authenticationCandidate.Username); - var octoResponse = redirect.Response(stateFromRequest.RedirectAfterLoginTo) + var octoResponse = UserAuthenticatedValidator.Redirect.Response(stateFromRequest.RedirectAfterLoginTo) .WithHeader("Expires", new[] {DateTime.UtcNow.AddYears(1).ToString("R", DateTimeFormatInfo.InvariantInfo)}) .WithCookie(new OctoCookie(UserAuthConstants.OctopusStateCookieName, Guid.NewGuid().ToString()) {HttpOnly = true, Secure = false, Expires = DateTimeOffset.MinValue}) .WithCookie(new OctoCookie(UserAuthConstants.OctopusNonceCookieName, Guid.NewGuid().ToString()) {HttpOnly = true, Secure = false, Expires = DateTimeOffset.MinValue}); @@ -208,67 +155,64 @@ async Task Handle(string code, string state, IOctoRequest sleep.For(1000); } - return BadRequest($"User login failed: {((IFailureResult) userResult).ErrorString}"); + throw new FailedAuthenticationException($"User login failed: {((IFailureResult) userResult).ErrorString}"); + } + catch (FailedAuthenticationException e) + { + return UserAuthenticatedValidator.BadRequest(log, e.Message); } } - IOctoResponseProvider BadRequest(string message) + async Task> RequestAuthToken(string code, string redirectUri, string codeVerifier) { - log.Error(message); - return loginFailed.Response(message); + var issuerConfig = await identityProviderConfigDiscoverer.GetConfigurationAsync(configurationStore.GetIssuer() ?? string.Empty); + using var client = new HttpClient(); + var request = new HttpRequestMessage(HttpMethod.Post, issuerConfig.TokenEndpoint); + + var formValues = new Dictionary + { + ["grant_type"] = "authorization_code", + ["code"] = code, + ["redirect_uri"] = redirectUri, + ["client_id"] = configurationStore.GetClientId()!, + ["client_secret"] = configurationStore.GetClientSecret()!.Value, + ["code_verifier"] = codeVerifier + }; + request.Content = new FormUrlEncodedContent(formValues!); + var response = await client.SendAsync(request, HttpCompletionOption.ResponseContentRead); + var body = await response.Content.ReadAsStringAsync(); + return JsonConvert.DeserializeObject>(body)!; } - IResultFromExtension GetOrCreateUser(UserResource userResource, string[] groups, CancellationToken cancellationToken) + async Task> GetAllPkceBlobsBelongingToExtension() { - var identityToMatch = NewIdentity(userResource); - - var matchingUsers = userStore.GetByIdentity(identityToMatch); - if (matchingUsers.Count() > 1) - throw new Exception("There are multiple users with this identity. OpenID Connect identity providers do not support users with duplicate email addresses. Please remove any duplicate users, or make the email addresses unique."); - var user = matchingUsers.SingleOrDefault(); - - if (user != null) + var allBlobsBelongingToExtension = await mediator.Request(new GetAllBlobsRequest(configurationStore.ConfigurationSettingsName), new CancellationToken()); + var pkceBlobs = new List(); + foreach (var blob in allBlobsBelongingToExtension.Blobs) { - userStore.SetSecurityGroupIds(ProviderName, user.Id, groups, clock.GetUtcTime()); - - var identity = user.Identities.FirstOrDefault(x => MatchesProviderAndExternalId(userResource, x)); - if (identity != null) + try { - return ResultFromExtension.Success(user); + pkceBlobs.Add(JsonConvert.DeserializeObject(Encoding.UTF8.GetString(blob))!); } - - identity = user.Identities.FirstOrDefault(x => x.IdentityProviderName == ProviderName && x.Claims[ClaimDescriptor.EmailClaimType].Value == userResource.EmailAddress); - if (identity != null) + catch (Exception e) { - return ResultFromExtension.Success(userStore.UpdateIdentity(user.Id, identityToMatch, cancellationToken)); + log.Warn($"Could not parse blob. This is most likely not a PkceBlob and will be skipped: {e.Message}"); } - - return ResultFromExtension.Success(userStore.AddIdentity(user.Id, identityToMatch, cancellationToken)); } - - if (!ConfigurationStore.GetAllowAutoUserCreation()) - return ResultFromExtension.Failed("User could not be located and auto user creation is not enabled."); - - var userResult = userStore.Create( - userResource.Username ?? string.Empty, - userResource.DisplayName ?? string.Empty, - userResource.EmailAddress ?? string.Empty, - cancellationToken, - new ProviderUserGroups { IdentityProviderName = ProviderName, GroupIds = groups }, - new[] { identityToMatch }); - if (userResult is IFailureResult failureResult) - return ResultFromExtension.Failed(failureResult.Errors); - return ResultFromExtension.Success(((ISuccessResult)userResult).Value); + return pkceBlobs; } - bool MatchesProviderAndExternalId(UserResource userResource, Identity x) + async Task RemoveExpiredBlobs(IEnumerable blobs) { - return x.IdentityProviderName == ProviderName && x.Claims.ContainsKey(IdentityCreator.ExternalIdClaimType) && x.Claims[IdentityCreator.ExternalIdClaimType].Value == userResource.ExternalId; + foreach (var blob in blobs.Where(blob => DateTimeOffset.UtcNow.Subtract(blob.TimeStamp).TotalSeconds > 30)) + { + await RemoveBlob(blob); + } } - Identity NewIdentity(UserResource userResource) + async Task RemoveBlob(PkceBlob blob) { - return identityCreator.Create(userResource); + await mediator.Do(new DeleteBlobCommand(configurationStore.ConfigurationSettingsName, blob.RequestId.ToString()), new CancellationToken()); } } } diff --git a/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedValidator.cs b/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedValidator.cs new file mode 100644 index 0000000..7d1068c --- /dev/null +++ b/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedValidator.cs @@ -0,0 +1,104 @@ +using System; +using System.Security.Claims; +using Octopus.Diagnostics; +using Octopus.Server.Extensibility.Authentication.HostServices; +using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Tokens; +using Octopus.Server.Extensibility.Extensions.Infrastructure.Web.Api; + +namespace Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Web +{ + public static class UserAuthenticatedValidator + { + public static readonly BadRequestRegistration LoginFailed = new("User login failed"); + public static readonly RedirectRegistration Redirect = new("Redirects back to the Octopus portal"); + + const string StateDescription = "As a security precaution, Octopus ensures the state object returned from the external identity provider matches what it expected."; + const string NonceDescription = "As a security precaution to prevent replay attacks, Octopus ensures the nonce returned in the claims from the external identity provider matches what it expected."; + + public static void ValidatePrincipalContainer(ClaimsPrincipal? principal, ClaimsPrincipalContainer principalContainer) + { + if (principal == null || !string.IsNullOrEmpty(principalContainer.Error)) + { + throw new FailedAuthenticationException($"The response from the external identity provider contained an error: {principalContainer.Error}"); + } + } + + public static void ValidateExpectedStateHashIsNotEmpty(string? expectedStateHash) + { + if (string.IsNullOrWhiteSpace(expectedStateHash)) + { + throw new FailedAuthenticationException($"User login failed: Missing State Hash Cookie. {StateDescription} In this case the Cookie containing the SHA256 hash of the state object is missing from the request."); + } + } + + public static void ValidateReceivedStateIsEqualToExpectedState(string stateFromRequestHash, string expectedStateHash, string? stateStringFromRequest) + { + if (stateFromRequestHash != expectedStateHash) + { + throw new FailedAuthenticationException($"User login failed: Tampered State. {StateDescription} In this case the state object looks like it has been tampered with. The state object is '{stateStringFromRequest}'. The SHA256 hash of the state was expected to be '{expectedStateHash}' but was '{stateFromRequestHash}'."); + } + } + + public static void ValidateUsername(string? username) + { + if (username == null) + { + throw new FailedAuthenticationException("Unable to determine username."); + } + } + + public static void ValidateUserIsNotBanned(InvalidLoginAction action) + { + if (action == InvalidLoginAction.Ban) + { + throw new FailedAuthenticationException("You have had too many failed login attempts in a short period of time. Please try again later."); + } + } + + public static void ValidateUserIsActive(bool isActive, string username) + { + if (!isActive) + { + throw new FailedAuthenticationException($"The Octopus User Account '{username}' has been disabled by an Administrator. If you believe this to be a mistake, please contact your Octopus Administrator to have your account re-enabled."); + } + } + + public static void ValidateUserIsNotServiceAccount(bool isService, string username) + { + if (isService) + { + throw new FailedAuthenticationException($"The Octopus User Account '{username}' is a Service Account, which are prevented from using Octopus interactively. Service Accounts are designed to authorize external systems to access the Octopus API using an API Key."); + } + } + + public static void ValidateExpectedNonceHashIsNotEmpty(string? expectedNonceHash) + { + if (string.IsNullOrWhiteSpace(expectedNonceHash)) + { + throw new FailedAuthenticationException($"User login failed: Missing Nonce Hash Cookie. {NonceDescription} In this case the Cookie containing the SHA256 hash of the nonce is missing from the request."); + } + } + + public static void ValidateNonceFromClaimsIsNotEmpty(Claim? nonceFromClaims) + { + if (nonceFromClaims == null) + { + throw new FailedAuthenticationException($"User login failed: Missing Nonce Claim. {NonceDescription} In this case the 'nonce' claim is missing from the security token."); + } + } + + public static void ValidateNonceFromClaimsHashIsEqualToExpectedNonce(string nonceFromClaimsHash, string expectedNonceHash, Claim nonceFromClaims) + { + if (nonceFromClaimsHash != expectedNonceHash) + { + throw new FailedAuthenticationException($"User login failed: Tampered Nonce. {NonceDescription} In this case the nonce looks like it has been tampered with or reused. The nonce is '{nonceFromClaims}'. The SHA256 hash of the state was expected to be '{expectedNonceHash}' but was '{nonceFromClaimsHash}'."); + } + } + + public static IOctoResponseProvider BadRequest(ILog log, string message) + { + log.Error(message); + return LoginFailed.Response(message); + } + } +} \ No newline at end of file diff --git a/source/Server.OpenIDConnect.Common/Web/UserAuthenticationAction.cs b/source/Server.OpenIDConnect.Common/Web/UserAuthenticationAction.cs index 1407691..c0842b0 100644 --- a/source/Server.OpenIDConnect.Common/Web/UserAuthenticationAction.cs +++ b/source/Server.OpenIDConnect.Common/Web/UserAuthenticationAction.cs @@ -80,7 +80,7 @@ public async Task ExecuteAsync(IOctoRequest request) var issuerConfig = await identityProviderConfigDiscoverer.GetConfigurationAsync(issuer); var response = ConfigurationStore.HasClientSecret - ? await BuildAuthorizationCodePkceResponse(model, new LoginStateWithSessionId(state.RedirectAfterLoginTo, state.UsingSecureConnection, Guid.NewGuid()), issuerConfig) + ? await BuildAuthorizationCodePkceResponse(model, new LoginStateWithRequestId(state.RedirectAfterLoginTo, state.UsingSecureConnection, Guid.NewGuid()), issuerConfig) : BuildHybridResponse(model, state, issuerConfig); return response; @@ -97,11 +97,11 @@ public async Task ExecuteAsync(IOctoRequest request) } } - async Task BuildAuthorizationCodePkceResponse(LoginRedirectLinkRequestModel model, LoginStateWithSessionId state, IssuerConfiguration issuerConfig) + async Task BuildAuthorizationCodePkceResponse(LoginRedirectLinkRequestModel model, LoginStateWithRequestId state, IssuerConfiguration issuerConfig) { var codeVerifier = Pkce.GenerateCodeVerifier(); - var pkceBlob = new PkceBlob(state.SessionId, codeVerifier, DateTimeOffset.UtcNow); - await mediator.Do(new PutBlobCommand(ConfigurationStore.ConfigurationSettingsName, state.SessionId.ToString(), JsonSerializer.SerializeToUtf8Bytes(pkceBlob)), new CancellationToken()); + var pkceBlob = new PkceBlob(state.RequestId, codeVerifier, DateTimeOffset.UtcNow); + await mediator.Do(new PutBlobCommand(ConfigurationStore.ConfigurationSettingsName, state.RequestId.ToString(), JsonSerializer.SerializeToUtf8Bytes(pkceBlob)), new CancellationToken()); var stateString = JsonConvert.SerializeObject(state); var codeChallenge = Pkce.GenerateCodeChallenge(codeVerifier); diff --git a/source/Server.OpenIDConnect.Common/Web/UserService.cs b/source/Server.OpenIDConnect.Common/Web/UserService.cs new file mode 100644 index 0000000..5240a6b --- /dev/null +++ b/source/Server.OpenIDConnect.Common/Web/UserService.cs @@ -0,0 +1,86 @@ +using System; +using System.Linq; +using System.Threading; +using Octopus.Data; +using Octopus.Data.Model.User; +using Octopus.Data.Storage.User; +using Octopus.Server.Extensibility.Authentication.HostServices; +using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Identities; +using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Infrastructure; +using Octopus.Server.Extensibility.Authentication.Resources.Identities; +using Octopus.Server.Extensibility.Results; +using Octopus.Time; + +namespace Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Web +{ + public class UserService : IUserService + { + readonly IUpdateableUserStore userStore; + readonly IClock clock; + + public UserService(IUpdateableUserStore userStore, IClock clock) + { + this.userStore = userStore; + this.clock = clock; + } + + public IResultFromExtension GetOrCreateUser( + UserResource userResource, + string[] groups, + string providerName, + IIdentityCreator identityCreator, + bool allowAutoUserCreation, + CancellationToken cancellationToken) + { + var identityToMatch = NewIdentity(userResource, identityCreator); + + var matchingUsers = userStore.GetByIdentity(identityToMatch); + if (matchingUsers.Length > 1) + throw new Exception("There are multiple users with this identity. OpenID Connect identity providers do not support users with duplicate email addresses. Please remove any duplicate users, or make the email addresses unique."); + var user = matchingUsers.SingleOrDefault(); + + if (user != null) + { + userStore.SetSecurityGroupIds(providerName, user.Id, groups, clock.GetUtcTime()); + + var identity = user.Identities.FirstOrDefault(x => MatchesProviderAndExternalId(userResource, x, providerName)); + if (identity != null) + { + return ResultFromExtension.Success(user); + } + + identity = user.Identities.FirstOrDefault(x => x.IdentityProviderName == providerName && x.Claims[ClaimDescriptor.EmailClaimType].Value == userResource.EmailAddress); + if (identity != null) + { + return ResultFromExtension.Success(userStore.UpdateIdentity(user.Id, identityToMatch, cancellationToken)); + } + + return ResultFromExtension.Success(userStore.AddIdentity(user.Id, identityToMatch, cancellationToken)); + } + + if (!allowAutoUserCreation) + return ResultFromExtension.Failed("User could not be located and auto user creation is not enabled."); + + var userResult = userStore.Create( + userResource.Username ?? string.Empty, + userResource.DisplayName ?? string.Empty, + userResource.EmailAddress ?? string.Empty, + cancellationToken, + new ProviderUserGroups { IdentityProviderName = providerName, GroupIds = groups }, + new[] { identityToMatch }); + if (userResult is IFailureResult failureResult) + return ResultFromExtension.Failed(failureResult.Errors); + return ResultFromExtension.Success(((ISuccessResult)userResult).Value); + } + + bool MatchesProviderAndExternalId(UserResource userResource, Identity identity, string providerName) + { + return identity.IdentityProviderName == providerName && identity.Claims.ContainsKey(IdentityCreator.ExternalIdClaimType) && identity.Claims[IdentityCreator.ExternalIdClaimType].Value == userResource.ExternalId; + } + + Identity NewIdentity(UserResource userResource, IIdentityCreator identityCreator) + { + return identityCreator.Create(userResource); + } + } +} \ No newline at end of file From e593661e712333dc39efd58f8ff3a4824a7bd575 Mon Sep 17 00:00:00 2001 From: Nelson Susanto Date: Mon, 11 Apr 2022 14:52:51 +1000 Subject: [PATCH 22/27] Update MessageContracts version --- .../Server.OpenIDConnect.Common.csproj | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/Server.OpenIDConnect.Common/Server.OpenIDConnect.Common.csproj b/source/Server.OpenIDConnect.Common/Server.OpenIDConnect.Common.csproj index ce081c7..2d4ef57 100644 --- a/source/Server.OpenIDConnect.Common/Server.OpenIDConnect.Common.csproj +++ b/source/Server.OpenIDConnect.Common/Server.OpenIDConnect.Common.csproj @@ -19,7 +19,7 @@ - + From 4762155845cf26e6956e76b13b42425dd49f966a Mon Sep 17 00:00:00 2001 From: Nelson Susanto Date: Mon, 11 Apr 2022 14:56:42 +1000 Subject: [PATCH 23/27] Remove imports --- .../Web/UserAuthenticatedPkceAction.cs | 4 ---- .../Web/UserAuthenticatedValidator.cs | 3 +-- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedPkceAction.cs b/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedPkceAction.cs index f695ed7..c61d40b 100644 --- a/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedPkceAction.cs +++ b/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedPkceAction.cs @@ -3,14 +3,12 @@ using System.Globalization; using System.Linq; using System.Net.Http; -using System.Security.Claims; using System.Text; using System.Threading; using System.Threading.Tasks; using Newtonsoft.Json; using Octopus.Data; using Octopus.Data.Model.User; -using Octopus.Data.Storage.User; using Octopus.Diagnostics; using Octopus.Server.Extensibility.Authentication.HostServices; using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Configuration; @@ -18,11 +16,9 @@ using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Infrastructure; using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Issuer; using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Tokens; -using Octopus.Server.Extensibility.Authentication.Resources.Identities; using Octopus.Server.Extensibility.Extensions.Infrastructure.Web.Api; using Octopus.Server.Extensibility.HostServices.Web; using Octopus.Server.Extensibility.Mediator; -using Octopus.Server.Extensibility.Results; using Octopus.Server.MessageContracts.Features.BlobStorage; using Octopus.Time; diff --git a/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedValidator.cs b/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedValidator.cs index 7d1068c..4239e9c 100644 --- a/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedValidator.cs +++ b/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedValidator.cs @@ -1,5 +1,4 @@ -using System; -using System.Security.Claims; +using System.Security.Claims; using Octopus.Diagnostics; using Octopus.Server.Extensibility.Authentication.HostServices; using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Tokens; From 232a483b9046a9b09cc8e715002cef849f777a78 Mon Sep 17 00:00:00 2001 From: Nelson Susanto Date: Thu, 14 Apr 2022 10:46:28 +1000 Subject: [PATCH 24/27] refactor wip --- .../Web/AuthServerResponseHandler.cs | 121 ++++++++++++++++++ .../Web/UserAuthenticatedAction.cs | 79 +++--------- .../Web/UserAuthenticatedPkceAction.cs | 60 ++------- .../Web/UserAuthenticatedValidator.cs | 4 +- 4 files changed, 153 insertions(+), 111 deletions(-) create mode 100644 source/Server.OpenIDConnect.Common/Web/AuthServerResponseHandler.cs diff --git a/source/Server.OpenIDConnect.Common/Web/AuthServerResponseHandler.cs b/source/Server.OpenIDConnect.Common/Web/AuthServerResponseHandler.cs new file mode 100644 index 0000000..f20a5cc --- /dev/null +++ b/source/Server.OpenIDConnect.Common/Web/AuthServerResponseHandler.cs @@ -0,0 +1,121 @@ +using System; +using System.Collections.Generic; +using System.Globalization; +using System.Linq; +using System.Threading.Tasks; +using Octopus.Data; +using Octopus.Data.Model.User; +using Octopus.Server.Extensibility.Authentication.HostServices; +using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Infrastructure; +using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Tokens; +using Octopus.Server.Extensibility.Authentication.Resources; +using Octopus.Server.Extensibility.Extensions.Infrastructure.Web.Api; +using Octopus.Server.Extensibility.HostServices.Web; +using Octopus.Time; + +namespace Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Web +{ + public class AuthServerResponseHandler where TAuthTokenHandler : IAuthTokenHandler + { + readonly TAuthTokenHandler authTokenHandler; + readonly IPrincipalToUserResourceMapper principalToUserResourceMapper; + readonly IAuthCookieCreator authCookieCreator; + readonly IInvalidLoginTracker loginTracker; + readonly ISleep sleep; + readonly IUrlEncoder encoder; + + public AuthServerResponseHandler( + TAuthTokenHandler authTokenHandler, + IPrincipalToUserResourceMapper principalToUserResourceMapper, + IAuthCookieCreator authCookieCreator, + IInvalidLoginTracker loginTracker, + ISleep sleep, + IUrlEncoder encoder) + { + this.authTokenHandler = authTokenHandler; + this.principalToUserResourceMapper = principalToUserResourceMapper; + this.authCookieCreator = authCookieCreator; + this.loginTracker = loginTracker; + this.sleep = sleep; + this.encoder = encoder; + } + + public async Task GetDetailsFromRequestEnsuringNoErrors(IDictionary requestForm) + { + var principalContainer = await authTokenHandler.GetPrincipalAsync(requestForm, out _); + UserAuthenticatedValidator.ValidatePrincipalContainer(principalContainer); + + return principalContainer; + } + + public void ValidateState(IOctoRequest request, string state) + { + var expectedStateHash = request.Cookies.ContainsKey(UserAuthConstants.OctopusStateCookieName) + ? encoder.UrlDecode(request.Cookies[UserAuthConstants.OctopusStateCookieName]) + : string.Empty; + UserAuthenticatedValidator.ValidateExpectedStateHashIsNotEmpty(expectedStateHash); + UserAuthenticatedValidator.ValidateReceivedStateIsEqualToExpectedState(State.Protect(state), expectedStateHash, state); + } + + public void ValidateNonce(IOctoRequest request, ClaimsPrincipalContainer principalContainer) + { + var expectedNonceHash = string.Empty; + if (request.Cookies.ContainsKey(UserAuthConstants.OctopusNonceCookieName)) + expectedNonceHash = encoder.UrlDecode(request.Cookies[UserAuthConstants.OctopusNonceCookieName]); + + UserAuthenticatedValidator.ValidateExpectedNonceHashIsNotEmpty(expectedNonceHash); + + var nonceFromClaims = principalContainer.Principal!.Claims.FirstOrDefault(c => c.Type == "nonce"); + UserAuthenticatedValidator.ValidateNonceFromClaimsIsNotEmpty(nonceFromClaims); + + var nonceFromClaimsHash = Nonce.Protect(nonceFromClaims!.Value); + UserAuthenticatedValidator.ValidateNonceFromClaimsHashIsEqualToExpectedNonce(expectedNonceHash, nonceFromClaimsHash, nonceFromClaims); + } + + public UserResource GetUserDetails(ClaimsPrincipalContainer principalContainer) + { + var authenticationCandidate = principalToUserResourceMapper.MapToUserResource(principalContainer.Principal!); + UserAuthenticatedValidator.ValidateUsername(authenticationCandidate.Username); + return authenticationCandidate; + } + + public InvalidLoginAction CheckIfAuthenticationAttemptIsBanned(string username, string host) + { + var action = loginTracker.BeforeAttempt(username, host); + UserAuthenticatedValidator.ValidateUserIsNotBanned(action); + return action; + } + + public IOctoResponseProvider Success(IOctoRequest request, ISuccessResult successResult, string username, LoginState state) + { + loginTracker.RecordSucess(username, request.Host); + + UserAuthenticatedValidator.ValidateUserIsActive(successResult.Value.IsActive, username); + UserAuthenticatedValidator.ValidateUserIsNotServiceAccount(successResult.Value.IsService, username); + + var octoResponse = UserAuthenticatedValidator.Redirect.Response(state.RedirectAfterLoginTo) + .WithHeader("Expires", new[] {DateTime.UtcNow.AddYears(1).ToString("R", DateTimeFormatInfo.InvariantInfo)}) + .WithCookie(new OctoCookie(UserAuthConstants.OctopusStateCookieName, Guid.NewGuid().ToString()) {HttpOnly = true, Secure = false, Expires = DateTimeOffset.MinValue}) + .WithCookie(new OctoCookie(UserAuthConstants.OctopusNonceCookieName, Guid.NewGuid().ToString()) {HttpOnly = true, Secure = false, Expires = DateTimeOffset.MinValue}); + + var authCookies = authCookieCreator.CreateAuthCookies(successResult.Value.IdentificationToken, TimeSpan.FromDays(20), request.IsHttps, state.UsingSecureConnection); + + foreach (var cookie in authCookies) + { + octoResponse = octoResponse.WithCookie(cookie); + } + + return octoResponse; + } + + public void Failure(string username, string host, InvalidLoginAction action) + { + loginTracker.RecordFailure(username, host); + + if (action == InvalidLoginAction.Slow) + { + sleep.For(1000); + } + } + } +} \ No newline at end of file diff --git a/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedAction.cs b/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedAction.cs index 20aab28..f0331de 100644 --- a/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedAction.cs +++ b/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedAction.cs @@ -63,80 +63,35 @@ protected UserAuthenticatedAction( public async Task ExecuteAsync(IOctoRequest request) { + var authServerResponseHandler = new AuthServerResponseHandler( + authTokenHandler, + principalToUserResourceMapper, + authCookieCreator, + loginTracker, + sleep, + encoder); try { - // Step 1: Try and get all of the details from the request making sure there are no errors passed back from the external identity provider - var principalContainer = await authTokenHandler.GetPrincipalAsync(request.Form.ToDictionary(pair => pair.Key, pair => (string?)pair.Value), out var stateStringFromRequest); - var principal = principalContainer.Principal; - UserAuthenticatedValidator.ValidatePrincipalContainer(principal, principalContainer); + var principalContainer = await authServerResponseHandler.GetDetailsFromRequestEnsuringNoErrors(request.Form.ToDictionary(pair => pair.Key, pair => (string?)pair.Value)); + var stateStringFromRequest = request.Form.ContainsKey("state") ? request.Form["state"] : string.Empty; - // Step 2: Validate the state object we passed wasn't tampered with - var expectedStateHash = string.Empty; - if (request.Cookies.ContainsKey(UserAuthConstants.OctopusStateCookieName)) - expectedStateHash = encoder.UrlDecode(request.Cookies[UserAuthConstants.OctopusStateCookieName]); - UserAuthenticatedValidator.ValidateExpectedStateHashIsNotEmpty(expectedStateHash); + authServerResponseHandler.ValidateState(request, stateStringFromRequest); + authServerResponseHandler.ValidateNonce(request, principalContainer); - var stateFromRequestHash = State.Protect(stateStringFromRequest); - UserAuthenticatedValidator.ValidateReceivedStateIsEqualToExpectedState(stateFromRequestHash, expectedStateHash, stateStringFromRequest); + var authenticationCandidate = authServerResponseHandler.GetUserDetails(principalContainer); + var username = authenticationCandidate.Username!; - var stateFromRequest = JsonConvert.DeserializeObject(stateStringFromRequest ?? string.Empty)!; - - // Step 3: Validate the nonce is as we expected to prevent replay attacks - var expectedNonceHash = string.Empty; - if (request.Cookies.ContainsKey(UserAuthConstants.OctopusNonceCookieName)) - expectedNonceHash = encoder.UrlDecode(request.Cookies[UserAuthConstants.OctopusNonceCookieName]); - - UserAuthenticatedValidator.ValidateExpectedNonceHashIsNotEmpty(expectedNonceHash); - - var nonceFromClaims = principal!.Claims.FirstOrDefault(c => c.Type == "nonce"); - UserAuthenticatedValidator.ValidateNonceFromClaimsIsNotEmpty(nonceFromClaims); - - var nonceFromClaimsHash = Nonce.Protect(nonceFromClaims!.Value); - UserAuthenticatedValidator.ValidateNonceFromClaimsHashIsEqualToExpectedNonce(expectedNonceHash, nonceFromClaimsHash, nonceFromClaims); - - // Step 4: Now the integrity of the request has been validated we can figure out which Octopus User this represents - var authenticationCandidate = principalToUserResourceMapper.MapToUserResource(principal); - UserAuthenticatedValidator.ValidateUsername(authenticationCandidate.Username); - - // Step 4a: Check if this authentication attempt is already being banned - var action = loginTracker.BeforeAttempt(authenticationCandidate.Username, request.Host); - UserAuthenticatedValidator.ValidateUserIsNotBanned(action); + var action = authServerResponseHandler.CheckIfAuthenticationAttemptIsBanned(username, request.Host); + var stateFromRequest = JsonConvert.DeserializeObject(stateStringFromRequest)!; using var cts = new CancellationTokenSource(TimeSpan.FromMinutes(1)); - // Step 4b: Try to get or create a the Octopus User this external identity represents var userResult = userService.GetOrCreateUser(authenticationCandidate, principalContainer.ExternalGroupIds, ProviderName, identityCreator, configurationStore.GetAllowAutoUserCreation(), cts.Token); if (userResult is ISuccessResult successResult) { - loginTracker.RecordSucess(authenticationCandidate.Username, request.Host); - - UserAuthenticatedValidator.ValidateUserIsActive(successResult.Value.IsActive, authenticationCandidate.Username); - - UserAuthenticatedValidator.ValidateUserIsNotServiceAccount(successResult.Value.IsService, authenticationCandidate.Username); - - var octoResponse = UserAuthenticatedValidator.Redirect.Response(stateFromRequest.RedirectAfterLoginTo) - .WithHeader("Expires", new[] {DateTime.UtcNow.AddYears(1).ToString("R", DateTimeFormatInfo.InvariantInfo)}) - .WithCookie(new OctoCookie(UserAuthConstants.OctopusStateCookieName, Guid.NewGuid().ToString()) {HttpOnly = true, Secure = false, Expires = DateTimeOffset.MinValue}) - .WithCookie(new OctoCookie(UserAuthConstants.OctopusNonceCookieName, Guid.NewGuid().ToString()) {HttpOnly = true, Secure = false, Expires = DateTimeOffset.MinValue}); - - var authCookies = authCookieCreator.CreateAuthCookies(successResult.Value.IdentificationToken, TimeSpan.FromDays(20), request.IsHttps, stateFromRequest.UsingSecureConnection); - - foreach (var cookie in authCookies) - { - octoResponse = octoResponse.WithCookie(cookie); - } - - return octoResponse; - } - - // Step 5: Handle other types of failures - loginTracker.RecordFailure(authenticationCandidate.Username, request.Host); - - // Step 5a: Slow this potential attacker down a bit since they seem to keep failing - if (action == InvalidLoginAction.Slow) - { - sleep.For(1000); + return authServerResponseHandler.Success(request, successResult, authenticationCandidate.Username!, stateFromRequest); } + authServerResponseHandler.Failure(authenticationCandidate.Username!, request.Host, action); throw new FailedAuthenticationException($"User login failed: {((IFailureResult) userResult).ErrorString}"); } catch (FailedAuthenticationException e) diff --git a/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedPkceAction.cs b/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedPkceAction.cs index c61d40b..3ae5dbf 100644 --- a/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedPkceAction.cs +++ b/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedPkceAction.cs @@ -92,65 +92,31 @@ async Task Handle(string code, string state, IOctoRequest var redirectUri = $"{request.Scheme}://{host}{configurationStore.RedirectUri}"; var response = await RequestAuthToken(code, redirectUri, blobFromOriginalRequest.CodeVerifier); + var authServerResponseHandler = new AuthServerResponseHandler( + authTokenHandler, + principalToUserResourceMapper, + authCookieCreator, + loginTracker, + sleep, + encoder); try { - // Step 1: Try and get all of the details from the request making sure there are no errors passed back from the external identity provider - var principalContainer = await authTokenHandler.GetPrincipalAsync(response, out var stateStringFromRequest); - var principal = principalContainer.Principal; - UserAuthenticatedValidator.ValidatePrincipalContainer(principal, principalContainer); + var principalContainer = await authServerResponseHandler.GetDetailsFromRequestEnsuringNoErrors(response); - // Step 2: Validate the state object we passed wasn't tampered with - var expectedStateHash = string.Empty; - stateStringFromRequest ??= state; - if (request.Cookies.ContainsKey(UserAuthConstants.OctopusStateCookieName)) - expectedStateHash = encoder.UrlDecode(request.Cookies[UserAuthConstants.OctopusStateCookieName]); - UserAuthenticatedValidator.ValidateExpectedStateHashIsNotEmpty(expectedStateHash); + authServerResponseHandler.ValidateState(request, state); - var stateFromRequestHash = State.Protect(stateStringFromRequest); - UserAuthenticatedValidator.ValidateReceivedStateIsEqualToExpectedState(stateFromRequestHash, expectedStateHash, stateStringFromRequest); + var authenticationCandidate = authServerResponseHandler.GetUserDetails(principalContainer); - // Step 3: Now the integrity of the request has been validated we can figure out which Octopus User this represents - var authenticationCandidate = principalToUserResourceMapper.MapToUserResource(principal!); - UserAuthenticatedValidator.ValidateUsername(authenticationCandidate.Username); - - // Step 3a: Check if this authentication attempt is already being banned - var action = loginTracker.BeforeAttempt(authenticationCandidate.Username, request.Host); - UserAuthenticatedValidator.ValidateUserIsNotBanned(action); + var action = authServerResponseHandler.CheckIfAuthenticationAttemptIsBanned(authenticationCandidate.Username!, request.Host); using var cts = new CancellationTokenSource(TimeSpan.FromMinutes(1)); - // Step 3b: Try to get or create a the Octopus User this external identity represents var userResult = userService.GetOrCreateUser(authenticationCandidate, principalContainer.ExternalGroupIds, ProviderName, identityCreator, configurationStore.GetAllowAutoUserCreation(), cts.Token); if (userResult is ISuccessResult successResult) { - loginTracker.RecordSucess(authenticationCandidate.Username, request.Host); - - UserAuthenticatedValidator.ValidateUserIsActive(successResult.Value.IsActive, authenticationCandidate.Username); - UserAuthenticatedValidator.ValidateUserIsNotServiceAccount(successResult.Value.IsService, authenticationCandidate.Username); - - var octoResponse = UserAuthenticatedValidator.Redirect.Response(stateFromRequest.RedirectAfterLoginTo) - .WithHeader("Expires", new[] {DateTime.UtcNow.AddYears(1).ToString("R", DateTimeFormatInfo.InvariantInfo)}) - .WithCookie(new OctoCookie(UserAuthConstants.OctopusStateCookieName, Guid.NewGuid().ToString()) {HttpOnly = true, Secure = false, Expires = DateTimeOffset.MinValue}) - .WithCookie(new OctoCookie(UserAuthConstants.OctopusNonceCookieName, Guid.NewGuid().ToString()) {HttpOnly = true, Secure = false, Expires = DateTimeOffset.MinValue}); - - var authCookies = authCookieCreator.CreateAuthCookies(successResult.Value.IdentificationToken, TimeSpan.FromDays(20), request.IsHttps, stateFromRequest.UsingSecureConnection); - - foreach (var cookie in authCookies) - { - octoResponse = octoResponse.WithCookie(cookie); - } - - return octoResponse; - } - - // Step 4: Handle other types of failures - loginTracker.RecordFailure(authenticationCandidate.Username, request.Host); - - // Step 4a: Slow this potential attacker down a bit since they seem to keep failing - if (action == InvalidLoginAction.Slow) - { - sleep.For(1000); + return authServerResponseHandler.Success(request, successResult, authenticationCandidate.Username!, stateFromRequest); } + authServerResponseHandler.Failure(authenticationCandidate.Username!, request.Host, action); throw new FailedAuthenticationException($"User login failed: {((IFailureResult) userResult).ErrorString}"); } catch (FailedAuthenticationException e) diff --git a/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedValidator.cs b/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedValidator.cs index 4239e9c..0e623aa 100644 --- a/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedValidator.cs +++ b/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedValidator.cs @@ -14,9 +14,9 @@ public static class UserAuthenticatedValidator const string StateDescription = "As a security precaution, Octopus ensures the state object returned from the external identity provider matches what it expected."; const string NonceDescription = "As a security precaution to prevent replay attacks, Octopus ensures the nonce returned in the claims from the external identity provider matches what it expected."; - public static void ValidatePrincipalContainer(ClaimsPrincipal? principal, ClaimsPrincipalContainer principalContainer) + public static void ValidatePrincipalContainer(ClaimsPrincipalContainer principalContainer) { - if (principal == null || !string.IsNullOrEmpty(principalContainer.Error)) + if (principalContainer.Principal == null || !string.IsNullOrEmpty(principalContainer.Error)) { throw new FailedAuthenticationException($"The response from the external identity provider contained an error: {principalContainer.Error}"); } From c2eea8e5b2eb29d2c9723be67e2ee0472cb57364 Mon Sep 17 00:00:00 2001 From: Nelson Susanto Date: Thu, 14 Apr 2022 12:20:33 +1000 Subject: [PATCH 25/27] Refactor into handler --- .../OpenIDConnectConfiguration.cs | 4 ++- .../Web/AuthServerResponseHandler.cs | 6 ++-- .../Web/IUserService.cs | 2 +- .../Web/UserAuthenticatedAction.cs | 14 +++----- .../Web/UserAuthenticatedPkceAction.cs | 32 ++++++++++--------- .../Web/UserService.cs | 11 ++++--- 6 files changed, 35 insertions(+), 34 deletions(-) diff --git a/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfiguration.cs b/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfiguration.cs index 0d67dba..d974a1a 100644 --- a/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfiguration.cs +++ b/source/Server.OpenIDConnect.Common/Configuration/OpenIDConnectConfiguration.cs @@ -1,4 +1,5 @@ -using Octopus.Data.Model; +using System; +using Octopus.Data.Model; using Octopus.Server.Extensibility.Extensions.Infrastructure.Configuration; namespace Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Configuration @@ -10,6 +11,7 @@ public abstract class OpenIDConnectConfiguration : ExtensionConfigurationDocumen public const string DefaultResponseMode = "form_post"; public const string DefaultScope = "openid%20profile%20email"; public const string DefaultNameClaimType = "name"; + public const string AuthCodeGrantType = "authorization_code"; protected OpenIDConnectConfiguration(string id) : base(id) { diff --git a/source/Server.OpenIDConnect.Common/Web/AuthServerResponseHandler.cs b/source/Server.OpenIDConnect.Common/Web/AuthServerResponseHandler.cs index f20a5cc..6bb75a0 100644 --- a/source/Server.OpenIDConnect.Common/Web/AuthServerResponseHandler.cs +++ b/source/Server.OpenIDConnect.Common/Web/AuthServerResponseHandler.cs @@ -72,7 +72,7 @@ public void ValidateNonce(IOctoRequest request, ClaimsPrincipalContainer princip UserAuthenticatedValidator.ValidateNonceFromClaimsHashIsEqualToExpectedNonce(expectedNonceHash, nonceFromClaimsHash, nonceFromClaims); } - public UserResource GetUserDetails(ClaimsPrincipalContainer principalContainer) + public UserResource MapPrincipalToUserResource(ClaimsPrincipalContainer principalContainer) { var authenticationCandidate = principalToUserResourceMapper.MapToUserResource(principalContainer.Principal!); UserAuthenticatedValidator.ValidateUsername(authenticationCandidate.Username); @@ -94,8 +94,8 @@ public IOctoResponseProvider Success(IOctoRequest request, ISuccessResult UserAuthenticatedValidator.ValidateUserIsNotServiceAccount(successResult.Value.IsService, username); var octoResponse = UserAuthenticatedValidator.Redirect.Response(state.RedirectAfterLoginTo) - .WithHeader("Expires", new[] {DateTime.UtcNow.AddYears(1).ToString("R", DateTimeFormatInfo.InvariantInfo)}) - .WithCookie(new OctoCookie(UserAuthConstants.OctopusStateCookieName, Guid.NewGuid().ToString()) {HttpOnly = true, Secure = false, Expires = DateTimeOffset.MinValue}) + .WithHeader("Expires", new[] { DateTime.UtcNow.AddYears(1).ToString("R", DateTimeFormatInfo.InvariantInfo) }) + .WithCookie(new OctoCookie(UserAuthConstants.OctopusStateCookieName, Guid.NewGuid().ToString()) { HttpOnly = true, Secure = false, Expires = DateTimeOffset.MinValue }) .WithCookie(new OctoCookie(UserAuthConstants.OctopusNonceCookieName, Guid.NewGuid().ToString()) {HttpOnly = true, Secure = false, Expires = DateTimeOffset.MinValue}); var authCookies = authCookieCreator.CreateAuthCookies(successResult.Value.IdentificationToken, TimeSpan.FromDays(20), request.IsHttps, state.UsingSecureConnection); diff --git a/source/Server.OpenIDConnect.Common/Web/IUserService.cs b/source/Server.OpenIDConnect.Common/Web/IUserService.cs index 49d73ae..a92ed8a 100644 --- a/source/Server.OpenIDConnect.Common/Web/IUserService.cs +++ b/source/Server.OpenIDConnect.Common/Web/IUserService.cs @@ -8,6 +8,6 @@ namespace Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Web { public interface IUserService { - IResultFromExtension GetOrCreateUser(UserResource userResource, string[] groups, string providerName, IIdentityCreator identityCreator, bool allowAutoUserCreation, CancellationToken cancellationToken); + IResultFromExtension GetOrCreateUser(UserResource userResource, string[] groups, string providerName, IIdentityCreator identityCreator, bool allowAutoUserCreation); } } \ No newline at end of file diff --git a/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedAction.cs b/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedAction.cs index f0331de..8d01c79 100644 --- a/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedAction.cs +++ b/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedAction.cs @@ -1,5 +1,4 @@ using System; -using System.Globalization; using System.Linq; using System.Threading; using System.Threading.Tasks; @@ -73,21 +72,18 @@ public async Task ExecuteAsync(IOctoRequest request) try { var principalContainer = await authServerResponseHandler.GetDetailsFromRequestEnsuringNoErrors(request.Form.ToDictionary(pair => pair.Key, pair => (string?)pair.Value)); - var stateStringFromRequest = request.Form.ContainsKey("state") ? request.Form["state"] : string.Empty; + var stateStringFromRequest = request.Form.ContainsKey("state") ? request.Form["state"] : string.Empty; authServerResponseHandler.ValidateState(request, stateStringFromRequest); authServerResponseHandler.ValidateNonce(request, principalContainer); - var authenticationCandidate = authServerResponseHandler.GetUserDetails(principalContainer); - var username = authenticationCandidate.Username!; - - var action = authServerResponseHandler.CheckIfAuthenticationAttemptIsBanned(username, request.Host); - var stateFromRequest = JsonConvert.DeserializeObject(stateStringFromRequest)!; + var authenticationCandidate = authServerResponseHandler.MapPrincipalToUserResource(principalContainer); + var action = authServerResponseHandler.CheckIfAuthenticationAttemptIsBanned(authenticationCandidate.Username!, request.Host); - using var cts = new CancellationTokenSource(TimeSpan.FromMinutes(1)); - var userResult = userService.GetOrCreateUser(authenticationCandidate, principalContainer.ExternalGroupIds, ProviderName, identityCreator, configurationStore.GetAllowAutoUserCreation(), cts.Token); + var userResult = userService.GetOrCreateUser(authenticationCandidate, principalContainer.ExternalGroupIds, ProviderName, identityCreator, configurationStore.GetAllowAutoUserCreation()); if (userResult is ISuccessResult successResult) { + var stateFromRequest = JsonConvert.DeserializeObject(stateStringFromRequest)!; return authServerResponseHandler.Success(request, successResult, authenticationCandidate.Username!, stateFromRequest); } diff --git a/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedPkceAction.cs b/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedPkceAction.cs index 3ae5dbf..5d12d10 100644 --- a/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedPkceAction.cs +++ b/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedPkceAction.cs @@ -1,6 +1,5 @@ using System; using System.Collections.Generic; -using System.Globalization; using System.Linq; using System.Net.Http; using System.Text; @@ -81,16 +80,11 @@ public async Task ExecuteAsync(IOctoRequest request) async Task Handle(string code, string state, IOctoRequest request) { - var stateFromRequest = JsonConvert.DeserializeObject(state)!; - - var blobs = await GetAllPkceBlobsBelongingToExtension(); - var blobFromOriginalRequest = blobs.Single(b => b.RequestId == stateFromRequest.RequestId); - await RemoveBlob(blobFromOriginalRequest); - await RemoveExpiredBlobs(blobs); - var host = request.Headers.ContainsKey("Host") ? request.Headers["Host"].Single() : request.Host; var redirectUri = $"{request.Scheme}://{host}{configurationStore.RedirectUri}"; - var response = await RequestAuthToken(code, redirectUri, blobFromOriginalRequest.CodeVerifier); + var stateFromRequest = JsonConvert.DeserializeObject(state)!; + var codeVerifier = await GetCodeVerifier(stateFromRequest.RequestId); + var response = await RequestAuthToken(code, redirectUri, codeVerifier); var authServerResponseHandler = new AuthServerResponseHandler( authTokenHandler, @@ -105,12 +99,10 @@ async Task Handle(string code, string state, IOctoRequest authServerResponseHandler.ValidateState(request, state); - var authenticationCandidate = authServerResponseHandler.GetUserDetails(principalContainer); - + var authenticationCandidate = authServerResponseHandler.MapPrincipalToUserResource(principalContainer); var action = authServerResponseHandler.CheckIfAuthenticationAttemptIsBanned(authenticationCandidate.Username!, request.Host); - using var cts = new CancellationTokenSource(TimeSpan.FromMinutes(1)); - var userResult = userService.GetOrCreateUser(authenticationCandidate, principalContainer.ExternalGroupIds, ProviderName, identityCreator, configurationStore.GetAllowAutoUserCreation(), cts.Token); + var userResult = userService.GetOrCreateUser(authenticationCandidate, principalContainer.ExternalGroupIds, ProviderName, identityCreator, configurationStore.GetAllowAutoUserCreation()); if (userResult is ISuccessResult successResult) { return authServerResponseHandler.Success(request, successResult, authenticationCandidate.Username!, stateFromRequest); @@ -133,7 +125,7 @@ async Task Handle(string code, string state, IOctoRequest var formValues = new Dictionary { - ["grant_type"] = "authorization_code", + ["grant_type"] = OpenIDConnectConfiguration.AuthCodeGrantType, ["code"] = code, ["redirect_uri"] = redirectUri, ["client_id"] = configurationStore.GetClientId()!, @@ -146,6 +138,16 @@ async Task Handle(string code, string state, IOctoRequest return JsonConvert.DeserializeObject>(body)!; } + async Task GetCodeVerifier(Guid requestId) + { + var blobs = await GetAllPkceBlobsBelongingToExtension(); + var blobFromOriginalRequest = blobs.Single(b => b.RequestId == requestId); + await RemoveBlob(blobFromOriginalRequest); + await RemoveAnyExpiredBlobs(blobs); + + return blobFromOriginalRequest.CodeVerifier; + } + async Task> GetAllPkceBlobsBelongingToExtension() { var allBlobsBelongingToExtension = await mediator.Request(new GetAllBlobsRequest(configurationStore.ConfigurationSettingsName), new CancellationToken()); @@ -164,7 +166,7 @@ async Task> GetAllPkceBlobsBelongingToExtension() return pkceBlobs; } - async Task RemoveExpiredBlobs(IEnumerable blobs) + async Task RemoveAnyExpiredBlobs(IEnumerable blobs) { foreach (var blob in blobs.Where(blob => DateTimeOffset.UtcNow.Subtract(blob.TimeStamp).TotalSeconds > 30)) { diff --git a/source/Server.OpenIDConnect.Common/Web/UserService.cs b/source/Server.OpenIDConnect.Common/Web/UserService.cs index 5240a6b..ba4b1ae 100644 --- a/source/Server.OpenIDConnect.Common/Web/UserService.cs +++ b/source/Server.OpenIDConnect.Common/Web/UserService.cs @@ -5,6 +5,7 @@ using Octopus.Data.Model.User; using Octopus.Data.Storage.User; using Octopus.Server.Extensibility.Authentication.HostServices; +using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Configuration; using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Identities; using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Infrastructure; using Octopus.Server.Extensibility.Authentication.Resources.Identities; @@ -29,8 +30,7 @@ public IResultFromExtension GetOrCreateUser( string[] groups, string providerName, IIdentityCreator identityCreator, - bool allowAutoUserCreation, - CancellationToken cancellationToken) + bool allowAutoUserCreation) { var identityToMatch = NewIdentity(userResource, identityCreator); @@ -39,6 +39,7 @@ public IResultFromExtension GetOrCreateUser( throw new Exception("There are multiple users with this identity. OpenID Connect identity providers do not support users with duplicate email addresses. Please remove any duplicate users, or make the email addresses unique."); var user = matchingUsers.SingleOrDefault(); + using var cts = new CancellationTokenSource(TimeSpan.FromMinutes(1)); if (user != null) { userStore.SetSecurityGroupIds(providerName, user.Id, groups, clock.GetUtcTime()); @@ -52,10 +53,10 @@ public IResultFromExtension GetOrCreateUser( identity = user.Identities.FirstOrDefault(x => x.IdentityProviderName == providerName && x.Claims[ClaimDescriptor.EmailClaimType].Value == userResource.EmailAddress); if (identity != null) { - return ResultFromExtension.Success(userStore.UpdateIdentity(user.Id, identityToMatch, cancellationToken)); + return ResultFromExtension.Success(userStore.UpdateIdentity(user.Id, identityToMatch, cts.Token)); } - return ResultFromExtension.Success(userStore.AddIdentity(user.Id, identityToMatch, cancellationToken)); + return ResultFromExtension.Success(userStore.AddIdentity(user.Id, identityToMatch, cts.Token)); } if (!allowAutoUserCreation) @@ -65,7 +66,7 @@ public IResultFromExtension GetOrCreateUser( userResource.Username ?? string.Empty, userResource.DisplayName ?? string.Empty, userResource.EmailAddress ?? string.Empty, - cancellationToken, + cts.Token, new ProviderUserGroups { IdentityProviderName = providerName, GroupIds = groups }, new[] { identityToMatch }); if (userResult is IFailureResult failureResult) From e9548c1909494e5c9beca2020641e759901ef1fd Mon Sep 17 00:00:00 2001 From: Nelson Susanto Date: Thu, 14 Apr 2022 12:32:22 +1000 Subject: [PATCH 26/27] Make pkce stuff in authentication action a bit clearer --- .../Web/UserAuthenticationAction.cs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/source/Server.OpenIDConnect.Common/Web/UserAuthenticationAction.cs b/source/Server.OpenIDConnect.Common/Web/UserAuthenticationAction.cs index c0842b0..edf8f3c 100644 --- a/source/Server.OpenIDConnect.Common/Web/UserAuthenticationAction.cs +++ b/source/Server.OpenIDConnect.Common/Web/UserAuthenticationAction.cs @@ -100,11 +100,10 @@ public async Task ExecuteAsync(IOctoRequest request) async Task BuildAuthorizationCodePkceResponse(LoginRedirectLinkRequestModel model, LoginStateWithRequestId state, IssuerConfiguration issuerConfig) { var codeVerifier = Pkce.GenerateCodeVerifier(); - var pkceBlob = new PkceBlob(state.RequestId, codeVerifier, DateTimeOffset.UtcNow); - await mediator.Do(new PutBlobCommand(ConfigurationStore.ConfigurationSettingsName, state.RequestId.ToString(), JsonSerializer.SerializeToUtf8Bytes(pkceBlob)), new CancellationToken()); + await InsertPkceBlob(new PkceBlob(state.RequestId, codeVerifier, DateTimeOffset.UtcNow), state.RequestId.ToString()); - var stateString = JsonConvert.SerializeObject(state); var codeChallenge = Pkce.GenerateCodeChallenge(codeVerifier); + var stateString = JsonConvert.SerializeObject(state); var url = urlBuilder.Build(model.ApiAbsUrl, issuerConfig, state: stateString, codeChallenge: codeChallenge); var response = Result.Response(new LoginRedirectLinkResponseModel {ExternalAuthenticationUrl = url}) .WithCookie(new OctoCookie(UserAuthConstants.OctopusStateCookieName, State.Protect(stateString)) {HttpOnly = true, Secure = state.UsingSecureConnection, Expires = DateTimeOffset.UtcNow.AddMinutes(20)}); @@ -124,5 +123,12 @@ IOctoResponseProvider BuildHybridResponse(LoginRedirectLinkRequestModel model, L .WithCookie(new OctoCookie(UserAuthConstants.OctopusNonceCookieName, Nonce.Protect(nonce)) {HttpOnly = true, Secure = state.UsingSecureConnection, Expires = DateTimeOffset.UtcNow.AddMinutes(20)}); return response; } + + async Task InsertPkceBlob(PkceBlob blob, string requestId) + { + await mediator.Do( + new PutBlobCommand(ConfigurationStore.ConfigurationSettingsName, requestId, JsonSerializer.SerializeToUtf8Bytes(blob)), + new CancellationToken()); + } } } \ No newline at end of file From c2f643f031890778d1c340e437e62a18ec17edbf Mon Sep 17 00:00:00 2001 From: Nelson Susanto Date: Thu, 21 Apr 2022 08:50:23 +1000 Subject: [PATCH 27/27] Move cancellation token up --- .../Web/IUserService.cs | 2 +- .../Web/UserAuthenticatedAction.cs | 3 ++- .../Web/UserAuthenticatedPkceAction.cs | 26 ++++++++++--------- .../Web/UserAuthenticationAction.cs | 7 ++--- .../Web/UserService.cs | 11 ++++---- 5 files changed, 26 insertions(+), 23 deletions(-) diff --git a/source/Server.OpenIDConnect.Common/Web/IUserService.cs b/source/Server.OpenIDConnect.Common/Web/IUserService.cs index a92ed8a..49d73ae 100644 --- a/source/Server.OpenIDConnect.Common/Web/IUserService.cs +++ b/source/Server.OpenIDConnect.Common/Web/IUserService.cs @@ -8,6 +8,6 @@ namespace Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Web { public interface IUserService { - IResultFromExtension GetOrCreateUser(UserResource userResource, string[] groups, string providerName, IIdentityCreator identityCreator, bool allowAutoUserCreation); + IResultFromExtension GetOrCreateUser(UserResource userResource, string[] groups, string providerName, IIdentityCreator identityCreator, bool allowAutoUserCreation, CancellationToken cancellationToken); } } \ No newline at end of file diff --git a/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedAction.cs b/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedAction.cs index 8d01c79..66b0d4c 100644 --- a/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedAction.cs +++ b/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedAction.cs @@ -80,7 +80,8 @@ public async Task ExecuteAsync(IOctoRequest request) var authenticationCandidate = authServerResponseHandler.MapPrincipalToUserResource(principalContainer); var action = authServerResponseHandler.CheckIfAuthenticationAttemptIsBanned(authenticationCandidate.Username!, request.Host); - var userResult = userService.GetOrCreateUser(authenticationCandidate, principalContainer.ExternalGroupIds, ProviderName, identityCreator, configurationStore.GetAllowAutoUserCreation()); + using var cts = new CancellationTokenSource(TimeSpan.FromMinutes(1)); + var userResult = userService.GetOrCreateUser(authenticationCandidate, principalContainer.ExternalGroupIds, ProviderName, identityCreator, configurationStore.GetAllowAutoUserCreation(), cts.Token); if (userResult is ISuccessResult successResult) { var stateFromRequest = JsonConvert.DeserializeObject(stateStringFromRequest)!; diff --git a/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedPkceAction.cs b/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedPkceAction.cs index 5d12d10..7ddbd4a 100644 --- a/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedPkceAction.cs +++ b/source/Server.OpenIDConnect.Common/Web/UserAuthenticatedPkceAction.cs @@ -83,7 +83,9 @@ async Task Handle(string code, string state, IOctoRequest var host = request.Headers.ContainsKey("Host") ? request.Headers["Host"].Single() : request.Host; var redirectUri = $"{request.Scheme}://{host}{configurationStore.RedirectUri}"; var stateFromRequest = JsonConvert.DeserializeObject(state)!; - var codeVerifier = await GetCodeVerifier(stateFromRequest.RequestId); + + using var cts = new CancellationTokenSource(TimeSpan.FromMinutes(1)); + var codeVerifier = await GetCodeVerifier(stateFromRequest.RequestId, cts.Token); var response = await RequestAuthToken(code, redirectUri, codeVerifier); var authServerResponseHandler = new AuthServerResponseHandler( @@ -102,7 +104,7 @@ async Task Handle(string code, string state, IOctoRequest var authenticationCandidate = authServerResponseHandler.MapPrincipalToUserResource(principalContainer); var action = authServerResponseHandler.CheckIfAuthenticationAttemptIsBanned(authenticationCandidate.Username!, request.Host); - var userResult = userService.GetOrCreateUser(authenticationCandidate, principalContainer.ExternalGroupIds, ProviderName, identityCreator, configurationStore.GetAllowAutoUserCreation()); + var userResult = userService.GetOrCreateUser(authenticationCandidate, principalContainer.ExternalGroupIds, ProviderName, identityCreator, configurationStore.GetAllowAutoUserCreation(), cts.Token); if (userResult is ISuccessResult successResult) { return authServerResponseHandler.Success(request, successResult, authenticationCandidate.Username!, stateFromRequest); @@ -138,19 +140,19 @@ async Task Handle(string code, string state, IOctoRequest return JsonConvert.DeserializeObject>(body)!; } - async Task GetCodeVerifier(Guid requestId) + async Task GetCodeVerifier(Guid requestId, CancellationToken cancellationToken) { - var blobs = await GetAllPkceBlobsBelongingToExtension(); + var blobs = await GetAllPkceBlobsBelongingToExtension(cancellationToken); var blobFromOriginalRequest = blobs.Single(b => b.RequestId == requestId); - await RemoveBlob(blobFromOriginalRequest); - await RemoveAnyExpiredBlobs(blobs); + await RemoveBlob(blobFromOriginalRequest, cancellationToken); + await RemoveAnyExpiredBlobs(blobs, cancellationToken); return blobFromOriginalRequest.CodeVerifier; } - async Task> GetAllPkceBlobsBelongingToExtension() + async Task> GetAllPkceBlobsBelongingToExtension(CancellationToken cancellationToken) { - var allBlobsBelongingToExtension = await mediator.Request(new GetAllBlobsRequest(configurationStore.ConfigurationSettingsName), new CancellationToken()); + var allBlobsBelongingToExtension = await mediator.Request(new GetAllBlobsRequest(configurationStore.ConfigurationSettingsName), cancellationToken); var pkceBlobs = new List(); foreach (var blob in allBlobsBelongingToExtension.Blobs) { @@ -166,17 +168,17 @@ async Task> GetAllPkceBlobsBelongingToExtension() return pkceBlobs; } - async Task RemoveAnyExpiredBlobs(IEnumerable blobs) + async Task RemoveAnyExpiredBlobs(IEnumerable blobs, CancellationToken cancellationToken) { foreach (var blob in blobs.Where(blob => DateTimeOffset.UtcNow.Subtract(blob.TimeStamp).TotalSeconds > 30)) { - await RemoveBlob(blob); + await RemoveBlob(blob, cancellationToken); } } - async Task RemoveBlob(PkceBlob blob) + async Task RemoveBlob(PkceBlob blob, CancellationToken cancellationToken) { - await mediator.Do(new DeleteBlobCommand(configurationStore.ConfigurationSettingsName, blob.RequestId.ToString()), new CancellationToken()); + await mediator.Do(new DeleteBlobCommand(configurationStore.ConfigurationSettingsName, blob.RequestId.ToString()), cancellationToken); } } } diff --git a/source/Server.OpenIDConnect.Common/Web/UserAuthenticationAction.cs b/source/Server.OpenIDConnect.Common/Web/UserAuthenticationAction.cs index edf8f3c..a39c4dd 100644 --- a/source/Server.OpenIDConnect.Common/Web/UserAuthenticationAction.cs +++ b/source/Server.OpenIDConnect.Common/Web/UserAuthenticationAction.cs @@ -100,7 +100,8 @@ public async Task ExecuteAsync(IOctoRequest request) async Task BuildAuthorizationCodePkceResponse(LoginRedirectLinkRequestModel model, LoginStateWithRequestId state, IssuerConfiguration issuerConfig) { var codeVerifier = Pkce.GenerateCodeVerifier(); - await InsertPkceBlob(new PkceBlob(state.RequestId, codeVerifier, DateTimeOffset.UtcNow), state.RequestId.ToString()); + using var cts = new CancellationTokenSource(TimeSpan.FromMinutes(1)); + await InsertPkceBlob(new PkceBlob(state.RequestId, codeVerifier, DateTimeOffset.UtcNow), state.RequestId.ToString(), cts.Token); var codeChallenge = Pkce.GenerateCodeChallenge(codeVerifier); var stateString = JsonConvert.SerializeObject(state); @@ -124,11 +125,11 @@ IOctoResponseProvider BuildHybridResponse(LoginRedirectLinkRequestModel model, L return response; } - async Task InsertPkceBlob(PkceBlob blob, string requestId) + async Task InsertPkceBlob(PkceBlob blob, string requestId, CancellationToken cancellationToken) { await mediator.Do( new PutBlobCommand(ConfigurationStore.ConfigurationSettingsName, requestId, JsonSerializer.SerializeToUtf8Bytes(blob)), - new CancellationToken()); + cancellationToken); } } } \ No newline at end of file diff --git a/source/Server.OpenIDConnect.Common/Web/UserService.cs b/source/Server.OpenIDConnect.Common/Web/UserService.cs index ba4b1ae..5240a6b 100644 --- a/source/Server.OpenIDConnect.Common/Web/UserService.cs +++ b/source/Server.OpenIDConnect.Common/Web/UserService.cs @@ -5,7 +5,6 @@ using Octopus.Data.Model.User; using Octopus.Data.Storage.User; using Octopus.Server.Extensibility.Authentication.HostServices; -using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Configuration; using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Identities; using Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Infrastructure; using Octopus.Server.Extensibility.Authentication.Resources.Identities; @@ -30,7 +29,8 @@ public IResultFromExtension GetOrCreateUser( string[] groups, string providerName, IIdentityCreator identityCreator, - bool allowAutoUserCreation) + bool allowAutoUserCreation, + CancellationToken cancellationToken) { var identityToMatch = NewIdentity(userResource, identityCreator); @@ -39,7 +39,6 @@ public IResultFromExtension GetOrCreateUser( throw new Exception("There are multiple users with this identity. OpenID Connect identity providers do not support users with duplicate email addresses. Please remove any duplicate users, or make the email addresses unique."); var user = matchingUsers.SingleOrDefault(); - using var cts = new CancellationTokenSource(TimeSpan.FromMinutes(1)); if (user != null) { userStore.SetSecurityGroupIds(providerName, user.Id, groups, clock.GetUtcTime()); @@ -53,10 +52,10 @@ public IResultFromExtension GetOrCreateUser( identity = user.Identities.FirstOrDefault(x => x.IdentityProviderName == providerName && x.Claims[ClaimDescriptor.EmailClaimType].Value == userResource.EmailAddress); if (identity != null) { - return ResultFromExtension.Success(userStore.UpdateIdentity(user.Id, identityToMatch, cts.Token)); + return ResultFromExtension.Success(userStore.UpdateIdentity(user.Id, identityToMatch, cancellationToken)); } - return ResultFromExtension.Success(userStore.AddIdentity(user.Id, identityToMatch, cts.Token)); + return ResultFromExtension.Success(userStore.AddIdentity(user.Id, identityToMatch, cancellationToken)); } if (!allowAutoUserCreation) @@ -66,7 +65,7 @@ public IResultFromExtension GetOrCreateUser( userResource.Username ?? string.Empty, userResource.DisplayName ?? string.Empty, userResource.EmailAddress ?? string.Empty, - cts.Token, + cancellationToken, new ProviderUserGroups { IdentityProviderName = providerName, GroupIds = groups }, new[] { identityToMatch }); if (userResult is IFailureResult failureResult)