Skip to content

Commit

Permalink
correct CheckHSMConnection error handling (#1000)
Browse files Browse the repository at this point in the history
With GetPrivKeyHandle removed in #992, we forgot to update the error
handling in CheckHSMConnection causing it to always return an error. Now
we rely soely on the earlier PrivateKeyHasPEMPrefix call for whether the
key is in the HSM.

Along the way, we cause main to error if the HSM connection check fails.
This allows autograph and the integration tests to fail immediately if
the HSM is believed to be unavailable.

We found this problem at staging deploy time because we weren't checking
the HSM connection on boot. The deploy was failing because the HSM
healthcheck failed, but those aren't checked in integration tests.
  • Loading branch information
jmhodges authored Sep 20, 2024
1 parent 153bb60 commit 5c446af
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 6 deletions.
16 changes: 12 additions & 4 deletions main.go
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,10 @@ func run(conf configuration, listen string, debug bool) {

// initialize the hsm if a configuration is defined
if conf.HSM.Path != "" {
ag.initHSM(conf)
err = ag.initHSM(conf)
if err != nil {
log.Fatalf("main.run: %s", err)
}
}

err = ag.addStats(conf)
Expand Down Expand Up @@ -376,7 +379,7 @@ func (a *autographer) addDB(dbConf database.Config) chan bool {
}

// initHSM sets up the HSM and notifies signers it is available
func (a *autographer) initHSM(conf configuration) {
func (a *autographer) initHSM(conf configuration) error {
tmpCtx, err := crypto11.Configure(&conf.HSM)
if err != nil {
log.Fatal(err)
Expand All @@ -386,15 +389,20 @@ func (a *autographer) initHSM(conf configuration) {
// tell the signers they can try using the HSM
for i := range conf.Signers {
conf.Signers[i].InitHSM(tmpCtx)
signerConf := conf.Signers[i]
signerConf := &conf.Signers[i]

// save the first signer with an HSM label as
// the key to test from the heartbeat handler
if a.heartbeatConf != nil && a.heartbeatConf.hsmSignerConf == nil && !signerConf.PrivateKeyHasPEMPrefix() {
a.heartbeatConf.hsmSignerConf = &signerConf
a.heartbeatConf.hsmSignerConf = signerConf
err := signerConf.CheckHSMConnection()
if err != nil {
return fmt.Errorf("hsm connection check failed during initHSM on signer id %#v: %w", signerConf.ID, err)
}
}
}
}
return nil
}

// addSigners initializes each signer specified in the configuration by parsing
Expand Down
4 changes: 2 additions & 2 deletions signer/signer.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ import (
"github.com/mozilla-services/autograph/formats"

"github.com/DataDog/datadog-go/statsd"
"github.com/mozilla-services/autograph/crypto11"
"github.com/miekg/pkcs11"
"github.com/mozilla-services/autograph/crypto11"

log "github.com/sirupsen/logrus"
)
Expand Down Expand Up @@ -407,7 +407,7 @@ func (cfg *Configuration) CheckHSMConnection() error {
if err != nil {
return fmt.Errorf("error fetching private key for signer %s: %w", cfg.ID, err)
}
return fmt.Errorf("unable to check HSM connection for signer %s private key is not stored in the HSM", cfg.ID)
return nil
}

// MakeKey generates a new key of type keyTpl and returns the priv and public interfaces.
Expand Down

0 comments on commit 5c446af

Please sign in to comment.