From fe5b7be7f648f76e7e23a6f1f4932cc4deb2c62e Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Thu, 24 Jan 2019 14:39:31 +0100 Subject: [PATCH 1/2] Fixes #3293 HttpClientTLSTest.testNoCommonTLSProtocol() fail with jdk 11.0.2. Updated to force TLS 1.3 vs TLS 1.2, rather TLS 1.2 vs TLS 1.1. Signed-off-by: Simone Bordet --- .../jetty/client/HttpClientTLSTest.java | 42 +++++++++---------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTLSTest.java b/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTLSTest.java index ecb7789efb7..3d86f7ded42 100644 --- a/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTLSTest.java +++ b/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTLSTest.java @@ -18,15 +18,6 @@ package org.eclipse.jetty.client; -import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.instanceOf; -import static org.junit.jupiter.api.Assertions.assertArrayEquals; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertNotNull; -import static org.junit.jupiter.api.Assertions.assertThrows; -import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.junit.jupiter.api.Assumptions.assumeTrue; - import java.io.BufferedReader; import java.io.InputStreamReader; import java.io.OutputStream; @@ -53,17 +44,22 @@ import org.eclipse.jetty.io.ssl.SslHandshakeListener; import org.eclipse.jetty.server.Handler; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.ServerConnector; -import org.eclipse.jetty.util.JavaVersion; import org.eclipse.jetty.util.ssl.SslContextFactory; import org.eclipse.jetty.util.thread.ExecutorThreadPool; import org.eclipse.jetty.util.thread.QueuedThreadPool; -import org.hamcrest.Matchers; import org.junit.jupiter.api.AfterEach; -import org.junit.jupiter.api.Assumptions; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.condition.DisabledOnJre; import org.junit.jupiter.api.condition.JRE; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.instanceOf; +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + public class HttpClientTLSTest { private Server server; @@ -114,7 +110,7 @@ public class HttpClientTLSTest public void testNoCommonTLSProtocol() throws Exception { SslContextFactory serverTLSFactory = createSslContextFactory(); - serverTLSFactory.setIncludeProtocols("TLSv1.2"); + serverTLSFactory.setIncludeProtocols("TLSv1.3"); startServer(serverTLSFactory, new EmptyServerHandler()); CountDownLatch serverLatch = new CountDownLatch(1); @@ -128,7 +124,7 @@ public class HttpClientTLSTest }); SslContextFactory clientTLSFactory = createSslContextFactory(); - clientTLSFactory.setIncludeProtocols("TLSv1.1"); + clientTLSFactory.setIncludeProtocols("TLSv1.2"); startClient(clientTLSFactory); CountDownLatch clientLatch = new CountDownLatch(1); @@ -141,7 +137,8 @@ public class HttpClientTLSTest } }); - assertThrows(ExecutionException.class, ()->{ + assertThrows(ExecutionException.class, () -> + { client.newRequest("localhost", connector.getLocalPort()) .scheme(HttpScheme.HTTPS.asString()) .timeout(5, TimeUnit.SECONDS) @@ -183,7 +180,8 @@ public class HttpClientTLSTest } }); - assertThrows(ExecutionException.class, ()->{ + assertThrows(ExecutionException.class, () -> + { client.newRequest("localhost", connector.getLocalPort()) .scheme(HttpScheme.HTTPS.asString()) .timeout(5, TimeUnit.SECONDS) @@ -226,7 +224,8 @@ public class HttpClientTLSTest } }); - assertThrows(ExecutionException.class, ()->{ + assertThrows(ExecutionException.class, () -> + { client.newRequest("localhost", connector.getLocalPort()) .scheme(HttpScheme.HTTPS.asString()) .timeout(5, TimeUnit.SECONDS) @@ -239,7 +238,7 @@ public class HttpClientTLSTest // In JDK 11, a mismatch on the client does not generate any bytes towards // the server, while in TLS 1.2 the client sends to the server the close_notify. - @DisabledOnJre( JRE.JAVA_11 ) + @DisabledOnJre(JRE.JAVA_11) @Test public void testMismatchBetweenTLSProtocolAndTLSCiphersOnClient() throws Exception { @@ -273,7 +272,8 @@ public class HttpClientTLSTest } }); - assertThrows(ExecutionException.class, ()->{ + assertThrows(ExecutionException.class, () -> + { client.newRequest("localhost", connector.getLocalPort()) .scheme(HttpScheme.HTTPS.asString()) .timeout(5, TimeUnit.SECONDS) @@ -321,7 +321,7 @@ public class HttpClientTLSTest } // Excluded because of a bug in JDK 11+27 where session resumption does not work. - @DisabledOnJre( JRE.JAVA_11 ) + @DisabledOnJre(JRE.JAVA_11) @Test public void testHandshakeSucceededWithSessionResumption() throws Exception { @@ -401,7 +401,7 @@ public class HttpClientTLSTest } // Excluded because of a bug in JDK 11+27 where session resumption does not work. - @DisabledOnJre( JRE.JAVA_11 ) + @DisabledOnJre(JRE.JAVA_11) @Test public void testClientRawCloseDoesNotInvalidateSession() throws Exception { From 169d93e790e69b7d23a2e3b9d5a6a35e6d2b55ce Mon Sep 17 00:00:00 2001 From: Jan Bartel Date: Wed, 30 Jan 2019 14:02:35 +1100 Subject: [PATCH 2/2] Jetty 9.4.x 3284 sessions and system clock jitter (#3296) * Issue 3284 System clock jitter can affect session timers. Signed-off-by: Jan Bartel --- .../server/session/AbstractSessionCache.java | 6 - .../eclipse/jetty/server/session/Session.java | 111 +++++++++++------- .../jetty/server/session/SessionHandler.java | 61 ++++++---- 3 files changed, 109 insertions(+), 69 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSessionCache.java b/jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSessionCache.java index 5520a661bfd..15b47c8699d 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSessionCache.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSessionCache.java @@ -616,12 +616,10 @@ public abstract class AbstractSessionCache extends ContainerLifeCycle implements { //get the session, if its not in memory, this will load it Session session = get(id); - //Always delete it from the backing data store if (_sessionDataStore != null) { - boolean dsdel = _sessionDataStore.delete(id); if (LOG.isDebugEnabled()) LOG.debug("Session {} deleted in session data store {}",id, dsdel); } @@ -635,10 +633,6 @@ public abstract class AbstractSessionCache extends ContainerLifeCycle implements return doDelete(id); } - - - - /** * @see org.eclipse.jetty.server.session.SessionCache#checkExpiration(Set) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/session/Session.java b/jetty-server/src/main/java/org/eclipse/jetty/server/session/Session.java index c7dc82c5d7e..a34bc0f0cdb 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/session/Session.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/session/Session.java @@ -116,7 +116,6 @@ public class Session implements SessionHandler.SessionIf public class SessionInactivityTimer { protected final CyclicTimeout _timer; - protected long _msec = -1; public SessionInactivityTimer() { @@ -127,31 +126,45 @@ public class Session implements SessionHandler.SessionIf { if (LOG.isDebugEnabled()) LOG.debug("Timer expired for session {}", getId()); - getSessionHandler().sessionInactivityTimerExpired(Session.this); + long now = System.currentTimeMillis(); + //handle what to do with the session after the timer expired + getSessionHandler().sessionInactivityTimerExpired(Session.this, now); + try (Lock lock = Session.this.lock()) + { + //grab the lock and check what happened to the session: if it didn't get evicted and + //it hasn't expired, we need to reset the timer + if (Session.this.isResident() && Session.this.getRequests() <= 0 && Session.this.isValid() && !Session.this.isExpiredAt(now)) + { + //session wasn't expired or evicted, we need to reset the timer + SessionInactivityTimer.this.schedule(Session.this.calculateInactivityTimeout(now)); + } + } } }; } - /** - * @param ms the timeout to set; -1 means that the timer will not be - * scheduled + * For backward api compatibility only. + * @see #schedule(long) */ - public void setTimeout(long ms) + @Deprecated + public void schedule () { - _msec = ms; - if (LOG.isDebugEnabled()) - LOG.debug("Session {} timer={}ms", getId(), ms); + schedule(calculateInactivityTimeout(System.currentTimeMillis())); } - public void schedule() + /** + * @param time the timeout to set; -1 means that the timer will not be + * scheduled + */ + public void schedule (long time) { - if (_msec > 0) + if (time >= 0) { if (LOG.isDebugEnabled()) - LOG.debug("(Re)starting timer for session {} at {}ms", getId(), _msec); - _timer.schedule(_msec, TimeUnit.MILLISECONDS); + LOG.debug("(Re)starting timer for session {} at {}ms", getId(), time); + _timer.schedule(time, TimeUnit.MILLISECONDS); } else { @@ -279,9 +292,15 @@ public class Session implements SessionHandler.SessionIf if (LOG.isDebugEnabled()) LOG.debug("Session {} complete, active requests={}", getId(), _requests); - // start the inactivity timer + // start the inactivity timer if necessary if (_requests == 0) - _sessionInactivityTimer.schedule(); + { + //update the expiry time to take account of the time all requests spent inside of the + //session. + long now = System.currentTimeMillis(); + _sessionData.calcAndSetExpiry(now); + _sessionInactivityTimer.schedule(calculateInactivityTimeout(now)); + } } } @@ -513,7 +532,7 @@ public class Session implements SessionHandler.SessionIf _sessionData.setMaxInactiveMs((long) secs * 1000L); _sessionData.calcAndSetExpiry(); _sessionData.setDirty(true); - updateInactivityTimer(); + if (LOG.isDebugEnabled()) { if (secs <= 0) @@ -524,14 +543,29 @@ public class Session implements SessionHandler.SessionIf } } - /** - * Set the inactivity timer to the smaller of the session maxInactivity (ie - * session-timeout from web.xml), or the inactive eviction time. - */ - public void updateInactivityTimer() + + @Deprecated + public void updateInactivityTimer() { + //for backward api compatibility only + } + + /** + * Calculate what the session timer setting should be based on: + * the time remaining before the session expires + * and any idle eviction time configured. + * The timer value will be the lesser of the above. + * + * @param now the time at which to calculate remaining expiry + * @return the time remaining before expiry or inactivity timeout + */ + public long calculateInactivityTimeout (long now) + { + long time = 0; + try (Lock lock = _lock.lock()) { + long remaining = _sessionData.getExpiry() - now; long maxInactive = _sessionData.getMaxInactiveMs(); int evictionPolicy = getSessionHandler().getSessionCache().getEvictionPolicy(); @@ -541,7 +575,7 @@ public class Session implements SessionHandler.SessionIf if (evictionPolicy < SessionCache.EVICT_ON_INACTIVITY) { // we do not want to evict inactive sessions - _sessionInactivityTimer.setTimeout(-1); + time = -1; if (LOG.isDebugEnabled()) LOG.debug("Session {} is immortal && no inactivity eviction", getId()); } @@ -549,7 +583,7 @@ public class Session implements SessionHandler.SessionIf { // sessions are immortal but we want to evict after // inactivity - _sessionInactivityTimer.setTimeout(TimeUnit.SECONDS.toMillis(evictionPolicy)); + time = TimeUnit.SECONDS.toMillis(evictionPolicy); if (LOG.isDebugEnabled()) LOG.debug("Session {} is immortal; evict after {} sec inactivity", getId(), evictionPolicy); } @@ -559,31 +593,33 @@ public class Session implements SessionHandler.SessionIf // sessions are not immortal if (evictionPolicy == SessionCache.NEVER_EVICT) { - // timeout is just the maxInactive setting - _sessionInactivityTimer.setTimeout(_sessionData.getMaxInactiveMs()); + // timeout is the time remaining until its expiry + time = (remaining > 0 ? remaining : 0); if (LOG.isDebugEnabled()) LOG.debug("Session {} no eviction", getId()); } else if (evictionPolicy == SessionCache.EVICT_ON_SESSION_EXIT) { // session will not remain in the cache, so no timeout - _sessionInactivityTimer.setTimeout(-1); + time = -1; if (LOG.isDebugEnabled()) LOG.debug("Session {} evict on exit", getId()); } else { // want to evict on idle: timer is lesser of the session's - // maxInactive and eviction timeout - _sessionInactivityTimer.setTimeout(Math.min(maxInactive, TimeUnit.SECONDS.toMillis(evictionPolicy))); + // expiration remaining and the time to evict + time = (remaining > 0 ? (Math.min(maxInactive, TimeUnit.SECONDS.toMillis(evictionPolicy))) : 0); + if (LOG.isDebugEnabled()) LOG.debug("Session {} timer set to lesser of maxInactive={} and inactivityEvict={}", getId(), maxInactive, evictionPolicy); } } } + + return time; } - /** * @see javax.servlet.http.HttpSession#getMaxInactiveInterval() */ @@ -963,16 +999,6 @@ public class Session implements SessionHandler.SessionIf return _lock.lock(); } - /* ------------------------------------------------------------- */ - /** - * Grab the lock on the session if it isn't locked already - * - * @return the lock - */ - public Lock lockIfNotHeld() - { - return _lock.lock(); - } /* ------------------------------------------------------------- */ /** @@ -1132,13 +1158,14 @@ public class Session implements SessionHandler.SessionIf } /* ------------------------------------------------------------- */ + /** + * @param resident + */ public void setResident(boolean resident) { _resident = resident; - if (_resident) - updateInactivityTimer(); - else + if (!_resident) _sessionInactivityTimer.destroy(); } diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/session/SessionHandler.java b/jetty-server/src/main/java/org/eclipse/jetty/server/session/SessionHandler.java index ef650dc60a6..dcd2037af3a 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/session/SessionHandler.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/session/SessionHandler.java @@ -1372,6 +1372,17 @@ public class SessionHandler extends ScopedHandler } } + /** + * @see #sessionInactivityTimerExpired(Session, long) + */ + @Deprecated + public void sessionInactivityTimerExpired (Session session) + { + //for backwards compilation compatibility only + sessionInactivityTimerExpired(session, System.currentTimeMillis()); + } + + /* ------------------------------------------------------------ */ /** * Each session has a timer that is configured to go off @@ -1379,20 +1390,28 @@ public class SessionHandler extends ScopedHandler * configurable amount of time, or the session itself * has passed its expiry. * + * If it has passed its expiry, then we will mark it for + * scavenging by next run of the HouseKeeper; if it has + * been idle longer than the configured eviction period, + * we evict from the cache. + * + * If none of the above are true, then the System timer + * is inconsistent and the caller of this method will + * need to reset the timer. + * * @param session the session + * @param now the time at which to check for expiry */ - public void sessionInactivityTimerExpired (Session session) + public void sessionInactivityTimerExpired (Session session, long now) { if (session == null) return; - //check if the session is: //1. valid //2. expired //3. idle - boolean expired = false; - try (Lock lock = session.lockIfNotHeld()) + try (Lock lock = session.lock()) { if (session.getRequests() > 0) return; //session can't expire or be idle if there is a request in it @@ -1402,27 +1421,27 @@ public class SessionHandler extends ScopedHandler if (!session.isValid()) return; //do nothing, session is no longer valid - - if (session.isExpiredAt(System.currentTimeMillis()) && session.getRequests() <=0) - expired = true; - } - if (expired) - { - //instead of expiring the session directly here, accumulate a list of - //session ids that need to be expired. This is an efficiency measure: as - //the expiration involves the SessionDataStore doing a delete, it is - //most efficient if it can be done as a bulk operation to eg reduce - //roundtrips to the persistent store. Only do this if the HouseKeeper that - //does the scavenging is configured to actually scavenge - if (_sessionIdManager.getSessionHouseKeeper() != null && _sessionIdManager.getSessionHouseKeeper().getIntervalSec() > 0) + if (session.isExpiredAt(now)) { - _candidateSessionIdsForExpiry.add(session.getId()); - if (LOG.isDebugEnabled())LOG.debug("Session {} is candidate for expiry", session.getId()); + //instead of expiring the session directly here, accumulate a list of + //session ids that need to be expired. This is an efficiency measure: as + //the expiration involves the SessionDataStore doing a delete, it is + //most efficient if it can be done as a bulk operation to eg reduce + //roundtrips to the persistent store. Only do this if the HouseKeeper that + //does the scavenging is configured to actually scavenge + if (_sessionIdManager.getSessionHouseKeeper() != null && _sessionIdManager.getSessionHouseKeeper().getIntervalSec() > 0) + { + _candidateSessionIdsForExpiry.add(session.getId()); + if (LOG.isDebugEnabled())LOG.debug("Session {} is candidate for expiry", session.getId()); + } + } + else + { + //possibly evict the session + _sessionCache.checkInactiveSession(session); } } - else - _sessionCache.checkInactiveSession(session); //if inactivity eviction is enabled the session will be deleted from the cache }