-
Notifications
You must be signed in to change notification settings - Fork 136
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
Clear the secrets from request for klog print in logGRPC()
#1462
base: master
Are you sure you want to change the base?
Clear the secrets from request for klog print in logGRPC()
#1462
Conversation
Welcome @mpatlasov! |
Hi @mpatlasov. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mpatlasov The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
pkg/gce-pd-csi-driver/utils.go
Outdated
klog.V(4).Infof("%s called with request: %s", info.FullMethod, req) | ||
// However malicious user still can put a secret in request as explained here: | ||
// https://github.com/kubernetes-sigs/gcp-compute-persistent-disk-csi-driver/issues/1372 | ||
if klog.V(4).Enabled() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We regularly use log level 4 in production. Since this driver doesn't actually require any secret, can we just clear the secrets from the request?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@msau42 , the request may be of different types, i.e. NodeExpandVolumeRequest
is not the only type with Secrets
(e.g. CreateVolumeRequest
, DeleteVolumeRequest
, etc.). I didn't find any straightforward way to clear the secrets from a request of arbitrary type, can you tell me how you suggest to do it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we can leverage the reflect package? https://pkg.go.dev/reflect#example-Value.FieldByName potentially looks promising. Or could we even use https://pkg.go.dev/reflect#StructTag to create a more efficient protosanitizer?
4eb6516
to
a618653
Compare
logGRPC()
logGRPC()
logGRPC()
logGRPC()
Last push reworks PR: instead of calling |
Malicious user can put a secret in request as explained here: kubernetes-sigs#1372.
a618653
to
afd457a
Compare
Last push addressed Jan's comment:
|
/ok-to-test |
/lgtm |
@msau42 , can you |
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@@ -56,14 +57,30 @@ func NewNodeServiceCapability(cap csi.NodeServiceCapability_RPC_Type) *csi.NodeS | |||
} | |||
} | |||
|
|||
// Reflect magic below simply clears Secrets map from request. | |||
func clearSecrets(req interface{}) interface{} { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jsafrane how do you feel about putting this into csi-lib-utils as an alternative to protosanitizer so that it is not specific to a csi driver?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me, but with a huge warning that only field explicitly named Secrets
is cleared.
@@ -56,14 +57,30 @@ func NewNodeServiceCapability(cap csi.NodeServiceCapability_RPC_Type) *csi.NodeS | |||
} | |||
} | |||
|
|||
// Reflect magic below simply clears Secrets map from request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to document all the assumptions about how secret fields are named in the csi spec.
// Note that secrets are not included in any RPC message. In the past protosanitizer and other log | ||
// Note that secrets may be included in some RPC messages | ||
// (https://github.com/kubernetes-sigs/gcp-compute-persistent-disk-csi-driver/issues/1372), | ||
// but the driver ignores them. In the past protosanitizer and other log |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you able to do a benchmark test to show that the reflect method does not signficantly increase CPU usage?
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
/remove-lifecycle stale |
Malicious user can put a secret in request as explained here: #1372.
What type of PR is this?
/kind bug
What this PR does / why we need it:
Malicious user can put a secret in request as explained here: #1372.
This PR simply clears the secrets from request.
Which issue(s) this PR fixes:
Fixes #1372
Does this PR introduce a user-facing change?: