Repository: helidon-io/helidon (java)

Issue:
Review ConcurrentHashMap usage in helidon-security-providers-oidc 
## Problem Description
Thread pinning can occur if the mapping function passed to any of the `compute` methods in `ConcurrentHashMap` results in any I/O. The internal implementation of `ConcurrentHashMap` will hold a lock while the VT is parked, causing the pinning. 

Note that I/O in the mapping function can be indirect: for example, using config or a service loader. Please verify that the following calls will never need I/O to complete:

```
OidcProvider:506
```

If required, re-implement the map using a traditional reentrant lock and a hash map.



Pull request:
4.x: Correctly guard concurrent access to hash map
Resolves #8972 

Patch:
diff --git a/http/http/src/main/java/io/helidon/http/SetCookie.java b/http/http/src/main/java/io/helidon/http/SetCookie.java
index efa15b0b8d..536620a4a2 100644
--- a/http/http/src/main/java/io/helidon/http/SetCookie.java
+++ b/http/http/src/main/java/io/helidon/http/SetCookie.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2018, 2023 Oracle and/or its affiliates.
+ * Copyright (c) 2018, 2024 Oracle and/or its affiliates.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -21,7 +21,9 @@ import java.time.Duration;
 import java.time.Instant;
 import java.time.ZoneId;
 import java.time.ZonedDateTime;
+import java.util.Locale;
 import java.util.Objects;
+import java.util.Optional;
 
 /**
  * Represents {@code 'Set-Cookie'} header value specified by <a href="https://tools.ietf.org/html/rfc6265">RFC6265</a>.
@@ -54,15 +56,6 @@ public class SetCookie {
         this.sameSite = builder.sameSite;
     }
 
-    /**
-     * Gets cookie's name.
-     *
-     * @return the name.
-     */
-    public String name() {
-        return name;
-    }
-
     /**
      * Creates a new fluent API builder.
      *
@@ -125,6 +118,10 @@ public class SetCookie {
                 hasNoValue(partName, partValue);
                 builder.httpOnly(true);
                 break;
+            case "samesite":
+                hasValue(partName, partValue);
+                builder.sameSite(SameSite.valueOf(partValue.toUpperCase(Locale.ROOT)));
+                break;
             default:
                 throw new IllegalArgumentException("Unexpected Set-Cookie part: " + partName);
             }
@@ -133,36 +130,105 @@ public class SetCookie {
     }
 
     /**
-     * Text representation of this cookie.
+     * Creates new instance.
      *
-     * @return cookie text
+     * @param name  a cookie name.
+     * @param value a cookie value.
+     * @return a new instance with just the name and value configured
      */
-    public String text() {
-        return toString();
+    public static SetCookie create(String name, String value) {
+        return builder(name, value)
+                .build();
     }
 
-    private static void hasNoValue(String partName, String partValue) {
-        if (partValue != null) {
-            throw new IllegalArgumentException("Set-Cookie parameter " + partName + " has to have no value!");
-        }
+    /**
+     * Name of the cookie.
+     *
+     * @return the name.
+     */
+    public String name() {
+        return name;
     }
 
-    private static void hasValue(String partName, String partValue) {
-        if (partValue == null) {
-            throw new IllegalArgumentException("Set-Cookie parameter " + partName + " has to have a value!");
-        }
+    /**
+     * Value of the cookie.
+     *
+     * @return value
+     */
+    public String value() {
+        return value;
     }
 
     /**
-     * Creates new instance.
+     * Expiration of cookie.
      *
-     * @param name  a cookie name.
-     * @param value a cookie value.
-     * @return a new instance with just the name and value configured
+     * @return expiration if defined
      */
-    public static SetCookie create(String name, String value) {
-        return builder(name, value)
-                .build();
+    public Optional<ZonedDateTime> expires() {
+        return Optional.ofNullable(expires);
+    }
+
+    /**
+     * Max age of cookie.
+     *
+     * @return max age if defined
+     */
+    public Optional<Duration> maxAge() {
+        return Optional.ofNullable(maxAge);
+    }
+
+    /**
+     * Domain of cookie.
+     *
+     * @return domain if defined
+     */
+    public Optional<String> domain() {
+        return Optional.ofNullable(domain);
+    }
+
+    /**
+     * Path of cookie.
+     *
+     * @return path if defined
+     */
+    public Optional<String> path() {
+        return Optional.ofNullable(path);
+    }
+
+    /**
+     * Secure attribute of cookie.
+     *
+     * @return whether secure was set
+     */
+    public boolean secure() {
+        return secure;
+    }
+
+    /**
+     * HttpOnly attribute of cookie.
+     *
+     * @return whether {@code HttpOnly} was set
+     */
+    public boolean httpOnly() {
+        return httpOnly;
+    }
+
+    /**
+     * Same site attribute of cookie.
+     *
+     * @return same site if defined
+     */
+    public Optional<SameSite> sameSite() {
+        return Optional.ofNullable(sameSite);
+    }
+
+    /**
+     * Text representation of this cookie.
+     *
+     * @return cookie text
+     */
+    public String text() {
+        return toString();
     }
 
     /**
@@ -211,6 +277,57 @@ public class SetCookie {
         return result.toString();
     }
 
+    private static void hasNoValue(String partName, String partValue) {
+        if (partValue != null) {
+            throw new IllegalArgumentException("Set-Cookie parameter " + partName + " has to have no value!");
+        }
+    }
+
+    private static void hasValue(String partName, String partValue) {
+        if (partValue == null) {
+            throw new IllegalArgumentException("Set-Cookie parameter " + partName + " has to have a value!");
+        }
+    }
+
+    /**
+     * The SameSite attribute of the Set-Cookie HTTP response header allows you to declare if your cookie should be restricted
+     * to a first-party or same-site context.
+     */
+    public enum SameSite {
+        /**
+         * Cookies are not sent on normal cross-site subrequests (for example to load images or frames into a third party site)
+         * , but are sent when a user is navigating to the origin site (i.e., when following a link).
+         *
+         * This is the default cookie value if SameSite has not been explicitly specified in recent browser versions
+         */
+        LAX("Lax"),
+        /**
+         * Cookies will only be sent in a first-party context and not be sent along with requests initiated by third party
+         * websites.
+         */
+        STRICT("Strict"),
+        /**
+         * Cookies will be sent in all contexts, i.e. in responses to both first-party and cross-origin requests. If
+         * SameSite=None is set, the cookie Secure attribute must also be set (or the cookie will be blocked).
+         */
+        NONE("None");
+
+        private final String text;
+
+        SameSite(String text) {
+            this.text = text;
+        }
+
+        /**
+         * Text to write to the same site cookie param.
+         *
+         * @return text to send in cookie
+         */
+        public String text() {
+            return text;
+        }
+    }
+
     /**
      * A fluent API builder for {@link SetCookie}.
      */
@@ -346,43 +463,4 @@ public class SetCookie {
             return this;
         }
     }
-
-    /**
-     * The SameSite attribute of the Set-Cookie HTTP response header allows you to declare if your cookie should be restricted
-     * to a first-party or same-site context.
-     */
-    public enum SameSite {
-        /**
-         * Cookies are not sent on normal cross-site subrequests (for example to load images or frames into a third party site)
-         * , but are sent when a user is navigating to the origin site (i.e., when following a link).
-         *
-         * This is the default cookie value if SameSite has not been explicitly specified in recent browser versions
-         */
-        LAX("Lax"),
-        /**
-         * Cookies will only be sent in a first-party context and not be sent along with requests initiated by third party
-         * websites.
-         */
-        STRICT("Strict"),
-        /**
-         * Cookies will be sent in all contexts, i.e. in responses to both first-party and cross-origin requests. If
-         * SameSite=None is set, the cookie Secure attribute must also be set (or the cookie will be blocked).
-         */
-        NONE("None");
-
-        private final String text;
-
-        SameSite(String text) {
-            this.text = text;
-        }
-
-        /**
-         * Text to write to the same site cookie param.
-         *
-         * @return text to send in cookie
-         */
-        public String text() {
-            return text;
-        }
-    }
 }
diff --git a/microprofile/jwt-auth/src/main/java/io/helidon/microprofile/jwt/auth/JwtAuthProvider.java b/microprofile/jwt-auth/src/main/java/io/helidon/microprofile/jwt/auth/JwtAuthProvider.java
index 194e897908..4daa1767a8 100644
--- a/microprofile/jwt-auth/src/main/java/io/helidon/microprofile/jwt/auth/JwtAuthProvider.java
+++ b/microprofile/jwt-auth/src/main/java/io/helidon/microprofile/jwt/auth/JwtAuthProvider.java
@@ -46,6 +46,7 @@ import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 import java.util.Set;
+import java.util.concurrent.locks.ReentrantLock;
 import java.util.function.Supplier;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
@@ -145,6 +146,7 @@ public class JwtAuthProvider implements AuthenticationProvider, OutboundSecurity
     private final LazyValue<Jwk> defaultJwk;
     private final LazyValue<Jwk> defaultDecryptionJwk;
     private final Map<OutboundTarget, JwtOutboundTarget> targetToJwtConfig = new IdentityHashMap<>();
+    private final ReentrantLock targetToJwtConfigLock = new ReentrantLock();
     private final String expectedIssuer;
     private final String cookiePrefix;
     private final String decryptionKeyAlgorithm;
@@ -394,7 +396,13 @@ public class JwtAuthProvider implements AuthenticationProvider, OutboundSecurity
                     Optional<OutboundTarget> maybeTarget = outboundConfig.findTarget(outboundEnv);
 
                     return maybeTarget.flatMap(target -> {
-                        JwtOutboundTarget jwtOutboundTarget = targetToJwtConfig.computeIfAbsent(target, this::toOutboundTarget);
+                        JwtOutboundTarget jwtOutboundTarget;
+                        try {
+                            targetToJwtConfigLock.lock();
+                            jwtOutboundTarget = targetToJwtConfig.computeIfAbsent(target, this::toOutboundTarget);
+                        } finally {
+                            targetToJwtConfigLock.unlock();
+                        }
 
                         if (null == jwtOutboundTarget.jwkKid) {
                             return Optional.of(OutboundSecurityResponse.builder()
@@ -418,8 +426,13 @@ public class JwtAuthProvider implements AuthenticationProvider, OutboundSecurity
                         Optional<OutboundTarget> maybeTarget = outboundConfig.findTarget(outboundEnv);
 
                         return maybeTarget.flatMap(target -> {
-                            JwtOutboundTarget jwtOutboundTarget = targetToJwtConfig
-                                    .computeIfAbsent(target, this::toOutboundTarget);
+                            JwtOutboundTarget jwtOutboundTarget;
+                            try {
+                                targetToJwtConfigLock.lock();
+                                jwtOutboundTarget = targetToJwtConfig.computeIfAbsent(target, this::toOutboundTarget);
+                            } finally {
+                                targetToJwtConfigLock.unlock();
+                            }
 
                             if (null == jwtOutboundTarget.jwkKid) {
                                 // just propagate existing token
diff --git a/security/providers/oidc/src/main/java/io/helidon/security/providers/oidc/OidcProvider.java b/security/providers/oidc/src/main/java/io/helidon/security/providers/oidc/OidcProvider.java
index 7023d1ac18..05f0ecfea5 100644
--- a/security/providers/oidc/src/main/java/io/helidon/security/providers/oidc/OidcProvider.java
+++ b/security/providers/oidc/src/main/java/io/helidon/security/providers/oidc/OidcProvider.java
@@ -26,6 +26,7 @@ import java.util.Map;
 import java.util.Optional;
 import java.util.ServiceLoader;
 import java.util.Set;
+import java.util.concurrent.locks.ReentrantLock;
 import java.util.stream.Collectors;
 
 import io.helidon.common.HelidonServiceLoader;
@@ -485,6 +486,7 @@ public final class OidcProvider implements AuthenticationProvider, OutboundSecur
     }
 
     private static final class OidcOutboundConfig {
+        private final ReentrantLock targetCacheLock = new ReentrantLock();
         private final Map<OutboundTarget, OidcOutboundTarget> targetCache = new HashMap<>();
         private final OutboundConfig outboundConfig;
         private final TokenHandler defaultTokenHandler;
@@ -503,17 +505,24 @@ public final class OidcProvider implements AuthenticationProvider, OutboundSecur
 
         private OidcOutboundTarget findTarget(SecurityEnvironment env) {
             return outboundConfig.findTarget(env)
-                    .map(value -> targetCache.computeIfAbsent(value, outboundTarget -> {
-                        boolean propagate = outboundTarget.getConfig()
-                                .flatMap(cfg -> cfg.get("propagate").asBoolean().asOptional())
-                                .orElse(true);
-                        TokenHandler handler = outboundTarget.getConfig()
-                                .flatMap(cfg -> cfg.get("outbound-token")
-                                        .map(TokenHandler::create)
-                                        .asOptional())
-                                .orElse(defaultTokenHandler);
-                        return new OidcOutboundTarget(propagate, handler);
-                    })).orElse(defaultTarget);
+                    .map(value -> {
+                        try {
+                            targetCacheLock.lock();
+                            return targetCache.computeIfAbsent(value, outboundTarget -> {
+                                boolean propagate = outboundTarget.getConfig()
+                                        .flatMap(cfg -> cfg.get("propagate").asBoolean().asOptional())
+                                        .orElse(true);
+                                TokenHandler handler = outboundTarget.getConfig()
+                                        .flatMap(cfg -> cfg.get("outbound-token")
+                                                .map(TokenHandler::create)
+                                                .asOptional())
+                                        .orElse(defaultTokenHandler);
+                                return new OidcOutboundTarget(propagate, handler);
+                            });
+                        } finally {
+                            targetCacheLock.unlock();
+                        }
+                    }).orElse(defaultTarget);
         }
     }
 


Test patch:
diff --git a/http/http/src/test/java/io/helidon/http/SetCookieTest.java b/http/http/src/test/java/io/helidon/http/SetCookieTest.java
index 956addb3ad..d9e8c56488 100644
--- a/http/http/src/test/java/io/helidon/http/SetCookieTest.java
+++ b/http/http/src/test/java/io/helidon/http/SetCookieTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2020, 2023 Oracle and/or its affiliates.
+ * Copyright (c) 2020, 2024 Oracle and/or its affiliates.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -15,8 +15,11 @@
  */
 package io.helidon.http;
 
+import java.time.Duration;
+
 import org.junit.jupiter.api.Test;
 
+import static io.helidon.common.testing.junit5.OptionalMatcher.optionalValue;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.is;
 import static org.junit.jupiter.api.Assertions.assertThrows;
@@ -34,9 +37,21 @@ public class SetCookieTest {
                 + "Domain=domain.value; "
                 + "Path=/; "
                 + "Secure; "
-                + "HttpOnly";
+                + "HttpOnly; "
+                + "SameSite=Lax";
         SetCookie setCookie = SetCookie.parse(template);
-        assertThat(setCookie.toString(), is(template));
+
+        assertThat(setCookie.name(), is("some-cookie"));
+        assertThat(setCookie.value(), is("some-cookie-value"));
+        assertThat(setCookie.expires(), optionalValue(is(DateTime.parse("Thu, 22 Oct 2015 07:28:00 GMT"))));
+        assertThat(setCookie.maxAge(), optionalValue(is(Duration.ofSeconds(2592000))));
+        assertThat(setCookie.domain(), optionalValue(is("domain.value")));
+        assertThat(setCookie.path(), optionalValue(is("/")));
+        assertThat(setCookie.secure(), is(true));
+        assertThat(setCookie.httpOnly(), is(true));
+        assertThat(setCookie.sameSite(), optionalValue(is(SetCookie.SameSite.LAX)));
+
+        assertThat("Generate same cookie value", setCookie.toString(), is(template));
     }
 
     @Test