Add feature-gated audit id handler that validates audit ids #129995
Conversation
|
Welcome @everettraven! |
|
Hi @everettraven. Thanks for your PR. I'm waiting for a kubernetes 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-sigs/prow repository. |
|
/sig auth |
|
/remove sig/api-machinery |
|
/remove-label sig/api-machinery |
|
@everettraven: The label(s) 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-sigs/prow repository. |
| BtreeWatchCache: { | ||
| {Version: version.MustParse("1.32"), Default: true, PreRelease: featuregate.Beta}, | ||
| AuditIDValidation: { | ||
| {Version: version.MustParse("1.33"), Default: true, PreRelease: featuregate.GA}, |
There was a problem hiding this comment.
Marked this as a GA feature gate due to #127801 (comment) - if there is a more appropriate value to place here, please let me know!
| @@ -60,6 +94,21 @@ func withAuditInit(handler http.Handler, newAuditIDFunc func() string) http.Hand | ||
| w.Header().Set(auditinternal.HeaderAuditID, auditID) | ||
| } | ||
|
|
||
| if auditIDValidationFunc != nil { |
There was a problem hiding this comment.
As it stands, if there is a validation function specified it will validate a generated audit ID as well. #127801 originally specifies that client provided audit ids should be validated. From what I could tell, when using WithValidatingAuditInit(), the validation function used (validateAuditID()) should always return valid for the defaultNewAuditID() function.
This means it should be safe to assume that the failure will only happen on client provided audit IDs, but in theory if the defaultNewAuditID() is updated to no longer always return a valid generated audit ID, some requests may fail due to this validation being run.
Open Question(s) to reviewers:
- Should the audit id validation be skipped if an audit id is generated?
- Are there any side effects to allowing generated audit ids to not adhere to the same validation requirements as client provided audit ids?
- Example: User supplies audit id that was previously generated. They receive a 400 status code. Is this a bad thing?
|
/ok-to-test |
3fff551 to
90f3033
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: everettraven 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 |
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
90f3033 to
6f683c5
Compare
| } else { | ||
| // if we are running a validating audit-id init test | ||
| // and the inner handler isn't expected to have run, we | ||
| // are testing an invalid audit-id scenario. In that case, | ||
| // ensure the status code returned is representative of | ||
| // a bad request (400). | ||
| if w.Code != http.StatusBadRequest { | ||
| t.Errorf("WithValidatingAuditID: expected status code %v but got %v", http.StatusBadRequest, w.Code) | ||
| } | ||
| } |
There was a problem hiding this comment.
Linter seems to be upset about this else statement here. I think this logic makes sense as is. Open to changing this to an else if if reviewers disagree that this is reasonable to override.
… handler that validates audit ids Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
…Validation feature gate is enabled Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
6f683c5 to
3d7d3c9
Compare
|
@everettraven: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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-sigs/prow repository. I understand the commands that are listed here. |
|
/remove-sig node cloud-provider device-management |
|
@everettraven: Those labels are not set on the issue: 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-sigs/prow repository. |
|
/remove-wg device-management |
|
/remove-sig api-machinery |
|
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-sigs/prow repository. |
|
/assign ritazh |
|
/triage accepted |
What type of PR is this?
/kind bug
What this PR does / why we need it:
Outlined in #127801, there is no validation performed on audit ids leading to potential security risks as injecting a malicious string may result in vulnerabilities for systems that do some processing on audit ids.
This PR addresses this issue by:
AuditIDValidation, that is enabled by defaultfilters.withAuditInit()function with logic to validate audit ids, adding 2 new input parameters to the function signature:http.Handlerto callServeHTTP()on if the audit id is invalidfilters.WithValidatingAuditInit()function that callsfilters.withAuditInit()providing inputs for the new parameters added tofilters.withAuditInit()filters.WithValidatingAuditInit()when theAuditIDValidationfeature gate is enabled andfilters.WithAuditInit()when it is disabled.Which issue(s) this PR fixes:
Fixes #127801
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: