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

Shorten the --log-http Authorization mask value #484

Merged
merged 1 commit into from
Nov 18, 2019
Merged

Shorten the --log-http Authorization mask value #484

merged 1 commit into from
Nov 18, 2019

Conversation

daisy-ycguo
Copy link
Member

@daisy-ycguo daisy-ycguo commented Nov 6, 2019 •
edited
Loading

Fixes #391

Proposed Changes

  • display --log-http Authorization as ********

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Nov 6, 2019
@knative-prow-robot knative-prow-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Nov 6, 2019
@daisy-ycguo
Copy link
Member Author

/retest

maximilien
maximilien suggested changes Nov 6, 2019
View reviewed changes
Copy link
Contributor

@maximilien maximilien left a comment

Choose a reason for hiding this comment

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

Why has the test for it not changed? If there is none, perhaps adding one?

Thanks for contribution.

@daisy-ycguo
Copy link
Member Author

@maximilien Thanks for review. Test added.

@maximilien
Copy link
Contributor

maximilien commented Nov 8, 2019 •
edited
Loading

/lgtm

/hold just to see if others have comments. If none by tomorrow, I’ll remove hold

@knative-prow-robot knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 8, 2019
@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 8, 2019
navidshaikh
navidshaikh reviewed Nov 8, 2019
View reviewed changes
Copy link
Collaborator

@navidshaikh navidshaikh left a comment

Choose a reason for hiding this comment

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

/lgtm
/hold cancel

@knative-prow-robot knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 8, 2019
@maximilien
Copy link
Contributor

/lgtm

rhuss
rhuss reviewed Nov 11, 2019
View reviewed changes
pkg/util/logging_http_transport.go Outdated
l += len(h)
}
r.Header.Set(k, strings.Repeat("*", l))
r.Header.Set(k, "(REDACTED FOR PRIVACY)")
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really have to be that verbose? I would be happy to just show 8 asterisks ("**********"). This seems to be a common practice when it comes to erasing passwords.

Why for 'privacy' ? And not for 'security' ? Is it because the password reveals something personal ?. And "redacted" means to be changed based on the value, but it's just "overwritten" without condition, not "redacted".

Nit-picking on high-level, sorry ;-) Just saying, that I really would prefer a simple commonly used, non-sense value like "**********", as, because it's non-sense, nobody questions its sense ;-)

If my comment goes over the top, feel free to merge nevertheless.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we just need to agree on the verbose to show instead of the real authorization string.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's do a quick vote.

Copy link
Contributor

Choose a reason for hiding this comment

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

Option 1:(REDACTED FOR PRIVACY)

Copy link
Contributor

Choose a reason for hiding this comment

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

Option 2: ********

Copy link
Contributor

Choose a reason for hiding this comment

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

In my first version of this PR it was "ELIDED", and people were like "what does ELIDED even mean?" so I think asterisks are fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

WG meeting approves the asterisks.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will update today. Thank you all.

@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 13, 2019
@daisy-ycguo
Copy link
Member Author

@rhuss @maximilien I updated based on comments. It's ready for review. Thank you.

@knative-metrics-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-knative-client-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/util/logging_http_transport.go 86.7% 85.2% -1.5

@daisy-ycguo daisy-ycguo mentioned this pull request Nov 13, 2019
Closed
@navidshaikh
Copy link
Collaborator

/retest

navidshaikh
navidshaikh approved these changes Nov 15, 2019
View reviewed changes
Copy link
Collaborator

@navidshaikh navidshaikh left a comment

Choose a reason for hiding this comment

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

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 15, 2019
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: daisy-ycguo, navidshaikh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 15, 2019
@daisy-ycguo
Copy link
Member Author

/retest

@knative-prow-robot knative-prow-robot merged commit d116fb9 into knative:master Nov 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sixolet sixolet sixolet left review comments

@rhuss rhuss rhuss left review comments

@maximilien maximilien maximilien requested changes

@navidshaikh navidshaikh navidshaikh approved these changes

@cppforlife cppforlife Awaiting requested review from cppforlife

@mattmoor mattmoor Awaiting requested review from mattmoor

Assignees

@maximilien maximilien

@navidshaikh navidshaikh

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

shorten the --log-http Authorization mask value
8 participants
@daisy-ycguo @maximilien @knative-metrics-robot @navidshaikh @knative-prow-robot @rhuss @sixolet @googlebot