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

Add minimal NewCertificateFromX509 implementation #248

Merged
merged 5 commits into from May 30, 2023

Conversation

hslatman
Copy link
Member

@hslatman hslatman commented May 25, 2023

Minimal implementation of NewCertificateFromX509 to create an x509.Certificate without signing a CSR.

This PR replaces #239 and #247. We decided on keeping the logic simple, and to not add generic processing of x509.Certificate and x509.CertificateRequest for now.

The changes from the other PRs might still be of interest in the (near) future when more options are added for manipulating a x509.Certificate or x509.CertificateRequest, so we may reconsider the decision by then.

@hslatman hslatman force-pushed the herman/minimal-certificate-without-csr branch from e844e81 to f5e8c24 Compare May 26, 2023 13:44
@hslatman hslatman marked this pull request as ready for review May 26, 2023 13:45
@hslatman hslatman requested a review from maraino May 26, 2023 13:53
hslatman added a commit to smallstep/cli that referenced this pull request May 26, 2023
Instead of relying on a new implementation based on generics,
smallstep/crypto#248 was created to have
a minimal implementation for supporting signing public keys.
maraino
maraino previously approved these changes May 26, 2023
Copy link
Contributor

@maraino maraino left a comment

Choose a reason for hiding this comment

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

lgtm, but we should put the logic in one private method.

Comment on lines 107 to 135
// If no template use only the certificate request with the
// default leaf key usages.
if o.CertBuffer == nil {
return nil, errors.New("not implemented yet; use FromX509WithTemplate option")
}

// With templates
var cert Certificate
if err := json.NewDecoder(o.CertBuffer).Decode(&cert); err != nil {
return nil, errors.Wrap(err, "error unmarshaling certificate")
}

// Enforce the public key from the template
cert.PublicKey = template.PublicKey
cert.PublicKeyAlgorithm = template.PublicKeyAlgorithm

// Generate the subjectAltName extension if the certificate contains SANs
// that are not supported in the Go standard library.
if cert.hasExtendedSANs() && !cert.hasExtension(oidExtensionSubjectAltName) {
ext, err := createCertificateSubjectAltNameExtension(cert, cert.Subject.IsEmpty())
if err != nil {
return nil, err
}
// Prepend extension to achieve a certificate as similar as possible to
// the one generated by the Go standard library.
cert.Extensions = append([]Extension{ext}, cert.Extensions...)
}

return &cert, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

This is duplicated logic that can be combined into one internal function.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in f1025e3

Copy link
Contributor

@maraino maraino left a comment

Choose a reason for hiding this comment

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

lgtm

@hslatman hslatman merged commit cc1d031 into master May 30, 2023
13 checks passed
@hslatman hslatman deleted the herman/minimal-certificate-without-csr branch May 30, 2023 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants