From 32063db46e75fd865ad2083b16e18dcede0e2168 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Wed, 12 Jul 2023 10:34:15 -0700 Subject: [PATCH] Validate apiGroup names are valid in federation_domain_watcher.go --- .../federation_domain_watcher.go | 91 ++++++++++----- .../federation_domain_watcher_test.go | 106 +++++++++++++++++- internal/supervisor/server/server.go | 1 + 3 files changed, 170 insertions(+), 28 deletions(-) diff --git a/internal/controller/supervisorconfig/federation_domain_watcher.go b/internal/controller/supervisorconfig/federation_domain_watcher.go index 44d3f9fc9..91780b92a 100644 --- a/internal/controller/supervisorconfig/federation_domain_watcher.go +++ b/internal/controller/supervisorconfig/federation_domain_watcher.go @@ -41,6 +41,7 @@ const ( typeIssuerIsUnique = "IssuerIsUnique" typeIdentityProvidersFound = "IdentityProvidersFound" typeDisplayNamesUnique = "DisplayNamesUnique" + typeAPIGroupSuffixValid = "APIGroupSuffixValid" reasonSuccess = "Success" reasonNotReady = "NotReady" @@ -53,6 +54,7 @@ const ( reasonIdentityProvidersObjectRefsNotFound = "IdentityProvidersObjectRefsNotFound" reasonIdentityProviderNotSpecified = "IdentityProviderNotSpecified" reasonDuplicateDisplayNames = "DuplicateDisplayNames" + reasonAPIGroupNameUnrecognized = "APIGroupNameUnrecognized" celTransformerMaxExpressionRuntime = 5 * time.Second ) @@ -66,6 +68,7 @@ type FederationDomainsSetter interface { type federationDomainWatcherController struct { federationDomainsSetter FederationDomainsSetter + apiGroup string clock clock.Clock client pinnipedclientset.Interface @@ -81,6 +84,7 @@ type federationDomainWatcherController struct { // FederationDomain objects and notifies a callback object of the collection of provider configs. func NewFederationDomainWatcherController( federationDomainsSetter FederationDomainsSetter, + apiGroupSuffix string, clock clock.Clock, client pinnipedclientset.Interface, federationDomainInformer configinformers.FederationDomainInformer, @@ -94,6 +98,7 @@ func NewFederationDomainWatcherController( Name: "FederationDomainWatcherController", Syncer: &federationDomainWatcherController{ federationDomainsSetter: federationDomainsSetter, + apiGroup: fmt.Sprintf("idp.supervisor.%s", apiGroupSuffix), clock: clock, client: client, federationDomainInformer: federationDomainInformer, @@ -297,12 +302,13 @@ func (c *federationDomainWatcherController) makeLegacyFederationDomainIssuer( }) } - conditions = c.addDuplicateDisplayNamesCondition(sets.Set[string]{}, conditions) - // This is the constructor for the backwards compatibility mode. federationDomainIssuer, err := federationdomainproviders.NewFederationDomainIssuerWithDefaultIDP(federationDomain.Spec.Issuer, defaultFederationDomainIdentityProvider) conditions = appendIssuerURLValidCondition(err, conditions) + conditions = appendDuplicateDisplayNamesCondition(sets.Set[string]{}, conditions) + conditions = appendAPIGroupSuffixCondition(c.apiGroup, sets.Set[string]{}, conditions) + return federationDomainIssuer, conditions, nil } @@ -314,6 +320,7 @@ func (c *federationDomainWatcherController) makeFederationDomainIssuerWithExplic idpNotFoundIndices := []int{} displayNames := sets.Set[string]{} duplicateDisplayNames := sets.Set[string]{} + badAPIGroupNames := sets.Set[string]{} for index, idp := range federationDomain.Spec.IdentityProviders { if displayNames.Has(idp.DisplayName) { @@ -321,7 +328,13 @@ func (c *federationDomainWatcherController) makeFederationDomainIssuerWithExplic } displayNames.Insert(idp.DisplayName) - // TODO: Validate that idp.ObjectRef.APIGroup is the expected APIGroup for IDP CRs "idp.supervisor.pinniped.dev" where .pinniped.dev is the configurable suffix + apiGroup := "nil" + if idp.ObjectRef.APIGroup != nil { + apiGroup = *idp.ObjectRef.APIGroup + } + if apiGroup != c.apiGroup { + badAPIGroupNames.Insert(apiGroup) + } // Validate that each objectRef resolves to an existing IDP. It does not matter if the IDP itself // is phase=Ready, because it will not be loaded into the cache if not ready. For each objectRef @@ -374,34 +387,14 @@ func (c *federationDomainWatcherController) makeFederationDomainIssuerWithExplic }) } - conditions = c.addDuplicateDisplayNamesCondition(duplicateDisplayNames, conditions) - // This is the constructor for any case other than the legacy case, including when there is an empty list of IDPs. federationDomainIssuer, err := federationdomainproviders.NewFederationDomainIssuer(federationDomain.Spec.Issuer, federationDomainIdentityProviders) conditions = appendIssuerURLValidCondition(err, conditions) - return federationDomainIssuer, conditions, nil -} -func (c *federationDomainWatcherController) addDuplicateDisplayNamesCondition(duplicateDisplayNames sets.Set[string], conditions []*configv1alpha1.Condition) []*configv1alpha1.Condition { - if duplicateDisplayNames.Len() > 0 { - duplicates := duplicateDisplayNames.UnsortedList() - sort.Strings(duplicates) - conditions = append(conditions, &configv1alpha1.Condition{ - Type: typeDisplayNamesUnique, - Status: configv1alpha1.ConditionFalse, - Reason: reasonDuplicateDisplayNames, - Message: fmt.Sprintf("the names specified by .spec.identityProviders[].displayName contain duplicates: %s", - strings.Join(duplicates, ", ")), - }) - } else { - conditions = append(conditions, &configv1alpha1.Condition{ - Type: typeDisplayNamesUnique, - Status: configv1alpha1.ConditionTrue, - Reason: reasonSuccess, - Message: "the names specified by .spec.identityProviders[].displayName are unique", - }) - } - return conditions + conditions = appendDuplicateDisplayNamesCondition(duplicateDisplayNames, conditions) + conditions = appendAPIGroupSuffixCondition(c.apiGroup, badAPIGroupNames, conditions) + + return federationDomainIssuer, conditions, nil } func (c *federationDomainWatcherController) findIDPsUIDByObjectRef(objectRef corev1.TypedLocalObjectReference, namespace string) (types.UID, bool, error) { @@ -562,6 +555,50 @@ func (c *federationDomainWatcherController) makeTransformationPipelineForIdentit return pipeline, nil } +func appendAPIGroupSuffixCondition(expectedSuffixName string, badSuffixNames sets.Set[string], conditions []*configv1alpha1.Condition) []*configv1alpha1.Condition { + if badSuffixNames.Len() > 0 { + badNames := badSuffixNames.UnsortedList() + sort.Strings(badNames) + conditions = append(conditions, &configv1alpha1.Condition{ + Type: typeAPIGroupSuffixValid, + Status: configv1alpha1.ConditionFalse, + Reason: reasonAPIGroupNameUnrecognized, + Message: fmt.Sprintf("the API groups specified by .spec.identityProviders[].objectRef.apiGroup are not recognized (should be %q): %s", + expectedSuffixName, strings.Join(badNames, ", ")), + }) + } else { + conditions = append(conditions, &configv1alpha1.Condition{ + Type: typeAPIGroupSuffixValid, + Status: configv1alpha1.ConditionTrue, + Reason: reasonSuccess, + Message: "the API groups specified by .spec.identityProviders[].objectRef.apiGroup are recognized", + }) + } + return conditions +} + +func appendDuplicateDisplayNamesCondition(duplicateDisplayNames sets.Set[string], conditions []*configv1alpha1.Condition) []*configv1alpha1.Condition { + if duplicateDisplayNames.Len() > 0 { + duplicates := duplicateDisplayNames.UnsortedList() + sort.Strings(duplicates) + conditions = append(conditions, &configv1alpha1.Condition{ + Type: typeDisplayNamesUnique, + Status: configv1alpha1.ConditionFalse, + Reason: reasonDuplicateDisplayNames, + Message: fmt.Sprintf("the names specified by .spec.identityProviders[].displayName contain duplicates: %s", + strings.Join(duplicates, ", ")), + }) + } else { + conditions = append(conditions, &configv1alpha1.Condition{ + Type: typeDisplayNamesUnique, + Status: configv1alpha1.ConditionTrue, + Reason: reasonSuccess, + Message: "the names specified by .spec.identityProviders[].displayName are unique", + }) + } + return conditions +} + func appendIssuerURLValidCondition(err error, conditions []*configv1alpha1.Condition) []*configv1alpha1.Condition { if err != nil { // Note that the FederationDomainIssuer constructors only validate the Issuer URL, diff --git a/internal/controller/supervisorconfig/federation_domain_watcher_test.go b/internal/controller/supervisorconfig/federation_domain_watcher_test.go index c460f58d1..10884cc78 100644 --- a/internal/controller/supervisorconfig/federation_domain_watcher_test.go +++ b/internal/controller/supervisorconfig/federation_domain_watcher_test.go @@ -89,6 +89,7 @@ func TestFederationDomainWatcherControllerInformerFilters(t *testing.T) { NewFederationDomainWatcherController( nil, + "", nil, nil, federationDomainInformer, @@ -128,7 +129,8 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { t.Parallel() const namespace = "some-namespace" - const apiGroupSupervisor = "idp.supervisor.pinniped.dev" + const apiGroupSuffix = "custom.suffix.pinniped.dev" + const apiGroupSupervisor = "idp.supervisor." + apiGroupSuffix frozenNow := time.Date(2020, time.September, 23, 7, 42, 0, 0, time.Local) frozenMetav1Now := metav1.NewTime(frozenNow) @@ -395,9 +397,32 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { } } + happyAPIGroupSuffixCondition := func(time metav1.Time, observedGeneration int64) configv1alpha1.Condition { + return configv1alpha1.Condition{ + Type: "APIGroupSuffixValid", + Status: "True", + ObservedGeneration: observedGeneration, + LastTransitionTime: time, + Reason: "Success", + Message: "the API groups specified by .spec.identityProviders[].objectRef.apiGroup are recognized", + } + } + + sadAPIGroupSuffixCondition := func(badNames string, time metav1.Time, observedGeneration int64) configv1alpha1.Condition { + return configv1alpha1.Condition{ + Type: "APIGroupSuffixValid", + Status: "False", + ObservedGeneration: observedGeneration, + LastTransitionTime: time, + Reason: "APIGroupNameUnrecognized", + Message: fmt.Sprintf("the API groups specified by .spec.identityProviders[].objectRef.apiGroup are not recognized (should be \"idp.supervisor.%s\"): %s", apiGroupSuffix, badNames), + } + } + allHappyConditionsLegacyConfigurationSuccess := func(issuer string, idpName string, time metav1.Time, observedGeneration int64) []configv1alpha1.Condition { return []configv1alpha1.Condition{ // expect them to be sorted alphabetically by type + happyAPIGroupSuffixCondition(frozenMetav1Now, 123), happyDisplayNamesUniqueCondition(frozenMetav1Now, 123), happyIdentityProvidersFoundConditionLegacyConfigurationSuccess(idpName, time, observedGeneration), happyIssuerIsUniqueCondition(time, observedGeneration), @@ -410,6 +435,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { allHappyConditionsSuccess := func(issuer string, time metav1.Time, observedGeneration int64) []configv1alpha1.Condition { return []configv1alpha1.Condition{ // expect them to be sorted alphabetically by type + happyAPIGroupSuffixCondition(frozenMetav1Now, 123), happyDisplayNamesUniqueCondition(frozenMetav1Now, 123), happyIdentityProvidersFoundConditionSuccess(frozenMetav1Now, 123), happyIssuerIsUniqueCondition(frozenMetav1Now, 123), @@ -539,6 +565,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { expectedFederationDomainStatusUpdate(invalidIssuerURLFederationDomain, configv1alpha1.FederationDomainPhaseError, []configv1alpha1.Condition{ + happyAPIGroupSuffixCondition(frozenMetav1Now, 123), happyDisplayNamesUniqueCondition(frozenMetav1Now, 123), happyIdentityProvidersFoundConditionLegacyConfigurationSuccess(oidcIdentityProvider.Name, frozenMetav1Now, 123), happyIssuerIsUniqueCondition(frozenMetav1Now, 123), @@ -583,6 +610,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { expectedFederationDomainStatusUpdate(invalidIssuerURLFederationDomain, configv1alpha1.FederationDomainPhaseError, []configv1alpha1.Condition{ + happyAPIGroupSuffixCondition(frozenMetav1Now, 123), happyDisplayNamesUniqueCondition(frozenMetav1Now, 123), happyIdentityProvidersFoundConditionLegacyConfigurationSuccess(oidcIdentityProvider.Name, frozenMetav1Now, 123), happyIssuerIsUniqueCondition(frozenMetav1Now, 123), @@ -625,6 +653,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { }, configv1alpha1.FederationDomainPhaseError, []configv1alpha1.Condition{ + happyAPIGroupSuffixCondition(frozenMetav1Now, 123), happyDisplayNamesUniqueCondition(frozenMetav1Now, 123), happyIdentityProvidersFoundConditionLegacyConfigurationSuccess(oidcIdentityProvider.Name, frozenMetav1Now, 123), sadIssuerIsUniqueCondition(frozenMetav1Now, 123), @@ -639,6 +668,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { }, configv1alpha1.FederationDomainPhaseError, []configv1alpha1.Condition{ + happyAPIGroupSuffixCondition(frozenMetav1Now, 123), happyDisplayNamesUniqueCondition(frozenMetav1Now, 123), happyIdentityProvidersFoundConditionLegacyConfigurationSuccess(oidcIdentityProvider.Name, frozenMetav1Now, 123), sadIssuerIsUniqueCondition(frozenMetav1Now, 123), @@ -702,6 +732,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { }, configv1alpha1.FederationDomainPhaseError, []configv1alpha1.Condition{ + happyAPIGroupSuffixCondition(frozenMetav1Now, 123), happyDisplayNamesUniqueCondition(frozenMetav1Now, 123), happyIdentityProvidersFoundConditionLegacyConfigurationSuccess(oidcIdentityProvider.Name, frozenMetav1Now, 123), happyIssuerIsUniqueCondition(frozenMetav1Now, 123), @@ -716,6 +747,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { }, configv1alpha1.FederationDomainPhaseError, []configv1alpha1.Condition{ + happyAPIGroupSuffixCondition(frozenMetav1Now, 123), happyDisplayNamesUniqueCondition(frozenMetav1Now, 123), happyIdentityProvidersFoundConditionLegacyConfigurationSuccess(oidcIdentityProvider.Name, frozenMetav1Now, 123), happyIssuerIsUniqueCondition(frozenMetav1Now, 123), @@ -730,6 +762,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { }, configv1alpha1.FederationDomainPhaseError, []configv1alpha1.Condition{ + happyAPIGroupSuffixCondition(frozenMetav1Now, 123), happyDisplayNamesUniqueCondition(frozenMetav1Now, 123), happyIdentityProvidersFoundConditionLegacyConfigurationSuccess(oidcIdentityProvider.Name, frozenMetav1Now, 123), unknownIssuerIsUniqueCondition(frozenMetav1Now, 123), @@ -758,6 +791,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { expectedFederationDomainStatusUpdate(federationDomain1, configv1alpha1.FederationDomainPhaseError, []configv1alpha1.Condition{ + happyAPIGroupSuffixCondition(frozenMetav1Now, 123), happyDisplayNamesUniqueCondition(frozenMetav1Now, 123), sadIdentityProvidersFoundConditionLegacyConfigurationIdentityProviderNotFound(frozenMetav1Now, 123), happyIssuerIsUniqueCondition(frozenMetav1Now, 123), @@ -769,6 +803,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { expectedFederationDomainStatusUpdate(federationDomain2, configv1alpha1.FederationDomainPhaseError, []configv1alpha1.Condition{ + happyAPIGroupSuffixCondition(frozenMetav1Now, 123), happyDisplayNamesUniqueCondition(frozenMetav1Now, 123), sadIdentityProvidersFoundConditionLegacyConfigurationIdentityProviderNotFound(frozenMetav1Now, 123), happyIssuerIsUniqueCondition(frozenMetav1Now, 123), @@ -792,6 +827,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { expectedFederationDomainStatusUpdate(federationDomain1, configv1alpha1.FederationDomainPhaseError, []configv1alpha1.Condition{ + happyAPIGroupSuffixCondition(frozenMetav1Now, 123), happyDisplayNamesUniqueCondition(frozenMetav1Now, 123), sadIdentityProvidersFoundConditionIdentityProviderNotSpecified(3, frozenMetav1Now, 123), happyIssuerIsUniqueCondition(frozenMetav1Now, 123), @@ -846,6 +882,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { }, configv1alpha1.FederationDomainPhaseError, []configv1alpha1.Condition{ + happyAPIGroupSuffixCondition(frozenMetav1Now, 123), happyDisplayNamesUniqueCondition(frozenMetav1Now, 123), sadIdentityProvidersFoundConditionIdentityProvidersObjectRefsNotFound( `.spec.identityProviders[].objectRef identifies resource(s) that cannot be found: `+ @@ -993,6 +1030,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { }, configv1alpha1.FederationDomainPhaseError, []configv1alpha1.Condition{ + happyAPIGroupSuffixCondition(frozenMetav1Now, 123), sadDisplayNamesUniqueCondition("duplicate1, duplicate2", frozenMetav1Now, 123), happyIdentityProvidersFoundConditionSuccess(frozenMetav1Now, 123), happyIssuerIsUniqueCondition(frozenMetav1Now, 123), @@ -1002,6 +1040,71 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { }), }, }, + { + name: "the federation domain has unrecognized api group names in objectRefs", + inputObjects: []runtime.Object{ + oidcIdentityProvider, + ldapIdentityProvider, + adIdentityProvider, + &configv1alpha1.FederationDomain{ + ObjectMeta: metav1.ObjectMeta{Name: "config1", Namespace: namespace, Generation: 123}, + Spec: configv1alpha1.FederationDomainSpec{ + Issuer: "https://issuer1.com", + IdentityProviders: []configv1alpha1.FederationDomainIdentityProvider{ + { + DisplayName: "name1", + ObjectRef: corev1.TypedLocalObjectReference{ + APIGroup: pointer.String("wrong.example.com"), + Kind: "OIDCIdentityProvider", + Name: oidcIdentityProvider.Name, + }, + }, + { + DisplayName: "name2", + ObjectRef: corev1.TypedLocalObjectReference{ + APIGroup: pointer.String("also-wrong.example.com"), + Kind: "LDAPIdentityProvider", + Name: ldapIdentityProvider.Name, + }, + }, + { + DisplayName: "name3", + ObjectRef: corev1.TypedLocalObjectReference{ + APIGroup: nil, // also wrong + Kind: "LDAPIdentityProvider", + Name: ldapIdentityProvider.Name, + }, + }, + { + DisplayName: "name4", + ObjectRef: corev1.TypedLocalObjectReference{ + APIGroup: pointer.String(apiGroupSupervisor), // correct + Kind: "ActiveDirectoryIdentityProvider", + Name: adIdentityProvider.Name, + }, + }, + }, + }, + }, + }, + wantFDIssuers: []*federationdomainproviders.FederationDomainIssuer{}, + wantStatusUpdates: []*configv1alpha1.FederationDomain{ + expectedFederationDomainStatusUpdate( + &configv1alpha1.FederationDomain{ + ObjectMeta: metav1.ObjectMeta{Name: "config1", Namespace: namespace, Generation: 123}, + }, + configv1alpha1.FederationDomainPhaseError, + []configv1alpha1.Condition{ + sadAPIGroupSuffixCondition("also-wrong.example.com, nil, wrong.example.com", frozenMetav1Now, 123), + happyDisplayNamesUniqueCondition(frozenMetav1Now, 123), + happyIdentityProvidersFoundConditionSuccess(frozenMetav1Now, 123), + happyIssuerIsUniqueCondition(frozenMetav1Now, 123), + happyIssuerURLValidCondition(frozenMetav1Now, 123), + happyOneTLSSecretPerIssuerHostnameCondition(frozenMetav1Now, 123), + sadReadyCondition(frozenMetav1Now, 123), + }), + }, + }, { name: "the federation domain specifies illegal const type, which shouldn't really happen since the CRD validates it", inputObjects: []runtime.Object{ @@ -1083,6 +1186,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { controller := NewFederationDomainWatcherController( federationDomainsSetter, + apiGroupSuffix, clocktesting.NewFakeClock(frozenNow), pinnipedAPIClient, pinnipedInformers.Config().V1alpha1().FederationDomains(), diff --git a/internal/supervisor/server/server.go b/internal/supervisor/server/server.go index 153869b01..d6605b2b3 100644 --- a/internal/supervisor/server/server.go +++ b/internal/supervisor/server/server.go @@ -167,6 +167,7 @@ func prepareControllers( WithController( supervisorconfig.NewFederationDomainWatcherController( issuerManager, + *cfg.APIGroupSuffix, clock.RealClock{}, pinnipedClient, federationDomainInformer,