aspnet / AspNetKatana

Microsoft's OWIN implementation, the Katana project
Apache License 2.0
963 stars 332 forks source link

OpenIdConnect: RequireState = true, always throws an exception #385

Closed nielszz closed 3 years ago

nielszz commented 3 years ago

RequireState defaults to true in the Microsoft.IdentityModel.Protocols.OpenIdConnect sub dependency. The Openid spec recommends to use it for the authorization code flow. source 3.1.2.1.

The problem is in this part of the code:

https://github.com/aspnet/AspNetKatana/blob/2c68ecff843266b852328e07626a97450b19d27a/src/Microsoft.Owin.Security.OpenIdConnect/OpenidConnectAuthenticationHandler.cs#L368

But because state is actually never set on the created OpenIdConnectProtocolValidationContext object this always throws an exception:

https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/blob/dev/src/Microsoft.IdentityModel.Protocols.OpenIdConnect/OpenIdConnectProtocolValidator.cs#L710

The workaround is of course to disable State validation, but it would be more appropiate to fix this issue. Or was this a deliberate choice at some point?

I also saw this old issue, but i don't know if the explanation there is still correct. Either way it is confusing that it should be set to false, in order to work.

Version used: 4.1.1.

Tratcher commented 3 years ago

The state is on the authorizationResponse OpenIdConnectMessage. There's already a separate state check requirement earlier in the code: https://github.com/aspnet/AspNetKatana/blob/2c68ecff843266b852328e07626a97450b19d27a/src/Microsoft.Owin.Security.OpenIdConnect/OpenidConnectAuthenticationHandler.cs#L290-L295 That's checking for state provided by the OpenidConnectAuthenticationHandler itself during the initial challenge. https://github.com/aspnet/AspNetKatana/blob/2c68ecff843266b852328e07626a97450b19d27a/src/Microsoft.Owin.Security.OpenIdConnect/OpenidConnectAuthenticationHandler.cs#L159

Can you back up and explain what higher level problem you're dealing with? Are you getting a specific error?

nielszz commented 3 years ago

Thanks for the reply.

I don't really have an issue if I set RequireState to false , it is confusing though. And at the least requires a comment with explanation in our codebase, to not confuse the other developers.

Anyway the problem arose, when upgrading from version 3.1.0. Perhaps that version was not as strict / had other defaults compared to the Microsoft.Owin.Security.OpenIdConnect 4.x package. Probably that in combination with intializing our own OpenIdConnectProtocolValidator object.

I think I can jump to the same conclusion that older issue had. It's not a bug, but it is somewhat implicit / unclear why it is legitimate to set it to false. I think we can resolve this issue.

Tratcher commented 3 years ago

I'm still confused why your getting an error, the state should be there.

nielszz commented 3 years ago

This is the stacktrace, this occurs because Microsoft.IdentityModel.Protocols.OpenIdConnect.OpenIdConnectProtocolValidator initializes RequireState with true, as can be seen here

ERROR Microsoft.IdentityModel.Protocols.OpenIdConnect.OpenIdConnectProtocolInvalidStateException: IDX21329: RequireState is '[PII is hidden]' but the OpenIdConnectProtocolValidationContext.State is null. State cannot be validated. at Microsoft.IdentityModel.Protocols.OpenIdConnect.OpenIdConnectProtocolValidator.ValidateState(OpenIdConnectProtocolValidationContext validationContext) in C:\agent2\_work\56\s\src\Microsoft.IdentityModel.Protocols.OpenIdConnect\OpenIdConnectProtocolValidator.cs:line 710 at Microsoft.IdentityModel.Protocols.OpenIdConnect.OpenIdConnectProtocolValidator.ValidateAuthenticationResponse(OpenIdConnectProtocolValidationContext validationContext) in C:\agent2\_work\56\s\src\Microsoft.IdentityModel.Protocols.OpenIdConnect\OpenIdConnectProtocolValidator.cs:line 262 at Microsoft.Owin.Security.OpenIdConnect.OpenIdConnectAuthenticationHandler.<AuthenticateCoreAsync>d__11.MoveNext() Microsoft.IdentityModel.Protocols.OpenIdConnect.OpenIdConnectProtocolInvalidStateException: IDX21329: RequireState is '[PII is hidden]' but the OpenIdConnectProtocolValidationContext.State is null. State cannot be validated. at Microsoft.IdentityModel.Protocols.OpenIdConnect.OpenIdConnectProtocolValidator.ValidateState(OpenIdConnectProtocolValidationContext validationContext) in C:\agent2\_work\56\s\src\Microsoft.IdentityModel.Protocols.OpenIdConnect\OpenIdConnectProtocolValidator.cs:line 710 at Microsoft.IdentityModel.Protocols.OpenIdConnect.OpenIdConnectProtocolValidator.ValidateAuthenticationResponse(OpenIdConnectProtocolValidationContext validationContext) in C:\agent2\_work\56\s\src\Microsoft.IdentityModel.Protocols.OpenIdConnect\OpenIdConnectProtocolValidator.cs:line 262 at Microsoft.Owin.Security.OpenIdConnect.OpenIdConnectAuthenticationHandler.<AuthenticateCoreAsync>d__11.MoveNext()

And we initialize the ProtocolValidator ourselves, so the options that are set here get lost in our case: https://github.com/aspnet/AspNetKatana/blob/569f1c872e86e57155d7865f4b3d627949cfcba2/src/Microsoft.Owin.Security.OpenIdConnect/OpenIdConnectAuthenticationOptions.cs#L61

Does that clarify the problem? If i'm right I also think there was a migration from Microsoft.Identitymodel.Protocols.Extensions perhaps that may also explain the difference in behavior. At least for the upgrade to Owin 4.1.1 it had to be removed.

Tratcher commented 3 years ago

Oh, that makes more sense. Yes, if you're replacing the validator then you need to reconfigure it. However, rather than replacing the validator why not change the settings on the existing one?

nielszz commented 3 years ago

Had not yet thought about that, that's a simple but effective solution :)

Either way it makes sense now why it's disabled by default. Thanks for the help.

I'm closing this issue.