From 4a318920caa3ab7a2bdb6f634148bf2a5db461b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Chalet?= Date: Fri, 30 Oct 2015 02:26:18 +0100 Subject: [PATCH] Implement client_secret hashing/key derivation --- samples/Mvc.Server/Startup.cs | 6 ++-- src/OpenIddict.Core/IOpenIddictStore.cs | 2 +- src/OpenIddict.Core/OpenIddictHelpers.cs | 40 +++++++++++++++++++++ src/OpenIddict.Core/OpenIddictManager.cs | 43 +++++++++++++++++++---- src/OpenIddict.Core/OpenIddictProvider.cs | 10 +++--- src/OpenIddict.Core/project.json | 4 ++- src/OpenIddict.EF/OpenIddictStore.cs | 15 ++------ src/OpenIddict.Models/Application.cs | 2 +- src/OpenIddict.Models/ApplicationType.cs | 12 ------- 9 files changed, 93 insertions(+), 41 deletions(-) create mode 100644 src/OpenIddict.Core/OpenIddictHelpers.cs delete mode 100644 src/OpenIddict.Models/ApplicationType.cs diff --git a/samples/Mvc.Server/Startup.cs b/samples/Mvc.Server/Startup.cs index 8429e98e..de41f161 100644 --- a/samples/Mvc.Server/Startup.cs +++ b/samples/Mvc.Server/Startup.cs @@ -1,4 +1,5 @@ using System.Linq; +using CryptoHelper; using Microsoft.AspNet.Builder; using Microsoft.AspNet.Identity.EntityFramework; using Microsoft.Data.Entity; @@ -7,6 +8,7 @@ using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using Mvc.Server.Models; using Mvc.Server.Services; +using OpenIddict; using OpenIddict.Models; namespace Mvc.Server { @@ -75,8 +77,8 @@ namespace Mvc.Server { DisplayName = "My client application", RedirectUri = "http://localhost:53507/signin-oidc", LogoutRedirectUri = "http://localhost:53507/", - Secret = "secret_secret_secret", - Type = ApplicationType.Confidential + Secret = Crypto.HashPassword("secret_secret_secret"), + Type = OpenIddictConstants.ApplicationTypes.Confidential }); context.SaveChanges(); diff --git a/src/OpenIddict.Core/IOpenIddictStore.cs b/src/OpenIddict.Core/IOpenIddictStore.cs index 243763ac..b4cd5551 100644 --- a/src/OpenIddict.Core/IOpenIddictStore.cs +++ b/src/OpenIddict.Core/IOpenIddictStore.cs @@ -9,6 +9,6 @@ namespace OpenIddict { Task GetApplicationTypeAsync(TApplication application, CancellationToken cancellationToken); Task GetDisplayNameAsync(TApplication application, CancellationToken cancellationToken); Task GetRedirectUriAsync(TApplication application, CancellationToken cancellationToken); - Task ValidateSecretAsync(TApplication application, string secret, CancellationToken cancellationToken); + Task GetHashedSecretAsync(TApplication application, CancellationToken cancellationToken); } } \ No newline at end of file diff --git a/src/OpenIddict.Core/OpenIddictHelpers.cs b/src/OpenIddict.Core/OpenIddictHelpers.cs new file mode 100644 index 00000000..727d25a6 --- /dev/null +++ b/src/OpenIddict.Core/OpenIddictHelpers.cs @@ -0,0 +1,40 @@ +using System; +using System.Threading.Tasks; + +namespace OpenIddict { + public static class OpenIddictHelpers { + public static async Task IsConfidentialApplicationAsync( + this OpenIddictManager manager, TApplication application) + where TUser : class + where TApplication : class { + if (manager == null) { + throw new ArgumentNullException(nameof(manager)); + } + + if (application == null) { + throw new ArgumentNullException(nameof(application)); + } + + var type = await manager.GetApplicationTypeAsync(application); + + return string.Equals(type, OpenIddictConstants.ApplicationTypes.Confidential, StringComparison.OrdinalIgnoreCase); + } + + public static async Task IsPublicApplicationAsync( + this OpenIddictManager manager, TApplication application) + where TUser : class + where TApplication : class { + if (manager == null) { + throw new ArgumentNullException(nameof(manager)); + } + + if (application == null) { + throw new ArgumentNullException(nameof(application)); + } + + var type = await manager.GetApplicationTypeAsync(application); + + return string.Equals(type, OpenIddictConstants.ApplicationTypes.Public, StringComparison.OrdinalIgnoreCase); + } + } +} diff --git a/src/OpenIddict.Core/OpenIddictManager.cs b/src/OpenIddict.Core/OpenIddictManager.cs index 5b982035..b3397869 100644 --- a/src/OpenIddict.Core/OpenIddictManager.cs +++ b/src/OpenIddict.Core/OpenIddictManager.cs @@ -1,6 +1,7 @@ using System; using System.Linq; using System.Threading.Tasks; +using CryptoHelper; using Microsoft.AspNet.Http; using Microsoft.AspNet.Identity; using Microsoft.Extensions.DependencyInjection; @@ -50,12 +51,21 @@ namespace OpenIddict { select claim.Value).FirstOrDefault(); } - public virtual Task GetApplicationTypeAsync(TApplication application) { + public virtual async Task GetApplicationTypeAsync(TApplication application) { if (application == null) { throw new ArgumentNullException(nameof(application)); } - return Store.GetApplicationTypeAsync(application, Context.RequestAborted); + var type = await Store.GetApplicationTypeAsync(application, Context.RequestAborted); + + // Ensure the application type returned by the store is supported by the manager. + if (!string.Equals(type, OpenIddictConstants.ApplicationTypes.Confidential, StringComparison.OrdinalIgnoreCase) && + !string.Equals(type, OpenIddictConstants.ApplicationTypes.Public, StringComparison.OrdinalIgnoreCase)) { + throw new InvalidOperationException("Only 'confidential' or 'public' applications are " + + "supported by the default OpenIddict manager."); + } + + return type; } public virtual Task GetDisplayNameAsync(TApplication application) { @@ -66,20 +76,41 @@ namespace OpenIddict { return Store.GetDisplayNameAsync(application, Context.RequestAborted); } - public virtual Task GetRedirectUriAsync(TApplication application) { + public virtual async Task ValidateRedirectUriAsync(TApplication application, string address) { if (application == null) { throw new ArgumentNullException(nameof(application)); } - return Store.GetRedirectUriAsync(application, Context.RequestAborted); + if (!string.Equals(address, await Store.GetRedirectUriAsync(application, Context.RequestAborted), StringComparison.Ordinal)) { + Logger.LogWarning("Client validation failed because {RedirectUri} was not a valid redirect_uri " + + "for {Client}", address, await GetDisplayNameAsync(application)); + + return false; + } + + return true; } - public virtual Task ValidateSecretAsync(TApplication application, string secret) { + public virtual async Task ValidateSecretAsync(TApplication application, string secret) { if (application == null) { throw new ArgumentNullException(nameof(application)); } - return Store.ValidateSecretAsync(application, secret, Context.RequestAborted); + var type = await GetApplicationTypeAsync(application); + if (!string.Equals(type, OpenIddictConstants.ApplicationTypes.Confidential, StringComparison.OrdinalIgnoreCase)) { + Logger.LogWarning("Client authentication cannot be enforced for non-confidential applications."); + + return false; + } + + var hash = await Store.GetHashedSecretAsync(application, Context.RequestAborted); + if (!Crypto.VerifyHashedPassword(hash, secret)) { + Logger.LogWarning("Client authentication failed for {Client}.", await GetDisplayNameAsync(application)); + + return false; + } + + return true; } } } \ No newline at end of file diff --git a/src/OpenIddict.Core/OpenIddictProvider.cs b/src/OpenIddict.Core/OpenIddictProvider.cs index 4bfcde1f..0544fdf3 100644 --- a/src/OpenIddict.Core/OpenIddictProvider.cs +++ b/src/OpenIddict.Core/OpenIddictProvider.cs @@ -51,7 +51,7 @@ namespace OpenIddict { return; } - if (!string.Equals(context.RedirectUri, await manager.GetRedirectUriAsync(application), StringComparison.Ordinal)) { + if (!await manager.ValidateRedirectUriAsync(application, context.RedirectUri)) { context.Rejected( error: OpenIdConnectConstants.Errors.InvalidClient, description: "Invalid redirect_uri"); @@ -114,8 +114,7 @@ namespace OpenIddict { // Reject tokens requests containing a client_secret // if the client application is not confidential. - var type = await manager.GetApplicationTypeAsync(application); - if (type == OpenIddictConstants.ApplicationTypes.Public && !string.IsNullOrEmpty(context.ClientSecret)) { + if (await manager.IsPublicApplicationAsync(application) && !string.IsNullOrEmpty(context.ClientSecret)) { context.Rejected( error: OpenIdConnectConstants.Errors.InvalidRequest, description: "Public clients are not allowed to send a client_secret"); @@ -124,7 +123,7 @@ namespace OpenIddict { } // Confidential applications MUST authenticate. - else if (type == OpenIddictConstants.ApplicationTypes.Confidential && + else if (await manager.IsConfidentialApplicationAsync(application) && !await manager.ValidateSecretAsync(application, context.ClientSecret)) { context.Rejected( error: OpenIdConnectConstants.Errors.InvalidClient, @@ -145,8 +144,7 @@ namespace OpenIddict { // To prevent downgrade attacks, ensure that authorization requests using the hybrid/implicit // flow are rejected if the client identifier corresponds to a confidential application. - var type = await manager.GetApplicationTypeAsync(application); - if (type == OpenIddictConstants.ApplicationTypes.Confidential && !context.Request.IsAuthorizationCodeFlow()) { + if (await manager.IsConfidentialApplicationAsync(application) && !context.Request.IsAuthorizationCodeFlow()) { context.Rejected( error: OpenIdConnectConstants.Errors.InvalidRequest, description: "Confidential clients can only use response_type=code."); diff --git a/src/OpenIddict.Core/project.json b/src/OpenIddict.Core/project.json index 35d781d8..e6311078 100644 --- a/src/OpenIddict.Core/project.json +++ b/src/OpenIddict.Core/project.json @@ -16,7 +16,9 @@ "version": "1.0.0-*" }, - "AspNet.Security.OpenIdConnect.Server": "1.0.0-*" + "AspNet.Security.OpenIdConnect.Server": "1.0.0-*", + + "CryptoHelper": "1.0.0-rc1-build03" }, "frameworks": { diff --git a/src/OpenIddict.EF/OpenIddictStore.cs b/src/OpenIddict.EF/OpenIddictStore.cs index 140607a0..70c8461c 100644 --- a/src/OpenIddict.EF/OpenIddictStore.cs +++ b/src/OpenIddict.EF/OpenIddictStore.cs @@ -33,16 +33,7 @@ namespace OpenIddict { throw new ArgumentNullException(nameof(application)); } - switch (application.Type) { - case ApplicationType.Confidential: - return Task.FromResult(OpenIddictConstants.ApplicationTypes.Confidential); - - case ApplicationType.Public: - return Task.FromResult(OpenIddictConstants.ApplicationTypes.Public); - - default: - throw new InvalidOperationException($"Unsupported application type ('{application.Type.ToString()}')."); - } + return Task.FromResult(application.Type); } public virtual Task GetDisplayNameAsync(TApplication application, CancellationToken cancellationToken) { @@ -61,12 +52,12 @@ namespace OpenIddict { return Task.FromResult(application.RedirectUri); } - public virtual Task ValidateSecretAsync(TApplication application, string secret, CancellationToken cancellationToken) { + public virtual Task GetHashedSecretAsync(TApplication application, CancellationToken cancellationToken) { if (application == null) { throw new ArgumentNullException(nameof(application)); } - return Task.FromResult(string.Equals(application.Secret, secret, StringComparison.Ordinal)); + return Task.FromResult(application.Secret); } } } \ No newline at end of file diff --git a/src/OpenIddict.Models/Application.cs b/src/OpenIddict.Models/Application.cs index 8fa63be0..13b25624 100644 --- a/src/OpenIddict.Models/Application.cs +++ b/src/OpenIddict.Models/Application.cs @@ -11,6 +11,6 @@ namespace OpenIddict.Models { public string RedirectUri { get; set; } public string LogoutRedirectUri { get; set; } public string Secret { get; set; } - public ApplicationType Type { get; set; } + public string Type { get; set; } } } \ No newline at end of file diff --git a/src/OpenIddict.Models/ApplicationType.cs b/src/OpenIddict.Models/ApplicationType.cs deleted file mode 100644 index 8f961062..00000000 --- a/src/OpenIddict.Models/ApplicationType.cs +++ /dev/null @@ -1,12 +0,0 @@ -/* - * Licensed under the Apache License, Version 2.0 (http://www.apache.org/licenses/LICENSE-2.0) - * See https://github.com/openiddict/core for more information concerning - * the license and the contributors participating to this project. - */ - -namespace OpenIddict.Models { - public enum ApplicationType { - Public = 0, - Confidential = 1 - } -} \ No newline at end of file