OIDC multiple redirect URLs (#13046)

api/client/client.go

@@ -1416,6 +1416,9 @@ func (c *Client) GetOIDCConnector(ctx context.Context, name string, withSecrets
 	if err != nil {
 		return nil, trail.FromGRPC(err)
 	}
+	// An old server would send RedirectURL instead of RedirectURLs
+	// DELETE IN 11.0.0
+	resp.CheckSetRedirectURL()
 	return resp, nil
 }
 
@@ -1428,6 +1431,9 @@ func (c *Client) GetOIDCConnectors(ctx context.Context, withSecrets bool) ([]typ
 	}
 	oidcConnectors := make([]types.OIDCConnector, len(resp.OIDCConnectors))
 	for i, oidcConnector := range resp.OIDCConnectors {
+		// An old server would send RedirectURL instead of RedirectURLs
+		// DELETE IN 11.0.0
+		oidcConnector.CheckSetRedirectURL()
 		oidcConnectors[i] = oidcConnector
 	}
 	return oidcConnectors, nil
@@ -1439,6 +1445,9 @@ func (c *Client) UpsertOIDCConnector(ctx context.Context, oidcConnector types.OI
 	if !ok {
 		return trace.BadParameter("invalid type %T", oidcConnector)
 	}
+	// An old server would expect RedirectURL instead of RedirectURLs
+	// DELETE IN 11.0.0
+	connector.CheckSetRedirectURL()
 	_, err := c.grpc.UpsertOIDCConnector(ctx, connector, c.callOpts...)
 	return trail.FromGRPC(err)
 }

api/client/client_test.go

@@ -25,6 +25,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/golang/protobuf/ptypes/empty"
 	"github.com/google/go-cmp/cmp"
 	"github.com/gravitational/teleport/api/client/proto"
 	"github.com/gravitational/teleport/api/defaults"
@@ -46,8 +47,8 @@ type mockServer struct {
 
 func newMockServer() *mockServer {
 	m := &mockServer{
-		grpc.NewServer(),
-		&proto.UnimplementedAuthServiceServer{},
+		grpc:                           grpc.NewServer(),
+		UnimplementedAuthServiceServer: &proto.UnimplementedAuthServiceServer{},
 	}
 	proto.RegisterAuthServiceServer(m.grpc, m)
 	return m
@@ -614,3 +615,101 @@ func TestGetResources(t *testing.T) {
 		})
 	}
 }
+
+type mockOIDCConnectorServer struct {
+	*mockServer
+	connectors map[string]*types.OIDCConnectorV3
+}
+
+func newMockOIDCConnectorServer() *mockOIDCConnectorServer {
+	m := &mockOIDCConnectorServer{
+		&mockServer{
+			grpc:                           grpc.NewServer(),
+			UnimplementedAuthServiceServer: &proto.UnimplementedAuthServiceServer{},
+		},
+		make(map[string]*types.OIDCConnectorV3),
+	}
+	proto.RegisterAuthServiceServer(m.grpc, m)
+	return m
+}
+
+func startMockOIDCConnectorServer(t *testing.T) string {
+	l, err := net.Listen("tcp", "")
+	require.NoError(t, err)
+	t.Cleanup(func() { require.NoError(t, l.Close()) })
+	go newMockOIDCConnectorServer().grpc.Serve(l)
+	return l.Addr().String()
+}
+
+func (m *mockOIDCConnectorServer) GetOIDCConnector(ctx context.Context, req *types.ResourceWithSecretsRequest) (*types.OIDCConnectorV3, error) {
+	conn, ok := m.connectors[req.Name]
+	if !ok {
+		return nil, trace.NotFound("not found")
+	}
+	return conn, nil
+}
+
+func (m *mockOIDCConnectorServer) GetOIDCConnectors(ctx context.Context, req *types.ResourcesWithSecretsRequest) (*types.OIDCConnectorV3List, error) {
+	var connectors []*types.OIDCConnectorV3
+	for _, conn := range m.connectors {
+		connectors = append(connectors, conn)
+	}
+	return &types.OIDCConnectorV3List{
+		OIDCConnectors: connectors,
+	}, nil
+}
+
+func (m *mockOIDCConnectorServer) UpsertOIDCConnector(ctx context.Context, oidcConnector *types.OIDCConnectorV3) (*empty.Empty, error) {
+	m.connectors[oidcConnector.Metadata.Name] = oidcConnector
+	return &empty.Empty{}, nil
+}
+
+// Test that client will perform properly with an old server
+// DELETE IN 11.0.0
+func TestSetOIDCRedirectURLBackwardsCompatibility(t *testing.T) {
+	ctx := context.Background()
+	addr := startMockOIDCConnectorServer(t)
+
+	// Create client
+	clt, err := New(ctx, Config{
+		Addrs: []string{addr},
+		Credentials: []Credentials{
+			&mockInsecureTLSCredentials{}, // TODO(Joerger) replace insecure credentials
+		},
+		DialOpts: []grpc.DialOption{
+			grpc.WithTransportCredentials(insecure.NewCredentials()), // TODO(Joerger) remove insecure dial option
+		},
+	})
+	require.NoError(t, err)
+
+	conn := &types.OIDCConnectorV3{
+		Metadata: types.Metadata{
+			Name: "one",
+		},
+	}
+
+	// Upsert should set "RedirectURL" on the provided connector if empty
+	conn.Spec.RedirectURLs = []string{"one.example.com"}
+	conn.Spec.RedirectURL = ""
+	err = clt.UpsertOIDCConnector(ctx, conn)
+	require.NoError(t, err)
+	require.Equal(t, 1, len(conn.GetRedirectURLs()))
+	require.Equal(t, conn.GetRedirectURLs()[0], conn.Spec.RedirectURL)
+
+	// GetOIDCConnector should set "RedirectURLs" on the received connector if empty
+	conn.Spec.RedirectURLs = []string{}
+	conn.Spec.RedirectURL = "one.example.com"
+	connResp, err := clt.GetOIDCConnector(ctx, conn.GetName(), false)
+	require.NoError(t, err)
+	require.Equal(t, 1, len(connResp.GetRedirectURLs()))
+	require.Equal(t, connResp.GetRedirectURLs()[0], "one.example.com")
+
+	// GetOIDCConnectors should set "RedirectURLs" on the received connectors if empty
+	conn.Spec.RedirectURLs = []string{}
+	conn.Spec.RedirectURL = "one.example.com"
+	connectorsResp, err := clt.GetOIDCConnectors(ctx, false)
+	require.NoError(t, err)
+	require.Equal(t, 1, len(connectorsResp))
+	require.Equal(t, 1, len(connectorsResp[0].GetRedirectURLs()))
+	require.Equal(t, "one.example.com", connectorsResp[0].GetRedirectURLs()[0])
+}

api/types/oidc.go

@@ -17,6 +17,7 @@ limitations under the License.
 package types
 
 import (
+	"net/url"
 	"time"
 
 	"github.com/gravitational/teleport/api/constants"
@@ -37,10 +38,8 @@ type OIDCConnector interface {
 	// ClientSecret is used to authenticate our client and should not
 	// be visible to end user
 	GetClientSecret() string
-	// RedirectURL - Identity provider will use this URL to redirect
-	// client's browser back to it after successful authentication
-	// Should match the URL on Provider's side
-	GetRedirectURL() string
+	// GetRedirectURLs returns list of redirect URLs.
+	GetRedirectURLs() []string
 	// GetACR returns the Authentication Context Class Reference (ACR) value.
 	GetACR() string
 	// GetProvider returns the identity provider.
@@ -62,8 +61,8 @@ type OIDCConnector interface {
 	SetClientID(string)
 	// SetIssuerURL sets the endpoint of the provider
 	SetIssuerURL(string)
-	// SetRedirectURL sets RedirectURL
-	SetRedirectURL(string)
+	// SetRedirectURLs sets the list of redirectURLs
+	SetRedirectURLs([]string)
 	// SetPrompt sets OIDC prompt value
 	SetPrompt(string)
 	// GetPrompt returns OIDC prompt value,
@@ -226,9 +225,9 @@ func (o *OIDCConnectorV3) SetIssuerURL(issuerURL string) {
 	o.Spec.IssuerURL = issuerURL
 }
 
-// SetRedirectURL sets client secret to some value
-func (o *OIDCConnectorV3) SetRedirectURL(redirectURL string) {
-	o.Spec.RedirectURL = redirectURL
+// SetRedirectURLs sets the list of redirectURLs
+func (o *OIDCConnectorV3) SetRedirectURLs(redirectURLs []string) {
+	o.Spec.RedirectURLs = redirectURLs
 }
 
 // SetACR sets the Authentication Context Class Reference (ACR) value.
@@ -277,11 +276,9 @@ func (o *OIDCConnectorV3) GetClientSecret() string {
 	return o.Spec.ClientSecret
 }
 
-// GetRedirectURL - Identity provider will use this URL to redirect
-// client's browser back to it after successful authentication
-// Should match the URL on Provider's side
-func (o *OIDCConnectorV3) GetRedirectURL() string {
-	return o.Spec.RedirectURL
+// GetRedirectURLs returns a list of the connector's redirect URLs.
+func (o *OIDCConnectorV3) GetRedirectURLs() []string {
+	return o.Spec.RedirectURLs
 }
 
 // GetACR returns the Authentication Context Class Reference (ACR) value.
@@ -359,16 +356,68 @@ func (o *OIDCConnectorV3) CheckAndSetDefaults() error {
 	if o.Metadata.Name == constants.Local {
 		return trace.BadParameter("ID: invalid connector name, %v is a reserved name", constants.Local)
 	}
+
 	if o.Spec.ClientID == "" {
 		return trace.BadParameter("ClientID: missing client id")
 	}
 
-	// make sure claim mappings have either roles or a role template
+	if len(o.GetClaimsToRoles()) == 0 {
+		return trace.BadParameter("claims_to_roles is empty, authorization with connector would never assign any roles")
+	}
 	for _, v := range o.Spec.ClaimsToRoles {
 		if len(v.Roles) == 0 {
 			return trace.BadParameter("add roles in claims_to_roles")
 		}
 	}
 
+	if _, err := url.Parse(o.GetIssuerURL()); err != nil {
+		return trace.BadParameter("bad IssuerURL '%v', err: %v", o.GetIssuerURL(), err)
+	}
+
+	// DELETE IN 11.0.0
+	o.CheckSetRedirectURL()
+
+	if len(o.GetRedirectURLs()) == 0 {
+		return trace.BadParameter("RedirectURL: missing redirect_url")
+	}
+	for _, redirectURL := range o.GetRedirectURLs() {
+		if _, err := url.Parse(redirectURL); err != nil {
+			return trace.BadParameter("bad RedirectURL '%v', err: %v", redirectURL, err)
+		}
+	}
+
+	if o.GetGoogleServiceAccountURI() != "" && o.GetGoogleServiceAccount() != "" {
+		return trace.BadParameter("one of either google_service_account_uri or google_service_account is supported, not both")
+	}
+
+	if o.GetGoogleServiceAccountURI() != "" {
+		uri, err := utils.ParseSessionsURI(o.GetGoogleServiceAccountURI())
+		if err != nil {
+			return trace.Wrap(err)
+		}
+		if uri.Scheme != "file" {
+			return trace.BadParameter("only file:// scheme is supported for google_service_account_uri")
+		}
+		if o.GetGoogleAdminEmail() == "" {
+			return trace.BadParameter("whenever google_service_account_uri is specified, google_admin_email should be set as well, read https://developers.google.com/identity/protools/OAuth2ServiceAccount#delegatingauthority for more details")
+		}
+	}
+
+	if o.GetGoogleServiceAccount() != "" {
+		if o.GetGoogleAdminEmail() == "" {
+			return trace.BadParameter("whenever google_service_account is specified, google_admin_email should be set as well, read https://developers.google.com/identity/protocols/OAuth2ServiceAccount#delegatingauthority for more details")
+		}
+	}
+
 	return nil
 }
+
+// RedirectURL must be checked/set when communicating with an old server or client.
+// DELETE IN 11.0.0
+func (o *OIDCConnectorV3) CheckSetRedirectURL() {
+	if o.Spec.RedirectURL == "" && len(o.Spec.RedirectURLs) != 0 {
+		o.Spec.RedirectURL = o.Spec.RedirectURLs[0]
+	} else if len(o.Spec.RedirectURLs) == 0 && o.Spec.RedirectURL != "" {
+		o.Spec.RedirectURLs = []string{o.Spec.RedirectURL}
+	}
+}