Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(tls-certificate): generate and parse libp2p tls certificate #1209

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

feat(tls-certificate): generate and parse libp2p tls certificate #1209

wants to merge 8 commits into from
+585 −1

Conversation

diegomrsantos
Copy link
Contributor

@diegomrsantos diegomrsantos commented Oct 3, 2024 •
edited
Loading

This PR implements the certificate generation and parsing for https://github.com/libp2p/specs/blob/master/tls/tls.md.

lchenut
lchenut reviewed Oct 4, 2024
View reviewed changes
libp2p/transports/tls/certificate.nim Outdated
const extValueSize = 256 # Buffer size for ASN.1 encoding
var
extValue: array[extValueSize, byte]
extPtr: ptr byte = addr extValue[extValueSize - 1] # Start at the end of the buffer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first I thought it was an error, I had to check mbedtls_asn1_write_octet_string to understand that this function works backward. Maybe you can add this as a comment somewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

lchenut
lchenut reviewed Oct 4, 2024
View reviewed changes
libp2p/transports/tls/certificate.nim Outdated

# Exception types for TLS certificate errors
type
TLSCertificateError* = object of Exception
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it is inside nim-libp2p, maybe inherit from LPError instead of exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

lchenut
lchenut reviewed Oct 4, 2024
View reviewed changes
libp2p/transports/tls/certificate.nim
return extValueSeq

proc makeLibp2pExtension(
identityKeypair: KeyPair, certificateKeypair: mbedtls_pk_context
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's my C background talking, but I would use a ptr mbedtls_pk_context here instead of mbedtls_pk_context. It could work in Nim (I'm not sure if it uses a copy, a reference or a pointer for this kind of call), but as mbedtls only use pointer for their context, I would change that for our own function aswell.

Copy link
Contributor Author

@diegomrsantos diegomrsantos Oct 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the benefit of it? I don't think this is a pattern in Nim, is it?

lchenut
lchenut reviewed Oct 4, 2024
View reviewed changes
libp2p/transports/tls/certificate.nim
Comment on lines +411 to +449
for i in 0 ..< LIBP2P_EXT_OID_DER.len:
if ptrInc(oid.p, i.uint)[] != LIBP2P_EXT_OID_DER[i]:
return MBEDTLS_ERR_OID_NOT_FOUND # Extension not handled by this callback
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use nimCmpMem? Not sure if importing just for that is necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is it defined?

@diegomrsantos diegomrsantos changed the title generate and parse libp2p tls certificate feat(tls-certificate): generate and parse libp2p tls certificate Oct 10, 2024
@diegomrsantos diegomrsantos mentioned this pull request Oct 17, 2024
Open
12 tasks
@richard-ramos richard-ramos force-pushed the tls branch 2 times, most recently from aedd4d3 to e2e6b6b Compare February 6, 2025 21:24
@richard-ramos
Copy link
Member

The certificates that was being generated was considered invalid due to not having a serial number. This was fixed in last commit and now the certificates are readable in openssl:

openssl x509 -in certificate.pem  -serial -text
serial=0B0A736832F1AC4C4BF193F41386C5D772B9BFA7
Certificate:
    Data:
        Version: 3 (0x2)
        Serial Number:
            0b:0a:73:68:32:f1:ac:4c:4b:f1:93:f4:13:86:c5:d7:72:b9:bf:a7
        Signature Algorithm: ecdsa-with-SHA256
        Issuer: CN = libp2p.io
        Validity
            Not Before: Jan  1 00:00:00 1975 GMT
            Not After : Jan  1 00:00:00 4096 GMT
        Subject: CN = libp2p.io
        Subject Public Key Info:
            Public Key Algorithm: id-ecPublicKey
                Public-Key: (256 bit)
                pub:
                    04:67:fc:ae:4a:3b:0f:e9:fa:66:b3:a6:f1:08:3c:
                    13:f6:26:9c:0a:ef:ea:db:06:8c:bd:03:d6:b2:b7:
                    d1:ba:c9:1c:1a:bd:4c:7b:08:c9:58:26:86:a8:5f:
                    b6:ed:e4:00:3a:3d:b9:c3:9a:5a:4a:20:f0:48:62:
                    04:e0:3e:2e:47
                ASN1 OID: prime256v1
                NIST CURVE: P-256
        X509v3 extensions:
            1.3.6.1.4.1.53594.1.1:
                0o.%...!../4..I....$.c.OC..
.c......&m= j.F0D. ;U.
..w..`..-..~..-.......]..1.!. -0...l.P#.-.v..~d.>...!..:..4..=.
            X509v3 Basic Constraints:
                CA:FALSE
            X509v3 Key Usage: critical
                Digital Signature, Key Encipherment
    Signature Algorithm: ecdsa-with-SHA256
    Signature Value:
        30:46:02:21:00:f3:86:c5:b2:5f:0e:83:e9:f4:53:23:11:6f:
        68:b4:0e:0a:e0:93:ca:ee:3a:48:7c:72:6f:3a:16:e5:14:e2:
        39:02:21:00:90:76:3a:d6:64:2d:61:1b:5f:5e:a2:7e:f5:17:
        48:f6:f9:47:4b:f6:23:6d:ae:fb:14:87:15:09:dd:20:a4:16
-----BEGIN CERTIFICATE-----
MIIB0zCCAXagAwIBAgIUCwpzaDLxrExL8ZP0E4bF13K5v6cwDAYIKoZIzj0EAwIF
ADAUMRIwEAYDVQQDDAlsaWJwMnAuaW8wIBcNNzUwMTAxMDAwMDAwWhgPNDA5NjAx
MDEwMDAwMDBaMBQxEjAQBgNVBAMMCWxpYnAycC5pbzBZMBMGByqGSM49AgEGCCqG
SM49AwEHA0IABGf8rko7D+n6ZrOm8Qg8E/YmnArv6tsGjL0D1rK30brJHBq9THsI
yVgmhqhftu3kADo9ucOaWkog8EhiBOA+LkejgaEwgZ4wgYAGCisGAQQBg6JaAQEE
cjBvBCUIAhIhA64vNM/RSX+NwZMkh2OlT0MusQquY4cQFRAIGSZtPSBqBEYwRAIg
O1XNCuWYdwcXYPgWLa0ffujeLYm1seHt3ONd+KYxgCECIC0w68TbbB9QI7UtE3bv
B35k1z6P2fAhsxw6r5Y0hIk9ADAJBgNVHRMEAjAAMA4GA1UdDwEB/wQEAwIFoDAM
BggqhkjOPQQDAgUAA0kAMEYCIQDzhsWyXw6D6fRTIxFvaLQOCuCTyu46SHxybzoW
5RTiOQIhAJB2OtZkLWEbX16ifvUXSPb5R0v2I22u+xSHFQndIKQW
-----END CERTIFICATE-----

cc: @kaiserd

@richard-ramos richard-ramos marked this pull request as ready for review February 6, 2025 22:56
@richard-ramos richard-ramos requested a review from kaiserd February 7, 2025 01:18
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 7, 2025

Commits must follow the Conventional Commits specification
Please fix these commit messages:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lchenut lchenut lchenut left review comments

@kaiserd kaiserd Awaiting requested review from kaiserd

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
nim-libp2p
Status: In Progress
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@diegomrsantos @richard-ramos @lchenut @kaiserd