From f531e4441a25c426cfc6c4a3120b2871dc799187 Mon Sep 17 00:00:00 2001 From: "Ben Hearsum (he/him)" Date: Mon, 23 Sep 2024 19:39:19 -0400 Subject: [PATCH] refactor: isolate HSM interactions and crypto11 use in a more generic interface (#993) The main driver behind this is to enable us to add a `GCPHSM` with its own MakeKey implementation to avoid cluttering up `Configuration.MakeKey` further. While I was doing this, I moved the other HSM and `crypto11`` related things elsewhere as well. Notably, two things are still present in `signer.Configuration` that would be part of a `signer.HSM` implementation in an ideal world: * The pub key casting in `Configuration.GetKeys` * `Configuration.CheckHSMConnection` entirely Both of these things depend on `Configuration.GetPrivateKey` which needs a `label` of a key to fetch. I don't think it's appropriate to move that label to the `HSM` interface (because it would tie an instance to a single key). I couldn't find a way to adjust the usage of these methods that didn't end up making things messier, or involved refactoring other things, so I'm punting on it for now. --- main.go | 2 +- signer/hsm.go | 75 ++++++++++++++++++++++++++++++++++++++++++++++++ signer/signer.go | 58 ++++++++++--------------------------- 3 files changed, 91 insertions(+), 44 deletions(-) create mode 100644 signer/hsm.go diff --git a/main.go b/main.go index 6d16df452..645a7e791 100755 --- a/main.go +++ b/main.go @@ -388,7 +388,7 @@ func (a *autographer) initHSM(conf configuration) error { // if we successfully initialized the crypto11 context, // tell the signers they can try using the HSM for i := range conf.Signers { - conf.Signers[i].InitHSM(tmpCtx) + conf.Signers[i].InitHSM(signer.NewAWSHSM(tmpCtx)) signerConf := &conf.Signers[i] // save the first signer with an HSM label as diff --git a/signer/hsm.go b/signer/hsm.go new file mode 100644 index 000000000..2ad5e7a9f --- /dev/null +++ b/signer/hsm.go @@ -0,0 +1,75 @@ +package signer + +import ( + "crypto" + "crypto/ecdsa" + "crypto/rsa" + "fmt" + "io" + + "github.com/miekg/pkcs11" + "github.com/mozilla-services/autograph/crypto11" +) + +type HSM interface { + GetPrivateKey(label []byte) (crypto.PrivateKey, error) + MakeKey(keyTpl interface{}, keyName string) (crypto.PrivateKey, crypto.PublicKey, error) + GetRand() io.Reader +} + +type GenericHSM struct { + ctx *pkcs11.Ctx +} + +// GetPrivateKey locates the keypair given by `label` in the HSM. +func (hsm *GenericHSM) GetPrivateKey(label []byte) (crypto.PrivateKey, error) { + return crypto11.FindKeyPair(nil, label) +} + +func (hsm *GenericHSM) GetRand() io.Reader { + return new(crypto11.PKCS11RandReader) +} + +type AWSHSM struct { + GenericHSM +} + +// MakeKey generates a new keypair of type `keyTpl` and returns the new key structs. +func (hsm *AWSHSM) MakeKey(keyTpl interface{}, keyName string) (crypto.PrivateKey, crypto.PublicKey, error) { + var slots []uint + slots, err := hsm.ctx.GetSlotList(true) + if err != nil { + return nil, nil, fmt.Errorf("failed to list PKCS#11 Slots: %w", err) + } + if len(slots) < 1 { + return nil, nil, fmt.Errorf("failed to find a usable slot in hsm context") + } + keyNameBytes := []byte(keyName) + switch keyTplType := keyTpl.(type) { + case *ecdsa.PublicKey: + priv, err := crypto11.GenerateECDSAKeyPairOnSlot(slots[0], keyNameBytes, keyNameBytes, keyTplType) + if err != nil { + return nil, nil, fmt.Errorf("failed to generate ecdsa key in hsm: %w", err) + } + pub := priv.PubKey.(*ecdsa.PublicKey) + return priv, pub, nil + case *rsa.PublicKey: + keySize := keyTplType.Size() + priv, err := crypto11.GenerateRSAKeyPairOnSlot(slots[0], keyNameBytes, keyNameBytes, keySize) + if err != nil { + return nil, nil, fmt.Errorf("failed to generate rsa key in hsm: %w", err) + } + pub := priv.PubKey.(*rsa.PublicKey) + return priv, pub, nil + default: + return nil, nil, fmt.Errorf("making key of type %T is not supported", keyTpl) + } +} + +func NewAWSHSM(ctx *pkcs11.Ctx) *AWSHSM { + return &AWSHSM{ + GenericHSM{ + ctx, + }, + } +} diff --git a/signer/signer.go b/signer/signer.go index 1146cbcd7..ac2b01092 100644 --- a/signer/signer.go +++ b/signer/signer.go @@ -26,7 +26,6 @@ import ( "github.com/mozilla-services/autograph/formats" "github.com/DataDog/datadog-go/statsd" - "github.com/miekg/pkcs11" "github.com/mozilla-services/autograph/crypto11" log "github.com/sirupsen/logrus" @@ -130,13 +129,13 @@ type Configuration struct { SignerOpts crypto.SignerOpts `json:"signer_opts,omitempty" yaml:"signeropts,omitempty"` isHsmAvailable bool - hsmCtx *pkcs11.Ctx + Hsm HSM } // InitHSM indicates that an HSM has been initialized -func (cfg *Configuration) InitHSM(ctx *pkcs11.Ctx) { +func (cfg *Configuration) InitHSM(hsm HSM) { cfg.isHsmAvailable = true - cfg.hsmCtx = ctx + cfg.Hsm = hsm } // Signer is an interface to a configurable issuer of digital signatures @@ -251,7 +250,7 @@ type TestFileGetter interface { // HSM if available and otherwise rand.Reader func (cfg *Configuration) GetRand() io.Reader { if cfg.isHsmAvailable { - return new(crypto11.PKCS11RandReader) + return cfg.Hsm.GetRand() } return rand.Reader } @@ -307,12 +306,10 @@ func (cfg *Configuration) GetKeys() (priv crypto.PrivateKey, pub crypto.PublicKe // GetPrivateKey uses a signer configuration to determine where a private // key should be accessed from. If it is in local configuration, it will -// be parsed and loaded in the signer. If it is in an HSM, it will be -// used via a PKCS11 interface. This is completely transparent to the -// caller, who should simply assume that the privatekey implements a -// crypto.Sign interface -// -// Note that we assume the PKCS11 library has been previously initialized +// be parsed and loaded in the signer. If it is in an HSM, this will be +// outsourced to `cfg.Hsm`, which knows how to locate a private key handle +// in an HSM. Either way, the returned value implements the crypto.Sign +// interface. func (cfg *Configuration) GetPrivateKey() (crypto.PrivateKey, error) { cfg.PrivateKey = removePrivateKeyNewlines(cfg.PrivateKey) if cfg.PrivateKeyHasPEMPrefix() { @@ -320,7 +317,7 @@ func (cfg *Configuration) GetPrivateKey() (crypto.PrivateKey, error) { } // otherwise, we assume the privatekey represents a label in the HSM if cfg.isHsmAvailable { - key, err := crypto11.FindKeyPair(nil, []byte(cfg.PrivateKey)) + key, err := cfg.Hsm.GetPrivateKey([]byte(cfg.PrivateKey)) if err != nil { return nil, err } @@ -395,6 +392,8 @@ func (cfg *Configuration) PrivateKeyHasPEMPrefix() bool { // CheckHSMConnection (exposed via the signer.Configuration // interface). It tried to fetch the signer private key and errors if // that fails or the private key is not an HSM key handle. +// Ideally this would be part of the HSM interface, but the check requires +// the label of a key on the HSM, which is part of the Configuration func (cfg *Configuration) CheckHSMConnection() error { if cfg.PrivateKeyHasPEMPrefix() { return fmt.Errorf("private key for signer %s has a PEM prefix and is not an HSM key label", cfg.ID) @@ -410,39 +409,12 @@ func (cfg *Configuration) CheckHSMConnection() error { return nil } -// MakeKey generates a new key of type keyTpl and returns the priv and public interfaces. -// If an HSM is available, it is used to generate and store the key, in which case 'priv' -// just points to the HSM handler and must be used via the crypto.Signer interface. +// MakeKey generates a new key of type keyTpl and returns the private and +// public interfaces. If an HSM is available, this is outsourced to `cfg.Hsm`, +// which will generate them in an HSM instead of in memory. func (cfg *Configuration) MakeKey(keyTpl interface{}, keyName string) (priv crypto.PrivateKey, pub crypto.PublicKey, err error) { if cfg.isHsmAvailable { - var slots []uint - slots, err = cfg.hsmCtx.GetSlotList(true) - if err != nil { - return nil, nil, fmt.Errorf("failed to list PKCS#11 Slots: %w", err) - } - if len(slots) < 1 { - return nil, nil, fmt.Errorf("failed to find a usable slot in hsm context") - } - keyNameBytes := []byte(keyName) - switch keyTplType := keyTpl.(type) { - case *ecdsa.PublicKey: - priv, err = crypto11.GenerateECDSAKeyPairOnSlot(slots[0], keyNameBytes, keyNameBytes, keyTplType) - if err != nil { - return nil, nil, fmt.Errorf("failed to generate ecdsa key in hsm: %w", err) - } - pub = priv.(*crypto11.PKCS11PrivateKeyECDSA).PubKey.(*ecdsa.PublicKey) - return - case *rsa.PublicKey: - keySize := keyTplType.Size() - priv, err = crypto11.GenerateRSAKeyPairOnSlot(slots[0], keyNameBytes, keyNameBytes, keySize) - if err != nil { - return nil, nil, fmt.Errorf("failed to generate rsa key in hsm: %w", err) - } - pub = priv.(*crypto11.PKCS11PrivateKeyRSA).PubKey.(*rsa.PublicKey) - return - default: - return nil, nil, fmt.Errorf("making key of type %T is not supported", keyTpl) - } + return cfg.Hsm.MakeKey(keyTpl, keyName) } // no hsm, make keys in memory switch keyTplType := keyTpl.(type) {