From e25eb054502def3b6fb10fcd53028b12196541c3 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Tue, 11 May 2021 10:31:33 -0700 Subject: [PATCH] Move Supervisor IDP discovery to its own new endpoint --- cmd/pinniped/cmd/kubeconfig.go | 121 ++++-- cmd/pinniped/cmd/kubeconfig_test.go | 369 ++++++++++++++---- internal/oidc/discovery/discovery_handler.go | 64 +-- .../oidc/discovery/discovery_handler_test.go | 85 +--- .../idpdiscovery/idp_discovery_handler.go | 75 ++++ .../idp_discovery_handler_test.go | 126 ++++++ internal/oidc/oidc.go | 1 + internal/oidc/provider/manager/manager.go | 5 +- .../oidc/provider/manager/manager_test.go | 45 ++- 9 files changed, 660 insertions(+), 231 deletions(-) create mode 100644 internal/oidc/idpdiscovery/idp_discovery_handler.go create mode 100644 internal/oidc/idpdiscovery/idp_discovery_handler_test.go diff --git a/cmd/pinniped/cmd/kubeconfig.go b/cmd/pinniped/cmd/kubeconfig.go index d59b61ba..832b1365 100644 --- a/cmd/pinniped/cmd/kubeconfig.go +++ b/cmd/pinniped/cmd/kubeconfig.go @@ -95,8 +95,12 @@ type getKubeconfigParams struct { credentialCachePathSet bool } -type supervisorDiscoveryResponse struct { - PinnipedIDPs []pinnipedIDPResponse `json:"pinniped_idps"` +type supervisorOIDCDiscoveryResponse struct { + PinnipedIDPsEndpoint string `json:"pinniped_identity_providers_endpoint"` +} + +type supervisorIDPsDiscoveryResponse struct { + PinnipedIDPs []pinnipedIDPResponse `json:"pinniped_identity_providers"` } type pinnipedIDPResponse struct { @@ -727,57 +731,38 @@ func hasPendingStrategy(credentialIssuer *configv1alpha1.CredentialIssuer) bool } func discoverSupervisorUpstreamIDP(ctx context.Context, flags *getKubeconfigParams) error { - issuerDiscoveryURL := flags.oidc.issuer + "/.well-known/openid-configuration" - request, err := http.NewRequestWithContext(ctx, http.MethodGet, issuerDiscoveryURL, nil) - if err != nil { - return fmt.Errorf("while forming request to issuer URL: %w", err) - } - transport := &http.Transport{ TLSClientConfig: &tls.Config{MinVersion: tls.VersionTLS12}, Proxy: http.ProxyFromEnvironment, } - httpClient := http.Client{Transport: transport} + httpClient := &http.Client{Transport: transport} if flags.oidc.caBundle != nil { rootCAs := x509.NewCertPool() ok := rootCAs.AppendCertsFromPEM(flags.oidc.caBundle) if !ok { - return fmt.Errorf("unable to fetch discovery data from issuer: could not parse CA bundle") + return fmt.Errorf("unable to fetch OIDC discovery data from issuer: could not parse CA bundle") } transport.TLSClientConfig.RootCAs = rootCAs } - response, err := httpClient.Do(request) + pinnipedIDPsEndpoint, err := discoverIDPsDiscoveryEndpointURL(ctx, flags.oidc.issuer, httpClient) if err != nil { - return fmt.Errorf("unable to fetch discovery data from issuer: %w", err) + return err } - defer func() { - _ = response.Body.Close() - }() - if response.StatusCode == http.StatusNotFound { - // 404 Not Found is not an error because OIDC discovery is an optional part of the OIDC spec. + if pinnipedIDPsEndpoint == "" { + // The issuer is not advertising itself as a Pinniped Supervisor which supports upstream IDP discovery. return nil } - if response.StatusCode != http.StatusOK { - // Other types of error responses aside from 404 are not expected. - return fmt.Errorf("unable to fetch discovery data from issuer: unexpected http response status: %s", response.Status) - } - rawBody, err := ioutil.ReadAll(response.Body) + upstreamIDPs, err := discoverAllAvailableSupervisorUpstreamIDPs(ctx, pinnipedIDPsEndpoint, httpClient) if err != nil { - return fmt.Errorf("unable to fetch discovery data from issuer: could not read response body: %w", err) + return err } - var body supervisorDiscoveryResponse - err = json.Unmarshal(rawBody, &body) - if err != nil { - return fmt.Errorf("unable to fetch discovery data from issuer: could not parse response JSON: %w", err) - } - - if len(body.PinnipedIDPs) == 1 { - flags.oidc.upstreamIDPName = body.PinnipedIDPs[0].Name - flags.oidc.upstreamIDPType = body.PinnipedIDPs[0].Type - } else if len(body.PinnipedIDPs) > 1 { - idpName, idpType, err := selectUpstreamIDP(body.PinnipedIDPs, flags.oidc.upstreamIDPName, flags.oidc.upstreamIDPType) + if len(upstreamIDPs) == 1 { + flags.oidc.upstreamIDPName = upstreamIDPs[0].Name + flags.oidc.upstreamIDPType = upstreamIDPs[0].Type + } else if len(upstreamIDPs) > 1 { + idpName, idpType, err := selectUpstreamIDP(upstreamIDPs, flags.oidc.upstreamIDPName, flags.oidc.upstreamIDPType) if err != nil { return err } @@ -787,6 +772,74 @@ func discoverSupervisorUpstreamIDP(ctx context.Context, flags *getKubeconfigPara return nil } +func discoverIDPsDiscoveryEndpointURL(ctx context.Context, issuer string, httpClient *http.Client) (string, error) { + issuerDiscoveryURL := issuer + "/.well-known/openid-configuration" + request, err := http.NewRequestWithContext(ctx, http.MethodGet, issuerDiscoveryURL, nil) + if err != nil { + return "", fmt.Errorf("while forming request to issuer URL: %w", err) + } + + response, err := httpClient.Do(request) + if err != nil { + return "", fmt.Errorf("unable to fetch OIDC discovery data from issuer: %w", err) + } + defer func() { + _ = response.Body.Close() + }() + if response.StatusCode == http.StatusNotFound { + // 404 Not Found is not an error because OIDC discovery is an optional part of the OIDC spec. + return "", nil + } + if response.StatusCode != http.StatusOK { + // Other types of error responses aside from 404 are not expected. + return "", fmt.Errorf("unable to fetch OIDC discovery data from issuer: unexpected http response status: %s", response.Status) + } + + rawBody, err := ioutil.ReadAll(response.Body) + if err != nil { + return "", fmt.Errorf("unable to fetch OIDC discovery data from issuer: could not read response body: %w", err) + } + + var body supervisorOIDCDiscoveryResponse + err = json.Unmarshal(rawBody, &body) + if err != nil { + return "", fmt.Errorf("unable to fetch OIDC discovery data from issuer: could not parse response JSON: %w", err) + } + + return body.PinnipedIDPsEndpoint, nil +} + +func discoverAllAvailableSupervisorUpstreamIDPs(ctx context.Context, pinnipedIDPsEndpoint string, httpClient *http.Client) ([]pinnipedIDPResponse, error) { + request, err := http.NewRequestWithContext(ctx, http.MethodGet, pinnipedIDPsEndpoint, nil) + if err != nil { + return nil, fmt.Errorf("while forming request to IDP discovery URL: %w", err) + } + + response, err := httpClient.Do(request) + if err != nil { + return nil, fmt.Errorf("unable to fetch IDP discovery data from issuer: %w", err) + } + defer func() { + _ = response.Body.Close() + }() + if response.StatusCode != http.StatusOK { + return nil, fmt.Errorf("unable to fetch IDP discovery data from issuer: unexpected http response status: %s", response.Status) + } + + rawBody, err := ioutil.ReadAll(response.Body) + if err != nil { + return nil, fmt.Errorf("unable to fetch IDP discovery data from issuer: could not read response body: %w", err) + } + + var body supervisorIDPsDiscoveryResponse + err = json.Unmarshal(rawBody, &body) + if err != nil { + return nil, fmt.Errorf("unable to fetch IDP discovery data from issuer: could not parse response JSON: %w", err) + } + + return body.PinnipedIDPs, nil +} + func selectUpstreamIDP(pinnipedIDPs []pinnipedIDPResponse, idpName, idpType string) (string, string, error) { pinnipedIDPsString, _ := json.Marshal(pinnipedIDPs) switch { diff --git a/cmd/pinniped/cmd/kubeconfig_test.go b/cmd/pinniped/cmd/kubeconfig_test.go index e244e14b..c2223350 100644 --- a/cmd/pinniped/cmd/kubeconfig_test.go +++ b/cmd/pinniped/cmd/kubeconfig_test.go @@ -75,21 +75,23 @@ func TestGetKubeconfig(t *testing.T) { } tests := []struct { - name string - args func(string, string) []string - env map[string]string - getPathToSelfErr error - getClientsetErr error - conciergeObjects func(string, string) []runtime.Object - conciergeReactions []kubetesting.Reactor - discoveryResponse string - discoveryStatusCode int - wantLogs func(string, string) []string - wantError bool - wantStdout func(string, string) string - wantStderr func(string, string) string - wantOptionsCount int - wantAPIGroupSuffix string + name string + args func(string, string) []string + env map[string]string + getPathToSelfErr error + getClientsetErr error + conciergeObjects func(string, string) []runtime.Object + conciergeReactions []kubetesting.Reactor + oidcDiscoveryResponse func(string) string + oidcDiscoveryStatusCode int + idpsDiscoveryResponse string + idpsDiscoveryStatusCode int + wantLogs func(string, string) []string + wantError bool + wantStdout func(string, string) string + wantStderr func(string, string) string + wantOptionsCount int + wantAPIGroupSuffix string }{ { name: "help flag passed", @@ -690,7 +692,7 @@ func TestGetKubeconfig(t *testing.T) { }, }, { - name: "when discovery document 400s", + name: "when OIDC discovery document 400s", args: func(issuerCABundle string, issuerURL string) []string { return []string{ "--kubeconfig", "./testdata/kubeconfig.yaml", @@ -703,7 +705,7 @@ func TestGetKubeconfig(t *testing.T) { jwtAuthenticator(issuerCABundle, issuerURL), } }, - discoveryStatusCode: http.StatusBadRequest, + oidcDiscoveryStatusCode: http.StatusBadRequest, wantLogs: func(issuerCABundle string, issuerURL string) []string { return []string{ `"level"=0 "msg"="discovered CredentialIssuer" "name"="test-credential-issuer"`, @@ -718,11 +720,11 @@ func TestGetKubeconfig(t *testing.T) { }, wantError: true, wantStderr: func(issuerCABundle string, issuerURL string) string { - return "Error: unable to fetch discovery data from issuer: unexpected http response status: 400 Bad Request\n" + return "Error: unable to fetch OIDC discovery data from issuer: unexpected http response status: 400 Bad Request\n" }, }, { - name: "when discovery document contains multiple pinniped_idps and no name or type flags are given", + name: "when IDP discovery document returns any error", args: func(issuerCABundle string, issuerURL string) []string { return []string{ "--kubeconfig", "./testdata/kubeconfig.yaml", @@ -735,9 +737,46 @@ func TestGetKubeconfig(t *testing.T) { jwtAuthenticator(issuerCABundle, issuerURL), } }, - discoveryStatusCode: http.StatusOK, - discoveryResponse: here.Docf(`{ - "pinniped_idps": [ + oidcDiscoveryResponse: func(issuerURL string) string { + return fmt.Sprintf(`{"pinniped_identity_providers_endpoint": "%s/pinniped_identity_providers"}`, issuerURL) + }, + idpsDiscoveryStatusCode: http.StatusBadRequest, + wantLogs: func(issuerCABundle string, issuerURL string) []string { + return []string{ + `"level"=0 "msg"="discovered CredentialIssuer" "name"="test-credential-issuer"`, + `"level"=0 "msg"="discovered Concierge operating in TokenCredentialRequest API mode"`, + `"level"=0 "msg"="discovered Concierge endpoint" "endpoint"="https://fake-server-url-value"`, + `"level"=0 "msg"="discovered Concierge certificate authority bundle" "roots"=0`, + `"level"=0 "msg"="discovered JWTAuthenticator" "name"="test-authenticator"`, + fmt.Sprintf(`"level"=0 "msg"="discovered OIDC issuer" "issuer"="%s"`, issuerURL), + `"level"=0 "msg"="discovered OIDC audience" "audience"="test-audience"`, + `"level"=0 "msg"="discovered OIDC CA bundle" "roots"=1`, + } + }, + wantError: true, + wantStderr: func(issuerCABundle string, issuerURL string) string { + return "Error: unable to fetch IDP discovery data from issuer: unexpected http response status: 400 Bad Request\n" + }, + }, + { + name: "when IDP discovery document contains multiple pinniped_idps and no name or type flags are given", + args: func(issuerCABundle string, issuerURL string) []string { + return []string{ + "--kubeconfig", "./testdata/kubeconfig.yaml", + "--skip-validation", + } + }, + conciergeObjects: func(issuerCABundle string, issuerURL string) []runtime.Object { + return []runtime.Object{ + credentialIssuer(), + jwtAuthenticator(issuerCABundle, issuerURL), + } + }, + oidcDiscoveryResponse: func(issuerURL string) string { + return fmt.Sprintf(`{"pinniped_identity_providers_endpoint": "%s/pinniped_identity_providers"}`, issuerURL) + }, + idpsDiscoveryResponse: here.Docf(`{ + "pinniped_identity_providers": [ {"name": "some-ldap-idp", "type": "ldap"}, {"name": "some-oidc-idp", "type": "oidc"} ] @@ -762,7 +801,7 @@ func TestGetKubeconfig(t *testing.T) { }, }, { - name: "when discovery document is not valid JSON", + name: "when OIDC discovery document is not valid JSON", args: func(issuerCABundle string, issuerURL string) []string { return []string{ "--kubeconfig", "./testdata/kubeconfig.yaml", @@ -775,8 +814,9 @@ func TestGetKubeconfig(t *testing.T) { jwtAuthenticator(issuerCABundle, issuerURL), } }, - discoveryStatusCode: http.StatusOK, - discoveryResponse: "this is not valid JSON", + oidcDiscoveryResponse: func(issuerURL string) string { + return "this is not valid JSON" + }, wantLogs: func(issuerCABundle string, issuerURL string) []string { return []string{ `"level"=0 "msg"="discovered CredentialIssuer" "name"="test-credential-issuer"`, @@ -791,11 +831,46 @@ func TestGetKubeconfig(t *testing.T) { }, wantError: true, wantStderr: func(issuerCABundle string, issuerURL string) string { - return "Error: unable to fetch discovery data from issuer: could not parse response JSON: invalid character 'h' in literal true (expecting 'r')\n" + return "Error: unable to fetch OIDC discovery data from issuer: could not parse response JSON: invalid character 'h' in literal true (expecting 'r')\n" }, }, { - name: "when tls information is missing from jwtauthenticator, test fails because discovery fails", + name: "when IDP discovery document is not valid JSON", + args: func(issuerCABundle string, issuerURL string) []string { + return []string{ + "--kubeconfig", "./testdata/kubeconfig.yaml", + "--skip-validation", + } + }, + conciergeObjects: func(issuerCABundle string, issuerURL string) []runtime.Object { + return []runtime.Object{ + credentialIssuer(), + jwtAuthenticator(issuerCABundle, issuerURL), + } + }, + oidcDiscoveryResponse: func(issuerURL string) string { + return fmt.Sprintf(`{"pinniped_identity_providers_endpoint": "%s/pinniped_identity_providers"}`, issuerURL) + }, + idpsDiscoveryResponse: "this is not valid JSON", + wantLogs: func(issuerCABundle string, issuerURL string) []string { + return []string{ + `"level"=0 "msg"="discovered CredentialIssuer" "name"="test-credential-issuer"`, + `"level"=0 "msg"="discovered Concierge operating in TokenCredentialRequest API mode"`, + `"level"=0 "msg"="discovered Concierge endpoint" "endpoint"="https://fake-server-url-value"`, + `"level"=0 "msg"="discovered Concierge certificate authority bundle" "roots"=0`, + `"level"=0 "msg"="discovered JWTAuthenticator" "name"="test-authenticator"`, + fmt.Sprintf(`"level"=0 "msg"="discovered OIDC issuer" "issuer"="%s"`, issuerURL), + `"level"=0 "msg"="discovered OIDC audience" "audience"="test-audience"`, + `"level"=0 "msg"="discovered OIDC CA bundle" "roots"=1`, + } + }, + wantError: true, + wantStderr: func(issuerCABundle string, issuerURL string) string { + return "Error: unable to fetch IDP discovery data from issuer: could not parse response JSON: invalid character 'h' in literal true (expecting 'r')\n" + }, + }, + { + name: "when tls information is missing from jwtauthenticator, errors because OIDC discovery fails", args: func(issuerCABundle string, issuerURL string) []string { return []string{ "--kubeconfig", "./testdata/kubeconfig.yaml", @@ -827,7 +902,7 @@ func TestGetKubeconfig(t *testing.T) { }, wantError: true, wantStderr: func(issuerCABundle string, issuerURL string) string { - return fmt.Sprintf("Error: unable to fetch discovery data from issuer: Get \"%s/.well-known/openid-configuration\": x509: certificate signed by unknown authority\n", issuerURL) + return fmt.Sprintf("Error: unable to fetch OIDC discovery data from issuer: Get \"%s/.well-known/openid-configuration\": x509: certificate signed by unknown authority\n", issuerURL) }, }, { @@ -868,6 +943,40 @@ func TestGetKubeconfig(t *testing.T) { return `Error: while forming request to issuer URL: parse "https%://bad-issuer-url/.well-known/openid-configuration": first path segment in URL cannot contain colon` + "\n" }, }, + { + name: "when the IDP discovery url is bad", + args: func(issuerCABundle string, issuerURL string) []string { + return []string{ + "--kubeconfig", "./testdata/kubeconfig.yaml", + "--skip-validation", + } + }, + conciergeObjects: func(issuerCABundle string, issuerURL string) []runtime.Object { + return []runtime.Object{ + credentialIssuer(), + jwtAuthenticator(issuerCABundle, issuerURL), + } + }, + oidcDiscoveryResponse: func(issuerURL string) string { + return `{"pinniped_identity_providers_endpoint": "https%://illegal_url"}` + }, + wantLogs: func(issuerCABundle string, issuerURL string) []string { + return []string{ + `"level"=0 "msg"="discovered CredentialIssuer" "name"="test-credential-issuer"`, + `"level"=0 "msg"="discovered Concierge operating in TokenCredentialRequest API mode"`, + `"level"=0 "msg"="discovered Concierge endpoint" "endpoint"="https://fake-server-url-value"`, + `"level"=0 "msg"="discovered Concierge certificate authority bundle" "roots"=0`, + `"level"=0 "msg"="discovered JWTAuthenticator" "name"="test-authenticator"`, + fmt.Sprintf(`"level"=0 "msg"="discovered OIDC issuer" "issuer"="%s"`, issuerURL), + `"level"=0 "msg"="discovered OIDC audience" "audience"="test-audience"`, + `"level"=0 "msg"="discovered OIDC CA bundle" "roots"=1`, + } + }, + wantError: true, + wantStderr: func(issuerCABundle string, issuerURL string) string { + return `Error: while forming request to IDP discovery URL: parse "https%://illegal_url": first path segment in URL cannot contain colon` + "\n" + }, + }, { name: "supervisor upstream IDP discovery fails to resolve ambiguity when type is specified but name is not", args: func(issuerCABundle string, issuerURL string) []string { @@ -881,8 +990,11 @@ func TestGetKubeconfig(t *testing.T) { "--upstream-identity-provider-type", "ldap", } }, - discoveryResponse: here.Docf(`{ - "pinniped_idps": [ + oidcDiscoveryResponse: func(issuerURL string) string { + return fmt.Sprintf(`{"pinniped_identity_providers_endpoint": "%s/pinniped_identity_providers"}`, issuerURL) + }, + idpsDiscoveryResponse: here.Docf(`{ + "pinniped_identity_providers": [ {"name": "some-ldap-idp", "type": "ldap"}, {"name": "some-other-ldap-idp", "type": "ldap"}, {"name": "some-oidc-idp", "type": "oidc"}, @@ -909,8 +1021,11 @@ func TestGetKubeconfig(t *testing.T) { "--upstream-identity-provider-name", "my-idp", } }, - discoveryResponse: here.Docf(`{ - "pinniped_idps": [ + oidcDiscoveryResponse: func(issuerURL string) string { + return fmt.Sprintf(`{"pinniped_identity_providers_endpoint": "%s/pinniped_identity_providers"}`, issuerURL) + }, + idpsDiscoveryResponse: here.Docf(`{ + "pinniped_identity_providers": [ {"name": "my-idp", "type": "ldap"}, {"name": "my-idp", "type": "oidc"}, {"name": "some-other-oidc-idp", "type": "oidc"} @@ -936,8 +1051,11 @@ func TestGetKubeconfig(t *testing.T) { "--upstream-identity-provider-type", "ldap", } }, - discoveryResponse: here.Docf(`{ - "pinniped_idps": [ + oidcDiscoveryResponse: func(issuerURL string) string { + return fmt.Sprintf(`{"pinniped_identity_providers_endpoint": "%s/pinniped_identity_providers"}`, issuerURL) + }, + idpsDiscoveryResponse: here.Docf(`{ + "pinniped_identity_providers": [ {"name": "some-oidc-idp", "type": "oidc"}, {"name": "some-other-oidc-idp", "type": "oidc"} ] @@ -961,8 +1079,11 @@ func TestGetKubeconfig(t *testing.T) { "--upstream-identity-provider-name", "my-nonexistent-idp", } }, - discoveryResponse: here.Docf(`{ - "pinniped_idps": [ + oidcDiscoveryResponse: func(issuerURL string) string { + return fmt.Sprintf(`{"pinniped_identity_providers_endpoint": "%s/pinniped_identity_providers"}`, issuerURL) + }, + idpsDiscoveryResponse: here.Docf(`{ + "pinniped_identity_providers": [ {"name": "some-oidc-idp", "type": "oidc"}, {"name": "some-other-oidc-idp", "type": "oidc"} ] @@ -1464,7 +1585,7 @@ func TestGetKubeconfig(t *testing.T) { }, }, { - name: "Find LDAP idp in discovery document, output ldap related flags", + name: "Find LDAP IDP in IDP discovery document, output ldap related flags", args: func(issuerCABundle string, issuerURL string) []string { return []string{ "--kubeconfig", "./testdata/kubeconfig.yaml", @@ -1477,8 +1598,13 @@ func TestGetKubeconfig(t *testing.T) { jwtAuthenticator(issuerCABundle, issuerURL), } }, - discoveryResponse: here.Docf(`{ - "pinniped_idps": [{"name": "some-ldap-idp", "type": "ldap"}] + oidcDiscoveryResponse: func(issuerURL string) string { + return fmt.Sprintf(`{"pinniped_identity_providers_endpoint": "%s/pinniped_identity_providers"}`, issuerURL) + }, + idpsDiscoveryResponse: here.Docf(`{ + "pinniped_identity_providers": [ + {"name": "some-ldap-idp", "type": "ldap"} + ] }`), wantLogs: func(issuerCABundle string, issuerURL string) []string { return []string{ @@ -1538,7 +1664,7 @@ func TestGetKubeconfig(t *testing.T) { }, }, { - name: "Find OIDC idp in discovery document, output oidc related flags", + name: "Find OIDC IDP in IDP discovery document, output oidc related flags", args: func(issuerCABundle string, issuerURL string) []string { return []string{ "--kubeconfig", "./testdata/kubeconfig.yaml", @@ -1551,8 +1677,13 @@ func TestGetKubeconfig(t *testing.T) { jwtAuthenticator(issuerCABundle, issuerURL), } }, - discoveryResponse: here.Docf(`{ - "pinniped_idps": [{"name": "some-oidc-idp", "type": "oidc"}] + oidcDiscoveryResponse: func(issuerURL string) string { + return fmt.Sprintf(`{"pinniped_identity_providers_endpoint": "%s/pinniped_identity_providers"}`, issuerURL) + }, + idpsDiscoveryResponse: here.Docf(`{ + "pinniped_identity_providers": [ + {"name": "some-oidc-idp", "type": "oidc"} + ] }`), wantLogs: func(issuerCABundle string, issuerURL string) []string { return []string{ @@ -1612,7 +1743,7 @@ func TestGetKubeconfig(t *testing.T) { }, }, { - name: "empty idp list in discovery document", + name: "empty IDP list in IDP discovery document", args: func(issuerCABundle string, issuerURL string) []string { return []string{ "--kubeconfig", "./testdata/kubeconfig.yaml", @@ -1625,8 +1756,11 @@ func TestGetKubeconfig(t *testing.T) { jwtAuthenticator(issuerCABundle, issuerURL), } }, - discoveryResponse: here.Docf(`{ - "pinniped_idps": [] + oidcDiscoveryResponse: func(issuerURL string) string { + return fmt.Sprintf(`{"pinniped_identity_providers_endpoint": "%s/pinniped_identity_providers"}`, issuerURL) + }, + idpsDiscoveryResponse: here.Docf(`{ + "pinniped_identity_providers": [] }`), wantLogs: func(issuerCABundle string, issuerURL string) []string { return []string{ @@ -1684,7 +1818,7 @@ func TestGetKubeconfig(t *testing.T) { }, }, { - name: "when discovery document 404s, dont set idp related flags", + name: "IDP discovery endpoint is not listed in OIDC discovery document", args: func(issuerCABundle string, issuerURL string) []string { return []string{ "--kubeconfig", "./testdata/kubeconfig.yaml", @@ -1697,7 +1831,80 @@ func TestGetKubeconfig(t *testing.T) { jwtAuthenticator(issuerCABundle, issuerURL), } }, - discoveryStatusCode: http.StatusNotFound, + oidcDiscoveryResponse: func(issuerURL string) string { + return `{"other_field": "other_value"}` + }, + idpsDiscoveryStatusCode: http.StatusBadRequest, // IDPs endpoint shouldn't be called by this test + wantLogs: func(issuerCABundle string, issuerURL string) []string { + return []string{ + `"level"=0 "msg"="discovered CredentialIssuer" "name"="test-credential-issuer"`, + `"level"=0 "msg"="discovered Concierge operating in TokenCredentialRequest API mode"`, + `"level"=0 "msg"="discovered Concierge endpoint" "endpoint"="https://fake-server-url-value"`, + `"level"=0 "msg"="discovered Concierge certificate authority bundle" "roots"=0`, + `"level"=0 "msg"="discovered JWTAuthenticator" "name"="test-authenticator"`, + fmt.Sprintf(`"level"=0 "msg"="discovered OIDC issuer" "issuer"="%s"`, issuerURL), + `"level"=0 "msg"="discovered OIDC audience" "audience"="test-audience"`, + `"level"=0 "msg"="discovered OIDC CA bundle" "roots"=1`, + } + }, + wantStdout: func(issuerCABundle string, issuerURL string) string { + return here.Docf(` + apiVersion: v1 + clusters: + - cluster: + certificate-authority-data: ZmFrZS1jZXJ0aWZpY2F0ZS1hdXRob3JpdHktZGF0YS12YWx1ZQ== + server: https://fake-server-url-value + name: kind-cluster-pinniped + contexts: + - context: + cluster: kind-cluster-pinniped + user: kind-user-pinniped + name: kind-context-pinniped + current-context: kind-context-pinniped + kind: Config + preferences: {} + users: + - name: kind-user-pinniped + user: + exec: + apiVersion: client.authentication.k8s.io/v1beta1 + args: + - login + - oidc + - --enable-concierge + - --concierge-api-group-suffix=pinniped.dev + - --concierge-authenticator-name=test-authenticator + - --concierge-authenticator-type=jwt + - --concierge-endpoint=https://fake-server-url-value + - --concierge-ca-bundle-data=ZmFrZS1jZXJ0aWZpY2F0ZS1hdXRob3JpdHktZGF0YS12YWx1ZQ== + - --issuer=%s + - --client-id=pinniped-cli + - --scopes=offline_access,openid,pinniped:request-audience + - --ca-bundle-data=%s + - --request-audience=test-audience + command: '.../path/to/pinniped' + env: [] + provideClusterInfo: true + `, + issuerURL, + base64.StdEncoding.EncodeToString([]byte(issuerCABundle))) + }, + }, + { + name: "when OIDC discovery document 404s, dont set idp related flags", + args: func(issuerCABundle string, issuerURL string) []string { + return []string{ + "--kubeconfig", "./testdata/kubeconfig.yaml", + "--skip-validation", + } + }, + conciergeObjects: func(issuerCABundle string, issuerURL string) []runtime.Object { + return []runtime.Object{ + credentialIssuer(), + jwtAuthenticator(issuerCABundle, issuerURL), + } + }, + oidcDiscoveryStatusCode: http.StatusNotFound, wantLogs: func(issuerCABundle string, issuerURL string) []string { return []string{ `"level"=0 "msg"="discovered CredentialIssuer" "name"="test-credential-issuer"`, @@ -1769,7 +1976,7 @@ func TestGetKubeconfig(t *testing.T) { jwtAuthenticator(issuerCABundle, issuerURL), } }, - discoveryStatusCode: http.StatusNotFound, + oidcDiscoveryStatusCode: http.StatusNotFound, wantLogs: func(issuerCABundle string, issuerURL string) []string { return []string{ `"level"=0 "msg"="discovered CredentialIssuer" "name"="test-credential-issuer"`, @@ -1828,7 +2035,7 @@ func TestGetKubeconfig(t *testing.T) { }, }, { - name: "when upstream idp related flags are sent, pass them through even when discovery shows a different idp", + name: "when upstream IDP related flags are sent, pass them through even when IDP discovery shows a different IDP", args: func(issuerCABundle string, issuerURL string) []string { return []string{ "--kubeconfig", "./testdata/kubeconfig.yaml", @@ -1843,8 +2050,13 @@ func TestGetKubeconfig(t *testing.T) { jwtAuthenticator(issuerCABundle, issuerURL), } }, - discoveryResponse: here.Docf(`{ - "pinniped_idps": [{"name": "some-other-ldap-idp", "type": "ldap"}] + oidcDiscoveryResponse: func(issuerURL string) string { + return fmt.Sprintf(`{"pinniped_identity_providers_endpoint": "%s/pinniped_identity_providers"}`, issuerURL) + }, + idpsDiscoveryResponse: here.Docf(`{ + "pinniped_identity_providers": [ + {"name": "some-other-ldap-idp", "type": "ldap"} + ] }`), wantLogs: func(issuerCABundle string, issuerURL string) []string { return []string{ @@ -1915,8 +2127,13 @@ func TestGetKubeconfig(t *testing.T) { "--oidc-ca-bundle", f.Name(), } }, - discoveryResponse: here.Docf(`{ - "pinniped_idps": [{"name": "some-ldap-idp", "type": "ldap"}] + oidcDiscoveryResponse: func(issuerURL string) string { + return fmt.Sprintf(`{"pinniped_identity_providers_endpoint": "%s/pinniped_identity_providers"}`, issuerURL) + }, + idpsDiscoveryResponse: here.Docf(`{ + "pinniped_identity_providers": [ + {"name": "some-ldap-idp", "type": "ldap"} + ] }`), wantStdout: func(issuerCABundle string, issuerURL string) string { return here.Docf(` @@ -1969,8 +2186,11 @@ func TestGetKubeconfig(t *testing.T) { "--upstream-identity-provider-type", "ldap", } }, - discoveryResponse: here.Docf(`{ - "pinniped_idps": [ + oidcDiscoveryResponse: func(issuerURL string) string { + return fmt.Sprintf(`{"pinniped_identity_providers_endpoint": "%s/pinniped_identity_providers"}`, issuerURL) + }, + idpsDiscoveryResponse: here.Docf(`{ + "pinniped_identity_providers": [ {"name": "some-ldap-idp", "type": "ldap"}, {"name": "some-oidc-idp", "type": "oidc"}, {"name": "some-other-oidc-idp", "type": "oidc"} @@ -2027,8 +2247,11 @@ func TestGetKubeconfig(t *testing.T) { "--upstream-identity-provider-name", "some-ldap-idp", } }, - discoveryResponse: here.Docf(`{ - "pinniped_idps": [ + oidcDiscoveryResponse: func(issuerURL string) string { + return fmt.Sprintf(`{"pinniped_identity_providers_endpoint": "%s/pinniped_identity_providers"}`, issuerURL) + }, + idpsDiscoveryResponse: here.Docf(`{ + "pinniped_identity_providers": [ {"name": "some-ldap-idp", "type": "ldap"}, {"name": "some-oidc-idp", "type": "oidc"}, {"name": "some-other-oidc-idp", "type": "oidc"} @@ -2076,22 +2299,36 @@ func TestGetKubeconfig(t *testing.T) { for _, tt := range tests { tt := tt t.Run(tt.name, func(t *testing.T) { + var issuerEndpointPtr *string issuerCABundle, issuerEndpoint := testutil.TLSTestServer(t, func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path == "/.well-known/openid-configuration" { - jsonResponseBody := tt.discoveryResponse - if tt.discoveryResponse == "" { - jsonResponseBody = "{}" + switch r.URL.Path { + case "/.well-known/openid-configuration": + jsonResponseBody := "{}" + if tt.oidcDiscoveryResponse != nil { + jsonResponseBody = tt.oidcDiscoveryResponse(*issuerEndpointPtr) } - if tt.discoveryStatusCode == 0 { - tt.discoveryStatusCode = http.StatusOK + if tt.oidcDiscoveryStatusCode == 0 { + tt.oidcDiscoveryStatusCode = http.StatusOK } - w.WriteHeader(tt.discoveryStatusCode) + w.WriteHeader(tt.oidcDiscoveryStatusCode) _, err = w.Write([]byte(jsonResponseBody)) require.NoError(t, err) - } else { - t.Fatalf("tried to call issuer at a path that wasn't the discovery endpoint.") + case "/pinniped_identity_providers": + jsonResponseBody := tt.idpsDiscoveryResponse + if tt.idpsDiscoveryResponse == "" { + jsonResponseBody = "{}" + } + if tt.idpsDiscoveryStatusCode == 0 { + tt.idpsDiscoveryStatusCode = http.StatusOK + } + w.WriteHeader(tt.idpsDiscoveryStatusCode) + _, err = w.Write([]byte(jsonResponseBody)) + require.NoError(t, err) + default: + t.Fatalf("tried to call issuer at a path that wasn't one of the expected discovery endpoints.") } }) + issuerEndpointPtr = &issuerEndpoint testLog := testlogger.New(t) cmd := kubeconfigCommand(kubeconfigDeps{ diff --git a/internal/oidc/discovery/discovery_handler.go b/internal/oidc/discovery/discovery_handler.go index dabdda02..542ef729 100644 --- a/internal/oidc/discovery/discovery_handler.go +++ b/internal/oidc/discovery/discovery_handler.go @@ -8,16 +8,10 @@ import ( "bytes" "encoding/json" "net/http" - "sort" "go.pinniped.dev/internal/oidc" ) -const ( - idpDiscoveryTypeLDAP = "ldap" - idpDiscoveryTypeOIDC = "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. @@ -46,7 +40,7 @@ type Metadata struct { // vvv Custom vvv - IDPs []IdentityProviderMetadata `json:"pinniped_idps"` + PinnipedIDPsEndpoint string `json:"pinniped_identity_providers_endpoint"` // ^^^ Custom ^^^ } @@ -57,14 +51,31 @@ type IdentityProviderMetadata struct { } // NewHandler returns an http.Handler that serves an OIDC discovery endpoint. -func NewHandler(issuerURL string, upstreamIDPs oidc.UpstreamIdentityProvidersLister) http.Handler { +func NewHandler(issuerURL string) http.Handler { + oidcConfig := Metadata{ + Issuer: issuerURL, + AuthorizationEndpoint: issuerURL + oidc.AuthorizationEndpointPath, + TokenEndpoint: issuerURL + oidc.TokenEndpointPath, + JWKSURI: issuerURL + oidc.JWKSEndpointPath, + PinnipedIDPsEndpoint: issuerURL + oidc.PinnipedIDPsPath, + ResponseTypesSupported: []string{"code"}, + SubjectTypesSupported: []string{"public"}, + IDTokenSigningAlgValuesSupported: []string{"ES256"}, + TokenEndpointAuthMethodsSupported: []string{"client_secret_basic"}, + ScopesSupported: []string{"openid", "offline"}, + ClaimsSupported: []string{"groups"}, + } + + var b bytes.Buffer + encodeErr := json.NewEncoder(&b).Encode(&oidcConfig) + encodedMetadata := b.Bytes() + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.Method != http.MethodGet { http.Error(w, `Method not allowed (try GET)`, http.StatusMethodNotAllowed) return } - encodedMetadata, encodeErr := metadata(issuerURL, upstreamIDPs) if encodeErr != nil { http.Error(w, encodeErr.Error(), http.StatusInternalServerError) return @@ -77,38 +88,3 @@ func NewHandler(issuerURL string, upstreamIDPs oidc.UpstreamIdentityProvidersLis } }) } - -func metadata(issuerURL string, upstreamIDPs oidc.UpstreamIdentityProvidersLister) ([]byte, error) { - oidcConfig := Metadata{ - Issuer: issuerURL, - AuthorizationEndpoint: issuerURL + oidc.AuthorizationEndpointPath, - TokenEndpoint: issuerURL + oidc.TokenEndpointPath, - JWKSURI: issuerURL + oidc.JWKSEndpointPath, - ResponseTypesSupported: []string{"code"}, - SubjectTypesSupported: []string{"public"}, - IDTokenSigningAlgValuesSupported: []string{"ES256"}, - TokenEndpointAuthMethodsSupported: []string{"client_secret_basic"}, - ScopesSupported: []string{"openid", "offline"}, - ClaimsSupported: []string{"groups"}, - IDPs: []IdentityProviderMetadata{}, - } - - // The cache of IDPs could change at any time, so always recalculate the list. - for _, provider := range upstreamIDPs.GetLDAPIdentityProviders() { - oidcConfig.IDPs = append(oidcConfig.IDPs, IdentityProviderMetadata{Name: provider.GetName(), Type: idpDiscoveryTypeLDAP}) - } - for _, provider := range upstreamIDPs.GetOIDCIdentityProviders() { - oidcConfig.IDPs = append(oidcConfig.IDPs, IdentityProviderMetadata{Name: provider.GetName(), Type: idpDiscoveryTypeOIDC}) - } - - // Nobody like an API that changes the results unnecessarily. :) - sort.SliceStable(oidcConfig.IDPs, func(i, j int) bool { - return oidcConfig.IDPs[i].Name < oidcConfig.IDPs[j].Name - }) - - var b bytes.Buffer - encodeErr := json.NewEncoder(&b).Encode(&oidcConfig) - encodedMetadata := b.Bytes() - - return encodedMetadata, encodeErr -} diff --git a/internal/oidc/discovery/discovery_handler_test.go b/internal/oidc/discovery/discovery_handler_test.go index 7236e544..fd37341b 100644 --- a/internal/oidc/discovery/discovery_handler_test.go +++ b/internal/oidc/discovery/discovery_handler_test.go @@ -9,10 +9,6 @@ import ( "net/http/httptest" "testing" - "go.pinniped.dev/internal/oidc/provider" - - "go.pinniped.dev/internal/testutil/oidctestutil" - "github.com/stretchr/testify/require" "go.pinniped.dev/internal/oidc" @@ -26,11 +22,10 @@ func TestDiscovery(t *testing.T) { method string path string - wantStatus int - wantContentType string - wantFirstResponseBodyJSON interface{} - wantSecondResponseBodyJSON interface{} - wantBodyString string + wantStatus int + wantContentType string + wantBodyJSON interface{} + wantBodyString string }{ { name: "happy path", @@ -39,43 +34,18 @@ func TestDiscovery(t *testing.T) { path: "/some/path" + oidc.WellKnownEndpointPath, wantStatus: http.StatusOK, wantContentType: "application/json", - wantFirstResponseBodyJSON: &Metadata{ + wantBodyJSON: &Metadata{ Issuer: "https://some-issuer.com/some/path", AuthorizationEndpoint: "https://some-issuer.com/some/path/oauth2/authorize", TokenEndpoint: "https://some-issuer.com/some/path/oauth2/token", JWKSURI: "https://some-issuer.com/some/path/jwks.json", + PinnipedIDPsEndpoint: "https://some-issuer.com/some/path/pinniped_identity_providers", ResponseTypesSupported: []string{"code"}, SubjectTypesSupported: []string{"public"}, IDTokenSigningAlgValuesSupported: []string{"ES256"}, TokenEndpointAuthMethodsSupported: []string{"client_secret_basic"}, ScopesSupported: []string{"openid", "offline"}, ClaimsSupported: []string{"groups"}, - IDPs: []IdentityProviderMetadata{ - {Name: "a-some-ldap-idp", Type: "ldap"}, - {Name: "a-some-oidc-idp", Type: "oidc"}, - {Name: "x-some-idp", Type: "ldap"}, - {Name: "x-some-idp", Type: "oidc"}, - {Name: "z-some-ldap-idp", Type: "ldap"}, - {Name: "z-some-oidc-idp", Type: "oidc"}, - }, - }, - wantSecondResponseBodyJSON: &Metadata{ - Issuer: "https://some-issuer.com/some/path", - AuthorizationEndpoint: "https://some-issuer.com/some/path/oauth2/authorize", - TokenEndpoint: "https://some-issuer.com/some/path/oauth2/token", - JWKSURI: "https://some-issuer.com/some/path/jwks.json", - ResponseTypesSupported: []string{"code"}, - SubjectTypesSupported: []string{"public"}, - IDTokenSigningAlgValuesSupported: []string{"ES256"}, - TokenEndpointAuthMethodsSupported: []string{"client_secret_basic"}, - ScopesSupported: []string{"openid", "offline"}, - ClaimsSupported: []string{"groups"}, - IDPs: []IdentityProviderMetadata{ - {Name: "some-other-ldap-idp-1", Type: "ldap"}, - {Name: "some-other-ldap-idp-2", Type: "ldap"}, - {Name: "some-other-oidc-idp-1", Type: "oidc"}, - {Name: "some-other-oidc-idp-2", Type: "oidc"}, - }, }, }, { @@ -91,16 +61,7 @@ func TestDiscovery(t *testing.T) { for _, test := range tests { test := test t.Run(test.name, func(t *testing.T) { - idpLister := oidctestutil.NewUpstreamIDPListerBuilder(). - WithOIDC(&oidctestutil.TestUpstreamOIDCIdentityProvider{Name: "z-some-oidc-idp"}). - WithOIDC(&oidctestutil.TestUpstreamOIDCIdentityProvider{Name: "x-some-idp"}). - WithLDAP(&oidctestutil.TestUpstreamLDAPIdentityProvider{Name: "a-some-ldap-idp"}). - WithOIDC(&oidctestutil.TestUpstreamOIDCIdentityProvider{Name: "a-some-oidc-idp"}). - WithLDAP(&oidctestutil.TestUpstreamLDAPIdentityProvider{Name: "z-some-ldap-idp"}). - WithLDAP(&oidctestutil.TestUpstreamLDAPIdentityProvider{Name: "x-some-idp"}). - Build() - - handler := NewHandler(test.issuer, idpLister) + handler := NewHandler(test.issuer) req := httptest.NewRequest(test.method, test.path, nil) rsp := httptest.NewRecorder() handler.ServeHTTP(rsp, req) @@ -109,36 +70,8 @@ func TestDiscovery(t *testing.T) { require.Equal(t, test.wantContentType, rsp.Header().Get("Content-Type")) - if test.wantFirstResponseBodyJSON != nil { - wantJSON, err := json.Marshal(test.wantFirstResponseBodyJSON) - require.NoError(t, err) - require.JSONEq(t, string(wantJSON), rsp.Body.String()) - } - - if test.wantBodyString != "" { - require.Equal(t, test.wantBodyString, rsp.Body.String()) - } - - // Change the list of IDPs in the cache. - idpLister.SetLDAPIdentityProviders([]provider.UpstreamLDAPIdentityProviderI{ - &oidctestutil.TestUpstreamLDAPIdentityProvider{Name: "some-other-ldap-idp-1"}, - &oidctestutil.TestUpstreamLDAPIdentityProvider{Name: "some-other-ldap-idp-2"}, - }) - idpLister.SetOIDCIdentityProviders([]provider.UpstreamOIDCIdentityProviderI{ - &oidctestutil.TestUpstreamOIDCIdentityProvider{Name: "some-other-oidc-idp-1"}, - &oidctestutil.TestUpstreamOIDCIdentityProvider{Name: "some-other-oidc-idp-2"}, - }) - - // Make the same request to the same handler instance again, and expect different results. - rsp = httptest.NewRecorder() - handler.ServeHTTP(rsp, req) - - require.Equal(t, test.wantStatus, rsp.Code) - - require.Equal(t, test.wantContentType, rsp.Header().Get("Content-Type")) - - if test.wantFirstResponseBodyJSON != nil { - wantJSON, err := json.Marshal(test.wantSecondResponseBodyJSON) + if test.wantBodyJSON != nil { + wantJSON, err := json.Marshal(test.wantBodyJSON) require.NoError(t, err) require.JSONEq(t, string(wantJSON), rsp.Body.String()) } diff --git a/internal/oidc/idpdiscovery/idp_discovery_handler.go b/internal/oidc/idpdiscovery/idp_discovery_handler.go new file mode 100644 index 00000000..9ee0bf76 --- /dev/null +++ b/internal/oidc/idpdiscovery/idp_discovery_handler.go @@ -0,0 +1,75 @@ +// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +// Package idpdiscovery provides a handler for the upstream IDP discovery endpoint. +package idpdiscovery + +import ( + "bytes" + "encoding/json" + "net/http" + "sort" + + "go.pinniped.dev/internal/oidc" +) + +const ( + idpDiscoveryTypeLDAP = "ldap" + idpDiscoveryTypeOIDC = "oidc" +) + +type response struct { + IDPs []identityProviderResponse `json:"pinniped_identity_providers"` +} + +type identityProviderResponse struct { + Name string `json:"name"` + Type string `json:"type"` +} + +// NewHandler returns an http.Handler that serves the upstream IDP discovery endpoint. +func NewHandler(upstreamIDPs oidc.UpstreamIdentityProvidersLister) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet { + http.Error(w, `Method not allowed (try GET)`, http.StatusMethodNotAllowed) + return + } + + encodedMetadata, encodeErr := responseAsJSON(upstreamIDPs) + if encodeErr != nil { + http.Error(w, encodeErr.Error(), http.StatusInternalServerError) + return + } + + w.Header().Set("Content-Type", "application/json") + if _, err := w.Write(encodedMetadata); err != nil { + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + }) +} + +func responseAsJSON(upstreamIDPs oidc.UpstreamIdentityProvidersLister) ([]byte, error) { + r := response{ + IDPs: []identityProviderResponse{}, + } + + // The cache of IDPs could change at any time, so always recalculate the list. + for _, provider := range upstreamIDPs.GetLDAPIdentityProviders() { + r.IDPs = append(r.IDPs, identityProviderResponse{Name: provider.GetName(), Type: idpDiscoveryTypeLDAP}) + } + for _, provider := range upstreamIDPs.GetOIDCIdentityProviders() { + r.IDPs = append(r.IDPs, identityProviderResponse{Name: provider.GetName(), Type: idpDiscoveryTypeOIDC}) + } + + // Nobody like an API that changes the results unnecessarily. :) + sort.SliceStable(r.IDPs, func(i, j int) bool { + return r.IDPs[i].Name < r.IDPs[j].Name + }) + + var b bytes.Buffer + encodeErr := json.NewEncoder(&b).Encode(&r) + encodedMetadata := b.Bytes() + + return encodedMetadata, encodeErr +} diff --git a/internal/oidc/idpdiscovery/idp_discovery_handler_test.go b/internal/oidc/idpdiscovery/idp_discovery_handler_test.go new file mode 100644 index 00000000..3912f9c9 --- /dev/null +++ b/internal/oidc/idpdiscovery/idp_discovery_handler_test.go @@ -0,0 +1,126 @@ +// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package idpdiscovery + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/require" + + "go.pinniped.dev/internal/oidc" + "go.pinniped.dev/internal/oidc/provider" + "go.pinniped.dev/internal/testutil/oidctestutil" +) + +func TestIDPDiscovery(t *testing.T) { + tests := []struct { + name string + + method string + path string + + wantStatus int + wantContentType string + wantFirstResponseBodyJSON interface{} + wantSecondResponseBodyJSON interface{} + wantBodyString string + }{ + { + name: "happy path", + method: http.MethodGet, + path: "/some/path" + oidc.WellKnownEndpointPath, + wantStatus: http.StatusOK, + wantContentType: "application/json", + wantFirstResponseBodyJSON: &response{ + IDPs: []identityProviderResponse{ + {Name: "a-some-ldap-idp", Type: "ldap"}, + {Name: "a-some-oidc-idp", Type: "oidc"}, + {Name: "x-some-idp", Type: "ldap"}, + {Name: "x-some-idp", Type: "oidc"}, + {Name: "z-some-ldap-idp", Type: "ldap"}, + {Name: "z-some-oidc-idp", Type: "oidc"}, + }, + }, + wantSecondResponseBodyJSON: &response{ + IDPs: []identityProviderResponse{ + {Name: "some-other-ldap-idp-1", Type: "ldap"}, + {Name: "some-other-ldap-idp-2", Type: "ldap"}, + {Name: "some-other-oidc-idp-1", Type: "oidc"}, + {Name: "some-other-oidc-idp-2", Type: "oidc"}, + }, + }, + }, + { + name: "bad method", + method: http.MethodPost, + path: oidc.WellKnownEndpointPath, + wantStatus: http.StatusMethodNotAllowed, + wantContentType: "text/plain; charset=utf-8", + wantBodyString: "Method not allowed (try GET)\n", + }, + } + for _, test := range tests { + test := test + t.Run(test.name, func(t *testing.T) { + idpLister := oidctestutil.NewUpstreamIDPListerBuilder(). + WithOIDC(&oidctestutil.TestUpstreamOIDCIdentityProvider{Name: "z-some-oidc-idp"}). + WithOIDC(&oidctestutil.TestUpstreamOIDCIdentityProvider{Name: "x-some-idp"}). + WithLDAP(&oidctestutil.TestUpstreamLDAPIdentityProvider{Name: "a-some-ldap-idp"}). + WithOIDC(&oidctestutil.TestUpstreamOIDCIdentityProvider{Name: "a-some-oidc-idp"}). + WithLDAP(&oidctestutil.TestUpstreamLDAPIdentityProvider{Name: "z-some-ldap-idp"}). + WithLDAP(&oidctestutil.TestUpstreamLDAPIdentityProvider{Name: "x-some-idp"}). + Build() + + handler := NewHandler(idpLister) + req := httptest.NewRequest(test.method, test.path, nil) + rsp := httptest.NewRecorder() + handler.ServeHTTP(rsp, req) + + require.Equal(t, test.wantStatus, rsp.Code) + + require.Equal(t, test.wantContentType, rsp.Header().Get("Content-Type")) + + if test.wantFirstResponseBodyJSON != nil { + wantJSON, err := json.Marshal(test.wantFirstResponseBodyJSON) + require.NoError(t, err) + require.JSONEq(t, string(wantJSON), rsp.Body.String()) + } + + if test.wantBodyString != "" { + require.Equal(t, test.wantBodyString, rsp.Body.String()) + } + + // Change the list of IDPs in the cache. + idpLister.SetLDAPIdentityProviders([]provider.UpstreamLDAPIdentityProviderI{ + &oidctestutil.TestUpstreamLDAPIdentityProvider{Name: "some-other-ldap-idp-1"}, + &oidctestutil.TestUpstreamLDAPIdentityProvider{Name: "some-other-ldap-idp-2"}, + }) + idpLister.SetOIDCIdentityProviders([]provider.UpstreamOIDCIdentityProviderI{ + &oidctestutil.TestUpstreamOIDCIdentityProvider{Name: "some-other-oidc-idp-1"}, + &oidctestutil.TestUpstreamOIDCIdentityProvider{Name: "some-other-oidc-idp-2"}, + }) + + // Make the same request to the same handler instance again, and expect different results. + rsp = httptest.NewRecorder() + handler.ServeHTTP(rsp, req) + + require.Equal(t, test.wantStatus, rsp.Code) + + require.Equal(t, test.wantContentType, rsp.Header().Get("Content-Type")) + + if test.wantFirstResponseBodyJSON != nil { + wantJSON, err := json.Marshal(test.wantSecondResponseBodyJSON) + require.NoError(t, err) + require.JSONEq(t, string(wantJSON), rsp.Body.String()) + } + + if test.wantBodyString != "" { + require.Equal(t, test.wantBodyString, rsp.Body.String()) + } + }) + } +} diff --git a/internal/oidc/oidc.go b/internal/oidc/oidc.go index 0297b43c..82fe511d 100644 --- a/internal/oidc/oidc.go +++ b/internal/oidc/oidc.go @@ -24,6 +24,7 @@ const ( TokenEndpointPath = "/oauth2/token" //nolint:gosec // ignore lint warning that this is a credential CallbackEndpointPath = "/callback" JWKSEndpointPath = "/jwks.json" + PinnipedIDPsPath = "/pinniped_identity_providers" ) const ( diff --git a/internal/oidc/provider/manager/manager.go b/internal/oidc/provider/manager/manager.go index e17a4737..1d41e4ef 100644 --- a/internal/oidc/provider/manager/manager.go +++ b/internal/oidc/provider/manager/manager.go @@ -16,6 +16,7 @@ import ( "go.pinniped.dev/internal/oidc/csrftoken" "go.pinniped.dev/internal/oidc/discovery" "go.pinniped.dev/internal/oidc/dynamiccodec" + "go.pinniped.dev/internal/oidc/idpdiscovery" "go.pinniped.dev/internal/oidc/jwks" "go.pinniped.dev/internal/oidc/provider" "go.pinniped.dev/internal/oidc/token" @@ -102,10 +103,12 @@ func (m *Manager) SetProviders(federationDomains ...*provider.FederationDomainIs wrapGetter(incomingProvider.Issuer(), m.secretCache.GetStateEncoderBlockKey), ) - m.providerHandlers[(issuerHostWithPath + oidc.WellKnownEndpointPath)] = discovery.NewHandler(issuer, m.upstreamIDPs) + m.providerHandlers[(issuerHostWithPath + oidc.WellKnownEndpointPath)] = discovery.NewHandler(issuer) m.providerHandlers[(issuerHostWithPath + oidc.JWKSEndpointPath)] = jwks.NewHandler(issuer, m.dynamicJWKSProvider) + m.providerHandlers[(issuerHostWithPath + oidc.PinnipedIDPsPath)] = idpdiscovery.NewHandler(m.upstreamIDPs) + m.providerHandlers[(issuerHostWithPath + oidc.AuthorizationEndpointPath)] = auth.NewHandler( issuer, m.upstreamIDPs, diff --git a/internal/oidc/provider/manager/manager_test.go b/internal/oidc/provider/manager/manager_test.go index f42fe11c..c99fadff 100644 --- a/internal/oidc/provider/manager/manager_test.go +++ b/internal/oidc/provider/manager/manager_test.go @@ -7,6 +7,7 @@ import ( "context" "crypto/ecdsa" "encoding/json" + "fmt" "io/ioutil" "net/http" "net/http/httptest" @@ -70,7 +71,7 @@ func TestManager(t *testing.T) { return req } - requireDiscoveryRequestToBeHandled := func(requestIssuer, requestURLSuffix, expectedIssuer, expectedIDPName, expectedIDPType string) { + requireDiscoveryRequestToBeHandled := func(requestIssuer, requestURLSuffix, expectedIssuer string) { recorder := httptest.NewRecorder() subject.ServeHTTP(recorder, newGetRequest(requestIssuer+oidc.WellKnownEndpointPath+requestURLSuffix)) @@ -85,9 +86,24 @@ func TestManager(t *testing.T) { err = json.Unmarshal(responseBody, &parsedDiscoveryResult) r.NoError(err) r.Equal(expectedIssuer, parsedDiscoveryResult.Issuer) - r.Len(parsedDiscoveryResult.IDPs, 1) - r.Equal(expectedIDPName, parsedDiscoveryResult.IDPs[0].Name) - r.Equal(expectedIDPType, parsedDiscoveryResult.IDPs[0].Type) + r.Equal(parsedDiscoveryResult.PinnipedIDPsEndpoint, expectedIssuer+oidc.PinnipedIDPsPath) + } + + requirePinnipedIDPsDiscoveryRequestToBeHandled := func(requestIssuer, requestURLSuffix, expectedIDPName, expectedIDPType string) { + recorder := httptest.NewRecorder() + + subject.ServeHTTP(recorder, newGetRequest(requestIssuer+oidc.PinnipedIDPsPath+requestURLSuffix)) + + r.False(fallbackHandlerWasCalled) + + // Minimal check to ensure that the right IDP discovery endpoint was called + r.Equal(http.StatusOK, recorder.Code) + responseBody, err := ioutil.ReadAll(recorder.Body) + r.NoError(err) + r.Equal( + fmt.Sprintf(`{"pinniped_identity_providers":[{"name":"%s","type":"%s"}]}`+"\n", expectedIDPName, expectedIDPType), + string(responseBody), + ) } requireAuthorizationRequestToBeHandled := func(requestIssuer, requestURLSuffix, expectedRedirectLocationPrefix string) (string, string) { @@ -289,14 +305,23 @@ func TestManager(t *testing.T) { } requireRoutesMatchingRequestsToAppropriateProvider := func() { - requireDiscoveryRequestToBeHandled(issuer1, "", issuer1, upstreamIDPName, upstreamIDPType) - requireDiscoveryRequestToBeHandled(issuer2, "", issuer2, upstreamIDPName, upstreamIDPType) - requireDiscoveryRequestToBeHandled(issuer2, "?some=query", issuer2, upstreamIDPName, upstreamIDPType) + requireDiscoveryRequestToBeHandled(issuer1, "", issuer1) + requireDiscoveryRequestToBeHandled(issuer2, "", issuer2) + requireDiscoveryRequestToBeHandled(issuer2, "?some=query", issuer2) // Hostnames are case-insensitive, so test that we can handle that. - requireDiscoveryRequestToBeHandled(issuer1DifferentCaseHostname, "", issuer1, upstreamIDPName, upstreamIDPType) - requireDiscoveryRequestToBeHandled(issuer2DifferentCaseHostname, "", issuer2, upstreamIDPName, upstreamIDPType) - requireDiscoveryRequestToBeHandled(issuer2DifferentCaseHostname, "?some=query", issuer2, upstreamIDPName, upstreamIDPType) + requireDiscoveryRequestToBeHandled(issuer1DifferentCaseHostname, "", issuer1) + requireDiscoveryRequestToBeHandled(issuer2DifferentCaseHostname, "", issuer2) + requireDiscoveryRequestToBeHandled(issuer2DifferentCaseHostname, "?some=query", issuer2) + + requirePinnipedIDPsDiscoveryRequestToBeHandled(issuer1, "", upstreamIDPName, upstreamIDPType) + requirePinnipedIDPsDiscoveryRequestToBeHandled(issuer2, "", upstreamIDPName, upstreamIDPType) + requirePinnipedIDPsDiscoveryRequestToBeHandled(issuer2, "?some=query", upstreamIDPName, upstreamIDPType) + + // Hostnames are case-insensitive, so test that we can handle that. + requirePinnipedIDPsDiscoveryRequestToBeHandled(issuer1DifferentCaseHostname, "", upstreamIDPName, upstreamIDPType) + requirePinnipedIDPsDiscoveryRequestToBeHandled(issuer2DifferentCaseHostname, "", upstreamIDPName, upstreamIDPType) + requirePinnipedIDPsDiscoveryRequestToBeHandled(issuer2DifferentCaseHostname, "?some=query", upstreamIDPName, upstreamIDPType) issuer1JWKS := requireJWKSRequestToBeHandled(issuer1, "", issuer1KeyID) issuer2JWKS := requireJWKSRequestToBeHandled(issuer2, "", issuer2KeyID)