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

HCD-23 Tighten up permissions on system keyspaces #1667

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tiagomlalves
Copy link
Member

What is the issue

Improves permissions security.

What does this PR fix and why was it fixed

Backports CASSANDRA-20090:

  • Restrict which permissions can be granted on system keyspaces
  • Ensure that GRANT... ON ALL KEYSPACES excludes system keyspaces

Copy link

github-actions bot commented Apr 1, 2025

Checklist before you submit for review

  • Make sure there is a PR in the CNDB project updating the Converged Cassandra version
  • Use NoSpamLogger for log lines that may appear frequently in the logs
  • Verify test results on Butler
  • Test coverage for new/modified code is > 80%
  • Proper code formatting
  • Proper title for each commit staring with the project-issue number, like CNDB-1234
  • Each commit has a meaningful description
  • Each commit is not very long and contains related changes
  • Renames, moves and reformatting are in distinct commits
  • All new files should contain the DataStax copyright header instead of the Apache License one

@tiagomlalves
Copy link
Member Author

Triggered #993

@tiagomlalves tiagomlalves marked this pull request as ready for review April 2, 2025 08:53
@cassci-bot
Copy link

❌ Build ds-cassandra-pr-gate/PR-1667 rejected by Butler


1 new test failure(s) in 1 builds
See build details here


Found 1 new test failures

Test Explanation Branch history Upstream history
o.a.c.u.b.BinLogTest.testTruncationReleasesLogS... regression 🔴 🔵🔵🔵🔵🔵🔵🔵

Found 2 known test failures

@bereng
Copy link
Collaborator

bereng commented Apr 3, 2025

On this one you'd need a CNDB PR imo to make sure we don't break anything there?

superuser.execute(format("GRANT MODIFY ON ALL KEYSPACES TO %s", user));
// User now has write permission on non-system keyspaces only
nonsuperuser.execute("INSERT INTO user_keyspace.t1 (k) VALUES (0)");
/* FIXME: 'User user has no MODIFY permission on <table system_views.local_node> or any of its parents'
Copy link
Collaborator

Choose a reason for hiding this comment

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

@tiagomlalves I am not seeing the problem here. If the tables names between OSS and CC changed why not just change the assertion message accordingly. What am I missing?

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is that we're trying to insert into system.local table but error message reports error on system_views.local_node table. Code is translating the table when writing to it but is not translating it for the error message.

Here, I see two solutions:

  1. accept the message and leave a comment with the follow up ticket
  2. use a different table with same semantics (happy to accept suggestion on the one to use).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not following 100% but if you accept the current message as it is, when that bug gets eventually fixed this test will fail and the author will fix it as well. I guess there is more value in targeting sensitive ks'.

Backports CASSANDRA-20090:
* Restrict which permissions can be granted on system keyspaces
* Ensure that GRANT... ON ALL KEYSPACES excludes system keyspaces
* Add system_traces to the always readable set
@tiagomlalves
Copy link
Member Author

On this one you'd need a CNDB PR imo to make sure we don't break anything there?

done!

@tiagomlalves
Copy link
Member Author

I've went through the CNDB failures and they don't seem to be related with these changes. I still need to rework the PR to uncomment the tests and accept the exception messages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants