Various cspki improvements #293
Conversation
|
@jvehent is this ready for review? |
|
yeah let's land this and I'll make other PRs for the following patches. the rewrite of crypto11 means it'll take me a while to get the rest out the door... |
|
OK I wasn't sure if it was still WIP |
| } | ||
| } | ||
| err = s.findAndSetEE(conf, tx) | ||
| if err != nil { | ||
| if err == nil { | ||
| log.Printf("contentsignaturepki %q: reusing existing EE %q", s.ID, s.eeLabel) |
| if err == nil { | ||
| log.Printf("contentsignaturepki %q: reusing existing EE %q", s.ID, s.eeLabel) | ||
| } else { | ||
| // No suitable end-entity found, making a new chain | ||
| if err == database.ErrNoSuitableEEFound { |
There was a problem hiding this comment.
I think we could flatten these down to one else if ladder or switch case though I'm not sure if that's idiomatic
There was a problem hiding this comment.
funny enough, I did this a few minutes ago because I thought it looked ugly
|
I think we should stick to crypto11 <1.0 and upgrade later since that'll be a major change. |
Pull Request Test Coverage Report for Build 2853
💛 - Coveralls |
| } | ||
| s.issuerPriv, s.issuerPub, s.rand, _, err = conf.GetKeysAndRand() | ||
| // we need to parse or retrieve from the hsm the issuer private key, |
There was a problem hiding this comment.
A handle for the issuer private key, right? We shouldn't require it to be exportable to the autograph CU.
| @@ -76,7 +74,7 @@ func (s *ContentSigner) makeChain() (chain string, name string, err error) { | |||
| // valid for longer than that to account for clock skew | |||
| notAfter := time.Now().UTC().Add(s.validity + s.clockSkewTolerance) | |||
|
|
|||
| block, _ := pem.Decode([]byte(s.PublicKey)) | |||
| block, _ := pem.Decode([]byte(s.IssuerCert)) | |||
| if block == nil { | |||
| err = errors.New("no pem block found in signer public key configuration") | |||
There was a problem hiding this comment.
nit: does this log msg need to be updated?
|
Also, is this PR returning the EE pub key in the sig req response now? |
|
Alright, I ended up adding 9e335e4 to this PR after all. It's what we discussed yesterday wrt to multiple instances initializing in parallel without blocking, which will be required if we want to initialize signers on-demand rather that at init time. |
|
@g-k r? |
| if err != nil { | ||
| return errors.Wrapf(err, "contentsignaturepki %q: failed to generate end entity", s.ID) | ||
| } | ||
| // make the certificate and upload the chain |
There was a problem hiding this comment.
nit: we should log the handle, ID, and label of any keys were generate
| err = s.makeAndUploadChain() | ||
| if err != nil { | ||
| return errors.Wrapf(err, "contentsignaturepki %q: failed to make chain and x5u", s.ID) | ||
| } |
There was a problem hiding this comment.
nit: logging the chain and upload location on success would be useful
WIP