Skip to content

Commit e0df51d

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 56a77b7 commit e0df51d

File tree

16 files changed

+235
-89
lines changed

16 files changed

+235
-89
lines changed

‎internal/controller/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 disabled 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/controller/provider_controller.go‎

Lines changed: 57 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ package controller
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 {
@@ -154,19 +160,33 @@ func (r *ProviderReconciler) reconcile(ctx context.Context, obj *apiv1beta2.Prov
154160

155161
// Mark the reconciliation as stalled if the inline URL and/or proxy are invalid.
156162
if err := r.validateURLs(obj); err != nil {
157-
conditions.MarkFalse(obj, meta.ReadyCondition, meta.InvalidURLReason, err.Error())
158-
conditions.MarkTrue(obj, meta.StalledCondition, meta.InvalidURLReason, err.Error())
163+
var reason string
164+
if errors.Is(err, insecureHTTPError) {
165+
reason = meta.InsecureConnectionsDisallowedReason
166+
} else {
167+
reason = meta.InvalidURLReason
168+
}
169+
conditions.MarkFalse(obj, meta.ReadyCondition, reason, err.Error())
170+
conditions.MarkTrue(obj, meta.StalledCondition, reason, err.Error())
159171
return ctrl.Result{Requeue: true}, err
160172
}
161173

162174
// Validate the provider credentials.
163175
if err := r.validateCredentials(ctx, obj); err != nil {
164-
conditions.MarkFalse(obj, meta.ReadyCondition, apiv1.ValidationFailedReason, err.Error())
176+
var reason string
177+
var urlErr *url.Error
178+
if errors.Is(err, insecureHTTPError) {
179+
reason = meta.InsecureConnectionsDisallowedReason
180+
} else if errors.As(err, &urlErr) {
181+
reason = meta.InvalidURLReason
182+
} else {
183+
reason = apiv1.ValidationFailedReason
184+
}
185+
conditions.MarkFalse(obj, meta.ReadyCondition, reason, err.Error())
165186
return ctrl.Result{Requeue: true}, err
166187
}
167188

168189
conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, apiv1.InitializedReason)
169-
170190
return ctrl.Result{RequeueAfter: obj.GetInterval()}, nil
171191
}
172192

@@ -175,12 +195,7 @@ func (r *ProviderReconciler) validateURLs(provider *apiv1beta2.Provider) error {
175195
proxy := provider.Spec.Proxy
176196

177197
if provider.Spec.SecretRef == nil {
178-
if _, err := url.ParseRequestURI(address); err != nil {
179-
return fmt.Errorf("invalid address %s: %w", address, err)
180-
}
181-
if _, err := url.ParseRequestURI(proxy); proxy != "" && err != nil {
182-
return fmt.Errorf("invalid proxy %s: %w", proxy, err)
183-
}
198+
return parseURLs(address, proxy, r.BlockInsecureHTTP)
184199
}
185200
return nil
186201
}
@@ -193,6 +208,11 @@ func (r *ProviderReconciler) validateCredentials(ctx context.Context, provider *
193208
token := ""
194209
headers := make(map[string]string)
195210
if provider.Spec.SecretRef != nil {
211+
// since a secret ref is provided, the object is not stalled even if spec.address
212+
// or spec.proxy are invalid, as the secret can change any time independently.
213+
if conditions.IsStalled(provider) {
214+
conditions.Delete(provider, meta.StalledCondition)
215+
}
196216
var secret corev1.Secret
197217
secretName := types.NamespacedName{Namespace: provider.Namespace, Name: provider.Spec.SecretRef.Name}
198218

@@ -253,6 +273,10 @@ func (r *ProviderReconciler) validateCredentials(ctx context.Context, provider *
253273
}
254274
}
255275

276+
if err := parseURLs(address, proxy, r.BlockInsecureHTTP); err != nil {
277+
return err
278+
}
279+
256280
factory := notifier.NewFactory(address, proxy, username, provider.Spec.Channel, token, headers, certPool, password, string(provider.UID))
257281
if _, err := factory.Notifier(provider.Spec.Type); err != nil {
258282
return fmt.Errorf("failed to initialize provider, error: %w", err)
@@ -316,3 +340,25 @@ func (r *ProviderReconciler) patch(ctx context.Context, obj *apiv1beta2.Provider
316340

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

‎internal/controller/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/controller/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)