Fixes issue 6011, adds tests for quarkus.http.auth.form.new-cookie-in…

…terval

There are three generally accepted behaviors for timeout and renewal for credential session cookies.

1. [absolute-timeout](https://cheatsheetseries.owasp.org/cheatsheets/Session_Management_Cheat_Sheet.html#absolute-timeout)
1. [idle-timeout]( https://cheatsheetseries.owasp.org/cheatsheets/Session_Management_Cheat_Sheet.html#idle-timeout)
1. [renewal-timeout](https://cheatsheetseries.owasp.org/cheatsheets/Session_Management_Cheat_Sheet.html#renewal-timeout)

Quarkus implements 2. as **timeout** (```quarkus.http.auth.form.timeout```) and 3. as **newCookieInterval** (```quarkus.http.auth.form.new-cookie-interval```).

The implementation of 3. does not renew the cookie as expected.

The test does login, several requests and uses ```Thread.sleep(...);``` to pace them. I hope this is not deemed problematic for the stability of TS on very slow/weirdly behaving systems.
The margins are generous though, in hundreds of ms.

The test passes with the fixed calculation of cookie renewal and it fails with the current one:

```
org.opentest4j.AssertionFailedError:
   Session cookie WAS eligible for renewal and should have been updated.
at io.quarkus.vertx.http.security.FormAuthCookiesTestCase.
   testCredentialCookieRotation(FormAuthCookiesTestCase.java:183)
```

Thank you for feedback.

extensions/vertx-http/deployment/src/test/java/io/quarkus/vertx/http/security/FormAuthCookiesTestCase.java

@@ -1,9 +1,34 @@
 package io.quarkus.vertx.http.security;
 
-import static org.hamcrest.Matchers.*;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.notNullValue;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 
+import java.io.IOException;
+import java.net.URL;
+import java.util.ArrayList;
+import java.util.List;
 import java.util.function.Supplier;
 
+import org.apache.commons.lang3.StringUtils;
+import org.apache.http.Consts;
+import org.apache.http.NameValuePair;
+import org.apache.http.client.CookieStore;
+import org.apache.http.client.config.CookieSpecs;
+import org.apache.http.client.config.RequestConfig;
+import org.apache.http.client.entity.UrlEncodedFormEntity;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.client.methods.HttpPost;
+import org.apache.http.cookie.Cookie;
+import org.apache.http.impl.client.BasicCookieStore;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClientBuilder;
+import org.apache.http.message.BasicNameValuePair;
+import org.apache.http.util.EntityUtils;
 import org.jboss.shrinkwrap.api.ShrinkWrap;
 import org.jboss.shrinkwrap.api.asset.StringAsset;
 import org.jboss.shrinkwrap.api.spec.JavaArchive;
@@ -12,6 +37,7 @@
 import org.junit.jupiter.api.extension.RegisterExtension;
 
 import io.quarkus.test.QuarkusUnitTest;
+import io.quarkus.test.common.http.TestHTTPResource;
 import io.restassured.RestAssured;
 import io.restassured.filter.cookie.CookieFilter;
 
@@ -25,8 +51,8 @@ public class FormAuthCookiesTestCase {
             "quarkus.http.auth.policy.r1.roles-allowed=admin\n" +
             "quarkus.http.auth.permission.roles1.paths=/admin%E2%9D%A4\n" +
             "quarkus.http.auth.permission.roles1.policy=r1\n" +
-            "quarkus.http.auth.form.timeout=PT6S\n" +
-            "quarkus.http.auth.form.new-cookie-interval=PT2S\n" +
+            "quarkus.http.auth.form.timeout=PT2S\n" +
+            "quarkus.http.auth.form.new-cookie-interval=PT1S\n" +
             "quarkus.http.auth.form.cookie-name=laitnederc-sukrauq\n" +
             "quarkus.http.auth.session.encryption-key=CHANGEIT-CHANGEIT-CHANGEIT-CHANGEIT-CHANGEIT\n";
 
@@ -88,4 +114,118 @@ public void testFormBasedAuthSuccess() {
                 .body(equalTo("admin:/admin%E2%9D%A4"));
 
     }
+
+    private String getCredentialCookie(CookieStore cookieStore) {
+        for (Cookie cookie : cookieStore.getCookies()) {
+            if ("laitnederc-sukrauq".equals(cookie.getName())) {
+                return cookie.getValue();
+            }
+        }
+        return null;
+    }
+
+    private void doRegularGet(CloseableHttpClient httpClient, CookieStore cookieStore, String credentialCookieValue)
+            throws IOException {
+        HttpGet httpGet = new HttpGet(url.toString() + "/admin%E2%9D%A4");
+        try (CloseableHttpResponse adminResponse = httpClient.execute(httpGet)) {
+            String credentialInCookieStore = getCredentialCookie(cookieStore);
+            assertEquals(credentialCookieValue, credentialInCookieStore,
+                    "Session cookie WAS NOT eligible for renewal and should have remained the same.");
+            assertEquals(200, adminResponse.getStatusLine().getStatusCode(), "HTTP 200 expected.");
+            assertEquals("admin:/admin%E2%9D%A4", EntityUtils.toString(adminResponse.getEntity(), "UTF-8"),
+                    "Unexpected web page content.");
+        }
+    }
+
+    @TestHTTPResource
+    URL url;
+
+    @Test
+    public void testCredentialCookieRotation() throws IOException, InterruptedException {
+
+        final CookieStore cookieStore = new BasicCookieStore();
+
+        try (CloseableHttpClient httpClient = HttpClientBuilder.create()
+                .setDefaultCookieStore(cookieStore)
+                .setDefaultRequestConfig(RequestConfig.custom()
+                        .setCookieSpec(CookieSpecs.STANDARD).build())
+                .build()) {
+
+            final List<NameValuePair> authForm = new ArrayList<>();
+            authForm.add(new BasicNameValuePair("j_username", "admin"));
+            authForm.add(new BasicNameValuePair("j_password", "admin"));
+            final UrlEncodedFormEntity entity = new UrlEncodedFormEntity(authForm, Consts.UTF_8);
+
+            // Login
+            HttpPost httpPost = new HttpPost(url.toString() + "/j_security_check");
+            httpPost.setEntity(entity);
+            String credentialCookieValue = null;
+            try (CloseableHttpResponse loginResponse = httpClient.execute(httpPost)) {
+                assertEquals(302, loginResponse.getStatusLine().getStatusCode(),
+                        "Login should have been successful and return HTTP 302 redirect.");
+                credentialCookieValue = getCredentialCookie(cookieStore);
+                assertTrue(StringUtils.isNotBlank(credentialCookieValue), "Credential cookie value must not be blank.");
+            }
+
+            Thread.sleep(400);
+
+            doRegularGet(httpClient, cookieStore, credentialCookieValue);
+
+            Thread.sleep(400);
+
+            doRegularGet(httpClient, cookieStore, credentialCookieValue);
+
+            Thread.sleep(400);
+
+            HttpGet httpGet = new HttpGet(url.toString() + "/admin%E2%9D%A4");
+            try (CloseableHttpResponse adminResponse = httpClient.execute(httpGet)) {
+                String credentialInCookieStore = getCredentialCookie(cookieStore);
+                assertNotEquals(credentialCookieValue, credentialInCookieStore,
+                        "Session cookie WAS eligible for renewal and should have been updated.");
+                assertEquals(200, adminResponse.getStatusLine().getStatusCode(), "HTTP 200 expected.");
+                assertEquals("admin:/admin%E2%9D%A4", EntityUtils.toString(adminResponse.getEntity(), "UTF-8"),
+                        "Unexpected web page content.");
+
+                credentialCookieValue = credentialInCookieStore;
+
+            }
+
+            Thread.sleep(400);
+
+            doRegularGet(httpClient, cookieStore, credentialCookieValue);
+
+            Thread.sleep(400);
+
+            doRegularGet(httpClient, cookieStore, credentialCookieValue);
+
+            Thread.sleep(2000);
+
+            httpGet = new HttpGet(url.toString() + "/admin%E2%9D%A4");
+            try (CloseableHttpResponse adminResponse = httpClient.execute(httpGet)) {
+                assertEquals(200, adminResponse.getStatusLine().getStatusCode(), "HTTP 200 from login page expected.");
+                assertEquals(":/login", EntityUtils.toString(adminResponse.getEntity(), "UTF-8"),
+                        "Login web page was expected. Quarkus should have enforced a new login.");
+                String redirectLocation = null;
+                for (Cookie cookie : cookieStore.getCookies()) {
+                    if ("quarkus-redirect-location".equals(cookie.getName())) {
+                        redirectLocation = cookie.getValue();
+                        break;
+                    }
+                }
+                assertTrue(StringUtils.isNotBlank(redirectLocation) && redirectLocation.contains("admin%E2%9D%A4"),
+                        "quarkus-redirect-location should have been set.");
+            }
+
+            httpPost = new HttpPost(url.toString() + "/j_security_check");
+            httpPost.setEntity(entity);
+            try (CloseableHttpResponse loginResponse = httpClient.execute(httpPost)) {
+                assertEquals(302, loginResponse.getStatusLine().getStatusCode(),
+                        "Login should have been successful and return HTTP 302 redirect.");
+                String newCredentialCookieValue = getCredentialCookie(cookieStore);
+                assertTrue(StringUtils.isNotBlank(newCredentialCookieValue), "Credential cookie value must not be blank.");
+                assertNotEquals(newCredentialCookieValue, credentialCookieValue,
+                        "New credential cookie must not be the same as the previous one.");
+            }
+        }
+    }
 }

extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/FormAuthConfig.java

@@ -42,21 +42,27 @@ public class FormAuthConfig {
     public boolean redirectAfterLogin;
 
     /**
-     * The inactivity timeout
+     * The inactivity (idle) timeout
+     *
+     * When inactivity timeout is reached, cookie is not renewed and a new login is enforced.
      */
     @ConfigItem(defaultValue = "PT30M")
     public Duration timeout;
 
     /**
-     * How old a cookie can get before it will be replaced with a new cookie with an updated timeout.
+     * How old a cookie can get before it will be replaced with a new cookie with an updated timeout, also
+     * referred to as "renewal-timeout".
      *
-     * Not that smaller values will result in slightly more server load (as new encrypted cookies will be
+     * Note that smaller values will result in slightly more server load (as new encrypted cookies will be
      * generated more often), however larger values affect the inactivity timeout as the timeout is set
      * when a cookie is generated.
      *
      * For example if this is set to 10 minutes, and the inactivity timeout is 30m, if a users last request
      * is when the cookie is 9m old then the actual timeout will happen 21m after the last request, as the timeout
      * is only refreshed when a new cookie is generated.
+     *
+     * In other words no timeout is tracked on the server side; the timestamp is encoded and encrypted in the cookie itself
+     * and it is decrypted and parsed with each request.
      */
     @ConfigItem(defaultValue = "PT1M")
     public Duration newCookieInterval;

extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/security/PersistentLoginManager.java

@@ -5,6 +5,7 @@
 import java.security.MessageDigest;
 import java.security.SecureRandom;
 import java.util.Base64;
+import java.util.Date;
 
 import javax.crypto.Cipher;
 import javax.crypto.KeyGenerator;
@@ -20,9 +21,7 @@
 
 /**
  * A class that manages persistent logins.
- *
  * This is done by encoding an expiry time, and the current username into an encrypted cookie
- *
  * TODO: make this pluggable
  */
 public class PersistentLoginManager {
@@ -35,31 +34,32 @@ public class PersistentLoginManager {
     private final String cookieName;
     private final long timeoutMillis;
     private final SecureRandom secureRandom = new SecureRandom();
-    private final long newCookieMillis;
+    private final long newCookieIntervalMillis;
 
-    public PersistentLoginManager(String encryptionKey, String cookieName, long timeoutMillis, long newCookieMillis) {
+    public PersistentLoginManager(String encryptionKey, String cookieName, long timeoutMillis, long newCookieIntervalMillis) {
         try {
             this.cookieName = cookieName;
-            this.newCookieMillis = newCookieMillis;
+            this.newCookieIntervalMillis = newCookieIntervalMillis;
             this.timeoutMillis = timeoutMillis;
             if (encryptionKey == null) {
-                secretKey = KeyGenerator.getInstance("AES").generateKey();
+                this.secretKey = KeyGenerator.getInstance("AES").generateKey();
             } else if (encryptionKey.length() < 16) {
                 throw new RuntimeException("Shared keys for persistent logins must be more than 16 characters long");
             } else {
                 MessageDigest sha256 = MessageDigest.getInstance("SHA-256");
                 sha256.update(encryptionKey.getBytes(StandardCharsets.UTF_8));
-                secretKey = new SecretKeySpec(sha256.digest(), "AES");
+                this.secretKey = new SecretKeySpec(sha256.digest(), "AES");
             }
         } catch (Exception t) {
             throw new RuntimeException(t);
         }
-
     }
 
     public RestoreResult restore(RoutingContext context) {
         Cookie existing = context.getCookie(cookieName);
+        // If there is no credential cookie, we have nothing to restore.
         if (existing == null) {
+            // Enforce new login.
             return null;
         }
         String val = existing.getValue();
@@ -74,14 +74,27 @@ public RestoreResult restore(RoutingContext context) {
             cipher.init(Cipher.DECRYPT_MODE, secretKey, new GCMParameterSpec(ENC_TAG_LENGTH, iv));
             String result = new String(cipher.doFinal(encrypted), StandardCharsets.UTF_8);
             int sep = result.indexOf(":");
+            // If parsing fails, something is wrong and we need to enforce a new login.
             if (sep == -1) {
+                // Enforce new login.
+                // TODO: Do we log anything? Potentially flooding the log by bots...
+                log.warnf("%s cookie parsing failed. Is encryption-key set for all instances?", cookieName);
                 return null;
             }
-            long expire = Long.parseLong(result.substring(0, sep));
-            if (System.currentTimeMillis() > expire) {
+            long expireIdle = Long.parseLong(result.substring(0, sep));
+            long now = System.currentTimeMillis();
+            log.debugf("Current time: %s", new Date(now).toString());
+            log.debugf("Expire idle timeout: %s", new Date(expireIdle).toString());
+            log.debugf("expireIdle - now is: %d - %d = %d", expireIdle, now, expireIdle - now);
+            // We don't attempt renewal, idle timeout already expired.
+            if (now > expireIdle) {
+                // Enforce new login.
                 return null;
             }
-            return new RestoreResult(result.substring(sep + 1), (System.currentTimeMillis() - expire) > newCookieMillis);
+            boolean newCookieNeeded = (timeoutMillis - (expireIdle - now)) > newCookieIntervalMillis;
+            log.debugf("Is new cookie needed? ( %d - ( %d - %d)) > %d : %b", timeoutMillis, expireIdle, now,
+                    newCookieIntervalMillis, newCookieNeeded);
+            return new RestoreResult(result.substring(sep + 1), newCookieNeeded);
         } catch (Exception e) {
             log.debug("Failed to restore persistent user session", e);
             return null;
@@ -101,6 +114,7 @@ public void save(SecurityIdentity identity, RoutingContext context, RestoreResul
             cipher.init(Cipher.ENCRYPT_MODE, secretKey, new GCMParameterSpec(ENC_TAG_LENGTH, iv));
             StringBuilder contents = new StringBuilder();
             long timeout = System.currentTimeMillis() + timeoutMillis;
+            log.debugf("The new cookie will expire at %s", new Date(timeout).toString());
             contents.append(timeout);
             contents.append(":");
             contents.append(identity.getPrincipal().getName());