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.

Sign up for GitHub

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

Jump to bottom

ansible-scylla-node: Add support for encryption at rest #282

Merged
merged 4 commits into from
Nov 30, 2023
Merged

ansible-scylla-node: Add support for encryption at rest #282

merged 4 commits into from
Nov 30, 2023

Conversation

igorribeiroduarte
Copy link
Collaborator

@igorribeiroduarte igorribeiroduarte commented Sep 6, 2023 •
edited
Loading

This PR adds the following features to the role:

  • The ability of managing table keys and system keys
  • The ability of enabling/disabling system encryption

Note that enabling data encryption is not part of this PR and needs to be done manually by the user.

The following tests were executed:

  • Validate that if handle_system_keys is set to true, any keys inside localhost_system_key_directory will be copied to system_key_directory
  • Validate that if handle_table_keys is set to true, any keys inside localhost_table_key_directory will be copied to table_key_directory
  • Validate that if any already existing key in the system_key_directory has a different content from a key with the same name in the localhost_system_key_diretory, the role will fail.
  • Validate that if any existing key in the table_key_directory has a different content from a key with the same name in the localhost_table_key_directory, the role will fail.
  • Validate that if system_info_encryption_local.enabled is set to true, the scylla.yaml file will be set appropriately
  • Validate that if system_info_encryption_kmip.enabled is set to true, the scylla.yaml file will be set appropriately
  • Validate that if system_info_encryption_kms.enabled is set to true, the scylla.yaml file will be set appropriately

@igorribeiroduarte igorribeiroduarte force-pushed the encryption_at_rest_support branch from c279144 to ab5141a Compare September 6, 2023 16:00
@igorribeiroduarte igorribeiroduarte marked this pull request as draft September 6, 2023 16:00
@igorribeiroduarte igorribeiroduarte force-pushed the encryption_at_rest_support branch from ab5141a to b46f483 Compare September 6, 2023 18:44
@igorribeiroduarte igorribeiroduarte force-pushed the encryption_at_rest_support branch 8 times, most recently from 3ba21bf to 456fb78 Compare October 6, 2023 13:58
@igorribeiroduarte igorribeiroduarte changed the title ansible-scylla-node: Add support for system resources encryption ansible-scylla-node: Add support for encryption at rest Oct 6, 2023
@igorribeiroduarte igorribeiroduarte force-pushed the encryption_at_rest_support branch 2 times, most recently from de6612c to cbedd15 Compare October 6, 2023 14:54
@igorribeiroduarte igorribeiroduarte marked this pull request as ready for review October 6, 2023 16:37
@vladzcloudius
Copy link
Collaborator

How was this PR tested?

@igorribeiroduarte
Copy link
Collaborator Author

How was this PR tested?

@vladzcloudius, the following tests were executed:

  • Executed the role for an existing cluster where encryption at rest was initially disabled, with both, system_info_encryption_local and data_encryption_local enabled:
    • Validated that after the rolling restart, the necessary params were set in scylla.yaml
    • Validated that the keys were generated and copied to the localhost
    • Validated that the same keys were copied to all scylla nodes
    • Validated that I'm able to encrypt a table with the data_key generated
  • Executed the role for an existing cluster with system_info_encryption_kmip enabled and validated that scylla.yaml was updated correctly.
  • Executed the role for an existing cluster, with keys in the localhost but without the keys on the nodes and validated that the keys were copied to the nodes. This test was executed with both, system_info_encryption_local and data_encryption_local enabled.
  • Executed the role for an existing cluster with keys on the nodes but without keys in the localhost and validated that the task failed. This test was executed with both, system_info_encryption_local and data_encryption_local enabled.
  • Executed the role for an existing cluster with keys on the nodes but with different keys in the localhost and validated that the task failed. This test was executed with both, system_info_encryption_local and data_encryption_local enabled.
  • Validated that if encryption at rest is disabled, the current behavior of the role is not affected.

vladzcloudius
vladzcloudius requested changes Oct 9, 2023
View reviewed changes
@igorribeiroduarte igorribeiroduarte marked this pull request as draft October 13, 2023 15:20
@igorribeiroduarte igorribeiroduarte force-pushed the encryption_at_rest_support branch 9 times, most recently from 6811a91 to 97e0f3e Compare October 18, 2023 12:32
@igorribeiroduarte igorribeiroduarte force-pushed the encryption_at_rest_support branch 5 times, most recently from 0aff41b to ef1cb01 Compare October 18, 2023 19:55
@igorribeiroduarte igorribeiroduarte marked this pull request as ready for review October 19, 2023 13:14
@vladzcloudius vladzcloudius requested a review from ManjotS October 26, 2023 14:43
vladzcloudius
vladzcloudius requested changes Nov 3, 2023
View reviewed changes
ansible-scylla-node/defaults/main.yml
# where the replicated keys will be kept.
# If you want to use this role for managing your system keys, regardless of them being used for system_info encryption
# or for encrypting the encrypted_keys table, you should set {{ handle_system_keys }} to true and place all of your system
# keys under the {{ localhost_system_key_directory }} folder. The role will copy all the local keys to the {{ system_key_directory }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks like an incorrect description of the localhost_system_key_directory content structure: you have a per-host sub-directories structure there, don't you?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

you have a per-host sub-directories structure there, don't you?

we do because the inventory_hostname is part of localhost_table_key_directory and localhost_system_key_directory by default:

localhost_system_key_directory: "{{ inventory_dir }}/encryption_at_rest/{{ inventory_hostname }}/system_encryption_keys"
localhost_table_key_directory: "{{ inventory_dir }}/encryption_at_rest/{{ inventory_hostname }}/table_encryption_keys"

Note that based on that the description is not wrong.
I added a comment clarifying that right below the statement that you mentioned.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not clear.
The best is to provide an example of the localhost_system_key_directory layout to avoid any confusion.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@igorribeiroduarte
ansible-scylla-node: Add task list for handling encryption at rest keys
2145690 
This patch adds a task list for managing local keys for encryption at rest.
This task list will be responsible for copying a local key to all the Scylla
nodes.
If the local key exists but is different from an already existing key on the nodes,
the role will fail.
In order to use this task list, the variables _localhost_key_path and _remote_key_path
must be defined.
@igorribeiroduarte igorribeiroduarte force-pushed the encryption_at_rest_support branch from ef1cb01 to b02280d Compare November 6, 2023 17:55
@igorribeiroduarte
ansible-scylla-node: Manage system keys for encryption at rest
d6be728 
The changes added here allow the user to manage system keys.
Once the user sets the variable {{ handle_system_keys }} to true, the role will be responsible
for copying any local keys stored in {{ localhost_system_key_directory }} to
{{ system_key_directory }}.
This patch also changes the scylla.yaml.j2 template file to update the system_key_directory
in the scylla.yaml file if {{ handle_system_keys }} is set to true.
Note that this patch only manages the keys without actually enabling/disabling any type
of encryption at rest in the cluster.
@igorribeiroduarte
ansible-scylla-node: Manage table keys for encryption at rest
ef5f289 
The changes added here allow the user to manage table keys.
Once the user sets the variable {{ handle_table_keys }} to true, the role will be responsible
for copying any local keys stored in {{ localhost_table_key_directory }} to
{{ table_key_directory }}.
Note that this patch only manages the keys without actually enabling/disabling any type
of encryption at rest in the cluster.
@igorribeiroduarte
ansible-scylla-node: Add support for system_info encryption
9f5acc8 
The changes added here allow the user to enable/disable encryption at rest
for system level data, such as commit logs, batches, hints logs and KMIP Password.
In order to do so, this patch adds the following yaml objects:
* system_info_encryption_local: Should be used for a Local Key Provider
* system_info_encryption_kmip: Should be used for a KMIP Key Provider
* system_info_encryption_kms: Should be used for a KMS Key Provider

These objects are documented on ansible-scylla-node/defaults/main.yml and you need
to configure them appropriately.

Once you enable any of them and execute the role, the scylla.yaml file will be adjusted
according to what it was configured.
@igorribeiroduarte igorribeiroduarte force-pushed the encryption_at_rest_support branch from b02280d to 9f5acc8 Compare November 29, 2023 12:22
@vladzcloudius vladzcloudius merged commit bba5385 into scylladb:master Nov 30, 2023
@igorribeiroduarte igorribeiroduarte mentioned this pull request Jul 4, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tarzanek tarzanek Awaiting requested review from tarzanek

@ManjotS ManjotS Awaiting requested review from ManjotS

@vladzcloudius vladzcloudius Awaiting requested review from vladzcloudius

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@igorribeiroduarte @vladzcloudius