Add flag to enforce mTLS on hubble relay clients

[marqc]

Fixes: #24265

Please ensure your pull request adheres to the following guidelines:

• For first time contributors, read Submitting a pull request
• All code is covered by unit and/or runtime tests where feasible.
• All commits contain a well written commit description including a title,
  description and a Fixes: #XXX line if the commit addresses a particular
  GitHub issue.
• If your commit description contains a Fixes: <commit-id> tag, then
  please add the commit author[s] as reviewer[s] to this issue.
• All commits are signed off. See the section Developer’s Certificate of Origin
• Provide a title or release-note blurb suitable for the release notes.
• Are you a user of Cilium? Please add yourself to the Users doc
• Thanks for contributing!

For some time hubble-relay clients (hubble CLI, hubble UI) support mTLS. This change adds option to enforce mTLS on connections to hubble-relay.
Fixes: 24265

Add tls-server-enforce-mtls flag to hubble-relay to enforce mTLS connection with clients.

[kaworu]

Thanks for the PR @marqc. I think we want a new flag for the CA used to validate Hubble Relay client connections. On the way I think we should rename some flags for consistency:

• tls-client-cert-file → tls-hubble-client-cert-file
• tls-client-key-file → tls-hubble-client-key-file

To clarify that this keypair is used to contact Hubble servers.

• tls-server-cert-file → tls-relay-server-cert-file
• tls-server-key-file → tls-relay-server-key-file

To clarify that this keypair is used by the Hubble Relay server. Then the new flag could be named tls-relay-client-ca-files for consistency. While technically a breaking change, the flags renaming should be transparently handled by the Helm charts, so unlikely to affect most users. What do you think? cc @rolinh

[marqc]

Thanks for the PR @marqc. I think we want a new flag for the CA used to validate Hubble Relay client connections. On the way I think we should rename some flags for consistency:

• tls-client-cert-file → tls-hubble-client-cert-file
• tls-client-key-file → tls-hubble-client-key-file

To clarify that this keypair is used to contact Hubble servers.

• tls-server-cert-file → tls-relay-server-cert-file
• tls-server-key-file → tls-relay-server-key-file

To clarify that this keypair is used by the Hubble Relay server. Then the new flag could be named tls-relay-client-ca-files for consistency. While technically a breaking change, the flags renaming should be transparently handled by the Helm charts, so unlikely to affect most users. What do you think? cc @rolinh

@kaworu For better backwards compatibility I implemented old flag deprecation instead of just renaming.

The complete set of flags after these changes:

./hubble-relay help serve
Run the gRPC proxy server.

Usage:
  hubble-relay serve [flags]

Flags:
      --cluster-name string                  Name of the current cluster (default "default")
      --dial-timeout duration                Dial timeout when connecting to hubble peers (default 5s)
      --disable-client-tls                   Disable (m)TLS and allow the connection to Hubble server instances to be over plaintext.
      --disable-server-tls                   Disable TLS for the server and allow clients to connect over plaintext.
      --gops                                 Run gops agent (default true)
      --gops-port int                        Port for gops server to listen on (default 9893)
  -h, --help                                 help for serve
      --listen-address string                Address on which to listen (default ":4245")
      --metrics-listen-address string        Address on which to listen for metrics
      --peer-service string                  Address of the server that implements the peer gRPC service (default "unix:///var/run/cilium/hubble.sock")
      --pprof                                Enable serving the pprof debugging API
      --pprof-address string                 Address that pprof listens on (default "localhost")
      --pprof-port int                       Port that pprof listens on (default 6062)
      --retry-timeout duration               Time to wait before attempting to reconnect to a hubble peer when the connection is lost (default 30s)
      --sort-buffer-drain-timeout duration   When the per-request flows sort buffer is not full, a flow is drained every time this timeout is reached (only affects requests in follow-mode) (default 1s)
      --sort-buffer-len-max int              Max number of flows that can be buffered for sorting before being sent to the client (per request) (default 100)
      --tls-hubble-client-cert-file string   Path to the public key file for the client certificate to connect to Hubble server instances. The file must contain PEM encoded data.
      --tls-hubble-client-key-file string    Path to the private key file for the client certificate to connect to Hubble server instances. The file must contain PEM encoded data.
      --tls-hubble-server-ca-files strings   Paths to one or more public key files of the CA which sign certificates for Hubble server instances.
      --tls-relay-client-ca-files strings    Paths to one or more public key files of the CA which sign certificates for Hubble relay client instances.
      --tls-relay-server-cert-file string    Path to the public key file for the Hubble Relay server. The file must contain PEM encoded data.
      --tls-relay-server-key-file string     Path to the private key file for the Hubble Relay server. The file must contain PEM encoded data.
      --tls-server-enforce-mtls              Enforces clients to use mTLS. This option requires providing CA files in tls-hubble-server-ca-files

Global Flags:
  -D, --debug   Enable debug messages

Are you sure that the new flag should be tls-relay-client-ca-files and not tls-relay-server-ca-files?

[kaworu]

Thanks for the update @marqc!

Are you sure that the new flag should be tls-relay-client-ca-files and not tls-relay-server-ca-files?

Yes, it convey that the CA is used to validate clients, this is consistent with

cilium/pkg/option/config.go

Lines 946 to 949 in 2a966f4

	// HubbleTLSClientCAFiles specifies the path to one or more client CA
	// certificates to use for TLS with mutual authentication (mTLS). The files
	// must contain PEM encoded data.
	HubbleTLSClientCAFiles = "hubble-tls-client-ca-files"

@kaworu For better backwards compatibility I implemented old flag deprecation instead of just renaming.

Awesome, could you please add a note in Documentation/operations/upgrade.rst about the flag deprecation and planned removal for v1.15 (in the Deprecated Options section)? Other than that LGTM.

[marqc]

@kaworu

Awesome, could you please add a note in Documentation/operations/upgrade.rst about the flag deprecation and planned removal for v1.15 (in the Deprecated Options section)? Other than that LGTM.

Documentation updated.

[zacharysarah]

@marqc Some changes required, otherwise LGTM.

[kaworu]

Thanks @marqc! Patch LGTM, however CI is unhappy:

HINT: to fix this, run 'make -C Documentation update-helm-values'

[marqc]

Thanks @marqc! Patch LGTM, however CI is unhappy:

HINT: to fix this, run 'make -C Documentation update-helm-values'

thanks, fixed

[rolinh]

LGTM, thanks!

[ti-mo]

/test

[ti-mo]

@marqc Thanks, nice work! I've kicked off tests and put this on the queue to revisit after the 1.14 release branch has been cut.

[ldelossa]

@marqc I think you know the deal by now ;), needs a rebase and a push back up. That will fix the two stuck tests you see in this PR.

[ldelossa]

/test

[kaworu]

/test

[ldelossa]

hmm, not sure why travisCI tests are getting blocked.

[ldelossa]

@marqc

Sorry to ask you to do this, but it appears all your PRs are having trouble triggering TravisCI.
I tried to see if I can open a PR myself with your fork, but GH does not recognize your fork when we attempt to 'compare across forks'.

I'm going to leave this message on your other PRs as well, but can you please close this PR and open a new one with the same changes? Hopefully this will fix these issues.

[squeed]

I've manually run the Travis tests (make && make integration-tests) and everything seems fine. I'll try and get this merged.