RFC-0004: Block insecure HTTP connections across Flux

[aryan9600]

Proposal for blocking HTTP connections across all Flux controllers and objects

TLDR:

• A new boolean flag --insecure-allow-http shall be added to all controllers, which would be used to allow/disallow HTTP connections at the controller level. The default value is true, which allows HTTP connections
• A new boolean field .spec.insecure shall be added to all relevant objects where the specified endpoint doesn't allow for inference of the transport type through the scheme and HTTP connections must be used.
• A new boolean flag --insecure shall be added to relevant CLI commands, to configure the value of .spec.insecure in the related object definition.

Sponsored by @pjbgf

Fixes #3076

[stefanprodan]

Please document the use of insecure for Alert Providers, ref: fluxcd/notification-controller#404

Besides adding the flag to notification-controller, I guess the Provider CRD should have the insecure field as well? If so which ones?

[pjbgf]

As food for thought, what if instead of the new controller level flag being named --allow-insecure-http we reused the terminology from the CLI: --insecure-skip-tls-verify? There is a different in meaning, but for non-http schemes, they align more nicely.

[aryan9600]

@pjbgf from what I understand, the --insecure-skip-tls-verify is used to skip checking the validity of the server's cert, but you can still access an endpoint using an HTTPS connection. I fail to see how that aligns with --allow-insecure-http which dictates whether a controller can access an endpoint using an HTTP connection.

[stealthybox]

"skip-tls-verify=true" would only suggest relaxing TLS behavior.

It doesn't mention and should not relate to blocking/allowing plain HTTP.

"force-https=false" (with a default to true) might make more sense both disabling TLS verification and permitting HTTP since it at least mentions the protocol.
I don't like this option because plain HTTP and unverified HTTPS are still inherently different. Unverified HTTP is still encrypted. Plain HTTP is not which makes a huge difference securing against third-parties on the wire.

As an SRE/ops person, I would just prefer 2 (or more) flags to control TLS verification and HTTP separately:

--insecure-skip-tls-verify
--insecure-allow-http

I would put the word "insecure" at the front, so the flags get grouped with other sensitive options.

If you need more flags to differentiate Kustomize remote base fetches from k8s API connections, that seems fine, but maybe not necessary.

I do think this feature is incomplete until it addresses how to get Kustomize to filter out HTTP.
Disabling remote bases completely seems like a temporary workaround if we want this level of configurability at the protocol layer.

Hopefully the Kustomize folks would be open to working this in?

[stefanprodan]

Remote bases bypass source-controller so they break many of Flux promises: as we don’t verify nor do we store the remote sources in-cluster as Flux artifacts. Pulling remote bases at every apply makes Flux unpredictable, for example: if external traffic is jammed, Flux can’t detect & correct drift as the last desired state is unknown. Tracing an object to its source is no longer accurate, diffing and any static analysis involves outbound traffic. And finally, there is no API to setup SSH or TLS certs for remote bases, so for private repos you’ll need to mount the private keys and certs in the kustomize-controller pod (breaks multi-tenancy).

I think Flux should encourage users into using sources which can be verified and trusted. Ideally all the external artifacts which make the desired state should by signed and transported securely, so no remote bases and no insecure protocols. Maybe with OCI we can make this story more appealing, flux build artifact could vendor the remote bases and flux push artifact could sign them.

[aryan9600]

I think whether we should advice Flux users to avoid using remote bases is outside the crux of this proposal. The allow-insecure-http flag is meant to disable any usage of HTTP connections despite of whether the feature itself is unsafe. But since we can't feasibly filter out HTTP connections to remote bases, I think the safe thing to do is not add this flag to kustomize-controller. The proposal can also be modified to have a note on why this flag has been excluded from kustomize-controller to clear any doubts over its absence. If at a later time, upstream Kustomize adds support for disabling access to remote bases served over HTTP, we can add this flag to kustomize-controller then.

[stefanprodan]

@aryan9600 please setup spellcheck in your editor

[stefanprodan]

LGTM

[pjbgf]

I agree with @stealthybox on the flag name, insecure should be used as a prefix like we do for the kubeconfig flags:

	flagInsecureKubeConfigExec = "insecure-kubeconfig-exec"
	flagInsecureKubeConfigTLS  = "insecure-kubeconfig-tls"

https://github.com/fluxcd/pkg/blob/87d4c63c2160c70f8557ad4dd7f2dacdf4e613dc/runtime/client/kubeconfig.go#L27-L28

[stefanprodan]

LGTM

[pjbgf]

LGTM!

Great job putting this together @aryan9600! 🙇

[stefanprodan]

I'm assigning RFC-0004 for this one, @aryan9600 please add Last update to today, and mark the PR ready.

[stefanprodan]

One more changed need, please set the status to implementable

[makkes]

I love how this RFC is aligning controller flags, CLI flags and CRD fields. Thanks!

One concern I have is the rather generic term insecure in both, the CRDs and the CLI as it's just not specific enough in my book. Calling it insecure-allow-http would be much better to understand from a user's perspective. There's already been lots of confusion in the Helm project around what "insecure" really means.

[darkowlzz]

LGTM!

[pjbgf]

@makkes I concur that --insecure is too generic, and I would also support the CLI to have a more explicit and meaningful flag name instead. This would decrease confusion if we were to introduce other --insecure-* flags in the future, which I believe is quite likely.

As for the CRD, although I agree that the same argument applies, I am not sure the trade-off of breaking backwards compatibility vs adding a more meaningful name is worth it on that case. But if that were to happen, we probably better do it ahead of GA. What are your thoughts here?

Do you think there would be other variants within our CRDs that contain a spec.Insecure field?