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.

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

Redact password #7198

Merged
merged 2 commits into from
Dec 18, 2020
Merged

Redact password #7198

merged 2 commits into from
Dec 18, 2020

Conversation

5antelope
Copy link
Member

Signed-off-by: crowu y.wu4515@gmail.com

Backport

YES

Status

READY

Description

If we do any SQL commend with password, Vitess will log it. And currently vitess only redact master password.

This PR tries to redact any password in the SQL query so that people won't see it as plain text in the log

Related Issue(s)

List related PRs against other branches:
This addresses #7197:

Todos

  • Tests
  • Documentation

Deployment Notes

NA

Impacted Areas in Vitess

List general components of the application that this PR will affect:

  • Query Serving
  • VReplication
  • Cluster Management
  • Build

Signed-off-by: crowu <y.wu4515@gmail.com>
@5antelope
Copy link
Member Author

Hm.. I don't think this change is responsible for the test failure here: https://github.com/vitessio/vitess/pull/7198/checks?check_run_id=1567275976

The change basically only change the string we add to log 😅 (?)

if j == -1 {
return input
}
input = input[:i+len(masterPasswordStart)] + strings.Repeat("*", j) + input[i+len(masterPasswordStart)+j:]
Copy link
Member

Choose a reason for hiding this comment

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

Why are we preserving the lenght of the password, should we not keep it a constant length? We do not want people to be able to infer the length either from the logs.
Otherwise, LGTM

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, good point - updated

@GuptaManan100 GuptaManan100 added Component: Query Serving P3 Type: Enhancement Logical improvement (somewhere between a bug and feature) labels Dec 17, 2020
Signed-off-by: crowu <y.wu4515@gmail.com>
@5antelope
Copy link
Member Author

Looks like failed tests are still unrelated to this change..

@deepthi
Copy link
Member

deepthi commented Dec 18, 2020

Looks like failed tests are still unrelated to this change..

Hopefully they are transient errors..

@5antelope
Copy link
Member Author

Looks like failed tests are still unrelated to this change..

Hopefully they are transient errors..

Looks like it's all cleared!

Copy link
Member

@deepthi deepthi left a comment

Choose a reason for hiding this comment

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

Nice work! Thank you for the contribution.

@deepthi deepthi merged commit 4647a4b into vitessio:master Dec 18, 2020
@5antelope 5antelope deleted the ywu/redact branch December 18, 2020 20:00
@askdba askdba added this to the v9.0 milestone Dec 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Query Serving Type: Enhancement Logical improvement (somewhere between a bug and feature)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants