TLS PSK implementation #1777

sunnysingh85 · 2024-05-21T20:24:30Z

This PR adds an implementation to use a pre-shared key (PSK) to complete a TLS handshake (TLS-PSK). This implementation is based on PSK APIs offered by BouncyCastle.

argha-c · 2024-06-25T18:15:53Z

zuul-core/src/main/java/com/netflix/zuul/netty/server/psk/TlsPskHandler.java

+    @Override
+    public void write(ChannelHandlerContext ctx, Object msg, ChannelPromise promise) throws Exception {
+        if (!(msg instanceof ByteBuf byteBufMsg)) {
+            ctx.write(msg, promise);


This will continue propagating the message to the handlers. Is that intentional?

For this case, typically we want to do the following:

release the message using ReferenceCountUtil.safeRelease(msg) to avoid memory leaks

set failure on the promise with an appropriate exception.

argha-c · 2024-06-25T18:41:08Z

zuul-core/src/main/java/com/netflix/zuul/netty/server/psk/TlsPskHandler.java

+        if (availableOutputBytes != 0) {
+            byte[] outputBytes = new byte[availableOutputBytes];
+            tlsPskServerProtocol.readOutput(outputBytes, 0, availableOutputBytes);
+            ctx.write(Unpooled.wrappedBuffer(outputBytes), promise)


This won't actually flush the buffer.
You might want to use .writeAndFlush instead in this case.

I was using .writeAndFlush previously but I ran into some issues during load testing with larger payloads and chunks. I can try to reproduce those and share more details.

argha-c · 2024-06-25T18:52:46Z

zuul-core/src/main/java/com/netflix/zuul/netty/server/psk/TlsPskHandler.java

+        try {
+            tlsPskServerProtocol.offerInput(bytesRead);
+        } catch (TlsFatalAlert tlsFatalAlert) {
+            writeOutputIfAvailable(ctx);


We should explicitly fail the handshake at this point? For ex, see this utility

It's unclear how/where this is handled?

Thanks for pointing this out. I'll update the code.

argha-c · 2024-06-25T20:36:30Z

zuul-core/src/main/java/com/netflix/zuul/netty/server/psk/TlsPskHandler.java

+        if (appDataAvailable > 0) {
+            byte[] appData = new byte[appDataAvailable];
+            tlsPskServerProtocol.readInput(appData, 0, appDataAvailable);
+            out.add(Unpooled.wrappedBuffer(appData));


Do we need to pass this data down to other handlers?

Based on my understanding, the fact that TlsPskHandler is a ByteToMessageDecoder, adding the decoded output to List<Object> out does the job of passing down the data to other handlers. LMK if I'm missing the point that you are making.

argha-c · 2024-06-25T21:02:12Z

zuul-core/src/main/java/com/netflix/zuul/netty/server/psk/TlsPskHandler.java

+
+                @Override
+                public String getCipherSuite() {
+                    return SUPPORTED_TLS_PSK_CIPHER_SUITE_MAP.get(


Is the suite map the accurate value here?
Or should it be derived from the underlying session, i.e. SessionParameters in this.getContext().getSession()?

I'm actually getting the negotiated cipher suite from context getContext().getSecurityParameters().getCipherSuite() which returns an integer but SSLSession requires me to return a name of the suite, the map is actually doing the conversion from int to String.

argha-c · 2024-06-25T21:04:44Z

zuul-core/src/main/java/com/netflix/zuul/netty/server/psk/TlsPskHandler.java

+        protected Vector getProtocolNames() {
+            Vector protocolNames = new Vector();
+            protocolNames.addElement(ProtocolName.HTTP_1_1);
+            protocolNames.addElement(ProtocolName.HTTP_2_TLS);


This list should probably be injected, esp. for clients we don't want to advertise h2 for.

argha-c · 2024-06-25T21:16:36Z

zuul-core/src/main/java/com/netflix/zuul/netty/server/psk/TlsPskHandler.java

+        @Override
+        @SneakyThrows // TODO: Ask BC folks to see if getExternalPSK can throw a checked exception
+        public TlsPSKExternal getExternalPSK(Vector clientPskIdentities) {
+            byte[] clientPskIdentity = ((PskIdentity)clientPskIdentities.get(0)).getIdentity();


Is the identity we want guaranteed to be first in an ordered list?

AFAIK we only plan to support one PSK identity in the hint, so it should be safe. And BC only invokes this method if clientPskIdentities is non empty.

argha-c · 2024-06-25T21:29:38Z

zuul-core/src/main/java/com/netflix/zuul/netty/server/psk/TlsPskHandler.java

+                psk = externalTlsPskProvider.provide(clientPskIdentity, this.context.getSecurityParametersHandshake().getClientRandom());
+            }catch (PskCreationFailureException e) {
+                throw switch (e.getTlsAlertMessage()) {
+                    case unknown_psk_identity -> new TlsFatalAlert(AlertDescription.unknown_psk_identity, "Unknown or null client PSk identity");


It's unclear from the API if this fails the handshake. Can you clarify how this aborts the handshake?
We probably need to handle things like firing SslCloseCompletionEvent and cleanup manually anyway.

jguerra · 2024-07-10T22:41:45Z

zuul-core/src/main/java/com/netflix/zuul/netty/server/psk/TlsPskHandler.java

+import java.util.concurrent.TimeUnit;
+import java.util.function.Consumer;
+
+public class TlsPskHandler extends ByteToMessageDecoder


I think it might be nice to break this class into two pieces: a decoder and a handler. That way the TlsPskHandler can extend ChannelDuplexHandler and avoid all the boilerplate of needing to implement the full interfaces. A pattern I've seen before (that may work) is to implement handlerAdded in TlsPskHanlder and use it to add the decoder to the pipeline automatically

This made the code so much cleaner, thanks for this comment!

jguerra · 2024-07-10T22:45:07Z

zuul-core/src/main/java/com/netflix/zuul/netty/server/psk/TlsPskHandler.java

+    @Override
+    public void channelRegistered(ChannelHandlerContext ctx) throws Exception {
+        tlsPskServer =
+                new ZuulPskServer(new JcaTlsCryptoProvider().create(new SecureRandom()), registry, externalTlsPskProvider, ctx);


can you make this SecureRandom a static constant instead? There isn't any concurrency benefits to create a new one every time

jguerra · 2024-07-10T23:00:06Z

zuul-core/src/main/java/com/netflix/zuul/netty/server/psk/TlsPskHandler.java

+        }
+    }
+
+    static class TlsPskServerProtocol extends TlsServerProtocol {


for improved readability it might be nice to extract these static inner classes

jguerra · 2024-07-26T17:02:31Z

zuul-core/src/main/java/com/netflix/zuul/netty/server/ssl/SslHandshakeInfoHandler.java

@@ -98,13 +105,23 @@ public void userEventTriggered(ChannelHandlerContext ctx, Object evt) throws Exc
                        serverCert = session.getLocalCertificates()[0];
                    }

+                    Boolean tlsHandshakeUsingExternalPSK = ctx.channel()
+                            .attr(ZuulPskServer.TLS_HANDSHAKE_USING_EXTERNAL_PSK)
+                            .get();


This looks like a NPE risk since the SslHandshakeInfo ctor will unbox this to a boolean. Is TLS_HANDSHAKE_USING_EXTERNAL_PSK always going to be set, or should this be set to false in the case of the attribute having not been set on the channel?

It seems like in ZuulPskServer, it is always set to false in notifyHandshakeBeginning and to true notifyHandshakeComplete. So it should always be set.

Actually talked to Sunny and this code is more generic and independent of ZuulPskServer. Made the change to default to false

jguerra · 2024-07-26T17:08:12Z

zuul-core/src/main/java/com/netflix/zuul/netty/server/psk/ZuulPskServer.java

+    }
+
+    @Override
+    @SneakyThrows


Are there other checked exceptions that you're not handling?

Sunny asked the BC folks but we havent received the response yet
bcgit/bc-java#1673

Todo Deepti : Why we are using @SneakyThrows

jguerra · 2024-07-26T17:12:51Z

zuul-core/src/main/java/com/netflix/zuul/netty/server/http2/Http2OrHttpHandler.java

@@ -70,6 +75,42 @@ public Http2OrHttpHandler(
        this.addHttpHandlerFn = addHttpHandlerFn;
    }

+    @Override
+    public void userEventTriggered(ChannelHandlerContext ctx, Object evt) throws Exception {


nit: add a javadoc saying this method is inspired by ApplicationProtocolNegotiationHandler.userEventTriggered

jguerra · 2024-07-26T17:23:35Z

zuul-core/src/main/java/com/netflix/zuul/netty/server/psk/TlsPskUtils.java

+    static byte[] readDirect(ByteBuf byteBufMsg) {
+        int length = byteBufMsg.readableBytes();
+        byte[] dest = new byte[length];
+        byteBufMsg.readSlice(length).getBytes(0, dest);


nit: I think byteBufMsg.readBytes(dest) essentially does the same thing, but is a little easier to parse

jguerra · 2024-07-26T17:26:45Z

zuul-core/src/main/java/com/netflix/zuul/netty/server/psk/TlsPskHandler.java

+            promise.setFailure(new IllegalStateException("Failed to write message on the channel. Message is not a ByteBuf"));
+            return;
+        }
+        byte[] appDataBytes = byteBufMsg.hasArray() ? byteBufMsg.array() : TlsPskUtils.readDirect(byteBufMsg);


could all of this logic (including the safeRelease) be put in the helper method?

jguerra · 2024-07-26T17:35:03Z

zuul-core/src/main/java/com/netflix/zuul/netty/server/psk/ZuulPskServer.java

+    }
+
+    @Override
+    public void notifyAlertRaised(short alertLevel, short alertDescription, String message, Throwable cause) {


There is one other event netty fires that we'd be interested in (if possible): https://github.com/netty/netty/blob/4.1/handler/src/main/java/io/netty/handler/ssl/SslCloseCompletionEvent.java netty will fire this event when the the ssl session is closed. Does bouncy castle trigger any similar alerts or anything when that happens?

Justin
In TLSProtocol.java file I see it raises close_notify alert : https://github.com/bcgit/bc-java/blob/main/tls/src/main/java/org/bouncycastle/tls/TlsProtocol.java#L300 which will eventually make it's way to this notifyAlertRaised method. Did you want us to do something specific with that alert?
https://github.com/bcgit/bc-java/blob/main/tls/src/main/java/org/bouncycastle/tls/TlsProtocol.java#L1717

I added code to send event SslCloseCompletionEvent on close_notify alert

…dler 2) adding comment in Http2OrHttpHandler and use readBytes instead of readSlice 3) adding SslCloseCompletionEvent on close_notify alert 4) handling null value TLS_HANDSHAKE_USING_EXTERNAL_PSK

jguerra · 2024-11-21T04:31:48Z

zuul-core/src/main/java/com/netflix/netty/common/ssl/SslHandshakeInfo.java

-            X509Certificate clientCertificate) {
+            X509Certificate clientCertificate,
+            boolean usingExternalPSK,
+            ClientPSKIdentityInfo clientPSKIdentityInfo) {


consider keeping the original constructor for backwards compatibility, and adding a new constructor with these two new parameters

sunnysingh85 added 2 commits May 17, 2024 13:52

TLS PSK implementation

29124da

Breaking apart the PSK creation to an interface

e906fb9

kyagna requested review from jguerra, gavinbunney, argha-c, connorworley, karim-z, kyagna, AlexanderEllis and lindseyreynolds May 21, 2024 21:01

padolph approved these changes May 21, 2024

View reviewed changes

sunnysingh85 added 2 commits May 24, 2024 11:10

Added license

0ef6c0a

Store PSK info in handshake info

99d644a

argha-c reviewed Jun 25, 2024

View reviewed changes

jguerra reviewed Jul 10, 2024

View reviewed changes

sunnysingh85 added 2 commits July 24, 2024 15:21

Addressed review comments

a19fa60

Added license

3cc8293

jguerra reviewed Jul 26, 2024

View reviewed changes

deeptiv1991 force-pushed the tls-psk branch 3 times, most recently from cd7056b to 18ef13b Compare October 16, 2024 20:37

1) moving read of byte buff and release to helper method in TLSPSKHan…

a3c5d39

…dler 2) adding comment in Http2OrHttpHandler and use readBytes instead of readSlice 3) adding SslCloseCompletionEvent on close_notify alert 4) handling null value TLS_HANDSHAKE_USING_EXTERNAL_PSK

deeptiv1991 force-pushed the tls-psk branch from 18ef13b to a3c5d39 Compare October 16, 2024 20:39

adding license header

44de590

deeptiv1991 force-pushed the tls-psk branch from 397d51f to 44de590 Compare October 24, 2024 20:44

deeptiv1991 added 2 commits October 24, 2024 14:53

Merge branch 'master' into tls-psk

4777374

Merge branch 'Netflix:master' into tls-psk

4ba06ab

jguerra reviewed Nov 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TLS PSK implementation #1777

TLS PSK implementation #1777

sunnysingh85 commented May 21, 2024

argha-c Jun 25, 2024

argha-c Jun 25, 2024

sunnysingh85 Jul 19, 2024

argha-c Jun 25, 2024 •

edited

Loading

sunnysingh85 Jul 19, 2024

argha-c Jun 25, 2024

sunnysingh85 Jul 19, 2024

argha-c Jun 25, 2024

sunnysingh85 Jul 19, 2024

argha-c Jun 25, 2024

argha-c Jun 25, 2024

sunnysingh85 Jul 24, 2024

argha-c Jun 25, 2024 •

edited

Loading

jguerra Jul 10, 2024

sunnysingh85 Jul 24, 2024

jguerra Jul 10, 2024

jguerra Jul 10, 2024

jguerra Jul 26, 2024

deeptiv1991 Oct 15, 2024

deeptiv1991 Oct 15, 2024

jguerra Jul 26, 2024

deeptiv1991 Oct 15, 2024

deeptiv1991 Oct 23, 2024

jguerra Jul 26, 2024

jguerra Jul 26, 2024

jguerra Jul 26, 2024

jguerra Jul 26, 2024

deeptiv1991 Oct 15, 2024

deeptiv1991 Oct 15, 2024

jguerra Nov 21, 2024

TLS PSK implementation #1777

Are you sure you want to change the base?

TLS PSK implementation #1777

Conversation

sunnysingh85 commented May 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

argha-c Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

argha-c Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

argha-c Jun 25, 2024 •

edited

Loading

argha-c Jun 25, 2024 •

edited

Loading