-
Notifications
You must be signed in to change notification settings - Fork 19
Conversation
Thanks for picking this up so quickly @marten-seemann. I believe there are a few other changes required to be compliant with that RFC. From a quick review it will also need
the general requirement (https://datatracker.ietf.org/doc/html/rfc3280#section-4.1.2):
I may not have captured everything, and there may be some additional fields required for a self-signed cert since it's also a CA. |
Not sure what to do here, we're using
I'm pretty sure we don't need to set this manually, at least parsing a certificate generated with |
You're right, version is populated by default: https://github.com/golang/go/blob/master/src/crypto/x509/x509.go#L1551-L1560 Looks like you just need to pass the The other fields like cURL specifically complains about the
|
That's weird. Probably not worth to debug this further, we can just set the time here. |
@peterargue Can you confirm if this works asap? We might be able to include this fix in the go-libp2p v0.16.0 release then. |
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 good. I've reached out to our community member that ran into this issue to test, but from code inspection of the java library, this should parse correctly.
crypto.go
Outdated
if err != nil { | ||
return nil, err | ||
} | ||
issuerSN, err := rand.Int(rand.Reader, bigNum) |
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.
nit: subjectSN
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 why does it have to be random? We could just have the peer ID or a constant string like libp2p.
It needs to be unique, so a constant string doesn’t work. Random because that will prevent people from using this value, which might break later when we see the need to change it. |
what's the concern here? that implementers might use the internal value you set within their logic and that could break if you decide to change the cert format? You could get around that by accepting a cert template. then implementers can define their own settings and the library is only responsible for adding the extension. You could also introduce a version to the |
so why not use the peer ID? |
Exactly. It's been a painful lesson learned from protocol design: If you're exposing something, someone will come to rely on this. That's why the IETF has introduced the concept of GREASE in recent years.
That seems out of scope for this PR. |
According to RFC3280, the issuer field must not be empty.
14f9cdd
to
22da1a4
Compare
1694: Update libp2p-tls to fix issue with secured gRPC certificate r=peterargue a=peterargue Depends on #1692, which updates the min go version to 1.16. Updates required to pull in this change: libp2p/go-libp2p-tls#100 The `libp2p/go-libp2p-tls` PR updates the certificate generated by the `NewIdentity` method to include subject and issuer fields, making it compliant with the x509 Certificate standard ([RFC3280](https://datatracker.ietf.org/doc/html/rfc3280#section-4.1.2)). This involves updating various other libp2p libraries, which now require go version 1.16. Co-authored-by: Peter Argue <[email protected]>
Fixes #97. @peterargue, can you confirm that this fixes the problem with the Java implementation?
According to RFC3280, the issuer field must not be empty.