[ELY-2359] Support HTTP Digest when fronted by load balancer #1909

Skyllarr · 2023-05-30T07:29:02Z

https://issues.redhat.com/browse/ELY-2359

Skyllarr · 2023-09-05T09:56:50Z

@fjuma This can be reviewed, tests are in the wilfdly PR as they need to setup clustering. Thank you!

fjuma

Thanks @Skyllarr! I've added some initial comments.

fjuma · 2023-09-05T19:13:17Z

http/base/src/main/java/org/wildfly/security/http/HttpConstants.java

@@ -51,6 +51,7 @@ private HttpConstants() {

    public static final String CONFIG_VALIDATE_DIGEST_URI = CONFIG_BASE + ".validate-digest-uri";
    public static final String CONFIG_SKIP_CERTIFICATE_VERIFICATION = CONFIG_BASE + ".skip-certificate-verification";
+    public static final String CONFIG_SESSION_DIGEST = CONFIG_BASE + ".session-digest";


I think it would be good to make it more clear that this is a boolean value. Maybe something like use-digest-session-based-nonce-manager or use-digest-session-nonce-manager or use-session-digest, etc.

@fjuma Thank you! Fixed to be use-session-based-digest-nonce-manager

fjuma · 2023-09-05T20:45:53Z

auth/server/base/src/main/java/org/wildfly/security/auth/server/MechanismConfiguration.java

@@ -146,6 +148,9 @@ public MechanismRealmConfiguration getMechanismRealmConfiguration(String realmNa
        return mechanismRealms.get(realmName);
    }

+    public boolean getSessionDigest() {


Just a minor comment on naming, since this is returning a boolean value, we could rename this to something like isUseSessionDigest or getUseSessionDigest (or something else depending on the property name you go with).

@fjuma Fixed

fjuma · 2023-09-05T20:48:32Z

auth/server/base/src/main/java/org/wildfly/security/auth/server/MechanismConfiguration.java

@@ -271,6 +277,12 @@ public Builder setServerCredentialSource(final CredentialSource serverCredential
            return this;
        }

+        public Builder setSessionDigest(final Boolean sessionDigest) {


Same here, would be good to rename this too.

fjuma · 2023-09-05T21:00:05Z

http/digest/src/main/java/org/wildfly/security/http/digest/DigestMechanismFactory.java

+            Object configSessionDigest = properties.get(CONFIG_SESSION_DIGEST);
+            String sessionDigest = configSessionDigest instanceof String ? (String) configSessionDigest : null;
+            if (Boolean.parseBoolean(sessionDigest)) {
+                nonceManager = new PersistentNonceManager(300000, 900000, true, 20, SHA256, ElytronMessages.httpDigest);


Might be good to make these parameter values constants so they can be shared with the default NonceManager too.

@fjuma Thank you! fixed by placing the constants to the NonceManagerUtils

http/digest/src/main/java/org/wildfly/security/http/digest/NonceManager.java

fjuma · 2023-09-05T21:03:16Z

http/digest/src/main/java/org/wildfly/security/http/digest/NonceManager.java

-        } catch (GeneralSecurityException e) {
-            throw new IllegalStateException(e);
-        }
+        return NonceManagerUtils.generateNonce(salt, algorithm, nonceCounter, privateKey );


Totally minor, there's an extra space before the closing bracket.

Fixed, thanks!

fjuma · 2023-09-05T21:44:28Z

http/digest/src/main/java/org/wildfly/security/http/digest/PersistentNonceManager.java

+     * @param algorithm the message digest algorithm to use when creating the digest portion of the nonce.
+     */
+
+    @Deprecated


Just to check, why is this deprecated?

@fjuma This constructor was deprecated because its associated parent constructor is deprecated. I will remove this constructor instead of adding it as deprecated

fjuma · 2023-09-05T21:47:45Z

.../digest/src/main/java/org/wildfly/security/http/digest/WildFlyElytronHttpDigestProvider.java

@@ -1,6 +1,6 @@
 /*
 * JBoss, Home of Professional Open Source.
- * Copyright 2018 Red Hat, Inc., and individual contributors
+ * Copyright 2023 Red Hat, Inc., and individual contributors


Just curious why this is change is needed. I don't seem to see other updates in this file.

Fixed, thanks!

fjuma · 2023-09-05T21:50:33Z

http/digest/src/main/java/org/wildfly/security/http/digest/DigestAuthenticationMechanism.java

+            if (request.getScope(Scope.SESSION) == null || !request.getScope(Scope.SESSION).exists()) {
+                request.getScope(Scope.SESSION).create();
+            }
+            PersistentNonceManager persistentNonceManager = (PersistentNonceManager) request.getScope(Scope.SESSION).getAttachment("persistentNonceManager");


I think it would be good to make the "persistentNonceManager" key a constant.

Fixed, thanks!

http/digest/src/main/java/org/wildfly/security/http/digest/PersistentNonceManager.java

fjuma · 2023-09-06T21:35:06Z

http/digest/src/main/java/org/wildfly/security/http/digest/PersistentNonceManager.java

+     *
+     * @param persistentNonceManager PersistentNonceManager instance to update the attributes from
+     */
+    void refreshInfoFromSessionNonceManager(PersistentNonceManager persistentNonceManager) {


Just so I understand better, would you be able to provide some details on why this is needed? Wondering if it's possible to make use of the nonce manager from the session directly rather than updating the attributes of another instance.

@fjuma The nonceManager in DigestAuthenticationmechanism, where this method is being used, is a final variable. I need to be retrieving the nonce manager with each request, but I cannot assign to a final variable. So I am assigning only its attributes instead. Should I change the nonceManager to be non final so I can assign to it with every request?

I removed the final from the nonceManager and removed this method. It is now being assigned directly instead of updating specific attributes

Skyllarr · 2023-12-05T15:21:58Z

@darranl Please review, thanks!

fjuma · 2023-12-06T21:49:12Z

http/digest/src/main/java/org/wildfly/security/http/digest/DigestMechanismFactory.java

+        } else if (properties.get(CONFIG_SESSION_BASED_DIGEST_NONCE_MANAGER) != null) {
+            Object configSessionDigest = properties.get(CONFIG_SESSION_BASED_DIGEST_NONCE_MANAGER);
+            String sessionDigest = configSessionDigest instanceof String ? (String) configSessionDigest : null;
+            if (Boolean.parseBoolean(sessionDigest)) {


I think Boolean.parseBoolean((String) properties.get(CONFIG_SESSION_BASED_DIGEST_NONCE_MANAGER)) could be used here.

Updated, thanks!

fjuma · 2023-12-06T21:57:02Z

http/digest/src/main/java/org/wildfly/security/http/digest/PersistentNonceManager.java

+     * @param log mechanism specific logger.
+     */
+    PersistentNonceManager(long validityPeriod, long nonceSessionTime, boolean singleUse, int keySize, String algorithm, ElytronMessages log) {
+        this.validityPeriodNano = validityPeriod * 1000000;


Just minor but looks like this constructor could just call the one below with customExecutor set to null.

fjuma · 2023-12-06T22:07:30Z

http/digest/src/main/java/org/wildfly/security/http/digest/PersistentNonceManager.java

+    private long nonceSessionTime;
+    private boolean singleUse;
+    private String algorithm;
+    private volatile ElytronMessages log;


Why is this volatile?

@fjuma Not sure why this was volatile, I removed it, thanks!

fjuma · 2024-01-03T15:24:45Z

@darranl When you get a chance, please review, thanks!

Skyllarr · 2024-01-10T10:41:21Z

@darranl Please review, thank you!

Skyllarr · 2024-01-30T15:36:35Z

@darranl Please review this one when you get a chance, thank you!

Skyllarr · 2024-03-07T12:19:24Z

@darranl Please review this one when you get a chance, thank you!

darranl · 2024-03-08T14:41:15Z

I am trying to unravel some of the history regarding the NonceManager, firstly I think I would say in this case we probably don't need to read too much into the variable being final - if at the time these classes were implemented we were setting it once and never changing it we would have set it to final more to allow the compiler to verify we set it to comething and will not change it - not that it is final so we can never change it.

It looks like the package is considered private API so I think this has the option to do what we want here rather than constrain ourselves with the past.

I see Ashley's changes also added the option to pass in a NonceManager but searching the code I can't find us using this yet.

So maybe we could go a few steps further:

Define a NonceManager interface / API in a public package with the methods we need including Ashley's fix.
Maybe the mechanism can always pass in the HttpServerRequest object so the NonceManager can interact directly?
Our two NonceManager implementations maybe should not be public?

auth/server/base/src/main/java/org/wildfly/security/auth/server/MechanismInformation.java

auth/server/base/src/main/java/org/wildfly/security/auth/server/MechanismConfiguration.java

Skyllarr · 2024-05-08T14:07:50Z

Just to post an update here - since this will be persisted within the HTTP session the backwards compatibility will need to be kept in mind

Skyllarr · 2024-05-08T14:20:50Z

I added some TODOs in the code. The remaining points to address are from Darran above:

Define a NonceManager interface / API in a public package with the methods we need including Ashley's fix.
Maybe the mechanism can always pass in the HttpServerRequest object so the NonceManager can interact directly?
Our two NonceManager implementations maybe should not be public?

darranl · 2024-09-15T17:21:06Z

FYI I am going to be picking up this one to continue, we can keep this PR here for now but I will likely create a new one to target specific branches.

Skyllarr requested a review from fjuma as a code owner May 30, 2023 07:29

Skyllarr force-pushed the ely-2359 branch from d9c3bc5 to 143ec82 Compare May 30, 2023 09:47

Skyllarr mentioned this pull request Jun 1, 2023

[WFLY-16555] Add tests and docs for HTTP Digest persisted in HTTP session wildfly/wildfly#16897

Open

Skyllarr force-pushed the ely-2359 branch from 143ec82 to 49df71c Compare June 2, 2023 16:22

Skyllarr force-pushed the ely-2359 branch from 49df71c to 0991d95 Compare August 23, 2023 09:03

Skyllarr force-pushed the ely-2359 branch 2 times, most recently from 4db7996 to 3bdc975 Compare September 5, 2023 09:49

fjuma requested changes Sep 5, 2023

View reviewed changes

fjuma reviewed Sep 6, 2023

View reviewed changes

Skyllarr force-pushed the ely-2359 branch from 3bdc975 to 438873a Compare December 5, 2023 14:40

fjuma requested changes Dec 6, 2023

View reviewed changes

fjuma requested a review from darranl December 6, 2023 22:38

Skyllarr force-pushed the ely-2359 branch 2 times, most recently from 4605226 to 12b892a Compare January 10, 2024 10:39

Skyllarr force-pushed the ely-2359 branch from 12b892a to 285f024 Compare March 7, 2024 11:58

darranl reviewed Mar 8, 2024

View reviewed changes

auth/server/base/src/main/java/org/wildfly/security/auth/server/MechanismInformation.java Outdated Show resolved Hide resolved

auth/server/base/src/main/java/org/wildfly/security/auth/server/MechanismConfiguration.java Outdated Show resolved Hide resolved

Skyllarr force-pushed the ely-2359 branch 3 times, most recently from 809be95 to 414d36f Compare May 8, 2024 09:19

[ELY-2359] Support HTTP Digest when fronted by load balancer

b260766

Skyllarr force-pushed the ely-2359 branch from 414d36f to b260766 Compare May 8, 2024 09:48

darranl added the Feature Request label Sep 15, 2024

[ELY-2359] Support HTTP Digest when fronted by load balancer #1909

Are you sure you want to change the base?

[ELY-2359] Support HTTP Digest when fronted by load balancer #1909

Conversation

Skyllarr commented May 30, 2023

Skyllarr commented Sep 5, 2023

fjuma left a 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

Skyllarr Dec 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Skyllarr commented Dec 5, 2023

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 commented Jan 3, 2024

Skyllarr commented Jan 10, 2024

Skyllarr commented Jan 30, 2024

Skyllarr commented Mar 7, 2024

darranl commented Mar 8, 2024

Skyllarr commented May 8, 2024

Skyllarr commented May 8, 2024

darranl commented Sep 15, 2024

Skyllarr Dec 5, 2023 •

edited

Loading