-
Notifications
You must be signed in to change notification settings - Fork 4
Make tls cert binary compat with the go implementation #6
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, although can't really judge since the tls code was copied. @DemiMarie any objections?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
}; | ||
|
||
let mut ext = rcgen::CustomExtension::from_oid_content(LIBP2P_OID, extension_content); | ||
ext.set_criticality(true); | ||
ext.set_criticality(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so unresolving this because it seems it's go-libp2p that is broken
Thanks for the ping. In short: signing the |
}; | ||
|
||
let mut ext = rcgen::CustomExtension::from_oid_content(LIBP2P_OID, extension_content); | ||
ext.set_criticality(true); | ||
ext.set_criticality(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like merging this PR is blocked on go-libp2p getting it's act together?
}; | ||
|
||
let mut ext = rcgen::CustomExtension::from_oid_content(LIBP2P_OID, extension_content); | ||
ext.set_criticality(true); | ||
ext.set_criticality(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so unresolving this because it seems it's go-libp2p that is broken
Using Using 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. |
Sounds reasonable. The example still needs to be improved or removed. |
I was playing around with the QuicTransport but still could not make it work (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?
why don't you want to use the swarm? |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
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).