Improve comments to clarify API usage and avoid unintentional validation weakening.

[TimHannMSFT]

• Updating some headers for clarity
• Marking some setters on TokenValidationParameters as obsolete

[TimHannMSFT]

Opted to just mark the following obsolete

        "RequireExpirationTime",  // Can be overridden by the LifetimeValidator
        "ValidateAudience", // Can be overridden by the AudienceValidator
        "ValidateIssuer",  // Can be overridden by the IssuerValidator
        "ValidateLifetime",  // Can be overridden by the LifetimeValidator

Everything else seemed to have valid usages or no good override validator (RequireAudience was always evaluated and couldn't be skipped by using the associated validator).

[TimHannMSFT]

For ease of review, here's the proposed wording for the release notes (https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/releases/tag/untagged-cc6fe1c38dd4b5bc70e2):

Breaking changes from 6.6.0:
If the consumer has turned on warnings as errors they may face breaking changes from some TokenValidationParameters being marked as obsolete/deprecated. There's no intention at the moment to remove these properties but changing them from the defaults represents significant degradation in security and is strongly recommended against. (the setters for RequireExpirationTime, ValidateAudience, ValidateIssuer, and ValidateLifetime have all been marked as obsolete to reflect they should no longer be used to alter the default values--as altering the default values disables important validation). For best practices on token validation, please refer to the wiki.

[kevinchalet]

the setters for RequireExpirationTime, ValidateAudience, ValidateIssuer, and ValidateLifetime have all been marked as obsolete to reflect they should no longer be used to alter the default values--as altering the default values disables important validation

I respectfully disagree with the changes introduced in this PR: there are actual use cases for disabling validation (e.g you typically don't want to reject identity tokens used as id_token_hints just because they are expired).

I'm even tempted to say that the recommended alternative - assigning a delegate that does nothing - is way more dangerous than the existing approach, which is crystal clear and can be flagged by analyzers if needed.

Do you seriously think that IssuerValidator = (issuer, token, tvp) => { return issuer; } is better than ValidateIssuer = false?

[leastprivilege]

I agree.

There are also valid use cases for not validating an audience. OAuth does not even have the concept of an audience. they call it scope instead.

iss, aud, exp etc. are all optional claims per JWT spec.

[TimHannMSFT]

Thanks for the feedback @kevinchalet & @leastprivilege , still discussing options inside the team. We are also investigating analyzers. Re: delegate vs bool flag, thinking around this is that by setting the delegate the consumer is much more actively--and obviously--denoting they're taking matters into their own hands; happy to admit reasonable people can disagree on this point though.

At base we're trying to best balance pain of this change with pain of security gaps. Both of your examples are good points of data in that discussion. Much appreciated.

[kevinchalet]

Thanks for the feedback @kevinchalet & @leastprivilege , still discussing options inside the team.

I'm sure you're already well aware of that, but I'd like to emphasize that IdentityModel is used in a myriad of third-party libraries, including @leastprivilege's IdentityServer and the OpenIddict stack I maintain. In these libraries, IdentityModel often has a critical role to play and it's something I'd love Microsoft to keep in mind: discussing options internally is fine, discussing options with everyone concerned - community included - would be way better. I know IdentityModel is maintained by the Azure AD team and that first-party usages dictate its development, but please don't forget the external projects that depend on it.

[leastprivilege]

I personally would like to avoid using this library at all. Given the importance and central role in the .NET stack - code quality, design decisions and versioning policies have been problematic in the past (to say the least).

Unfortunately the tight coupling with ASP.NET Core makes this not feasible right now.

JWT (and its satellites) are specs - they are immutable. Once there is an implementation for a spec, it is done. That's what the core JWT lib should be. If you then want to layer the opinionated (or AAD) approach of the day on top - do it separately, but leave the core alone.

JsonWebToken/Handler is a step in the right direction. TokenValidationParameters needs a rebirth as well for that plumbing.

..and instead of massaging some properties - maybe you should fix your JSON serialization problems first.

[brentschmaltz]

@leastprivilege

and instead of massaging some properties - maybe you should fix your JSON serialization problems first.

As you noticed we have invested in JsonWebToken/Handler and will continue to do so.
We have improved the performance of our base64urlencoding and will be moving to the Concept of ClaimsSets, with JsonClaimSet as being the first.
For NET 461 and above, we are switching to System.Text.Json, decoding the JWT into bytes, then accessing claims directly from the JsonElement.
The ClaimsIdentity will be Lazy and only users that need will cause the json to be translated to claims.
Most of the work in taking place in this topic branch brentsch/json.

[kevinchalet]

We are also investigating analyzers.

For the record, it's happening here:

• Add analyzers for Microsoft.IdentityModel.Tokens (Wilson) Security Issues dotnet/roslyn-analyzers#5420
• Document new Roslyn Analyzer Rules for Wilson Token Validation dotnet/docs#25911

@TimHannMSFT it's always a good idea to add links to related tickets/PRs so that folks can track the progress of a feature, specially when the approach changes quite radically.

[TimHannMSFT]

Appreciate the community feedback here. After getting more feedback and discussing internally we have opted to abandon this change. As I mentioned (and @kevinchalet linked above), we're looking to solve the same problem via Roslyn analyzers which will allow a much more granular opt out which allows us to better meet our goals while providing minimal disruption for consumers of this library.