From c49ebf4b57ff0925e0ca33c33967e5b52ce738f6 Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Wed, 7 Oct 2020 11:33:50 -0400 Subject: [PATCH] supervisor-oidc: int test passes, but impl needs refactor Signed-off-by: Andrew Keesler --- cmd/pinniped-supervisor/main.go | 5 +- internal/oidc/discovery/discovery.go | 37 +++++++++--- internal/oidc/discovery/discovery_test.go | 57 ++++++++++++------- test/integration/supervisor_discovery_test.go | 6 +- 4 files changed, 68 insertions(+), 37 deletions(-) diff --git a/cmd/pinniped-supervisor/main.go b/cmd/pinniped-supervisor/main.go index 6e0866338..96758ba25 100644 --- a/cmd/pinniped-supervisor/main.go +++ b/cmd/pinniped-supervisor/main.go @@ -23,7 +23,6 @@ import ( "go.pinniped.dev/internal/controller/supervisorconfig" "go.pinniped.dev/internal/controllerlib" "go.pinniped.dev/internal/downward" - "go.pinniped.dev/internal/oidc" "go.pinniped.dev/internal/oidc/discovery" "go.pinniped.dev/internal/oidc/issuerprovider" ) @@ -34,10 +33,8 @@ const ( ) func start(ctx context.Context, l net.Listener, discoveryHandler http.Handler) { - mux := http.NewServeMux() - mux.Handle(oidc.WellKnownURLPath, discoveryHandler) server := http.Server{ - Handler: mux, + Handler: discoveryHandler, } errCh := make(chan error) diff --git a/internal/oidc/discovery/discovery.go b/internal/oidc/discovery/discovery.go index 96f858f51..2293e9637 100644 --- a/internal/oidc/discovery/discovery.go +++ b/internal/oidc/discovery/discovery.go @@ -9,21 +9,36 @@ import ( "fmt" "net/http" "net/url" + + "go.pinniped.dev/internal/oidc" ) // Metadata holds all fields (that we care about) from the OpenID Provider Metadata section in the // OpenID Connect Discovery specification: // https://openid.net/specs/openid-connect-discovery-1_0.html#rfc.section.3. type Metadata struct { + // vvvRequiredvvv + Issuer string `json:"issuer"` AuthorizationEndpoint string `json:"authorization_endpoint"` TokenEndpoint string `json:"token_endpoint"` - JWKSURL string `json:"jwks_url"` + JWKSURI string `json:"jwks_uri"` ResponseTypesSupported []string `json:"response_types_supported"` SubjectTypesSupported []string `json:"subject_types_supported"` IDTokenSigningAlgValuesSupported []string `json:"id_token_signing_alg_values_supported"` + + // ^^^Required^^^ + + // vvvOptionalvvv + + TokenEndpointAuthMethodsSupported []string `json:"token_endpoint_auth_methods_supported"` + TokenEndpointAuthSigningAlgoValuesSupported []string `json:"token_endpoint_auth_signing_alg_values_supported"` + ScopesSupported []string `json:"scopes_supported"` + ClaimsSupported []string `json:"claims_supported"` + + // ^^^Optional^^^ } // IssuerGetter holds onto an issuer which can be retrieved via its GetIssuer function. If there is @@ -41,7 +56,7 @@ func New(ig IssuerGetter) http.Handler { w.Header().Set("Content-Type", "application/json") issuer := ig.GetIssuer() - if issuer == nil { + if issuer == nil || r.URL.Path != issuer.Path+oidc.WellKnownURLPath { http.Error(w, `{"error": "OIDC discovery not available (unknown issuer)"}`, http.StatusNotFound) return } @@ -53,13 +68,17 @@ func New(ig IssuerGetter) http.Handler { issuerURL := issuer.String() oidcConfig := Metadata{ - Issuer: issuerURL, - AuthorizationEndpoint: fmt.Sprintf("%s/oauth2/v0/auth", issuerURL), - TokenEndpoint: fmt.Sprintf("%s/oauth2/v0/token", issuerURL), - JWKSURL: fmt.Sprintf("%s/oauth2/v0/keys", issuerURL), - ResponseTypesSupported: []string{}, - SubjectTypesSupported: []string{}, - IDTokenSigningAlgValuesSupported: []string{}, + Issuer: issuerURL, + AuthorizationEndpoint: fmt.Sprintf("%s/oauth2/v0/auth", issuerURL), + TokenEndpoint: fmt.Sprintf("%s/oauth2/v0/token", issuerURL), + JWKSURI: fmt.Sprintf("%s/jwks.json", issuerURL), + ResponseTypesSupported: []string{"code"}, + SubjectTypesSupported: []string{"public"}, + IDTokenSigningAlgValuesSupported: []string{"RS256"}, + TokenEndpointAuthMethodsSupported: []string{"client_secret_basic"}, + TokenEndpointAuthSigningAlgoValuesSupported: []string{"RS256"}, + ScopesSupported: []string{"openid", "offline"}, + ClaimsSupported: []string{"groups"}, } if err := json.NewEncoder(w).Encode(&oidcConfig); err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) diff --git a/internal/oidc/discovery/discovery_test.go b/internal/oidc/discovery/discovery_test.go index 47fab0701..7d1c481d2 100644 --- a/internal/oidc/discovery/discovery_test.go +++ b/internal/oidc/discovery/discovery_test.go @@ -12,6 +12,7 @@ import ( "github.com/stretchr/testify/require" + "go.pinniped.dev/internal/oidc" "go.pinniped.dev/internal/oidc/issuerprovider" ) @@ -21,6 +22,7 @@ func TestDiscovery(t *testing.T) { issuer *url.URL method string + path string wantStatus int wantContentType string @@ -29,47 +31,58 @@ func TestDiscovery(t *testing.T) { { name: "nil issuer", method: http.MethodGet, + path: oidc.WellKnownURLPath, wantStatus: http.StatusNotFound, wantBody: map[string]string{ "error": "OIDC discovery not available (unknown issuer)", }, }, { - name: "issuer without path", - issuer: must(url.Parse("https://some-issuer.com")), - method: http.MethodGet, - wantStatus: http.StatusOK, - wantContentType: "application/json", - wantBody: &Metadata{ - Issuer: "https://some-issuer.com", - AuthorizationEndpoint: "https://some-issuer.com/oauth2/v0/auth", - TokenEndpoint: "https://some-issuer.com/oauth2/v0/token", - JWKSURL: "https://some-issuer.com/oauth2/v0/keys", - ResponseTypesSupported: []string{}, - SubjectTypesSupported: []string{}, - IDTokenSigningAlgValuesSupported: []string{}, + name: "root path mismatch", + issuer: must(url.Parse("https://some-issuer.com/some/path")), + method: http.MethodGet, + path: "/some/other/path" + oidc.WellKnownURLPath, + wantStatus: http.StatusNotFound, + wantBody: map[string]string{ + "error": "OIDC discovery not available (unknown issuer)", }, }, { - name: "issuer with path", + name: "well-known path mismatch", + issuer: must(url.Parse("https://some-issuer.com/some/path")), + method: http.MethodGet, + path: "/some/path/that/is/not/the/well-known/path", + wantStatus: http.StatusNotFound, + wantBody: map[string]string{ + "error": "OIDC discovery not available (unknown issuer)", + }, + }, + { + name: "issuer path matches", issuer: must(url.Parse("https://some-issuer.com/some/path")), method: http.MethodGet, + path: "/some/path" + oidc.WellKnownURLPath, wantStatus: http.StatusOK, wantContentType: "application/json", wantBody: &Metadata{ - Issuer: "https://some-issuer.com/some/path", - AuthorizationEndpoint: "https://some-issuer.com/some/path/oauth2/v0/auth", - TokenEndpoint: "https://some-issuer.com/some/path/oauth2/v0/token", - JWKSURL: "https://some-issuer.com/some/path/oauth2/v0/keys", - ResponseTypesSupported: []string{}, - SubjectTypesSupported: []string{}, - IDTokenSigningAlgValuesSupported: []string{}, + Issuer: "https://some-issuer.com/some/path", + AuthorizationEndpoint: "https://some-issuer.com/some/path/oauth2/v0/auth", + TokenEndpoint: "https://some-issuer.com/some/path/oauth2/v0/token", + JWKSURI: "https://some-issuer.com/some/path/jwks.json", + ResponseTypesSupported: []string{"code"}, + SubjectTypesSupported: []string{"public"}, + IDTokenSigningAlgValuesSupported: []string{"RS256"}, + TokenEndpointAuthMethodsSupported: []string{"client_secret_basic"}, + TokenEndpointAuthSigningAlgoValuesSupported: []string{"RS256"}, + ScopesSupported: []string{"openid", "offline"}, + ClaimsSupported: []string{"groups"}, }, }, { name: "bad method", issuer: must(url.Parse("https://some-issuer.com")), method: http.MethodPost, + path: oidc.WellKnownURLPath, wantStatus: http.StatusMethodNotAllowed, wantBody: map[string]string{ "error": "Method not allowed (try GET)", @@ -83,7 +96,7 @@ func TestDiscovery(t *testing.T) { p.SetIssuer(test.issuer) handler := New(p) - req := httptest.NewRequest(test.method, "/this/path/shouldnt/matter", nil) + req := httptest.NewRequest(test.method, test.path, nil) rsp := httptest.NewRecorder() handler.ServeHTTP(rsp, req) diff --git a/test/integration/supervisor_discovery_test.go b/test/integration/supervisor_discovery_test.go index d0456ce07..d2e0bca29 100644 --- a/test/integration/supervisor_discovery_test.go +++ b/test/integration/supervisor_discovery_test.go @@ -118,14 +118,16 @@ func TestSupervisorOIDCDiscovery(t *testing.T) { // Check that the response matches our expectations. expectedResultTemplate := here.Doc(`{ "issuer": "%s", - "authorization_endpoint": "%s/connect/authorize", - "token_endpoint": "%s/connect/token", + "authorization_endpoint": "%s/oauth2/v0/auth", + "token_endpoint": "%s/oauth2/v0/token", "token_endpoint_auth_methods_supported": ["client_secret_basic"], "token_endpoint_auth_signing_alg_values_supported": ["RS256"], "jwks_uri": "%s/jwks.json", "scopes_supported": ["openid", "offline"], "response_types_supported": ["code"], "claims_supported": ["groups"], + "subject_types_supported": ["public"], + "id_token_signing_alg_values_supported": ["RS256"] }`) expectedJSON := fmt.Sprintf(expectedResultTemplate, issuer, issuer, issuer, issuer)