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

Remove 'percona_tde.pg_tde_key_provider' user catalog #230

Merged
merged 7 commits into from
Jul 5, 2024

Conversation

codeforall
Copy link
Contributor

Remove 'percona_tde.pg_tde_key_provider' user catalog and introduce provider info file for key providers

This commit removes the 'percona_tde.pg_tde_key_provider' user catalog and replaces it with a provider info file for saving key providers. This change ensures that the key provider information can be accessed during recovery, as the user catalogs cannot be relied upon in such scenarios.

The commit maintains the current API functions, so callers will not experience any differences in functionality or usage after this change.

Additionally, the commit adjusts how the shared memory manager retrieves information about the number of LWLocks required by the extension, optimizing the process.

TODO: Implement xlog message for cleaning up the provider info file during recovery operations to ensure consistency and avoid potential issues.

…rovider info file for key providers

This commit removes the 'percona_tde.pg_tde_key_provider' user catalog and
replaces it with a provider info file for saving key providers.
This change ensures that the key provider information can be accessed
during recovery, as the user catalogs cannot be relied upon in such scenarios.

The commit maintains the current API functions, so callers will not experience
any differences in functionality or usage after this change.

Additionally, the commit adjusts how the shared memory manager retrieves
information about the number of LWLocks required by the extension, optimizing
the process.

TODO: Implement xlog message for cleaning up the provider info file during
recovery operations to ensure consistency and avoid potential issues.
@codeforall codeforall linked an issue Jul 3, 2024 that may be closed by this pull request
@ImTheKai ImTheKai requested review from dAdAbird and dutow July 3, 2024 18:48
src/common/pg_tde_utils.c Outdated Show resolved Hide resolved
src/catalog/tde_keyring.c Outdated Show resolved Hide resolved
EXECUTE FUNCTION keyring_delete_dependency_check_trigger();

-- Key Provider Management

CREATE OR REPLACE FUNCTION pg_tde_add_key_provider(provider_type VARCHAR(10), provider_name VARCHAR(128), options JSON)
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 also have pg_tde_remove_key_provider() and/or pg_tde_edit_key_provider()? And also pg_tde_list_key_providers(), since there is no way to check what providers are there

Plus, since we ain't restricted with the db access anymore, it make sense to have ability to set a "global" key provider which applies to all dbs by default by can be redefined to a db-local one. It needs thoughts around security restrictions. But met be convenient for clusters with multiple dbs. It's definitely not for now, just thoughts out loud.

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's a good idea. We can take it up as a separate PR.


void InitializeKeyProviderInfo(void)
{
ereport(LOG, (errmsg("Initializing TDE principal key info")));
Copy link
Member

Choose a reason for hiding this comment

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

Here and in many places - in the backend code (and in most of our code) ereport/elog error messages never start with capitalized letter if it is not an abbr. (like CPU, SQL etc) or all caps word. Let's be consistent with the style

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed for all messages added as part of this PR!
We might also need to do a code run-through and fix similar inconsistencies in other parts of the code.

-- Note: The table is created using heap storage becasue we do not want this table
-- to be encrypted by pg_tde. This table is used to store key provider information
-- and we do not want to encrypt this table using pg_tde.
CREATE TABLE percona_tde.pg_tde_key_provider(provider_id SERIAL,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we replace this with a view that provides the same functionality?

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 believe we would need to provide a view and edit function as mentioned in the comment #230 (comment), but we can handle it as a separate PR

codeforall and others added 3 commits July 5, 2024 14:30
Co-authored-by: Andrew Pogrebnoi <[email protected]>
Co-authored-by: Andrew Pogrebnoi <[email protected]>
Refactor log/error messages and variable names;
fix memory leak in get_keyring_infofile_path function
Copy link
Member

@dAdAbird dAdAbird left a comment

Choose a reason for hiding this comment

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

found a couple more of minor things, other than that LGTM

src/catalog/tde_keyring.c Outdated Show resolved Hide resolved
src/catalog/tde_keyring.c Outdated Show resolved Hide resolved
codeforall and others added 2 commits July 5, 2024 18:11
Co-authored-by: Andrew Pogrebnoi <[email protected]>
Co-authored-by: Andrew Pogrebnoi <[email protected]>
@codeforall codeforall merged commit e270322 into percona:main Jul 5, 2024
8 checks passed
@dutow dutow mentioned this pull request Aug 19, 2024
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.

Remove User Catalogs for Storing Key Provider Information
3 participants