[ELY-2584] Add the ability to specify that the OIDC Authentication Request should include request and request_uri parameters #1984

PrarthonaPaul · 2023-09-26T16:16:04Z

https://issues.redhat.com/browse/ELY-2584
Analysis doc: wildfly/wildfly-proposals#532
Relevant discussion page:

Signing JWTs with symmetrical secret key keycloak/keycloak#24514
Sending Request Object by reference keycloak/keycloak#23150
Hows does a client communicate public Key to Keycloak that will be used to verify signed and encrypted JWTs? keycloak/keycloak#22729

Skyllarr

@PrarthonaPaul Looks good! I've just had a quick look at it and added some minor comments, I'll look at it more and its affiliated wildfly PR later this week

Skyllarr · 2023-10-02T17:37:43Z

http/oidc/pom.xml

+        <dependency>
+            <groupId>org.keycloak</groupId>
+            <artifactId>keycloak-services</artifactId>
+            <version>3.1.0.Final</version>


@PrarthonaPaul Just a minor, please put a version as a property to the properties tag in the parent pom

Hi @Skyllarr
I am not sure how to do this. Do you have an example of this being done for another dependency?
Thanks!

@PrarthonaPaul In the parent pom.xml we have properties tag where you can find versions of libraries, see here. Place your keycloak-services dependency there. Then, as with other libraries in that parent pom.xml, place your dependency to the dependencyManagement tag the same way as other deps are. Then you can just use this dependency in child modules without having to specify a version. So in http/oidc/pom.xml you will then be able to place it without a version tag

Thank you!
I have updated the pom files to reflect this.

Skyllarr · 2023-10-02T17:38:47Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/JWTClientCredentialsProvider.java

@@ -156,7 +156,7 @@ protected JwtClaims createRequestToken(String clientId, String tokenUrl) {
        return jwtClaims;
    }

-    private static KeyPair loadKeyPairFromKeyStore(String keyStoreFile, String storePassword, String keyPassword, String keyAlias, String keyStoreType) {
+    public static KeyPair loadKeyPairFromKeyStore(String keyStoreFile, String storePassword, String keyPassword, String keyAlias, String keyStoreType) {


@PrarthonaPaul Just a minor, can this method be protected instead of public?

Yes, I have changed it to protected.

Skyllarr · 2023-10-02T17:43:45Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcJsonConfiguration.java

@@ -41,12 +41,13 @@
        "expose-token", "bearer-only", "autodetect-bearer-only",
        "connection-pool-size",
        "allow-any-hostname", "disable-trust-manager", "truststore", "truststore-password",
-        "client-keystore", "client-keystore-password", "client-key-password",
+        "client-keystore-file", "client-keystore-password", "client-key-password", "client-key-alias", "client-keystore-type",


@PrarthonaPaul Just a minor, should the "client-keystore" stay here as possible configuration property because of backwards compatibility? Users could have been using it before?

oh! Thanks for noticing it! I have fixed it! Also changed the class to add a new variable called clientKeyStoreFile that's different from clientKeystore along with a getter and setter for that.

Skyllarr · 2023-10-02T17:46:49Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcRequestAuthenticator.java

+            deployment.setClientKeyStore(deployment.getClientKeyStore().replace(PROTOCOL_CLASSPATH, Objects.requireNonNull(Thread.currentThread().getContextClassLoader().getResource("")).getPath()));
+        }
+        if (!deployment.getRequestSignatureAlgorithm().equals(NONE) && deployment.getClientKeyStore() == null){
+            throw new RuntimeException("Invalid keystore configuration for signing Request Objects.");


@PrarthonaPaul Just a minor, please put this exception into the ElytronMessages class

Added! Thanks

Skyllarr · 2023-10-02T17:48:10Z

http/oidc/src/test/java/org/wildfly/security/http/oidc/KeycloakConfiguration.java

+    }
+
+    private static KeyStore createEmptyKeyStore() throws IOException, GeneralSecurityException {
+        KeyStore keyStore = KeyStore.getInstance("JKS");


Just a minor, let's not forget to change this from JKS to PKCS12

Fixed!
Thanks for going though the code!

PrarthonaPaul · 2023-09-26T18:19:14Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcRequestAuthenticator.java

+                } else {
+                    // send request as usual
+                    redirectUriBuilder.addParameter(REDIRECT_URI, redirectUri)
+                            .addParameter(STATE, state)
+                            .addParameters(forwardedQueryParams);
+                }


Currently if request or request_uri is not supported, then the authentication is sent as usual.
However, we can change it so that if it is not supported, then we throw a runtime error.
Another thing we can do, is keep it as is, but add a log message saying "Request/request_uri is not supported by the OpenID provider. Request is sent using the Oauth2 format"

@PrarthonaPaul Thinking.. this should be discussed in the proposal, I see you asked me this there as well. If the authentication is sent as oauth despite a different configuration it has to be logged as a minimum.

I think we should log this and then document that the request and the request_uri methods are used only if available with the OpenID provider, if not available a warning message will be logged. I will write this on the proposal as well

I have fixed the elytron code and the docs to reflect it.
Will change it in the proposal as well. Thanks!

PrarthonaPaul · 2023-10-03T17:25:48Z

http/oidc/pom.xml

+        <dependency>
+            <groupId>org.keycloak</groupId>
+            <artifactId>keycloak-services</artifactId>
+            <version>3.1.0.Final</version>


Hi @Skyllarr
I am not sure how to do this. Do you have an example of this being done for another dependency?
Thanks!

PrarthonaPaul · 2023-10-03T17:37:28Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/JWTClientCredentialsProvider.java

@@ -156,7 +156,7 @@ protected JwtClaims createRequestToken(String clientId, String tokenUrl) {
        return jwtClaims;
    }

-    private static KeyPair loadKeyPairFromKeyStore(String keyStoreFile, String storePassword, String keyPassword, String keyAlias, String keyStoreType) {
+    public static KeyPair loadKeyPairFromKeyStore(String keyStoreFile, String storePassword, String keyPassword, String keyAlias, String keyStoreType) {


Yes, I have changed it to protected.

PrarthonaPaul · 2023-10-03T17:38:38Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcJsonConfiguration.java

@@ -41,12 +41,13 @@
        "expose-token", "bearer-only", "autodetect-bearer-only",
        "connection-pool-size",
        "allow-any-hostname", "disable-trust-manager", "truststore", "truststore-password",
-        "client-keystore", "client-keystore-password", "client-key-password",
+        "client-keystore-file", "client-keystore-password", "client-key-password", "client-key-alias", "client-keystore-type",


oh! Thanks for noticing it! I have fixed it! Also changed the class to add a new variable called clientKeyStoreFile that's different from clientKeystore along with a getter and setter for that.

PrarthonaPaul · 2023-10-03T17:38:49Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcRequestAuthenticator.java

+            deployment.setClientKeyStore(deployment.getClientKeyStore().replace(PROTOCOL_CLASSPATH, Objects.requireNonNull(Thread.currentThread().getContextClassLoader().getResource("")).getPath()));
+        }
+        if (!deployment.getRequestSignatureAlgorithm().equals(NONE) && deployment.getClientKeyStore() == null){
+            throw new RuntimeException("Invalid keystore configuration for signing Request Objects.");


Added! Thanks

PrarthonaPaul · 2023-10-03T17:39:12Z

http/oidc/src/test/java/org/wildfly/security/http/oidc/KeycloakConfiguration.java

+    }
+
+    private static KeyStore createEmptyKeyStore() throws IOException, GeneralSecurityException {
+        KeyStore keyStore = KeyStore.getInstance("JKS");


Fixed!
Thanks for going though the code!

PrarthonaPaul · 2023-10-03T18:12:32Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcRequestAuthenticator.java

+        } else if (deployment.getRequestSignatureAlgorithm().contains(HS_256)
+                || deployment.getRequestSignatureAlgorithm().contains(HS_384)
+                || deployment.getRequestSignatureAlgorithm().contains(HS_512)) { //signed with symmetric key
+            jsonWebSignature.setAlgorithmHeaderValue(deployment.getRequestSignatureAlgorithm());
+            Key key = new HmacKey(deployment.getResourceCredentials().get("secret").toString().getBytes(StandardCharsets.UTF_8));   //the client secret is a shared secret between the server and the client
+            jsonWebSignature.setDoKeyValidation(false);


For now, when the user wants to sign using symmetric keys, we are signing using the client secret. However, I am not sure what to do when they use JWT as client credentials instead of client secret.

@PrarthonaPaul Can they use JWT as a client credential? Can you please describe the background for the situation you are asking about?

in the proposal you mentioned:

In order to sign the jwt using an algorithm other than none, the user must specify the KeyPair used.

So I thought the keypair can be used in all situations for signing but you might be asking about something else

Yes, sorry.
According to this doc: https://www.keycloak.org/docs/latest/securing_apps/#_client_authentication_adapter
we can either set up the client credentials to be just the client secret, which is a just a hash/SHA (like 19666a4f-32dd-4049-b082-684c74115f28).
But you can also configure it using a JWT. But it is not related to the request RFE.
This is what the options are for specifying the client credentials:

Now the reason I am using the client secret to sign a JWT for my RFE is because as we found out HS keys are symmetric. And from what I have seen instead of creating a certificate, we create a secret that is shared between the client. For Keycloak, the client secret is something that is shared between the client and the server.

But what I am wondering is what happens when the user configures the client credentials as a JWT or X509 certificate?

fjuma · 2023-10-30T17:24:16Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/JWTClientCredentialsProvider.java

@@ -156,7 +156,7 @@ protected JwtClaims createRequestToken(String clientId, String tokenUrl) {
        return jwtClaims;
    }

-    private static KeyPair loadKeyPairFromKeyStore(String keyStoreFile, String storePassword, String keyPassword, String keyAlias, String keyStoreType) {
+    protected static KeyPair loadKeyPairFromKeyStore(String keyStoreFile, String storePassword, String keyPassword, String keyAlias, String keyStoreType) {


Since this is a generic helper method that will also be used in OidcRequestAuthenticator, it would be better to move this method to either an existing utility class or introduce a new utility class and include this method there.

Sounds good.
Does this class: https://github.com/wildfly-security/wildfly-elytron/blob/2.x/http/oidc/src/main/java/org/wildfly/security/http/oidc/ClientCredentialsProviderUtils.java look like a good one to move this function to?
If not, then we can create another utility class called JWTClientCredentialsProviderUtils.java or something.

Since the method will also be used for signing the authentication request in addition to being used for client credentials handling, I think we could create another utility class but try to give it a more generic name (e.g., JWTSigningUtils or something along those lines).

Okay! Sounds good! Thanks :)

This is done!

fjuma · 2023-10-30T17:25:14Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/Oidc.java

@@ -52,10 +52,14 @@ public class Oidc {
    public static final String TEXT_CONTENT_TYPE = "text/*";
    public static final String DISCOVERY_PATH = ".well-known/openid-configuration";
    public static final String KEYCLOAK_REALMS_PATH = "realms/";
+    public static final String KEYSTORE_PASS = "password";


Is this something that's used for tests? It shouldn't be added to the constants class.

moved relevant constants to KeycloakConfiguration class.

fjuma · 2023-10-30T17:25:50Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/Oidc.java

@@ -52,10 +52,14 @@ public class Oidc {
    public static final String TEXT_CONTENT_TYPE = "text/*";
    public static final String DISCOVERY_PATH = ".well-known/openid-configuration";
    public static final String KEYCLOAK_REALMS_PATH = "realms/";
+    public static final String KEYSTORE_PASS = "password";
+    public static final String PKCS12_KEYSTORE_TYPE = "PKCS12";


Same here, this looks like something that's only needed for tests, it shouldn't be in the Oidc constants class.

moved to keycloakConfiguration class

fjuma · 2023-10-30T17:26:23Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/Oidc.java

    public static final String JSON_CONFIG_CONTEXT_PARAM = "org.wildfly.security.http.oidc.json.config";
    static final String ACCOUNT_PATH = "account";
    public static final String CLIENTS_MANAGEMENT_REGISTER_NODE_PATH = "clients-managements/register-node";
    public static final String CLIENTS_MANAGEMENT_UNREGISTER_NODE_PATH = "clients-managements/unregister-node";
+    public static final String ADMIN_CONSOLE_PATH = "admin/master/console/#";


Please check all the new constants introduced in this class, it looks like these were meant for tests purposes.

fjuma · 2023-10-30T17:32:33Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/Oidc.java

@@ -116,6 +125,35 @@ public class Oidc {
    public static final String X_REQUESTED_WITH = "X-Requested-With";
    public static final String XML_HTTP_REQUEST = "XMLHttpRequest";

+    /* Accepted Request Object Signing Algorithms for KeyCloak*/
+    public static final String NONE = "none";
+    public static final String RS_256 = "RS256";


Note that this class already contains some constants with similar names and also makes use of the AlgorithmIdentifiers class.

These newly added constants appear to be specific to the Keycloak OpenID provider.

Are these really needed?

Can we get the allowed values from discovery?

Fixed!
I am using AlgorithmIdentifiers class and throwing an exception if the algorithm is not supported.

fjuma · 2023-10-30T17:32:52Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/Oidc.java

+    public static final String PS_512 = "PS512";
+
+    /* Accepted Request Object Encrypting Algorithms for KeyCloak*/
+    public static final String RSA_OAEP = "RSA-OAEP";


Same comment as above here.

fjuma · 2023-10-30T17:33:05Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/Oidc.java

+    public static final String RSA1_5 = "RSA1_5";
+
+    /* Accepted Request Object Encryption Methods for KeyCloak*/
+    public static final String A256GCM = "A256GCM";


Same here, can we get these accepted values through discovery instead?

It seems like most of these, I use for testing, and some of them I don't use at all.
So, I can remove/move those to a test Class, like KeycloakConfiguration. For some of the others, I am using them in the actual implementation. So, I can replace them using AlgorithmIdentifiers.
I can get them using Discovery and I think I do get them. But I don't think I use them yet.

fjuma · 2023-10-30T17:35:56Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcClientConfiguration.java

@@ -223,6 +249,15 @@ protected void resolveUrls() {
                    tokenUrl = config.getTokenEndpoint();
                    logoutUrl = config.getLogoutEndpoint();
                    jwksUrl = config.getJwksUri();
+                    authorizationEndpoint = config.getAuthorizationEndpoint();


Which endpoint is this?

I don't see something that corresponds to this added in OidcProviderMetadata, is this necessary? Or is it just the pushedAuthorizationRequestEndpoint that's needed?

Actually I do see authorization_endpoint already referenced in the existing OidcProviderMetadata code. But just to understand better, is the authorizationEndpoint variable that's being introduced here used anywhere?

It has this endpoint: http://localhost:8080/realms/myrealm/protocol/openid-connect/auth/device
I don't think I use it tho. So I can get rid of it.

fjuma · 2023-10-30T17:55:01Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcClientConfiguration.java

+                    requestObjectEncryptionEncValuesSupported = config.getRequestObjectEncryptionEncValuesSupported();
+                    requestObjectEncryptionAlgValuesSupported = config.getRequestObjectEncryptionAlgValuesSupported();
+                    requestUriParameterSupported = config.getRequestUriParameterSupported();
+                    realmKeySet = createrealmKeySet();


I don't think we need to create the realm key set here. It would be better to do this when we know we actually need to make use of it (similar to JWKPublicKeyLocator where we use the jwks url when we know we need to).

moved to OidcPublicKeyExtractor

fjuma · 2023-10-30T17:57:44Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcClientConfiguration.java

@@ -237,6 +272,26 @@ protected void resolveUrls() {
        }
    }

+    private JsonWebKeySet createrealmKeySet() throws Exception{


"realm" is terminology specific to Keycloak so I don't think we should use it in the method name here.

This method is meant to obtain the encryption keys from the OpenID provider, right? We should rename this method to reflect that.

removed the function from this class and moved it to a utility class. New function is called extractPublicKeySet

fjuma · 2023-10-30T17:58:53Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcClientConfiguration.java

@@ -237,6 +272,26 @@ protected void resolveUrls() {
        }
    }

+    private JsonWebKeySet createrealmKeySet() throws Exception{
+        HttpGet request = new HttpGet(jwksUrl);


Can we make use of the Oidc.sendJsonHttpRequest method here to simplify the logic below?

I tried doing this and kept getting a class mismatch for the JsonSerialization.readValue() function.
I tried this for the JsonWebKeySet, String, and StringBuilder classes.
When I was initially writing this function, I was also trying to use JsonSerialization.readValue() and did not succeed. Which is why I had to do this workaround.

There should be an example in JWKPublicKeyLocator where we call sendJsonHttpRequest and store the result in a JsonWebKeySet. Did you take a look there to see how that differs from what you were trying to do?

I see what the difference is.
The JWKPublicKeyLocator class uses org.wildfly.security.jose.jwk.JsonWebKeySet and I am using the Jose4j one, which is why the parsing failed before.
I have switched it to the wildfly.security one now and it works.
Thank you!

fjuma · 2023-10-30T18:01:51Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcClientConfiguration.java

+        return authenticationRequestFormat;
+    }
+
+    public void setAuthenticationRequestFormat(String requestObjectType ) {


s/requestObjectType/authenticationRequestFormat ?

fjuma · 2023-10-30T18:02:48Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcClientConfiguration.java

+        return requestSignatureAlgorithm;
+    }
+
+    public void setRequestSignatureAlgorithm(String algorithm) {


s/algorithm/requestSignatureAlgorithm

fjuma · 2023-10-30T18:02:59Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcClientConfiguration.java

+        return requestEncryptAlgorithm;
+    }
+
+    public void setRequestEncryptAlgorithm(String algorithm) {


s/algorithm/requestEncryptionAlgorithm

fjuma · 2023-10-30T18:03:22Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcClientConfiguration.java

+        return requestEncryptEncValue;
+    }
+
+    public void setRequestEncryptEncValue (String enc) {


extra white space before bracket

fjuma · 2023-10-30T18:05:42Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcClientConfiguration.java

+        this.requestSignatureAlgorithm = algorithm;
+    }
+
+    public String getRequestEncryptAlgorithm() {


We should be consistent with the naming pattern for the signing case, there we used getRequestObjectSigningAlgValuesSupported so we should follow a similar pattern here and below.

fjuma · 2023-10-30T18:06:46Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcClientConfiguration.java

+        this.requestEncryptEncValue = enc;
+    }
+
+    public String getRealmKey () {


We shouldn't use "realm" here. We should rename this to better explain what this returns.

I wasn't using these functions, so I removed anything related to realmKey that I added.

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcClientConfiguration.java

fjuma · 2023-10-30T18:08:26Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcClientConfiguration.java

+        this.clientKeyAlias = alias;
+    }
+
+    public JsonWebKeySet getrealmKeySet() {


Note that method names should use camel case, i.e., style wise, getrealm should be getRealm.

However, we should rename this to not mention realm as described in comments above.

moved to utility class and renamed.

fjuma · 2023-10-30T18:17:24Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcRequestAuthenticator.java

+                    redirectUriBuilder.addParameter(REDIRECT_URI, redirectUri)
+                            .addParameter(STATE, state)
+                            .addParameters(forwardedQueryParams);
+                    log.info("The OpenID provider does not support the request parameter. Sending the request using OAuth2 format.");


This should make use of ElytronMessages.

fjuma · 2024-03-20T22:02:49Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/ElytronMessages.java

+    @Message(id = 23057, value = "Invalid keystore configuration for signing Request Objects.")
+    IOException invalidKeyStoreConfiguration();
+
+    @Message(id = 23058, value = "The signature algorithm specified is not supported by the OpenID Provider.")


s/The signature algorithm/request object signature algorithm

fjuma · 2024-03-20T22:03:39Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/ElytronMessages.java

+    @Message(id = 23058, value = "The signature algorithm specified is not supported by the OpenID Provider.")
+    IOException invalidRequestObjectSignatureAlgorithm();
+
+    @Message(id = 23059, value = "The encryption algorithm specified is not supported by the OpenID Provider.")


s/The encryption algorithm/request object encryption algorithm

fjuma · 2024-03-20T22:03:56Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/ElytronMessages.java

+    @Message(id = 23059, value = "The encryption algorithm specified is not supported by the OpenID Provider.")
+    IOException invalidRequestObjectEncryptionAlgorithm();
+
+    @Message(id = 23060, value = "The content encryption algorithm specified is not supported by the OpenID Provider.")


s/The content encryption algorithm/The request object content encryption algorithm

fjuma · 2024-03-20T22:05:00Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/ElytronMessages.java

+    @Message(id = 23060, value = "The content encryption algorithm specified is not supported by the OpenID Provider.")
+    IOException invalidRequestObjectContentEncryptionAlgorithm();
+
+    @LogMessage(level = INFO)


I think we should use WARN here instead of INFO.

fjuma · 2024-03-20T22:22:57Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcPublicKeyExtractor.java

+ *
+ * @author <a href="mailto:prpaul@redhat.com">Prarthona Paul</a>
+ */
+public interface OidcPublicKeyExtractor {


This looks similar to the PublicKeyLocator interface. Can we make use of the existing interface instead?

fjuma · 2024-03-20T22:27:13Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/JWTSigningUtils.java

+ * @author <a href="mailto:prpaul@redhat.com">Prarthona Paul</a>
+ */
+
+public class JWTSigningUtils implements JWTSigning{


I don't think the JWTSignging interface is needed. We can just use the utility class.

fjuma · 2024-03-20T22:28:25Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/Oidc.java

+        /**
+         * Get the string value for this referral mode.
+         *
+         * @return the string value for this referral mode


Likely just a copy/paste issue but "referral mode" needs to be updated.

fjuma · 2024-03-20T22:31:25Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcClientConfigurationBuilder.java

+        } else {
+            oidcClientConfiguration.setRequestSignatureAlgorithm(NONE);
+        }
+        if (oidcJsonConfiguration.getRequestEncryptAlgorithm() != null && oidcJsonConfiguration.getRequestContentEncryptionMethod() != null) { //both are required to encrypt the request object


Might be good to throw an IllegalArgumentException if only one is specified if that's invalid configuration.

Since we know that we'll need to encrypt the request object, maybe we can set the public key locator for that here on the oidcClientConfiguration (e.g., oidcClientConfiguration.setEncPublicKeyLocator or something like that) similar to the way we set the public key locator for the SIG case.

fjuma · 2024-03-20T22:32:59Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcClientConfiguration.java

+        this.requestObjectSigningKeystoreFile = keyStoreFile;
+    }
+
+    public String getRequestObjectSigningKeyStorePassword() {


We should be consistent with Keystore or KeyStore in the method names in this file.

fjuma · 2024-03-20T22:33:31Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcClientContext.java

+        }
+
+        @Override
+        public String getRequestObjectSigningKeyStorePassword() {


We should be consistent with Keystore vs KeyStore in the method names in this file.

fjuma · 2024-03-21T14:24:49Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/JWKPublicKeySetExtractor.java

+ *
+ * @author <a href="mailto:prpaul@redhat.com">Prarthona Paul</a>
+ * */
+public class JWKPublicKeySetExtractor implements OidcPublicKeyExtractor {


I think this could be a non-public class that implements PublicKeyLocator, similar to JWKPublicKeyLocator but it would be used to obtain the public key for JWK.Use.ENC instead of JWK.Use.SIG. This should also cache the key like JWKPublicKeyLocator does.

I have followed the PublicKeyLocator class to implement caching.
Please let me know if it makes sense.

fjuma · 2024-03-21T14:25:34Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/JWTSigning.java

+ * @author <a href="mailto:prpaul@redhat.com">Prarthona Paul</a>
+ */
+
+public interface JWTSigning {


I don't think we need this interface.

fjuma · 2024-03-21T14:28:40Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcClientConfigurationBuilder.java

+        String[] unsupportedAttributes = unsupportedAttributesParameter.split(" ");
+        for (String attributeName : unsupportedAttributes) {
+            switch(attributeName) {
+                case AUTHENTICATION_REQUEST_FORMAT:


Similar to the scope RFE, we shouldn't need to hardcode information about the unsupported attributes for WildFly. We'll need to add the handling for unsupported attributes in the elytron-oidc-client subsystem.

fjuma · 2024-03-21T14:35:45Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcClientConfiguration.java

+    protected String authenticationRequestFormat;
+    protected String requestSignatureAlgorithm;
+    protected String requestEncryptAlgorithm;
+    protected String requestContentEncryptionMethod;


We should be consistent with the use of request vs requestObject. I think using requestObject makes sense since that matches the actual attribute names too.

fjuma · 2024-03-21T14:38:01Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcJsonConfiguration.java

+    protected String authenticationRequestFormat;
+
+    @JsonProperty("request-object-signing-algorithm")
+    protected String requestSignatureAlgorithm;


requestObjectSignatureAlgorithm

fjuma · 2024-03-21T14:38:09Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcJsonConfiguration.java

+    protected String requestSignatureAlgorithm;
+
+    @JsonProperty("request-object-encryption-algorithm")
+    protected String requestEncryptAlgorithm;


requestObjectEncryptionAlgorithm

fjuma · 2024-03-21T14:38:19Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcJsonConfiguration.java

+    protected String requestEncryptAlgorithm;
+
+    @JsonProperty("request-object-content-encryption-algorithm")
+    protected String requestContentEncryptionMethod;


requestObjectContentEncryptionAlgorithm

fjuma · 2024-03-21T14:42:24Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcJsonConfiguration.java

@@ -62,8 +63,16 @@ public class OidcJsonConfiguration {
    protected String clientKeystore;
    @JsonProperty("client-keystore-password")
    protected String clientKeystorePassword;
-    @JsonProperty("client-key-password")


Why was this removed?

fjuma · 2024-03-21T14:43:14Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcJsonConfiguration.java

        "allow-any-hostname", "disable-trust-manager", "truststore", "truststore-password",
-        "client-keystore", "client-keystore-password", "client-key-password",


Looks like client-key-password was removed?

fjuma · 2024-03-21T14:48:08Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcProviderMetadata.java

+        return pushedAuthorizationRequestEndpoint;
+    }
+
+    public void setPushedAuthorizationRequestEndpoint (String url) {


Just minor, extra space before the (

fjuma · 2024-03-21T15:17:04Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcConfigurationServletListener.java

@@ -66,7 +67,7 @@ public void contextInitialized(ServletContextEvent sce) {
                if (is == null) {
                    oidcClientConfiguration = new OidcClientConfiguration();
                } else {
-                    oidcClientConfiguration = OidcClientConfigurationBuilder.build(is);
+                    oidcClientConfiguration = OidcClientConfigurationBuilder.buildWithoutUnsupportedAttributes(is, servletContext.getInitParameter(JSON_CONFIG_UNSUPPORTED_ATTRIBUTE_PARAM));


Same comment as on the other PR:

https://github.com/wildfly-security/wildfly-elytron/pull/1925/files#r1532368580

fjuma · 2024-03-21T15:20:46Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcRequestAuthenticator.java

+        }
+    }
+
+    public KeyPair getkeyPair() throws IOException {


This can be private static. The deployment can be passed as a parameter.

fjuma · 2024-03-21T15:47:48Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcRequestAuthenticator.java

+            jsonEncryption.setAlgorithmConstraints(new AlgorithmConstraints(AlgorithmConstraints.ConstraintType.PERMIT, deployment.getRequestEncryptAlgorithm(), deployment.getRequestContentEncryptionMethod()));
+            jsonEncryption.setAlgorithmHeaderValue(deployment.getRequestEncryptAlgorithm());
+            jsonEncryption.setEncryptionMethodHeaderParameter(deployment.getRequestContentEncryptionMethod());
+            JWK[] jwkList = new JWKPublicKeySetExtractor().extractPublicKeySet(deployment).getKeys();


Instead of needing to use new JWKPublicKeySetExtractor(), we can get the public key locator to use from the oidcClientConfiguration (e.g., oidcClientConfiguration.getEncPublicKeyLocator) and then make use of that to be obtain the public key.

fjuma · 2024-03-21T15:55:52Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcRequestAuthenticator.java

+            jsonEncryption.setEncryptionMethodHeaderParameter(deployment.getRequestContentEncryptionMethod());
+            JWK[] jwkList = new JWKPublicKeySetExtractor().extractPublicKeySet(deployment).getKeys();
+            for (JWK jwk : jwkList) {
+                if (jwk.getPublicKeyUse().equals("enc")) {  //JWTs are to be encrypted with public keys shared by the OpenID provider and decrypted by the private key they hold


I think the logic of finding the "enc" public key would be part of the locator class.

fjuma · 2024-03-21T16:00:07Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcRequestAuthenticator.java

            return redirectUriBuilder.build().toString();
        } catch (URISyntaxException e) {
            throw log.unableToCreateRedirectResponse(e);
+        } catch (IOException | JoseException | InvalidJwtException e) {
+            throw new RuntimeException(e);


I think we should add a new message to ElytronMessages for this (there should be other examples there for RuntimeExceptions that wrap another exception).

I have changed this to be a nested try-catch, where the inner try catch surrounds createRequestWithRequestParameter(REQUEST, redirectUriBuilder, redirectUri, state, forwardedQueryParams);
However, log.unableToCreateRequestWithRequestParameter() throws a runtime exception, since it can either be an IO exception, or a joseException or InvalidJwtException.
Alternatively, I could throw these exceptions separately, but earlier in the stack of function calls.

darranl · 2024-03-21T19:24:11Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/JWKPublicKeySetExtractor.java

@@ -0,0 +1,44 @@
+/*
+ * JBoss, Home of Professional Open Source.
+ * Copyright 2020 Red Hat, Inc., and individual contributors


PrarthonaPaul · 2024-04-04T20:12:39Z

Thanks, @fjuma and @darranl for your review.
I have addressed your comments on this PR.

…quest should include request and request_uri parameters

PrarthonaPaul requested review from fjuma and Skyllarr as code owners September 26, 2023 16:16

PrarthonaPaul mentioned this pull request Sep 26, 2023

[WFLY-17143] Add the ability to specify that the OIDC Authentication Request should include request and request_uri parameters wildfly/wildfly#17219

Open

PrarthonaPaul force-pushed the ELY-2584 branch 2 times, most recently from 93dc491 to 3ce624a Compare September 28, 2023 16:02

Skyllarr reviewed Oct 2, 2023

View reviewed changes

PrarthonaPaul force-pushed the ELY-2584 branch from 3ce624a to aa4ac31 Compare October 3, 2023 17:37

PrarthonaPaul commented Oct 5, 2023

View reviewed changes

PrarthonaPaul force-pushed the ELY-2584 branch from aa4ac31 to 58fd4a6 Compare October 6, 2023 14:11

PrarthonaPaul force-pushed the ELY-2584 branch from 58fd4a6 to 0bc8938 Compare October 13, 2023 20:11

fjuma reviewed Oct 30, 2023

View reviewed changes

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcClientConfiguration.java Show resolved Hide resolved

fjuma reviewed Oct 30, 2023

View reviewed changes

PrarthonaPaul force-pushed the ELY-2584 branch from 6bf8688 to 62ba916 Compare November 8, 2023 18:20

PrarthonaPaul force-pushed the ELY-2584 branch from 62ba916 to cd4478d Compare February 26, 2024 20:47

PrarthonaPaul force-pushed the ELY-2584 branch 2 times, most recently from 968c2e5 to 634191c Compare March 20, 2024 14:37

fjuma requested changes Mar 20, 2024

View reviewed changes

fjuma reviewed Mar 21, 2024

View reviewed changes

darranl reviewed Mar 21, 2024

View reviewed changes

PrarthonaPaul force-pushed the ELY-2584 branch 3 times, most recently from df298ec to 6ad38e0 Compare April 4, 2024 17:01

[ELY-2584] Add the ability to specify that the OIDC Authentication Re…

553ccb6

…quest should include request and request_uri parameters

PrarthonaPaul force-pushed the ELY-2584 branch 2 times, most recently from a764c3f to ab5f2a1 Compare April 12, 2024 14:17

[squash] added changes for community stability level deployment

ec566f3

PrarthonaPaul force-pushed the ELY-2584 branch from ab5f2a1 to ec566f3 Compare April 15, 2024 14:16

		"allow-any-hostname", "disable-trust-manager", "truststore", "truststore-password",
		"client-keystore", "client-keystore-password", "client-key-password",

[ELY-2584] Add the ability to specify that the OIDC Authentication Request should include request and request_uri parameters #1984

Are you sure you want to change the base?

[ELY-2584] Add the ability to specify that the OIDC Authentication Request should include request and request_uri parameters #1984

Conversation

PrarthonaPaul commented Sep 26, 2023 • edited

Skyllarr left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Skyllarr Oct 6, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjuma Oct 30, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PrarthonaPaul commented Sep 26, 2023 •

edited

Skyllarr left a comment •

edited

Skyllarr Oct 6, 2023 •

edited

fjuma Oct 30, 2023 •

edited