(feature): Add ActiveTokenEndpoint to WsFederationConfiguration #2100
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,205 @@ | ||
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. | ||
|
|
||
| using System; | ||
| using System.Security.Cryptography; | ||
| using System.Security.Cryptography.X509Certificates; | ||
| using Microsoft.IdentityModel.Tokens; | ||
| using Microsoft.IdentityModel.Xml; | ||
| using static Microsoft.IdentityModel.Logging.LogHelper; | ||
TimHannMSFT marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| namespace Microsoft.IdentityModel.Protocols.WsFederation | ||
| { | ||
| /// <summary> | ||
| /// Defines a class for validating the WsFederationConfiguration. | ||
| /// </summary> | ||
| public class WsFederationConfigurationValidator : IConfigurationValidator<WsFederationConfiguration> | ||
| { | ||
| /// <summary> | ||
| /// Validates a WsFederationConfiguration. | ||
| /// </summary> | ||
| /// <param name="configuration">WsFederationConfiguration to validate</param> | ||
| /// <returns>A <see cref="ConfigurationValidationResult"/> containing the validation result.</returns> | ||
| /// <exception cref="ArgumentNullException">If the provided configuration is null</exception> | ||
| public ConfigurationValidationResult Validate(WsFederationConfiguration configuration) | ||
| { | ||
| if (configuration == null) | ||
| throw LogArgumentNullException(nameof(configuration)); | ||
|
|
||
| if (string.IsNullOrWhiteSpace(configuration.Issuer)) | ||
| { | ||
| return new ConfigurationValidationResult | ||
| { | ||
| ErrorMessage = LogMessages.IDX22700, | ||
| Succeeded = false | ||
| }; | ||
| } | ||
|
There was a problem hiding this comment. Consider doing all these checks up front and use StringBuilder to build out an error message. In the current model a developer may face one issue, update, face another issue, update...etc. Getting all the errors up front allows for more actionable errors/fewer iterations of debugging. Also less code maintenance costs IMO since if we decide to add another check it doesn't require another log message. #WontFix There was a problem hiding this comment. The current code assume some of the checks before it. While is not a bad idea it requires a refactor of the whole method. If you dont mind I would like to postpone this improvement. |
||
|
|
||
| if (configuration.Signature == null) | ||
| { | ||
| return new ConfigurationValidationResult | ||
| { | ||
| ErrorMessage = LogMessages.IDX22701, | ||
| Succeeded = false | ||
| }; | ||
| } | ||
|
|
||
| if (configuration.Signature.KeyInfo == null) | ||
| { | ||
| return new ConfigurationValidationResult | ||
| { | ||
| ErrorMessage = LogMessages.IDX22702, | ||
| Succeeded = false | ||
| }; | ||
| } | ||
|
|
||
| if (string.IsNullOrWhiteSpace(configuration.Signature.SignatureValue)) | ||
| { | ||
| return new ConfigurationValidationResult | ||
| { | ||
| ErrorMessage = LogMessages.IDX22703, | ||
| Succeeded = false | ||
| }; | ||
| } | ||
|
|
||
| if (configuration.Signature.SignedInfo == null) | ||
| { | ||
| return new ConfigurationValidationResult | ||
| { | ||
| ErrorMessage = LogMessages.IDX22704, | ||
| Succeeded = false | ||
| }; | ||
| } | ||
|
|
||
| if (string.IsNullOrWhiteSpace(configuration.Signature.SignedInfo.SignatureMethod)) | ||
| { | ||
| return new ConfigurationValidationResult | ||
| { | ||
| ErrorMessage = LogMessages.IDX22705, | ||
| Succeeded = false | ||
| }; | ||
| } | ||
|
|
||
| if (configuration.Signature.SignedInfo.References == null || configuration.Signature.SignedInfo.References.Count == 0) | ||
| { | ||
| return new ConfigurationValidationResult | ||
| { | ||
| ErrorMessage = LogMessages.IDX22706, | ||
| Succeeded = false | ||
| }; | ||
| } | ||
|
|
||
| if (string.IsNullOrWhiteSpace(configuration.ActiveTokenEndpoint)) | ||
| { | ||
| return new ConfigurationValidationResult | ||
| { | ||
| ErrorMessage = LogMessages.IDX22707, | ||
| Succeeded = false | ||
| }; | ||
| } | ||
|
|
||
| if (!Uri.IsWellFormedUriString(configuration.ActiveTokenEndpoint, UriKind.Absolute)) | ||
| { | ||
| return new ConfigurationValidationResult | ||
| { | ||
| ErrorMessage = LogMessages.IDX22708, | ||
| Succeeded = false | ||
| }; | ||
| } | ||
|
|
||
| if (string.IsNullOrWhiteSpace(configuration.TokenEndpoint)) | ||
| { | ||
| return new ConfigurationValidationResult | ||
| { | ||
| ErrorMessage = LogMessages.IDX22709, | ||
| Succeeded = false | ||
| }; | ||
| } | ||
|
|
||
| if (!Uri.IsWellFormedUriString(configuration.TokenEndpoint, UriKind.Absolute)) | ||
| { | ||
| return new ConfigurationValidationResult | ||
| { | ||
| ErrorMessage = LogMessages.IDX22710, | ||
| Succeeded = false | ||
| }; | ||
| } | ||
|
|
||
| if (configuration.SigningKeys == null || configuration.SigningKeys.Count == 0) | ||
| { | ||
| return new ConfigurationValidationResult | ||
| { | ||
| ErrorMessage = LogMessages.IDX22711, | ||
| Succeeded = false | ||
| }; | ||
| } | ||
|
|
||
| // Get the thumbprint of the cert used to sign the metadata | ||
| string signingKeyId = string.Empty; | ||
| var signatureX509Data = configuration.Signature.KeyInfo.X509Data.GetEnumerator(); | ||
|
|
||
| if (signatureX509Data.MoveNext()) | ||
| { | ||
| var signatureCertData = signatureX509Data.Current.Certificates.GetEnumerator(); | ||
| if (signatureCertData.MoveNext() && !string.IsNullOrWhiteSpace(signatureCertData.Current)) | ||
| { | ||
| X509Certificate2 cert = null; | ||
|
|
||
| try | ||
| { | ||
| cert = new X509Certificate2(Convert.FromBase64String(signatureCertData.Current)); | ||
| signingKeyId = cert.Thumbprint; | ||
| } | ||
| catch (CryptographicException) | ||
| { | ||
| return new ConfigurationValidationResult | ||
| { | ||
| ErrorMessage = LogMessages.IDX22712, | ||
| Succeeded = false | ||
| }; | ||
| } | ||
| finally | ||
| { | ||
| if (cert != null) | ||
| { | ||
| ((IDisposable)cert).Dispose(); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // We know the key used to sign the doc is part of the token signing keys as per the spec. | ||
| // http://docs.oasis-open.org/wsfed/federation/v1.2/os/ws-federation-1.2-spec-os.html#_Toc223174958:~:text=%3C/fed%3ATargetScopes%20%3E-,3.1.15%20%5BSignature%5D%20Property,-The%20OPTIONAL%20%5BSignature | ||
| // If the metadata is for a token issuer then the key used to sign issued tokens SHOULD | ||
| // be used to sign this document. This means that if a <fed:TokenSigningKey> is specified, | ||
| // it SHOULD be used to sign this document. | ||
|
|
||
| foreach (SecurityKey key in configuration.SigningKeys) | ||
| { | ||
| if (key == null || key.CryptoProviderFactory == null || signingKeyId != key.KeyId) | ||
| continue; | ||
|
|
||
| try | ||
| { | ||
| configuration.Signature.Verify(key, key.CryptoProviderFactory); | ||
|
There was a problem hiding this comment. Looks like this can throw There was a problem hiding this comment. This is a good catch. On the our scenario this is not a real problem. If we deserialize the configuration using the metadataSerializer class the security key always use the default crypto provider and this value is never null. Having said that, it is possible to modify this property before calling the validator. We need to handle that scenario. I will update the logic to check if there is a crypto provider before attempting to verify. |
||
|
|
||
| return new ConfigurationValidationResult | ||
| { | ||
| Succeeded = true | ||
| }; | ||
| } | ||
| catch (XmlValidationException) | ||
|
There was a problem hiding this comment.
There was a problem hiding this comment. Although the only exception expected is this, the cryptoprovider may potentially throw any other exception. We could eat the exception and try to continue however given that we checked for the key thumbprint we do not expect any other key to be valid. We do not expect multiple entries with the same thumbprint and different crypto provider factory. In my opinion the best thing to do is to let that exception bubble up. The other alternative I see would be to update the ConfigurationValidationResult object to include exception information, given that that object is shared across ConfigurationValidators I would prefer to not change the pattern. There was a problem hiding this comment. We do want to try rest of the keys in the list if one fails correct? In that case, like Danny suggested, it should be continue. |
||
| { | ||
| // We know the signature is invalid at this point | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| return new ConfigurationValidationResult | ||
| { | ||
| ErrorMessage = LogMessages.IDX22713, | ||
| Succeeded = false | ||
| }; | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will modify this in our 7.x release when we will use the utf8Reader and set the ActiveTokenEndpoint to the "token_endpoint" and PassiveTokenEndpoint to "authorization_endpoint".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we keeping track of the changes we're intending to make/are in the process of making somewhere? We'll definitely want to make sure we have really clear release notes when we go to 7.x so it's clear who's impacted.
This comment is more an FYI for the community then? There's no action for this PR correct?