GUACAMOLE-2258 : OpenID code flow, client secret and PKCE#1198
Conversation
adb014
changed the title
|
I've figured out how to use a SessionManager directly in getLoginURI to store the PKCE verifier. The local REST redirect and callback for the session are no longer needed and have be removed. Making the implementation cleaner |
|
Looking at the discussion in #517, I finally decided to add the well-known endpoint and an enum for the response_type to limit the values allowed. Given this it was also easy to add the use of "token" response_types as discussed in #517 for AWS Cognito. A minimal guacamole.properties can then be The issuer, authorization, token and jwks endpoints returned by the well-known endpoint can be overridden with the previous endpoints in which case it’s possible to not have a well-known endpoint. Can't see anything else to add to this patch. Can someone review it ? |
necouchman
left a comment
necouchman
left a comment
There was a problem hiding this comment.
@adb014 I've done an initial review, here, and, generally speaking, I believe what you've got is pretty sound. Most of the comments I have made have to do with style - being consistent about the style you've implemented, and matching that to the existing code style. If you take a look at those comments I think you'll get the gist of what to pay attention to.
From a functional perspective, my main concern is that I believe you've written/tested this assuming that the OpenID well-known endpoint will be provided, and there are some places where it seems like the code, as implemented, would error out or at least behave erratically if the well-known endpoint happened to be left out of the configuration. So, a little work to make sure that the changes you're making don't break existing configurations would be great.
Thanks for picking this up and running with it - it seems like there's a growing community of folks who will be very happy to have these additional OpenID functions available.
|
I’m on my phone at the moment, so will look at this later. Though I tested with 7 scenarios
The PKCE path seemed sufficiently independent that it only needed testing once. All of these were tested against a local keycloak only. If the well-known endpoint is not given then the calls like confService.getIssuer() will see the manual value in priority and return it. If only the well-known endpoint is set, then the confWellKnown.getIssuer() method is called from within confService.gerIssuer(). This call might fail initially if the well-known data is not available yet. I tested in docker and the guacamole container starts faster than keycloak 😆. This seemed like a likely scenario so I protected the detection of the well-known data in a runnable called till valid data is returned or it times out. This does mean that the identity provider must be available at least once in the first 2 minutes of starting guacamole, and the well-known data won’t be looked for later. Even if the call to fails initially on start up following calls with later connections will work once the well-known data is found. Should I allow the init function of confWellKnown to run later during calls to the getters ? Seemed that the failure scenario was sufficiently rare that it’s not needed |
|
Ok, I've treated all of your comments. Thinking about it, I still have one issue. Guacamole displays the data returned by keycloak in the navigation bar, including the "code" field for code flow... If the user refreshs the page, as they often do if there is a problem, authenticateUser will be called again with the stale code and this will be posted to the identity provider. The identity provider detects this as a code "reuse" and probably an attack. This is bad An idea would be in authenticateUser we do a redirect to the guacamole root instead of returning authenticateUser. Though this currently breaks things. The only other choice I see is the use of code like (function guacOpenIDTransformToken() {
if (window.history && history.replaceState)
history.replaceState(null, "", window.location.pathname + window.location.hash);
if (/^#(?![?\/])(.*&)?id_token=/.test(location.hash))
location.hash = '/?' + location.hash.substring(1);
})();client side in transformToken.js to hide the non URL fragment query parameters returned by the identity provider from the user.. Still needs testing, so I'll submit another patch soon |
Forget it. I'm an idiot.. The existing js code converts the query parameters into a URL fragment so they won't be posted to the server on a refresh.. The code as it stands is ok. |
adb014
left a comment
adb014
left a comment
There was a problem hiding this comment.
My comments were in a self review for some reason !
|
@necouchman anticipating that you’ll want a patch to the manual, I started think what is needed but ran into a question. At the moment the configuration is split into required and optional configuration options. However, with the proposed changes only openid-client-id and openid-redirect-uri remain required. The other existing « required » options can either remain required or be replaced with openid-well-known-endpoint. To me the split required/optional make less sense now. What I propose is a discussion implicit flow versus code flow and another section on the well-known endpoint and the fact it can replace up to 4 (3 with implicit flow) other required configuration options. I then propose to combine the existing required and optional sections into a single section. How do you want this addressed ? |
I went ahead and implemented a possible change to the manual in a seperate pull request |
There was a problem hiding this comment.
Thanks for continuing to work on this @adb014. I've got the next round of requested changes - several more style issues that need to be addressed to make sure that the style conforms to the generally-accepted and used style throughout the code, along with some exception-handling issues and a couple of other minor things.
Also, your commit messages need some tweaks:
- Make sure they are descriptive of the changes.
- Make sure they are tagged with the
GUACAMOLE-2258:prefix.
| * Utility class to open a http connection to a URL, send a body | ||
| * and receive a response in the form of a parsed JSON | ||
| */ | ||
| public final class JsonUrlReader { |
There was a problem hiding this comment.
I think there are multiple things to think about:
- Risk is certainly one of them, though it's a bit of a trade. You either write your own functions and risk that you've done something bad and it will have to be corrected, or you rely on someone else. There's risk either way.
- Maintainability is more what I had in mind - if someone else already has a class and set of functions for reading JSON from a URL, and they're actively maintaining it, it allows the Guacamole project to use that and not have to maintain a set of classes for reading JSON URLs, which is not the goal of this project. It's why we aren't writing our own JDBC drivers, SAML client, OpenID client, LDAP client, etc.
To your point, as long as the Spring OAuth2 code is licensed in a way that allows us to leverage it, I have no problem with considering using a different OAuth2 client or some code from that. I don't think there's anything specific that ties Guacamole to the currently-implemented OpenID client.
Aren't all the commits warpped up into a single commit against the guacamole-client repo when the PR is accepted with a seperate description ? I've just given my commits a bit of description so that I can personally find the right commit if needed. If the commit messages are all imported in to guacamole, yes I understand that they'll need to be more descriptive and tagged |
No - a merge commit is added, but each of your commits will still be there, and thus they need to be descriptive, not just to you, but to the entire community. |
Do you want me to recommit them against a clean guacamole-client tree with a single more descriptive commit message and the JIRA ticket in a new pull-request ? |
adb014
force-pushed
the
main
branch
2 times, most recently
from
0e2a0bd to
a434b1d
Compare
@necouchman Ok I reworked all of the commit messages and force pushed them to my repo |
|
Looking at the CI build failure there is an error in the build of guacamole-common-js. I rebased against the latest main branch and tested again and it build here. Can you relaunch the workflow, as this looks like the build problem is not with my code ? |
|
From what I understand in my other PR this should be rebased against staging/1.6.1. However, I didn't create a personal branch for these change and so as far as I can see any forced push is going to mess up this PR... @necouchman can to resolve any outsatnding issues you have with this PR, I'll correct for the requested changes and after rebase against staging/1.6.1 and generate a new PR |
I believe this should stay based against the |
After reading, https://cwiki.apache.org/confluence/display/GUAC/Version+Numbering+and+Branching+Scheme, and particularly the phrase and this PR definitely does require major documentation updates.. Need to re-rebase #1216 as well ;-) |
2 participants
The existing OpenID extension only supports OpenID implicit flow. Knowing that this method is deprecated in OAuth2 due to its poor security, Guacamole really needs to add code flow authorization to its OpenID extension.
This pull request add code flow, but also allows for pkce challenges to be passed to the authorization endpoint and pkce verifiers and client secrets to be passed to the token endpoint. This options for this are
Allowing the existing implicit flow code to be used by default without changes to existing configurations.
One problem I had is that the getLoginURI method didn't seem to allow me to save the PKCE verifier and in any case I didn't want the code returned by the identity provider to appear in the browser navigation bar. So I created a local REST redirect api to talk to the identity provider and receive the response. If you know how to use the AuthenticationSessionManager within getLoginURI and hide the returned code in the navigation bar this might be done differently.
There is a draft JAR built against guacamole 1.6.0 on my fork of guacamole-client