Repository: netty/netty
File: handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java (Java)

Code before:
                SslContextBuilder.forServer(kmf) :
                SslContextBuilder.forServer(ssc.certificate(), ssc.privateKey());
        if (mutualAuth) {
            serverContextBuilder.clientAuth(ClientAuth.REQUIRE);
        }
        serverSslCtx = wrapContext(param, serverContextBuilder.trustManager(InsecureTrustManagerFactory.INSTANCE)
                                     .sslProvider(sslServerProvider())
                                     .sslContextProvider(serverSslContextProvider())
                                     .protocols(param.protocols())
                                     .ciphers(param.ciphers())
                                     .build());
        SSLEngine clientEngine = null;
        SSLEngine serverEngine = null;
        String key = "key";
        try {
            clientEngine = wrapEngine(clientSslCtx.newEngine(UnpooledByteBufAllocator.DEFAULT));
            serverEngine = wrapEngine(serverSslCtx.newEngine(UnpooledByteBufAllocator.DEFAULT));

            clientEngine.getSession().putValue(key, Boolean.TRUE);
            assertEquals(Boolean.TRUE, clientEngine.getSession().getValue(key));
            serverEngine.getSession().putValue(key, Boolean.TRUE);
            assertEquals(Boolean.TRUE, serverEngine.getSession().getValue(key));

            handshake(param.type(), param.delegate(), clientEngine, serverEngine);

            SSLSession clientSession = clientEngine.getSession();
            SSLSession serverSession = serverEngine.getSession();

            clientSession.removeValue(key);
            serverSession.removeValue(key);

            assertNull(clientSession.getPeerHost());
            assertNull(serverSession.getPeerHost());
            assertEquals(-1, clientSession.getPeerPort());
            assertEquals(-1, serverSession.getPeerPort());

            assertTrue(clientSession.getCreationTime() > 0);
            assertTrue(serverSession.getCreationTime() > 0);

            assertTrue(clientSession.getLastAccessedTime() > 0);
            assertTrue(serverSession.getLastAccessedTime() > 0);

            assertEquals(clientSession.getCreationTime(), clientSession.getLastAccessedTime());
            assertEquals(serverSession.getCreationTime(), serverSession.getLastAccessedTime());

            assertEquals(param.combo().protocol, clientSession.getProtocol());
            assertEquals(param.combo().protocol, serverSession.getProtocol());

            assertEquals(param.combo().cipher, clientSession.getCipherSuite());
            assertEquals(param.combo().cipher, serverSession.getCipherSuite());


Review comment (bug):
@XueleiFan I want to bring your attention to the use case captured here in this test, in the context of https://bugs.openjdk.org/browse/JDK-8245551.

What is being replicated here, is that a `TrustManager` implementation may - as part of validating the peer certificate - also compute a principal or other authorization data, capturing the identity, privileges and/or capabilities of the peer, and put that in the handshake session. Thus allowing the surrounding framework or application to later extract this data, make authorization decisions based on it.

Using the `TrustManager` in this way has two main advantages.

1) Such authorization decisions needs to be based on the _validated_ peer certificate chain, rather than the chain-as-presented (which may have the certs out-of-order), and the `TrustManager` is in the correct place and time to compute and inspect validated certificate chains.

2) Extracting and validating such certificate data is potentially expensive and may involve encryption operations (for instance, the JDK `TrustManager` implementation does not share the validated chain it computes, so implementations based on it will have to compute it again), so it is desirable for performance to do this work per session, rather than per request.

From reading https://bugs.openjdk.org/browse/JDK-8245551 it sounds like we are being set up for these sessions to statelessly resume. It's not clear from the issue how distributed sessions would be enabled, or if it is somehow enabled by default and happens automatically (how would that work?). But stateless resumption would cause the authorization data to be lost, which the application might not expected, and may cause security issues.

I'm wondering if the session being stateless is an a priori assumption, or if it will be determined based on whether the handshake session gets any information added to it, or something else. If it's going to be the case that a session may or may not be in a stateless mode, and there's no changing this even as early as the `TrustManager` calls, then it's certainly desirable to be able to determine this fact from the session (and handshake session) so that informed decisions can be made about it; to reject sessions, to deploy work-arounds, etc.

I'll also note that in this scenario, if the session data is local-only then it won't be advantageous for the sessions to move across hosts, since the applications will be forced to have a request-level filter that re-computes validated certificate chains and extracts authorization on every move.

That's my feedback on the JDK issue.

/cc @sbordet FYI.

Code after:
        }

        final String handshakeKey = "handshake";
        TrustManagerFactory tmf = new ConstantTrustManagerFactory(new EmptyExtendedX509TrustManager() {
            @Override
            public void checkClientTrusted(java.security.cert.X509Certificate[] chain,
                                           String authType, SSLEngine engine) {
                // This is broken in conscrypt.
                // TODO: Open an issue in the conscrypt project.
                if (!Conscrypt.isEngineSupported(engine)) {
                    assertEquals(0, engine.getHandshakeSession().getValueNames().length);
                }
                engine.getHandshakeSession().putValue(handshakeKey, Boolean.TRUE);
            }

            @Override
            public void checkServerTrusted(java.security.cert.X509Certificate[] chain,
                                           String authType, SSLEngine engine) {
                // This is broken in conscrypt.
                // TODO: Open an issue in the conscrypt project.
                if (!Conscrypt.isEngineSupported(engine)) {
                    assertEquals(0, engine.getHandshakeSession().getValueNames().length);
                }
                engine.getHandshakeSession().putValue(handshakeKey, Boolean.TRUE);
            }
        });

        clientSslCtx = wrapContext(param, clientContextBuilder
                                        .trustManager(tmf)
                                        .sslProvider(sslClientProvider())
                                        .sslContextProvider(clientSslContextProvider())
                                        .protocols(param.protocols())
                                        .ciphers(param.ciphers())
                                        .build());

        SslContextBuilder serverContextBuilder = kmf != null ?
                SslContextBuilder.forServer(kmf) :
                SslContextBuilder.forServer(ssc.certificate(), ssc.privateKey());
        if (mutualAuth) {
            serverContextBuilder.clientAuth(ClientAuth.REQUIRE);
        }
        serverSslCtx = wrapContext(param, serverContextBuilder.trustManager(tmf)
                                     .sslProvider(sslServerProvider())
                                     .sslContextProvider(serverSslContextProvider())
                                     .protocols(param.protocols())
                                     .ciphers(param.ciphers())
                                     .build());
        SSLEngine clientEngine = null;
        SSLEngine serverEngine = null;
        String key = "key";
        try {