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

PG-853: Access control of pg_tde SQL functions #277

Merged
merged 3 commits into from
Sep 9, 2024

Conversation

codeforall
Copy link
Contributor

Add SQL interfaces for granting and revoking access to key management and viewer functions. This commit introduces four new SQL functions to manage access to key-related functionalities in the pg_tde extension:

  • tde_grant_key_management_to_role: Grants execute permissions on key management functions to the specified user or role.
  • tde_revoke_key_management_from_role: Revokes execute permissions on key management functions from the specified user or role.
  • tde_grant_key_viewer_to_role: Grants execute permissions on key viewer functions to the specified user or role.
  • tde_revoke_key_viewer_from_role: Revokes execute permissions on key viewer functions from the specified user or role.

Additionally, upon creating the extension, all execute permissions are revoked from the PUBLIC role. Therefore, a superuser must explicitly grant the necessary permissions to non-superusers to access these functions after the extension is created.

These additions provide a more controlled and secure way to manage permissions for key management and viewer functionalities within the extension.

Add SQL interfaces for granting and revoking access to key management and viewer
functions. This commit introduces four new SQL functions to manage access to
key-related functionalities in the `pg_tde` extension:

- `tde_grant_key_management_to_role`: Grants execute permissions on key
    management functions to the specified user or role.
- `tde_revoke_key_management_from_role`: Revokes execute permissions on
    key management functions from the specified user or role.
- `tde_grant_key_viewer_to_role`: Grants execute permissions on key
    viewer functions to the specified user or role.
- `tde_revoke_key_viewer_from_role`: Revokes execute permissions on
    key viewer functions from the specified user or role.

Additionally, upon creating the extension, all execute permissions are revoked
from the `PUBLIC` role. Therefore, a superuser must explicitly grant the
necessary permissions to non-superusers to access these functions after the
extension is created.

These additions provide a more controlled and secure way to manage permissions
for key management and viewer functionalities within the extension.
Comment on lines +54 to +56
PGTDE::append_to_file("-- pg_tde_set_principal_key should also fail");
($cmdret, $stdout, $stderr) = $node->psql('postgres', "SELECT pg_tde_set_principal_key('test-db-principal-key','file-vault');", extra_params => ['-a', '-U', 'test_access']);
PGTDE::append_to_file($stderr);
Copy link
Member

Choose a reason for hiding this comment

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

I'd also check if there is no access for the pg_tde_rotate_principal_key

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

pg_tde--1.0.sql Outdated
Comment on lines 347 to 352
PERFORM tde_grant_execute_privilege_on_function(target_user_or_role, 'pg_tde_list_all_key_providers', 'OUT INT, OUT varchar, OUT varchar, OUT JSON');
PERFORM tde_grant_execute_privilege_on_function(target_user_or_role, 'pg_tde_is_encrypted', 'VARCHAR');

PERFORM tde_grant_execute_privilege_on_function(target_user_or_role, 'pg_tde_principal_key_info_internal', 'BOOLEAN');
PERFORM tde_grant_execute_privilege_on_function(target_user_or_role, 'pg_tde_principal_key_info', '');
PERFORM tde_grant_execute_privilege_on_function(target_user_or_role, 'pg_tde_principal_key_info', 'pg_tde_global');
Copy link
Member

Choose a reason for hiding this comment

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

Should we use just

PERFORM tde_grant_key_viewer_to_role(target_user_or_role);

instead? It'll be easier to read and maintain both functions as for me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@dAdAbird
Copy link
Member

dAdAbird commented Sep 5, 2024

Another question:
Can we just grant/revoke access to only C functions like *_internal etc? So makes less PERFORM tde_grant_execute_privilege_on_function ... code. Hence easier to follow and less chances to make mistake maintaining tde_grant_*, tde_revoke_* functions.

I mean, shouldn't revoke access to pg_tde_add_key_provider_internal implicitly make pg_tde_add_key_provider_file and pg_tde_add_key_provider_vault_v2 functions useless?

@dutow
Copy link
Collaborator

dutow commented Sep 5, 2024

Why tde_* instead of pg_tde_*, which we used for the other functions so far? Shouldn't these use the same prefix?

This commit includes the following updates:
- Updated the prefix of newly added function names from `tde_` to `pg_tde_`.
- Enhanced the `access_control` test case to also verify permissions for the
`rotate_key`, `..list_key_providers`, and `key_info` functions.
@codeforall
Copy link
Contributor Author

Why tde_* instead of pg_tde_*, which we used for the other functions so far? Shouldn't these use the same prefix?

Another question: Can we just grant/revoke access to only C functions like *_internal etc? So makes less PERFORM tde_grant_execute_privilege_on_function ... code. Hence easier to follow and less chances to make mistake maintaining tde_grant_*, tde_revoke_* functions.

I mean, shouldn't revoke access to pg_tde_add_key_provider_internal implicitly make pg_tde_add_key_provider_file and pg_tde_add_key_provider_vault_v2 functions useless?

The reason I'm performing the grant/revoke on every function is to ensure that if a "permission denied" error occurs, it reflects the name of the function the user actually called. Simply revoking access from the *_internal function would also trigger a permission denied error, but it would always refer to the internal function that the user did not directly execute.

@codeforall codeforall merged commit 19f722e into percona:main Sep 9, 2024
13 checks passed
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