-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
stmtsummary: check authUsers using the polymorphic stmtSummaryStats #58427
base: master
Are you sure you want to change the base?
Conversation
`isAuthed` was factored out of `getStmtByDigestElementRow` so it could be shared with `getStmtByDigestCumulativeRow`, but the logic was hardcoded to use the cumulative statement stats, breaking the auth checks for the original interval-based statement summary tables.
Hi @henrybw. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with 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. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #58427 +/- ##
================================================
+ Coverage 73.5204% 74.4520% +0.9315%
================================================
Files 1681 1681
Lines 463800 475455 +11655
================================================
+ Hits 340988 353986 +12998
+ Misses 101988 100751 -1237
+ Partials 20824 20718 -106
Flags with carried forward coverage won't be shown. Click here to find out more.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: xhebox 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 |
[LGTM Timeline notifier]Timeline:
|
/run-check-issue-triage-complete |
// Test the same query digests, but run as a different user in a new statement | ||
// summary interval. The old user should not be able to access the rows generated | ||
// for the new user. | ||
ssMap.beginTimeForCurInterval = time.Now().Unix() |
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 this required?
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.
I think it is. The intent here is to create a new statement summary interval with a new, empty authUsers
set, because authUsers
represents the union of all users who have executed this query for a given interval. If we use the same interval, then authUsers
will include both users.
More broadly speaking, testing with a new interval makes sure that a user cannot read rows from the current statement summary that they are not authorized to access, even if they ran the same query in the past. (This is what the bug would have allowed, since it was effectively not resetting authUsers
for new statement summaries.)
/ok-to-test |
@henrybw: The following test failed, say
Full PR test history. Your PR dashboard. 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. |
@henrybw: The following test failed, say
Full PR test history. Your PR dashboard. 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. |
What problem does this PR solve?
Issue Number: close #58403
Problem Summary:
isAuthed
was factored out ofgetStmtByDigestElementRow
so it could be shared withgetStmtByDigestCumulativeRow
, but the logic was hardcoded to use the cumulative statement stats, breaking the auth checks for the original interval-based statement summary tables.What changed and how does it work?
isAuthed
, checkssStats.authUsers
rather than checkingssbd.cumulative.authUsers
.TestAccessPrivilege
to test the same query digests as before, but run as a different user in a new statement summary interval (which should not be able to access rows generated for the old user).Check List
Tests
Reverted the fix and confirmed that the modified TestAccessPrivilege test fails.
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.