diff --git a/internal/concierge/server/server.go b/internal/concierge/server/server.go index 584a84b61..dbcafb0aa 100644 --- a/internal/concierge/server/server.go +++ b/internal/concierge/server/server.go @@ -174,17 +174,17 @@ func getAggregatedAPIServerConfig( startControllersPostStartHook func(context.Context), apiGroupSuffix string, ) (*apiserver.Config, error) { - apiGroup, ok := groupsuffix.Replace(loginv1alpha1.GroupName, apiGroupSuffix) + loginConciergeAPIGroup, ok := groupsuffix.Replace(loginv1alpha1.GroupName, apiGroupSuffix) if !ok { return nil, fmt.Errorf("cannot make api group from %s/%s", loginv1alpha1.GroupName, apiGroupSuffix) } - scheme := getAggregatedAPIServerScheme(apiGroup) + scheme := getAggregatedAPIServerScheme(loginConciergeAPIGroup, apiGroupSuffix) codecs := serializer.NewCodecFactory(scheme) - defaultEtcdPathPrefix := fmt.Sprintf("/registry/%s", apiGroup) + defaultEtcdPathPrefix := fmt.Sprintf("/registry/%s", loginConciergeAPIGroup) groupVersion := schema.GroupVersion{ - Group: apiGroup, + Group: loginConciergeAPIGroup, Version: loginv1alpha1.SchemeGroupVersion.Version, } @@ -224,7 +224,7 @@ func getAggregatedAPIServerConfig( return apiServerConfig, nil } -func getAggregatedAPIServerScheme(apiGroup string) *runtime.Scheme { +func getAggregatedAPIServerScheme(loginConciergeAPIGroup, apiGroupSuffix string) *runtime.Scheme { // standard set up of the server side scheme scheme := runtime.NewScheme() @@ -232,7 +232,7 @@ func getAggregatedAPIServerScheme(apiGroup string) *runtime.Scheme { metav1.AddToGroupVersion(scheme, metav1.Unversioned) // nothing fancy is required if using the standard group - if apiGroup == loginv1alpha1.GroupName { + if loginConciergeAPIGroup == loginv1alpha1.GroupName { utilruntime.Must(loginv1alpha1.AddToScheme(scheme)) utilruntime.Must(loginapi.AddToScheme(scheme)) return scheme @@ -257,7 +257,7 @@ func getAggregatedAPIServerScheme(apiGroup string) *runtime.Scheme { panic(err) // programmer error, scheme internal code is broken } newGVK := schema.GroupVersionKind{ - Group: apiGroup, + Group: loginConciergeAPIGroup, Version: gvk.Version, Kind: gvk.Kind, } @@ -270,5 +270,44 @@ func getAggregatedAPIServerScheme(apiGroup string) *runtime.Scheme { utilruntime.Must(loginv1alpha1.RegisterConversions(scheme)) utilruntime.Must(loginv1alpha1.RegisterDefaults(scheme)) + // we do not want to return errors from the scheme and instead would prefer to defer + // to the REST storage layer for consistency. The simplest way to do this is to force + // a cache miss from the authenticator cache. Kube API groups are validated via the + // IsDNS1123Subdomain func thus we can easily create a group that is guaranteed never + // to be in the authenticator cache. Add a timestamp just to be extra sure. + const authenticatorCacheMissPrefix = "_INVALID_API_GROUP_" + authenticatorCacheMiss := authenticatorCacheMissPrefix + time.Now().UTC().String() + + // we do not have any defaulting functions for *loginv1alpha1.TokenCredentialRequest + // today, but we may have some in the future. Calling AddTypeDefaultingFunc overwrites + // any previously registered defaulting function. Thus to make sure that we catch + // a situation where we add a defaulting func, we attempt to call it here with a nil + // *loginv1alpha1.TokenCredentialRequest. This will do nothing when there is no + // defaulting func registered, but it will almost certainly panic if one is added. + scheme.Default((*loginv1alpha1.TokenCredentialRequest)(nil)) + + // on incoming requests, restore the authenticator API group to the standard group + // note that we are responsible for duplicating this logic for every external API version + scheme.AddTypeDefaultingFunc(&loginv1alpha1.TokenCredentialRequest{}, func(obj interface{}) { + credentialRequest := obj.(*loginv1alpha1.TokenCredentialRequest) + + if credentialRequest.Spec.Authenticator.APIGroup == nil { + // force a cache miss because this is an invalid request + plog.Debug("invalid token credential request, nil group", "authenticator", credentialRequest.Spec.Authenticator) + credentialRequest.Spec.Authenticator.APIGroup = &authenticatorCacheMiss + return + } + + restoredGroup, ok := groupsuffix.Unreplace(*credentialRequest.Spec.Authenticator.APIGroup, apiGroupSuffix) + if !ok { + // force a cache miss because this is an invalid request + plog.Debug("invalid token credential request, wrong group", "authenticator", credentialRequest.Spec.Authenticator) + credentialRequest.Spec.Authenticator.APIGroup = &authenticatorCacheMiss + return + } + + credentialRequest.Spec.Authenticator.APIGroup = &restoredGroup + }) + return scheme } diff --git a/internal/concierge/server/server_test.go b/internal/concierge/server/server_test.go index c270837f7..258e914ef 100644 --- a/internal/concierge/server/server_test.go +++ b/internal/concierge/server/server_test.go @@ -13,12 +13,14 @@ import ( "github.com/google/go-cmp/cmp" "github.com/spf13/cobra" "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" loginapi "go.pinniped.dev/generated/1.20/apis/concierge/login" loginv1alpha1 "go.pinniped.dev/generated/1.20/apis/concierge/login/v1alpha1" + "go.pinniped.dev/internal/groupsuffix" ) const knownGoodUsage = ` @@ -124,13 +126,13 @@ func Test_getAggregatedAPIServerScheme(t *testing.T) { } tests := []struct { - name string - apiGroup string - want map[schema.GroupVersionKind]reflect.Type + name string + apiGroupSuffix string + want map[schema.GroupVersionKind]reflect.Type }{ { - name: "regular api group", - apiGroup: "login.concierge.pinniped.dev", + name: "regular api group", + apiGroupSuffix: "pinniped.dev", want: map[schema.GroupVersionKind]reflect.Type{ // all the types that are in the aggregated API group @@ -171,8 +173,8 @@ func Test_getAggregatedAPIServerScheme(t *testing.T) { }, }, { - name: "other api group", - apiGroup: "login.concierge.walrus.tld", + name: "other api group", + apiGroupSuffix: "walrus.tld", want: map[schema.GroupVersionKind]reflect.Type{ // all the types that are in the aggregated API group @@ -216,8 +218,46 @@ func Test_getAggregatedAPIServerScheme(t *testing.T) { for _, tt := range tests { tt := tt t.Run(tt.name, func(t *testing.T) { - scheme := getAggregatedAPIServerScheme(tt.apiGroup) + loginConciergeAPIGroup, ok := groupsuffix.Replace("login.concierge.pinniped.dev", tt.apiGroupSuffix) + require.True(t, ok) + + scheme := getAggregatedAPIServerScheme(loginConciergeAPIGroup, tt.apiGroupSuffix) require.Equal(t, tt.want, scheme.AllKnownTypes()) + + // make a credential request like a client would send + authenticationConciergeAPIGroup := "authentication.concierge." + tt.apiGroupSuffix + credentialRequest := &loginv1alpha1.TokenCredentialRequest{ + Spec: loginv1alpha1.TokenCredentialRequestSpec{ + Authenticator: corev1.TypedLocalObjectReference{ + APIGroup: &authenticationConciergeAPIGroup, + }, + }, + } + + // run defaulting on it + scheme.Default(credentialRequest) + + // make sure the group is restored if needed + require.Equal(t, "authentication.concierge.pinniped.dev", *credentialRequest.Spec.Authenticator.APIGroup) + + // make a credential request in the standard group + defaultAuthenticationConciergeAPIGroup := "authentication.concierge.pinniped.dev" + defaultCredentialRequest := &loginv1alpha1.TokenCredentialRequest{ + Spec: loginv1alpha1.TokenCredentialRequestSpec{ + Authenticator: corev1.TypedLocalObjectReference{ + APIGroup: &defaultAuthenticationConciergeAPIGroup, + }, + }, + } + + // run defaulting on it + scheme.Default(defaultCredentialRequest) + + if tt.apiGroupSuffix == "pinniped.dev" { // when using the standard group, this should just work + require.Equal(t, "authentication.concierge.pinniped.dev", *defaultCredentialRequest.Spec.Authenticator.APIGroup) + } else { // when using any other group, this should always be a cache miss + require.True(t, strings.HasPrefix(*defaultCredentialRequest.Spec.Authenticator.APIGroup, "_INVALID_API_GROUP_2")) + } }) } } diff --git a/internal/controller/authenticator/authncache/cache.go b/internal/controller/authenticator/authncache/cache.go index 87f317dff..2816d27c2 100644 --- a/internal/controller/authenticator/authncache/cache.go +++ b/internal/controller/authenticator/authncache/cache.go @@ -6,7 +6,6 @@ package authncache import ( "context" - "fmt" "sort" "sync" @@ -15,13 +14,12 @@ import ( "k8s.io/klog/v2" loginapi "go.pinniped.dev/generated/1.20/apis/concierge/login" + "go.pinniped.dev/internal/constable" "go.pinniped.dev/internal/plog" ) -var ( - // ErrNoSuchAuthenticator is returned by Cache.AuthenticateTokenCredentialRequest() when the requested authenticator is not configured. - ErrNoSuchAuthenticator = fmt.Errorf("no such authenticator") -) +// ErrNoSuchAuthenticator is returned by Cache.AuthenticateTokenCredentialRequest() when the requested authenticator is not configured. +const ErrNoSuchAuthenticator = constable.Error("no such authenticator") // Cache implements the authenticator.Token interface by multiplexing across a dynamic set of authenticators // loaded from authenticator resources.