Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add insecure-allow-http flag to block HTTP traffic #531

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+235 −89
Open

Add insecure-allow-http flag to block HTTP traffic #531

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 25 additions & 1 deletion internal/controller/alert_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"bytes"
"context"
"encoding/json"
"encoding/pem"
"fmt"
"net/http"
"net/http/httptest"
Expand Down Expand Up @@ -186,13 +187,33 @@ func TestAlertReconciler_EventHandler(t *testing.T) {
stopCh := make(chan struct{})
go eventServer.ListenAndServe(stopCh, eventMdlw, store)

rcvServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
// Run a TLS server, since HTTP traffic is disabled for the ProviderReconciler.
rcvServer := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
req = r
w.WriteHeader(200)
}))
defer rcvServer.Close()
defer close(stopCh)

// Get the CA certificate from the server and create a secret to be referenced
// later in the Provider.
cert := rcvServer.Certificate().Raw
pemBlock := &pem.Block{
Type: "CERTIFICATE",
Bytes: cert,
}
pemBytes := pem.EncodeToMemory(pemBlock)
secret := &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: "provider-ca",
Namespace: namespace,
},
Data: map[string][]byte{
"caFile": pemBytes,
},
}
g.Expect(k8sClient.Create(context.Background(), secret)).To(Succeed())

providerKey := types.NamespacedName{
Name: fmt.Sprintf("provider-%s", randStringRunes(5)),
Namespace: namespace,
Expand All @@ -205,6 +226,9 @@ func TestAlertReconciler_EventHandler(t *testing.T) {
Spec: apiv1beta2.ProviderSpec{
Type: "generic",
Address: rcvServer.URL,
CertSecretRef: &meta.LocalObjectReference{
Name: "provider-ca",
},
},
}
g.Expect(k8sClient.Create(context.Background(), provider)).To(Succeed())
Expand Down
68 changes: 57 additions & 11 deletions internal/controller/provider_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package controller
import (
"context"
"crypto/x509"
"errors"
"fmt"
"net/url"
"time"
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -154,19 +160,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
}

Expand All @@ -175,12 +195,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
}
Expand All @@ -193,6 +208,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}

Expand Down Expand Up @@ -253,6 +273,10 @@ func (r *ProviderReconciler) validateCredentials(ctx context.Context, provider *
}
}

if err := parseURLs(address, proxy, r.BlockInsecureHTTP); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this PR wait until #540 which is trying to remove the provider and alert reconcilers as a whole ?
validateCredentials() is doing more than just validating the credentials and there's a copy of it in the event handler as well. With #540, this implementation will change. Instead of setting a status, it'll become a dispatch failed warning event on the respective alert object, and maybe on the provider object too. Maybe RFC-0004 can be updated to mention how this should be reflected on status-less objects via k8s events.

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)
Expand Down Expand Up @@ -316,3 +340,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
}
144 changes: 141 additions & 3 deletions internal/controller/provider_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand All @@ -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
Expand All @@ -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))
}
})
}
}
9 changes: 5 additions & 4 deletions internal/controller/suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
6 changes: 0 additions & 6 deletions internal/notifier/alertmanager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import (
"context"
"crypto/x509"
"fmt"
"net/url"
"strings"

eventv1 "github.com/fluxcd/pkg/apis/event/v1beta1"
Expand All @@ -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,
Expand Down
Loading