add insecure-allow-http flag to block HTTP traffic

Add a new flag `insecure-allow-http` that blocks the controller from reaching any HTTP endpoints. Its set to true by default to avoid making a breaking change. If HTTP traffic is disabled and a `Provider` specifies an address with the `http` scheme, the reconciler errors out and uses the `InsecureConnectionsDisallowed` reason for the object's conditions. Ref: https://github.com/fluxcd/flux2/tree/main/rfcs/0004-insecure-http Signed-off-by: Sanskar Jaiswal <[email protected]>
fluxcd · May 22, 2023 · f12f508 · f12f508
1 parent 49122b9
commit f12f508
Show file tree

Hide file tree

Showing 15 changed files with 210 additions and 88 deletions.
diff --git a/internal/controllers/provider_controller.go b/internal/controllers/provider_controller.go
@@ -19,6 +19,7 @@ package controllers
 import (
 	"context"
 	"crypto/x509"
+	"errors"
 	"fmt"
 	"net/url"
 	"time"
@@ -48,13 +49,18 @@ import (
 	"github.com/fluxcd/notification-controller/internal/notifier"
 )
 
+// insecureHTTPError occurs when insecure HTTP communication is tried
+// and such behaviour is blocked.
+var insecureHTTPError = errors.New("use of insecure plain HTTP connections is blocked")
+
 // ProviderReconciler reconciles a Provider object
 type ProviderReconciler struct {
 	client.Client
 	helper.Metrics
 	kuberecorder.EventRecorder
 
-	ControllerName string
+	ControllerName    string
+	BlockInsecureHTTP bool
 }
 
 type ProviderReconcilerOptions struct {
@@ -158,19 +164,33 @@ func (r *ProviderReconciler) reconcile(ctx context.Context, obj *apiv1beta2.Prov
 
 	// Mark the reconciliation as stalled if the inline URL and/or proxy are invalid.
 	if err := r.validateURLs(obj); err != nil {
-		conditions.MarkFalse(obj, meta.ReadyCondition, meta.InvalidURLReason, err.Error())
-		conditions.MarkTrue(obj, meta.StalledCondition, meta.InvalidURLReason, err.Error())
+		var reason string
+		if errors.Is(err, insecureHTTPError) {
+			reason = meta.InsecureConnectionsDisallowedReason
+		} else {
+			reason = meta.InvalidURLReason
+		}
+		conditions.MarkFalse(obj, meta.ReadyCondition, reason, err.Error())
+		conditions.MarkTrue(obj, meta.StalledCondition, reason, err.Error())
 		return ctrl.Result{Requeue: true}, err
 	}
 
 	// Validate the provider credentials.
 	if err := r.validateCredentials(ctx, obj); err != nil {
-		conditions.MarkFalse(obj, meta.ReadyCondition, apiv1.ValidationFailedReason, err.Error())
+		var reason string
+		var urlErr *url.Error
+		if errors.Is(err, insecureHTTPError) {
+			reason = meta.InsecureConnectionsDisallowedReason
+		} else if errors.As(err, &urlErr) {
+			reason = meta.InvalidURLReason
+		} else {
+			reason = apiv1.ValidationFailedReason
+		}
+		conditions.MarkFalse(obj, meta.ReadyCondition, reason, err.Error())
 		return ctrl.Result{Requeue: true}, err
 	}
 
 	conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, apiv1.InitializedReason)
-
 	return ctrl.Result{RequeueAfter: obj.GetInterval()}, nil
 }
 
@@ -179,12 +199,7 @@ func (r *ProviderReconciler) validateURLs(provider *apiv1beta2.Provider) error {
 	proxy := provider.Spec.Proxy
 
 	if provider.Spec.SecretRef == nil {
-		if _, err := url.ParseRequestURI(address); err != nil {
-			return fmt.Errorf("invalid address %s: %w", address, err)
-		}
-		if _, err := url.ParseRequestURI(proxy); proxy != "" && err != nil {
-			return fmt.Errorf("invalid proxy %s: %w", proxy, err)
-		}
+		return parseURLs(address, proxy, r.BlockInsecureHTTP)
 	}
 	return nil
 }
@@ -197,6 +212,11 @@ func (r *ProviderReconciler) validateCredentials(ctx context.Context, provider *
 	token := ""
 	headers := make(map[string]string)
 	if provider.Spec.SecretRef != nil {
+		// since a secret ref is provided, the object is not stalled even if spec.address
+		// or spec.proxy are invalid, as the secret can change any time independently.
+		if conditions.IsStalled(provider) {
+			conditions.Delete(provider, meta.StalledCondition)
+		}
 		var secret corev1.Secret
 		secretName := types.NamespacedName{Namespace: provider.Namespace, Name: provider.Spec.SecretRef.Name}
 
@@ -257,6 +277,10 @@ func (r *ProviderReconciler) validateCredentials(ctx context.Context, provider *
 		}
 	}
 
+	if err := parseURLs(address, proxy, r.BlockInsecureHTTP); err != nil {
+		return err
+	}
+
 	factory := notifier.NewFactory(address, proxy, username, provider.Spec.Channel, token, headers, certPool, password, string(provider.UID))
 	if _, err := factory.Notifier(provider.Spec.Type); err != nil {
 		return fmt.Errorf("failed to initialize provider, error: %w", err)
@@ -320,3 +344,25 @@ func (r *ProviderReconciler) patch(ctx context.Context, obj *apiv1beta2.Provider
 
 	return nil
 }
+
+// parseURLs parses the provided URL strings and returns any error that
+// might occur when doing so. It raises an `insecureHTTPError` error when the
+// scheme of either URL is "http" and `blockHTTP` is set to true.
+func parseURLs(address, proxy string, blockHTTP bool) error {
+	addrURL, err := url.ParseRequestURI(address)
+	if err != nil {
+		return fmt.Errorf("invalid address %s: %w", address, err)
+	}
+	proxyURL, err := url.ParseRequestURI(proxy)
+	if proxy != "" && err != nil {
+		return fmt.Errorf("invalid proxy %s: %w", proxy, err)
+	}
+
+	if proxyURL != nil && proxyURL.Scheme == "http" && blockHTTP {
+		return fmt.Errorf("consider changing proxy to use HTTPS: %w", insecureHTTPError)
+	}
+	if addrURL != nil && addrURL.Scheme == "http" && blockHTTP {
+		return fmt.Errorf("consider changing address to use HTTPS: %w", insecureHTTPError)
+	}
+	return nil
+}
diff --git a/internal/controllers/provider_controller_test.go b/internal/controllers/provider_controller_test.go
@@ -163,7 +163,7 @@ func TestProviderReconciler_Reconcile(t *testing.T) {
 		g.Expect(conditions.GetReason(resultP, meta.ReadyCondition)).To(BeIdenticalTo(meta.InvalidURLReason))
 	})
 
-	t.Run("recovers from staleness", func(t *testing.T) {
+	t.Run("recovers from being stalled", func(t *testing.T) {
 		g := NewWithT(t)
 
 		g.Expect(k8sClient.Get(context.Background(), client.ObjectKeyFromObject(provider), resultP)).To(Succeed())
@@ -180,14 +180,92 @@ func TestProviderReconciler_Reconcile(t *testing.T) {
 		g.Expect(conditions.Has(resultP, meta.StalledCondition)).To(BeFalse())
 	})
 
-	t.Run("finalizes suspended object", func(t *testing.T) {
+	t.Run("HTTP connections are blocked", func(t *testing.T) {
+		g := NewWithT(t)
+		g.Expect(k8sClient.Get(context.Background(), client.ObjectKeyFromObject(provider), resultP)).To(Succeed())
+
+		resultP.Spec.Proxy = "http://proxy.internal"
+		g.Expect(k8sClient.Update(context.Background(), resultP)).To(Succeed())
+
+		g.Eventually(func() bool {
+			_ = k8sClient.Get(context.Background(), client.ObjectKeyFromObject(provider), resultP)
+			return !conditions.IsReady(resultP)
+		}, timeout, time.Second).Should(BeTrue())
+
+		g.Expect(conditions.Has(resultP, meta.ReconcilingCondition)).To(BeFalse())
+		g.Expect(conditions.Has(resultP, meta.StalledCondition)).To(BeTrue())
+		g.Expect(conditions.GetObservedGeneration(resultP, meta.StalledCondition)).To(BeIdenticalTo(resultP.Generation))
+		g.Expect(conditions.GetReason(resultP, meta.StalledCondition)).To(BeIdenticalTo(meta.InsecureConnectionsDisallowedReason))
+		g.Expect(conditions.GetReason(resultP, meta.ReadyCondition)).To(BeIdenticalTo(meta.InsecureConnectionsDisallowedReason))
+	})
+
+	t.Run("becomes not ready with InvalidURLReason if secret has an invalid address", func(t *testing.T) {
 		g := NewWithT(t)
 
+		secret := &corev1.Secret{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      secretName,
+				Namespace: namespaceName,
+			},
+			StringData: map[string]string{
+				"token":   "test",
+				"address": "http//invalid",
+			},
+		}
+		g.Expect(k8sClient.Update(context.Background(), secret)).To(Succeed())
+
 		g.Expect(k8sClient.Get(context.Background(), client.ObjectKeyFromObject(provider), resultP)).To(Succeed())
+		resultP.Spec.SecretRef = &meta.LocalObjectReference{
+			Name: secretName,
+		}
+		resultP.Spec.Proxy = ""
+		resultP.Spec.Address = ""
+		g.Expect(k8sClient.Update(context.Background(), resultP)).To(Succeed())
 
-		resultP.Spec.Suspend = true
+		g.Eventually(func() bool {
+			_ = k8sClient.Get(context.Background(), client.ObjectKeyFromObject(provider), resultP)
+			return !conditions.IsStalled(resultP)
+		}, timeout, time.Second).Should(BeTrue())
+
+		g.Expect(conditions.GetReason(resultP, meta.ReadyCondition)).To(BeIdenticalTo(meta.InvalidURLReason))
+
+		g.Expect(conditions.Has(resultP, meta.ReconcilingCondition)).To(BeTrue())
+		g.Expect(conditions.GetReason(resultP, meta.ReconcilingCondition)).To(BeIdenticalTo(meta.ProgressingWithRetryReason))
+		g.Expect(conditions.GetObservedGeneration(resultP, meta.ReconcilingCondition)).To(BeIdenticalTo(resultP.Generation))
+	})
+
+	t.Run("is not stalled if there is a secret ref even if spec.address is invalid", func(t *testing.T) {
+		g := NewWithT(t)
+
+		g.Expect(k8sClient.Get(context.Background(), client.ObjectKeyFromObject(provider), resultP)).To(Succeed())
+
+		resultP.Spec.Address = "http://invalid|"
 		g.Expect(k8sClient.Update(context.Background(), resultP)).To(Succeed())
 
+		g.Eventually(func() bool {
+			_ = k8sClient.Get(context.Background(), client.ObjectKeyFromObject(provider), resultP)
+			return !conditions.IsReady(resultP)
+		}, timeout, time.Second).Should(BeTrue())
+
+		g.Expect(conditions.Has(resultP, meta.StalledCondition)).To(BeFalse())
+		g.Expect(conditions.Has(resultP, meta.ReconcilingCondition)).To(BeTrue())
+		g.Expect(conditions.GetReason(resultP, meta.ReconcilingCondition)).To(BeIdenticalTo(meta.ProgressingWithRetryReason))
+	})
+
+	t.Run("finalizes suspended object", func(t *testing.T) {
+		g := NewWithT(t)
+
+		g.Eventually(func() bool {
+			if err := k8sClient.Get(context.Background(), client.ObjectKeyFromObject(provider), resultP); err != nil {
+				return false
+			}
+			resultP.Spec.Suspend = true
+			if err := k8sClient.Update(context.Background(), resultP); err != nil {
+				return false
+			}
+			return true
+		}, timeout, time.Second).Should(BeTrue())
+
 		g.Eventually(func() bool {
 			_ = k8sClient.Get(context.Background(), client.ObjectKeyFromObject(provider), resultP)
 			return resultP.Spec.Suspend == true
@@ -201,3 +279,63 @@ func TestProviderReconciler_Reconcile(t *testing.T) {
 		}, timeout, time.Second).Should(BeTrue())
 	})
 }
+
+func Test_parseURLs(t *testing.T) {
+	tests := []struct {
+		name      string
+		address   string
+		proxy     string
+		blockHTTP bool
+		err       error
+		errMsg    string
+	}{
+		{
+			name:    "valid address and proxy",
+			address: "http://example.com",
+			proxy:   "http://proxy.com",
+		},
+		{
+			name:    "invalid address",
+			address: "http//invalid",
+			errMsg:  "invalid address",
+		},
+		{
+			name:    "invalid proxy",
+			address: "http://example.com",
+			proxy:   "http//invalid",
+			errMsg:  "invalid proxy",
+		},
+		{
+			name:      "block http proxy",
+			address:   "http://example.com",
+			proxy:     "http://proxy.com",
+			blockHTTP: true,
+			err:       insecureHTTPError,
+			errMsg:    "consider changing proxy",
+		},
+		{
+			name:      "block http address",
+			address:   "http://example.com",
+			blockHTTP: true,
+			err:       insecureHTTPError,
+			errMsg:    "consider changing address",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			g := NewWithT(t)
+			err := parseURLs(tt.address, tt.proxy, tt.blockHTTP)
+
+			if tt.errMsg == "" {
+				g.Expect(err).ToNot(HaveOccurred())
+			} else {
+				g.Expect(err).To(HaveOccurred())
+				g.Expect(err.Error()).To(ContainSubstring(tt.errMsg))
+			}
+			if tt.err != nil {
+				g.Expect(err).To(MatchError(tt.err))
+			}
+		})
+	}
+}
diff --git a/internal/controllers/suite_test.go b/internal/controllers/suite_test.go
@@ -81,10 +81,11 @@ func TestMain(m *testing.M) {
 	}
 
 	if err := (&ProviderReconciler{
-		Client:         testEnv,
-		Metrics:        testMetricsH,
-		ControllerName: controllerName,
-		EventRecorder:  testEnv.GetEventRecorderFor(controllerName),
+		Client:            testEnv,
+		Metrics:           testMetricsH,
+		ControllerName:    controllerName,
+		EventRecorder:     testEnv.GetEventRecorderFor(controllerName),
+		BlockInsecureHTTP: true,
 	}).SetupWithManagerAndOptions(testEnv, ProviderReconcilerOptions{
 		RateLimiter: controller.GetDefaultRateLimiter(),
 	}); err != nil {

diff --git a/internal/notifier/alertmanager.go b/internal/notifier/alertmanager.go
@@ -20,7 +20,6 @@ import (
 	"context"
 	"crypto/x509"
 	"fmt"
-	"net/url"
 	"strings"
 
 	eventv1 "github.com/fluxcd/pkg/apis/event/v1beta1"
@@ -39,11 +38,6 @@ type AlertManagerAlert struct {
 }
 
 func NewAlertmanager(hookURL string, proxyURL string, certPool *x509.CertPool) (*Alertmanager, error) {
-	_, err := url.ParseRequestURI(hookURL)
-	if err != nil {
-		return nil, fmt.Errorf("invalid Alertmanager URL %s: '%w'", hookURL, err)
-	}
-
 	return &Alertmanager{
 		URL:      hookURL,
 		ProxyURL: proxyURL,

diff --git a/internal/notifier/forwarder.go b/internal/notifier/forwarder.go
@@ -23,7 +23,6 @@ import (
 	"crypto/x509"
 	"encoding/json"
 	"fmt"
-	"net/url"
 
 	eventv1 "github.com/fluxcd/pkg/apis/event/v1beta1"
 
@@ -45,10 +44,6 @@ type Forwarder struct {
 }
 
 func NewForwarder(hookURL string, proxyURL string, headers map[string]string, certPool *x509.CertPool, hmacKey []byte) (*Forwarder, error) {
-	if _, err := url.ParseRequestURI(hookURL); err != nil {
-		return nil, fmt.Errorf("invalid hook URL %s: %w", hookURL, err)
-	}
-
 	if hmacKey != nil && len(hmacKey) == 0 {
 		return nil, fmt.Errorf("HMAC key is empty")
 	}

diff --git a/internal/notifier/google_chat.go b/internal/notifier/google_chat.go
@@ -19,7 +19,6 @@ package notifier
 import (
 	"context"
 	"fmt"
-	"net/url"
 	"strings"
 
 	eventv1 "github.com/fluxcd/pkg/apis/event/v1beta1"
@@ -74,11 +73,6 @@ type GoogleChatCardWidgetKeyValue struct {
 
 // NewGoogleChat validates the Google Chat URL and returns a GoogleChat object
 func NewGoogleChat(hookURL string, proxyURL string) (*GoogleChat, error) {
-	_, err := url.ParseRequestURI(hookURL)
-	if err != nil {
-		return nil, fmt.Errorf("invalid Google Chat hook URL %s", hookURL)
-	}
-
 	return &GoogleChat{
 		URL:      hookURL,
 		ProxyURL: proxyURL,

diff --git a/internal/notifier/grafana.go b/internal/notifier/grafana.go
@@ -20,7 +20,6 @@ import (
 	"context"
 	"crypto/x509"
 	"fmt"
-	"net/url"
 	"strings"
 
 	eventv1 "github.com/fluxcd/pkg/apis/event/v1beta1"
@@ -45,11 +44,6 @@ type GraphitePayload struct {
 
 // NewGrafana validates the Grafana URL and returns a Grafana object
 func NewGrafana(URL string, proxyURL string, token string, certPool *x509.CertPool, username string, password string) (*Grafana, error) {
-	_, err := url.ParseRequestURI(URL)
-	if err != nil {
-		return nil, fmt.Errorf("invalid Grafana URL %s", URL)
-	}
-
 	return &Grafana{
 		URL:      URL,
 		ProxyURL: proxyURL,