Skip to content
This repository has been archived by the owner on Dec 10, 2021. It is now read-only.

Make tls cert binary compat with the go implementation #6

Merged
merged 1 commit into from
Aug 2, 2021
Merged

Make tls cert binary compat with the go implementation #6

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 5 additions & 14 deletions src/tls/certificate.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ use super::LIBP2P_SIGNING_PREFIX_LENGTH;
use libp2p::identity::Keypair;

const LIBP2P_OID: &[u64] = &[1, 3, 6, 1, 4, 1, 53594, 1, 1]; // Based on libp2p TLS 1.3 specs
const LIBP2P_SIGNATURE_ALGORITHM_PUBLIC_KEY_LENGTH: usize = 65;
const LIBP2P_SIGNATURE_ALGORITHM_PUBLIC_KEY_LENGTH: usize = 91;
static LIBP2P_SIGNATURE_ALGORITHM: &rcgen::SignatureAlgorithm = &rcgen::PKCS_ECDSA_P256_SHA256;

/// Generates a self-signed TLS certificate that includes a libp2p-specific
Expand All @@ -38,7 +38,7 @@ pub(crate) fn make_cert(keypair: &Keypair) -> Result<rcgen::Certificate, super::
// The libp2p-specific extension to the certificate contains a signature of the public key
// of the certificate using the libp2p private key.
let libp2p_ext_signature = {
let certif_pubkey = certif_keypair.public_key_raw();
let certif_pubkey = certif_keypair.public_key_der();
kpp marked this conversation as resolved.
Show resolved Hide resolved
assert_eq!(
certif_pubkey.len(),
LIBP2P_SIGNATURE_ALGORITHM_PUBLIC_KEY_LENGTH,
Expand All @@ -47,28 +47,19 @@ pub(crate) fn make_cert(keypair: &Keypair) -> Result<rcgen::Certificate, super::
let mut buf =
[0u8; LIBP2P_SIGNING_PREFIX_LENGTH + LIBP2P_SIGNATURE_ALGORITHM_PUBLIC_KEY_LENGTH];
buf[..LIBP2P_SIGNING_PREFIX_LENGTH].copy_from_slice(&super::LIBP2P_SIGNING_PREFIX[..]);
buf[LIBP2P_SIGNING_PREFIX_LENGTH..].copy_from_slice(certif_pubkey);
buf[LIBP2P_SIGNING_PREFIX_LENGTH..].copy_from_slice(&certif_pubkey);
keypair.sign(&buf)?
};

// Generate the libp2p-specific extension.
let libp2p_extension: rcgen::CustomExtension = {
let extension_content = {
let serialized_pubkey = keypair.public().into_protobuf_encoding();
yasna::construct_der(|writer| {
writer.write_sequence(|writer| {
writer
.next()
.write_bitvec_bytes(&serialized_pubkey, serialized_pubkey.len() * 8);
writer
.next()
.write_bitvec_bytes(&libp2p_ext_signature, libp2p_ext_signature.len() * 8);
})
})
yasna::encode_der(&(serialized_pubkey, libp2p_ext_signature))
dvc94ch marked this conversation as resolved.
Show resolved Hide resolved
};

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

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.

Copy link
Contributor Author

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

Copy link

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.

Copy link
Contributor Author

@kpp kpp Jul 30, 2021 •
edited
Loading

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

Copy link
Member

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

ext
};

Expand Down
6 changes: 4 additions & 2 deletions src/tls/verifier.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ fn verify_presented_certs(presented_certs: &[Certificate]) -> Result<(), TLSErro
certificate
.check_self_issued()
.map_err(TLSError::WebPKIError)?;
verify_libp2p_signature(&extension, certificate.subject_public_key_info().key())
verify_libp2p_signature(&extension, certificate.subject_public_key_info().spki())
.map_err(TLSError::WebPKIError)
}

Expand All @@ -204,7 +204,9 @@ struct Libp2pExtension<'a> {

fn parse_libp2p_extension(extension: Input<'_>) -> Result<Libp2pExtension<'_>, Error> {
fn read_bit_string<'a>(input: &mut Reader<'a>, e: Error) -> Result<Input<'a>, Error> {
der::bit_string_with_no_unused_bits(input).map_err(|_| e)
// The specification states that this is a BIT STRING, but the Go implementation
// uses an OCTET STRING. OCTET STRING is superior in this context, so use it.
der::expect_tag_and_get_value(input, der::Tag::OctetString).map_err(|_| e)
}

let e = Error::ExtensionValueInvalid;
Expand Down