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.

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

henrybw
Copy link
Contributor

@henrybw henrybw commented Dec 20, 2024

What problem does this PR solve?

Issue Number: close #58403

Problem Summary:

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.

What changed and how does it work?

  • In isAuthed, check ssStats.authUsers rather than checking ssbd.cumulative.authUsers.
  • Extend 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

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
    Reverted the fix and confirmed that the modified TestAccessPrivilege test fails.
  • No need to test
    • I checked and no code files have been changed.

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

`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.
@ti-chi-bot ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/needs-triage-completed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 20, 2024
Copy link

tiprow bot commented Dec 20, 2024

Hi @henrybw. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

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.

Copy link

codecov bot commented Dec 20, 2024

Codecov Report

Attention: Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 74.4520%. Comparing base (05a35ca) to head (4ba1489).
Report is 58 commits behind head on master.

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     
Flag Coverage Δ
integration 44.1783% <25.0000%> (?)
unit 73.2664% <75.0000%> (+0.9834%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
dumpling 53.0100% <ø> (+0.3190%) ⬆️
parser ∅ <ø> (∅)
br 45.7964% <ø> (+0.0250%) ⬆️

Copy link

ti-chi-bot bot commented Dec 20, 2024

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Dec 20, 2024
Copy link

ti-chi-bot bot commented Dec 20, 2024

[LGTM Timeline notifier]

Timeline:

  • 2024-12-20 05:45:23.268808385 +0000 UTC m=+1194913.357610928: ☑️ agreed by xhebox.

@xhebox
Copy link
Contributor

xhebox commented Dec 20, 2024

/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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this required?

Copy link
Contributor Author

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.)

@hawkingrei
Copy link
Member

/ok-to-test

@ti-chi-bot ti-chi-bot bot added the ok-to-test Indicates a PR is ready to be tested. label Dec 27, 2024
Copy link

tiprow bot commented Dec 27, 2024

@henrybw: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
fast_test_tiprow 4ba1489 link true /test fast_test_tiprow

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.

Copy link

ti-chi-bot bot commented Dec 27, 2024

@henrybw: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
idc-jenkins-ci-tidb/unit-test 4ba1489 link true /test unit-test

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. ok-to-test Indicates a PR is ready to be tested. release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DATA RACE in the TestStmtSummaryIssue35340
4 participants