OIDC: validate the client_secret field

- Make the client_secret required, as OIDC auth won't work without it - Raise a clear error if unsupported file:// URLs are used Closes #39107
gravitational · Oct 16, 2024 · 712051d · 712051d
1 parent d602f89
commit 712051d
Show file tree

Hide file tree

Showing 5 changed files with 36 additions and 2 deletions.
diff --git a/api/types/oidc.go b/api/types/oidc.go
@@ -20,6 +20,7 @@ import (
 	"net/netip"
 	"net/url"
 	"slices"
+	"strings"
 	"time"
 
 	"github.com/gravitational/trace"
@@ -403,7 +404,14 @@ func (o *OIDCConnectorV3) CheckAndSetDefaults() error {
 	}
 
 	if o.Spec.ClientID == "" {
-		return trace.BadParameter("ClientID: missing client id")
+		return trace.BadParameter("OIDC connector is missing required client_id")
+	}
+
+	if o.Spec.ClientSecret == "" {
+		return trace.BadParameter("OIDC connector is missing required client_secret")
+	}
+	if strings.HasPrefix(o.Spec.ClientSecret, "file://") {
+		return trace.BadParameter("the client_secret must be a literal value, file:// URLs are not supported")
 	}
 
 	if len(o.GetClaimsToRoles()) == 0 {

diff --git a/api/types/oidc_external_test.go b/api/types/oidc_external_test.go
@@ -17,6 +17,7 @@ package types
 import (
 	"testing"
 
+	"github.com/gravitational/trace"
 	"github.com/stretchr/testify/require"
 )
 
@@ -55,3 +56,20 @@ func TestOIDCClaimsRoundTrip(t *testing.T) {
 		})
 	}
 }
+
+func TestClientSecretFileURI(t *testing.T) {
+	_, err := NewOIDCConnector("test-connector", OIDCConnectorSpecV3{
+		ClientID:     "some-client-id",
+		ClientSecret: "file://is-not-allowed",
+		ClaimsToRoles: []ClaimMapping{
+			{
+				Claim: "team",
+				Value: "dev",
+				Roles: []string{"dev-team-access"},
+			},
+		},
+	})
+	require.Error(t, err)
+	require.True(t, trace.IsBadParameter(err))
+	require.ErrorContains(t, err, "file:// URLs are not supported")
+}
diff --git a/lib/auth/auth_test.go b/lib/auth/auth_test.go
@@ -1343,7 +1343,8 @@ func TestOIDCConnectorCRUDEventsEmitted(t *testing.T) {
 
 	ctx := context.Background()
 	oidc, err := types.NewOIDCConnector("test", types.OIDCConnectorSpecV3{
-		ClientID: "a",
+		ClientID:     "a",
+		ClientSecret: "b",
 		ClaimsToRoles: []types.ClaimMapping{
 			{
 				Claim: "dummy",

diff --git a/lib/services/oidc_test.go b/lib/services/oidc_test.go
@@ -109,6 +109,7 @@ func TestOIDCUnmarshal(t *testing.T) {
 				},
 				"spec": {
 					"client_id": "id-from-google.apps.googleusercontent.com",
+					"client_secret": "secret-key-from-google",
 					"claims_to_roles": [
 						{
 							"claim": "roles",
@@ -125,6 +126,7 @@ func TestOIDCUnmarshal(t *testing.T) {
 			}`,
 			expectSpec: types.OIDCConnectorSpecV3{
 				ClientID:      "id-from-google.apps.googleusercontent.com",
+				ClientSecret:  "secret-key-from-google",
 				ClaimsToRoles: []types.ClaimMapping{{Claim: "roles", Value: "teleport-user", Roles: []string{"dictator"}}},
 				RedirectURLs: []string{
 					"https://localhost:3080/v1/webapi/oidc/callback",
@@ -159,6 +161,7 @@ func TestOIDCCheckAndSetDefaults(t *testing.T) {
 			desc: "basic spec and defaults",
 			spec: types.OIDCConnectorSpecV3{
 				ClientID:      "id-from-google.apps.googleusercontent.com",
+				ClientSecret:  "some-client-secret",
 				ClaimsToRoles: []types.ClaimMapping{{Claim: "roles", Value: "teleport-user", Roles: []string{"dictator"}}},
 				RedirectURLs:  []string{"https://localhost:3080/v1/webapi/oidc/callback"},
 			},
@@ -175,6 +178,7 @@ func TestOIDCCheckAndSetDefaults(t *testing.T) {
 			desc: "omit prompt",
 			spec: types.OIDCConnectorSpecV3{
 				ClientID:      "id-from-google.apps.googleusercontent.com",
+				ClientSecret:  "some-client-secret",
 				ClaimsToRoles: []types.ClaimMapping{{Claim: "roles", Value: "teleport-user", Roles: []string{"dictator"}}},
 				RedirectURLs: []string{
 					"https://localhost:3080/v1/webapi/oidc/callback",
@@ -191,6 +195,7 @@ func TestOIDCCheckAndSetDefaults(t *testing.T) {
 			desc: "invalid claims to roles",
 			spec: types.OIDCConnectorSpecV3{
 				ClientID:      "id-from-google.apps.googleusercontent.com",
+				ClientSecret:  "some-client-secret",
 				ClaimsToRoles: []types.ClaimMapping{{Claim: "roles", Value: "teleport-user"}},
 				RedirectURLs: []string{
 					"https://localhost:3080/v1/webapi/oidc/callback",
@@ -215,6 +220,7 @@ func TestOIDCCheckAndSetDefaults(t *testing.T) {
 func TestOIDCGetRedirectURL(t *testing.T) {
 	conn, err := types.NewOIDCConnector("oidc", types.OIDCConnectorSpecV3{
 		ClientID:      "id-from-google.apps.googleusercontent.com",
+		ClientSecret:  "some-client-secret",
 		ClaimsToRoles: []types.ClaimMapping{{Claim: "roles", Value: "teleport-user", Roles: []string{"dictator"}}},
 		RedirectURLs: []string{
 			"https://proxy.example.com/v1/webapi/oidc/callback",

diff --git a/tool/tsh/common/tsh_test.go b/tool/tsh/common/tsh_test.go
@@ -3879,6 +3879,7 @@ func mockConnector(t *testing.T) types.OIDCConnector {
 		IssuerURL:    "https://auth.example.com",
 		RedirectURLs: []string{"https://cluster.example.com"},
 		ClientID:     "fake-client",
+		ClientSecret: "fake-secret",
 		ClaimsToRoles: []types.ClaimMapping{
 			{
 				Claim: "groups",