Move SAML connection validation after auth checks (#19318)

Move the validation for SAML connectors being inserted or updated (Upsert)
from before the role check for Insert/Update to after it. The validation
can perform an HTTP GET request if the `entity_descriptor_url` field is
set. This should at least require that the user have permission to
Upsert a SAML OIDC connector.

Ensure that roles exist that are referenced in SAML connectors, as the
roles in SAML connectors are validated as the connector is inserted or
updated. The validation has moved to auth.Server (from grpcserver) so
that is now required in tests that operate against auth.Server.

Add a test case to test that an access denied error is returned when
upserting an invalid SAML connector instead of a validation failure,
showing that we are not using input from untrusted sources. Also test
that validation is still performed when access is permitted and that a
valid SAML connector can be upserted.

Backport: #17531

lib/auth/auth_test.go

@@ -1090,6 +1090,12 @@ func TestSAMLConnectorCRUDEventsEmitted(t *testing.T) {
 	})
 	require.NoError(t, err)
 
+	// SAML connector validation requires the roles in mappings exist.
+	role, err := types.NewRole("dummy", types.RoleSpecV5{})
+	require.NoError(t, err)
+	err = s.a.CreateRole(ctx, role)
+	require.NoError(t, err)
+
 	// test saml create
 	saml, err := types.NewSAMLConnector("test", types.SAMLConnectorSpecV2{
 		AssertionConsumerService: "a",
@@ -1099,7 +1105,7 @@ func TestSAMLConnectorCRUDEventsEmitted(t *testing.T) {
 			{
 				Name:  "dummy",
 				Value: "dummy",
-				Roles: []string{"dummy"},
+				Roles: []string{role.GetName()},
 			},
 		},
 		Cert: string(certBytes),

lib/auth/grpcserver.go

@@ -2665,9 +2665,6 @@ func (g *GRPCServer) UpsertSAMLConnector(ctx context.Context, samlConnector *typ
 	if err != nil {
 		return nil, trace.Wrap(err)
 	}
-	if err = services.ValidateSAMLConnector(samlConnector, auth); err != nil {
-		return nil, trace.Wrap(err)
-	}
 	if err = auth.ServerWithRoles.UpsertSAMLConnector(ctx, samlConnector); err != nil {
 		return nil, trace.Wrap(err)
 	}

lib/auth/grpcserver_test.go

@@ -22,6 +22,8 @@ import (
 	"encoding/base32"
 	"fmt"
 	"net"
+	"net/http"
+	"net/http/httptest"
 	"sort"
 	"testing"
 	"time"
@@ -54,6 +56,7 @@ import (
 	"github.com/gravitational/teleport/lib/auth/testauthority"
 	wanlib "github.com/gravitational/teleport/lib/auth/webauthn"
 	"github.com/gravitational/teleport/lib/defaults"
+	"github.com/gravitational/teleport/lib/modules"
 	"github.com/gravitational/teleport/lib/services"
 	"github.com/gravitational/teleport/lib/tlsca"
 )
@@ -2529,3 +2532,103 @@ func TestExport(t *testing.T) {
 		})
 	}
 }
+
+// TestSAMLValidation tests that SAML validation does not perform an HTTP
+// request if the calling user does not have permissions to create or update
+// a SAML connector.
+func TestSAMLValidation(t *testing.T) {
+	modules.SetTestModules(t, &modules.TestModules{
+		TestFeatures: modules.Features{SAML: true},
+	})
+
+	// minimal entity_descriptor to pass validation. not actually valid
+	const minimalEntityDescriptor = `
+<md:EntityDescriptor xmlns:md="urn:oasis:names:tc:SAML:2.0:metadata" entityID="http://example.com">
+  <md:IDPSSODescriptor>
+    <md:SingleSignOnService Location="http://example.com" />
+  </md:IDPSSODescriptor>
+</md:EntityDescriptor>`
+
+	allowSAMLUpsert := types.RoleConditions{
+		Rules: []types.Rule{{
+			Resources: []string{types.KindSAML},
+			Verbs:     []string{types.VerbCreate, types.VerbUpdate},
+		}},
+	}
+
+	testCases := []struct {
+		desc               string
+		allow              types.RoleConditions
+		entityDescriptor   string
+		entityServerCalled bool
+		assertErr          func(error) bool
+	}{
+		{
+			desc:               "access denied",
+			allow:              types.RoleConditions{},
+			entityServerCalled: false,
+			assertErr:          trace.IsAccessDenied,
+		},
+		{
+			desc:               "validation failure",
+			allow:              allowSAMLUpsert,
+			entityDescriptor:   "", // validation fails with no issuer
+			entityServerCalled: true,
+			assertErr:          trace.IsBadParameter,
+		},
+		{
+			desc:               "access permitted",
+			allow:              allowSAMLUpsert,
+			entityDescriptor:   minimalEntityDescriptor,
+			entityServerCalled: true,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+			defer cancel()
+
+			server := newTestTLSServer(t)
+			// Create an http server to serve the entity descriptor url
+			entityServerCalled := false
+			entityServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				entityServerCalled = true
+				_, err := w.Write([]byte(tc.entityDescriptor))
+				require.NoError(t, err)
+			}))
+
+			role, err := CreateRole(ctx, server.Auth(), "test_role", types.RoleSpecV5{Allow: tc.allow})
+			require.NoError(t, err)
+			user, err := CreateUser(server.Auth(), "test_user", role)
+			require.NoError(t, err)
+
+			connector, err := types.NewSAMLConnector("test_connector", types.SAMLConnectorSpecV2{
+				AssertionConsumerService: "http://localhost:65535/acs", // not called
+				EntityDescriptorURL:      entityServer.URL,
+				AttributesToRoles: []types.AttributeMapping{
+					// not used. can be any name, value but role must exist
+					{Name: "groups", Value: "admin", Roles: []string{role.GetName()}},
+				},
+			})
+			require.NoError(t, err)
+
+			client, err := server.NewClient(TestUser(user.GetName()))
+			require.NoError(t, err)
+
+			err = client.UpsertSAMLConnector(ctx, connector)
+
+			if tc.assertErr != nil {
+				require.Error(t, err)
+				require.True(t, tc.assertErr(err), "UpsertSAMLConnector error type mismatch. got: %T", trace.Unwrap(err))
+			} else {
+				require.NoError(t, err)
+			}
+			if tc.entityServerCalled {
+				require.True(t, entityServerCalled, "entity_descriptor_url was not called")
+			} else {
+				require.False(t, entityServerCalled, "entity_descriptor_url was called")
+			}
+		})
+	}
+}

lib/auth/saml.go

@@ -45,6 +45,9 @@ var ErrSAMLNoRoles = trace.AccessDenied("No roles mapped from claims. The mappin
 
 // UpsertSAMLConnector creates or updates a SAML connector.
 func (a *Server) UpsertSAMLConnector(ctx context.Context, connector types.SAMLConnector) error {
+	if err := services.ValidateSAMLConnector(connector, a); err != nil {
+		return trace.Wrap(err)
+	}
 	if err := a.Services.UpsertSAMLConnector(ctx, connector); err != nil {
 		return trace.Wrap(err)
 	}

lib/auth/saml_test.go

@@ -220,12 +220,18 @@ func TestPingSAMLWorkaround(t *testing.T) {
 		PrivateKey: fixtures.EncryptionKeyPEM,
 	}
 
+	// SAML connector validation requires the roles in mappings exist.
+	role, err := types.NewRole("admin", types.RoleSpecV5{})
+	require.NoError(t, err)
+	err = a.CreateRole(ctx, role)
+	require.NoError(t, err)
+
 	connector, err := types.NewSAMLConnector("ping", types.SAMLConnectorSpecV2{
 		AssertionConsumerService: "https://proxy.example.com:3080/v1/webapi/saml/acs",
 		Provider:                 "ping",
 		Display:                  "Ping",
 		AttributesToRoles: []types.AttributeMapping{
-			{Name: "groups", Value: "ping-admin", Roles: []string{"admin"}},
+			{Name: "groups", Value: "ping-admin", Roles: []string{role.GetName()}},
 		},
 		EntityDescriptor:  entityDescriptor,
 		SigningKeyPair:    signingKeypair,
@@ -471,6 +477,12 @@ V115UGOwvjOOxmOFbYBn865SHgMndFtr</ds:X509Certificate></ds:X509Data></ds:KeyInfo>
 	}, nil, 10*365*24*time.Hour)
 	require.NoError(t, err)
 
+	// SAML connector validation requires the roles in mappings exist.
+	connectorRole, err := types.NewRole("baz", types.RoleSpecV5{})
+	require.NoError(t, err)
+	err = a.CreateRole(ctx, connectorRole)
+	require.NoError(t, err)
+
 	conn, err := types.NewSAMLConnector("saml-test-conn", types.SAMLConnectorSpecV2{
 		Issuer:                   "test",
 		SSO:                      "test",
@@ -479,7 +491,7 @@ V115UGOwvjOOxmOFbYBn865SHgMndFtr</ds:X509Certificate></ds:X509Data></ds:KeyInfo>
 		AttributesToRoles: []types.AttributeMapping{{
 			Name:  "foo",
 			Value: "bar",
-			Roles: []string{"baz"},
+			Roles: []string{connectorRole.GetName()},
 		}},
 		SigningKeyPair: &types.AsymmetricKeyPair{
 			PrivateKey: string(keyPEM),

lib/services/saml_test.go

@@ -43,8 +43,6 @@ func TestParseFromMetadata(t *testing.T) {
 
 	oc, err := UnmarshalSAMLConnector(raw.Raw)
 	require.NoError(t, err)
-	err = ValidateSAMLConnector(oc, nil)
-	require.NoError(t, err)
 	require.Equal(t, oc.GetIssuer(), "http://www.okta.com/exkafftca6RqPVgyZ0h7")
 	require.Equal(t, oc.GetSSO(), "https://dev-813354.oktapreview.com/app/gravitationaldev813354_teleportsaml_1/exkafftca6RqPVgyZ0h7/sso/saml")
 	require.Equal(t, oc.GetAssertionConsumerService(), "https://localhost:3080/v1/webapi/saml/acs")