You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by GitBox <gi...@apache.org> on 2020/06/09 15:49:00 UTC
[GitHub] [guacamole-client] lapchynski opened a new pull request #517: GUACAMOLE-1094: Add overridable response_type to auth-openid extension
lapchynski opened a new pull request #517:
URL: https://github.com/apache/guacamole-client/pull/517
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](https://issues.apache.org/jira/browse/GUACAMOLE-1094) for further explanation.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] necouchman commented on pull request #517: GUACAMOLE-1094: Add overridable response_type to auth-openid extension
Posted by GitBox <gi...@apache.org>.
necouchman commented on pull request #517:
URL: https://github.com/apache/guacamole-client/pull/517#issuecomment-840848797
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] necouchman commented on pull request #517: GUACAMOLE-1094: Add overridable response_type to auth-openid extension
Posted by GitBox <gi...@apache.org>.
necouchman commented on pull request #517:
URL: https://github.com/apache/guacamole-client/pull/517#issuecomment-640761113
@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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] lapchynski edited a comment on pull request #517: GUACAMOLE-1094: Add overridable response_type to auth-openid extension
Posted by GitBox <gi...@apache.org>.
lapchynski edited a comment on pull request #517:
URL: https://github.com/apache/guacamole-client/pull/517#issuecomment-642137980
@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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] lapchynski commented on pull request #517: GUACAMOLE-1094: Add overridable response_type to auth-openid extension
Posted by GitBox <gi...@apache.org>.
lapchynski commented on pull request #517:
URL: https://github.com/apache/guacamole-client/pull/517#issuecomment-640774842
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] knacktim commented on pull request #517: GUACAMOLE-1094: Add overridable response_type to auth-openid extension
Posted by GitBox <gi...@apache.org>.
knacktim commented on pull request #517:
URL: https://github.com/apache/guacamole-client/pull/517#issuecomment-641458650
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:
I think long term Guacamole will want to consume that public info to at least validate its configuration.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] knacktim commented on pull request #517: GUACAMOLE-1094: Add overridable response_type to auth-openid extension
Posted by GitBox <gi...@apache.org>.
knacktim commented on pull request #517:
URL: https://github.com/apache/guacamole-client/pull/517#issuecomment-642144188
> 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](https://openid.net/developers/certified/) 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] lapchynski commented on pull request #517: GUACAMOLE-1094: Add overridable response_type to auth-openid extension
Posted by GitBox <gi...@apache.org>.
lapchynski commented on pull request #517:
URL: https://github.com/apache/guacamole-client/pull/517#issuecomment-642137980
@knacktim It looks like at least AWS Cognito and GCP Identity Platform have a discovery document 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 discovery document 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 discovery document to begin with.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org