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
Add datapolicy tags to staging/src/k8s.io/client-go/ #96001
Add datapolicy tags to staging/src/k8s.io/client-go/ #96001
Conversation
@serathius: GitHub didn't allow me to request PR reviews from the following users: PurelyApplied. Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
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. |
/assign @sttts |
/retest |
/assign @deads2k |
} | ||
|
||
// Response defines metadata about a failed request, including HTTP status code and | ||
// response headers. | ||
type Response struct { | ||
// Headers holds HTTP headers returned by the server. | ||
Header map[string][]string | ||
Header map[string][]string `datapolicy:"token"` |
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.
by the server, not the client.
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.
Is there possibility of those headers having any sensitive information?
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.
am not aware of any such case.
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.
This is still marked as token
These annotations won't harm. Of course most structs in this PR are not exposed anywhere, but they could in the future by accident. So this looks fine. Left some comments. |
a462719
to
e24f8c9
Compare
There were some comments about sensitivity of client certificate, don't see them now. Maybe they were deleted or GitHub lost tem |
/retest |
ad60272
to
8798c32
Compare
8798c32
to
21d0271
Compare
Sorry for test failure spam, looks like my local repository was corrupted resulting in prow failing to merge: $git merge --no-ff -m 'Merge +refs/pull/96372/head:refs/pr/96372' f36893d68352e7f3e9d4a627052784beab7bb13f
merge: f36893d68352e7f3e9d4a627052784beab7bb13f - not something we can merge
$git log f36893d68352e7f3e9d4a627052784beab7bb13f
fatal: bad object f36893d68352e7f3e9d4a627052784beab7bb13f |
PTAL @sttts |
/lgtm
|
/approve |
21d0271
to
e29c568
Compare
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
Fixed gofmt. |
As this PR covers field that was responsible for |
// PEM-encoded client TLS certificate. | ||
// +optional | ||
ClientCertificateData string | ||
// PEM-encoded client TLS private key. | ||
// +optional | ||
ClientKeyData string | ||
ClientKeyData string `datapolicy:"secret-key"` |
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.
is there anything that will catch a typo in these values? how does data policy handle unknown datapolicy values?
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.
Values of data policy are informational only. Datapolicy library will report list of all values.
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.
Two weeks late to the party here, but KEP-1933 is going to use these field tags for its static analysis. It would be relatively easy to add an analysis step that examines what values correspond to datapolicy
keys in field tags.
It would be a little messier to check for typos is the datapolicy
key itself, but we could bark up that tree if we're concerned about 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.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: liggitt, serathius, sttts 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 |
/kind feature
What this PR does / why we need it:
This PR adds "datapolicy" tags to golang structures as described in Kubernetes system components logs sanitization KEP. Those tags will be used by for ensuring this data will not be written to logs by Kubernetes system components.
List of datapolicy tags available:
security-key
- for TLS certificate keys. Keywords:key
,cert
,pem
token
- for HTTP authorization tokens. Keywords:token
,secret
,header
,auth
password
- anything passwordlike. Keywords:password
Special notes for your reviewer:
Due to size of Kubernetes codebase first iteration of tagging was done based on greping for particular keyword. Please ensure that tagged fields do contain type of sensitive data that matches their tag. Feel free to suggest any additional places that you think should be tagged.
Does this PR introduce a user-facing change?:
No
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
/cc @PurelyApplied
/sig instrumentation security
/priority important-soon
/milestone v1.20