Make tls cert binary compat with the go implementation #6

kpp · 2021-07-30T10:51:53Z

Make cert parsing and serialization compatible with the go implementation according to the specs.

I changed examples/smoke locally to test it, it worked (successfully connected).

dvc94ch

LGTM, although can't really judge since the tls code was copied. @DemiMarie any objections?

DemiMarie

Signing the SubjectPublicKeyInfo instead of the raw public key is fantastic, but the remaining changes should be reverted unless there is a good justification for them.

src/tls/verifier.rs

DemiMarie · 2021-07-30T11:23:27Z

src/tls/certificate.rs

        };

        let mut ext = rcgen::CustomExtension::from_oid_content(LIBP2P_OID, extension_content);
-        ext.set_criticality(true);
+        ext.set_criticality(false);


From an X.509 perspective, the extension is critical, in that the whole certificate is useless to a peer that cannot interpret it. That said, if using false here improved interoperability, I would be okay with it.

According to https://github.com/libp2p/specs/blob/master/tls/tls.md:

This extension MAY be marked critical.

I tried keep it, but the go implementation fails to connect:

go-libp2p-quic-transport/cmd$ go run client/main.go /ip4/127.0.0.1/udp/8383/quic 12D3KooWKnmstT37WmaiMRviHevoHZvWBrL7KsAXddcgrzqpp79i 2021/07/30 15:02:04 Dialing /ip4/127.0.0.1/udp/8383/quic 2021/07/30 15:02:04 failed to sufficiently increase receive buffer size (was: 208 kiB, wanted: 2048 kiB, got: 416 kiB). See https://github.com/lucas-clemente/quic-go/wiki/UDP-Receive-Buffer-Size for details. 2021/07/30 15:02:04 CRYPTO_ERROR (0x12a): certificate verification failed: x509: unhandled critical extension exit status 1

That is a bug in the Go implementation, and is most likely a result of them using the Go standard library to verify X.509 certificates. webpki has the same limitation, and in fact that limitation is why I wrote barebones-x509 in the first place.

That's right, here is the bug: https://github.com/libp2p/go-libp2p-tls/blob/master/crypto.go#L116.

However the extension MAY be marked as critical, so we don't have to.

Filed an issue: libp2p/go-libp2p-tls#87

so unresolving this because it seems it's go-libp2p that is broken

src/tls/certificate.rs

DemiMarie · 2021-07-30T11:31:29Z

LGTM, although can't really judge since the tls code was copied. @DemiMarie any objections?

Thanks for the ping. In short: signing the SubjectPublicKeyInfo is a very good change from a security perspective, for which I am grateful. The libp2p extension really should be critical, though, and most of the ASN.1 handling changes are not necessary.

examples/server.rs

src/tls/certificate.rs

DemiMarie · 2021-07-30T13:44:41Z

src/tls/certificate.rs

        };

        let mut ext = rcgen::CustomExtension::from_oid_content(LIBP2P_OID, extension_content);
-        ext.set_criticality(true);
+        ext.set_criticality(false);


That is a bug in the Go implementation, and is most likely a result of them using the Go standard library to verify X.509 certificates. webpki has the same limitation, and in fact that limitation is why I wrote barebones-x509 in the first place.

src/tls/verifier.rs

dvc94ch · 2021-07-30T15:45:18Z

Isn't it lovely when pl doesn't follow it's own specs? I recall their mdns spec being incompatible with their go implementation which was incompatible with their js implementation. Not sure if it's fixed yet, this was about 5y ago. I've generally given up on trying to be compliant.

dvc94ch

looks like merging this PR is blocked on go-libp2p getting it's act together?

dvc94ch · 2021-07-30T18:35:48Z

src/tls/certificate.rs

        };

        let mut ext = rcgen::CustomExtension::from_oid_content(LIBP2P_OID, extension_content);
-        ext.set_criticality(true);
+        ext.set_criticality(false);


so unresolving this because it seems it's go-libp2p that is broken

src/tls/certificate.rs

examples/server.rs

DemiMarie · 2021-08-02T08:38:38Z

looks like merging this PR is blocked on go-libp2p getting it's act together?

Using SubjectPublicKeyInfo instead of the raw public key is an enormous security win, so we should certainly keep that change! The previous behavior was not exploitable as far as I can tell, but that does not mean an exploit did not exist. Signing SubjectPublicKeyInfo, on the other hand, is cryptographically sound.

Using OCTET STRING instead of BIT STRING saves 2 bytes of wire overhead without any drawbacks I can think of, and matches both the Go and JS implementations.

That leaves the certificate criticality. While the certificate should be marked as critical, this is only a MAY in the specification for a reason. Leaving it as non-critical is okay.

In short, the only change that go-libp2p really needs to make is to allow the libp2p extension to be critical, and even that should not block rust-libp2p from supporting TLS and QUIC. Allowing the libp2p extension to be critical required me to write an entire X.509 parsing library, so I understand why the go-libp2p developers made the choice they did.

dvc94ch · 2021-08-02T08:40:22Z

Sounds reasonable. The example still needs to be improved or removed.

kpp · 2021-08-02T11:53:59Z

I was playing around with the QuicTransport but still could not make it work (none of the accept/dial futures actually resolve).
I will remove the example which I used to test your code.
Still there are a lot of questions left for me I would like to hear your advice to.

dvc94ch · 2021-08-02T12:00:07Z

none of the accept/dial futures actually resolve

the tests pass right? so that can't be right. you're probably not polling something maybe the stream muxer?

Still there are a lot of questions left for me I would like to hear your advice to.

why don't you want to use the swarm?

kpp · 2021-08-02T12:06:06Z

I've just tested the go version running server/main.go and then client/main.go. They don't work either! I spent two weeks trying to figure out what's wrong with my code.
I am deleting the example... Done

dvc94ch

Thank you!

dvc94ch reviewed Jul 30, 2021

View reviewed changes

DemiMarie suggested changes Jul 30, 2021

View reviewed changes

kpp commented Jul 30, 2021

View reviewed changes

examples/server.rs Outdated Show resolved Hide resolved

DemiMarie suggested changes Jul 30, 2021

View reviewed changes

Make tls certs binary compat with the go implementation

60c9e4c

kpp mentioned this pull request Aug 24, 2023

Tracking issue for QUIC support paritytech/polkadot-sdk#536

Open

10 tasks

dvc94ch reviewed Jul 30, 2021

View reviewed changes

dvc94ch approved these changes Aug 2, 2021

View reviewed changes

dvc94ch merged commit f2a8602 into ipfs-rust:master Aug 2, 2021

kpp deleted the go_compat branch August 2, 2021 13:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make tls cert binary compat with the go implementation #6

Make tls cert binary compat with the go implementation #6

kpp commented Jul 30, 2021

dvc94ch left a comment

DemiMarie left a comment

DemiMarie Jul 30, 2021

kpp Jul 30, 2021

DemiMarie Jul 30, 2021

kpp Jul 30, 2021 •

edited

Loading

dvc94ch Jul 30, 2021

DemiMarie commented Jul 30, 2021

DemiMarie Jul 30, 2021

dvc94ch commented Jul 30, 2021 •

edited

Loading

dvc94ch left a comment

dvc94ch Jul 30, 2021

DemiMarie commented Aug 2, 2021

dvc94ch commented Aug 2, 2021

kpp commented Aug 2, 2021

dvc94ch commented Aug 2, 2021

kpp commented Aug 2, 2021 •

edited

Loading

dvc94ch left a comment

Make tls cert binary compat with the go implementation #6

Make tls cert binary compat with the go implementation #6

Conversation

kpp commented Jul 30, 2021

dvc94ch left a comment

Choose a reason for hiding this comment

DemiMarie left a comment

Choose a reason for hiding this comment

DemiMarie Jul 30, 2021

Choose a reason for hiding this comment

kpp Jul 30, 2021

Choose a reason for hiding this comment

DemiMarie Jul 30, 2021

Choose a reason for hiding this comment

kpp Jul 30, 2021 • edited Loading

Choose a reason for hiding this comment

dvc94ch Jul 30, 2021

Choose a reason for hiding this comment

DemiMarie commented Jul 30, 2021

DemiMarie Jul 30, 2021

Choose a reason for hiding this comment

dvc94ch commented Jul 30, 2021 • edited Loading

dvc94ch left a comment

Choose a reason for hiding this comment

dvc94ch Jul 30, 2021

Choose a reason for hiding this comment

DemiMarie commented Aug 2, 2021

dvc94ch commented Aug 2, 2021

kpp commented Aug 2, 2021

dvc94ch commented Aug 2, 2021

kpp commented Aug 2, 2021 • edited Loading

dvc94ch left a comment

Choose a reason for hiding this comment

kpp Jul 30, 2021 •

edited

Loading

dvc94ch commented Jul 30, 2021 •

edited

Loading

kpp commented Aug 2, 2021 •

edited

Loading