Various fixes to SAML encryption key handling for SSO (gravitational#…

…6767)

api/types/saml.go

@@ -1,5 +1,5 @@
 /*
-Copyright 2020 Gravitational, Inc.
+Copyright 2020-2021 Gravitational, Inc.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -134,14 +134,19 @@ func (o *SAMLConnectorV2) SetResourceID(id int64) {
 
 // WithoutSecrets returns an instance of resource without secrets.
 func (o *SAMLConnectorV2) WithoutSecrets() Resource {
-	k := o.GetSigningKeyPair()
-	if k == nil {
-		return o
-	}
-	k2 := *k
-	k2.PrivateKey = ""
+	k1 := o.GetSigningKeyPair()
+	k2 := o.GetEncryptionKeyPair()
 	o2 := *o
-	o2.SetSigningKeyPair(&k2)
+	if k1 != nil {
+		q1 := *k1
+		q1.PrivateKey = ""
+		o2.SetSigningKeyPair(&q1)
+	}
+	if k2 != nil {
+		q2 := *k2
+		q2.PrivateKey = ""
+		o2.SetEncryptionKeyPair(&q2)
+	}
 	return &o2
 }
 

api/types/saml_test.go

@@ -0,0 +1,41 @@
+/*
+Copyright 2021 Gravitational, Inc.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package types
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+// TestSAMLSecretsStrip tests the WithoutSecrets method on SAMLConnectorV2.
+func TestSAMLSecretsStrip(t *testing.T) {
+	connector, err := NewSAMLConnector("test", SAMLConnectorSpecV2{
+		AssertionConsumerService: "test",
+		SSO:                      "test",
+		EntityDescriptor:         "test",
+		SigningKeyPair:           &AsymmetricKeyPair{PrivateKey: "test"},
+		EncryptionKeyPair:        &AsymmetricKeyPair{PrivateKey: "test"},
+	})
+	require.Nil(t, err)
+	require.Equal(t, connector.GetSigningKeyPair().PrivateKey, "test")
+	require.Equal(t, connector.GetEncryptionKeyPair().PrivateKey, "test")
+
+	withoutSecrets := connector.WithoutSecrets().(*SAMLConnectorV2)
+	require.Equal(t, withoutSecrets.GetSigningKeyPair().PrivateKey, "")
+	require.Equal(t, withoutSecrets.GetEncryptionKeyPair().PrivateKey, "")
+}

lib/services/saml.go

@@ -162,20 +162,37 @@ func GetSAMLServiceProvider(sc types.SAMLConnector, clock clockwork.Clock) (*sam
 		return nil, trace.BadParameter("no identity provider certificate provided, either set certificate as a parameter or via entity_descriptor")
 	}
 
-	signingKeyStore, err := utils.ParseSigningKeyStorePEM(sc.GetSigningKeyPair().PrivateKey, sc.GetSigningKeyPair().Cert)
-	if err != nil {
-		return nil, trace.Wrap(err, "failed to parse certificate defined in signing_key_pair")
-	}
-
-	// The encryption keystore here is defaulted to the value of the signing keystore
-	// if no separate assertion decryption keys are provided. We do this here to initialize
-	// the variable but if set to nil, gosaml2 will do this internally anyway.
-	encryptionKeyStore := signingKeyStore
+	signingKeyPair := sc.GetSigningKeyPair()
 	encryptionKeyPair := sc.GetEncryptionKeyPair()
-	if encryptionKeyPair != nil {
-		encryptionKeyStore, err = utils.ParseSigningKeyStorePEM(encryptionKeyPair.PrivateKey, encryptionKeyPair.Cert)
+	var keyStore *utils.KeyStore
+	var signingKeyStore *utils.KeyStore
+	var err error
+
+	// Due to some weird design choices with how gosaml2 keys are configured we have to do some trickery
+	// in order to default properly when SAML assertion encryption is turned off.
+	// Below are the different possible cases.
+	if encryptionKeyPair == nil {
+		// Case 1: Only the signing key pair is set. This means that SAML encryption is not expected
+		// and we therefore configure the main key that gets used for all operations as the signing key.
+		// This is done because gosaml2 mandates an encryption key even if not used.
+		log.Info("No assertion_key_pair was detected. Falling back to signing key for all SAML operations.")
+		keyStore, err = utils.ParseKeyStorePEM(signingKeyPair.PrivateKey, signingKeyPair.Cert)
+		signingKeyStore = keyStore
+		if err != nil {
+			return nil, trace.Wrap(err, "failed to parse certificate or private key defined in signing_key_pair")
+		}
+	} else {
+		// Case 2: An encryption keypair is configured. This means that encrypted SAML responses are expected.
+		// Since gosaml2 always uses the main key for encryption, we set it to assertion_key_pair.
+		// To handle signing correctly, we now instead set the optional signing key in gosaml2 to signing_key_pair.
+		log.Info("Detected assertion_key_pair and configured it to decrypt SAML responses.")
+		keyStore, err = utils.ParseKeyStorePEM(encryptionKeyPair.PrivateKey, encryptionKeyPair.Cert)
+		if err != nil {
+			return nil, trace.Wrap(err, "failed to parse certificate or private key defined in assertion_key_pair")
+		}
+		signingKeyStore, err = utils.ParseKeyStorePEM(signingKeyPair.PrivateKey, signingKeyPair.Cert)
 		if err != nil {
-			return nil, trace.Wrap(err, "failed to parse certificate defined in assertion_key_pair")
+			return nil, trace.Wrap(err, "failed to parse certificate or private key defined in signing_key_pair")
 		}
 	}
 
@@ -188,8 +205,8 @@ func GetSAMLServiceProvider(sc types.SAMLConnector, clock clockwork.Clock) (*sam
 		SignAuthnRequestsCanonicalizer: dsig.MakeC14N11Canonicalizer(),
 		AudienceURI:                    sc.GetAudience(),
 		IDPCertificateStore:            &certStore,
-		SPKeyStore:                     encryptionKeyStore,
 		SPSigningKeyStore:              signingKeyStore,
+		SPKeyStore:                     keyStore,
 		Clock:                          dsig.NewFakeClock(clock),
 		NameIdFormat:                   "urn:oasis:names:tc:SAML:1.1:nameid-format:unspecified",
 	}

lib/utils/certs.go

@@ -34,8 +34,8 @@ import (
 	"github.com/sirupsen/logrus"
 )
 
-// ParseSigningKeyStore parses signing key store from PEM encoded key pair
-func ParseSigningKeyStorePEM(keyPEM, certPEM string) (*SigningKeyStore, error) {
+// ParseKeyStorePEM parses signing key store from PEM encoded key pair
+func ParseKeyStorePEM(keyPEM, certPEM string) (*KeyStore, error) {
 	_, err := tlsutils.ParseCertificatePEM([]byte(certPEM))
 	if err != nil {
 		return nil, trace.Wrap(err)
@@ -46,22 +46,22 @@ func ParseSigningKeyStorePEM(keyPEM, certPEM string) (*SigningKeyStore, error) {
 	}
 	rsaKey, ok := key.(*rsa.PrivateKey)
 	if !ok {
-		return nil, trace.BadParameter("key of type %T is not supported, only RSA keys are supported for signatures", key)
+		return nil, trace.BadParameter("key of type %T is not supported, only RSA keys are supported", key)
 	}
 	certASN, _ := pem.Decode([]byte(certPEM))
 	if certASN == nil {
 		return nil, trace.BadParameter("expected PEM-encoded block")
 	}
-	return &SigningKeyStore{privateKey: rsaKey, cert: certASN.Bytes}, nil
+	return &KeyStore{privateKey: rsaKey, cert: certASN.Bytes}, nil
 }
 
-// SigningKeyStore is used to sign using X509 digital signatures
-type SigningKeyStore struct {
+// KeyStore is used to sign and decrypt data using X509 digital signatures.
+type KeyStore struct {
 	privateKey *rsa.PrivateKey
 	cert       []byte
 }
 
-func (ks *SigningKeyStore) GetKeyPair() (*rsa.PrivateKey, []byte, error) {
+func (ks *KeyStore) GetKeyPair() (*rsa.PrivateKey, []byte, error) {
 	return ks.privateKey, ks.cert, nil
 }