From: ivan.martiniak Date: Tue, 28 Jun 2022 05:00:58 +0000 (+0200) Subject: Fix periodic NETCONF Call Home connection dropping X-Git-Tag: v4.0.2~19 X-Git-Url: https://git.opendaylight.org/gerrit/gitweb?a=commitdiff_plain;h=refs%2Fchanges%2F33%2F100133%2F37;p=netconf.git Fix periodic NETCONF Call Home connection dropping Callhome devices make reconnection every hour due to key re-exchange, which is part of SSHD implementation. When a specific session is authenticated and activated, in case of key re-exchange, authentication (invoking of doAuth() method) does not need to be made again. While we are at it, also make sure CallHomeSessionContext's constructor does not have side-effects -- while this cannot quite happen since we do not reuse ClientSessions, it is a needless leak. JIRA: NETCONF-681 Change-Id: I824c92d230c7570570d5eed21d489c435bdc8b22 Signed-off-by: Ivan Martiniak Signed-off-by: Robert Varga --- diff --git a/netconf/callhome-protocol/src/main/java/org/opendaylight/netconf/callhome/protocol/CallHomeSessionContext.java b/netconf/callhome-protocol/src/main/java/org/opendaylight/netconf/callhome/protocol/CallHomeSessionContext.java index 298763768a..83a62f4fcd 100644 --- a/netconf/callhome-protocol/src/main/java/org/opendaylight/netconf/callhome/protocol/CallHomeSessionContext.java +++ b/netconf/callhome-protocol/src/main/java/org/opendaylight/netconf/callhome/protocol/CallHomeSessionContext.java @@ -10,6 +10,7 @@ package org.opendaylight.netconf.callhome.protocol; import static com.google.common.base.Preconditions.checkArgument; import static java.util.Objects.requireNonNull; +import com.google.common.annotations.VisibleForTesting; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import io.netty.channel.EventLoopGroup; import io.netty.util.concurrent.GlobalEventExecutor; @@ -37,10 +38,11 @@ import org.slf4j.LoggerFactory; class CallHomeSessionContext implements CallHomeProtocolSessionContext { private static final Logger LOG = LoggerFactory.getLogger(CallHomeSessionContext.class); - static final Session.AttributeKey SESSION_KEY = new Session.AttributeKey<>(); - private static final String NETCONF = "netconf"; + @VisibleForTesting + static final Session.AttributeKey SESSION_KEY = new Session.AttributeKey<>(); + private final ClientSession sshSession; private final CallHomeAuthorization authorization; private final Factory factory; @@ -57,11 +59,14 @@ class CallHomeSessionContext implements CallHomeProtocolSessionContext { checkArgument(this.authorization.isServerAllowed(), "Server was not allowed."); this.factory = requireNonNull(factory); this.sshSession = requireNonNull(sshSession); - this.sshSession.setAttribute(SESSION_KEY, this); this.remoteAddress = (InetSocketAddress) this.sshSession.getIoSession().getRemoteAddress(); serverKey = this.sshSession.getServerKey(); } + final void associate() { + sshSession.setAttribute(SESSION_KEY, this); + } + static CallHomeSessionContext getFrom(final ClientSession sshSession) { return sshSession.getAttribute(SESSION_KEY); } @@ -152,23 +157,18 @@ class CallHomeSessionContext implements CallHomeProtocolSessionContext { } static class Factory { - + private final ConcurrentMap sessions = new ConcurrentHashMap<>(); private final EventLoopGroup nettyGroup; private final NetconfClientSessionNegotiatorFactory negotiatorFactory; private final CallHomeNetconfSubsystemListener subsystemListener; - private final ConcurrentMap sessions = new ConcurrentHashMap<>(); Factory(final EventLoopGroup nettyGroup, final NetconfClientSessionNegotiatorFactory negotiatorFactory, final CallHomeNetconfSubsystemListener subsystemListener) { - this.nettyGroup = requireNonNull(nettyGroup, "nettyGroup"); - this.negotiatorFactory = requireNonNull(negotiatorFactory, "negotiatorFactory"); + this.nettyGroup = requireNonNull(nettyGroup); + this.negotiatorFactory = requireNonNull(negotiatorFactory); this.subsystemListener = requireNonNull(subsystemListener); } - void remove(final CallHomeSessionContext session) { - sessions.remove(session.getSessionId(), session); - } - ReverseSshChannelInitializer getChannelInitializer(final NetconfClientSessionListener listener) { return ReverseSshChannelInitializer.create(negotiatorFactory, listener); } @@ -177,18 +177,27 @@ class CallHomeSessionContext implements CallHomeProtocolSessionContext { return subsystemListener; } + EventLoopGroup getNettyGroup() { + return nettyGroup; + } + @Nullable CallHomeSessionContext createIfNotExists(final ClientSession sshSession, final CallHomeAuthorization authorization, final SocketAddress remoteAddress) { - CallHomeSessionContext session = new CallHomeSessionContext(sshSession, authorization, - remoteAddress, this); - CallHomeSessionContext preexisting = sessions.putIfAbsent(session.getSessionId(), session); - // If preexisting is null - session does not exist, so we can safely create new one, otherwise we return - // null and incoming connection will be rejected. - return preexisting == null ? session : null; + final var newSession = new CallHomeSessionContext(sshSession, authorization, remoteAddress, this); + final var existing = sessions.putIfAbsent(newSession.getSessionId(), newSession); + if (existing == null) { + // There was no mapping, but now there is. Associate the the context with the session. + newSession.associate(); + return newSession; + } + + // We already have a mapping, do not create a new one. But also check if the current session matches + // the one stored in the session. This can happen during rekeying. + return existing == CallHomeSessionContext.getFrom(sshSession) ? existing : null; } - EventLoopGroup getNettyGroup() { - return nettyGroup; + void remove(final CallHomeSessionContext session) { + sessions.remove(session.getSessionId(), session); } } } diff --git a/netconf/callhome-protocol/src/main/java/org/opendaylight/netconf/callhome/protocol/NetconfCallHomeServer.java b/netconf/callhome-protocol/src/main/java/org/opendaylight/netconf/callhome/protocol/NetconfCallHomeServer.java index 373c4a90b9..8feecf2506 100644 --- a/netconf/callhome-protocol/src/main/java/org/opendaylight/netconf/callhome/protocol/NetconfCallHomeServer.java +++ b/netconf/callhome-protocol/src/main/java/org/opendaylight/netconf/callhome/protocol/NetconfCallHomeServer.java @@ -76,7 +76,10 @@ public final class NetconfCallHomeServer implements AutoCloseable, ServerKeyVeri LOG.debug("SSH session {} event {}", session, event); switch (event) { case KeyEstablished: - doAuth(clientSession); + // Case of key re-exchange - if session is once authenticated, it does not need to be made again + if (!clientSession.isAuthenticated()) { + doAuth(clientSession); + } break; case Authenticated: CallHomeSessionContext.getFrom(clientSession).openNetconfChannel(); @@ -148,21 +151,22 @@ public final class NetconfCallHomeServer implements AutoCloseable, ServerKeyVeri public boolean verifyServerKey(final ClientSession sshClientSession, final SocketAddress remoteAddress, final PublicKey serverKey) { final CallHomeAuthorization authorization = authProvider.provideAuth(remoteAddress, serverKey); - // server is not authorized if (!authorization.isServerAllowed()) { + // server is not authorized LOG.info("Incoming session {} was rejected by Authorization Provider.", sshClientSession); return false; } - CallHomeSessionContext session = sessionFactory.createIfNotExists( - sshClientSession, authorization, remoteAddress); - // Session was created, session with same name does not exists - if (session != null) { - return true; + + if (sessionFactory.createIfNotExists(sshClientSession, authorization, remoteAddress) == null) { + // Session was not created, session with same name exists + LOG.info("Incoming session {} was rejected. Session with same name {} is already active.", sshClientSession, + authorization.getSessionName()); + return false; } - // Session was not created, session with same name exists - LOG.info("Incoming session {} was rejected. Session with same name {} is already active.", - sshClientSession, authorization.getSessionName()); - return false; + + // Session was created, session with same name does not exist + LOG.debug("Incoming session {} was successfully verified.", sshClientSession); + return true; } public void bind() throws IOException { diff --git a/netconf/callhome-protocol/src/test/java/org/opendaylight/netconf/callhome/protocol/CallHomeSessionContextTest.java b/netconf/callhome-protocol/src/test/java/org/opendaylight/netconf/callhome/protocol/CallHomeSessionContextTest.java index 58560706ca..fd418064af 100644 --- a/netconf/callhome-protocol/src/test/java/org/opendaylight/netconf/callhome/protocol/CallHomeSessionContextTest.java +++ b/netconf/callhome-protocol/src/test/java/org/opendaylight/netconf/callhome/protocol/CallHomeSessionContextTest.java @@ -7,6 +7,8 @@ */ package org.opendaylight.netconf.callhome.protocol; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyBoolean; import static org.mockito.ArgumentMatchers.anyString; @@ -98,12 +100,16 @@ public class CallHomeSessionContextTest { @Test public void theContextShouldBeSettableAndRetrievableAsASessionAttribute() { - // redo instance below because previous constructor happened too early to capture behavior + // when instance = realFactory.createIfNotExists(mockSession, mockAuth, address); + // then + assertNotNull(instance); + verify(mockSession, times(1)).setAttribute(CallHomeSessionContext.SESSION_KEY, instance); + verify(mockSession, times(0)).getAttribute(any()); + // when CallHomeSessionContext.getFrom(mockSession); // then - verify(mockSession, times(1)).setAttribute(CallHomeSessionContext.SESSION_KEY, instance); verify(mockSession, times(1)).getAttribute(CallHomeSessionContext.SESSION_KEY); } @@ -185,10 +191,10 @@ public class CallHomeSessionContextTest { @Test @Ignore + // FIXME: enable this test public void failureToOpenTheChannelShouldCauseTheSessionToClose() { // given instance = realFactory.createIfNotExists(mockSession, mockAuth, address); - OpenFuture mockFuture = mock(OpenFuture.class); Mockito.doReturn(false).when(mockFuture).isOpened(); Mockito.doReturn(new RuntimeException("test")).when(mockFuture).getException(); @@ -202,4 +208,11 @@ public class CallHomeSessionContextTest { // You'll see an error message logged to the console - it is expected. verify(mockSession, times(1)).close(anyBoolean()); } + + @Test + public void theContextConstructorShouldNotModifySession() { + instance = new CallHomeSessionContext(mockSession, mockAuth, address, realFactory); + verify(mockSession, times(0)).setAttribute(any(), any()); + assertNull(CallHomeSessionContext.getFrom(mockSession)); + } } diff --git a/netconf/callhome-protocol/src/test/java/org/opendaylight/netconf/callhome/protocol/NetconfCallHomeServerTest.java b/netconf/callhome-protocol/src/test/java/org/opendaylight/netconf/callhome/protocol/NetconfCallHomeServerTest.java index ca41cf1408..250ab80bfa 100644 --- a/netconf/callhome-protocol/src/test/java/org/opendaylight/netconf/callhome/protocol/NetconfCallHomeServerTest.java +++ b/netconf/callhome-protocol/src/test/java/org/opendaylight/netconf/callhome/protocol/NetconfCallHomeServerTest.java @@ -117,8 +117,9 @@ public class NetconfCallHomeServerTest { final PublicKey serverKey = mock(PublicKey.class); doReturn(serverKey).when(mockSession).getServerKey(); - SessionListener listener = instance.createSessionListener(); + final SessionListener listener = instance.createSessionListener(); doReturn(mockAuthFuture).when(mockContext).authorize(); + doReturn(false).when(mockSession).isAuthenticated(); // when listener.sessionEvent(mockSession, evt[pass]); // then @@ -130,7 +131,6 @@ public class NetconfCallHomeServerTest { @Test public void verificationOfTheServerKeyShouldBeSuccessfulForServerIsAllowed() { // given - ClientSessionImpl mockClientSession = mock(ClientSessionImpl.class); Mockito.doReturn("test").when(mockClientSession).toString(); SocketAddress mockSocketAddr = mock(SocketAddress.class); @@ -138,13 +138,11 @@ public class NetconfCallHomeServerTest { Mockito.doReturn(true).when(mockAuth).isServerAllowed(); Mockito.doReturn("some-session-name").when(mockAuth).getSessionName(); - Mockito.doReturn(mockAuth).when(mockCallHomeAuthProv).provideAuth(mockSocketAddr, mockPublicKey); - Mockito.doReturn(null).when(mockFactory).createIfNotExists(mockClientSession, mockAuth, mockSocketAddr); // expect - instance.verifyServerKey(mockClientSession, mockSocketAddr, mockPublicKey); + assertFalse(instance.verifyServerKey(mockClientSession, mockSocketAddr, mockPublicKey)); } @Test