mirrors/pinniped
1
0
Fork 0
mirror of https://github.com/vmware-tanzu/pinniped.git synced 2026-01-03 11:45:45 +00:00
Code Issues Actions Packages Projects Releases Wiki Activity

Improve JWTAuthenticator validation of Issuer,Discovery

Browse Source
This commit is contained in:
Benjamin A. Petersen
2024-03-13 17:18:57 -04:00
parent 0467e5c1d5
commit a45a537cdb
4 changed files with 325 additions and 30 deletions
Download Patch File Download Diff File Expand all files Collapse all files

65
internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go
View File

@@ -53,18 +53,21 @@ const (
typeJWKSFetchValid = "JWKSFetchValid"
typeAuthenticatorValid = "AuthenticatorValid"
reasonSuccess = "Success"
reasonNotReady = "NotReady"
reasonUnableToValidate = "UnableToValidate"
reasonInvalidIssuerURL = "InvalidIssuerURL"
reasonInvalidIssuerURLScheme = "InvalidIssuerURLScheme"
reasonInvalidProviderJWKSURL = "InvalidProviderJWKSURL"
reasonInvalidProviderJWKSURLScheme = "InvalidProviderJWKSURLScheme"
reasonInvalidTLSConfiguration = "InvalidTLSConfiguration"
reasonInvalidDiscoveryProbe = "InvalidDiscoveryProbe"
reasonInvalidAuthenticator = "InvalidAuthenticator"
reasonInvalidTokenSigningFailure = "InvalidTokenSigningFailure"
reasonInvalidCouldNotFetchJWKS = "InvalidCouldNotFetchJWKS"
reasonSuccess = "Success"
reasonNotReady = "NotReady"
reasonUnableToValidate = "UnableToValidate"
reasonInvalidIssuerURL = "InvalidIssuerURL"
reasonInvalidIssuerURLScheme = "InvalidIssuerURLScheme"
reasonInvalidIssuerURLFragment = "InvalidIssuerURLContainsFragment"
reasonInvalidIssuerURLQueryParams = "InvalidIssuerURLContainsQueryParams"
reasonInvalidIssuerURLContainsWellKnownEndpoint = "InvalidIssuerURLContainsWellKnownEndpoint"
reasonInvalidProviderJWKSURL = "InvalidProviderJWKSURL"
reasonInvalidProviderJWKSURLScheme = "InvalidProviderJWKSURLScheme"
reasonInvalidTLSConfiguration = "InvalidTLSConfiguration"
reasonInvalidDiscoveryProbe = "InvalidDiscoveryProbe"
reasonInvalidAuthenticator = "InvalidAuthenticator"
reasonInvalidTokenSigningFailure = "InvalidTokenSigningFailure"
reasonInvalidCouldNotFetchJWKS = "InvalidCouldNotFetchJWKS"
msgUnableToValidate = "unable to validate; see other conditions for details"
@@ -286,6 +289,39 @@ func (c *jwtCacheFillerController) validateIssuer(issuer string, conditions []*m
return nil, conditions, false
}
if strings.HasSuffix(issuerURL.Path, "/.well-known/openid-configuration") {
msg := fmt.Sprintf("spec.issuer %s cannot include path '/.well-known/openid-configuration'", issuer)
conditions = append(conditions, &metav1.Condition{
Type: typeIssuerURLValid,
Status: metav1.ConditionFalse,
Reason: reasonInvalidIssuerURLContainsWellKnownEndpoint,
Message: msg,
})
return nil, conditions, false
}
if len(issuerURL.Query()) != 0 {
msg := fmt.Sprintf("spec.issuer %s cannot include query params", issuer)
conditions = append(conditions, &metav1.Condition{
Type: typeIssuerURLValid,
Status: metav1.ConditionFalse,
Reason: reasonInvalidIssuerURLQueryParams,
Message: msg,
})
return nil, conditions, false
}
if issuerURL.Fragment != "" {
msg := fmt.Sprintf("spec.issuer %s cannot include fragment", issuer)
conditions = append(conditions, &metav1.Condition{
Type: typeIssuerURLValid,
Status: metav1.ConditionFalse,
Reason: reasonInvalidIssuerURLFragment,
Message: msg,
})
return nil, conditions, false
}
conditions = append(conditions, &metav1.Condition{
Type: typeIssuerURLValid,
Status: metav1.ConditionTrue,
@@ -305,11 +341,12 @@ func (c *jwtCacheFillerController) validateProviderDiscovery(ctx context.Context
})
return nil, nil, conditions, nil
}
provider, err := coreosoidc.NewProvider(ctx, issuer)
pJSON := &providerJSON{}
if err != nil {
errText := "could not perform oidc discovery on provider issuer"
msg := fmt.Sprintf("%s: %s", errText, err.Error())
msg := fmt.Sprintf("%s: %s", errText, pinnipedcontroller.TruncateMostLongErr(err))
conditions = append(conditions, &metav1.Condition{
Type: typeDiscoveryValid,
Status: metav1.ConditionFalse,
@@ -317,7 +354,7 @@ func (c *jwtCacheFillerController) validateProviderDiscovery(ctx context.Context
Message: msg,
})
// resync err, may be machine or other types of non-config error
return nil, nil, conditions, fmt.Errorf("%s: %w", errText, err)
return nil, nil, conditions, fmt.Errorf("%s: %s", errText, err)
}
msg := "discovery performed successfully"
conditions = append(conditions, &metav1.Condition{

249
internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go
View File

@@ -87,6 +87,20 @@ func TestController(t *testing.T) {
_, err := fmt.Fprintf(w, `{"issuer": "%s", "jwks_uri": "%s"}`, goodOIDCIssuerServer.URL, goodOIDCIssuerServer.URL+"/jwks.json")
require.NoError(t, err)
}))
goodMux.Handle("/path/to/not/found", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Content-Type", "application/json")
w.WriteHeader(http.StatusNotFound)
require.NoError(t, err)
}))
goodMux.Handle("/path/to/not/found/.well-known/openid-configuration", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Content-Type", "application/json")
w.WriteHeader(http.StatusNotFound)
_, err := fmt.Fprintf(w, `<html>
<head><title>%s</title></head>
<body>%s</body>
</html>`, "404 not found page", "lots of text that is at least 300 characters long 0123456789 abcdefghijklmnopqrstuvwxyz lots of text that is at least 300 characters long 0123456789 abcdefghijklmnopqrstuvwxyz lots of text that is at least 300 characters long 0123456789 abcdefghijklmnopqrstuvwxyz lots of text that is at least 300 characters long 0123456789 abcdefghijklmnopqrstuvwxyz lots of text that is at least 300 characters long 0123456789 abcdefghijklmnopqrstuvwxyz lots of text that is at least 300 characters long 0123456789 abcdefghijklmnopqrstuvwxyz lots of text that is at least 300 characters long 0123456789 abcdefghijklmnopqrstuvwxyz lots of text that is at least 300 characters long 0123456789 abcdefghijklmnopqrstuvwxyz should not reach end of string")
require.NoError(t, err)
}))
goodMux.Handle("/jwks.json", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
ecJWK := jose.JSONWebKey{
Key: goodECSigningKey,
@@ -360,6 +374,39 @@ func TestController(t *testing.T) {
}
}
sadIssuerURLValidInvalidFragment := func(issuer string, time metav1.Time, observedGeneration int64) metav1.Condition {
return metav1.Condition{
Type: "IssuerURLValid",
Status: "False",
ObservedGeneration: observedGeneration,
LastTransitionTime: time,
Reason: "InvalidIssuerURLContainsFragment",
Message: fmt.Sprintf("spec.issuer %s cannot include fragment", issuer),
}
}
sadIssuerURLValidInvalidQueryParams := func(issuer string, time metav1.Time, observedGeneration int64) metav1.Condition {
return metav1.Condition{
Type: "IssuerURLValid",
Status: "False",
ObservedGeneration: observedGeneration,
LastTransitionTime: time,
Reason: "InvalidIssuerURLContainsQueryParams",
Message: fmt.Sprintf("spec.issuer %s cannot include query params", issuer),
}
}
sadIssuerURLValidInvalidWellKnownEndpoint := func(issuer string, time metav1.Time, observedGeneration int64) metav1.Condition {
return metav1.Condition{
Type: "IssuerURLValid",
Status: "False",
ObservedGeneration: observedGeneration,
LastTransitionTime: time,
Reason: "InvalidIssuerURLContainsWellKnownEndpoint",
Message: fmt.Sprintf("spec.issuer %s cannot include path '/.well-known/openid-configuration'", issuer),
}
}
happyAuthenticatorValid := func(time metav1.Time, observedGeneration int64) metav1.Condition {
return metav1.Condition{
Type: "AuthenticatorValid",
@@ -428,6 +475,17 @@ func TestController(t *testing.T) {
}
}
sadDiscoveryURLValidExcessiveLongError := func(issuer string, time metav1.Time, observedGeneration int64) metav1.Condition {
return metav1.Condition{
Type: "DiscoveryURLValid",
Status: "False",
ObservedGeneration: observedGeneration,
LastTransitionTime: time,
Reason: "InvalidDiscoveryProbe",
Message: "could not perform oidc discovery on provider issuer: 404 Not Found: <html>\n\t\t \t<head><title>404 not found page</title></head>\n\t\t\t<body>lots of text that is at least 300 characters long 0123456789 abcdefghijklmnopqrstuvwxyz lots of text that is at least 300 characters long 0123456789 abcdefghijklmnopqrstuvwxyz lots of text that is at least 300 charact [truncated 534 chars]",
}
}
happyJWKSURLValid := func(time metav1.Time, observedGeneration int64) metav1.Condition {
return metav1.Condition{
Type: "JWKSURLValid",
@@ -1095,6 +1153,147 @@ func TestController(t *testing.T) {
updateStatusAction,
}
},
}, {
name: "validateIssuer: issuer cannot include fragment: loop will fail sync, will write failed and unknown conditions, but will not enqueue a resync due to user config error",
jwtAuthenticators: []runtime.Object{
&auth1alpha1.JWTAuthenticator{
ObjectMeta: metav1.ObjectMeta{
Name: "test-name",
},
Spec: auth1alpha1.JWTAuthenticatorSpec{
Issuer: "https://www.example.com/foo/bar/#do-not-include-fragment",
Audience: goodAudience,
TLS: conciergetestutil.TlsSpecFromTLSConfig(goodOIDCIssuerServer.TLS),
},
},
},
syncKey: controllerlib.Key{Name: "test-name"},
wantActions: func() []coretesting.Action {
updateStatusAction := coretesting.NewUpdateAction(jwtAuthenticatorsGVR, "", &auth1alpha1.JWTAuthenticator{
ObjectMeta: metav1.ObjectMeta{
Name: "test-name",
},
Spec: auth1alpha1.JWTAuthenticatorSpec{
Issuer: "https://www.example.com/foo/bar/#do-not-include-fragment",
Audience: goodAudience,
TLS: conciergetestutil.TlsSpecFromTLSConfig(goodOIDCIssuerServer.TLS),
},
Status: auth1alpha1.JWTAuthenticatorStatus{
Conditions: conditionstestutil.Replace(
allHappyConditionsSuccess(goodIssuer, frozenMetav1Now, 0),
[]metav1.Condition{
sadReadyCondition(frozenMetav1Now, 0),
sadIssuerURLValidInvalidFragment("https://www.example.com/foo/bar/#do-not-include-fragment", frozenMetav1Now, 0),
unknownDiscoveryURLValid(frozenMetav1Now, 0),
unknownAuthenticatorValid(frozenMetav1Now, 0),
unknownJWKSURLValid(frozenMetav1Now, 0),
unknownJWKSFetch(frozenMetav1Now, 0),
},
),
Phase: "Error",
},
})
updateStatusAction.Subresource = "status"
return []coretesting.Action{
coretesting.NewListAction(jwtAuthenticatorsGVR, jwtAUthenticatorGVK, "", metav1.ListOptions{}),
coretesting.NewWatchAction(jwtAuthenticatorsGVR, "", metav1.ListOptions{}),
updateStatusAction,
}
},
}, {
name: "validateIssuer: issuer cannot include query params: loop will fail sync, will write failed and unknown conditions, but will not enqueue a resync due to user config error",
jwtAuthenticators: []runtime.Object{
&auth1alpha1.JWTAuthenticator{
ObjectMeta: metav1.ObjectMeta{
Name: "test-name",
},
Spec: auth1alpha1.JWTAuthenticatorSpec{
Issuer: "https://www.example.com/foo/bar/?query-params=not-allowed",
Audience: goodAudience,
TLS: conciergetestutil.TlsSpecFromTLSConfig(goodOIDCIssuerServer.TLS),
},
},
},
syncKey: controllerlib.Key{Name: "test-name"},
wantActions: func() []coretesting.Action {
updateStatusAction := coretesting.NewUpdateAction(jwtAuthenticatorsGVR, "", &auth1alpha1.JWTAuthenticator{
ObjectMeta: metav1.ObjectMeta{
Name: "test-name",
},
Spec: auth1alpha1.JWTAuthenticatorSpec{
Issuer: "https://www.example.com/foo/bar/?query-params=not-allowed",
Audience: goodAudience,
TLS: conciergetestutil.TlsSpecFromTLSConfig(goodOIDCIssuerServer.TLS),
},
Status: auth1alpha1.JWTAuthenticatorStatus{
Conditions: conditionstestutil.Replace(
allHappyConditionsSuccess(goodIssuer, frozenMetav1Now, 0),
[]metav1.Condition{
sadReadyCondition(frozenMetav1Now, 0),
sadIssuerURLValidInvalidQueryParams("https://www.example.com/foo/bar/?query-params=not-allowed", frozenMetav1Now, 0),
unknownDiscoveryURLValid(frozenMetav1Now, 0),
unknownAuthenticatorValid(frozenMetav1Now, 0),
unknownJWKSURLValid(frozenMetav1Now, 0),
unknownJWKSFetch(frozenMetav1Now, 0),
},
),
Phase: "Error",
},
})
updateStatusAction.Subresource = "status"
return []coretesting.Action{
coretesting.NewListAction(jwtAuthenticatorsGVR, jwtAUthenticatorGVK, "", metav1.ListOptions{}),
coretesting.NewWatchAction(jwtAuthenticatorsGVR, "", metav1.ListOptions{}),
updateStatusAction,
}
},
}, {
name: "validateIssuer: issuer cannot include .well-known in path: loop will fail sync, will write failed and unknown conditions, but will not enqueue a resync due to user config error",
jwtAuthenticators: []runtime.Object{
&auth1alpha1.JWTAuthenticator{
ObjectMeta: metav1.ObjectMeta{
Name: "test-name",
},
Spec: auth1alpha1.JWTAuthenticatorSpec{
Issuer: "https://www.example.com/foo/bar/.well-known/openid-configuration",
Audience: goodAudience,
TLS: conciergetestutil.TlsSpecFromTLSConfig(goodOIDCIssuerServer.TLS),
},
},
},
syncKey: controllerlib.Key{Name: "test-name"},
wantActions: func() []coretesting.Action {
updateStatusAction := coretesting.NewUpdateAction(jwtAuthenticatorsGVR, "", &auth1alpha1.JWTAuthenticator{
ObjectMeta: metav1.ObjectMeta{
Name: "test-name",
},
Spec: auth1alpha1.JWTAuthenticatorSpec{
Issuer: "https://www.example.com/foo/bar/.well-known/openid-configuration",
Audience: goodAudience,
TLS: conciergetestutil.TlsSpecFromTLSConfig(goodOIDCIssuerServer.TLS),
},
Status: auth1alpha1.JWTAuthenticatorStatus{
Conditions: conditionstestutil.Replace(
allHappyConditionsSuccess(goodIssuer, frozenMetav1Now, 0),
[]metav1.Condition{
sadReadyCondition(frozenMetav1Now, 0),
sadIssuerURLValidInvalidWellKnownEndpoint("https://www.example.com/foo/bar/.well-known/openid-configuration", frozenMetav1Now, 0),
unknownDiscoveryURLValid(frozenMetav1Now, 0),
unknownAuthenticatorValid(frozenMetav1Now, 0),
unknownJWKSURLValid(frozenMetav1Now, 0),
unknownJWKSFetch(frozenMetav1Now, 0),
},
),
Phase: "Error",
},
})
updateStatusAction.Subresource = "status"
return []coretesting.Action{
coretesting.NewListAction(jwtAuthenticatorsGVR, jwtAUthenticatorGVK, "", metav1.ListOptions{}),
coretesting.NewWatchAction(jwtAuthenticatorsGVR, "", metav1.ListOptions{}),
updateStatusAction,
}
},
}, {
name: "validateProviderDiscovery: could not perform oidc discovery on provider issuer: loop will fail sync, will write failed and unknown conditions, and will enqueue new sync",
jwtAuthenticators: []runtime.Object{
@@ -1136,6 +1335,56 @@ func TestController(t *testing.T) {
}
},
wantSyncLoopErr: testutil.WantExactErrorString(`could not perform oidc discovery on provider issuer: Get "` + goodIssuer + `/foo/bar/baz/shizzle/.well-known/openid-configuration": tls: failed to verify certificate: x509: certificate signed by unknown authority`),
}, {
name: "validateProviderDiscovery: excessively long errors truncated: loop will fail sync, will write failed and unknown conditions, and will enqueue new sync",
jwtAuthenticators: []runtime.Object{
&auth1alpha1.JWTAuthenticator{
ObjectMeta: metav1.ObjectMeta{
Name: "test-name",
},
Spec: auth1alpha1.JWTAuthenticatorSpec{
Issuer: goodIssuer + "/path/to/not/found",
Audience: goodAudience,
TLS: conciergetestutil.TlsSpecFromTLSConfig(goodOIDCIssuerServer.TLS),
},
},
},
syncKey: controllerlib.Key{Name: "test-name"},
wantActions: func() []coretesting.Action {
updateStatusAction := coretesting.NewUpdateAction(jwtAuthenticatorsGVR, "", &auth1alpha1.JWTAuthenticator{
ObjectMeta: metav1.ObjectMeta{
Name: "test-name",
},
Spec: auth1alpha1.JWTAuthenticatorSpec{
Issuer: goodIssuer + "/path/to/not/found",
Audience: goodAudience,
TLS: conciergetestutil.TlsSpecFromTLSConfig(goodOIDCIssuerServer.TLS),
},
Status: auth1alpha1.JWTAuthenticatorStatus{
Conditions: conditionstestutil.Replace(
allHappyConditionsSuccess(goodIssuer, frozenMetav1Now, 0),
[]metav1.Condition{
happyIssuerURLValid(frozenMetav1Now, 0),
sadReadyCondition(frozenMetav1Now, 0),
sadDiscoveryURLValidExcessiveLongError(goodIssuer+"/path/to/not/found", frozenMetav1Now, 0),
unknownAuthenticatorValid(frozenMetav1Now, 0),
unknownJWKSURLValid(frozenMetav1Now, 0),
unknownJWKSFetch(frozenMetav1Now, 0),
happyTLSConfigurationValidCAParsed(frozenMetav1Now, 0),
},
),
Phase: "Error",
},
})
updateStatusAction.Subresource = "status"
return []coretesting.Action{
coretesting.NewListAction(jwtAuthenticatorsGVR, jwtAUthenticatorGVK, "", metav1.ListOptions{}),
coretesting.NewWatchAction(jwtAuthenticatorsGVR, "", metav1.ListOptions{}),
updateStatusAction,
}
},
// not currently truncating the logged err
wantSyncLoopErr: testutil.WantExactErrorString("could not perform oidc discovery on provider issuer: 404 Not Found: <html>\n\t\t \t<head><title>404 not found page</title></head>\n\t\t\t<body>lots of text that is at least 300 characters long 0123456789 abcdefghijklmnopqrstuvwxyz lots of text that is at least 300 characters long 0123456789 abcdefghijklmnopqrstuvwxyz lots of text that is at least 300 characters long 0123456789 abcdefghijklmnopqrstuvwxyz lots of text that is at least 300 characters long 0123456789 abcdefghijklmnopqrstuvwxyz lots of text that is at least 300 characters long 0123456789 abcdefghijklmnopqrstuvwxyz lots of text that is at least 300 characters long 0123456789 abcdefghijklmnopqrstuvwxyz lots of text that is at least 300 characters long 0123456789 abcdefghijklmnopqrstuvwxyz lots of text that is at least 300 characters long 0123456789 abcdefghijklmnopqrstuvwxyz should not reach end of string</body>\n\t\t</html>"),
},
// cannot be tested currently the way the coreos lib works.
// the constructor requires an issuer in the payload and validates the issuer matches the actual issuer,

20
internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go
View File

@@ -1,4 +1,4 @@
// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved.
// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0
// Package oidcupstreamwatcher implements a controller which watches OIDCIdentityProviders.
@@ -342,7 +342,7 @@ func (c *oidcWatcherController) validateIssuer(ctx context.Context, upstream *v1
Type: typeOIDCDiscoverySucceeded,
Status: metav1.ConditionFalse,
Reason: reasonUnreachable,
Message: fmt.Sprintf("failed to perform OIDC discovery against %q:\n%s", upstream.Spec.Issuer, truncateMostLongErr(err)),
Message: fmt.Sprintf("failed to perform OIDC discovery against %q:\n%s", upstream.Spec.Issuer, pinnipedcontroller.TruncateMostLongErr(err)),
}
}
@@ -361,7 +361,7 @@ func (c *oidcWatcherController) validateIssuer(ctx context.Context, upstream *v1
Type: typeOIDCDiscoverySucceeded,
Status: metav1.ConditionFalse,
Reason: reasonInvalidResponse,
Message: fmt.Sprintf("failed to unmarshal OIDC discovery response from %q:\n%s", upstream.Spec.Issuer, truncateMostLongErr(err)),
Message: fmt.Sprintf("failed to unmarshal OIDC discovery response from %q:\n%s", upstream.Spec.Issuer, pinnipedcontroller.TruncateMostLongErr(err)),
}
}
if additionalDiscoveryClaims.RevocationEndpoint != "" {
@@ -473,18 +473,6 @@ func computeScopes(additionalScopes []string) []string {
return set.List()
}
func truncateMostLongErr(err error) string {
const max = 300
msg := err.Error()
// always log oidc and x509 errors completely
if len(msg) <= max || strings.Contains(msg, "oidc:") || strings.Contains(msg, "x509:") {
return msg
}
return msg[:max] + fmt.Sprintf(" [truncated %d chars]", len(msg)-max)
}
func validateHTTPSURL(maybeHTTPSURL, endpointType, reason string) (*url.URL, *metav1.Condition) {
parsedURL, err := url.Parse(maybeHTTPSURL)
if err != nil {
@@ -492,7 +480,7 @@ func validateHTTPSURL(maybeHTTPSURL, endpointType, reason string) (*url.URL, *me
Type: typeOIDCDiscoverySucceeded,
Status: metav1.ConditionFalse,
Reason: reason,
Message: fmt.Sprintf("failed to parse %s URL: %v", endpointType, truncateMostLongErr(err)),
Message: fmt.Sprintf("failed to parse %s URL: %v", endpointType, pinnipedcontroller.TruncateMostLongErr(err)),
}
}
if parsedURL.Scheme != "https" {

21
internal/controller/utilserr.go Normal file
View File

@@ -0,0 +1,21 @@
// Copyright 2024 the Pinniped contributors. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0
package controller
import (
"fmt"
"strings"
)
func TruncateMostLongErr(err error) string {
const max = 300
msg := err.Error()
// always log oidc and x509 errors completely
if len(msg) <= max || strings.Contains(msg, "oidc:") || strings.Contains(msg, "x509:") {
return msg
}
return msg[:max] + fmt.Sprintf(" [truncated %d chars]", len(msg)-max)
}