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.

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

Support generating custom x509 certificates #1481

Merged

Conversation

peterargue
Copy link
Contributor

@peterargue peterargue commented May 10, 2022

This PR was originally opened here libp2p/go-libp2p-tls#99 and relates to this issue: fixes #1538

Context

libp2ptls.NewIdentity generates a new Identity which includes an x509 certificate with a special signed extension which ties the TLS key pair to a libp2p network identity. In flow, we use this key pair to secure a gRPC interface which is accessible to nodes on the network as well as external public clients, allowing them to verify that they've connected to the correct staked node.

The current implementation of NewIdentity does not support customizing the certificate template, and uses default options that aren't compatible with standard TLS clients (e.g. Subject doesn't include a cn field).

Proposal

This PR refactors keyToCertificate into 3 methods

  • GenerateSignedExtension (exported), which generates the signed extension
  • certTemplate which returns the default certificate template used to generate the self-signed certificate,
  • keyToCertificate, which accepts an x509.Certificate template, adds the signed extension and returns the tls.Certificate object.

This PR also adds a new optional parameter to NewIdentity, which can provide a custom certificate template.

The changes are intended to be backwards compatible and add the ability for a caller to specify the x509 certificate fields they wish to include in the generated certificate.

@peterargue
Copy link
Contributor Author

@marten-seemann here's the new PR

@MarcoPolo MarcoPolo self-requested a review May 16, 2022 18:58
Copy link
Collaborator

@MarcoPolo MarcoPolo left a comment

Choose a reason for hiding this comment

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

This looks good to me, but I'll let @marten-seemann submit the approval. Thanks for this and moving the PR over here!

@peterargue
Copy link
Contributor Author

@marten-seemann this look OK to you?

Copy link
Collaborator

@MarcoPolo MarcoPolo left a comment

Choose a reason for hiding this comment

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

I approve this, but I want @marten-seemann to look too since this touches certs and crypto.

@MarcoPolo
Copy link
Collaborator

friendly ping @marten-seemann

Copy link
Contributor

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

I added a few comments. Other than that, the code looks good to me.
We should probably also have a test where we perform a handshake using a custom certificate, to make sure things work end-to-end.

}

// DefaultCertTemplate returns the default template for generating an Identity's TLS certificates.
func DefaultCertTemplate() (*x509.Certificate, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function doesn't seem worth exposing. All that we're doing here is set a serial number, the subject and the validity period.
It's also slightly misleading, as the Default implies that it's safe to use for multiple identities, which it is not.

// GenerateSignedExtension uses the provided private key to sign the public key, and returns the
// signature within a pkix.Extension.
// This extension is included in a certificate to cryptographically tie it to the libp2p private key.
func GenerateSignedExtension(sk ic.PrivKey, pubKey crypto.PublicKey) (*pkix.Extension, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: return a pkix.Extension here. It's a very small struct.

@peterargue
Copy link
Contributor Author

Thanks @marten-seemann. I just pushed some changes addressing your comments.

@peterargue
Copy link
Contributor Author

Hey @marten-seemann think this PR is ok to merge?

@marten-seemann
Copy link
Contributor

Yes, it is. I approved it. But we won't merge it before the v0.21 release.

@MarcoPolo
Copy link
Collaborator

Right after #1648, we’ll merge this. We wanted to make this release pretty small and we’ve started running some smoke tests for v0.21.

@peterargue
Copy link
Contributor Author

awesome. thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

tls: allow generating TLS key pairs for use outside of libp2p
3 participants