Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

hubble: Conditionally redact user info present in URLs in (L7) HTTP flows #28848

Merged
merged 1 commit into from Nov 3, 2023
Merged

hubble: Conditionally redact user info present in URLs in (L7) HTTP flows #28848

merged 1 commit into from Nov 3, 2023

Conversation

ioandr
Copy link
Contributor

@ioandr ioandr commented Oct 27, 2023 •
edited by dylandreimerink

This PR deals with the first (and last remaining) item listed in #23887 (comment):

HTTP URL: sanitize user info part (ie the optional username and password details for a URL), on/off.

More specifically:

  • Add business logic to the (L7) HTTP parser to conditionally redact sensitive user info that is potentially present in observed URLs (e.g., a password that is used in basic authentication) based on Hubble's configuration.

  • Extend the Cilium CLI with the --hubble-redact-http-userinfo option and update Cilium's Helm chart, templates and docs as needed.

    hubble:
 ...
 redact:
   ...
   http:
     userInfo: true

  • Set the default value of this option to true to preserve existing functionality.

  • Extend test suite to check whether password observed in URL is redacted.

  • Fix issue in L7 HTTP parser where sensitive values were redacted in (L7) HTTP flows, but not in (L7) HTTP summaries.

NOTE

This PR is also related to #28798 where we investigate why the user:pass@ part of a URL is not observed by Hubble. To overcome this and test this PR for now, one can "short-circuit" Cilium's Envoy to always include the user:pass@ part in observed URLs by modifying https://github.com/cilium/cilium/blob/main/pkg/envoy/accesslog.go#L19 as follows:

- u, err := url.Parse(fmt.Sprintf("%s://%s/%s", scheme, host, strings.TrimPrefix(path, "/")))
+ u, err := url.Parse(fmt.Sprintf("%s://%s:%s@%s/%s", scheme, "user", "pass", host, strings.TrimPrefix(path, "/")))

Closes #23887

hubble: Conditionally redact user info present in URLs in (L7) HTTP flows

@maintainer-s-little-helper 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 Oct 27, 2023
@ioandr ioandr marked this pull request as ready for review October 27, 2023 19:37
@ioandr ioandr requested review from a team as code owners October 27, 2023 19:37
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Oct 27, 2023
tommyp1ckles
tommyp1ckles reviewed Oct 30, 2023
View reviewed changes
Copy link
Contributor

@tommyp1ckles tommyp1ckles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR 😄

pkg/hubble/parser/seven/http.go Outdated Show resolved Hide resolved
install/kubernetes/cilium/values.yaml.tmpl Show resolved Hide resolved
gandro
gandro approved these changes Oct 30, 2023
View reviewed changes
Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I second @tommyp1ckles'c concerns about un-idomatic variable names (_url vs url_), but besides that the changes look good to me.

pkg/hubble/parser/seven/http.go Outdated Show resolved Hide resolved
@dylandreimerink dylandreimerink added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Oct 30, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Oct 30, 2023
lambdanis
lambdanis requested changes Oct 30, 2023
View reviewed changes
Copy link
Contributor

@lambdanis lambdanis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One docs nit, and echoing @tommyp1ckles comment about the var names.

Documentation/observability/visibility.rst Outdated Show resolved Hide resolved
install/kubernetes/cilium/values.yaml.tmpl Show resolved Hide resolved
@tommyp1ckles
Copy link
Contributor

Everything looks good on my end, just waiting Re: moving the helm change to a separate PR.

marseel
marseel approved these changes Oct 31, 2023
View reviewed changes
Copy link
Contributor

@marseel marseel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good

lambdanis
lambdanis requested changes Oct 31, 2023
View reviewed changes
Copy link
Contributor

@lambdanis lambdanis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks @ioandr. Can you squash the fixups into a single commit?

@ioandr
hubble: Add config option to redact user info in L7 flows
11b31b0 
Add business logic to L7 HTTP parser to conditionally redact sensitive
user info (e.g., password used in basic authentication) when present
in observed URLs.

* Add the '--hubble-redact-http-userinfo' option to the Cilium CLI.
  Preserve existing functionality by setting it to true by default.
* Add unit tests to verify that password in observed URL is redacted.
* Fix issue in L7 HTTP parser where sensitive values were redacted in
  (L7) HTTP flows, but not in (L7) HTTP summaries.
* Update documentation as needed.
* Update Helm chart templates, values and docs as needed.

Closes cilium#23887

Signed-off-by: Ioannis Androulidakis <androulidakis.ioannis@gmail.com>
@ioandr
Copy link
Contributor Author

ioandr commented Oct 31, 2023

Looks good, thanks @ioandr. Can you squash the fixups into a single commit?

Sure, will do. Thanks!

lambdanis
lambdanis approved these changes Oct 31, 2023
View reviewed changes
Copy link
Contributor

@lambdanis lambdanis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

@lambdanis
Copy link
Contributor

/test

@ioandr
Copy link
Contributor Author

ioandr commented Oct 31, 2023

@asauber kind reminder for another approval here 🤗

@aanm aanm merged commit 1a0553c into cilium:main Nov 3, 2023
61 of 62 checks passed
@rolinh rolinh mentioned this pull request Nov 3, 2023
Closed
5 tasks
@ioandr ioandr deleted the hubble-redact-userinfo branch November 3, 2023 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gandro gandro gandro approved these changes

@ChrsMark ChrsMark ChrsMark left review comments

@marseel marseel marseel approved these changes

@lambdanis lambdanis lambdanis approved these changes

@christarazi christarazi christarazi approved these changes

@aanm aanm aanm approved these changes

@tommyp1ckles tommyp1ckles tommyp1ckles approved these changes

@asauber asauber Awaiting requested review from asauber asauber is a code owner automatically assigned from cilium/cli

Assignees
No one assigned
Labels
kind/community-contribution This was a contribution made by a community member. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Hubble: add option to sanitize sensitive L7 data from flows
9 participants
@ioandr @tommyp1ckles @lambdanis @gandro @marseel @christarazi @aanm @ChrsMark @dylandreimerink