-
Notifications
You must be signed in to change notification settings - Fork 420
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
initial commit to move issuer validator to separate assembly from idweb #1736
Conversation
@@ -47,3 +47,4 @@ | |||
[assembly: InternalsVisibleTo("Microsoft.IdentityModel.KeyVaultExtensions.Tests, PublicKey=0024000004800000940000000602000000240000525341310004000001000100b5fc90e7027f67871e773a8fde8938c81dd402ba65b9201d60593e96c492651e889cc13f1415ebb53fac1131ae0bd333c5ee6021672d9718ea31a8aebd0da0072f25d87dba6fc90ffd598ed4da35e44c398c454307e8e33b8426143daec9f596836f97c8f74750e5975c64e2189f45def46b2a2b1247adc3652bf5c308055da9")] | |||
[assembly: InternalsVisibleTo("Microsoft.IdentityModel.Protocols, PublicKey=0024000004800000940000000602000000240000525341310004000001000100b5fc90e7027f67871e773a8fde8938c81dd402ba65b9201d60593e96c492651e889cc13f1415ebb53fac1131ae0bd333c5ee6021672d9718ea31a8aebd0da0072f25d87dba6fc90ffd598ed4da35e44c398c454307e8e33b8426143daec9f596836f97c8f74750e5975c64e2189f45def46b2a2b1247adc3652bf5c308055da9")] | |||
[assembly: InternalsVisibleTo("Microsoft.IdentityModel.Protocols.Tests, PublicKey=0024000004800000940000000602000000240000525341310004000001000100b5fc90e7027f67871e773a8fde8938c81dd402ba65b9201d60593e96c492651e889cc13f1415ebb53fac1131ae0bd333c5ee6021672d9718ea31a8aebd0da0072f25d87dba6fc90ffd598ed4da35e44c398c454307e8e33b8426143daec9f596836f97c8f74750e5975c64e2189f45def46b2a2b1247adc3652bf5c308055da9")] | |||
[assembly: InternalsVisibleTo("Microsoft.IdentityModel.Validators, PublicKey=0024000004800000940000000602000000240000525341310004000001000100b5fc90e7027f67871e773a8fde8938c81dd402ba65b9201d60593e96c492651e889cc13f1415ebb53fac1131ae0bd333c5ee6021672d9718ea31a8aebd0da0072f25d87dba6fc90ffd598ed4da35e44c398c454307e8e33b8426143daec9f596836f97c8f74750e5975c64e2189f45def46b2a2b1247adc3652bf5c308055da9")] |
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.
this is for the M.IM.Json opensource classes.
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.
LGTM
Thanks @jennyf19.
For the error message strings that were renamed form IDW to IDX401xx,
shall we want to do a transformation at the IdWeb level ? or just document that they had changed?
I'm not sure what we will do, but can discuss once these updates are out and we can try things out more easily. In reply to: 786380485 |
/// <exception cref="ArgumentNullException"> if <paramref name="validationParameters"/> is null.</exception> | ||
/// <exception cref="SecurityTokenInvalidIssuerException">if the issuer is invalid or if there is a network issue. </exception> | ||
public string Validate( | ||
string actualIssuer, |
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.
suggest name: issuer rather than actualIssuer. #Resolved
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.
fixed
{ | ||
if (string.IsNullOrEmpty(actualIssuer)) | ||
{ | ||
throw new ArgumentNullException(nameof(actualIssuer)); |
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.
normally we log all exceptions thrown using LogHelper.LogArgumentNullException, we should follow this same pattern here.
see:
Line 223 in d8ad857
throw LogHelper.LogExceptionMessage(new SecurityTokenInvalidIssuerException(LogMessages.IDX10211) |
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.
yes, thanks! i meant to look into this yesterday. it was on my list. updated and fixed.
namespace Microsoft.IdentityModel.Validators | ||
{ | ||
/// <summary> | ||
/// Generic class that validates token issuer from the Microsoft identity platform (AAD). |
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.
Wilson validates SAML and SAML2 tokens, we need to comment that this class only works with either a JsonWebToken or a JwtSecurityToken. #Resolved
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.
updated the comment. good point.
/// Initializes a new instance of the <see cref="AadIssuerValidatorFactory"/> class. | ||
/// </summary> | ||
/// <param name="httpClient">Optional HttpClient to use to retrieve the endpoint metadata (can be null).</param> | ||
public AadIssuerValidatorFactory( |
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.
normally we don't use default parameters and have multiple ctors.
The reason is adding a default parameter can cause a runtime failure, so we just choose to not use them for public apis. #Resolved
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.
make sense. we had an issue w/this w/MSAL.NET a few weeks ago.
fixed.
catch (Exception ex) | ||
{ | ||
throw LogHelper.LogExceptionMessage(new SecurityTokenInvalidIssuerException( | ||
string.Format( |
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.
LogHelper.FormatInvariant is used on other places to make sure log parameters are obfuscated. There is a parallel work stream that would allow non-PII arguments not to get obfuscated, but it will work in conjunction with LogHelper.FormatInvariant. #Resolved
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.
should i use LogHelper.FormatInvariant
instead?
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.
yes please use LogHelper.FormatInvariant
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.
fixed. thanks. very cool
throw new ArgumentNullException(nameof(validationParameters)); | ||
} | ||
|
||
string tenantId = GetTenantIdFromToken(securityToken); |
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.
Can we pull GetTenantId into IsValidIssuer? Thinking that for single-tenant apps there will be no need to extract tenantId and it would result in a waste of cycles for each request.
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.
potentially IsValidIssuer could be called several times. would you recommend having the tenantId as a ref parameter of IsValidIssuer? computed the first time
if (securityToken == null) | ||
{ | ||
throw new ArgumentNullException(nameof(securityToken)); | ||
} |
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.
if (securityToken == null) | |
{ | |
throw new ArgumentNullException(nameof(securityToken)); | |
} | |
_ = securityToken ?? throw new ArgumentNullException(nameof(securityToken)); | |
``` #Resolved |
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.
nice! thx. fixed.
@jmprieur there is more work to do before this should be pulled in. |
TokenValidationParameters validationParameters) | ||
{ | ||
_ = issuer ?? throw new ArgumentNullException(nameof(issuer)); | ||
|
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 usually don't have blank lines.
we try to have minimal spacing #Resolved
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.
sounds good. have removed several.
/// </summary> | ||
public class AadIssuerValidator | ||
{ | ||
internal AadIssuerValidator( |
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.
How will this constructor be used? #Resolved
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.
Something like the following:
AadIssuerValidatorFactory factory = new AadIssuerValidatorFactory(/* http client */);
options.TokenValidationParameters.IssuerValidator = factory.GetAadIssuerValidator(authority).Validate;
@jennyf19 : we should add a <example><code>
blob in the XML comment to show how this method is used.
Or with SAL policies, this sample code
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.
update the xml
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.
I am questioning the need for the factory, a static method on the class would do the same thing.
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.
fixed
/// <summary> | ||
/// Factory class for creating the AadIssuerValidator per authority. | ||
/// </summary> | ||
public class AadIssuerValidatorFactory |
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.
I am not completely understanding the need for this factory.
It seems that the usage pattern is a singleton that is attached to TokenValidationParameters on startup.
Help me understand the need to cache the delegate.
We could just as easily have a static method on the class. #Pending
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.
Previously we had a static, but customers wanted to inject an HttpClient to retrieve the OpenId Connect doc, so we moved to a factory and we cache the delegates because there can be several authories in an application and several token versions.
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.
There could be two overloads and one could have a HttpClient as a parameter.
When there is no HttpClient passed, we need to use the HttpClient from BaseConfigurationManager.
Reusing the same HttpClient (in the null case we create and reuse a HttpClient) could lead to stale DNS issues.
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.
i think is is resovled now by using the validationParams to get the issuer, in the v2 case.
/// <summary> | ||
/// An implementation of IConfigurationRetriever geared towards Azure AD issuers metadata. | ||
/// </summary> | ||
internal class IssuerConfigurationRetriever : IConfigurationRetriever<IssuerMetadata> |
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.
Why do we need a custom IConfigurationRetriever?
Are we getting a standard discovery document? #Resolved
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.
I'm not sure. What do you recommend?
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 do retrieve a standard discovery document, but only are interested in the issuer property, so why deserialize everything?
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.
Everything will get deserialized anyway, the entire json is read into memory.
That will take place once and is a very small time compared to the http get to the discovery endpoint.
These custom classes will need to be supported for a long time, it they are not needed, i recommend using the standard.
In the future is we need anything additional from AAD, we will need to change the object design.
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.
got it. fixed.
{ | ||
if (securityToken.Issuer.EndsWith("v2.0", StringComparison.OrdinalIgnoreCase)) | ||
{ | ||
if (AadIssuerV2 == null) |
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.
In the v2 case can't TokenValidationParameters.ConfigurationManager and / or Configuration be used to get the issuer from: BaseConfiguration? #Resolved
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.
This was a new feature we just added a couple of weeks ago.
@mafurman knows most about it.
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.
yes. fixed. thanks for the pointing that out.
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.
The logic here for making use of TokenValidationParameters.ConfigurationManager + Configuration looks good. :)
This should fit well into the most recent Last Known Good (LKG) PR as one of the exceptions it looks for is SecurityTokenInvalidIssuerException in order to determine if LKG fallback or configuration refresh is necessary.
private ConfigurationManager<IssuerMetadata> CreateConfigManager( | ||
string aadAuthority) | ||
{ | ||
if (HttpClient != null) |
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 should be using the HttpClient from BaseConfigurationManager, we will need to expose that.
The asp.net team asked us to make sure all calls use that HttpClient. #Resolved
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.
that's cool. we can have a static method then.
|
||
if (securityToken is JsonWebToken jsonWebToken) | ||
{ | ||
jsonWebToken.TryGetPayloadValue(AadIssuerValidatorConstants.Tid, out string tid); |
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.
this could be written (same below):
if (jsonWebToken.TryGetPayloadValue(AadIssuerValidatorConstants.Tid, out string tid) return tid
#Resolved
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.
fixed.
return (string)tid; | ||
|
||
jwtSecurityToken.Payload.TryGetValue(AadIssuerValidatorConstants.TenantId, out object tenantId); | ||
if (tenantId != null) |
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.
why check null here but not above? #Resolved
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.
not sure, but have used what you suggested here as well.
{ | ||
try | ||
{ | ||
Uri issuerFromTemplateUri = new Uri(validIssuerTemplate.Replace("{tenantid}", tenantId)); |
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.
Why do we need to create the Uri's?
Below we just compare the strings. #Resolved
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.
will do, but need to check on the actual string format, example if there are trailing /'s anywhere or not.
if (AadIssuerV2 == null) | ||
{ | ||
IssuerMetadata issuerMetadata = | ||
CreateConfigManager(AadAuthority).GetConfigurationAsync().ConfigureAwait(false).GetAwaiter().GetResult(); |
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.
I think there is a race condition here.
Consider two threads with different tokens, one v1, one v2.
AadAuthority is used in both cases. CreateV1Authority could modify AadAuthority.
It may be simpler to have two separate variables and initialize them on creation since the change in the V1 case is not dependent on the token arriving.
In any case, two variables would help. #Resolved
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.
good point. fixed.
return uri.Segments[1].TrimEnd('/'); | ||
|
||
if (uri.Segments.Length == 5 && uri.Segments[1].TrimEnd('/') == AadIssuerValidatorConstants.Tfp) | ||
throw new SecurityTokenInvalidIssuerException(LogMessages.IDX40104); |
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.
make sure to use LogHelper.LogExceptionMessage when throwing. #Resolved
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.
yes missed this one. i've checked the remaining files as well and updated if needed.
/// TokenValidationParameters.IssuerValidator = aadIssuerValidator.Validate; | ||
/// </code></example> | ||
/// <remarks>The issuer is considered as valid if it has the same HTTP scheme and authority as the | ||
/// authority from the configuration file, has a tenant ID, and optionally v2.0 (this web API |
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.
nit: should this say "(if this web API...)" #Resolved
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.
probably, yes. thanks. fixed.
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.
Do we need to add any tests that plug this new IssuerValidator into the JwtSecurityTokenHandler/JsonWebTokenHandler? I know we have tests that use custom validators so it might not be necessary.
|
||
return issuerFromTemplate == actualIssuer; | ||
} | ||
#pragma warning disable CA1031 // Do not catch general exception types |
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 generally put these in the GlobalSuppressions.cs file that's present in each assembly. #Resolved
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.
makes sense. was following another class that did this, but have added a GS file. thanks again
namespace Microsoft.IdentityModel.Validators | ||
{ | ||
/// <summary> | ||
/// Generic class that validates either JsonWebTokens or JwtSecurityTokens issued from the Microsoft identity platform (AAD). |
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.
Should this comment say "that validates the issuer for either"? #Resolved
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.
yes! fixed
{ | ||
if (securityToken.Issuer.EndsWith("v2.0", StringComparison.OrdinalIgnoreCase)) | ||
{ | ||
if (AadIssuerV2 == null) |
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.
The logic here for making use of TokenValidationParameters.ConfigurationManager + Configuration looks good. :)
This should fit well into the most recent Last Known Good (LKG) PR as one of the exceptions it looks for is SecurityTokenInvalidIssuerException in order to determine if LKG fallback or configuration refresh is necessary.
|
||
var actualIssuers = validator.Validate(ValidatorConstants.AadIssuer, jwtSecurityToken, new TokenValidationParameters() { ValidIssuers = new[] { ValidatorConstants.AadIssuer } }); | ||
|
||
Assert.Equal(ValidatorConstants.AadIssuer, actualIssuers); |
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.
nit: We generally like to use our CompareContext and IdentityComparer classes during testing as they allow us to get more information out of test failures (e.g. if the first AssertEqual here fails we won't know anything about the next one). #Resolved
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.
cool. I didn't see that. i've used IdentityComparer. thanks
- xml comments - use `IdentityComparer` in tests clean-up
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.
LGTM
Thanks @jennyf19
initial commit to move issuer validator to separate assembly from MS.Id.Web
I'm unable to get the VS test runner to run, but the tests pass in the command line. will keep trying to get that sorted out.
@GeoK have addressed your PR comments from the initial PR in Id.Web.
note: only code changes were from suggestions 1 and 2 from @GeoK between Id.Web and this commit.
Also resolves #1731
Update @brentschmaltz
In
OpenIdConnectOptions
configuration, we would now do the following with the latest changes (10.24):Update 10.25
Tested M.IM.Validators with ID Web -> Web app calls web API calls graph & B2C calls web API