Support for signing CSRs with challengePassword#381
Conversation
This commit adds the ChallengePassword field to the `x509util.CertificateRequest` struct. If this one is added, the method GetCertificateRequest will re-sign the CSR with the new attribute.
maraino force-pushed the mariano/challenge-password branch from 39463fc to b568818 Compare 
x509util/certificate_request.go Outdated
| return nil, errors.Wrap(err, "error creating certificate request") | ||
| } | ||
| | ||
| // Prepend challenge password and sign again |
There was a problem hiding this comment.
| // Prepend challenge password and sign again | |
| // If a challenge password is provided, encode and prepend it as | |
| // a challenge password attribute. [x509.CertificateRequest.Attributes] is marked deprecated, | |
| // so this requires some low level ASN1 operations. |
There was a problem hiding this comment.
A bit more commentary seemed useful to me.
Commit marking Attributes as deprecated: golang/go@ccd9d9d
Proposal for accessing PKCS10 attributes: golang/go#60718
There was a problem hiding this comment.
Fixed in 7a84055
I've changed the suggestion as it is not exactly like that. Go still supports setting attributes but is skipping those that do not match a format similar to the extensions.
I also saw the proposal tool.
x509util/certificate_request.go Outdated
| // Marshal challengePassword to ans1.RawValue | ||
| // Build challengePassword attribute (RFC 2985 section-5.4) |
There was a problem hiding this comment.
One of these maybe can be deleted, or combine in a single comment?
There was a problem hiding this comment.
Fixed in 7a84055, I left the old comment from my previous implementation where I was just using encoding/asn1.
| | ||
| b, err := builder.Bytes() | ||
| if err != nil { | ||
| return nil, errors.Wrap(err, "error marshaling challenge password") |
There was a problem hiding this comment.
| return nil, errors.Wrap(err, "error marshaling challenge password") | |
| return nil, fmt.Errorf("error marshaling challenge password: %w", err) |
Unless you want to keep the stack trace for this one.
There was a problem hiding this comment.
The majority of this package is using pkg/errors, so I'm doing it in the new code too. It is sometimes helpful. I'm not using it on new packages that I create.
There was a problem hiding this comment.
I also don't consider using always fmt.Errorf in all the crypto packages as the best option. If we decide to tackle the errors, we should consider using more typed errors.
| var signature []byte | ||
| signature, err = c.Signer.Sign(rand.Reader, signed, hashFunc) | ||
| if err != nil { | ||
| return nil, errors.Wrap(err, "error creating certificate request") |
There was a problem hiding this comment.
| return nil, errors.Wrap(err, "error creating certificate request") | |
| return nil, fmt.Errorf("error creating certificate request: %", err) |
Unless you want to keep the stack trace
| }, | ||
| }) | ||
| if err != nil { | ||
| return nil, errors.Wrap(err, "error creating certificate request") |
There was a problem hiding this comment.
| return nil, errors.Wrap(err, "error creating certificate request") | |
| return nil, fmt.Errorf("error creating certificate request: %w", err) |
Unless you want to keep the stack trace
x509util/certificate_request_test.go Outdated
| SignatureAlgorithm: SignatureAlgorithm(x509.SHA256WithRSA), | ||
| Signer: rsaKey, | ||
| }, expectedUTF8String, assert.NoError}, | ||
| {"fail challengePAssword", &CertificateRequest{ |
There was a problem hiding this comment.
| {"fail challengePAssword", &CertificateRequest{ | |
| {"fail challengePassword", &CertificateRequest{ |
| } | ||
| } | ||
| if !found { | ||
| return nil, errors.Errorf("error creating certificate request: unsupported signature algorithm %s", sigAlgoOID) |
There was a problem hiding this comment.
| return nil, errors.Errorf("error creating certificate request: unsupported signature algorithm %s", sigAlgoOID) | |
| return nil, fmt.Errorf("error creating certificate request: unsupported signature algorithm %s", sigAlgoOID) |
| // Marshal tbsCertificateRequest | ||
| tbsCSRContents, err := asn1.Marshal(tbsCSR) | ||
| if err != nil { | ||
| return nil, errors.Wrap(err, "error creating certificate request") |
There was a problem hiding this comment.
| return nil, errors.Wrap(err, "error creating certificate request") | |
| return nil, fmt.Errorf("error creating certificate request: %w", err) |
Unless you want to keep the stack trace
| | ||
| rest, err := asn1.Unmarshal(asn1Data, &csr) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
| return nil, err | |
| return nil, fmt.Errorf("error unmarshaling certificate request: %w", err) |
| SANs []SubjectAlternativeName `json:"sans"` | ||
| Extensions []Extension `json:"extensions"` | ||
| SignatureAlgorithm SignatureAlgorithm `json:"signatureAlgorithm"` | ||
| ChallengePassword string `json:"-"` |
There was a problem hiding this comment.
Instead of supporting this specific property like this, it might be an option to support it as a slice of Attributes? Then the ChallengePassword could be one specific Attribute. Processing it would be similar to []Extension, but with some custom logic for the additional (supported) attribute OIDs and values, and would allow other attributes to be set more easily too.
Probably good for a future iteration, though.
There was a problem hiding this comment.
A nice to have: support the challenge password in NewCertificateRequestFromX509 too?
There was a problem hiding this comment.
I was focusing on adding minimal support to create CSRs with it. I'm not focusing on parsing them, this can go in a future PR. Parsing requires a little bit more effort, but it's not hard.
2 participants
Description
This commit adds the ChallengePassword field to the
x509util.CertificateRequeststruct. If this one is added, the method GetCertificateRequest will re-sign the CSR with the new attribute.