Skip to content

Commit

Permalink
fix security vulnerabilities (#296)
Browse files Browse the repository at this point in the history
  • Loading branch information
@kerenlahav
kerenlahav authored Jul 3, 2023
1 parent f447409 commit 7e5f8b8
Show file tree
Hide file tree
Showing 11 changed files with 212 additions and 853 deletions.
20 changes: 11 additions & 9 deletions api/v1/servicebinding_validating_webhook.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ import (
"reflect"
"time"

"sigs.k8s.io/controller-runtime/pkg/webhook/admission"

"fmt"

"github.com/SAP/sap-btp-service-operator/api"
Expand Down Expand Up @@ -47,22 +49,22 @@ func (sb *ServiceBinding) SetupWebhookWithManager(mgr ctrl.Manager) error {
var _ webhook.Validator = &ServiceBinding{}

// ValidateCreate implements webhook.Validator so a webhook will be registered for the type
func (sb *ServiceBinding) ValidateCreate() error {
func (sb *ServiceBinding) ValidateCreate() (admission.Warnings, error) {
servicebindinglog.Info("validate create", "name", sb.Name)
if sb.Spec.CredRotationPolicy != nil {
if err := sb.validateCredRotatingConfig(); err != nil {
return err
return nil, err
}
}
return nil
return nil, nil
}

// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type
func (sb *ServiceBinding) ValidateUpdate(old runtime.Object) error {
func (sb *ServiceBinding) ValidateUpdate(old runtime.Object) (admission.Warnings, error) {
servicebindinglog.Info("validate update", "name", sb.Name)
if sb.Spec.CredRotationPolicy != nil {
if err := sb.validateCredRotatingConfig(); err != nil {
return err
return nil, err
}
}

Expand All @@ -75,9 +77,9 @@ func (sb *ServiceBinding) ValidateUpdate(old runtime.Object) error {
}

if specChanged && (sb.Status.BindingID != "" || isStale) {
return fmt.Errorf("updating service bindings is not supported")
return nil, fmt.Errorf("updating service bindings is not supported")
}
return nil
return nil, nil
}

func (sb *ServiceBinding) specChanged(old runtime.Object) bool {
Expand All @@ -92,11 +94,11 @@ func (sb *ServiceBinding) specChanged(old runtime.Object) bool {
}

// ValidateDelete implements webhook.Validator so a webhook will be registered for the type
func (sb *ServiceBinding) ValidateDelete() error {
func (sb *ServiceBinding) ValidateDelete() (admission.Warnings, error) {
servicebindinglog.Info("validate delete", "name", sb.Name)

// TODO(user): fill in your validation logic upon object deletion.
return nil
return nil, nil
}

func (sb *ServiceBinding) validateCredRotatingConfig() error {
Expand Down
50 changes: 25 additions & 25 deletions api/v1/servicebinding_validating_webhook_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ var _ = Describe("Service Binding Webhook Test", func() {
Context("Validator", func() {
Context("Validate create", func() {
It("should succeed", func() {
err := binding.ValidateCreate()
_, err := binding.ValidateCreate()
Expect(err).ToNot(HaveOccurred())
})
})
Expand All @@ -31,15 +31,15 @@ var _ = Describe("Service Binding Webhook Test", func() {
When("Service instance name changed", func() {
It("should succeed", func() {
newBinding.Spec.ServiceInstanceName = "new-service-instance"
err := newBinding.ValidateUpdate(binding)
_, err := newBinding.ValidateUpdate(binding)
Expect(err).ToNot(HaveOccurred())
})
})

When("External name changed", func() {
It("should succeed", func() {
newBinding.Spec.ExternalName = "new-external-instance"
err := newBinding.ValidateUpdate(binding)
_, err := newBinding.ValidateUpdate(binding)
Expect(err).ToNot(HaveOccurred())
})
})
Expand All @@ -49,15 +49,15 @@ var _ = Describe("Service Binding Webhook Test", func() {
newBinding.Spec.Parameters = &runtime.RawExtension{
Raw: []byte("params"),
}
err := newBinding.ValidateUpdate(binding)
_, err := newBinding.ValidateUpdate(binding)
Expect(err).ToNot(HaveOccurred())
})
})

When("ParametersFrom were changed", func() {
It("should succeed", func() {
newBinding.Spec.ParametersFrom[0].SecretKeyRef.Name = "newName"
err := newBinding.ValidateUpdate(binding)
_, err := newBinding.ValidateUpdate(binding)
Expect(err).ToNot(HaveOccurred())
})
})
Expand All @@ -66,7 +66,7 @@ var _ = Describe("Service Binding Webhook Test", func() {
When("Metadata changed", func() {
It("should succeed", func() {
newBinding.Finalizers = append(newBinding.Finalizers, "newFinalizer")
err := newBinding.ValidateUpdate(binding)
_, err := newBinding.ValidateUpdate(binding)
Expect(err).ToNot(HaveOccurred())
})
})
Expand All @@ -78,7 +78,7 @@ var _ = Describe("Service Binding Webhook Test", func() {
RotatedBindingTTL: "1s",
RotationFrequency: "1s",
}
err := newBinding.ValidateUpdate(binding)
_, err := newBinding.ValidateUpdate(binding)
Expect(err).ToNot(HaveOccurred())
})

Expand All @@ -88,14 +88,14 @@ var _ = Describe("Service Binding Webhook Test", func() {
RotatedBindingTTL: "1x",
RotationFrequency: "1y",
}
err := newBinding.ValidateUpdate(binding)
_, err := newBinding.ValidateUpdate(binding)
Expect(err).To(HaveOccurred())
})

It("should fail on update with stale label", func() {
newBinding.Spec.ParametersFrom[0].SecretKeyRef.Name = "newName"
newBinding.Labels = map[string]string{api.StaleBindingIDLabel: "true"}
err := newBinding.ValidateUpdate(binding)
_, err := newBinding.ValidateUpdate(binding)
Expect(err).To(HaveOccurred())
})

Expand All @@ -104,7 +104,7 @@ var _ = Describe("Service Binding Webhook Test", func() {
When("Status changed", func() {
It("should succeed", func() {
newBinding.Status.BindingID = "12345"
err := newBinding.ValidateUpdate(binding)
_, err := newBinding.ValidateUpdate(binding)
Expect(err).ToNot(HaveOccurred())
})
})
Expand All @@ -120,23 +120,23 @@ var _ = Describe("Service Binding Webhook Test", func() {
When("Service instance name changed", func() {
It("should fail", func() {
newBinding.Spec.ServiceInstanceName = "new-service-instance"
err := newBinding.ValidateUpdate(binding)
_, err := newBinding.ValidateUpdate(binding)
Expect(err).To(HaveOccurred())
})
})

When("External name changed", func() {
It("should fail", func() {
newBinding.Spec.ExternalName = "new-external-instance"
err := newBinding.ValidateUpdate(binding)
_, err := newBinding.ValidateUpdate(binding)
Expect(err).To(HaveOccurred())
})
})

When("secret name changed", func() {
It("should fail", func() {
newBinding.Spec.SecretName = "newsecret"
err := newBinding.ValidateUpdate(binding)
_, err := newBinding.ValidateUpdate(binding)
Expect(err).To(HaveOccurred())
})
})
Expand All @@ -145,7 +145,7 @@ var _ = Describe("Service Binding Webhook Test", func() {
It("should fail", func() {
secretKey := "secret-key"
newBinding.Spec.SecretKey = &secretKey
err := newBinding.ValidateUpdate(binding)
_, err := newBinding.ValidateUpdate(binding)
Expect(err).To(HaveOccurred())
})
})
Expand All @@ -154,7 +154,7 @@ var _ = Describe("Service Binding Webhook Test", func() {
It("should fail", func() {
secretRootKey := "root"
newBinding.Spec.SecretRootKey = &secretRootKey
err := newBinding.ValidateUpdate(binding)
_, err := newBinding.ValidateUpdate(binding)
Expect(err).To(HaveOccurred())
})
})
Expand All @@ -164,34 +164,34 @@ var _ = Describe("Service Binding Webhook Test", func() {
newBinding.Spec.Parameters = &runtime.RawExtension{
Raw: []byte("params"),
}
err := newBinding.ValidateUpdate(binding)
_, err := newBinding.ValidateUpdate(binding)
Expect(err).To(HaveOccurred())
})
})

When("ParametersFrom were changed", func() {
It("should fail on changed name", func() {
newBinding.Spec.ParametersFrom[0].SecretKeyRef.Name = "newName"
err := newBinding.ValidateUpdate(binding)
_, err := newBinding.ValidateUpdate(binding)
Expect(err).To(HaveOccurred())
})

It("should fail on changed key", func() {
newBinding.Spec.ParametersFrom[0].SecretKeyRef.Key = "newName"
err := newBinding.ValidateUpdate(binding)
_, err := newBinding.ValidateUpdate(binding)
Expect(err).To(HaveOccurred())
})

It("should fail on nil array", func() {
newBinding.Spec.ParametersFrom = nil
err := newBinding.ValidateUpdate(binding)
_, err := newBinding.ValidateUpdate(binding)
Expect(err).To(HaveOccurred())
})

It("should fail on changed array", func() {
p := ParametersFromSource{}
newBinding.Spec.ParametersFrom[0] = p
err := newBinding.ValidateUpdate(binding)
_, err := newBinding.ValidateUpdate(binding)
Expect(err).To(HaveOccurred())
})

Expand All @@ -201,7 +201,7 @@ var _ = Describe("Service Binding Webhook Test", func() {
When("Metadata changed", func() {
It("should succeed", func() {
newBinding.Finalizers = append(newBinding.Finalizers, "newFinalizer")
err := newBinding.ValidateUpdate(binding)
_, err := newBinding.ValidateUpdate(binding)
Expect(err).ToNot(HaveOccurred())
})
})
Expand All @@ -213,7 +213,7 @@ var _ = Describe("Service Binding Webhook Test", func() {
RotatedBindingTTL: "1s",
RotationFrequency: "1s",
}
err := newBinding.ValidateUpdate(binding)
_, err := newBinding.ValidateUpdate(binding)
Expect(err).ToNot(HaveOccurred())
})

Expand All @@ -223,23 +223,23 @@ var _ = Describe("Service Binding Webhook Test", func() {
RotatedBindingTTL: "1x",
RotationFrequency: "1y",
}
err := newBinding.ValidateUpdate(binding)
_, err := newBinding.ValidateUpdate(binding)
Expect(err).To(HaveOccurred())
})
})

When("Status changed", func() {
It("should succeed", func() {
newBinding.Status.BindingID = "12345"
err := newBinding.ValidateUpdate(binding)
_, err := newBinding.ValidateUpdate(binding)
Expect(err).ToNot(HaveOccurred())
})
})
})

Context("Validate delete", func() {
It("should succeed", func() {
err := binding.ValidateDelete()
_, err := binding.ValidateDelete()
Expect(err).ToNot(HaveOccurred())
})
})
Expand Down
9 changes: 2 additions & 7 deletions api/v1/webhooks/servicebinding_mutating_webhook.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,13 @@ import (
var bindinglog = logf.Log.WithName("servicebinding-webhook")

type ServiceBindingDefaulter struct {
decoder *admission.Decoder
Decoder *admission.Decoder
}

func (s *ServiceBindingDefaulter) Handle(_ context.Context, req admission.Request) admission.Response {
bindinglog.Info("Defaulter webhook for servicebinding")
binding := &servicesv1.ServiceBinding{}
err := s.decoder.Decode(req, binding)
err := s.Decoder.Decode(req, binding)
if err != nil {
return admission.Errored(http.StatusBadRequest, err)
}
Expand Down Expand Up @@ -73,8 +73,3 @@ func (s *ServiceBindingDefaulter) Handle(_ context.Context, req admission.Reques
}
return admission.PatchResponseFromRaw(req.Object.Raw, marshaledInstance)
}

func (s *ServiceBindingDefaulter) InjectDecoder(d *admission.Decoder) error {
s.decoder = d
return nil
}
11 changes: 3 additions & 8 deletions api/v1/webhooks/serviceinstance_mutating_webhook.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,13 @@ import (
var instancelog = logf.Log.WithName("serviceinstance-webhook")

type ServiceInstanceDefaulter struct {
decoder *admission.Decoder
Decoder *admission.Decoder
}

func (s *ServiceInstanceDefaulter) Handle(_ context.Context, req admission.Request) admission.Response {
instancelog.Info("Defaulter webhook for serviceinstance")
instance := &servicesv1.ServiceInstance{}
err := s.decoder.Decode(req, instance)
err := s.Decoder.Decode(req, instance)
if err != nil {
return admission.Errored(http.StatusBadRequest, err)
}
Expand Down Expand Up @@ -59,7 +59,7 @@ func (s *ServiceInstanceDefaulter) setServiceInstanceUserInfo(req admission.Requ
instance.Spec.UserInfo = userInfo
} else if req.Operation == v1admission.Update {
oldInstance := &servicesv1.ServiceInstance{}
err := s.decoder.DecodeRaw(req.OldObject, oldInstance)
err := s.Decoder.DecodeRaw(req.OldObject, oldInstance)
if err != nil {
return err
}
Expand All @@ -69,8 +69,3 @@ func (s *ServiceInstanceDefaulter) setServiceInstanceUserInfo(req admission.Requ
}
return nil
}

func (s *ServiceInstanceDefaulter) InjectDecoder(d *admission.Decoder) error {
s.decoder = d
return nil
}
2 changes: 1 addition & 1 deletion controllers/serviceinstance_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -631,7 +631,7 @@ func getTags(tags []byte) ([]string, error) {

func getSpecHash(serviceInstance *servicesv1.ServiceInstance) string {
spec := serviceInstance.Spec
spec.Shared = pointer.BoolPtr(false)
spec.Shared = pointer.Bool(false)
specBytes, _ := json.Marshal(spec)
s := string(specBytes)
return generateEncodedMD5Hash(s)
Expand Down
9 changes: 8 additions & 1 deletion controllers/serviceinstance_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,14 @@ var _ = Describe("ServiceInstance controller", func() {

It("provisioning should fail", func() {
serviceInstance = createInstance(ctx, instanceSpec)
Expect(serviceInstance.Status.Conditions[0].Message).To(ContainSubstring("provided plan id does not match"))
Eventually(func() bool {
err := k8sClient.Get(ctx, defaultLookupKey, serviceInstance)
if err != nil {
return false
}
cond := meta.FindStatusCondition(serviceInstance.Status.Conditions, api.ConditionSucceeded)
return cond != nil && strings.Contains(cond.Message, "provided plan id does not match")
}, timeout, interval).Should(BeTrue())
})
})
})
Expand Down
Loading

0 comments on commit 7e5f8b8

Please sign in to comment.