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

[OSS-ONLY] Restrict grant to/from any babelfish created role via PG port #3386

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

Conversation

shalinilohia50
Copy link
Contributor

@shalinilohia50 shalinilohia50 commented Jan 9, 2025

Description

Restrict grant to/from any babelfish created role via PG port
All server and database roles including fixed server roles and database roles have 'bbf_role_admin' as its member. Added a check to identify if 'bbf_role_admin' is a member of any role in addition to the existing checks to avoid granting any Babelfish db/server role to another Babelfish db/server roles including fixed db and server roles.

Issues Resolved

Task: BABEL-5505
Signed-off-by: Shalini Lohia lshalini@amazon.com

Test Scenarios Covered

  • Use case based -
  • Boundary conditions -
  • Arbitrary inputs -
  • Negative test cases -
  • Minor version upgrade tests -
  • Major version upgrade tests -
  • Performance tests -
  • Tooling impact -
  • Client tests -

@shalinilohia50 shalinilohia50 changed the title Restrict grant to/from any babelfish created role [OSS-ONLY] Restrict grant to/from any babelfish created role Jan 9, 2025
if (OidIsValid(roleid) && IS_DEFAULT_BBF_SERVER_ROLE(rolename))
if (OidIsValid(roleid) && is_babelfish_role(rolename))
{
pfree(rolename);
Copy link
Contributor

Choose a reason for hiding this comment

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

We didn't palloc'd this object so shouldn't be free'ing it as well.

@@ -973,7 +975,8 @@ is_babelfish_role(const char *role)
&& OidIsValid(bbf_msdb_guest_oid)
&& is_member_of_role(role_oid, bbf_master_guest_oid)
&& is_member_of_role(role_oid, bbf_tempdb_guest_oid)
&& is_member_of_role(role_oid, bbf_msdb_guest_oid))
&& is_member_of_role(role_oid, bbf_msdb_guest_oid)
&& is_member_of_role(bbf_admin_oid, role_oid))
Copy link
Contributor

Choose a reason for hiding this comment

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

What about only checking membership against bbf_role_admin? Is it sufficient or do we still need the above checks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried, that wasn't sufficient for a case where a TSQL user having sysadmin privilege was able to create/alter a user via PG port. Since TSQL user created the PG user, it was being considered as a BBF user and didn't get dropped.

@shalinilohia50 shalinilohia50 changed the title [OSS-ONLY] Restrict grant to/from any babelfish created role [OSS-ONLY] Restrict grant to/from any babelfish created role via PG port Jan 9, 2025
@coveralls
Copy link
Collaborator

coveralls commented Jan 9, 2025

Pull Request Test Coverage Report for Build 12685975063

Details

  • 4 of 5 (80.0%) changed or added relevant lines in 1 file are covered.
  • 7 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.01%) to 74.508%

Changes Missing Coverage Covered Lines Changed/Added Lines %
contrib/babelfishpg_tds/src/backend/tds/tdsutils.c 4 5 80.0%
Files with Coverage Reduction New Missed Lines %
contrib/babelfishpg_tds/src/backend/tds/tdscomm.c 2 75.95%
contrib/babelfishpg_tds/src/backend/tds/tdsutils.c 5 72.34%
Totals Coverage Status
Change from base Build 12683446175: -0.01%
Covered Lines: 46377
Relevant Lines: 62244

💛 - Coveralls

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.

4 participants