Add bundle_file_arn to plugin options for awssecret (spiffe#3578)

* Generate new certificates and keys on every test run

The static test keys and certificates that currently live in
testdata/keys/EC are brittle and won't pass additional validation added
to the plugin. The certificate is expired and if replaced we're just
kicking the can down the road.

This commit moves towards a setup similar to how the disk
upstreamauthority plugin works, with dynamically generated keys and
certificates that will never be out of date. Instead of static test
data, on each run we generate chains of keys and certificates with
generateTestData() which returns both a function that will wrap these
values in a fake secrets manager client and a struct containing the
keys/certs so that we can use the same values to assert against SPIRE's
returned X509CaChain and UpstreamX509Roots.

Signed-off-by: Jay Crumb <jay.crumb@unit21.ai>

* Add bundle_file_arn to plugin options for awssecret

This will allow us to specify addtional certificates that should
be added to the trust bundle when using non-self-signed certificates in
the cert_file_arn configuration field. Without this, it is only possible
to use the awssecret UpstreamAuthority with a self-signed cert.
Otherwise, you will end up with a partial chain when presented with the
trust bundle + client certificate chain.

This parameter is optional, and when omitted does not break existing
flows.

Signed-off-by: Jay Crumb <jay.crumb@unit21.ai>

* Add tests for specifying an intermediate as secret/key

This adds a few new testcases:

- Ensuring that bundle_file_arn is properly validated if it's present
- Ensuring that when specifying a bundle, the non self-signed CA
  specified in cert_file_arn is included in the resultant x509CaChain
  and the upstreamX509Roots contains the specified root

Signed-off-by: Jay Crumb <jay.crumb@unit21.ai>

* Add support for specifying an intermediate CA in cert_file_arn

This is a pretty major refactor of the awssecret plugin which allows
specifying an intermediate CA in cert_file_arn and brings this plugin
much more inline with the disk upstreamauthority. In addition to
allowing intermediates, we are now much stricter about checking the
provided certificate, key, and bundle at plugin load time. On init, we
now check that either cert_file_arn is a single self-signed CA or that
it is not self signed and can be verified with the roots specified in
bundle_file_arn.

Signed-off-by: Jay Crumb <jay.crumb@unit21.ai>

* Document new bundle_file_path parameter

This new parameter is optional as long as you are using a self-signed CA
in cert_file_arn. Otherwise bundle_file_arn should include one or more
roots to be included in the trust bundle, one of which must be the end
of the chain for the certificate specified in cert_file_arn.

Signed-off-by: Jay Crumb <jay.crumb@unit21.ai>

* Remove old static test cert/key

These are no longer needed since we're generating dynamic ones on every
test run now.

Signed-off-by: Jay Crumb <jay.crumb@unit21.ai>

* Update doc/plugin_server_upstreamauthority_awssecret.md

Signed-off-by: Jay Crumb <jay.crumb@unit21.ai>
Co-authored-by: Evan Gilman <evan2645@gmail.com>

Signed-off-by: Jay Crumb <jay.crumb@unit21.ai>
Co-authored-by: Evan Gilman <evan2645@gmail.com>

doc/plugin_server_upstreamauthority_awssecret.md

@@ -10,8 +10,9 @@ The plugin accepts the following configuration options:
 | Configuration     | Description                                           |
 |-------------------|-------------------------------------------------------|
 | region            | AWS Region that the AWS Secrets Manager is running in |
-| cert_file_arn     | ARN of the "upstream" CA certificate                  |
+| cert_file_arn     | ARN of the "upstream" CA certificate that will be used for signing. If more than one certificate is present, they will be added to the chain in order of appearance, where the first certificate will be the one used for signing.                 |
 | key_file_arn      | ARN of the "upstream" CA key file                     |
+| bundle_file_arn   | ARN of roots to include in the trust bundle. If `cert_file_arn` contains a self-signed root CA certificate this field can be left unset. Otherwise, `bundle_file_arn` must include one or more root CA certificates |
 | access_key_id     | AWS access key ID                                     |
 | secret_access_key | AWS secret access key                                 |
 | secret_token      | AWS secret token                                      |
@@ -40,6 +41,7 @@ A sample configuration:
             region = "us-west-2",
             cert_file_arn = "cert",
             key_file_arn = "key",
+            bundle_file_arn = "bundle",
             access_key_id = "ACCESS_KEY_ID",
             secret_access_key = "SECRET_ACCESS_KEY",
             secret_token = "SECRET_TOKEN"

pkg/server/plugin/upstreamauthority/awssecret/awslib_fake_test.go

@@ -2,17 +2,36 @@ package awssecret
 
 import (
 	"context"
+	"crypto"
+	"crypto/ecdsa"
+	"crypto/x509"
 	"fmt"
-	"os"
+	"math/big"
+	"net/url"
+	"testing"
+	"time"
 
 	"github.com/aws/aws-sdk-go-v2/aws"
 	"github.com/aws/aws-sdk-go-v2/service/secretsmanager"
+	"github.com/spiffe/spire/pkg/common/pemutil"
+	"github.com/spiffe/spire/test/clock"
+	"github.com/spiffe/spire/test/testca"
+	"github.com/spiffe/spire/test/testkey"
+	"github.com/stretchr/testify/require"
 )
 
 type fakeSecretsManagerClient struct {
 	storage map[string]string
 }
 
+type testKeysAndCerts struct {
+	rootKey          *ecdsa.PrivateKey
+	rootCert         *x509.Certificate
+	alternativeKey   *ecdsa.PrivateKey
+	intermediateKey  *ecdsa.PrivateKey
+	intermediateCert *x509.Certificate
+}
+
 func (sm *fakeSecretsManagerClient) GetSecretValue(ctx context.Context, input *secretsmanager.GetSecretValueInput, optFns ...func(*secretsmanager.Options)) (*secretsmanager.GetSecretValueOutput, error) {
 	if value, ok := sm.storage[*input.SecretId]; ok {
 		return &secretsmanager.GetSecretValueOutput{
@@ -23,35 +42,86 @@ func (sm *fakeSecretsManagerClient) GetSecretValue(ctx context.Context, input *s
 	return nil, fmt.Errorf("secret not found")
 }
 
-func newFakeSecretsManagerClient(ctx context.Context, config *Configuration, region string) (secretsManagerClient, error) {
+func generateTestData(t *testing.T, clk clock.Clock) (*testKeysAndCerts, func(context.Context, *Configuration, string) (secretsManagerClient, error)) {
+	var keys testkey.Keys
+
+	rootKey := keys.NewEC256(t)
+	rootCertificate := createCertificate(t, clk, "spiffe://root", rootKey, nil, nil)
+
+	intermediateKey := keys.NewEC256(t)
+	intermediateCertificate := createCertificate(t, clk, "spiffe://intermediate", intermediateKey, rootCertificate, rootKey)
+
+	alternativeKey := keys.NewEC256(t)
+
 	sm := new(fakeSecretsManagerClient)
 
-	if region == "" {
-		return nil, &aws.MissingRegionError{}
+	sm.storage = map[string]string{
+		"cert":              certToPEMstr(rootCertificate),
+		"key":               keyToPEMstr(t, rootKey),
+		"alternative_key":   keyToPEMstr(t, alternativeKey),
+		"bundle":            certToPEMstr(rootCertificate),
+		"intermediate_cert": certToPEMstr(intermediateCertificate),
+		"intermediate_key":  keyToPEMstr(t, intermediateKey),
+		"invalid_cert":      "no a certificate",
+		"invalid_key":       "no a key",
 	}
 
-	cert, err := os.ReadFile("testdata/keys/EC/cert.pem")
-	if err != nil {
-		return nil, err
+	keysAndCerts := &testKeysAndCerts{
+		rootKey:          rootKey,
+		rootCert:         rootCertificate,
+		alternativeKey:   alternativeKey,
+		intermediateKey:  intermediateKey,
+		intermediateCert: intermediateCertificate,
 	}
 
-	key, err := os.ReadFile("testdata/keys/EC/private_key.pem")
-	if err != nil {
-		return nil, err
+	makeSecretsManagerClient := func(ctx context.Context, config *Configuration, region string) (secretsManagerClient, error) {
+		if region == "" {
+			return nil, &aws.MissingRegionError{}
+		}
+		return sm, nil
 	}
 
-	alternativeKey, err := os.ReadFile("testdata/keys/EC/alternative_key.pem")
-	if err != nil {
-		return nil, err
+	return keysAndCerts, makeSecretsManagerClient
+}
+
+func createCertificate(
+	t *testing.T, clk clock.Clock,
+	uri string,
+	key crypto.Signer,
+	parent *x509.Certificate,
+	parentKey crypto.Signer,
+) *x509.Certificate {
+	now := clk.Now()
+
+	u, err := url.Parse(uri)
+	require.NoError(t, err)
+
+	template := &x509.Certificate{
+		SerialNumber:          big.NewInt(1),
+		BasicConstraintsValid: true,
+		IsCA:                  true,
+		NotBefore:             now,
+		NotAfter:              now.Add(time.Hour * 24),
+		URIs:                  []*url.URL{u},
 	}
 
-	sm.storage = map[string]string{
-		"cert":            string(cert),
-		"key":             string(key),
-		"alternative_key": string(alternativeKey),
-		"invalid_cert":    "no a certificate",
-		"invalid_key":     "no a key",
+	// Making the template and key their own parents
+	// generates a self-signed certificate
+	if parent == nil {
+		parent = template
+		parentKey = key
 	}
 
-	return sm, nil
+	return testca.CreateCertificate(t, template, parent, key.Public(), parentKey)
+}
+
+func certToPEMstr(cert *x509.Certificate) string {
+	return string(pemutil.EncodeCertificate(cert))
+}
+
+func keyToPEMstr(t *testing.T, key *ecdsa.PrivateKey) string {
+	data, err := pemutil.EncodeECPrivateKey(key)
+	require.NoError(t, err)
+
+	return string(data)
 }

pkg/server/plugin/upstreamauthority/awssecret/awssecret.go

@@ -2,7 +2,6 @@ package awssecret
 
 import (
 	"context"
-	"crypto"
 	"crypto/x509"
 	"os"
 	"sync"
@@ -42,6 +41,7 @@ type Configuration struct {
 	Region          string `hcl:"region" json:"region"`
 	CertFileARN     string `hcl:"cert_file_arn" json:"cert_file_arn"`
 	KeyFileARN      string `hcl:"key_file_arn" json:"key_file_arn"`
+	BundleFileARN   string `hcl:"bundle_file_arn" json:"bundle_file_arn"`
 	AccessKeyID     string `hcl:"access_key_id" json:"access_key_id"`
 	SecretAccessKey string `hcl:"secret_access_key" json:"secret_access_key"`
 	SecurityToken   string `hcl:"secret_token" json:"secret_token"`
@@ -54,9 +54,10 @@ type Plugin struct {
 
 	log hclog.Logger
 
-	mtx        sync.RWMutex
-	cert       *x509.Certificate
-	upstreamCA *x509svid.UpstreamCA
+	mtx           sync.RWMutex
+	upstreamCerts []*x509.Certificate
+	bundleCerts   []*x509.Certificate
+	upstreamCA    *x509svid.UpstreamCA
 
 	hooks struct {
 		clock     clock.Clock
@@ -94,8 +95,9 @@ func (p *Plugin) Configure(ctx context.Context, req *configv1.ConfigureRequest)
 		return nil, status.Errorf(codes.InvalidArgument, "failed to create AWS client: %v", err)
 	}
 
-	key, cert, err := fetchFromSecretsManager(ctx, config, sm)
+	keyPEMstr, certsPEMstr, bundleCertsPEMstr, err := fetchFromSecretsManager(ctx, config, sm)
 	if err != nil {
+		p.log.Error("Error loading files from AWS: %v", err)
 		return nil, err
 	}
 
@@ -107,13 +109,16 @@ func (p *Plugin) Configure(ctx context.Context, req *configv1.ConfigureRequest)
 	p.mtx.Lock()
 	defer p.mtx.Unlock()
 
-	p.cert = cert
-	p.upstreamCA = x509svid.NewUpstreamCA(
-		x509util.NewMemoryKeypair(cert, key),
-		trustDomain,
-		x509svid.UpstreamCAOptions{
-			Clock: p.hooks.clock,
-		})
+	upstreamCA, upstreamCerts, bundleCerts, err := p.loadUpstreamCAAndCerts(
+		trustDomain, keyPEMstr, certsPEMstr, bundleCertsPEMstr,
+	)
+	if err != nil {
+		return nil, err
+	}
+
+	p.upstreamCerts = upstreamCerts
+	p.bundleCerts = bundleCerts
+	p.upstreamCA = upstreamCA
 
 	return &configv1.ConfigureResponse{}, nil
 }
@@ -133,12 +138,12 @@ func (p *Plugin) MintX509CAAndSubscribe(request *upstreamauthorityv1.MintX509CAR
 		return status.Errorf(codes.Internal, "unable to sign CSR: %v", err)
 	}
 
-	x509CAChain, err := x509certificate.ToPluginProtos([]*x509.Certificate{cert})
+	x509CAChain, err := x509certificate.ToPluginProtos(append([]*x509.Certificate{cert}, p.upstreamCerts...))
 	if err != nil {
 		return status.Errorf(codes.Internal, "unable to form response X.509 CA chain: %v", err)
 	}
 
-	upstreamX509Roots, err := x509certificate.ToPluginProtos([]*x509.Certificate{p.cert})
+	upstreamX509Roots, err := x509certificate.ToPluginProtos(p.bundleCerts)
 	if err != nil {
 		return status.Errorf(codes.Internal, "unable to form response upstream X.509 roots: %v", err)
 	}
@@ -154,38 +159,99 @@ func (p *Plugin) PublishJWTKeyAndSubscribe(*upstreamauthorityv1.PublishJWTKeyReq
 	return status.Error(codes.Unimplemented, "publishing upstream is unsupported")
 }
 
-func fetchFromSecretsManager(ctx context.Context, config *Configuration, sm secretsManagerClient) (crypto.PrivateKey, *x509.Certificate, error) {
-	keyPEMstr, err := readARN(ctx, sm, config.KeyFileARN)
+func (p *Plugin) loadUpstreamCAAndCerts(trustDomain spiffeid.TrustDomain, keyPEMstr, certsPEMstr, bundleCertsPEMstr string) (*x509svid.UpstreamCA, []*x509.Certificate, []*x509.Certificate, error) {
+	key, err := pemutil.ParsePrivateKey([]byte(keyPEMstr))
 	if err != nil {
-		return nil, nil, status.Errorf(codes.InvalidArgument, "unable to read %s: %v", config.KeyFileARN, err)
+		return nil, nil, nil, status.Errorf(codes.Internal, "unable to parse private key: %v", err)
 	}
 
-	key, err := pemutil.ParsePrivateKey([]byte(keyPEMstr))
+	certs, err := pemutil.ParseCertificates([]byte(certsPEMstr))
 	if err != nil {
-		return nil, nil, status.Errorf(codes.Internal, "unable to parse private key: %v", err)
+		return nil, nil, nil, status.Errorf(codes.Internal, "unable to parse certificate: %v", err)
 	}
 
-	certPEMstr, err := readARN(ctx, sm, config.CertFileARN)
+	caCert := certs[0] // pemutil guarantees at least one cert
+
+	var trustBundle []*x509.Certificate
+	if bundleCertsPEMstr == "" {
+		// If there is no bundle payload configured then the value of certs
+		// must be a self-signed cert. We enforce this by requiring that there is
+		// exactly one certificate; this certificate is reused for the trust
+		// bundle and bundleCertsPEMstr is ignored
+		if len(certs) != 1 {
+			return nil, nil, nil, status.Error(codes.InvalidArgument, "with no bundle_file_arn configured only self-signed CAs are supported")
+		}
+		trustBundle = certs
+		certs = nil
+	} else {
+		// If there is a bundle, instead of using the payload of cert_file_arn
+		// to populate the trust bundle, we assume that certs is a chain of
+		// intermediates and populate the trust bundle with roots from
+		// bundle_file_arn
+		trustBundle, err = pemutil.ParseCertificates([]byte(bundleCertsPEMstr))
+		if err != nil {
+			return nil, nil, nil, status.Errorf(codes.InvalidArgument, "unable to load upstream CA bundle: %v", err)
+		}
+	}
+
+	matched, err := x509util.CertificateMatchesPrivateKey(caCert, key)
 	if err != nil {
-		return nil, nil, status.Errorf(codes.InvalidArgument, "unable to read %s: %v", config.CertFileARN, err)
+		return nil, nil, nil, status.Errorf(codes.InvalidArgument, "unable to verify CA cert matches private key: %v", err)
+	}
+	if !matched {
+		return nil, nil, nil, status.Error(codes.InvalidArgument, "unable to load upstream CA: certificate and private key do not match")
 	}
 
-	cert, err := pemutil.ParseCertificate([]byte(certPEMstr))
+	intermediates := x509.NewCertPool()
+	roots := x509.NewCertPool()
+
+	for _, c := range certs {
+		intermediates.AddCert(c)
+	}
+	for _, c := range trustBundle {
+		roots.AddCert(c)
+	}
+	selfVerifyOpts := x509.VerifyOptions{
+		Intermediates: intermediates,
+		Roots:         roots,
+	}
+	_, err = caCert.Verify(selfVerifyOpts)
 	if err != nil {
-		return nil, nil, status.Errorf(codes.Internal, "unable to parse certificate: %v", err)
+		return nil, nil, nil, status.Error(codes.InvalidArgument, "unable to load upstream CA: certificate could not be validated with the provided bundle or is not self signed")
 	}
 
-	// Validate cert matches private key
-	matched, err := x509util.CertificateMatchesPrivateKey(cert, key)
+	// If we get to this point we've successfully validated that:
+	// - cert_file_arn contains a single self-signed certificate OR
+	// - cert_file_arn contains a chain of certificates which terminate at a root
+	//   which is provided in bundle_file_arn
+	return x509svid.NewUpstreamCA(
+		x509util.NewMemoryKeypair(caCert, key),
+		trustDomain,
+		x509svid.UpstreamCAOptions{
+			Clock: p.hooks.clock,
+		},
+	), certs, trustBundle, nil
+}
+
+func fetchFromSecretsManager(ctx context.Context, config *Configuration, sm secretsManagerClient) (string, string, string, error) {
+	keyPEMstr, err := readARN(ctx, sm, config.KeyFileARN)
 	if err != nil {
-		return nil, nil, status.Errorf(codes.Internal, "unable to validate certificate: %v", err)
+		return "", "", "", status.Errorf(codes.InvalidArgument, "unable to read %s: %v", config.KeyFileARN, err)
 	}
 
-	if !matched {
-		return nil, nil, status.Errorf(codes.InvalidArgument, "certificate and private key does not match")
+	certsPEMstr, err := readARN(ctx, sm, config.CertFileARN)
+	if err != nil {
+		return "", "", "", status.Errorf(codes.InvalidArgument, "unable to read %s: %v", config.CertFileARN, err)
+	}
+	var bundlePEMstr string
+	if config.BundleFileARN != "" {
+		bundlePEMstr, err = readARN(ctx, sm, config.BundleFileARN)
+		if err != nil {
+			return "", "", "", status.Errorf(codes.InvalidArgument, "unable to read %s: %v", config.BundleFileARN, err)
+		}
 	}
 
-	return key, cert, nil
+	return keyPEMstr, certsPEMstr, bundlePEMstr, nil
 }
 
 func (p *Plugin) validateConfig(req *configv1.ConfigureRequest) (*Configuration, error) {