Skip to content

Commit

Permalink
AUT-164: remove rsa key caching feature (#939)
Browse files Browse the repository at this point in the history 
This has been unused for ~3 years. This is essentially an unbitrotted version of #793.
  • Loading branch information
@bhearsum
bhearsum authored Aug 2, 2024
1 parent e701c90 commit d6da10e
Show file tree
Hide file tree
Showing 6 changed files with 12 additions and 245 deletions.
27 changes: 0 additions & 27 deletions signer/signer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,29 +34,6 @@ import (
// IDFormat is a regex for the format IDs must follow
const IDFormat = `^[a-zA-Z0-9-_]{1,64}$`

// RSACacheConfig is a config for the RSAKeyCache
type RSACacheConfig struct {
// NumKeys is the number of RSA keys matching the issuer size
// to cache
NumKeys uint64

// NumGenerators is the number of key generator workers to run
// that populate the RSA key cache
NumGenerators uint8

// GeneratorSleepDuration is how frequently each cache key
// generator tries to add a key to the cache chan
GeneratorSleepDuration time.Duration

// FetchTimeout is how long a consumer waits for the cache
// before generating its own key
FetchTimeout time.Duration

// StatsSampleRate is how frequently the monitor reports the
// cache size and capacity
StatsSampleRate time.Duration
}

// RecommendationConfig is a config for the XPI recommendation file
type RecommendationConfig struct {
// AllowedStates is a map of strings the signer is allowed to
Expand Down Expand Up @@ -99,10 +76,6 @@ type Configuration struct {
// certificate chain to validate a content signature
X5U string `json:"x5u,omitempty" yaml:"x5u,omitempty"`

// RSACacheConfig for XPI signers this specifies config for an
// RSA cache
RSACacheConfig RSACacheConfig `json:"rsacacheconfig,omitempty" yaml:"rsacacheconfig,omitempty"`

// RecommendationConfig specifies config values for
// recommendations files for XPI signers
RecommendationConfig RecommendationConfig `yaml:"recommendation,omitempty"`
Expand Down
11 changes: 1 addition & 10 deletions signer/xpi/README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,7 @@ signers:
-----END PRIVATE KEY-----
```
The signer can also include optional config params for an RSA key cache
and recommendations file:
The signer can also include optional config params for a recommendations file:
``` yaml
signers:
Expand All @@ -73,14 +72,6 @@ signers:
partner: true
relative_start: 0h
duration: 26298h
# RSA key gen is slow and CPU intensive, so we can optionally
# pregenerate and cache keys with a worker pool
rsacacheconfig:
numkeys: 25
numgenerators: 2
generatorsleepduration: 1m
fetchtimeout: 100ms
statssamplerate: 1m
certificate: |
-----BEGIN CERTIFICATE-----
MIIH0zCCBbugAwIBAgIBATANBgkqhkiG9w0BAQsFADCBvDELMAkGA1UEBhMCVVMx
Expand Down
12 changes: 0 additions & 12 deletions signer/xpi/omnija_bench_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,24 +2,12 @@ package xpi

import (
"testing"
"time"

"github.com/mozilla-services/autograph/signer"
)

func BenchmarkResignOmnija(b *testing.B) {
// initialize a system addon signer with an RSA key
testcase := validSignerConfigs[1]

// don't use an RSA key cache
testcase.RSACacheConfig = signer.RSACacheConfig{
NumKeys: 0,
NumGenerators: 0,
GeneratorSleepDuration: 10 * time.Minute,
FetchTimeout: 0,
StatsSampleRate: 10 * time.Minute,
}

s, err := New(testcase, nil)
if err != nil {
b.Fatalf("signer initialization failed with: %v", err)
Expand Down
61 changes: 1 addition & 60 deletions signer/xpi/x509.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,76 +12,17 @@ import (
"math/big"
"time"

log "github.com/sirupsen/logrus"
"go.mozilla.org/cose"
)

// populateRsaCache adds an rsa key to the cache every
// XPISigner.rsaCacheSleepDuration, blocks when the cache channel is
// full, and should be run as a goroutine
func (s *XPISigner) populateRsaCache(size int) {
var (
err error
key *rsa.PrivateKey
start time.Time
)
for {
start = time.Now()
key, err = rsa.GenerateKey(s.rand, size)
if err != nil {
log.Fatalf("xpi: error generating RSA key for cache: %q", err)
}
if key == nil {
log.Fatal("xpi: error generated nil RSA key for cache")
}

if s.stats != nil {
s.stats.SendHistogram("xpi.rsa_cache.gen_key_dur", time.Since(start))
}
s.rsaCache <- key
time.Sleep(s.rsaCacheGeneratorSleepDuration)
}
}

// monitorRsaCacheSize sends the number of cached keys and cache size
// to datadog. It should be run as a goroutine
func (s *XPISigner) monitorRsaCacheSize() {
if s.stats == nil {
return
}
for {
s.stats.SendGauge("xpi.rsa_cache.chan_len", len(s.rsaCache))

// chan capacity should be constant but is useful for
// knowing % cache filled across deploys
s.stats.SendGauge("xpi.rsa_cache.chan_cap", cap(s.rsaCache))

time.Sleep(s.rsaCacheSizeSampleRate)
}
}

// retrieve a key from the cache or generate one if it takes too long
// or if the size is wrong
func (s *XPISigner) getRsaKey(size int) (*rsa.PrivateKey, error) {
var (
err error
key *rsa.PrivateKey
start time.Time
)
start = time.Now()
select {
case key = <-s.rsaCache:
if key.N.BitLen() != size {
// it's theoritically impossible for this to happen
// because the end entity has the same key size has
// the signer, but we're paranoid so handling it
log.Warnf("WARNING: xpi rsa cache returned a key of size %d when %d was requested", key.N.BitLen(), size)
key, err = rsa.GenerateKey(s.rand, size)
}
case <-time.After(s.rsaCacheFetchTimeout):
// generate a key if none available
key, err = rsa.GenerateKey(s.rand, size)
}
key, err = rsa.GenerateKey(s.rand, size)

if s.stats != nil {
s.stats.SendHistogram("xpi.rsa_cache.get_key", time.Since(start))
Expand Down
40 changes: 1 addition & 39 deletions signer/xpi/xpi.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,22 +83,6 @@ type XPISigner struct {
// rand is the CSPRNG to use from the HSM or crypto/rand
rand io.Reader

// rsa cache is used to pre-generate RSA private keys and speed up
// the signing process
rsaCache chan *rsa.PrivateKey

// rsaCacheGeneratorSleepDuration is how frequently each cache key
// generator tries to add a key to the cache chan
rsaCacheGeneratorSleepDuration time.Duration

// rsaCacheFetchTimeout is how long a consumer waits for the
// cache before generating its own key
rsaCacheFetchTimeout time.Duration

// rsaCacheSizeSampleRate is how frequently the monitor
// reports the cache size and capacity
rsaCacheSizeSampleRate time.Duration

// stats is the statsd client for reporting metrics
stats *signer.StatsClient

Expand Down Expand Up @@ -208,33 +192,11 @@ func New(conf signer.Configuration, stats *signer.StatsClient) (s *XPISigner, er
s.recommendationFilePath = conf.RecommendationConfig.FilePath
log.Infof("xpi: signer %q is ignoring recommendation file path %q", s.ID, s.recommendationFilePath)

// If the private key is rsa, launch go routines that
// populates the rsa cache with private keys of the same
// length
// If the private key is rsa, sanity check the length
if issuerKey, ok := s.issuerKey.(*rsa.PrivateKey); ok {
if issuerKey.N.BitLen() < rsaKeyMinSize {
return nil, fmt.Errorf("xpi: issuer RSA key must be at least %d bits", rsaKeyMinSize)
}
if conf.RSACacheConfig.StatsSampleRate < 5*time.Second {
log.Warnf("xpi: sampling rsa cache as rate of %q (less than 5s)", conf.RSACacheConfig.StatsSampleRate)
}
s.rsaCacheGeneratorSleepDuration = conf.RSACacheConfig.GeneratorSleepDuration
s.rsaCacheFetchTimeout = conf.RSACacheConfig.FetchTimeout
s.rsaCacheSizeSampleRate = conf.RSACacheConfig.StatsSampleRate

s.rsaCache = make(chan *rsa.PrivateKey, conf.RSACacheConfig.NumKeys)
for i := 0; i < int(conf.RSACacheConfig.NumGenerators); i++ {
go s.populateRsaCache(issuerKey.N.BitLen())
}

log.Infof("xpi: %d RSA key cache started with %d generators running every %q\n and a %q timeout", conf.RSACacheConfig.NumKeys, conf.RSACacheConfig.NumGenerators, s.rsaCacheGeneratorSleepDuration, s.rsaCacheFetchTimeout)

if s.stats == nil {
log.Warnf("xpi: No statsd client found to send RSA cache metrics")
} else {
go s.monitorRsaCacheSize()
log.Infof("xpi: Started RSA cache monitor")
}
}

return
Expand Down
106 changes: 9 additions & 97 deletions signer/xpi/xpi_test.go
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
package xpi

import (
"crypto/rand"
"crypto/rsa"
"crypto/x509"
"encoding/pem"
"fmt"
Expand Down Expand Up @@ -48,16 +46,12 @@ func TestSignFile(t *testing.T) {
t.Run(fmt.Sprintf("test sign file %s signer id %s (%d)", input.name, testcase.ID, i), func(t *testing.T) {
t.Parallel()

// initialize a signer
testcase.RSACacheConfig = signer.RSACacheConfig{
NumKeys: 5,
NumGenerators: 2,
GeneratorSleepDuration: time.Minute,
FetchTimeout: 100 * time.Millisecond,
StatsSampleRate: 10 * time.Second,
statsdClient, err := statsd.NewBuffered("localhost:8135", 1)
if err != nil {
t.Fatalf("passing testcase %d: Error constructing statsdClient: %v", i, err)
}

signerStatsClient, err := signer.NewStatsClient(testcase, &statsd.NoOpClient{})
statsdClient.Namespace = "test_autograph_stats_ns"
signerStatsClient, err := signer.NewStatsClient(testcase, statsdClient)
if err != nil {
t.Fatalf("passing testcase %d: Error constructing signer.StatsdClient: %v", i, err)
}
Expand Down Expand Up @@ -280,11 +274,12 @@ func TestNewFailure(t *testing.T) {

for i, testcase := range invalidSignerConfigs {
_, err := New(testcase.cfg, nil)
if !strings.Contains(err.Error(), testcase.err) {
t.Fatalf("testcase %d expected to fail with '%v' but failed with '%v' instead", i, testcase.err, err)
}
// check for lack of error first, otherwise `err.Error()` will cause a panic if no error
// is present.
if err == nil {
t.Fatalf("testcase %d expected to fail with '%v' but succeeded", i, testcase.err)
} else if !strings.Contains(err.Error(), testcase.err) {
t.Fatalf("testcase %d expected to fail with '%v' but failed with '%v' instead", i, testcase.err, err)
}
}
}
Expand Down Expand Up @@ -491,89 +486,6 @@ func TestVerifyUnfinishedSignature(t *testing.T) {
}
}

func TestRsaCaching(t *testing.T) {
t.Parallel()

// initialize an RSA signer with cache
testcase := validSignerConfigs[0]
testcase.RSACacheConfig.NumKeys = 2
testcase.RSACacheConfig.NumGenerators = 0 // we'll run populateRsaCache directly
s, err := New(testcase, nil)
if err != nil {
t.Fatalf("signer initialization failed with: %v", err)
}
keySize := s.issuerKey.(*rsa.PrivateKey).N.BitLen()

// should drop cached key with an invalid size and generate a new one
smallKey, err := rsa.GenerateKey(s.rand, 512)
if err != nil {
t.Fatalf("generating test key failed with: %v", err)
}
go func() { s.rsaCache <- smallKey }()
if os.Getenv("CI") == "true" {
// sleep longer when running in continuous integration
time.Sleep(30 * time.Second)
} else {
time.Sleep(10 * time.Second)
}
if len(s.rsaCache) != 1 {
t.Fatalf("have an unexpected number of keys in chan wanted 1 got: %d", len(s.rsaCache))
}
key, err := s.getRsaKey(keySize)
if err != nil {
t.Fatalf("signer initialization failed with: %v", err)
}
if key.N.BitLen() != keySize {
t.Fatalf("key bitlen does not match. expected %d, got %d", keySize, key.N.BitLen())
}

go s.populateRsaCache(keySize)
if os.Getenv("CI") == "true" {
// sleep longer when running in continuous integration
time.Sleep(30 * time.Second)
} else {
time.Sleep(10 * time.Second)
}
// retrieving a rsa key should be really fast now
start := time.Now()
key, err = s.getRsaKey(keySize)
if err != nil {
t.Fatalf("signer initialization failed with: %v", err)
}
cachedElapsed := time.Since(start)
t.Logf("retrieved rsa key from cache in %q", cachedElapsed)

start = time.Now()
rsa.GenerateKey(rand.Reader, keySize)
generatedElapsed := time.Since(start)
t.Logf("generated rsa key without cache in %q", generatedElapsed)

if cachedElapsed > generatedElapsed {
t.Fatal("key retrieval from populated cache took longer than generating directly")
}
if key.N.BitLen() != keySize {
t.Fatalf("key bitlen does not match. expected %d, got %d", keySize, key.N.BitLen())
}
}

func TestRSACacheSizeMonitor(t *testing.T) {

t.Run("runs without statsd", func(t *testing.T) {
t.Parallel()

// initialize an RSA signer with cache
testcase := validSignerConfigs[0]
s, err := New(testcase, nil)
if err != nil {
t.Fatalf("signer initialization failed with: %v", err)
}

s.stats = nil
// this should not panic
s.monitorRsaCacheSize()
})
}

func TestSignFileWithCOSESignatures(t *testing.T) {
t.Parallel()

Expand Down

0 comments on commit d6da10e

Please sign in to comment.