From 88f7dd12db08f302d4c78fb507ccb300e7270cb3 Mon Sep 17 00:00:00 2001 From: Michal Karm Babacek Date: Sun, 8 Dec 2019 01:56:25 +0100 Subject: [PATCH 1/3] Fixes issue 6011, adds tests for quarkus.http.auth.form.new-cookie-interval 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. Concatenates log messages, drops level from warn to debug --- .../security/FormAuthCookiesTestCase.java | 146 +++++++++++++++++- .../vertx/http/runtime/FormAuthConfig.java | 12 +- .../security/PersistentLoginManager.java | 34 ++-- 3 files changed, 175 insertions(+), 17 deletions(-) diff --git a/extensions/vertx-http/deployment/src/test/java/io/quarkus/vertx/http/security/FormAuthCookiesTestCase.java b/extensions/vertx-http/deployment/src/test/java/io/quarkus/vertx/http/security/FormAuthCookiesTestCase.java index 6623d50a1809c..35e2c489d5827 100644 --- a/extensions/vertx-http/deployment/src/test/java/io/quarkus/vertx/http/security/FormAuthCookiesTestCase.java +++ b/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"; @@ -89,4 +115,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 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."); + } + } + } } diff --git a/extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/FormAuthConfig.java b/extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/FormAuthConfig.java index 7b1699e8e1124..3f0a1803e0284 100644 --- a/extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/FormAuthConfig.java +++ b/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; diff --git a/extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/security/PersistentLoginManager.java b/extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/security/PersistentLoginManager.java index 2416d80de257f..f6da62631fb87 100644 --- a/extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/security/PersistentLoginManager.java +++ b/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,25 @@ 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. + log.debugf("%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, Expire idle timeout: %s, expireIdle - now is: %d - %d = %d", + new Date(now).toString(), new Date(expireIdle).toString(), 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 +112,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()); From 91e2d80c0493c3957290e65fb1fe727bc931162e Mon Sep 17 00:00:00 2001 From: Michal Karm Babacek Date: Tue, 18 Feb 2020 13:47:00 +0100 Subject: [PATCH 2/3] Pulls System.clearProperty from stuartwdouglas:graceful-shutdown --- .../quarkus/vertx/http/runtime/VertxHttpRecorder.java | 10 ++++++++++ .../src/main/java/io/quarkus/test/QuarkusUnitTest.java | 1 + 2 files changed, 11 insertions(+) diff --git a/extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/VertxHttpRecorder.java b/extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/VertxHttpRecorder.java index 7b2d5e934f98c..098171c3c01f3 100644 --- a/extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/VertxHttpRecorder.java +++ b/extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/VertxHttpRecorder.java @@ -564,6 +564,8 @@ private static class WebDeploymentVerticle extends AbstractVerticle { private final HttpServerOptions httpOptions; private final HttpServerOptions httpsOptions; private final LaunchMode launchMode; + private volatile boolean clearHttpProperty = false; + private volatile boolean clearHttpsProperty = false; public WebDeploymentVerticle(int port, int httpsPort, String host, HttpServerOptions httpOptions, HttpServerOptions httpsOptions, LaunchMode launchMode) { @@ -588,6 +590,7 @@ public void start(Future startFuture) { int actualPort = event.result().actualPort(); if (actualPort != port) { // Override quarkus.http.(test-)?port + clearHttpProperty = true; System.setProperty(launchMode == LaunchMode.TEST ? "quarkus.http.test-port" : "quarkus.http.port", String.valueOf(actualPort)); // Set in HttpOptions to output the port in the Timing class @@ -608,6 +611,7 @@ public void start(Future startFuture) { int actualPort = event.result().actualPort(); if (actualPort != httpsPort) { // Override quarkus.https.(test-)?port + clearHttpsProperty = true; System.setProperty(launchMode == LaunchMode.TEST ? "quarkus.https.test-port" : "quarkus.https.port", String.valueOf(actualPort)); // Set in HttpOptions to output the port in the Timing class @@ -623,6 +627,12 @@ public void start(Future startFuture) { @Override public void stop(Future stopFuture) { + if (clearHttpProperty) { + System.clearProperty(launchMode == LaunchMode.TEST ? "quarkus.http.test-port" : "quarkus.http.port"); + } + if (clearHttpsProperty) { + System.clearProperty(launchMode == LaunchMode.TEST ? "quarkus.https.test-port" : "quarkus.https.port"); + } httpServer.close(new Handler>() { @Override public void handle(AsyncResult event) { diff --git a/test-framework/junit5-internal/src/main/java/io/quarkus/test/QuarkusUnitTest.java b/test-framework/junit5-internal/src/main/java/io/quarkus/test/QuarkusUnitTest.java index 1273fe18b7edb..1f9ce585ca2aa 100644 --- a/test-framework/junit5-internal/src/main/java/io/quarkus/test/QuarkusUnitTest.java +++ b/test-framework/junit5-internal/src/main/java/io/quarkus/test/QuarkusUnitTest.java @@ -445,6 +445,7 @@ public void afterAll(ExtensionContext extensionContext) throws Exception { } curatedApplication.close(); } finally { + System.clearProperty("test.url"); Thread.currentThread().setContextClassLoader(originalClassLoader); timeoutTask.cancel(); timeoutTask = null; From 08e0b81524689f5cf3f0131c2b5c621b12c66a88 Mon Sep 17 00:00:00 2001 From: Michal Karm Babacek Date: Wed, 19 Feb 2020 11:39:42 +0100 Subject: [PATCH 3/3] Tuning the timer for cookie renewal, should help on very slow VMs --- .../security/FormAuthCookiesTestCase.java | 23 ++++++++++++++----- 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/extensions/vertx-http/deployment/src/test/java/io/quarkus/vertx/http/security/FormAuthCookiesTestCase.java b/extensions/vertx-http/deployment/src/test/java/io/quarkus/vertx/http/security/FormAuthCookiesTestCase.java index 35e2c489d5827..bab97de48d694 100644 --- a/extensions/vertx-http/deployment/src/test/java/io/quarkus/vertx/http/security/FormAuthCookiesTestCase.java +++ b/extensions/vertx-http/deployment/src/test/java/io/quarkus/vertx/http/security/FormAuthCookiesTestCase.java @@ -138,6 +138,13 @@ private void doRegularGet(CloseableHttpClient httpClient, CookieStore cookieStor } } + private void waitForPointInTime(long pointInFuture) throws InterruptedException { + long wait = pointInFuture - System.currentTimeMillis(); + assertTrue(wait > 0, "Having to wait for " + wait + + " ms for another request is unexpected. The previous one took too long."); + Thread.sleep(wait); + } + @TestHTTPResource URL url; @@ -168,15 +175,17 @@ public void testCredentialCookieRotation() throws IOException, InterruptedExcept assertTrue(StringUtils.isNotBlank(credentialCookieValue), "Credential cookie value must not be blank."); } - Thread.sleep(400); + long t0 = System.currentTimeMillis(); + + waitForPointInTime(t0 + 400); doRegularGet(httpClient, cookieStore, credentialCookieValue); - Thread.sleep(400); + waitForPointInTime(t0 + 700); doRegularGet(httpClient, cookieStore, credentialCookieValue); - Thread.sleep(400); + waitForPointInTime(t0 + 1300); HttpGet httpGet = new HttpGet(url.toString() + "/admin%E2%9D%A4"); try (CloseableHttpResponse adminResponse = httpClient.execute(httpGet)) { @@ -191,15 +200,17 @@ public void testCredentialCookieRotation() throws IOException, InterruptedExcept } - Thread.sleep(400); + t0 = System.currentTimeMillis(); + + waitForPointInTime(t0 + 400); doRegularGet(httpClient, cookieStore, credentialCookieValue); - Thread.sleep(400); + waitForPointInTime(t0 + 700); doRegularGet(httpClient, cookieStore, credentialCookieValue); - Thread.sleep(2000); + waitForPointInTime(t0 + 3600); httpGet = new HttpGet(url.toString() + "/admin%E2%9D%A4"); try (CloseableHttpResponse adminResponse = httpClient.execute(httpGet)) {