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

Do not log secret in log #82

Closed
karataliu opened this issue Apr 26, 2018 · 3 comments
Closed

Do not log secret in log #82

karataliu opened this issue Apr 26, 2018 · 3 comments

Comments

@karataliu
Copy link

https://github.com/kubernetes-csi/external-provisioner/blob/v0.2.1/pkg/controller/controller.go#L89

This line will log 'controller_create_secrets' if present
@shubheksha
Copy link

I'll work on this!

@shubheksha shubheksha mentioned this issue May 15, 2018
Closed
@pohly pohly mentioned this issue May 15, 2018
Closed
@pohly
Copy link
Contributor

pohly commented May 15, 2018

Instead of changing the logging, we could also just warn about this pitfall. It is limited to loglevel >= 5 (i.e. not the default).

Note that the CSI example .yaml does use such a high log level, though. So there is definitely a risk that people accidentally log secrets.

I'm undecided myself. Thoughts anyone?

@jsafrane
Copy link
Contributor

I got an idea, perhaps we can add a new argument --log-csi-messages with clear description in --help output that it logs also secrets.

@jsafrane jsafrane mentioned this issue Aug 15, 2018
Merged
@pohly pohly mentioned this issue Oct 10, 2018
Closed
6 tasks
pohly added a commit to pohly/external-provisioner that referenced this issue Nov 21, 2018
@pohly
never log secrets
97954d6 
When running at glog level >= 5, external-provisioner logged the full
CreateVolumeRequest, including the secrets. Secrets should never be
logged at any level to avoid accidentally exposing them.

We need to filter out the secrets. With older CSI versions, that could
have been done based on the field name, which is still an option
should this get backported. With CSI 1.0, a custom field option marks
fields as secret. Using that option has the advantage that the code
will continue to work also when new secret fields get added in the
future.

For the sake of simplicity, JSON is now used as representation of the
string instead of the former compact text format from gRPC. That makes
it possible to strip values from a map with generic types, instead of
having to copy and manipulate the real generated structures.

Another option would have been to copy
https://github.com/golang/protobuf/blob/master/proto/text.go and
modify it so that it skips secret fields, but that's over 800 lines of
code.

Ultimately this new package should live in a "csi-common" repo and
also include other utility code, like logGRPC itself.

Fixes: kubernetes-csi#82, kubernetes-csi#167
@pohly pohly mentioned this issue Nov 21, 2018
Closed
@msau42 msau42 mentioned this issue Nov 30, 2018
Merged
This was referenced Jan 15, 2019
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@pohly @jsafrane @shubheksha @karataliu