From d3d8ef44a0499b70df3d64b6621480f826b202a4 Mon Sep 17 00:00:00 2001 From: Matt Moyer Date: Fri, 13 Nov 2020 15:28:37 -0600 Subject: [PATCH 1/2] Make more fields in UpstreamOIDCProvider optional. Signed-off-by: Matt Moyer --- .../idp/v1alpha1/types_upstreamoidcprovider.go.tmpl | 7 ++++++- .../idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml | 7 ------- .../supervisor/idp/v1alpha1/types_upstreamoidcprovider.go | 7 ++++++- .../idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml | 7 ------- .../supervisor/idp/v1alpha1/types_upstreamoidcprovider.go | 7 ++++++- .../idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml | 7 ------- .../supervisor/idp/v1alpha1/types_upstreamoidcprovider.go | 7 ++++++- .../idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml | 7 ------- 8 files changed, 24 insertions(+), 32 deletions(-) diff --git a/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go.tmpl b/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go.tmpl index 7a16991b..cc8ca0fa 100644 --- a/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go.tmpl +++ b/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go.tmpl @@ -16,7 +16,7 @@ const ( // PhaseReady is the phase for an UpstreamOIDCProvider resource in a healthy state. PhaseReady UpstreamOIDCProviderPhase = "Ready" - // PhaseErorr is the phase for an UpstreamOIDCProvider in an unhealthy state. + // PhaseError is the phase for an UpstreamOIDCProvider in an unhealthy state. PhaseError UpstreamOIDCProviderPhase = "Error" ) @@ -40,6 +40,7 @@ type UpstreamOIDCProviderStatus struct { type OIDCAuthorizationConfig struct { // AdditionalScopes are the scopes in addition to "openid" that will be requested as part of the authorization // request flow with an OIDC identity provider. By default only the "openid" scope will be requested. + // +optional AdditionalScopes []string `json:"additionalScopes"` } @@ -47,10 +48,12 @@ type OIDCAuthorizationConfig struct { type OIDCClaims struct { // Groups provides the name of the token claim that will be used to ascertain the groups to which // an identity belongs. + // +optional Groups string `json:"groups"` // Username provides the name of the token claim that will be used to ascertain an identity's // username. + // +optional Username string `json:"username"` } @@ -74,10 +77,12 @@ type UpstreamOIDCProviderSpec struct { // AuthorizationConfig holds information about how to form the OAuth2 authorization request // parameters to be used with this OIDC identity provider. + // +optional AuthorizationConfig OIDCAuthorizationConfig `json:"authorizationConfig"` // Claims provides the names of token claims that will be used when inspecting an identity from // this OIDC identity provider. + // +optional Claims OIDCClaims `json:"claims"` // OIDCClient contains OIDC client information to be used used with this OIDC identity diff --git a/deploy/supervisor/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml b/deploy/supervisor/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml index e5234b5b..451a4474 100644 --- a/deploy/supervisor/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml +++ b/deploy/supervisor/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml @@ -64,8 +64,6 @@ spec: items: type: string type: array - required: - - additionalScopes type: object claims: description: Claims provides the names of token claims that will be @@ -79,9 +77,6 @@ spec: description: Username provides the name of the token claim that will be used to ascertain an identity's username. type: string - required: - - groups - - username type: object client: description: OIDCClient contains OIDC client information to be used @@ -104,8 +99,6 @@ spec: pattern: ^https:// type: string required: - - authorizationConfig - - claims - client - issuer type: object diff --git a/generated/1.17/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go b/generated/1.17/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go index 7a16991b..cc8ca0fa 100644 --- a/generated/1.17/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go +++ b/generated/1.17/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go @@ -16,7 +16,7 @@ const ( // PhaseReady is the phase for an UpstreamOIDCProvider resource in a healthy state. PhaseReady UpstreamOIDCProviderPhase = "Ready" - // PhaseErorr is the phase for an UpstreamOIDCProvider in an unhealthy state. + // PhaseError is the phase for an UpstreamOIDCProvider in an unhealthy state. PhaseError UpstreamOIDCProviderPhase = "Error" ) @@ -40,6 +40,7 @@ type UpstreamOIDCProviderStatus struct { type OIDCAuthorizationConfig struct { // AdditionalScopes are the scopes in addition to "openid" that will be requested as part of the authorization // request flow with an OIDC identity provider. By default only the "openid" scope will be requested. + // +optional AdditionalScopes []string `json:"additionalScopes"` } @@ -47,10 +48,12 @@ type OIDCAuthorizationConfig struct { type OIDCClaims struct { // Groups provides the name of the token claim that will be used to ascertain the groups to which // an identity belongs. + // +optional Groups string `json:"groups"` // Username provides the name of the token claim that will be used to ascertain an identity's // username. + // +optional Username string `json:"username"` } @@ -74,10 +77,12 @@ type UpstreamOIDCProviderSpec struct { // AuthorizationConfig holds information about how to form the OAuth2 authorization request // parameters to be used with this OIDC identity provider. + // +optional AuthorizationConfig OIDCAuthorizationConfig `json:"authorizationConfig"` // Claims provides the names of token claims that will be used when inspecting an identity from // this OIDC identity provider. + // +optional Claims OIDCClaims `json:"claims"` // OIDCClient contains OIDC client information to be used used with this OIDC identity diff --git a/generated/1.17/crds/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml b/generated/1.17/crds/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml index e5234b5b..451a4474 100644 --- a/generated/1.17/crds/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml +++ b/generated/1.17/crds/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml @@ -64,8 +64,6 @@ spec: items: type: string type: array - required: - - additionalScopes type: object claims: description: Claims provides the names of token claims that will be @@ -79,9 +77,6 @@ spec: description: Username provides the name of the token claim that will be used to ascertain an identity's username. type: string - required: - - groups - - username type: object client: description: OIDCClient contains OIDC client information to be used @@ -104,8 +99,6 @@ spec: pattern: ^https:// type: string required: - - authorizationConfig - - claims - client - issuer type: object diff --git a/generated/1.18/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go b/generated/1.18/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go index 7a16991b..cc8ca0fa 100644 --- a/generated/1.18/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go +++ b/generated/1.18/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go @@ -16,7 +16,7 @@ const ( // PhaseReady is the phase for an UpstreamOIDCProvider resource in a healthy state. PhaseReady UpstreamOIDCProviderPhase = "Ready" - // PhaseErorr is the phase for an UpstreamOIDCProvider in an unhealthy state. + // PhaseError is the phase for an UpstreamOIDCProvider in an unhealthy state. PhaseError UpstreamOIDCProviderPhase = "Error" ) @@ -40,6 +40,7 @@ type UpstreamOIDCProviderStatus struct { type OIDCAuthorizationConfig struct { // AdditionalScopes are the scopes in addition to "openid" that will be requested as part of the authorization // request flow with an OIDC identity provider. By default only the "openid" scope will be requested. + // +optional AdditionalScopes []string `json:"additionalScopes"` } @@ -47,10 +48,12 @@ type OIDCAuthorizationConfig struct { type OIDCClaims struct { // Groups provides the name of the token claim that will be used to ascertain the groups to which // an identity belongs. + // +optional Groups string `json:"groups"` // Username provides the name of the token claim that will be used to ascertain an identity's // username. + // +optional Username string `json:"username"` } @@ -74,10 +77,12 @@ type UpstreamOIDCProviderSpec struct { // AuthorizationConfig holds information about how to form the OAuth2 authorization request // parameters to be used with this OIDC identity provider. + // +optional AuthorizationConfig OIDCAuthorizationConfig `json:"authorizationConfig"` // Claims provides the names of token claims that will be used when inspecting an identity from // this OIDC identity provider. + // +optional Claims OIDCClaims `json:"claims"` // OIDCClient contains OIDC client information to be used used with this OIDC identity diff --git a/generated/1.18/crds/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml b/generated/1.18/crds/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml index e5234b5b..451a4474 100644 --- a/generated/1.18/crds/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml +++ b/generated/1.18/crds/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml @@ -64,8 +64,6 @@ spec: items: type: string type: array - required: - - additionalScopes type: object claims: description: Claims provides the names of token claims that will be @@ -79,9 +77,6 @@ spec: description: Username provides the name of the token claim that will be used to ascertain an identity's username. type: string - required: - - groups - - username type: object client: description: OIDCClient contains OIDC client information to be used @@ -104,8 +99,6 @@ spec: pattern: ^https:// type: string required: - - authorizationConfig - - claims - client - issuer type: object diff --git a/generated/1.19/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go b/generated/1.19/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go index 7a16991b..cc8ca0fa 100644 --- a/generated/1.19/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go +++ b/generated/1.19/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go @@ -16,7 +16,7 @@ const ( // PhaseReady is the phase for an UpstreamOIDCProvider resource in a healthy state. PhaseReady UpstreamOIDCProviderPhase = "Ready" - // PhaseErorr is the phase for an UpstreamOIDCProvider in an unhealthy state. + // PhaseError is the phase for an UpstreamOIDCProvider in an unhealthy state. PhaseError UpstreamOIDCProviderPhase = "Error" ) @@ -40,6 +40,7 @@ type UpstreamOIDCProviderStatus struct { type OIDCAuthorizationConfig struct { // AdditionalScopes are the scopes in addition to "openid" that will be requested as part of the authorization // request flow with an OIDC identity provider. By default only the "openid" scope will be requested. + // +optional AdditionalScopes []string `json:"additionalScopes"` } @@ -47,10 +48,12 @@ type OIDCAuthorizationConfig struct { type OIDCClaims struct { // Groups provides the name of the token claim that will be used to ascertain the groups to which // an identity belongs. + // +optional Groups string `json:"groups"` // Username provides the name of the token claim that will be used to ascertain an identity's // username. + // +optional Username string `json:"username"` } @@ -74,10 +77,12 @@ type UpstreamOIDCProviderSpec struct { // AuthorizationConfig holds information about how to form the OAuth2 authorization request // parameters to be used with this OIDC identity provider. + // +optional AuthorizationConfig OIDCAuthorizationConfig `json:"authorizationConfig"` // Claims provides the names of token claims that will be used when inspecting an identity from // this OIDC identity provider. + // +optional Claims OIDCClaims `json:"claims"` // OIDCClient contains OIDC client information to be used used with this OIDC identity diff --git a/generated/1.19/crds/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml b/generated/1.19/crds/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml index e5234b5b..451a4474 100644 --- a/generated/1.19/crds/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml +++ b/generated/1.19/crds/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml @@ -64,8 +64,6 @@ spec: items: type: string type: array - required: - - additionalScopes type: object claims: description: Claims provides the names of token claims that will be @@ -79,9 +77,6 @@ spec: description: Username provides the name of the token claim that will be used to ascertain an identity's username. type: string - required: - - groups - - username type: object client: description: OIDCClient contains OIDC client information to be used @@ -104,8 +99,6 @@ spec: pattern: ^https:// type: string required: - - authorizationConfig - - claims - client - issuer type: object From c10393b495ab6714e1f2c02cd425931c65f9815b Mon Sep 17 00:00:00 2001 From: Matt Moyer Date: Fri, 13 Nov 2020 15:29:32 -0600 Subject: [PATCH 2/2] Mask the raw error messages from go-oidc, since they are dangerous. Signed-off-by: Matt Moyer --- .../supervisorconfig/upstreamwatcher/upstreamwatcher.go | 4 +--- .../upstreamwatcher/upstreamwatcher_test.go | 6 +++--- test/integration/supervisor_upstream_test.go | 2 +- 3 files changed, 5 insertions(+), 7 deletions(-) diff --git a/internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher.go b/internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher.go index 3b96043e..144d2944 100644 --- a/internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher.go +++ b/internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher.go @@ -9,7 +9,6 @@ import ( "fmt" "net/url" "sort" - "strings" "time" "github.com/coreos/go-oidc" @@ -208,12 +207,11 @@ func (c *controller) validateIssuer(ctx context.Context, upstream *v1alpha1.Upst var err error discoveredProvider, err = oidc.NewProvider(ctx, upstream.Spec.Issuer) if err != nil { - err := fmt.Errorf("failed to perform OIDC discovery against %q: %w", upstream.Spec.Issuer, err) return &v1alpha1.Condition{ Type: typeOIDCDiscoverySucceeded, Status: v1alpha1.ConditionFalse, Reason: reasonUnreachable, - Message: strings.TrimSpace(err.Error()), + Message: fmt.Sprintf("failed to perform OIDC discovery against %q", upstream.Spec.Issuer), } } diff --git a/internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher_test.go b/internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher_test.go index 5ac79bff..2e91a523 100644 --- a/internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher_test.go +++ b/internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher_test.go @@ -208,8 +208,8 @@ func TestController(t *testing.T) { wantErr: controllerlib.ErrSyntheticRequeue.Error(), wantLogs: []string{ `upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, - `upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="failed to perform OIDC discovery against \"invalid-url\": Get \"invalid-url/.well-known/openid-configuration\": unsupported protocol scheme \"\"" "reason"="Unreachable" "status"="False" "type"="OIDCDiscoverySucceeded"`, - `upstream-observer "error"="UpstreamOIDCProvider has a failing condition" "msg"="found failing condition" "message"="failed to perform OIDC discovery against \"invalid-url\": Get \"invalid-url/.well-known/openid-configuration\": unsupported protocol scheme \"\"" "name"="test-name" "namespace"="test-namespace" "reason"="Unreachable" "type"="OIDCDiscoverySucceeded"`, + `upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="failed to perform OIDC discovery against \"invalid-url\"" "reason"="Unreachable" "status"="False" "type"="OIDCDiscoverySucceeded"`, + `upstream-observer "error"="UpstreamOIDCProvider has a failing condition" "msg"="found failing condition" "message"="failed to perform OIDC discovery against \"invalid-url\"" "name"="test-name" "namespace"="test-namespace" "reason"="Unreachable" "type"="OIDCDiscoverySucceeded"`, }, wantResultingCache: []provider.UpstreamOIDCIdentityProvider{}, wantResultingUpstreams: []v1alpha1.UpstreamOIDCProvider{{ @@ -229,7 +229,7 @@ func TestController(t *testing.T) { Status: "False", LastTransitionTime: now, Reason: "Unreachable", - Message: `failed to perform OIDC discovery against "invalid-url": Get "invalid-url/.well-known/openid-configuration": unsupported protocol scheme ""`, + Message: `failed to perform OIDC discovery against "invalid-url"`, }, }, }, diff --git a/test/integration/supervisor_upstream_test.go b/test/integration/supervisor_upstream_test.go index 389d2c27..40ef0e90 100644 --- a/test/integration/supervisor_upstream_test.go +++ b/test/integration/supervisor_upstream_test.go @@ -42,7 +42,7 @@ func TestSupervisorUpstreamOIDCDiscovery(t *testing.T) { Type: "OIDCDiscoverySucceeded", Status: v1alpha1.ConditionFalse, Reason: "Unreachable", - Message: `failed to perform OIDC discovery against "https://127.0.0.1:444444/issuer": Get "https://127.0.0.1:444444/issuer/.well-known/openid-configuration": dial tcp: address 444444: invalid port`, + Message: `failed to perform OIDC discovery against "https://127.0.0.1:444444/issuer"`, }, }) })