You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by "necouchman (via GitHub)" <gi...@apache.org> on 2026/05/02 14:01:45 UTC
Re: [PR] GUACAMOLE-2258 : OpenID code flow, client secret and PKCE [guacamole-client]
necouchman commented on code in PR #1198:
URL: https://github.com/apache/guacamole-client/pull/1198#discussion_r3176617467
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-openid/src/main/java/org/apache/guacamole/auth/openid/conf/ConfigurationService.java:
##########
@@ -260,11 +339,16 @@ public class ConfigurationService {
* guacamole.properties.
*
* @throws GuacamoleException
- * If guacamole.properties cannot be parsed, or if the authorization
- * endpoint property is missing.
+ * If guacamole.properties cannot be parsed, or it authorization and
+ * well-known endpoints are property is missing.
Review Comment:
There are a couple of typos, here - I think the original `or if the` language was correct, and the change should just be to change the second line to `and well-known endpoint properties are missing.`
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-openid/src/main/java/org/apache/guacamole/auth/openid/conf/ConfigurationService.java:
##########
@@ -260,11 +339,16 @@ public class ConfigurationService {
* guacamole.properties.
*
* @throws GuacamoleException
- * If guacamole.properties cannot be parsed, or if the authorization
- * endpoint property is missing.
+ * If guacamole.properties cannot be parsed, or it authorization and
+ * well-known endpoints are property is missing.
*/
public URI getAuthorizationEndpoint() throws GuacamoleException {
- return environment.getRequiredProperty(OPENID_AUTHORIZATION_ENDPOINT);
+ URI authorization_endpoint = environment.getProperty(OPENID_AUTHORIZATION_ENDPOINT);
+ authorization_endpoint = authorization_endpoint == null ? confWellKnown.getAuthorizationEndpoint() : authorization_endpoint;
Review Comment:
This is a nice succinct line of code, but I don't know that it makes for terribly readable code. Also, there is a form of the `environment.getProperty()` call that lets you specify a default value, so you could really just combine this line with the above:
```
URI authorization_endpoint = environment.getProperty(OPENID_AUTHORIZATION_ENDPOINT, confWellKnown.getAuthorizationEndpoint());
```
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-openid/src/main/java/org/apache/guacamole/auth/openid/conf/ConfigurationService.java:
##########
@@ -330,11 +419,117 @@ public String getIssuer() throws GuacamoleException {
* guacamole.properties.
*
* @throws GuacamoleException
- * If guacamole.properties cannot be parsed, or if the JWKS endpoint
- * property is missing.
+ * If guacamole.properties cannot be parsed, or if the JWKS and
+ * well-known endpoints are property is missing.
*/
public URI getJWKSEndpoint() throws GuacamoleException {
- return environment.getRequiredProperty(OPENID_JWKS_ENDPOINT);
+ URI jwks_uri = environment.getProperty(OPENID_JWKS_ENDPOINT);
+ jwks_uri = jwks_uri == null ? confWellKnown.getJWKSEndpoint() : jwks_uri;
+ if (jwks_uri == null) {
+ throw new GuacamoleException("Property openid-jwks-endpoint or openid-well-known-endpoint is required");
+ }
+ return jwks_uri;
+ }
+
+ /**
+ * Returns the token endpoint (URI) of the OIDC service as
+ * configured with guacamole.properties.
+ *
+ * @return
+ * The token endpoint of the OIDC service, as configured with
+ * guacamole.properties.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed, or if the token and
+ * well-known endpoints are property is missing.
Review Comment:
-> `well-known endpoint properties are missing.`
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-openid/src/main/java/org/apache/guacamole/auth/openid/conf/ConfigurationService.java:
##########
@@ -330,11 +419,117 @@ public String getIssuer() throws GuacamoleException {
* guacamole.properties.
*
* @throws GuacamoleException
- * If guacamole.properties cannot be parsed, or if the JWKS endpoint
- * property is missing.
+ * If guacamole.properties cannot be parsed, or if the JWKS and
+ * well-known endpoints are property is missing.
*/
public URI getJWKSEndpoint() throws GuacamoleException {
- return environment.getRequiredProperty(OPENID_JWKS_ENDPOINT);
+ URI jwks_uri = environment.getProperty(OPENID_JWKS_ENDPOINT);
+ jwks_uri = jwks_uri == null ? confWellKnown.getJWKSEndpoint() : jwks_uri;
+ if (jwks_uri == null) {
+ throw new GuacamoleException("Property openid-jwks-endpoint or openid-well-known-endpoint is required");
+ }
+ return jwks_uri;
+ }
+
+ /**
+ * Returns the token endpoint (URI) of the OIDC service as
+ * configured with guacamole.properties.
+ *
+ * @return
+ * The token endpoint of the OIDC service, as configured with
+ * guacamole.properties.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed, or if the token and
+ * well-known endpoints are property is missing.
+ */
+ public URI getTokenEndpoint() throws GuacamoleException {
+ URI token_endpoint = environment.getProperty(OPENID_TOKEN_ENDPOINT);
+ token_endpoint = token_endpoint == null ? confWellKnown.getTokenEndpoint() : token_endpoint;
+ if (token_endpoint == null) {
+ throw new GuacamoleException("Property openid-token-endpoint or openid-well-known-endpoint is required");
+ }
+ return token_endpoint;
+ }
+
+ /**
+ * Returns the reponse type of the OpenID service as configured with guacamole.properties.
+ *
+ * @return
+ * The reponse type of the OpenID service, as configured with guacamole.properties. Can
+ * be either 'id_token', 'token' or 'code'.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed.
+ */
+ public OpenIDResponseType getResponseType() throws GuacamoleException {
+ return environment.getProperty(OPENID_RESPONSE_TYPE, DEFAULT_RESPONSE_TYPE);
+ }
+
+ /**
+ * Returns true if the response type defines an implict flow
+ *
+ * @return
+ * The whether implicit flow is used or not, as configured with guacamole.properties.
Review Comment:
This should be something more like:
```
true if implicit flow should be used, otherwise false.
```
And the mention of guacamole.properties can be in the main body of the comment above.
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-openid/src/main/java/org/apache/guacamole/auth/openid/token/TokenValidationService.java:
##########
@@ -98,26 +107,36 @@ public JwtClaims validateToken(String token) throws GuacamoleException {
.setExpectedAudience(confService.getClientID())
.setVerificationKeyResolver(resolver)
.build();
-
+
+ /* Exchange code → token */
+ if (! confService.isImplicitFlow()) {
Review Comment:
You can remove the space between the `!` and `confService`.
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-openid/src/main/java/org/apache/guacamole/auth/openid/conf/OpenIDWellKnown.java:
##########
@@ -0,0 +1,227 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.auth.openid.conf;
+
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+import java.io.IOException;
+import java.net.URI;
+import java.net.URL;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import java.util.Map;
+import javax.ws.rs.core.UriBuilder;
+import org.apache.guacamole.auth.openid.util.JsonUrlReader;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.properties.URIGuacamoleProperty;
+import org.jose4j.json.JsonUtil;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Class for retrieving well-known endpoint data.
+ */
+@Singleton
+public class OpenIDWellKnown {
+
+ /**
+ * Logger for this class.
+ */
+ private static final Logger logger = LoggerFactory.getLogger(OpenIDWellKnown.class);
+
+
+ /**
+ * The number of attempts to the well-known endpoint to get the values before giving up
+ */
+ private static final int MAX_ATTEMPTS = 24;
+
+ /**
+ * The delay between each attempt to well-known endpoint in seconds
+ */
+ private static final long DELAY_SECONDS = 5;
+
+ /**
+ * The detected issuer
+ */
+ private static String issuer = null;
+
+ /**
+ * The detected authorization endpoint
+ */
+ private static URI authorization_endpoint = null;
+
+ /**
+ * The detected token endpoint
+ */
+ private static URI token_endpoint = null;
+
+ /**
+ * The detected jwks_uri
+ */
+ private static URI jwks_uri = null;
+
+ /**
+ * Empty constructor of the class to populate data recovered from a OIDC
+ * well-known URL. The class will be populated on injection by Guice
+ */
+ public OpenIDWellKnown() { }
+
+ /**
+ * The well-known endpoint (URI) of the OIDC service.
+ */
+ private static final URIGuacamoleProperty OPENID_WELL_KNOWN_ENDPOINT =
+ new URIGuacamoleProperty() {
+
+ @Override
+ public String getName() { return "openid-well-known-endpoint"; }
+ };
+
+ /**
+ * Returns the well-known endpoint (URI) of the OIDC service as
+ * configured with guacamole.properties.
+ *
+ * @return
+ * The well-known endpoint of the OIDC service, as configured with
+ * guacamole.properties.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed, or if the well-known
+ * endpoint property is missing.
+ */
+ private URI getWellKnownEndpoint() throws GuacamoleException {
+ return environment.getProperty(OPENID_WELL_KNOWN_ENDPOINT);
+ }
+
+ /**
+ * Returns the issuer to expect for all received ID tokens, as configured
+ * from the well_known endpoint.
+ *
+ * @return
+ * The issuer to expect for all received ID tokens, as returned by the
+ * well-known endpoint.
+ */
+ public String getIssuer() {
+ return issuer;
+ }
+
+ /**
+ * Returns the authorization endpoint (URI) of the OpenID service as
+ * configured from the well_known endpoint.
+ *
+ * @return
+ * The authorization endpoint of the OpenID service, as returned by the
+ * well-known endpoint.
+ */
+ public URI getAuthorizationEndpoint() {
+ return authorization_endpoint;
+ }
+
+ /**
+ * Returns the token endpoint (URI) of the OpenID service as
+ * configured from the well_known endpoint.
+ *
+ * @return
+ * The token endpoint of the OpenID service, as returned by the
+ * well-known endpoint.
+ */
+ public URI getTokenEndpoint() {
+ return token_endpoint;
+ }
+
+ /**
+ * Returns the endpoint (URI) of the JWKS service which defines how
+ * received ID tokens (JWTs) shall be validated, as configured from
+ * the well-known endpoint.
+ *
+ * @return
+ * The endpoint (URI) of the JWKS service which defines how received ID
+ * tokens (JWTs) shall be validated, as configured from the
+ * well-known endpoint.
+ */
+ public URI getJWKSEndpoint() {
+ return jwks_uri;
+ }
+
+ /**
+ * The Guacamole server environment.
+ */
+ @Inject
+ private Environment environment;
+
+ /*
Review Comment:
This is missing the extra `*` that defines this as a JSDoc-style comment block.
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-openid/src/main/java/org/apache/guacamole/auth/openid/conf/ConfigurationService.java:
##########
@@ -313,10 +397,15 @@ public URI getRedirectURI() throws GuacamoleException {
*
* @throws GuacamoleException
* If guacamole.properties cannot be parsed, or if the issuer property
- * is missing.
+ * and well-known endpoints are property is missing.
*/
public String getIssuer() throws GuacamoleException {
- return environment.getRequiredProperty(OPENID_ISSUER);
+ String issuer = environment.getProperty(OPENID_ISSUER);
+ issuer = issuer == null ? confWellKnown.getIssuer() : issuer;
Review Comment:
Similar to the above in `getAuthorizationEndpoint()`, I'd suggest this line use the `getProperty()` call that uses `confWellKnown.getIssuer()` as the default value.
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-openid/src/main/java/org/apache/guacamole/auth/openid/conf/ConfigurationService.java:
##########
@@ -330,11 +419,117 @@ public String getIssuer() throws GuacamoleException {
* guacamole.properties.
*
* @throws GuacamoleException
- * If guacamole.properties cannot be parsed, or if the JWKS endpoint
- * property is missing.
+ * If guacamole.properties cannot be parsed, or if the JWKS and
+ * well-known endpoints are property is missing.
*/
public URI getJWKSEndpoint() throws GuacamoleException {
- return environment.getRequiredProperty(OPENID_JWKS_ENDPOINT);
+ URI jwks_uri = environment.getProperty(OPENID_JWKS_ENDPOINT);
+ jwks_uri = jwks_uri == null ? confWellKnown.getJWKSEndpoint() : jwks_uri;
Review Comment:
This can also be changed to make the well-known endpoint call the default value.
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-openid/src/main/java/org/apache/guacamole/auth/openid/conf/ConfigurationService.java:
##########
@@ -313,10 +397,15 @@ public URI getRedirectURI() throws GuacamoleException {
*
* @throws GuacamoleException
* If guacamole.properties cannot be parsed, or if the issuer property
- * is missing.
+ * and well-known endpoints are property is missing.
Review Comment:
-> `and well-known endpoint properties are missing.`
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-openid/src/main/java/org/apache/guacamole/auth/openid/conf/ConfigurationService.java:
##########
@@ -330,11 +419,117 @@ public String getIssuer() throws GuacamoleException {
* guacamole.properties.
*
* @throws GuacamoleException
- * If guacamole.properties cannot be parsed, or if the JWKS endpoint
- * property is missing.
+ * If guacamole.properties cannot be parsed, or if the JWKS and
+ * well-known endpoints are property is missing.
*/
public URI getJWKSEndpoint() throws GuacamoleException {
- return environment.getRequiredProperty(OPENID_JWKS_ENDPOINT);
+ URI jwks_uri = environment.getProperty(OPENID_JWKS_ENDPOINT);
+ jwks_uri = jwks_uri == null ? confWellKnown.getJWKSEndpoint() : jwks_uri;
+ if (jwks_uri == null) {
+ throw new GuacamoleException("Property openid-jwks-endpoint or openid-well-known-endpoint is required");
+ }
+ return jwks_uri;
+ }
+
+ /**
+ * Returns the token endpoint (URI) of the OIDC service as
+ * configured with guacamole.properties.
+ *
+ * @return
+ * The token endpoint of the OIDC service, as configured with
+ * guacamole.properties.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed, or if the token and
+ * well-known endpoints are property is missing.
+ */
+ public URI getTokenEndpoint() throws GuacamoleException {
+ URI token_endpoint = environment.getProperty(OPENID_TOKEN_ENDPOINT);
+ token_endpoint = token_endpoint == null ? confWellKnown.getTokenEndpoint() : token_endpoint;
Review Comment:
Also can use default value of `getProperty()`, here.
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-openid/src/main/java/org/apache/guacamole/auth/openid/conf/ConfigurationService.java:
##########
@@ -330,11 +419,117 @@ public String getIssuer() throws GuacamoleException {
* guacamole.properties.
*
* @throws GuacamoleException
- * If guacamole.properties cannot be parsed, or if the JWKS endpoint
- * property is missing.
+ * If guacamole.properties cannot be parsed, or if the JWKS and
+ * well-known endpoints are property is missing.
*/
public URI getJWKSEndpoint() throws GuacamoleException {
- return environment.getRequiredProperty(OPENID_JWKS_ENDPOINT);
+ URI jwks_uri = environment.getProperty(OPENID_JWKS_ENDPOINT);
+ jwks_uri = jwks_uri == null ? confWellKnown.getJWKSEndpoint() : jwks_uri;
+ if (jwks_uri == null) {
+ throw new GuacamoleException("Property openid-jwks-endpoint or openid-well-known-endpoint is required");
+ }
+ return jwks_uri;
+ }
+
+ /**
+ * Returns the token endpoint (URI) of the OIDC service as
+ * configured with guacamole.properties.
+ *
+ * @return
+ * The token endpoint of the OIDC service, as configured with
+ * guacamole.properties.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed, or if the token and
+ * well-known endpoints are property is missing.
+ */
+ public URI getTokenEndpoint() throws GuacamoleException {
+ URI token_endpoint = environment.getProperty(OPENID_TOKEN_ENDPOINT);
+ token_endpoint = token_endpoint == null ? confWellKnown.getTokenEndpoint() : token_endpoint;
+ if (token_endpoint == null) {
+ throw new GuacamoleException("Property openid-token-endpoint or openid-well-known-endpoint is required");
+ }
+ return token_endpoint;
+ }
+
+ /**
+ * Returns the reponse type of the OpenID service as configured with guacamole.properties.
+ *
+ * @return
+ * The reponse type of the OpenID service, as configured with guacamole.properties. Can
+ * be either 'id_token', 'token' or 'code'.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed.
+ */
+ public OpenIDResponseType getResponseType() throws GuacamoleException {
+ return environment.getProperty(OPENID_RESPONSE_TYPE, DEFAULT_RESPONSE_TYPE);
+ }
+
+ /**
+ * Returns true if the response type defines an implict flow
+ *
+ * @return
+ * The whether implicit flow is used or not, as configured with guacamole.properties.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed.
+ */
+ public boolean isImplicitFlow() throws GuacamoleException {
+ OpenIDResponseType response_type = environment.getProperty(OPENID_RESPONSE_TYPE, DEFAULT_RESPONSE_TYPE);
+ return response_type != OpenIDResponseType.CODE;
+ }
+
+ /**
+ * Returns the OIDC client secret used for toen validation, as configured
Review Comment:
`toen` -> `token`
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-openid/src/main/java/org/apache/guacamole/auth/openid/conf/ConfigurationService.java:
##########
@@ -330,11 +419,117 @@ public String getIssuer() throws GuacamoleException {
* guacamole.properties.
*
* @throws GuacamoleException
- * If guacamole.properties cannot be parsed, or if the JWKS endpoint
- * property is missing.
+ * If guacamole.properties cannot be parsed, or if the JWKS and
+ * well-known endpoints are property is missing.
*/
public URI getJWKSEndpoint() throws GuacamoleException {
- return environment.getRequiredProperty(OPENID_JWKS_ENDPOINT);
+ URI jwks_uri = environment.getProperty(OPENID_JWKS_ENDPOINT);
+ jwks_uri = jwks_uri == null ? confWellKnown.getJWKSEndpoint() : jwks_uri;
+ if (jwks_uri == null) {
+ throw new GuacamoleException("Property openid-jwks-endpoint or openid-well-known-endpoint is required");
+ }
+ return jwks_uri;
+ }
+
+ /**
+ * Returns the token endpoint (URI) of the OIDC service as
+ * configured with guacamole.properties.
+ *
+ * @return
+ * The token endpoint of the OIDC service, as configured with
+ * guacamole.properties.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed, or if the token and
+ * well-known endpoints are property is missing.
+ */
+ public URI getTokenEndpoint() throws GuacamoleException {
+ URI token_endpoint = environment.getProperty(OPENID_TOKEN_ENDPOINT);
+ token_endpoint = token_endpoint == null ? confWellKnown.getTokenEndpoint() : token_endpoint;
+ if (token_endpoint == null) {
+ throw new GuacamoleException("Property openid-token-endpoint or openid-well-known-endpoint is required");
+ }
+ return token_endpoint;
+ }
+
+ /**
+ * Returns the reponse type of the OpenID service as configured with guacamole.properties.
+ *
+ * @return
+ * The reponse type of the OpenID service, as configured with guacamole.properties. Can
+ * be either 'id_token', 'token' or 'code'.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed.
+ */
+ public OpenIDResponseType getResponseType() throws GuacamoleException {
+ return environment.getProperty(OPENID_RESPONSE_TYPE, DEFAULT_RESPONSE_TYPE);
+ }
+
+ /**
+ * Returns true if the response type defines an implict flow
+ *
+ * @return
+ * The whether implicit flow is used or not, as configured with guacamole.properties.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed.
+ */
+ public boolean isImplicitFlow() throws GuacamoleException {
+ OpenIDResponseType response_type = environment.getProperty(OPENID_RESPONSE_TYPE, DEFAULT_RESPONSE_TYPE);
+ return response_type != OpenIDResponseType.CODE;
+ }
+
+ /**
+ * Returns the OIDC client secret used for toen validation, as configured
+ * with guacamole.properties. This value is typically provided by the OIDC
+ * service when OIDC credentials are generated for your application, and
+ * may be null.
+ *
+ * @return
+ * The client secret to use when communicating with the OIDC service,
+ * as configured with guacamole.properties.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed.
+ */
+ public String getClientSecret() throws GuacamoleException {
+ return environment.getProperty(OPENID_CLIENT_SECRET);
+ }
+
+ /**
+ * Returns a boolean value of whether "Proof Key for Code Exchange (PKCE)"
Review Comment:
There's an extra space between `Returns` and `a`, here.
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-openid/src/main/java/org/apache/guacamole/auth/openid/conf/ConfigurationService.java:
##########
@@ -330,11 +419,117 @@ public String getIssuer() throws GuacamoleException {
* guacamole.properties.
*
* @throws GuacamoleException
- * If guacamole.properties cannot be parsed, or if the JWKS endpoint
- * property is missing.
+ * If guacamole.properties cannot be parsed, or if the JWKS and
+ * well-known endpoints are property is missing.
Review Comment:
-> `and well-known endpoint properties are missing.`
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-openid/src/main/java/org/apache/guacamole/auth/openid/conf/OpenIDWellKnown.java:
##########
@@ -0,0 +1,227 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.auth.openid.conf;
+
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+import java.io.IOException;
+import java.net.URI;
+import java.net.URL;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import java.util.Map;
+import javax.ws.rs.core.UriBuilder;
+import org.apache.guacamole.auth.openid.util.JsonUrlReader;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.properties.URIGuacamoleProperty;
+import org.jose4j.json.JsonUtil;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Class for retrieving well-known endpoint data.
+ */
+@Singleton
+public class OpenIDWellKnown {
+
+ /**
+ * Logger for this class.
+ */
+ private static final Logger logger = LoggerFactory.getLogger(OpenIDWellKnown.class);
+
+
+ /**
+ * The number of attempts to the well-known endpoint to get the values before giving up
+ */
+ private static final int MAX_ATTEMPTS = 24;
+
+ /**
+ * The delay between each attempt to well-known endpoint in seconds
+ */
+ private static final long DELAY_SECONDS = 5;
+
+ /**
+ * The detected issuer
+ */
+ private static String issuer = null;
+
+ /**
+ * The detected authorization endpoint
+ */
+ private static URI authorization_endpoint = null;
+
+ /**
+ * The detected token endpoint
+ */
+ private static URI token_endpoint = null;
+
+ /**
+ * The detected jwks_uri
+ */
+ private static URI jwks_uri = null;
+
+ /**
+ * Empty constructor of the class to populate data recovered from a OIDC
+ * well-known URL. The class will be populated on injection by Guice
+ */
+ public OpenIDWellKnown() { }
+
+ /**
+ * The well-known endpoint (URI) of the OIDC service.
+ */
+ private static final URIGuacamoleProperty OPENID_WELL_KNOWN_ENDPOINT =
+ new URIGuacamoleProperty() {
+
+ @Override
+ public String getName() { return "openid-well-known-endpoint"; }
+ };
+
+ /**
+ * Returns the well-known endpoint (URI) of the OIDC service as
+ * configured with guacamole.properties.
+ *
+ * @return
+ * The well-known endpoint of the OIDC service, as configured with
+ * guacamole.properties.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed, or if the well-known
+ * endpoint property is missing.
+ */
+ private URI getWellKnownEndpoint() throws GuacamoleException {
+ return environment.getProperty(OPENID_WELL_KNOWN_ENDPOINT);
Review Comment:
Note that this call to `getProperty()` will *not* throw a `GuacamoleException` if the `OPENID_WELL_KNOWN_ENDPOINT` property is missing. It will return null. If you want the behavior you've described in the `@throws` block of the method documentation, you'll need to use `getRequiredProperty()`, instead.
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-openid/src/main/java/org/apache/guacamole/auth/openid/conf/ConfigurationService.java:
##########
@@ -330,11 +419,117 @@ public String getIssuer() throws GuacamoleException {
* guacamole.properties.
*
* @throws GuacamoleException
- * If guacamole.properties cannot be parsed, or if the JWKS endpoint
- * property is missing.
+ * If guacamole.properties cannot be parsed, or if the JWKS and
+ * well-known endpoints are property is missing.
*/
public URI getJWKSEndpoint() throws GuacamoleException {
- return environment.getRequiredProperty(OPENID_JWKS_ENDPOINT);
+ URI jwks_uri = environment.getProperty(OPENID_JWKS_ENDPOINT);
+ jwks_uri = jwks_uri == null ? confWellKnown.getJWKSEndpoint() : jwks_uri;
+ if (jwks_uri == null) {
+ throw new GuacamoleException("Property openid-jwks-endpoint or openid-well-known-endpoint is required");
+ }
+ return jwks_uri;
+ }
+
+ /**
+ * Returns the token endpoint (URI) of the OIDC service as
+ * configured with guacamole.properties.
+ *
+ * @return
+ * The token endpoint of the OIDC service, as configured with
+ * guacamole.properties.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed, or if the token and
+ * well-known endpoints are property is missing.
+ */
+ public URI getTokenEndpoint() throws GuacamoleException {
+ URI token_endpoint = environment.getProperty(OPENID_TOKEN_ENDPOINT);
+ token_endpoint = token_endpoint == null ? confWellKnown.getTokenEndpoint() : token_endpoint;
+ if (token_endpoint == null) {
+ throw new GuacamoleException("Property openid-token-endpoint or openid-well-known-endpoint is required");
+ }
+ return token_endpoint;
+ }
+
+ /**
+ * Returns the reponse type of the OpenID service as configured with guacamole.properties.
+ *
+ * @return
+ * The reponse type of the OpenID service, as configured with guacamole.properties. Can
+ * be either 'id_token', 'token' or 'code'.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed.
+ */
+ public OpenIDResponseType getResponseType() throws GuacamoleException {
+ return environment.getProperty(OPENID_RESPONSE_TYPE, DEFAULT_RESPONSE_TYPE);
+ }
+
+ /**
+ * Returns true if the response type defines an implict flow
Review Comment:
...otherwise false, as configured in guacamole.properties.
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-openid/src/main/java/org/apache/guacamole/auth/openid/conf/ConfigurationService.java:
##########
@@ -330,11 +419,117 @@ public String getIssuer() throws GuacamoleException {
* guacamole.properties.
*
* @throws GuacamoleException
- * If guacamole.properties cannot be parsed, or if the JWKS endpoint
- * property is missing.
+ * If guacamole.properties cannot be parsed, or if the JWKS and
+ * well-known endpoints are property is missing.
*/
public URI getJWKSEndpoint() throws GuacamoleException {
- return environment.getRequiredProperty(OPENID_JWKS_ENDPOINT);
+ URI jwks_uri = environment.getProperty(OPENID_JWKS_ENDPOINT);
+ jwks_uri = jwks_uri == null ? confWellKnown.getJWKSEndpoint() : jwks_uri;
+ if (jwks_uri == null) {
+ throw new GuacamoleException("Property openid-jwks-endpoint or openid-well-known-endpoint is required");
+ }
+ return jwks_uri;
+ }
+
+ /**
+ * Returns the token endpoint (URI) of the OIDC service as
+ * configured with guacamole.properties.
+ *
+ * @return
+ * The token endpoint of the OIDC service, as configured with
+ * guacamole.properties.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed, or if the token and
+ * well-known endpoints are property is missing.
+ */
+ public URI getTokenEndpoint() throws GuacamoleException {
+ URI token_endpoint = environment.getProperty(OPENID_TOKEN_ENDPOINT);
+ token_endpoint = token_endpoint == null ? confWellKnown.getTokenEndpoint() : token_endpoint;
+ if (token_endpoint == null) {
+ throw new GuacamoleException("Property openid-token-endpoint or openid-well-known-endpoint is required");
+ }
+ return token_endpoint;
+ }
+
+ /**
+ * Returns the reponse type of the OpenID service as configured with guacamole.properties.
+ *
+ * @return
+ * The reponse type of the OpenID service, as configured with guacamole.properties. Can
+ * be either 'id_token', 'token' or 'code'.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed.
+ */
+ public OpenIDResponseType getResponseType() throws GuacamoleException {
+ return environment.getProperty(OPENID_RESPONSE_TYPE, DEFAULT_RESPONSE_TYPE);
+ }
+
+ /**
+ * Returns true if the response type defines an implict flow
+ *
+ * @return
+ * The whether implicit flow is used or not, as configured with guacamole.properties.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed.
+ */
+ public boolean isImplicitFlow() throws GuacamoleException {
+ OpenIDResponseType response_type = environment.getProperty(OPENID_RESPONSE_TYPE, DEFAULT_RESPONSE_TYPE);
+ return response_type != OpenIDResponseType.CODE;
+ }
+
+ /**
+ * Returns the OIDC client secret used for toen validation, as configured
+ * with guacamole.properties. This value is typically provided by the OIDC
+ * service when OIDC credentials are generated for your application, and
+ * may be null.
+ *
+ * @return
+ * The client secret to use when communicating with the OIDC service,
+ * as configured with guacamole.properties.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed.
+ */
+ public String getClientSecret() throws GuacamoleException {
+ return environment.getProperty(OPENID_CLIENT_SECRET);
+ }
+
+ /**
+ * Returns a boolean value of whether "Proof Key for Code Exchange (PKCE)"
+ * will be used, as configured with guacamole.properties. The choice of
+ * whether to use PKCE is up to you, but the OIDC service must support
+ * it. By default will be false.
+ *
+ * @return
+ * The whether to use PKCE or not, as configured with guacamole.properties.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed.
+ */
+ public boolean isPKCERequired() throws GuacamoleException {
+ Boolean enabled = environment.getProperty(OPENID_PKCE_REQUIRED);
+ return enabled != null && enabled;
Review Comment:
Two things:
* I don't think there's any reason to use a `Boolean` here when you're just returning a `boolean`.
* Again, make use of the default value option - if false is the default, this can be simplified to:
```
return environment.getProperty(OPENID_PKCE_REQUIRED, false);
```
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-openid/src/main/java/org/apache/guacamole/auth/openid/conf/ConfigurationService.java:
##########
@@ -330,11 +419,117 @@ public String getIssuer() throws GuacamoleException {
* guacamole.properties.
*
* @throws GuacamoleException
- * If guacamole.properties cannot be parsed, or if the JWKS endpoint
- * property is missing.
+ * If guacamole.properties cannot be parsed, or if the JWKS and
+ * well-known endpoints are property is missing.
*/
public URI getJWKSEndpoint() throws GuacamoleException {
- return environment.getRequiredProperty(OPENID_JWKS_ENDPOINT);
+ URI jwks_uri = environment.getProperty(OPENID_JWKS_ENDPOINT);
+ jwks_uri = jwks_uri == null ? confWellKnown.getJWKSEndpoint() : jwks_uri;
+ if (jwks_uri == null) {
+ throw new GuacamoleException("Property openid-jwks-endpoint or openid-well-known-endpoint is required");
+ }
+ return jwks_uri;
+ }
+
+ /**
+ * Returns the token endpoint (URI) of the OIDC service as
+ * configured with guacamole.properties.
+ *
+ * @return
+ * The token endpoint of the OIDC service, as configured with
+ * guacamole.properties.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed, or if the token and
+ * well-known endpoints are property is missing.
+ */
+ public URI getTokenEndpoint() throws GuacamoleException {
+ URI token_endpoint = environment.getProperty(OPENID_TOKEN_ENDPOINT);
+ token_endpoint = token_endpoint == null ? confWellKnown.getTokenEndpoint() : token_endpoint;
+ if (token_endpoint == null) {
+ throw new GuacamoleException("Property openid-token-endpoint or openid-well-known-endpoint is required");
+ }
+ return token_endpoint;
+ }
+
+ /**
+ * Returns the reponse type of the OpenID service as configured with guacamole.properties.
+ *
+ * @return
+ * The reponse type of the OpenID service, as configured with guacamole.properties. Can
+ * be either 'id_token', 'token' or 'code'.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed.
+ */
+ public OpenIDResponseType getResponseType() throws GuacamoleException {
+ return environment.getProperty(OPENID_RESPONSE_TYPE, DEFAULT_RESPONSE_TYPE);
+ }
+
+ /**
+ * Returns true if the response type defines an implict flow
+ *
+ * @return
+ * The whether implicit flow is used or not, as configured with guacamole.properties.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed.
+ */
+ public boolean isImplicitFlow() throws GuacamoleException {
+ OpenIDResponseType response_type = environment.getProperty(OPENID_RESPONSE_TYPE, DEFAULT_RESPONSE_TYPE);
+ return response_type != OpenIDResponseType.CODE;
+ }
+
+ /**
+ * Returns the OIDC client secret used for toen validation, as configured
+ * with guacamole.properties. This value is typically provided by the OIDC
+ * service when OIDC credentials are generated for your application, and
+ * may be null.
+ *
+ * @return
+ * The client secret to use when communicating with the OIDC service,
+ * as configured with guacamole.properties.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed.
+ */
+ public String getClientSecret() throws GuacamoleException {
+ return environment.getProperty(OPENID_CLIENT_SECRET);
+ }
+
+ /**
+ * Returns a boolean value of whether "Proof Key for Code Exchange (PKCE)"
+ * will be used, as configured with guacamole.properties. The choice of
+ * whether to use PKCE is up to you, but the OIDC service must support
+ * it. By default will be false.
+ *
+ * @return
+ * The whether to use PKCE or not, as configured with guacamole.properties.
Review Comment:
Similar to above, this should be something more like:
```
true if PKCE should be used, otherwise false.
```
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-openid/src/main/java/org/apache/guacamole/auth/openid/conf/OpenIDResponseType.java:
##########
@@ -0,0 +1,67 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.auth.openid.conf;
+
+import org.apache.guacamole.properties.EnumGuacamoleProperty.PropertyValue;
+
+/**
+ * This enum represents the valid OIDC reponse types that can be used
+ */
+ public enum OpenIDResponseType {
+ /*
+ * Response type "id_token" used for implciit flow as specified in the OpenID standard
+ */
+ @PropertyValue("id_token")
+ ID_TOKEN("id_token"),
+
+ /* Response type "token" used for implicit flow by certain Identity Providers, notably
+ * AWS Cognito. This corresponds to the official OIDC response_type "id_token token"
+ * that returns both the "id_token" and "access_token" parameters
+ */
+ @PropertyValue("token")
+ TOKEN("token"),
+
+ /**
+ * Response type "code" used for code flow authentication
+ */
+ @PropertyValue("code")
+ CODE("code");
+
+ /*
+ * The string value of the response type used
+ */
+ public final String STRING_VALUE;
Review Comment:
The comment blocks in this section each use a slightly different style. The style used elsewhere in the code is as follows:
```
/**
* Documentation of the enum type, variable, etc.
*/
```
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-openid/src/main/java/org/apache/guacamole/auth/openid/conf/OpenIDWellKnown.java:
##########
@@ -0,0 +1,227 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.auth.openid.conf;
+
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+import java.io.IOException;
+import java.net.URI;
+import java.net.URL;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import java.util.Map;
+import javax.ws.rs.core.UriBuilder;
+import org.apache.guacamole.auth.openid.util.JsonUrlReader;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.properties.URIGuacamoleProperty;
+import org.jose4j.json.JsonUtil;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Class for retrieving well-known endpoint data.
+ */
+@Singleton
+public class OpenIDWellKnown {
+
+ /**
+ * Logger for this class.
+ */
+ private static final Logger logger = LoggerFactory.getLogger(OpenIDWellKnown.class);
+
+
+ /**
+ * The number of attempts to the well-known endpoint to get the values before giving up
+ */
+ private static final int MAX_ATTEMPTS = 24;
+
+ /**
+ * The delay between each attempt to well-known endpoint in seconds
+ */
+ private static final long DELAY_SECONDS = 5;
+
+ /**
+ * The detected issuer
+ */
+ private static String issuer = null;
+
+ /**
+ * The detected authorization endpoint
+ */
+ private static URI authorization_endpoint = null;
+
+ /**
+ * The detected token endpoint
+ */
+ private static URI token_endpoint = null;
+
+ /**
+ * The detected jwks_uri
+ */
+ private static URI jwks_uri = null;
Review Comment:
Two issues, here:
* I'm not sure these should be `static`? I know this is a `Singleton` class, but you're still expecting that these are private and would be accessed via the get methods defined below, so I think they should not be static?
* If they're static, the generally-accepted style is `UPPER_CASE_WITH_UNDERSCORES`. If they're not static, the generally-accepted style used elsewhere in the code is `camelCaseWithoutUnderscores`.
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-openid/src/main/java/org/apache/guacamole/auth/openid/token/TokenValidationService.java:
##########
@@ -70,20 +73,26 @@ public class TokenValidationService {
private NonceService nonceService;
/**
- * Validates the given ID token, returning the JwtClaims contained therein.
- * If the ID token is invalid, null is returned.
+ * Validates the given ID token, using implicit flow. Also validates codes,
+ * exchanging them for id_tokens before validation. If the id_token or
+ * code is invalid, null is returned, otherwise the JwtClaims in the
+ * id_token are returned.
*
* @param token
- * The ID token to validate.
+ * The ID token to validate if implicit flow or the code to exchange for
+ * an id_token and then validate.
+ *
+ * @param verifier
+ * A PKCE verifier or null if not used. Only used with code flow
*
* @return
- * The JWT claims contained within the given ID token if it passes tests,
- * or null if the token is not valid.
+ * The JWT claims contained within the id_token if it passes tests,
+ * or null if the id_token is not valid.
Review Comment:
Would it make more sense, here to throw an exception instead of returning `null` if the id_token is invalid? Presumably an invalid id_token constitutes an authentication failure, which should then be passed back to the user, perhaps using `GuacamoleInvalidCredentialsException`? Or is there some reason within the code flow that `null` is preferred for this?
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-openid/src/main/java/org/apache/guacamole/auth/openid/util/PKCEUtil.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.auth.openid.util;
+
+import java.security.MessageDigest;
+import java.security.SecureRandom;
+
+/**
+ * Utility class for generating PKCE parameters.
+ *
+ * Supports:
+ * - code_verifier (random Base64URL)
+ * - code_challenge (S256)
+ */
+public final class PKCEUtil {
+ /**
+ * Get the verifier data from a cryptographically secure random source
+ */
+ private static final SecureRandom RANDOM = new SecureRandom();
+
+ /*
+ * Class to create PKCE challenges and verifiers. This class should not be instantiated
+ */
+ private PKCEUtil() {}
+
+ /**
+ * Generates a high-entropy PKCE code_verifier.
+ *
+ * @return
+ * A 256bit or 64 byte random Base64 URL encode string
+ */
+ public static String generateCodeVerifier() {
+ byte[] bytes = new byte[64];
+ RANDOM.nextBytes(bytes);
+ return base64Url(bytes);
+ }
+
+ /**
+ * Computes the PKCE code_challenge = BASE64URL(SHA256(code_verifier)).
+ *
+ * @param String verifier
+ * A string containing the S256 verifier calculated bu generateCodeVerifier
+ *
+ * @return
+ * The generated S256 code challenge used for the PKCE request encoded
+ * in Base64 URL format.
+ */
+ public static String generateCodeChallenge(String verifier) throws Exception {
+ MessageDigest sha = MessageDigest.getInstance("SHA-256");
+ byte[] hash = sha.digest(verifier.getBytes("US-ASCII"));
+ return base64Url(hash);
+ }
+
+ /**
+ * Base64URL encoding without padding.
Review Comment:
This should be a little more descriptive...
```
A method that Base64URL-encodes a provided byte array without padding...
```
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-openid/src/main/java/org/apache/guacamole/auth/openid/conf/OpenIDWellKnown.java:
##########
@@ -0,0 +1,227 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.auth.openid.conf;
+
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+import java.io.IOException;
+import java.net.URI;
+import java.net.URL;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import java.util.Map;
+import javax.ws.rs.core.UriBuilder;
+import org.apache.guacamole.auth.openid.util.JsonUrlReader;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.properties.URIGuacamoleProperty;
+import org.jose4j.json.JsonUtil;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Class for retrieving well-known endpoint data.
+ */
+@Singleton
+public class OpenIDWellKnown {
+
+ /**
+ * Logger for this class.
+ */
+ private static final Logger logger = LoggerFactory.getLogger(OpenIDWellKnown.class);
+
+
+ /**
+ * The number of attempts to the well-known endpoint to get the values before giving up
+ */
+ private static final int MAX_ATTEMPTS = 24;
+
+ /**
+ * The delay between each attempt to well-known endpoint in seconds
+ */
+ private static final long DELAY_SECONDS = 5;
+
+ /**
+ * The detected issuer
+ */
+ private static String issuer = null;
+
+ /**
+ * The detected authorization endpoint
+ */
+ private static URI authorization_endpoint = null;
+
+ /**
+ * The detected token endpoint
+ */
+ private static URI token_endpoint = null;
+
+ /**
+ * The detected jwks_uri
+ */
+ private static URI jwks_uri = null;
+
+ /**
+ * Empty constructor of the class to populate data recovered from a OIDC
+ * well-known URL. The class will be populated on injection by Guice
+ */
+ public OpenIDWellKnown() { }
+
+ /**
+ * The well-known endpoint (URI) of the OIDC service.
+ */
+ private static final URIGuacamoleProperty OPENID_WELL_KNOWN_ENDPOINT =
+ new URIGuacamoleProperty() {
+
+ @Override
+ public String getName() { return "openid-well-known-endpoint"; }
+ };
+
+ /**
+ * Returns the well-known endpoint (URI) of the OIDC service as
+ * configured with guacamole.properties.
+ *
+ * @return
+ * The well-known endpoint of the OIDC service, as configured with
+ * guacamole.properties.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed, or if the well-known
+ * endpoint property is missing.
+ */
+ private URI getWellKnownEndpoint() throws GuacamoleException {
+ return environment.getProperty(OPENID_WELL_KNOWN_ENDPOINT);
+ }
+
+ /**
+ * Returns the issuer to expect for all received ID tokens, as configured
+ * from the well_known endpoint.
+ *
+ * @return
+ * The issuer to expect for all received ID tokens, as returned by the
+ * well-known endpoint.
+ */
+ public String getIssuer() {
+ return issuer;
+ }
+
+ /**
+ * Returns the authorization endpoint (URI) of the OpenID service as
+ * configured from the well_known endpoint.
+ *
+ * @return
+ * The authorization endpoint of the OpenID service, as returned by the
+ * well-known endpoint.
+ */
+ public URI getAuthorizationEndpoint() {
+ return authorization_endpoint;
+ }
+
+ /**
+ * Returns the token endpoint (URI) of the OpenID service as
+ * configured from the well_known endpoint.
+ *
+ * @return
+ * The token endpoint of the OpenID service, as returned by the
+ * well-known endpoint.
+ */
+ public URI getTokenEndpoint() {
+ return token_endpoint;
+ }
+
+ /**
+ * Returns the endpoint (URI) of the JWKS service which defines how
+ * received ID tokens (JWTs) shall be validated, as configured from
+ * the well-known endpoint.
+ *
+ * @return
+ * The endpoint (URI) of the JWKS service which defines how received ID
+ * tokens (JWTs) shall be validated, as configured from the
+ * well-known endpoint.
+ */
+ public URI getJWKSEndpoint() {
+ return jwks_uri;
+ }
+
+ /**
+ * The Guacamole server environment.
+ */
+ @Inject
+ private Environment environment;
Review Comment:
This injected variable should be at the top with the other class variables, before any methods are defined.
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-openid/src/main/java/org/apache/guacamole/auth/openid/conf/OpenIDWellKnown.java:
##########
@@ -0,0 +1,227 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.auth.openid.conf;
+
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+import java.io.IOException;
+import java.net.URI;
+import java.net.URL;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import java.util.Map;
+import javax.ws.rs.core.UriBuilder;
+import org.apache.guacamole.auth.openid.util.JsonUrlReader;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.properties.URIGuacamoleProperty;
+import org.jose4j.json.JsonUtil;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Class for retrieving well-known endpoint data.
+ */
+@Singleton
+public class OpenIDWellKnown {
+
+ /**
+ * Logger for this class.
+ */
+ private static final Logger logger = LoggerFactory.getLogger(OpenIDWellKnown.class);
+
+
+ /**
+ * The number of attempts to the well-known endpoint to get the values before giving up
+ */
+ private static final int MAX_ATTEMPTS = 24;
+
+ /**
+ * The delay between each attempt to well-known endpoint in seconds
+ */
+ private static final long DELAY_SECONDS = 5;
+
+ /**
+ * The detected issuer
+ */
+ private static String issuer = null;
+
+ /**
+ * The detected authorization endpoint
+ */
+ private static URI authorization_endpoint = null;
+
+ /**
+ * The detected token endpoint
+ */
+ private static URI token_endpoint = null;
+
+ /**
+ * The detected jwks_uri
+ */
+ private static URI jwks_uri = null;
+
+ /**
+ * Empty constructor of the class to populate data recovered from a OIDC
+ * well-known URL. The class will be populated on injection by Guice
+ */
+ public OpenIDWellKnown() { }
+
+ /**
+ * The well-known endpoint (URI) of the OIDC service.
+ */
+ private static final URIGuacamoleProperty OPENID_WELL_KNOWN_ENDPOINT =
+ new URIGuacamoleProperty() {
+
+ @Override
+ public String getName() { return "openid-well-known-endpoint"; }
+ };
+
+ /**
+ * Returns the well-known endpoint (URI) of the OIDC service as
+ * configured with guacamole.properties.
+ *
+ * @return
+ * The well-known endpoint of the OIDC service, as configured with
+ * guacamole.properties.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed, or if the well-known
+ * endpoint property is missing.
+ */
+ private URI getWellKnownEndpoint() throws GuacamoleException {
+ return environment.getProperty(OPENID_WELL_KNOWN_ENDPOINT);
+ }
+
+ /**
+ * Returns the issuer to expect for all received ID tokens, as configured
+ * from the well_known endpoint.
+ *
+ * @return
+ * The issuer to expect for all received ID tokens, as returned by the
+ * well-known endpoint.
Review Comment:
These comments should be consistent about using `well-known` vs. `well_known`. I think `well-known` is the way to go.
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-openid/src/main/java/org/apache/guacamole/auth/openid/OpenIDAuthenticationSession.java:
##########
@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.auth.openid;
+
+import org.apache.guacamole.net.auth.AuthenticationSession;
+
+/**
+ * Representation of an in-progress OpenID authentication attempt.
+ */
+
Review Comment:
No blank line needed, here.
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-openid/src/main/java/org/apache/guacamole/auth/openid/OpenIDAuthenticationSession.java:
##########
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.auth.openid;
+
+import org.apache.guacamole.net.auth.AuthenticationSession;
+
+/**
+ * Representation of an in-progress OpenID authentication attempt.
+ */
+public class OpenIDAuthenticationSession extends AuthenticationSession {
+ /**
Review Comment:
There still should be a blank line above this one.
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-openid/src/main/java/org/apache/guacamole/auth/openid/token/TokenValidationService.java:
##########
@@ -131,6 +150,73 @@ public JwtClaims validateToken(String token) throws GuacamoleException {
return null;
}
+ /**
+ * URLEncodes a key/value pair
+ *
+ * @param key
+ * The key to encode
+ *
+ * @param value
+ * The value to encode
+ *
+ * @return
+ * The urlencoded kay/value pair
+ */
+ private String urlencode(String key, String value) {
+ StringBuilder builder = new StringBuilder();
+ return builder.append(URLEncoder.encode(key, StandardCharsets.UTF_8))
+ .append("=")
+ .append(URLEncoder.encode(value, StandardCharsets.UTF_8))
+ .toString();
+ }
+
+ /**
+ * Exchanges the authorization code for tokens.
+ *
+ * @param code
+ * The authorization code received from the IdP.
+ *
+ * @param codeVerifier
+ * The PKCE verifier (or null if PKCE is disabled).
+ *
+ * @return
+ * The token string returned.
+ *
+ * @throws GuacamoleException
+ * If a valid token is not returned.
+ */
+ private String exchangeCode(String code, String verifier) throws GuacamoleException {
+
+ try {
+ StringBuilder bodyBuilder = new StringBuilder();
+ bodyBuilder.append(urlencode("grant_type", "authorization_code")).append("&");
+ bodyBuilder.append(urlencode("code", code)).append("&");
+ bodyBuilder.append(urlencode("redirect_uri", confService.getRedirectURI().toString())).append("&");
+ bodyBuilder.append(urlencode("scope", confService.getScope())).append("&");
+ bodyBuilder.append(urlencode("client_id", confService.getClientID()));
+
+ String clientSecret = confService.getClientSecret();
+ if (clientSecret != null && !clientSecret.trim().isEmpty()) {
+ bodyBuilder.append("&").append(urlencode("client_secret", clientSecret));
+ }
+
+ if (confService.isPKCERequired()) {
+ bodyBuilder.append("&").append(urlencode("code_verifier", verifier));
+ }
+
+ Map<String,Object> json =
+ JsonUrlReader.fetch("POST", confService.getTokenEndpoint(),
+ bodyBuilder.toString());
+
+ return (String) json.get("id_token");
+
+ }
+ catch (Exception e) {
+ logger.info("Rejected invalid OpenID code exchange: {}", e.getMessage(), e);
+ }
Review Comment:
Catching `Exception` here is probably not a good idea:
* This method `throws GuacamoleException`, but if you catch the overall `Exception` class, here, that will never actually happen, and this method will just return null every time. I suspect we actually want the `GuacamoleException` exceptions to be passed on to the callers.
* By catching `Exception` this makes the very generic logger message here both misleading and unhelpful. The OpenID code exchange isn't necessarily rejected, here, as it states - it could be that a property wasn't properly configured or there was an issue retrieving it. You are passing through the underlying exception and message, which is good, but the initial log message doesn't really help.
* Also, `logger.info()` is probably not the correct level, here - this constitutes an error, not an informational message, so it should be `logger.error()`.
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-openid/src/main/java/org/apache/guacamole/auth/openid/util/JsonUrlReader.java:
##########
@@ -0,0 +1,98 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.auth.openid.util;
+
+import java.io.BufferedReader;
+import java.io.OutputStream;
+import java.io.InputStreamReader;
+import java.net.HttpURLConnection;
+import java.net.URL;
+import java.nio.charset.StandardCharsets;
+import java.util.Map;
+import org.apache.guacamole.GuacamoleException;
+import org.jose4j.json.JsonUtil;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/*
+ * 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 {
Review 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.
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-openid/src/main/java/org/apache/guacamole/auth/openid/conf/ConfigurationService.java:
##########
@@ -110,6 +120,66 @@ public class ConfigurationService {
};
+ /**
+ * The token endpoint (URI) of the OIDC service.
+ */
+ private static final URIGuacamoleProperty OPENID_TOKEN_ENDPOINT =
+ new URIGuacamoleProperty() {
+ @Override
+ public String getName() {
+ return "openid-token-endpoint";
+ }
+ };
+
+ /**
+ * The reponse type of the OpenID service.
+ */
+ private static final EnumGuacamoleProperty<OpenIDResponseType> OPENID_RESPONSE_TYPE =
+ new EnumGuacamoleProperty<OpenIDResponseType>(OpenIDResponseType.class) {
+
+ @Override
+ public String getName() { return "openid-response-type"; }
+
+ };
+
+ /**
+ * OIDC client secret which should be submitted to the OIDC service when
+ * validating tokens with code flow
+ */
+ private static final StringGuacamoleProperty OPENID_CLIENT_SECRET =
+ new StringGuacamoleProperty() {
+ @Override
+ public String getName() {
+ return "openid-client-secret";
+ }
+ };
+
+ /**
+ * True if "Proof Key for Code Exchange" (PKCE) must be used.
+ */
+ private static final BooleanGuacamoleProperty OPENID_PKCE_REQUIRED =
+ new BooleanGuacamoleProperty() {
+ @Override
+ public String getName() {
+ return "openid-pkce-required";
+ }
+ };
+
+ /**
+ * The maximum amount of time to allow for an in-progress OpenID
+ * authentication attempt to be completed, in minutes. A user that takes
+ * longer than this amount of time to complete authentication with their
+ * identity provider will be redirected back to the identity provider to
+ * try again.
+ */
+ private static final IntegerGuacamoleProperty OPENID_AUTH_TIMEOUT =
+ new IntegerGuacamoleProperty() {
+ @Override
+ public String getName() { return "openid-auth-timeout"; }
+
+ };
+
+
Review Comment:
There are a few more minor tweaks that should be made, here - adding some newlines after the `getName()` overrides in a couple of places.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org