Hubble: improve security by adding an option to redact API key in Kafka requests (L7) #25844

ioandr · 2023-06-01T22:42:51Z

Add option to redact API key in Kafka requests.

TODOS

Depends on: #25746

kaworu

Thanks for the PR @ioandr, patch LGTM.

I would like to see a new test ensuring that KafkaRedactAPIKey is honored. Also although a breaking change I think it should be true by default cc @rolinh.

rolinh

Thanks for the patch! Unfortunately, we can't mutate LogRecord structures so the patch needs to be updated (see comment below).

Also although a breaking change I think it should be true by default cc @rolinh.

Mmh, I'm not sure about that. If L7 visibility was an opt-out feature, then I'd clearly say yes, Hubble should redact Kafka's API keys by default. However, L7 visibility is opt-in and its (security) implications should be well understood when enabling it (note: we should probably update the documentation to make that clear). My point is that even if we try our best to redact sensitive data, there's no guarantee that Hubble will always correctly redact all sensitive data from L7 payloads. Therefore, L7 flows should be treated as sensitive data overall. I also think that if we redact (known) sensitive data by default, then that means we should do it overall which would imply that we also redact all HTTP headers by default and I'm not sure we would want that.

pkg/hubble/parser/seven/kafka.go

ioandr · 2023-06-05T22:09:39Z

I would like to see a new test ensuring that KafkaRedactAPIKey is honored.

Hi @kaworu, thanks for taking a look at this!

I pushed a commit that updates the Kafka unit tests to cover the Kafka API key redact option. LMK if this is what you had in mind.

kaworu · 2023-06-06T07:11:04Z

@rolinh generally agree with all that you wrote

I also think that if we redact (known) sensitive data by default, then that means we should do it overall which would imply that we also redact all HTTP headers by default and I'm not sure we would want that.

HTTP headers may or may not be sensitive data, while I think we can agree basic auth password and Kafka API key always are. We're already redacting the former unconditionally, so in my opinion doing it by default for the latter would be consistent. Not feeling too strongly about this though.

kaworu

Thanks @ioandr for the quick update! The patch LGTM besides the missing doc update 🚀

ioandr · 2023-06-06T21:43:10Z

Thanks @ioandr for the quick update! The patch LGTM besides the missing doc

Sure, I can prepare a short doc as discussed above.

However the following is not 100% clear to me:

How are we planning to expose this option to end-users? Do we want end-users to control this option via the cilium CLI? i.e.,

$ cilium config set hubble-l7-parser-kafka-redact-api-key true/false

IIUC the above command directly updates the kube-system/cilium-config ConfigMap.

We can also add a new argument in the cilium-agent CLI. Looking at the codebase, this requires the following:

Extend Cilium https://github.com/cilium/cilium/blob/main/pkg/option/config.go with an option to control whether Hubble's L7 parser redacts Kafka API keys - we already declare Hubble* options there
Set a sane default value for the newly added Hubble L7 option under https://github.com/cilium/cilium/blob/main/pkg/defaults/defaults.go
Pass context to the L7 payload parser upon creation: https://github.com/cilium/cilium/blob/main/daemon/cmd/hubble.go#L141
Initialize flag in https://github.com/cilium/cilium/blob/main/daemon/cmd/daemon_main.go
Possibly update Cilium's Helm chart, so that Cilium's ConfigMap contains this option
Update user-facing files as neededHubbleMetrics (e.g., README, Helm Reference, etc.)

This is what @ChrsMark has done in his PR that tackles the redaction of HTTP query params, so maybe we want a uniform approach in both. PTAL: https://github.com/cilium/cilium/pull/25746/files

CC @kaworu @rolinh

ChrsMark · 2023-06-20T19:27:59Z

Hey @pippolo84 @nathanjsweet @asauber @aditighag any chances having a look into this one? It should be a simple one 🙏🏽. Despite being labeled with dont-merge/wait-until-release it would be great if can have it reviewed and ready for merge :).

zacharysarah

LGTM

pippolo84

Thanks! 💯

jrajahalme

LGTM

jrajahalme · 2023-06-21T12:28:04Z

@kaworu Looks like your concern was addressed?

kaworu

Thanks for the update @ioandr patch LGMT!

@kaworu Looks like your concern was addressed?

Yes, although I still find it inconsistent to redact the HTTP password unconditionally while the Kafka API key redaction is opt-in, but that's arguably out of the scope of this PR.

ioandr · 2023-06-21T12:59:46Z

Yes, although I still find it inconsistent to redact the HTTP password unconditionally while the Kafka API key redaction is opt-in, but that's arguably out of the scope of this PR.

Indeed, we can address this with a small, dedicated issue/PR that will make HTTP password redaction in L7 flows configurable, similar to what we did for URL query parameters and Kafka API key.

Redaction of HTTP password was already implemented and non-configurable. As such, we avoided this refactoring to keep our PRs small and concise.

Thanks for the review @kaworu

kaworu · 2023-06-21T13:03:00Z

/test

ChrsMark · 2023-06-21T13:10:02Z

+1 on providing a follow-up for --hubble-redact=http-basic-auth-password which will be true by default :).

ldelossa · 2023-06-27T13:07:32Z

@ioandr, Can you please rebase this PR on main and push? Should fix some issues with the tests.

ioandr · 2023-06-27T13:52:29Z

@ldelossa the only test that is failing is the mergeability one - I suppose due to the dont-merge/wait-until-release label that prevents merge.

LMK if you need any further action from my side.

kaworu · 2023-06-27T13:57:14Z

/test

asauber

LGTM, my main feedback here would be that we should list the "allowed" redacted values more clearly in the docs; make it clear that right now there are exactly two options and what they are.

ioandr · 2023-06-28T16:17:03Z

LGTM, my main feedback here would be that we should list the "allowed" redacted values more clearly in the docs; make it clear that right now there are exactly two options and what they are.

Hi @asauber, thank you for your feedback on this. How about we rephrase the doc as follows:

To harden security, Cilium provides the ``--hubble-redact`` option which
accepts a comma-separated list of values that control how Hubble handles
sensitive information present in Layer 7 flows. More specifically, it offers
the following features and values for supported Layer 7 protocols:

* For HTTP: redacting URL query (GET) parameters (``--hubble-redact=http-url-query``)
* For Kafka: redacting API key (``--hubble-redact=kafka-api-key``)

asauber · 2023-06-28T20:59:34Z

@ioandr That phrasing looks good to me

Co-authored-by: ChrsMark <chrismarkou92@gmail.com> Signed-off-by: Ioannis Androulidakis <androulidakis.ioannis@gmail.com>

* Extend accepted values for the `--hubble-redact` CLI option * Add unit tests for Kafka in L7 parser * Update the `visibility.rst` document * Update Helm chart templates and values Co-authored-by: ChrsMark <chrismarkou92@gmail.com> Signed-off-by: Ioannis Androulidakis <androulidakis.ioannis@gmail.com>

ldelossa · 2023-06-29T15:13:34Z

/test

ioandr · 2023-06-30T07:21:23Z

@ldelossa I see that you removed the dont-merge/wait-until-release label so I suppose there is no blocker towards merging this.

I see that one test is failing with:

connectivity test failed: timeout reached waiting lookup for localhost from pod cilium-test/client-6965d549d5-mjxk9 to server on pod cilium-test/echo-other-node-545c9b778b-m6d6t to succeed (last error: context deadline exceeded)
Error: Process completed with exit code 1.

I believe this is something temporary/flaky, that is, the test failure is not caused by the changes of this PR. Maybe we can re-trigger tests?

kaworu · 2023-06-30T09:05:55Z

/ci-multicluster
EDIT: previous CI run hit #26513

ioandr requested a review from a team as a code owner June 1, 2023 22:42

ioandr requested a review from kaworu June 1, 2023 22:42

maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jun 1, 2023

github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Jun 1, 2023

ioandr closed this Jun 1, 2023

ioandr reopened this Jun 1, 2023

ioandr marked this pull request as draft June 1, 2023 22:46

kaworu requested changes Jun 5, 2023

View reviewed changes

kaworu added kind/feature This introduces new functionality. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. sig/hubble Impacts hubble server or relay labels Jun 5, 2023

maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Jun 5, 2023

rolinh requested changes Jun 5, 2023

View reviewed changes

pkg/hubble/parser/seven/kafka.go Outdated Show resolved Hide resolved

ioandr force-pushed the feature-hubble-kafka-redact branch from 00e1df7 to f18bf51 Compare June 5, 2023 22:01

ioandr force-pushed the feature-hubble-kafka-redact branch from f18bf51 to edcadad Compare June 5, 2023 22:29

kaworu approved these changes Jun 6, 2023

View reviewed changes

ChrsMark mentioned this pull request Jun 7, 2023

Hubble: Add option to remove query from HTTP flows #25746

Merged

2 tasks

ioandr force-pushed the feature-hubble-kafka-redact branch 2 times, most recently from 68ea860 to 086d452 Compare June 9, 2023 08:58

ioandr marked this pull request as ready for review June 9, 2023 08:58

ioandr requested review from a team as code owners June 9, 2023 08:58

zacharysarah approved these changes Jun 21, 2023

View reviewed changes

pippolo84 approved these changes Jun 21, 2023

View reviewed changes

jrajahalme approved these changes Jun 21, 2023

View reviewed changes

kaworu approved these changes Jun 21, 2023

View reviewed changes

ioandr force-pushed the feature-hubble-kafka-redact branch from 51cb34f to 91929a2 Compare June 27, 2023 13:16

asauber approved these changes Jun 27, 2023

View reviewed changes

nathanjsweet approved these changes Jun 28, 2023

View reviewed changes

ioandr and others added 2 commits June 29, 2023 02:05

hubble: Introduce default value for redaction

a330d76

Co-authored-by: ChrsMark <chrismarkou92@gmail.com> Signed-off-by: Ioannis Androulidakis <androulidakis.ioannis@gmail.com>

ioandr force-pushed the feature-hubble-kafka-redact branch from 91929a2 to 675f79e Compare June 28, 2023 23:05

ldelossa removed the dont-merge/wait-until-release Freeze window for current release is blocking non-bugfix PRs label Jun 29, 2023

maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 30, 2023

ldelossa merged commit 386b17a into cilium:main Jun 30, 2023
65 checks passed

ioandr deleted the feature-hubble-kafka-redact branch June 30, 2023 14:03

ioandr restored the feature-hubble-kafka-redact branch June 30, 2023 14:03

ioandr deleted the feature-hubble-kafka-redact branch June 30, 2023 14:04

ChrsMark mentioned this pull request Aug 8, 2023

Refactor hubble redact settings schema #26989

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hubble: improve security by adding an option to redact API key in Kafka requests (L7) #25844

Hubble: improve security by adding an option to redact API key in Kafka requests (L7) #25844

ioandr commented Jun 1, 2023 •

edited

kaworu left a comment

rolinh left a comment

ioandr commented Jun 5, 2023 •

edited

kaworu commented Jun 6, 2023

kaworu left a comment

ioandr commented Jun 6, 2023 •

edited

ChrsMark commented Jun 20, 2023

zacharysarah left a comment

pippolo84 left a comment

jrajahalme left a comment

jrajahalme commented Jun 21, 2023

kaworu left a comment

ioandr commented Jun 21, 2023

kaworu commented Jun 21, 2023

ChrsMark commented Jun 21, 2023

ldelossa commented Jun 27, 2023

ioandr commented Jun 27, 2023

kaworu commented Jun 27, 2023

asauber left a comment

ioandr commented Jun 28, 2023 •

edited

asauber commented Jun 28, 2023

ldelossa commented Jun 29, 2023

ioandr commented Jun 30, 2023

kaworu commented Jun 30, 2023 •

edited

Hubble: improve security by adding an option to redact API key in Kafka requests (L7) #25844

Hubble: improve security by adding an option to redact API key in Kafka requests (L7) #25844

Conversation

ioandr commented Jun 1, 2023 • edited

TODOS

kaworu left a comment

Choose a reason for hiding this comment

rolinh left a comment

Choose a reason for hiding this comment

ioandr commented Jun 5, 2023 • edited

kaworu commented Jun 6, 2023

kaworu left a comment

Choose a reason for hiding this comment

ioandr commented Jun 6, 2023 • edited

ChrsMark commented Jun 20, 2023

zacharysarah left a comment

Choose a reason for hiding this comment

pippolo84 left a comment

Choose a reason for hiding this comment

jrajahalme left a comment

Choose a reason for hiding this comment

jrajahalme commented Jun 21, 2023

kaworu left a comment

Choose a reason for hiding this comment

ioandr commented Jun 21, 2023

kaworu commented Jun 21, 2023

ChrsMark commented Jun 21, 2023

ldelossa commented Jun 27, 2023

ioandr commented Jun 27, 2023

kaworu commented Jun 27, 2023

asauber left a comment

Choose a reason for hiding this comment

ioandr commented Jun 28, 2023 • edited

asauber commented Jun 28, 2023

ldelossa commented Jun 29, 2023

ioandr commented Jun 30, 2023

kaworu commented Jun 30, 2023 • edited

ioandr commented Jun 1, 2023 •

edited

ioandr commented Jun 5, 2023 •

edited

ioandr commented Jun 6, 2023 •

edited

ioandr commented Jun 28, 2023 •

edited

kaworu commented Jun 30, 2023 •

edited