Skip to content

Commit 784b870

Browse files
aryan9600aryan9600
committed
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]>
1 parent 49122b9 commit 784b870

File tree

16 files changed

+235
-89
lines changed

16 files changed

+235
-89
lines changed

‎internal/controllers/alert_controller_test.go‎

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import (
2020
"bytes"
2121
"context"
2222
"encoding/json"
23+
"encoding/pem"
2324
"fmt"
2425
"net/http"
2526
"net/http/httptest"
@@ -186,13 +187,33 @@ func TestAlertReconciler_EventHandler(t *testing.T) {
186187
stopCh := make(chan struct{})
187188
go eventServer.ListenAndServe(stopCh, eventMdlw, store)
188189

189-
rcvServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
190+
// Run a TLS server, since HTTP traffic is disbaled for the ProviderReconciler.
191+
rcvServer := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
190192
req = r
191193
w.WriteHeader(200)
192194
}))
193195
defer rcvServer.Close()
194196
defer close(stopCh)
195197

198+
// Get the CA certificate from the server and create a secret to be referenced
199+
// later in the Provider.
200+
cert := rcvServer.Certificate().Raw
201+
pemBlock := &pem.Block{
202+
Type: "CERTIFICATE",
203+
Bytes: cert,
204+
}
205+
pemBytes := pem.EncodeToMemory(pemBlock)
206+
secret := &corev1.Secret{
207+
ObjectMeta: metav1.ObjectMeta{
208+
Name: "provider-ca",
209+
Namespace: namespace,
210+
},
211+
Data: map[string][]byte{
212+
"caFile": pemBytes,
213+
},
214+
}
215+
g.Expect(k8sClient.Create(context.Background(), secret)).To(Succeed())
216+
196217
providerKey := types.NamespacedName{
197218
Name: fmt.Sprintf("provider-%s", randStringRunes(5)),
198219
Namespace: namespace,
@@ -205,6 +226,9 @@ func TestAlertReconciler_EventHandler(t *testing.T) {
205226
Spec: apiv1beta2.ProviderSpec{
206227
Type: "generic",
207228
Address: rcvServer.URL,
229+
CertSecretRef: &meta.LocalObjectReference{
230+
Name: "provider-ca",
231+
},
208232
},
209233
}
210234
g.Expect(k8sClient.Create(context.Background(), provider)).To(Succeed())

‎internal/controllers/provider_controller.go‎

Lines changed: 57 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ package controllers
1919
import (
2020
"context"
2121
"crypto/x509"
22+
"errors"
2223
"fmt"
2324
"net/url"
2425
"time"
@@ -48,13 +49,18 @@ import (
4849
"github.com/fluxcd/notification-controller/internal/notifier"
4950
)
5051

52+
// insecureHTTPError occurs when insecure HTTP communication is tried
53+
// and such behaviour is blocked.
54+
var insecureHTTPError = errors.New("use of insecure plain HTTP connections is blocked")
55+
5156
// ProviderReconciler reconciles a Provider object
5257
type ProviderReconciler struct {
5358
client.Client
5459
helper.Metrics
5560
kuberecorder.EventRecorder
5661

57-
ControllerName string
62+
ControllerName string
63+
BlockInsecureHTTP bool
5864
}
5965

6066
type ProviderReconcilerOptions struct {
@@ -158,19 +164,33 @@ func (r *ProviderReconciler) reconcile(ctx context.Context, obj *apiv1beta2.Prov
158164

159165
// Mark the reconciliation as stalled if the inline URL and/or proxy are invalid.
160166
if err := r.validateURLs(obj); err != nil {
161-
conditions.MarkFalse(obj, meta.ReadyCondition, meta.InvalidURLReason, err.Error())
162-
conditions.MarkTrue(obj, meta.StalledCondition, meta.InvalidURLReason, err.Error())
167+
var reason string
168+
if errors.Is(err, insecureHTTPError) {
169+
reason = meta.InsecureConnectionsDisallowedReason
170+
} else {
171+
reason = meta.InvalidURLReason
172+
}
173+
conditions.MarkFalse(obj, meta.ReadyCondition, reason, err.Error())
174+
conditions.MarkTrue(obj, meta.StalledCondition, reason, err.Error())
163175
return ctrl.Result{Requeue: true}, err
164176
}
165177

166178
// Validate the provider credentials.
167179
if err := r.validateCredentials(ctx, obj); err != nil {
168-
conditions.MarkFalse(obj, meta.ReadyCondition, apiv1.ValidationFailedReason, err.Error())
180+
var reason string
181+
var urlErr *url.Error
182+
if errors.Is(err, insecureHTTPError) {
183+
reason = meta.InsecureConnectionsDisallowedReason
184+
} else if errors.As(err, &urlErr) {
185+
reason = meta.InvalidURLReason
186+
} else {
187+
reason = apiv1.ValidationFailedReason
188+
}
189+
conditions.MarkFalse(obj, meta.ReadyCondition, reason, err.Error())
169190
return ctrl.Result{Requeue: true}, err
170191
}
171192

172193
conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, apiv1.InitializedReason)
173-
174194
return ctrl.Result{RequeueAfter: obj.GetInterval()}, nil
175195
}
176196

@@ -179,12 +199,7 @@ func (r *ProviderReconciler) validateURLs(provider *apiv1beta2.Provider) error {
179199
proxy := provider.Spec.Proxy
180200

181201
if provider.Spec.SecretRef == nil {
182-
if _, err := url.ParseRequestURI(address); err != nil {
183-
return fmt.Errorf("invalid address %s: %w", address, err)
184-
}
185-
if _, err := url.ParseRequestURI(proxy); proxy != "" && err != nil {
186-
return fmt.Errorf("invalid proxy %s: %w", proxy, err)
187-
}
202+
return parseURLs(address, proxy, r.BlockInsecureHTTP)
188203
}
189204
return nil
190205
}
@@ -197,6 +212,11 @@ func (r *ProviderReconciler) validateCredentials(ctx context.Context, provider *
197212
token := ""
198213
headers := make(map[string]string)
199214
if provider.Spec.SecretRef != nil {
215+
// since a secret ref is provided, the object is not stalled even if spec.address
216+
// or spec.proxy are invalid, as the secret can change any time independently.
217+
if conditions.IsStalled(provider) {
218+
conditions.Delete(provider, meta.StalledCondition)
219+
}
200220
var secret corev1.Secret
201221
secretName := types.NamespacedName{Namespace: provider.Namespace, Name: provider.Spec.SecretRef.Name}
202222

@@ -257,6 +277,10 @@ func (r *ProviderReconciler) validateCredentials(ctx context.Context, provider *
257277
}
258278
}
259279

280+
if err := parseURLs(address, proxy, r.BlockInsecureHTTP); err != nil {
281+
return err
282+
}
283+
260284
factory := notifier.NewFactory(address, proxy, username, provider.Spec.Channel, token, headers, certPool, password, string(provider.UID))
261285
if _, err := factory.Notifier(provider.Spec.Type); err != nil {
262286
return fmt.Errorf("failed to initialize provider, error: %w", err)
@@ -320,3 +344,25 @@ func (r *ProviderReconciler) patch(ctx context.Context, obj *apiv1beta2.Provider
320344

321345
return nil
322346
}
347+
348+
// parseURLs parses the provided URL strings and returns any error that
349+
// might occur when doing so. It raises an `insecureHTTPError` error when the
350+
// scheme of either URL is "http" and `blockHTTP` is set to true.
351+
func parseURLs(address, proxy string, blockHTTP bool) error {
352+
addrURL, err := url.ParseRequestURI(address)
353+
if err != nil {
354+
return fmt.Errorf("invalid address %s: %w", address, err)
355+
}
356+
proxyURL, err := url.ParseRequestURI(proxy)
357+
if proxy != "" && err != nil {
358+
return fmt.Errorf("invalid proxy %s: %w", proxy, err)
359+
}
360+
361+
if proxyURL != nil && proxyURL.Scheme == "http" && blockHTTP {
362+
return fmt.Errorf("consider changing proxy to use HTTPS: %w", insecureHTTPError)
363+
}
364+
if addrURL != nil && addrURL.Scheme == "http" && blockHTTP {
365+
return fmt.Errorf("consider changing address to use HTTPS: %w", insecureHTTPError)
366+
}
367+
return nil
368+
}

‎internal/controllers/provider_controller_test.go‎

Lines changed: 141 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -163,7 +163,7 @@ func TestProviderReconciler_Reconcile(t *testing.T) {
163163
g.Expect(conditions.GetReason(resultP, meta.ReadyCondition)).To(BeIdenticalTo(meta.InvalidURLReason))
164164
})
165165

166-
t.Run("recovers from staleness", func(t *testing.T) {
166+
t.Run("recovers from being stalled", func(t *testing.T) {
167167
g := NewWithT(t)
168168

169169
g.Expect(k8sClient.Get(context.Background(), client.ObjectKeyFromObject(provider), resultP)).To(Succeed())
@@ -180,14 +180,92 @@ func TestProviderReconciler_Reconcile(t *testing.T) {
180180
g.Expect(conditions.Has(resultP, meta.StalledCondition)).To(BeFalse())
181181
})
182182

183-
t.Run("finalizes suspended object", func(t *testing.T) {
183+
t.Run("HTTP connections are blocked", func(t *testing.T) {
184+
g := NewWithT(t)
185+
g.Expect(k8sClient.Get(context.Background(), client.ObjectKeyFromObject(provider), resultP)).To(Succeed())
186+
187+
resultP.Spec.Proxy = "http://proxy.internal"
188+
g.Expect(k8sClient.Update(context.Background(), resultP)).To(Succeed())
189+
190+
g.Eventually(func() bool {
191+
_ = k8sClient.Get(context.Background(), client.ObjectKeyFromObject(provider), resultP)
192+
return !conditions.IsReady(resultP)
193+
}, timeout, time.Second).Should(BeTrue())
194+
195+
g.Expect(conditions.Has(resultP, meta.ReconcilingCondition)).To(BeFalse())
196+
g.Expect(conditions.Has(resultP, meta.StalledCondition)).To(BeTrue())
197+
g.Expect(conditions.GetObservedGeneration(resultP, meta.StalledCondition)).To(BeIdenticalTo(resultP.Generation))
198+
g.Expect(conditions.GetReason(resultP, meta.StalledCondition)).To(BeIdenticalTo(meta.InsecureConnectionsDisallowedReason))
199+
g.Expect(conditions.GetReason(resultP, meta.ReadyCondition)).To(BeIdenticalTo(meta.InsecureConnectionsDisallowedReason))
200+
})
201+
202+
t.Run("becomes not ready with InvalidURLReason if secret has an invalid address", func(t *testing.T) {
184203
g := NewWithT(t)
185204

205+
secret := &corev1.Secret{
206+
ObjectMeta: metav1.ObjectMeta{
207+
Name: secretName,
208+
Namespace: namespaceName,
209+
},
210+
StringData: map[string]string{
211+
"token": "test",
212+
"address": "http//invalid",
213+
},
214+
}
215+
g.Expect(k8sClient.Update(context.Background(), secret)).To(Succeed())
216+
186217
g.Expect(k8sClient.Get(context.Background(), client.ObjectKeyFromObject(provider), resultP)).To(Succeed())
218+
resultP.Spec.SecretRef = &meta.LocalObjectReference{
219+
Name: secretName,
220+
}
221+
resultP.Spec.Proxy = ""
222+
resultP.Spec.Address = ""
223+
g.Expect(k8sClient.Update(context.Background(), resultP)).To(Succeed())
187224

188-
resultP.Spec.Suspend = true
225+
g.Eventually(func() bool {
226+
_ = k8sClient.Get(context.Background(), client.ObjectKeyFromObject(provider), resultP)
227+
return !conditions.IsStalled(resultP)
228+
}, timeout, time.Second).Should(BeTrue())
229+
230+
g.Expect(conditions.GetReason(resultP, meta.ReadyCondition)).To(BeIdenticalTo(meta.InvalidURLReason))
231+
232+
g.Expect(conditions.Has(resultP, meta.ReconcilingCondition)).To(BeTrue())
233+
g.Expect(conditions.GetReason(resultP, meta.ReconcilingCondition)).To(BeIdenticalTo(meta.ProgressingWithRetryReason))
234+
g.Expect(conditions.GetObservedGeneration(resultP, meta.ReconcilingCondition)).To(BeIdenticalTo(resultP.Generation))
235+
})
236+
237+
t.Run("is not stalled if there is a secret ref even if spec.address is invalid", func(t *testing.T) {
238+
g := NewWithT(t)
239+
240+
g.Expect(k8sClient.Get(context.Background(), client.ObjectKeyFromObject(provider), resultP)).To(Succeed())
241+
242+
resultP.Spec.Address = "http://invalid|"
189243
g.Expect(k8sClient.Update(context.Background(), resultP)).To(Succeed())
190244

245+
g.Eventually(func() bool {
246+
_ = k8sClient.Get(context.Background(), client.ObjectKeyFromObject(provider), resultP)
247+
return !conditions.IsReady(resultP)
248+
}, timeout, time.Second).Should(BeTrue())
249+
250+
g.Expect(conditions.Has(resultP, meta.StalledCondition)).To(BeFalse())
251+
g.Expect(conditions.Has(resultP, meta.ReconcilingCondition)).To(BeTrue())
252+
g.Expect(conditions.GetReason(resultP, meta.ReconcilingCondition)).To(BeIdenticalTo(meta.ProgressingWithRetryReason))
253+
})
254+
255+
t.Run("finalizes suspended object", func(t *testing.T) {
256+
g := NewWithT(t)
257+
258+
g.Eventually(func() bool {
259+
if err := k8sClient.Get(context.Background(), client.ObjectKeyFromObject(provider), resultP); err != nil {
260+
return false
261+
}
262+
resultP.Spec.Suspend = true
263+
if err := k8sClient.Update(context.Background(), resultP); err != nil {
264+
return false
265+
}
266+
return true
267+
}, timeout, time.Second).Should(BeTrue())
268+
191269
g.Eventually(func() bool {
192270
_ = k8sClient.Get(context.Background(), client.ObjectKeyFromObject(provider), resultP)
193271
return resultP.Spec.Suspend == true
@@ -201,3 +279,63 @@ func TestProviderReconciler_Reconcile(t *testing.T) {
201279
}, timeout, time.Second).Should(BeTrue())
202280
})
203281
}
282+
283+
func Test_parseURLs(t *testing.T) {
284+
tests := []struct {
285+
name string
286+
address string
287+
proxy string
288+
blockHTTP bool
289+
err error
290+
errMsg string
291+
}{
292+
{
293+
name: "valid address and proxy",
294+
address: "http://example.com",
295+
proxy: "http://proxy.com",
296+
},
297+
{
298+
name: "invalid address",
299+
address: "http//invalid",
300+
errMsg: "invalid address",
301+
},
302+
{
303+
name: "invalid proxy",
304+
address: "http://example.com",
305+
proxy: "http//invalid",
306+
errMsg: "invalid proxy",
307+
},
308+
{
309+
name: "block http proxy",
310+
address: "http://example.com",
311+
proxy: "http://proxy.com",
312+
blockHTTP: true,
313+
err: insecureHTTPError,
314+
errMsg: "consider changing proxy",
315+
},
316+
{
317+
name: "block http address",
318+
address: "http://example.com",
319+
blockHTTP: true,
320+
err: insecureHTTPError,
321+
errMsg: "consider changing address",
322+
},
323+
}
324+
325+
for _, tt := range tests {
326+
t.Run(tt.name, func(t *testing.T) {
327+
g := NewWithT(t)
328+
err := parseURLs(tt.address, tt.proxy, tt.blockHTTP)
329+
330+
if tt.errMsg == "" {
331+
g.Expect(err).ToNot(HaveOccurred())
332+
} else {
333+
g.Expect(err).To(HaveOccurred())
334+
g.Expect(err.Error()).To(ContainSubstring(tt.errMsg))
335+
}
336+
if tt.err != nil {
337+
g.Expect(err).To(MatchError(tt.err))
338+
}
339+
})
340+
}
341+
}

‎internal/controllers/suite_test.go‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -81,10 +81,11 @@ func TestMain(m *testing.M) {
8181
}
8282

8383
if err := (&ProviderReconciler{
84-
Client: testEnv,
85-
Metrics: testMetricsH,
86-
ControllerName: controllerName,
87-
EventRecorder: testEnv.GetEventRecorderFor(controllerName),
84+
Client: testEnv,
85+
Metrics: testMetricsH,
86+
ControllerName: controllerName,
87+
EventRecorder: testEnv.GetEventRecorderFor(controllerName),
88+
BlockInsecureHTTP: true,
8889
}).SetupWithManagerAndOptions(testEnv, ProviderReconcilerOptions{
8990
RateLimiter: controller.GetDefaultRateLimiter(),
9091
}); err != nil {

‎internal/notifier/alertmanager.go‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@ import (
2020
"context"
2121
"crypto/x509"
2222
"fmt"
23-
"net/url"
2423
"strings"
2524

2625
eventv1 "github.com/fluxcd/pkg/apis/event/v1beta1"
@@ -39,11 +38,6 @@ type AlertManagerAlert struct {
3938
}
4039

4140
func NewAlertmanager(hookURL string, proxyURL string, certPool *x509.CertPool) (*Alertmanager, error) {
42-
_, err := url.ParseRequestURI(hookURL)
43-
if err != nil {
44-
return nil, fmt.Errorf("invalid Alertmanager URL %s: '%w'", hookURL, err)
45-
}
46-
4741
return &Alertmanager{
4842
URL: hookURL,
4943
ProxyURL: proxyURL,

0 commit comments

Comments
 (0)