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 signature algorithm to template during certificate creation #1929

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Horiodino
Copy link
Contributor

@Horiodino Horiodino commented Jan 27, 2025

Summary

Added ToSignatureAlgorithm to support specifying non-default hash algorithms during certificate creation

#1161

Release Note

Documentation

Copy link
Contributor

@haydentherapper haydentherapper 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 overall! We need to also invoke it wherever we call x509.CreateCertificate, like in

finalCertBytes, err := x509.CreateCertificate(rand.Reader, cert, certChain[0], publicKey, privateKey)
or the other CA implementations in pkg/ca.

@@ -522,3 +541,47 @@ func ValidateKMSConfig(config KMSConfig) error {

return nil
}

// ToSignatureAlgorithm returns the x509.SignatureAlgorithm for the given signer and hash algorithm.
func ToSignatureAlgorithm(signer crypto.Signer, hash crypto.Hash) (x509.SignatureAlgorithm, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move this to https://github.com/sigstore/fulcio/blob/main/pkg/ca/common.go?

For context, pkg/certmaker was just added to support a new CLI utility, and I'd prefer to add new functions to it that are only used by the utility.

We should still keep the places above where you updated tmpl.SignatureAlgorithm though, that LGTM.

Copy link

codecov bot commented Jan 27, 2025

Codecov Report

Attention: Patch coverage is 77.55102% with 11 lines in your changes missing coverage. Please review.

Project coverage is 52.30%. Comparing base (cf238ac) to head (d29472e).
Report is 292 commits behind head on main.

Files with missing lines Patch % Lines
pkg/certmaker/certmaker.go 77.55% 8 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1929      +/-   ##
==========================================
- Coverage   57.93%   52.30%   -5.63%     
==========================================
  Files          50       73      +23     
  Lines        3119     5695    +2576     
==========================================
+ Hits         1807     2979    +1172     
- Misses       1154     2439    +1285     
- Partials      158      277     +119     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

Successfully merging this pull request may close these issues.

2 participants