From 89be2528381b5ddb1a2c935e5c6424bf52369788 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Chalet?= Date: Tue, 12 Sep 2017 02:51:49 +0200 Subject: [PATCH] Rename OpenIddictApplicationStore.GetHashedSecretAsync()/SetHashedSecretAsync() --- .../Managers/OpenIddictApplicationManager.cs | 234 ++++++++++-------- .../Stores/IOpenIddictApplicationStore.cs | 34 +-- .../Stores/OpenIddictApplicationStore.cs | 56 +++-- .../OpenIddictApplication.cs | 5 +- 4 files changed, 186 insertions(+), 143 deletions(-) diff --git a/src/OpenIddict.Core/Managers/OpenIddictApplicationManager.cs b/src/OpenIddict.Core/Managers/OpenIddictApplicationManager.cs index c03f9505..41eba574 100644 --- a/src/OpenIddict.Core/Managers/OpenIddictApplicationManager.cs +++ b/src/OpenIddict.Core/Managers/OpenIddictApplicationManager.cs @@ -61,6 +61,8 @@ namespace OpenIddict.Core /// /// Creates a new application. + /// Note: the default implementation automatically hashes the client + /// secret before storing it in the database, for security reasons. /// /// The application to create. /// The client secret associated with the application, if applicable. @@ -78,7 +80,7 @@ namespace OpenIddict.Core throw new ArgumentNullException(nameof(application)); } - if (!string.IsNullOrEmpty(await Store.GetHashedSecretAsync(application, cancellationToken))) + if (!string.IsNullOrEmpty(await Store.GetClientSecretAsync(application, cancellationToken))) { throw new ArgumentException("The client secret hash cannot be directly set on the application entity."); } @@ -101,12 +103,13 @@ namespace OpenIddict.Core throw new InvalidOperationException("A client secret must be provided when creating a confidential application."); } - await Store.SetHashedSecretAsync(application, null, cancellationToken); + await Store.SetClientSecretAsync(application, null, cancellationToken); } else { - await Store.SetHashedSecretAsync(application, Crypto.HashPassword(secret), cancellationToken); + secret = await ObfuscateClientSecretAsync(secret, cancellationToken); + await Store.SetClientSecretAsync(application, secret, cancellationToken); } await ValidateAsync(application, cancellationToken); @@ -309,7 +312,7 @@ namespace OpenIddict.Core throw new ArgumentNullException(nameof(application)); } - return !string.IsNullOrEmpty(await Store.GetHashedSecretAsync(application, cancellationToken)); + return !string.IsNullOrEmpty(await Store.GetClientSecretAsync(application, cancellationToken)); } /// @@ -358,15 +361,37 @@ namespace OpenIddict.Core } /// - /// Updates the client secret associated with an application. + /// Updates an existing application. /// - /// The application. + /// The application to update. + /// The that can be used to abort the operation. + /// + /// A that can be used to monitor the asynchronous operation. + /// + public virtual async Task UpdateAsync([NotNull] TApplication application, CancellationToken cancellationToken) + { + if (application == null) + { + throw new ArgumentNullException(nameof(application)); + } + + await ValidateAsync(application, cancellationToken); + await Store.UpdateAsync(application, cancellationToken); + } + + /// + /// Updates an existing application and replaces the existing secret. + /// Note: the default implementation automatically hashes the client + /// secret before storing it in the database, for security reasons. + /// + /// The application to update. /// The client secret associated with the application. /// The that can be used to abort the operation. /// /// A that can be used to monitor the asynchronous operation. /// - public virtual async Task SetClientSecretAsync([NotNull] TApplication application, [CanBeNull] string secret, CancellationToken cancellationToken) + public virtual async Task UpdateAsync([NotNull] TApplication application, + [CanBeNull] string secret, CancellationToken cancellationToken) { if (application == null) { @@ -375,64 +400,119 @@ namespace OpenIddict.Core if (string.IsNullOrEmpty(secret)) { - await Store.SetHashedSecretAsync(application, null, cancellationToken); + await Store.SetClientSecretAsync(application, null, cancellationToken); } else { - await Store.SetHashedSecretAsync(application, Crypto.HashPassword(secret), cancellationToken); + secret = await ObfuscateClientSecretAsync(secret, cancellationToken); + await Store.SetClientSecretAsync(application, secret, cancellationToken); } await UpdateAsync(application, cancellationToken); } /// - /// Updates an existing application. + /// Validates the specified post_logout_redirect_uri. /// - /// The application to update. + /// The address that should be compared to the post_logout_redirect_uri stored in the database. /// The that can be used to abort the operation. /// - /// A that can be used to monitor the asynchronous operation. + /// A that can be used to monitor the asynchronous operation, whose result + /// returns a boolean indicating whether the post_logout_redirect_uri was valid. /// - public virtual async Task UpdateAsync([NotNull] TApplication application, CancellationToken cancellationToken) + public virtual async Task ValidateLogoutRedirectUriAsync(string address, CancellationToken cancellationToken) + { + // Warning: SQL engines like Microsoft SQL Server are known to use case-insensitive lookups by default. + // To ensure a case-sensitive comparison is used, string.Equals(Ordinal) is manually called here. + foreach (var application in await Store.FindByLogoutRedirectUriAsync(address, cancellationToken)) + { + // Note: the post_logout_redirect_uri must be compared using case-sensitive "Simple String Comparison". + if (string.Equals(address, await Store.GetLogoutRedirectUriAsync(application, cancellationToken), StringComparison.Ordinal)) + { + return true; + } + } + + Logger.LogWarning("Client validation failed because '{PostLogoutRedirectUri}' " + + "was not a valid post_logout_redirect_uri.", address); + + return false; + } + + /// + /// Validates the redirect_uri associated with an application. + /// + /// The application. + /// The address that should be compared to the redirect_uri stored in the database. + /// The that can be used to abort the operation. + /// + /// A that can be used to monitor the asynchronous operation, + /// whose result returns a boolean indicating whether the redirect_uri was valid. + /// + public virtual async Task ValidateRedirectUriAsync([NotNull] TApplication application, string address, CancellationToken cancellationToken) { if (application == null) { throw new ArgumentNullException(nameof(application)); } - await ValidateAsync(application, cancellationToken); - await Store.UpdateAsync(application, cancellationToken); + // Note: the redirect_uri must be compared using case-sensitive "Simple String Comparison". + // See http://openid.net/specs/openid-connect-core-1_0.html#AuthRequest for more information. + if (string.Equals(address, await Store.GetRedirectUriAsync(application, cancellationToken), StringComparison.Ordinal)) + { + return true; + } + + Logger.LogWarning("Client validation failed because '{RedirectUri}' was not a valid redirect_uri " + + "for '{Client}'.", address, await GetDisplayNameAsync(application, cancellationToken)); + + return false; } /// - /// Updates an existing application. + /// Validates the client_secret associated with an application. /// - /// The application to update. - /// The client secret associated with the application. + /// The application. + /// The secret that should be compared to the client_secret stored in the database. /// The that can be used to abort the operation. + /// A that can be used to monitor the asynchronous operation. /// - /// A that can be used to monitor the asynchronous operation. + /// A that can be used to monitor the asynchronous operation, + /// whose result returns a boolean indicating whether the client secret was valid. /// - public virtual async Task UpdateAsync([NotNull] TApplication application, - [CanBeNull] string secret, CancellationToken cancellationToken) + public virtual async Task ValidateClientSecretAsync([NotNull] TApplication application, string secret, CancellationToken cancellationToken) { if (application == null) { throw new ArgumentNullException(nameof(application)); } - if (string.IsNullOrEmpty(secret)) + if (!await IsConfidentialAsync(application, cancellationToken)) { - await Store.SetHashedSecretAsync(application, null, cancellationToken); + Logger.LogWarning("Client authentication cannot be enforced for non-confidential applications."); + + return false; } - else + var value = await Store.GetClientSecretAsync(application, cancellationToken); + if (string.IsNullOrEmpty(value)) { - await Store.SetHashedSecretAsync(application, Crypto.HashPassword(secret), cancellationToken); + Logger.LogError("Client authentication failed for {Client} because " + + "no client secret was associated with the application."); + + return false; } - await UpdateAsync(application, cancellationToken); + if (!await ValidateClientSecretAsync(secret, value, cancellationToken)) + { + Logger.LogWarning("Client authentication failed for {Client}.", + await GetDisplayNameAsync(application, cancellationToken)); + + return false; + } + + return true; } /// @@ -469,17 +549,17 @@ namespace OpenIddict.Core "supported by the default application manager.", nameof(application)); } - var hash = await Store.GetHashedSecretAsync(application, cancellationToken); + var secret = await Store.GetClientSecretAsync(application, cancellationToken); // Ensure a client secret was specified if the client is a confidential application. - if (string.IsNullOrEmpty(hash) && + if (string.IsNullOrEmpty(secret) && string.Equals(type, OpenIddictConstants.ClientTypes.Confidential, StringComparison.OrdinalIgnoreCase)) { throw new ArgumentException("The client secret cannot be null or empty for a confidential application.", nameof(application)); } // Ensure no client secret was specified if the client is a public application. - else if (!string.IsNullOrEmpty(hash) && + else if (!string.IsNullOrEmpty(secret) && string.Equals(type, OpenIddictConstants.ClientTypes.Public, StringComparison.OrdinalIgnoreCase)) { throw new ArgumentException("A client secret cannot be associated with a public application.", nameof(application)); @@ -522,106 +602,60 @@ namespace OpenIddict.Core } /// - /// Validates the specified post_logout_redirect_uri. - /// - /// The address that should be compared to the post_logout_redirect_uri stored in the database. - /// The that can be used to abort the operation. - /// - /// A that can be used to monitor the asynchronous operation, whose result - /// returns a boolean indicating whether the post_logout_redirect_uri was valid. - /// - public virtual async Task ValidateLogoutRedirectUriAsync(string address, CancellationToken cancellationToken) - { - // Warning: SQL engines like Microsoft SQL Server are known to use case-insensitive lookups by default. - // To ensure a case-sensitive comparison is used, string.Equals(Ordinal) is manually called here. - foreach (var application in await Store.FindByLogoutRedirectUriAsync(address, cancellationToken)) - { - // Note: the post_logout_redirect_uri must be compared using case-sensitive "Simple String Comparison". - if (string.Equals(address, await Store.GetLogoutRedirectUriAsync(application, cancellationToken), StringComparison.Ordinal)) - { - return true; - } - } - - Logger.LogWarning("Client validation failed because '{PostLogoutRedirectUri}' " + - "was not a valid post_logout_redirect_uri.", address); - - return false; - } - - /// - /// Validates the redirect_uri associated with an application. + /// Obfuscates the specified client secret so it can be safely stored in a database. + /// By default, this method returns a complex hashed representation computed using PBKDF2. /// - /// The application. - /// The address that should be compared to the redirect_uri stored in the database. + /// The client secret. /// The that can be used to abort the operation. /// - /// A that can be used to monitor the asynchronous operation, - /// whose result returns a boolean indicating whether the redirect_uri was valid. + /// A that can be used to monitor the asynchronous operation. /// - public virtual async Task ValidateRedirectUriAsync([NotNull] TApplication application, string address, CancellationToken cancellationToken) + protected virtual Task ObfuscateClientSecretAsync([NotNull] string secret, CancellationToken cancellationToken) { - if (application == null) - { - throw new ArgumentNullException(nameof(application)); - } - - // Note: the redirect_uri must be compared using case-sensitive "Simple String Comparison". - // See http://openid.net/specs/openid-connect-core-1_0.html#AuthRequest for more information. - if (string.Equals(address, await Store.GetRedirectUriAsync(application, cancellationToken), StringComparison.Ordinal)) + if (string.IsNullOrEmpty(secret)) { - return true; + throw new ArgumentException("The secret cannot be null or empty.", nameof(secret)); } - Logger.LogWarning("Client validation failed because '{RedirectUri}' was not a valid redirect_uri " + - "for '{Client}'.", address, await GetDisplayNameAsync(application, cancellationToken)); - - return false; + return Task.FromResult(Crypto.HashPassword(secret)); } /// - /// Validates the client_secret associated with an application. + /// Validates the specified value to ensure it corresponds to the client secret. + /// Note: when overriding this method, using a time-constant comparer is strongly recommended. /// - /// The application. - /// The secret that should be compared to the client_secret stored in the database. + /// The client secret to compare to the value stored in the database. + /// The value stored in the database, which is usually a hashed representation of the secret. /// The that can be used to abort the operation. - /// A that can be used to monitor the asynchronous operation. /// /// A that can be used to monitor the asynchronous operation, - /// whose result returns a boolean indicating whether the client secret was valid. + /// whose result returns a boolean indicating whether the specified value was valid. /// - public virtual async Task ValidateClientSecretAsync([NotNull] TApplication application, string secret, CancellationToken cancellationToken) + protected virtual Task ValidateClientSecretAsync( + [NotNull] string secret, [NotNull] string comparand, CancellationToken cancellationToken) { - if (application == null) + if (string.IsNullOrEmpty(secret)) { - throw new ArgumentNullException(nameof(application)); + throw new ArgumentException("The secret cannot be null or empty.", nameof(secret)); } - if (!await IsConfidentialAsync(application, cancellationToken)) + if (string.IsNullOrEmpty(comparand)) { - Logger.LogWarning("Client authentication cannot be enforced for non-confidential applications."); - - return false; + throw new ArgumentException("The comparand cannot be null or empty.", nameof(comparand)); } - var hash = await Store.GetHashedSecretAsync(application, cancellationToken); - if (string.IsNullOrEmpty(hash)) + try { - Logger.LogError("Client authentication failed for {Client} because " + - "no client secret was associated with the application."); - - return false; + return Task.FromResult(Crypto.VerifyHashedPassword(comparand, secret)); } - if (!Crypto.VerifyHashedPassword(hash, secret)) + catch (Exception exception) { - Logger.LogWarning("Client authentication failed for {Client}.", - await GetDisplayNameAsync(application, cancellationToken)); + Logger.LogWarning(exception, "An error occurred while trying to verify a client secret. " + + "This may indicate that the hashed entry is corrupted or malformed."); - return false; + return Task.FromResult(false); } - - return true; } } } \ No newline at end of file diff --git a/src/OpenIddict.Core/Stores/IOpenIddictApplicationStore.cs b/src/OpenIddict.Core/Stores/IOpenIddictApplicationStore.cs index 1b6338bd..88281ae9 100644 --- a/src/OpenIddict.Core/Stores/IOpenIddictApplicationStore.cs +++ b/src/OpenIddict.Core/Stores/IOpenIddictApplicationStore.cs @@ -99,37 +99,39 @@ namespace OpenIddict.Core Task GetClientIdAsync([NotNull] TApplication application, CancellationToken cancellationToken); /// - /// Retrieves the client type associated with an application. + /// Retrieves the client secret associated with an application. + /// Note: depending on the manager used to create the application, + /// the client secret may be hashed for security reasons. /// /// The application. /// The that can be used to abort the operation. /// /// A that can be used to monitor the asynchronous operation, - /// whose result returns the client type of the application (by default, "public"). + /// whose result returns the client secret associated with the application. /// - Task GetClientTypeAsync([NotNull] TApplication application, CancellationToken cancellationToken); + Task GetClientSecretAsync([NotNull] TApplication application, CancellationToken cancellationToken); /// - /// Retrieves the display name associated with an application. + /// Retrieves the client type associated with an application. /// /// The application. /// The that can be used to abort the operation. /// /// A that can be used to monitor the asynchronous operation, - /// whose result returns the display name associated with the application. + /// whose result returns the client type of the application (by default, "public"). /// - Task GetDisplayNameAsync([NotNull] TApplication application, CancellationToken cancellationToken); + Task GetClientTypeAsync([NotNull] TApplication application, CancellationToken cancellationToken); /// - /// Retrieves the hashed secret associated with an application. + /// Retrieves the display name associated with an application. /// /// The application. /// The that can be used to abort the operation. /// /// A that can be used to monitor the asynchronous operation, - /// whose result returns the hashed secret associated with the application. + /// whose result returns the display name associated with the application. /// - Task GetHashedSecretAsync([NotNull] TApplication application, CancellationToken cancellationToken); + Task GetDisplayNameAsync([NotNull] TApplication application, CancellationToken cancellationToken); /// /// Retrieves the unique identifier associated with an application. @@ -176,26 +178,28 @@ namespace OpenIddict.Core Task> GetTokensAsync([NotNull] TApplication application, CancellationToken cancellationToken); /// - /// Sets the client type associated with an application. + /// Sets the client secret associated with an application. + /// Note: depending on the manager used to create the application, + /// the client secret may be hashed for security reasons. /// /// The application. - /// The client type associated with the application. + /// The client secret associated with the application. /// The that can be used to abort the operation. /// /// A that can be used to monitor the asynchronous operation. /// - Task SetClientTypeAsync([NotNull] TApplication application, [NotNull] string type, CancellationToken cancellationToken); + Task SetClientSecretAsync([NotNull] TApplication application, [CanBeNull] string secret, CancellationToken cancellationToken); /// - /// Sets the hashed secret associated with an application. + /// Sets the client type associated with an application. /// /// The application. - /// The hashed client secret associated with the application. + /// The client type associated with the application. /// The that can be used to abort the operation. /// /// A that can be used to monitor the asynchronous operation. /// - Task SetHashedSecretAsync([NotNull] TApplication application, [CanBeNull] string hash, CancellationToken cancellationToken); + Task SetClientTypeAsync([NotNull] TApplication application, [NotNull] string type, CancellationToken cancellationToken); /// /// Updates an existing application. diff --git a/src/OpenIddict.EntityFrameworkCore/Stores/OpenIddictApplicationStore.cs b/src/OpenIddict.EntityFrameworkCore/Stores/OpenIddictApplicationStore.cs index fb8ffe3f..90c4fb05 100644 --- a/src/OpenIddict.EntityFrameworkCore/Stores/OpenIddictApplicationStore.cs +++ b/src/OpenIddict.EntityFrameworkCore/Stores/OpenIddictApplicationStore.cs @@ -208,60 +208,62 @@ namespace OpenIddict.EntityFrameworkCore } /// - /// Retrieves the client type associated with an application. + /// Retrieves the client secret associated with an application. + /// Note: depending on the manager used to create the application, + /// the client secret may be hashed for security reasons. /// /// The application. /// The that can be used to abort the operation. /// /// A that can be used to monitor the asynchronous operation, - /// whose result returns the client type of the application (by default, "public"). + /// whose result returns the client secret associated with the application. /// - public virtual Task GetClientTypeAsync([NotNull] TApplication application, CancellationToken cancellationToken) + public virtual Task GetClientSecretAsync([NotNull] TApplication application, CancellationToken cancellationToken) { if (application == null) { throw new ArgumentNullException(nameof(application)); } - return Task.FromResult(application.Type); + return Task.FromResult(application.ClientSecret); } /// - /// Retrieves the display name associated with an application. + /// Retrieves the client type associated with an application. /// /// The application. /// The that can be used to abort the operation. /// /// A that can be used to monitor the asynchronous operation, - /// whose result returns the display name associated with the application. + /// whose result returns the client type of the application (by default, "public"). /// - public virtual Task GetDisplayNameAsync([NotNull] TApplication application, CancellationToken cancellationToken) + public virtual Task GetClientTypeAsync([NotNull] TApplication application, CancellationToken cancellationToken) { if (application == null) { throw new ArgumentNullException(nameof(application)); } - return Task.FromResult(application.DisplayName); + return Task.FromResult(application.Type); } /// - /// Retrieves the hashed secret associated with an application. + /// Retrieves the display name associated with an application. /// /// The application. /// The that can be used to abort the operation. /// /// A that can be used to monitor the asynchronous operation, - /// whose result returns the hashed secret associated with the application. + /// whose result returns the display name associated with the application. /// - public virtual Task GetHashedSecretAsync([NotNull] TApplication application, CancellationToken cancellationToken) + public virtual Task GetDisplayNameAsync([NotNull] TApplication application, CancellationToken cancellationToken) { if (application == null) { throw new ArgumentNullException(nameof(application)); } - return Task.FromResult(application.ClientSecret); + return Task.FromResult(application.DisplayName); } /// @@ -353,49 +355,51 @@ namespace OpenIddict.EntityFrameworkCore } /// - /// Sets the client type associated with an application. + /// Sets the client secret associated with an application. + /// Note: depending on the manager used to create the application, + /// the client secret may be hashed for security reasons. /// /// The application. - /// The client type associated with the application. + /// The client secret associated with the application. /// The that can be used to abort the operation. /// /// A that can be used to monitor the asynchronous operation. /// - public virtual Task SetClientTypeAsync([NotNull] TApplication application, [NotNull] string type, CancellationToken cancellationToken) + public virtual Task SetClientSecretAsync([NotNull] TApplication application, + [CanBeNull] string secret, CancellationToken cancellationToken) { if (application == null) { throw new ArgumentNullException(nameof(application)); } - if (string.IsNullOrEmpty(type)) - { - throw new ArgumentException("The client type cannot be null or empty.", nameof(type)); - } - - application.Type = type; + application.ClientSecret = secret; return Task.FromResult(0); } /// - /// Sets the hashed secret associated with an application. + /// Sets the client type associated with an application. /// /// The application. - /// The hashed client secret associated with the application. + /// The client type associated with the application. /// The that can be used to abort the operation. /// /// A that can be used to monitor the asynchronous operation. /// - public virtual Task SetHashedSecretAsync([NotNull] TApplication application, - [CanBeNull] string hash, CancellationToken cancellationToken) + public virtual Task SetClientTypeAsync([NotNull] TApplication application, [NotNull] string type, CancellationToken cancellationToken) { if (application == null) { throw new ArgumentNullException(nameof(application)); } - application.ClientSecret = hash; + if (string.IsNullOrEmpty(type)) + { + throw new ArgumentException("The client type cannot be null or empty.", nameof(type)); + } + + application.Type = type; return Task.FromResult(0); } diff --git a/src/OpenIddict.Models/OpenIddictApplication.cs b/src/OpenIddict.Models/OpenIddictApplication.cs index 140421d4..120ac6ef 100644 --- a/src/OpenIddict.Models/OpenIddictApplication.cs +++ b/src/OpenIddict.Models/OpenIddictApplication.cs @@ -45,8 +45,9 @@ namespace OpenIddict.Models public virtual string ClientId { get; set; } /// - /// Gets or sets the hashed client secret - /// associated with the current application. + /// Gets or sets the client secret associated with the current application. + /// Note: depending on the application manager used to create this instance, + /// this property may be hashed or encrypted for security reasons. /// public virtual string ClientSecret { get; set; }