GUACAMOLE-2258 : OpenID code flow, client secret and PKCE

[adb014]

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

openid-flow-type: implicit | code
openid-authorization-endpoint: https://identity-provider/auth
openid-token-endpoint:  https://identity-provider/token
openid-jwks-endpoint: https://identity-provider/jwks
openid-issuer: identity-provider
openid-client-id: my-client-id
openid-client-secret: my-client-secret | null
openid-pkce-required: true | false
openid-redirect-uri: https://example.net/guacamole

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

[adb014]

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

[adb014]

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

openid-response-type: id_token | token | code
openid-well-known-endpoint: https://identity-provider/well-known
openid-client-id: my-client-id
openid-client-secret: my-client-secret | null
openid-pkce-required: true | false
openid-redirect-uri: https://example.net/guacamole

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]

@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.

[adb014]

I’m on my phone at the moment, so will look at this later. Though I tested with 7 scenarios

• id_token + well-known
• id_token + manual endpoint and issuer
• token + well-known
• token + manual endpoint and issuer
• code + well-known
• code + manual endpoint and issuer
• code + manual endpoint and issuer + pkce

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

[adb014]

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

[adb014]

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

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]

My comments were in a self review for some reason !

[adb014]

On my machine using "GET" here doesn't crash here with an IllegalArgumentException though the documentation makes me think it should. Should I protect this in a if/else block ? Or is

.method(method, (method == "GET" ? HttpRequest.BodyPublishers.NoBody() :  HttpRequest.BodyPublishers.ofString(body == null ? "" : body, StandardCharsets.UTF_8)))

just too ugly ?

[adb014]

@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 ?