Identityserver4: Add more configuration validation to token/authorize pipeline

[leastprivilege]

initiated by https://github.com/IdentityServer/IdentityServer4/issues/2072

let's collect more config checks here and then implement them in one go.

• AccessTokenLifetime is 0

what else?

[LindaLawton]

I think we should limit this to configuration errors that can cause hard to diagnose errors. Which in and of itself is going to be really hard to come up with a sensible list.

1. Browser enabled with no redirect URI set?
2. Using JavaScript without setting cors (not sure on this one my cors knowledge is lacking)
3. RequireClientSecret missing a secret?

I don't think its worth invaliding the max tokenlifetime, being that its an int the potential age of the access token is greater then the epoch time. I think you and I discussed this on stack. link

What i would to do is create a system validating clients so that we can set up what is critical for each of the grant types as well as add the ability for developers to add validation for their own custom grant types. Possibly even over ride the standard settings that we set for the existing grant types.

[brockallen]

Ideas:

• maybe add a check to IClientStoreExtensions.FindEnabledClientByIdAsync since most (if not all) of our code should be funneling thru that API to load a client
  
  
  
  • maybe add a new service in DI to do the validation. possibly devs could replace to add their own checks? On the values in Properties for example?
  
  

[LindaLawton]

On the values in Properties for example? <---- Love that idea since i am using properties.

[chrisowhite]

Are you guys looking to implement specific default rules for 2.x or would that be a 3.0 thing (saw the comment from @leastprivilege in the above PR)?

[leastprivilege]

I will merge the base infrastructure today. But it will be no-op by default. Then we can "crowd source" validation rules to be used optionally. They will then become the default in 3.0

[leastprivilege]

OK - here's the new validator:

https://github.com/IdentityServer/IdentityServer4/blob/dev/src/IdentityServer4/Validation/DefaultClientConfigurationValidator.cs

It's not enabled by default - but in the host it is:
https://github.com/IdentityServer/IdentityServer4/blob/dev/src/Host/Startup.cs#L52

I added a bunch of validation checks already. Feel free to PR to add new ones!

Each check should have a corresponding test here:
https://github.com/IdentityServer/IdentityServer4/blob/dev/test/IdentityServer.UnitTests/Validation/ClientConfigurationValidation.cs

[LindaLawton]

What happens if no context.Client.PostLogoutRedirectUris is supplied? Should we have a dummy check on that one as well?

Looks great very clean way of doing it.

[leastprivilege]

Well - that's an optional setting.

[leastprivilege]

https://github.com/IdentityServer/IdentityServer4/issues/2317

[janpieterz]

When, for example, using an int.MaxValue (a valid configuration value) for SlidingRefreshTokenLifetime, the result of below line can mean the integer becomes a negative value. This is relevant for scenario's with, for example, the AbsoluteRefreshTokenLifetime set to 0 and a Sliding expiration, where people expect them to be valid forever (or at least significantly long enough to not matter).

https://github.com/IdentityServer/IdentityServer4/blob/325ea0e67b1de2c836f26eb38436ff07177028d4/src/IdentityServer4/Services/DefaultRefreshTokenService.cs#L120

There's a couple of options here:

1. Ensure the configuration can never exceed int.MaxValue / 2 when configuring a client (but what about database changes?)
2. Change the processing code to ensure this won't happen (either on load of client settings limit it to int.MaxValue / 2 and log it, or when issuing a new lifetime ensure it doesn't pass it and go negative)
3. Change it to a uint and if exceeding set it to uint.MaxValue
4. Ignore 😄

Happy to submit a PR once we know the option.

[leastprivilege]

@brockallen opinion?

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[lock[bot]]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.