From 85fdb738e76f6923944b60b72d8156f36b3d60b7 Mon Sep 17 00:00:00 2001 From: Hugo Shaka Date: Thu, 25 Jul 2024 17:09:52 -0400 Subject: [PATCH] Reuse existing SAMLConnector signing key when possible (#44655) * Reuse existing SAMLConnector signing key when possible * lint --- lib/auth/saml.go | 30 ++++++++++ lib/services/saml.go | 26 +++++++++ lib/services/saml_test.go | 112 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 168 insertions(+) diff --git a/lib/auth/saml.go b/lib/auth/saml.go index f295508ea360b..c193bd2ecec8b 100644 --- a/lib/auth/saml.go +++ b/lib/auth/saml.go @@ -58,6 +58,16 @@ func (a *Server) UpsertSAMLConnector(ctx context.Context, connector types.SAMLCo if err := services.ValidateSAMLConnector(connector, a); err != nil { return nil, trace.Wrap(err) } + + // If someone is applying a SAML Connector obtained with `tctl get` without secrets, the signing key pair is + // not empty (cert is set) but the private key is missing. Such a SAML resource is invalid and not usable. + if connector.GetSigningKeyPair().PrivateKey == "" { + err := services.FillSAMLSigningKeyFromExisting(ctx, connector, a.Services) + if err != nil { + return nil, trace.Wrap(err) + } + } + upserted, err := a.Services.UpsertSAMLConnector(ctx, connector) if err != nil { return nil, trace.Wrap(err) @@ -86,6 +96,18 @@ func (a *Server) UpdateSAMLConnector(ctx context.Context, connector types.SAMLCo if err := services.ValidateSAMLConnector(connector, a); err != nil { return nil, trace.Wrap(err) } + + // If someone is applying a SAML Connector obtained with `tctl get` without secrets, the signing key pair is + // not empty (cert is set) but the private key is missing. In this case we want to look up the existing SAML + // connector and populate the singing key from it if it's the same certificate. This avoids accidentally clearing + // the private key and creating an unusable connector. + if connector.GetSigningKeyPair().PrivateKey == "" { + err := services.FillSAMLSigningKeyFromExisting(ctx, connector, a.Services) + if err != nil { + return nil, trace.Wrap(err) + } + } + updated, err := a.Services.UpdateSAMLConnector(ctx, connector) if err != nil { return nil, trace.Wrap(err) @@ -114,6 +136,14 @@ func (a *Server) CreateSAMLConnector(ctx context.Context, connector types.SAMLCo if err := services.ValidateSAMLConnector(connector, a); err != nil { return nil, trace.Wrap(err) } + + // If someone is applying a SAML Connector obtained with `tctl get` without secrets, the signing key pair is + // not empty (cert is set) but the private key is missing. This SAML Connector is invalid, we must reject it + // with an actionable message. + if connector.GetSigningKeyPair().PrivateKey == "" { + return nil, trace.BadParameter("Missing private key for signing connector. " + services.ErrMsgHowToFixMissingPrivateKey) + } + created, err := a.Services.CreateSAMLConnector(ctx, connector) if err != nil { return nil, trace.Wrap(err) diff --git a/lib/services/saml.go b/lib/services/saml.go index 5b6c0b575db41..b31012eeaab65 100644 --- a/lib/services/saml.go +++ b/lib/services/saml.go @@ -41,6 +41,12 @@ import ( "github.com/gravitational/teleport/lib/utils" ) +type SAMLConnectorGetter interface { + GetSAMLConnector(ctx context.Context, id string, withSecrets bool) (types.SAMLConnector, error) +} + +const ErrMsgHowToFixMissingPrivateKey = "You must either specify the singing key pair (obtain the existing one with `tctl get saml --with-secrets`) or let Teleport generate a new one (remove singing_key_pair in the resource you're trying to create)." + // ValidateSAMLConnector validates the SAMLConnector and sets default values. // If a remote to fetch roles is specified, roles will be validated to exist. func ValidateSAMLConnector(sc types.SAMLConnector, rg RoleGetter) error { @@ -333,3 +339,23 @@ func MarshalSAMLConnector(samlConnector types.SAMLConnector, opts ...MarshalOpti return nil, trace.BadParameter("unrecognized SAML connector version %T", samlConnector) } } + +// FillSAMLSigningKeyFromExisting looks up the existing SAML connector and populates the signing key if it's missing. +// This must be called only if the SAML Connector signing key pair has been initialized (ValidateSAMLConnector) and +// the private key is still empty. +func FillSAMLSigningKeyFromExisting(ctx context.Context, connector types.SAMLConnector, sg SAMLConnectorGetter) error { + existing, err := sg.GetSAMLConnector(ctx, connector.GetName(), true /* with secrets */) + switch { + case trace.IsNotFound(err): + return trace.BadParameter("failed to create SAML connector, the SAML connector has no signing key set. " + ErrMsgHowToFixMissingPrivateKey) + case err != nil: + return trace.BadParameter("failed to update SAML connector, the SAML connector has no signing key set and looking up the existing connector failed with the error: %s. %s", err.Error(), ErrMsgHowToFixMissingPrivateKey) + } + + existingSkp := existing.GetSigningKeyPair() + if existingSkp == nil || existingSkp.Cert != connector.GetSigningKeyPair().Cert { + return trace.BadParameter("failed to update the SAML connector, the SAML connector has no signing key and its signing certificate does not match the existing one. " + ErrMsgHowToFixMissingPrivateKey) + } + connector.SetSigningKeyPair(existingSkp) + return nil +} diff --git a/lib/services/saml_test.go b/lib/services/saml_test.go index c73c37b7c87ab..ab060dd218d9f 100644 --- a/lib/services/saml_test.go +++ b/lib/services/saml_test.go @@ -20,8 +20,10 @@ package services import ( "context" + "crypto/x509/pkix" "strings" "testing" + "time" "github.com/gravitational/trace" "github.com/stretchr/testify/require" @@ -30,6 +32,7 @@ import ( "github.com/gravitational/teleport/api/types" "github.com/gravitational/teleport/lib/defaults" "github.com/gravitational/teleport/lib/fixtures" + "github.com/gravitational/teleport/lib/utils" ) func TestParseFromMetadata(t *testing.T) { @@ -127,6 +130,115 @@ func TestValidateRoles(t *testing.T) { } } +type mockSAMLGetter map[string]types.SAMLConnector + +func (m mockSAMLGetter) GetSAMLConnector(_ context.Context, id string, withSecrets bool) (types.SAMLConnector, error) { + connector, ok := m[id] + if !ok { + return nil, trace.NotFound("%s not found", id) + } + return connector, nil +} + +func TestFillSAMLSigningKeyFromExisting(t *testing.T) { + t.Parallel() + + // Test setup: generate the fixtures + existingKeyPEM, existingCertPEM, err := utils.GenerateSelfSignedSigningCert(pkix.Name{ + Organization: []string{"Teleport OSS"}, + CommonName: "teleport.localhost.localdomain", + }, nil, 10*365*24*time.Hour) + require.NoError(t, err) + + existingSkp := &types.AsymmetricKeyPair{ + PrivateKey: string(existingKeyPEM), + Cert: string(existingCertPEM), + } + + existingConnectorName := "existing" + existingConnectors := mockSAMLGetter{ + existingConnectorName: &types.SAMLConnectorV2{ + Spec: types.SAMLConnectorSpecV2{ + SigningKeyPair: existingSkp, + }, + }, + } + + _, unrelatedCertPEM, err := utils.GenerateSelfSignedSigningCert(pkix.Name{ + Organization: []string{"Teleport OSS"}, + CommonName: "teleport.localhost.localdomain", + }, nil, 10*365*24*time.Hour) + require.NoError(t, err) + + // Test setup: define test cases + testCases := []struct { + name string + connectorName string + connectorSpec types.SAMLConnectorSpecV2 + assertErr require.ErrorAssertionFunc + assertResult require.ValueAssertionFunc + }{ + { + name: "should read singing key from existing connector with matching cert", + connectorName: existingConnectorName, + connectorSpec: types.SAMLConnectorSpecV2{ + SigningKeyPair: &types.AsymmetricKeyPair{ + PrivateKey: "", + Cert: string(existingCertPEM), + }, + }, + assertErr: require.NoError, + assertResult: func(t require.TestingT, value interface{}, args ...interface{}) { + require.Implements(t, (*types.SAMLConnector)(nil), value) + connector := value.(types.SAMLConnector) + skp := connector.GetSigningKeyPair() + require.Equal(t, existingSkp, skp) + }, + }, + { + name: "should error when there's no existing connector", + connectorName: "non-existing", + connectorSpec: types.SAMLConnectorSpecV2{ + SigningKeyPair: &types.AsymmetricKeyPair{ + PrivateKey: "", + Cert: string(unrelatedCertPEM), + }, + }, + assertErr: require.Error, + }, + { + name: "should error when existing connector cert is not matching", + connectorName: existingConnectorName, + connectorSpec: types.SAMLConnectorSpecV2{ + SigningKeyPair: &types.AsymmetricKeyPair{ + PrivateKey: "", + Cert: string(unrelatedCertPEM), + }, + }, + assertErr: require.Error, + }, + } + + // Test execution + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + connector := &types.SAMLConnectorV2{ + Metadata: types.Metadata{ + Name: tc.connectorName, + }, + Spec: tc.connectorSpec, + } + + err := FillSAMLSigningKeyFromExisting(ctx, connector, existingConnectors) + tc.assertErr(t, err) + if tc.assertResult != nil { + tc.assertResult(t, connector) + } + }) + } +} + // roleSet is a basic set of roles keyed by role name. It implements the // RoleGetter interface, returning the role if it exists, or a trace.NotFound // error if it does not exist.