Tokens from ADFS are failing validation

[brentschmaltz]

see: https://github.com/aspnet/Security/issues/1852#issuecomment-427673049 for details. Short story is ADFS has defined a separate "access_token_issuer" in metadata to validate against for the issuer.

[brentschmaltz]

We should at a minimum add a first class property to OpenIdConnectConfiguration: AccessTokenIssuer that picks up the property: 'access_token_issuer'.

[rasitha1]

Couple of things to note:

1. Since 'access_token_issuer' is ADFS specific and not part of the OIDC spec, does it make sense to add the property to OpenIdConnnectConfiguration class? (That's why I was thinking using the AdditionalData dictionary but somehow pull that to ValidIssuers list.)

2. I was looking at the OIDC spec and saw this line:

The issuer value returned MUST be identical to the Issuer URL that was directly used to retrieve the configuration information. This MUST also be identical to the iss Claim value in ID Tokens issued from this Issuer.

Isn't ADFS violating the last part of above statement when it is issuing tokens with iss with /service/trust instead of the hosted url? If so, this will just be a workaround for existing ADFS instances where we can't change the /servie/trust uri. So does it still make sense to add this public property to OpenIdConnectConfiguration class?

[AndersAbel]

@rasitha1 The ADFS behaviour is definitely non-standard. When deploying a new ADFS farm, the fix is to change the federation service identifier (which is the value used for access_token_issuer) so that it is the same as the issuer field.

I think that adding support for handling this would make sense in a Microsoft library. But I also think it should be an opt-in setting. In the Sustainsys.Saml2 library I have a config section Compatibility where flags for enabling non-standard behaviour goes. I think the same would make sense here, to make clear what happens. To help developers, a detection could be added that throws a helpful exception message indicating that setting the flag would help.

[brentschmaltz]

@rasitha1 @AndersAbel I agree with you. Technically, the OIDC spec is about id_tokens. So purists will argue that an access token is a contract between the idp and resource manager. I find this while technically correct, practically infeasible.

I think your idea of 'opt in' is needed. @AndersAbel is your config standard config or do you have a code equivalent?

I know that adding the 'access_token_issuer' as a first class property is a bit microsoftish, but it's about helping customers. We don't have to apply it automatically unless the user opts in, but it will be easy to find.

[AndersAbel]

@brentschmaltz The compatibility flags are gathered in one place in https://github.com/Sustainsys/Saml2/blob/master/Sustainsys.Saml2/Configuration/Compatibility.cs. So it is a per-handler-instance-setting.

I've also added information about compatibility flags in exception messages. This reduced the number of support questions dramatically:

https://github.com/Sustainsys/Saml2/blob/9dfacfc7d784fe961fccc5eec735767bf3a95358/Sustainsys.Saml2/SAML2P/Saml2Response.cs#L176

[mafurman]

We need to coordinate with ASP.NET to include additional OpenIdConnectConfiguration data on the TokenValidationParameters.PropertyBag.

[Tratcher]

Somewhere around here? https://github.com/aspnet/AspNetCore/blob/86728ecc17c339f699d21c8abd5ee93037b51a1e/src/Security/Authentication/OpenIdConnect/src/OpenIdConnectHandler.cs#L1213-L1220 Since this is custom data I assume you'd need an extension point where people could manipulate it to suite their scenarios.

[brentschmaltz]

@Tratcher we could add it there, but since this is for the access_token, perhaps JwtBearer. The value will be in the AdditionalData property. https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/blob/72f03a9c62215688b759ea180263db8b498280ea/src/Microsoft.IdentityModel.Protocols.OpenIdConnect/Configuration/OpenIdConnectConfiguration.cs#L107. Not sure how to special case this yet.

[Tratcher]

Oh, JwtBearer would be here: https://github.com/aspnet/AspNetCore/blob/4cb86bc15160e493e6331190da9973736f0fc240/src/Security/Authentication/JwtBearer/src/JwtBearerHandler.cs#L91-L99

[brentschmaltz]

@mafurman @RojaEnnam we could handle this with our ASP.NET handler.

[jennyf19]

@brentschmaltz thoughts on this one?

[brentschmaltz]

@jennyf19 this is an edge case. I believe we will have to expand BaseConfiguration to contain AdditionalData where the "access_token_issuer" can be found. Then a custom IssuerValidator could be used.

[brentschmaltz]

@jennyf19 @Tratcher closing as this is an edge case.