fix(beyondtrust): validate store without panicking on API key auth (#6553)

Co-authored-by: Jean-Philippe Evrard <jean-philippe.evrard+rochepub@external.roche.com>
Co-authored-by: Gergely Bräutigam <gergely.brautigam@sap.com>
Signed-off-by: Emmanuel Yusufu Kimaswa <kimaswaemma36@gmail.com>

providers/v1/beyondtrust/provider.go

@@ -49,6 +49,8 @@ const (
 	errMissingProvider      = "storeSpec is missing provider"
 	errInvalidProvider      = "invalid provider spec. Missing field in store %s"
 	errInvalidHostURL       = "invalid host URL"
+	errMissingAuthMethod    = "no authentication method configured: set auth.apiKey, or auth.clientId and auth.clientSecret"
+	errIncompleteOAuth      = "both clientId and clientSecret must be set for OAuth authentication"
 	errNoSuchKeyFmt         = "no such key in secret: %q"
 	errInvalidRetrievalPath = "invalid retrieval path. Provide one path, separator and name"
 	errNotImplemented       = "not implemented"
@@ -401,24 +403,73 @@ func (p *Provider) ValidateStore(store esv1.GenericStore) (admission.Warnings, e
 		return nil, fmt.Errorf(errInvalidProvider, store.GetObjectMeta().String())
 	}
 
+	if provider.Server == nil {
+		return nil, errors.New(errInvalidHostURL)
+	}
+
 	apiURL, err := url.Parse(provider.Server.APIURL)
-	if err != nil {
+	if err != nil || apiURL.Host == "" {
 		return nil, errors.New(errInvalidHostURL)
 	}
 
-	if provider.Auth.ClientID.SecretRef != nil {
-		return nil, err
+	if provider.Auth == nil {
+		return nil, errors.New(errMissingAuthMethod)
 	}
 
-	if provider.Auth.ClientSecret.SecretRef != nil {
-		return nil, err
+	// Auth method requirements must stay in sync with loadCredentialsFromConfig.
+	switch {
+	case provider.Auth.APIKey != nil:
+		if err := validateAuthRef(store, "apiKey", provider.Auth.APIKey); err != nil {
+			return nil, err
+		}
+	case provider.Auth.ClientID != nil && provider.Auth.ClientSecret != nil:
+		if err := validateAuthRef(store, "clientId", provider.Auth.ClientID); err != nil {
+			return nil, err
+		}
+		if err := validateAuthRef(store, "clientSecret", provider.Auth.ClientSecret); err != nil {
+			return nil, err
+		}
+	case provider.Auth.ClientID != nil || provider.Auth.ClientSecret != nil:
+		return nil, errors.New(errIncompleteOAuth)
+	default:
+		return nil, errors.New(errMissingAuthMethod)
 	}
 
-	if apiURL.Host == "" {
-		return nil, errors.New(errInvalidHostURL)
+	// loadCertificateFromConfig silently ignores an incomplete certificate pair,
+	// so warn instead of rejecting the store.
+	var warnings admission.Warnings
+	hasCertificate := provider.Auth.Certificate != nil
+	hasCertificateKey := provider.Auth.CertificateKey != nil
+	if hasCertificate != hasCertificateKey {
+		warnings = append(warnings, "certificate and certificateKey must both be set for client-certificate authentication; the incomplete pair is ignored")
+	}
+	if hasCertificate {
+		if err := validateAuthRef(store, "certificate", provider.Auth.Certificate); err != nil {
+			return warnings, err
+		}
 	}
+	if hasCertificateKey {
+		if err := validateAuthRef(store, "certificateKey", provider.Auth.CertificateKey); err != nil {
+			return warnings, err
+		}
+	}
+
+	return warnings, nil
+}
 
-	return nil, nil
+func validateAuthRef(store esv1.GenericStore, field string, ref *esv1.BeyondTrustProviderSecretRef) error {
+	if ref.SecretRef == nil && ref.Value == "" {
+		return fmt.Errorf("%s: either secretRef or value must be set", field)
+	}
+	if err := validateSecretRef(ref); err != nil {
+		return fmt.Errorf("%s: %w", field, err)
+	}
+	if ref.SecretRef != nil {
+		if err := esutils.ValidateReferentSecretSelector(store, *ref.SecretRef); err != nil {
+			return fmt.Errorf("%s: %w", field, err)
+		}
+	}
+	return nil
 }
 
 // NewProvider creates a new Provider instance.

providers/v1/beyondtrust/provider_test.go

@@ -1053,3 +1053,131 @@ func TestPushSecret_OwnerFieldsArePropagated(t *testing.T) {
 		})
 	}
 }
+
+func TestValidateStore(t *testing.T) {
+	p := &Provider{}
+	validRef := &esv1.BeyondTrustProviderSecretRef{Value: "x"}
+
+	newStore := func(prov *esv1.BeyondtrustProvider) esv1.GenericStore {
+		return &esv1.SecretStore{Spec: esv1.SecretStoreSpec{
+			Provider: &esv1.SecretStoreProvider{Beyondtrust: prov},
+		}}
+	}
+
+	tests := []struct {
+		name     string
+		store    esv1.GenericStore
+		wantErr  bool
+		wantWarn bool
+	}{
+		{
+			name: "api key auth validates without panicking",
+			store: newStore(&esv1.BeyondtrustProvider{
+				Server: &esv1.BeyondtrustServer{APIURL: fakeAPIURL},
+				Auth:   &esv1.BeyondtrustAuth{APIKey: validRef},
+			}),
+			wantErr: false,
+		},
+		{
+			name: "client id and secret auth validates",
+			store: newStore(&esv1.BeyondtrustProvider{
+				Server: &esv1.BeyondtrustServer{APIURL: fakeAPIURL},
+				Auth:   &esv1.BeyondtrustAuth{ClientID: validRef, ClientSecret: validRef},
+			}),
+			wantErr: false,
+		},
+		{
+			name: "empty api url is rejected",
+			store: newStore(&esv1.BeyondtrustProvider{
+				Server: &esv1.BeyondtrustServer{APIURL: ""},
+				Auth:   &esv1.BeyondtrustAuth{APIKey: validRef},
+			}),
+			wantErr: true,
+		},
+		{
+			name: "missing server is rejected",
+			store: newStore(&esv1.BeyondtrustProvider{
+				Auth: &esv1.BeyondtrustAuth{APIKey: validRef},
+			}),
+			wantErr: true,
+		},
+		{
+			name: "missing auth is rejected",
+			store: newStore(&esv1.BeyondtrustProvider{
+				Server: &esv1.BeyondtrustServer{APIURL: fakeAPIURL},
+			}),
+			wantErr: true,
+		},
+		{
+			name: "auth without any method is rejected",
+			store: newStore(&esv1.BeyondtrustProvider{
+				Server: &esv1.BeyondtrustServer{APIURL: fakeAPIURL},
+				Auth:   &esv1.BeyondtrustAuth{},
+			}),
+			wantErr: true,
+		},
+		{
+			name: "client id without client secret is rejected",
+			store: newStore(&esv1.BeyondtrustProvider{
+				Server: &esv1.BeyondtrustServer{APIURL: fakeAPIURL},
+				Auth:   &esv1.BeyondtrustAuth{ClientID: validRef},
+			}),
+			wantErr: true,
+		},
+		{
+			name: "auth ref without secretRef or value is rejected",
+			store: newStore(&esv1.BeyondtrustProvider{
+				Server: &esv1.BeyondtrustServer{APIURL: fakeAPIURL},
+				Auth:   &esv1.BeyondtrustAuth{APIKey: &esv1.BeyondTrustProviderSecretRef{}},
+			}),
+			wantErr: true,
+		},
+		{
+			name: "cross-namespace secretRef is rejected for a namespaced store",
+			store: newStore(&esv1.BeyondtrustProvider{
+				Server: &esv1.BeyondtrustServer{APIURL: fakeAPIURL},
+				Auth: &esv1.BeyondtrustAuth{APIKey: &esv1.BeyondTrustProviderSecretRef{
+					SecretRef: &esmeta.SecretKeySelector{
+						Name:      "creds",
+						Key:       "apikey",
+						Namespace: new("other-namespace"),
+					},
+				}},
+			}),
+			wantErr: true,
+		},
+		{
+			name: "certificate without key warns but validates",
+			store: newStore(&esv1.BeyondtrustProvider{
+				Server: &esv1.BeyondtrustServer{APIURL: fakeAPIURL},
+				Auth:   &esv1.BeyondtrustAuth{APIKey: validRef, Certificate: validRef},
+			}),
+			wantErr:  false,
+			wantWarn: true,
+		},
+		{
+			name: "complete certificate pair validates without warning",
+			store: newStore(&esv1.BeyondtrustProvider{
+				Server: &esv1.BeyondtrustServer{APIURL: fakeAPIURL},
+				Auth:   &esv1.BeyondtrustAuth{APIKey: validRef, Certificate: validRef, CertificateKey: validRef},
+			}),
+			wantErr: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			warnings, err := p.ValidateStore(tt.store)
+			if tt.wantErr {
+				require.Error(t, err)
+			} else {
+				require.NoError(t, err)
+			}
+			if tt.wantWarn {
+				require.NotEmpty(t, warnings)
+			} else {
+				require.Empty(t, warnings)
+			}
+		})
+	}
+}