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

cdh: improves the luks-encrypt-storage script #666

Merged

Conversation

wainersm
Copy link
Member

While reviewing kata-containers/kata-containers#9999 I wanted to understand confidential-data-hub/storage/scripts/luks-encrypt-storage better so I took a deep look at this file. I began to worry whether the logic to detect the blocks could fail or not, and in case of failure if it should continue the script as if everything went well (possible not cleaning up the block pages if any). Is it concern? So I decided to add a check for the block numbers, that if empty then the script bails out. Let me know if it is wrong and will introduce another issue that I could anticipate.

While in here, I delinted the script.

Cc @Xynnn007 @ChengyuZhu6 @fitzthum

In order to format the ephemeral storage device with data integrity, the
luks-encrypt-storage script will read the block numbers to have its pages
cleaned. This introduced a check on the script to ensure that at least one
block is read, otherwise it will bail out as this might indicate a bug on
the logic to find blocks.

Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
Removed all Shellcheck's "Double quote to prevent globbing and word splitting"
warnings so the script is clean now.

Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
@wainersm wainersm requested a review from sameo as a code owner August 13, 2024 18:48
@wainersm
Copy link
Member Author

ah, tested I didn't break the script with:

$ loop_file="/tmp/test.img"
$ sudo dd if=/dev/zero of=$loop_file bs=1M count=1000
1000+0 records in
1000+0 records out
1048576000 bytes (1.0 GB, 1000 MiB) copied, 0.825012 s, 1.3 GB/s
$ sudo losetup -fP $loop_file
$ device=$(sudo losetup -j $loop_file | awk -F'[: ]' '{print $1}')
$ echo $device
/dev/loop2
$ device_num=$(sudo lsblk -no MAJ:MIN $device)
$ echo $device_num
7:2
$ mkdir -p /tmp/target_path
$ sudo touch /run/encrypt_storage.key
$ sudo ./confidential-data-hub/storage/scripts/luks-encrypt-storage $device_num
mke2fs 1.47.0 (5-Feb-2023)
Clearing page at 0
1+0 records in
1+0 records out
4096 bytes (4.1 kB, 4.0 KiB) copied, 9.7763e-05 s, 41.9 MB/s
Clearing page at 32768
1+0 records in
1+0 records out
4096 bytes (4.1 kB, 4.0 KiB) copied, 8.9638e-05 s, 45.7 MB/s
Clearing page at 98304
1+0 records in
1+0 records out
4096 bytes (4.1 kB, 4.0 KiB) copied, 0.00010671 s, 38.4 MB/s
Clearing page at 163840
1+0 records in
1+0 records out
4096 bytes (4.1 kB, 4.0 KiB) copied, 8.4598e-05 s, 48.4 MB/s
Clearing page at 229376
1+0 records in
1+0 records out
4096 bytes (4.1 kB, 4.0 KiB) copied, 8.1933e-05 s, 50.0 MB/s
mke2fs 1.47.0 (5-Feb-2023)
Creating filesystem with 247971 4k blocks and 62080 inodes
Filesystem UUID: f6a6053c-e52e-46d4-a12b-6661fbc3cf40
Superblock backups stored on blocks: 
        32768, 98304, 163840, 229376

Allocating group tables: done                            
Writing inode tables: done                            
Creating journal (4096 blocks): done
Writing superblocks and filesystem accounting information: done

$ lsblk |grep "encrypted_disk"
└─encrypted_disk_IUeXM_dif                    253:1    0 968.6M  0 crypt 
  └─encrypted_disk_IUeXM                      253:2    0 968.6M  0 crypt /tmp/target_path

Copy link
Member

@ChengyuZhu6 ChengyuZhu6 left a comment

Choose a reason for hiding this comment

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

It makes sense to me. LGTM thanks for your improvement.

Copy link

@huoqifeng huoqifeng left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks for harden the script! @wainersm

@Xynnn007 Xynnn007 merged commit d8d352f into confidential-containers:main Aug 14, 2024
5 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.

4 participants