Implement TLS security protocol including early muxer negotiation #283

ianopolous · 2023-05-24T12:00:28Z

Early muxer negotiation is when the security protocol handshake is used to negotiate the muxer to avoid a round trip. In TLS this is done using ALPN. It is not implemented for Noise here.

This also includes a few small changes:

Remove the insecure Secio from builders so it can't be accidentally included
Upgrade to Java 17 which is required for Ed25519 TLS certs
Expose the event loop on Streams which we needed to implement p2p http proxies in Nabu

Part of #272

ianopolous · 2023-05-24T12:04:19Z

~~I've just noticed the old yamux changes got in there somehow. This was already my second attempt at cherry-picking onto v1.0.0.~~
Edit I've fixed it.

Nashatyrev · 2023-05-25T04:24:55Z

@ianopolous thanks for another contribution 👍

Some questions here:

is that PR possible without switching to Java 17? We are potentially considering upgrading the lib to Java 17 at some point, however if possible I would prefer make that transition in a separate PR.
- Maybe BouncyCastle Ed25519 implementation could be used instead of JDK?
- Is it possible to make Ed25519 support pluggable?
- As the last resort: could Ed25519 support be omitted for now?
Could you please explain usecases for the eventLoop()? We were trying originally to keep Netty API exposure as minimal as possible. Exposing the Netty EventLoop (which is not quite a basic api) via generic Stream looks slightly painful for me.

I'm planning to revisit early muxer handshake concept and probably make a separate PR for introducing it

ianopolous · 2023-05-25T11:38:07Z

I think I'm most of the way to removing the need for java 17. I've pushed it to a new branch: https://github.com/Peergos/jvm-libp2p/tree/feat/upstream-tls-java-11

However I can't get gradle to find the new bouncy castle jar (which is in maven central like the rest). Any chance you could have a look @Nashatyrev ?

Nashatyrev · 2023-05-25T12:13:34Z

However I can't get gradle to find the new bouncy castle jar (which is in maven central like the rest).

Wouldn't that help?

Index: versions.gradle
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/versions.gradle b/versions.gradle
--- a/versions.gradle	(revision d4eb98c8bd4d6ae86eca228c35682b2da9edf9b7)
+++ b/versions.gradle	(date 1685016596704)
@@ -40,6 +40,7 @@
         dependencySet(group: "org.bouncycastle", version: "1.70") {
             entry 'bcprov-jdk15on'
             entry 'bcpkix-jdk15on'
+            entry 'bctls-jdk15on'
         }
     }
 }

ianopolous · 2023-05-25T13:36:01Z

Thank you @Nashatyrev. It's building and running in that java11 branch. But I can't get it to work. It just times out getting the Ping controller. Any chance you could have a look?

I'm testing using HostTestJava and running Kubo locally (You'll need to change the Peerid in the test if you run it)

Nashatyrev · 2023-05-25T16:20:48Z

Do you mean it doesn't work with TLS security, but works with others?
Or did it stop working after switching to BC TLS implementation?

ianopolous · 2023-05-25T17:17:25Z

The test is specifically for TLS (If I switch to Noise it is fine). It is working in this branch on java 17, but not in the java 11 branch with BC.

I'm running it locally, along with Kubo.

ianopolous · 2023-05-26T09:59:58Z

I think https://github.com/Peergos/jvm-libp2p/tree/feat/upstream-tls-java-11 is quite close. It works fine Java to Java using Ed25519 certs using BouncyCastle. Talking to Kubo (which uses ECDSA over P-256) the TLS handshake completes, but a subsequent protocol select times out.

Nashatyrev · 2023-05-26T11:06:10Z

@ianopolous
You may want to try adding debug wire loggers:

jvm-libp2p/libp2p/src/test/kotlin/io/libp2p/core/HostTest.kt

Lines 138 to 142 in 89ba1c2

    
           beforeSecureHandler.addHandler(beforeSecureTestHandler1) 
        
           afterSecureHandler.addHandler(afterSecureTestHandler1) 
        
           streamPreHandler.addHandler(preStreamTestHandler1) 
        
           streamHandler.addHandler(streamTestHandler1) 
        
           muxFramesHandler.addHandler(muxFrameTestHandler1)

Just use addLogger() instead of addHandler()

ianopolous · 2023-05-26T11:17:53Z

~~@Nashatyrev Is that possible from the Java Host test?~~

Don't worry, I'll use the Kotlin one.

ianopolous · 2023-05-26T13:18:38Z

The plot thickens. Running HostTestJava to Kubo works with {Noise + (mplex or yamux)}, but fails with TLS

However, if I disable early muxer negotiation, then TLS works! So it's possible we broke early muxer with some of the upstream changes??

EDIT: Nope. It runs fine on this branch. So that likely means there is something wrong with Bouncy Castle ALPN...

BINGO! I've got it. It was a bug in Netty's interaction with BC for ALPN. Updating Netty fixes it!

ianopolous · 2023-05-26T14:37:31Z

@Nashatyrev Ok. We're now good with Java 11.

The use for the EventLoop is a p2p http proxy. It accepts incoming libp2p connections and proxies it to a fixed http endpoint or http handler. We needed access to the eventloop to add the new http handlers when receiving a new connection: https://github.com/Peergos/nabu/blob/master/src/main/java/org/peergos/protocol/http/HttpProtocol.java#L103

Todo: * muxer negotiation via client hello info * verify remote certs

Implemented full trust manager TODO: * Muxer negotiation in client hello info * close stream on cipher error

Remove Secio from defaults!! Kubo still drops us during handshake

But default to Ed25519

Mark certificate extension as critical

Don't remove the SetupHandler before adding the TLS handler.

…l in early muxer negotiation

Handle no early negotiated muxer protocol in TLS

Nashatyrev · 2023-05-30T07:10:50Z

@Nashatyrev Ok. We're now good with Java 11.

Great news! Thanks!
Sorry I'm still on the way with the early muxer negotiation integration

The use for the EventLoop is a p2p http proxy. It accepts incoming libp2p connections and proxies it to a fixed http endpoint or http handler. We needed access to the eventloop to add the new http handlers when receiving a new connection: https://github.com/Peergos/nabu/blob/master/src/main/java/org/peergos/protocol/http/HttpProtocol.java#L103

Sorry, I didn't dive deeply into the usage case above.
Anyway I would suggest to discuss that eventLoop question in a separate PR/Issue

This reverts commit ea86489.

This reverts commit ec080fe.

ianopolous · 2023-05-30T12:52:30Z

@Nashatyrev I've removed the EventLoop stuff from this PR. Are you planning on PRing your early muxer stuff into this branch?

~~EDIT somehow those two reverts have lost the work to support java 11. Ugh.~~ FIXED

…onal

Nashatyrev · 2023-05-31T09:44:24Z

libp2p/src/main/kotlin/io/libp2p/security/tls/TLSSecureChannel.kt

+                ApplicationProtocolConfig.Protocol.ALPN,
+                ApplicationProtocolConfig.SelectorFailureBehavior.FATAL_ALERT,
+                ApplicationProtocolConfig.SelectedListenerFailureBehavior.FATAL_ALERT,
+                muxerIds.plus("libp2p") // early muxer negotiation


Why do we need adding "libp2p" here?
Just doing some minor refactoring and curious if it would be the same effect if we don't add this value?

This is for compatibility with clients that do not support early muxer negotiation (they just send "libp2p"). So definitely necessary.

…puxer protocol, pass a list of StreamMuxers to constructor. SecureChannel.Session contains NegotiatedStreamMuxer instance instead of string protocol representation

Nashatyrev

Basically LGTM. Just please consider some minor refactor here: Peergos#9

I'm still slightly concerned with SecureChannel.Session.earlyMuxer property. It has a transient nature and is not intended to be exposed in the API. But I'm planning to address it later in a separate PR to not block further upstream work

More type safety for early muxer negotiation

ianopolous force-pushed the feat/upstream-tls branch from cacc181 to a185004 Compare May 24, 2023 12:06

ianopolous mentioned this pull request May 24, 2023

Upstream new protocols (Yamux, TLS, Quic) #272

Open

ianopolous added 16 commits May 26, 2023 15:40

Add key type tests

98b97da

Initial attempt at libp2p-tls implementation

b634975

Todo: * muxer negotiation via client hello info * verify remote certs

Update to Java 17 minimum, which Ed25519 asn1 needs

1c1edba

Improve tls cert test

ff32b9c

Linting

5d946d1

More linting

cab2522

Lint tests

ddaece6

First successful libp2p-tls connections!! (Java-Java)

3a2cbb0

Implemented full trust manager TODO: * Muxer negotiation in client hello info * close stream on cipher error

Add TLS tests

923a043

Linting

52fc181

Use jdk17 in jitpack

c85991e

Implement early muxer negiation in TLS using ALPN

aa867f6

Remove Secio from defaults!! Kubo still drops us during handshake

Linting

c27715f

Support ECDSA certificate keys in TLS as well as Ed25519

d56d880

But default to Ed25519

Remove UShortLengthCodec in tls.

933dba3

Mark certificate extension as critical

Hooray first successful TLS handshake with Kubo!

38b4b46

Don't remove the SetupHandler before adding the TLS handler.

ianopolous added 10 commits May 26, 2023 15:40

Push muxer channel initializer rather than directly initialise channe…

66353e5

…l in early muxer negotiation

Use context allocator in TLS

f5fd8f2

Handle no early negotiated muxer protocol in TLS

Ping over TLS working to kubo!!

fde75d0

Reenable early muxer negotiation in TLS (It works!)

8d54096

Change test to use tls and yamux

647afe3

Expose EventLoop on Stream

ec080fe

Expose EventLoop on Kotlin Stream

ea86489

Fix TLS test

0a359bc

Lint test

bdf4690

Fix simulation test classes

d3ed41e

ianopolous force-pushed the feat/upstream-tls branch from 86c808a to d3ed41e Compare May 26, 2023 14:44

ianopolous added 4 commits May 30, 2023 13:36

Revert "Expose EventLoop on Kotlin Stream"

f1dd5bf

This reverts commit ea86489.

Revert "Expose EventLoop on Stream"

4324ccb

This reverts commit ec080fe.

Remove other eventLoop() impl

61e630e

Revert java17 usage that somehow cherry-picked back in

8cf6610

Reinstate Java 11 support which somehow got lost

115dbac

ianopolous force-pushed the feat/upstream-tls branch from 11f68f4 to 115dbac Compare May 30, 2023 13:35

SecureChannel: rename nextProto to muxerProtocol and make it opti…

1e4467b

…onal

Nashatyrev reviewed May 31, 2023

View reviewed changes

Nashatyrev added 4 commits May 31, 2023 15:11

Add more type safety: since SecureChannel may early negotiate just a …

d5ba11b

…puxer protocol, pass a list of StreamMuxers to constructor. SecureChannel.Session contains NegotiatedStreamMuxer instance instead of string protocol representation

Remove unused variable

7390327

Remove accident star import optimization

e74723a

Return back the "libp2p" entry for the TLS ALPN

3071e67

Nashatyrev approved these changes May 31, 2023

View reviewed changes

Nashatyrev and others added 2 commits May 31, 2023 16:22

Formatting

2ee67b0

Merge pull request #9 from Nashatyrev/feature/upstream-tls-early-muxer

34992ac

More type safety for early muxer negotiation

Nashatyrev merged commit de42c11 into libp2p:v1.0.0 May 31, 2023

Nashatyrev mentioned this pull request Aug 18, 2023

Integrate V1.0.0 changes into develop #303

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement TLS security protocol including early muxer negotiation #283

Implement TLS security protocol including early muxer negotiation #283

ianopolous commented May 24, 2023 •

edited by Nashatyrev

Loading

ianopolous commented May 24, 2023 •

edited

Loading

Nashatyrev commented May 25, 2023

ianopolous commented May 25, 2023

Nashatyrev commented May 25, 2023

ianopolous commented May 25, 2023

Nashatyrev commented May 25, 2023

ianopolous commented May 25, 2023 •

edited

Loading

ianopolous commented May 26, 2023 •

edited

Loading

Nashatyrev commented May 26, 2023

ianopolous commented May 26, 2023 •

edited

Loading

ianopolous commented May 26, 2023 •

edited

Loading

ianopolous commented May 26, 2023

Nashatyrev commented May 30, 2023

ianopolous commented May 30, 2023 •

edited

Loading

Nashatyrev May 31, 2023

ianopolous May 31, 2023

Nashatyrev left a comment

Implement TLS security protocol including early muxer negotiation #283

Implement TLS security protocol including early muxer negotiation #283

Conversation

ianopolous commented May 24, 2023 • edited by Nashatyrev Loading

ianopolous commented May 24, 2023 • edited Loading

Nashatyrev commented May 25, 2023

ianopolous commented May 25, 2023

Nashatyrev commented May 25, 2023

ianopolous commented May 25, 2023

Nashatyrev commented May 25, 2023

ianopolous commented May 25, 2023 • edited Loading

ianopolous commented May 26, 2023 • edited Loading

Nashatyrev commented May 26, 2023

ianopolous commented May 26, 2023 • edited Loading

ianopolous commented May 26, 2023 • edited Loading

ianopolous commented May 26, 2023

Nashatyrev commented May 30, 2023

ianopolous commented May 30, 2023 • edited Loading

Nashatyrev May 31, 2023

Choose a reason for hiding this comment

ianopolous May 31, 2023

Choose a reason for hiding this comment

Nashatyrev left a comment

Choose a reason for hiding this comment

ianopolous commented May 24, 2023 •

edited by Nashatyrev

Loading

ianopolous commented May 24, 2023 •

edited

Loading

ianopolous commented May 25, 2023 •

edited

Loading

ianopolous commented May 26, 2023 •

edited

Loading

ianopolous commented May 26, 2023 •

edited

Loading

ianopolous commented May 26, 2023 •

edited

Loading

ianopolous commented May 30, 2023 •

edited

Loading