guacamole-client icon indicating copy to clipboard operation
guacamole-client copied to clipboard

GUACAMOLE-1094: Add overridable response_type to auth-openid extension

Open lapchynski opened this issue 5 years ago • 8 comments

Allows overriding the default value of the response_type parameter in the OpenID auth extension for compatibility with authentication servers that require specific values that are different than the default "id_token", like AWS Cognito. See the Jira issue for further explanation.

lapchynski avatar Jun 08 '20 16:06 lapchynski

@lapchynski: Thanks for the contribution. Overall this looks good to me. My only question would be - are there a set of discreet values that are valid response types, and instead of making this a free-form field, should we instead try to make sure that the admin is putting something into the field that is valid per the OID specification?

necouchman avatar Jun 08 '20 17:06 necouchman

Personally I think it'd be better as a free-form field to allow the admin more flexibility with OIDC implementations that may not follow the spec to the letter. If everything followed the spec to the letter then leaving "response_type=id_token" hard-coded wouldn't be an issue, but by limiting it to a discrete set of values we'd be excluding any IdP implementations that are out of spec in a way we didn't consider and include in the list of allowed values. And if an admin breaks something in their setup with an out-of-spec value then I think that's on them to debug in their particular setup.

But if you think a discrete set of valid values would better follow the conventions of the rest of the project or have other reasons for preferring it over free-form, I'd be open to changing it.

Or alternatively, it could log a warning on out-of-spec values.

lapchynski avatar Jun 08 '20 17:06 lapchynski

From my experience most OIDC implementations have a public info endpoint that reveals all of the values it supports in that field.

For example Keycloak has: https://<host>/auth/realms/default/.well-known/openid-configuration

Which returns: image

I think long term Guacamole will want to consume that public info to at least validate its configuration.

tworcester avatar Jun 09 '20 17:06 tworcester

@knacktim It looks like at least AWS Cognito and GCP Identity Platform have a provider metadata endpoint, although for Cognito it doesn't seem to be documented or I couldn't find documentation for it if it is.

I think using the provider metadata for validation would be a good idea, but since if we're already using that data for validation why not just set the rest of the options (authorization-endpoint, jwks-endpoint, issuer, etc) directly from it and make the equivalent guacamole.properties options optional overrides? If I'm understanding the spec correctly, that seems like it's kinda the point of the provider metadata endpoint to begin with.

lapchynski avatar Jun 10 '20 17:06 lapchynski

I think using the provider metadata for validation would be a good idea, but since if we're already using that data for validation why not just set the rest of the options (authorization-endpoint, jwks-endpoint, issuer, etc) directly from it and make the equivalent guacamole.properties options optional overrides? If I'm understanding the spec correctly, that seems like it's kinda the point of the provider metadata endpoint to begin with.

Totally agree with this. That is actually how the oidc-client-js library here implements things. You give it the 'Authority' property and it uses that + the OIDC standard endpoints to configure itself using the metadata endpoint. Then provides an override in the local configuration.js file.

tworcester avatar Jun 10 '20 17:06 tworcester

@lapchynski: Sounds like maybe there's an opportunity to go another route here and pull most of the values from the OID provider. Any progress on that?

Also, there are some merge conflicts, now.

necouchman avatar May 13 '21 21:05 necouchman