You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Attila Bukor (Code Review)" <ge...@cloudera.org> on 2021/11/15 19:05:24 UTC
[kudu-CR] [encryption] KUDU-3316 Add encrypted file keys to files
Attila Bukor has uploaded this change for review. ( http://gerrit.cloudera.org:8080/18025
Change subject: [encryption] KUDU-3316 Add encrypted file keys to files
......................................................................
[encryption] KUDU-3316 Add encrypted file keys to files
The previous patches in the data at rest encryption saga add the ability
to encrypt data at rest, but files were encrypted using the same hard-coded
key.
This patch adds an extra header to encrypted files to store the
encryption algorithm used and the encrypted file key. For now, the file
keys are encrypted with the same dummy encryption key.
The header is a bit different from the one described in the design doc,
because the writes don't have to be aligned to the block size after all,
so I saw no reason to pad to 64 bytes, so the header is now only 40
bytes. The encryption algorithm and key length was also changed to be
stored in 1 byte instead of 2 for easier handling. The magic string is
"kuduenc" instead of "kuduen".
This patch also introduces a new flag which is hidden for now:
--encryption_key_length. This can be set to any valid AES key length as
per its specification (128, 192, or 256 bits), as only AES encryption is
supported for now, and there are no plans to support anything else in
the foreseeable future.
File sizes as returned by *File APIs are logical, so on disk they're 40
bytes larger. The GetFileSize() can return both the logical and the
physical file size, but GetFileSizeOnDisk() actually uses block count
and must be physical. Some of the tests rely on this and make
assumptions based on the numbers reported here, these tests are not run
with encryption enabled.
Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
---
M src/kudu/consensus/log-test-base.h
M src/kudu/fs/dir_util.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/util/env-test.cc
M src/kudu/util/env.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/env_util.cc
M src/kudu/util/pb_util-test.cc
11 files changed, 364 insertions(+), 86 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/18025/1
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@apache.org>
[kudu-CR] [security] KUDU-3316 Add encrypted file keys
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/18025 )
Change subject: [security] KUDU-3316 Add encrypted file keys
......................................................................
Patch Set 13: Verified+1
(10 comments)
> Patch Set 13: Verified-1
>
> Build Failed
>
> http://jenkins.kudu.apache.org/job/kudu-gerrit/25006/ : FAILURE
http://gerrit.cloudera.org:8080/#/c/18025/8//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18025/8//COMMIT_MSG@36
PS8, Line 36: of it.
> Well, I guess the regular way should work there, like
Thanks, I'll try that.
http://gerrit.cloudera.org:8080/#/c/18025/11/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/18025/11/src/kudu/fs/log_block_manager.cc@662
PS11, Line 662: Should only be called
: // when a block is fully written,
> nit: mind updating this?
Done
http://gerrit.cloudera.org:8080/#/c/18025/11/src/kudu/fs/log_block_manager.cc@749
PS11, Line 749: auto en
> nit: auto or size_t?
Done
http://gerrit.cloudera.org:8080/#/c/18025/11/src/kudu/fs/log_block_manager.cc@750
PS11, Line 750: // If we have an encryption heade
> Please add a comment describing why this is here.
Done
http://gerrit.cloudera.org:8080/#/c/18025/11/src/kudu/fs/log_block_manager.cc@937
PS11, Line 937: }
> nit: const?
Done
http://gerrit.cloudera.org:8080/#/c/18025/11/src/kudu/fs/log_block_manager.cc@1095
PS11, Line 1095: ritten.
> I'm curious how this changed? What in this patch results in 0-length record
We have some tests that create empty blocks, like TestMetadataTruncation and TestReuseBlockIds. We always had these tests, but until now record->offset() was also 0. Now record->offset() is 4096 due to alignments, and *data_file_size is 64.
http://gerrit.cloudera.org:8080/#/c/18025/11/src/kudu/fs/log_block_manager.cc@1318
PS11, Line 1318: int64_t off = std::max(preallocated_offset_, block_start_offset);
> Can you add a comment for this too?
Done
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/env_posix.cc@827
PS8, Line 827: m, 1
> One alternative might be passing 'eh' as const pointer and removing the 'de
Yea that's a good idea, thank you. I don't know why I didn't think of that.
http://gerrit.cloudera.org:8080/#/c/18025/11/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
http://gerrit.cloudera.org:8080/#/c/18025/11/src/kudu/util/env_posix.cc@243
PS11, Line 243: AES128ECB = 0xFD,
: AES192ECB = 0XFE,
: AES256ECB = 0xFF,
> nit: I'd also expect just adding some bit (e.g. the MSB) for ECB vs CTR, li
These are special values that should never be written to an encryption header. The 0x03-0xFC range is reserved for future use if we ever want to add support for some other encryption ciphers.
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/file_cache-test.cc
File src/kudu/util/file_cache-test.cc:
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/file_cache-test.cc@103
PS8, Line 103: unique_ptr<RWFile> f;
: RWFileOptions opts;
: opts.is_sensitive = true;
: RETURN_NOT_OK(env_->NewRWFile(opts, name, &f));
: RETURN_NOT_OK(f->Write(f->GetEncryptionHeaderSize(), data));
> Looking through this again, it seems like whenever we deal with Read() and
Yea, the OffsetFromEnd approach has occurred to me too, but I discarded it because of LBM.
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 13
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Tue, 25 Jan 2022 21:45:52 +0000
Gerrit-HasComments: Yes
[kudu-CR] [encryption] KUDU-3316 Add encrypted file keys
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/18025 )
Change subject: [encryption] KUDU-3316 Add encrypted file keys
......................................................................
Patch Set 4:
(12 comments)
http://gerrit.cloudera.org:8080/#/c/18025/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18025/3//COMMIT_MSG@30
PS3, Line 30: made to code that handles files and relies on sizes and offsets,
: including in tests.
:
: I ran the full test suite manually with encryption enabled to make sure
: turning on encryption doesn't break anything.
:
> I'm curious what the policy is for using physical vs logical size then. I w
You're right, it's better to use physical sizes everywhere and update the code to take the header size into account.
http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/consensus/consensus_meta.cc
File src/kudu/consensus/consensus_meta.cc:
http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/consensus/consensus_meta.cc@416
PS3, Line 416:
> Shouldn't this show the header too?
Done
http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env.h
File src/kudu/util/env.h:
http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env.h@194
PS3, Line 194:
> Looking at proliferation of extra parameters for the methods like this, I s
Actually, that was my original approach, but decided against it, I don't remember exactly why, but I think this was clearer. Also, these are now gone with the simplification of the size calculation, so it shouldn't be a problem.
http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env.h@730
PS3, Line 730: WriteStringToFileSync(Env* env
> nit: maybe, name this WriteStringToFileEncryptedSync()? It seems the namin
Done
http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env_posix.cc@919
PS3, Line 919: return write_fd_;
> Consider adding DCHECK()/CHECK() on st.st_size being greater or equal to kE
Done
http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env_posix.cc@1431
PS3, Line 1431:
> Should be a const ref to avoid a copy
Done
http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env_posix.cc@1432
PS3, Line 1432:
> nit: consider defining this once.
Done
http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env_posix.cc@1460
PS3, Line 1460: }
> Could be const and static?
Done
http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env_posix.cc@1461
PS3, Line 1461:
> +1 nit, maybe call it padding?
Done
http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env_posix.cc@1582
PS3, Line 1582:
> nit: extra semicolon
Done
http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env_posix.cc@1710
PS3, Line 1710:
> nit: consider some WITH_HEADER enum?
Done
http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env_posix.cc@2256
PS3, Line 2256: return IOError("stat", errno);
: }
> Is there any concern that a user accidentally switches between encryption a
Yea, startup should/will fail in this case. Do you have any tips on how to make this safer?
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Thu, 06 Jan 2022 20:03:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] [encryption] KUDU-3316 Add encrypted file keys
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Zoltan Chovan, Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18025
to look at the new patch set (#4).
Change subject: [encryption] KUDU-3316 Add encrypted file keys
......................................................................
[encryption] KUDU-3316 Add encrypted file keys
The previous patches in the data at rest encryption saga add the ability
to encrypt data at rest, but files were encrypted using the same hard-coded
key.
This patch adds an extra header to encrypted files to store the
encryption algorithm used and the encrypted file key. For now, the file
keys are encrypted with the same dummy encryption key which was
previously used to encrypt the files.
The header is a bit different from the one described in the design doc:
The encryption algorithm and key length was changed to be stored in
1 byte instead of 2 for easier handling and the magic string is
"kuduenc" instead of "kuduen".
This patch also introduces a new flag which is hidden for now:
--encryption_key_length. This can be set to any valid AES key length as
per its specification (128, 192, or 256 bits), as only AES encryption is
supported for now, and there are no plans to support anything else in
the foreseeable future.
As we add a 64-byte header to encrypted files, some changes had to be
made to code that handles files and relies on sizes and offsets,
including in tests.
I ran the full test suite manually with encryption enabled to make sure
turning on encryption doesn't break anything.
Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
---
M src/kudu/consensus/log-test.cc
M src/kudu/consensus/log.cc
M src/kudu/consensus/log_index.cc
M src/kudu/consensus/log_util.cc
M src/kudu/fs/dir_util.cc
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/log_block_manager-test-util.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_service-test.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/tablet_copy_source_session.cc
M src/kudu/util/env-test.cc
M src/kudu/util/env.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/env_util.cc
M src/kudu/util/env_util.h
M src/kudu/util/file_cache-stress-test.cc
M src/kudu/util/file_cache-test.cc
M src/kudu/util/file_cache.cc
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
26 files changed, 695 insertions(+), 247 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/18025/4
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [security] KUDU-3316 Add encrypted file keys
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18025 )
Change subject: [security] KUDU-3316 Add encrypted file keys
......................................................................
Patch Set 15: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18025/14//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18025/14//COMMIT_MSG@33
PS14, Line 33: With encryption enabled, extra
: steps are necessary to align the first block, and aligning blocks makes
: it impractical to hide encryption header size within Env and use
: logical file sizes outside of it.
> Technically, I think it would be possible. I started to work on a POC to se
Sure. I'm curious though, is the LBM the main part that would be complicated by the change? I was hoping that the "application code" above the Env would be greatly simplified, and only more specialized "power-user-esque" code like the LBM would have some minor complications. Though if it's more than that, happy to just punt.
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 15
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Sat, 29 Jan 2022 01:06:30 +0000
Gerrit-HasComments: Yes
[kudu-CR] [security] KUDU-3316 Add encrypted file keys
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18025 )
Change subject: [security] KUDU-3316 Add encrypted file keys
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18025/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18025/3//COMMIT_MSG@30
PS3, Line 30: File sizes as returned by *File APIs are logical, so on disk they're 40
: bytes larger. The GetFileSize() can return both the logical and the
: physical file size, but GetFileSizeOnDisk() actually uses block count
: and must be physical. Some of the tests rely on this and make
: assumptions based on the numbers reported here, these tests are not run
: with encryption enabled.
> You're right, it's better to use physical sizes everywhere and update the c
I'm not sure I understand what's the problem if returning the actual size with the encryption header by GetFileSizeOnDisk() (and that does seem reasonable because of OnDisk suffix) and returning the logical size elsewhere when working with actual data in an encrypted file.
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 3
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Wed, 12 Jan 2022 18:00:51 +0000
Gerrit-HasComments: Yes
[kudu-CR] [encryption] KUDU-3316 Add encrypted file keys
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Zoltan Chovan, Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18025
to look at the new patch set (#6).
Change subject: [encryption] KUDU-3316 Add encrypted file keys
......................................................................
[encryption] KUDU-3316 Add encrypted file keys
The previous patches in the data at rest encryption saga add the ability
to encrypt data at rest, but files were encrypted using the same hard-coded
key.
This patch adds an extra header to encrypted files to store the
encryption algorithm used and the encrypted file key. For now, the file
keys are encrypted with the same dummy encryption key which was
previously used to encrypt the files.
The header is a bit different from the one described in the design doc:
The encryption algorithm and key length was changed to be stored in
1 byte instead of 2 for easier handling and the magic string is
"kuduenc" instead of "kuduen".
This patch also introduces a new flag which is hidden for now:
--encryption_key_length. This can be set to any valid AES key length as
per its specification (128, 192, or 256 bits), as only AES encryption is
supported for now, and there are no plans to support anything else in
the foreseeable future.
As we add a 64-byte header to encrypted files, some changes had to be
made to code that handles files and relies on sizes and offsets,
including in tests.
This commit also changes the PBC tool to check if a file is encrypted
based on the encryption header instead of the file name.
I ran the full test suite manually with encryption enabled to make sure
turning on encryption doesn't break anything.
Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
---
M src/kudu/consensus/log-test.cc
M src/kudu/consensus/log_index.cc
M src/kudu/consensus/log_util.cc
M src/kudu/fs/dir_util.cc
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/log_block_manager-test-util.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_pbc.cc
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_service-test.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/tablet_copy_source_session.cc
M src/kudu/util/env-test.cc
M src/kudu/util/env.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/env_util.cc
M src/kudu/util/env_util.h
M src/kudu/util/file_cache-stress-test.cc
M src/kudu/util/file_cache-test.cc
M src/kudu/util/file_cache.cc
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
26 files changed, 685 insertions(+), 246 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/18025/6
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 6
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [security] KUDU-3316 Add encrypted file keys
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/18025 )
Change subject: [security] KUDU-3316 Add encrypted file keys
......................................................................
[security] KUDU-3316 Add encrypted file keys
The previous patches in the data at rest encryption saga add the ability
to encrypt data at rest, but files were encrypted using the same hard-coded
key.
This patch adds an extra header to encrypted files to store the
encryption algorithm used and the encrypted file key. For now, the file
keys are encrypted with the same dummy encryption key which was
previously used to encrypt the files.
The header is a bit different from the one described in the design doc:
The encryption algorithm and key length was changed to be stored in
1 byte instead of 2 for easier handling and the magic string is
"kuduenc" instead of "kuduen".
This patch also introduces a new flag which is hidden for now:
--encryption_key_length. This can be set to any valid AES key length as
per its specification (128, 192, or 256 bits), as only AES encryption is
supported for now, and there are no plans to support anything else in
the foreseeable future.
As we add a 64-byte header to encrypted files, some changes had to be
made to code that handles files and relies on sizes and offsets,
including in tests. One of these changes is in the LogBlockManager,
which expects blocks to be aligned to file system block boundaries,
which is necessary for hole punching. With encryption enabled, extra
steps are necessary to align the first block, and aligning blocks makes
it impractical to hide encryption header size within Env and use
logical file sizes outside of it.
This commit also changes the PBC tool to check if a file is encrypted
based on the encryption header instead of the file name.
I ran the full test suite manually locally and on dist-test with
encryption enabled to make sure turning on encryption doesn't break
anything:
http://dist-test.cloudera.org/job?job_id=abukor.1643215963.60435
To make running dist-test with encryption enabled possible, this commit
also adds forwarding the KUDU_ENCRYPT_DATA_IN_TESTS environment variable
to dist_test.py.
Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Reviewed-on: http://gerrit.cloudera.org:8080/18025
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong <aw...@cloudera.com>
---
M build-support/dist_test.py
M src/kudu/consensus/consensus_meta-test.cc
M src/kudu/consensus/log-test.cc
M src/kudu/consensus/log_index.cc
M src/kudu/consensus/log_util.cc
M src/kudu/fs/dir_util.cc
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/log_block_manager-test-util.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_pbc.cc
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_service-test.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/tablet_copy_source_session.cc
M src/kudu/util/env-test.cc
M src/kudu/util/env.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/env_util.cc
M src/kudu/util/env_util.h
M src/kudu/util/file_cache-stress-test.cc
M src/kudu/util/file_cache-test.cc
M src/kudu/util/file_cache.cc
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
28 files changed, 715 insertions(+), 255 deletions(-)
Approvals:
Alexey Serbin: Looks good to me, approved
Kudu Jenkins: Verified
Andrew Wong: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 19
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [encryption] KUDU-3316 Add encrypted file keys to files
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, zchovan@cloudera.com, Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18025
to look at the new patch set (#3).
Change subject: [encryption] KUDU-3316 Add encrypted file keys to files
......................................................................
[encryption] KUDU-3316 Add encrypted file keys to files
The previous patches in the data at rest encryption saga add the ability
to encrypt data at rest, but files were encrypted using the same hard-coded
key.
This patch adds an extra header to encrypted files to store the
encryption algorithm used and the encrypted file key. For now, the file
keys are encrypted with the same dummy encryption key.
The header is a bit different from the one described in the design doc,
because the writes don't have to be aligned to the block size after all,
so I saw no reason to pad to 64 bytes, so the header is now only 40
bytes. The encryption algorithm and key length was also changed to be
stored in 1 byte instead of 2 for easier handling. The magic string is
"kuduenc" instead of "kuduen".
This patch also introduces a new flag which is hidden for now:
--encryption_key_length. This can be set to any valid AES key length as
per its specification (128, 192, or 256 bits), as only AES encryption is
supported for now, and there are no plans to support anything else in
the foreseeable future.
File sizes as returned by *File APIs are logical, so on disk they're 40
bytes larger. The GetFileSize() can return both the logical and the
physical file size, but GetFileSizeOnDisk() actually uses block count
and must be physical. Some of the tests rely on this and make
assumptions based on the numbers reported here, these tests are not run
with encryption enabled.
Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
---
M src/kudu/consensus/consensus_meta.cc
M src/kudu/consensus/log-test-base.h
M src/kudu/fs/dir_util.cc
M src/kudu/fs/log_block_manager-test-util.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/util/env-test.cc
M src/kudu/util/env.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/env_util.cc
M src/kudu/util/pb_util-test.cc
M src/kudu/util/rolling_log-test.cc
15 files changed, 367 insertions(+), 99 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/18025/3
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 3
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <zc...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [security] KUDU-3316 Add encrypted file keys
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18025 )
Change subject: [security] KUDU-3316 Add encrypted file keys
......................................................................
Patch Set 18: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 18
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Mon, 31 Jan 2022 01:24:43 +0000
Gerrit-HasComments: No
[kudu-CR] [security] KUDU-3316 Add encrypted file keys
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/18025 )
Change subject: [security] KUDU-3316 Add encrypted file keys
......................................................................
Patch Set 15:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18025/14/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
http://gerrit.cloudera.org:8080/#/c/18025/14/src/kudu/util/env_posix.cc@850
PS14, Line 850: (key_size + 15) & -16)};
> Seems we should be able to do this with some bitwise operations.
Done
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 15
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Sat, 29 Jan 2022 00:08:31 +0000
Gerrit-HasComments: Yes
[kudu-CR] [security] KUDU-3316 Add encrypted file keys
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/18025 )
Change subject: [security] KUDU-3316 Add encrypted file keys
......................................................................
Patch Set 10:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/18025/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18025/3//COMMIT_MSG@30
PS3, Line 30: made to code that handles files and relies on sizes and offsets,
: including in tests.
:
: This commit also changes the PBC tool to check if a file is encrypted
: based on the encryption header instead of the file name.
:
> I'm not sure I understand what's the problem if returning the actual size w
OnDisk() means block count * block size, so that's not really the same as file size - header size. Logical size everywhere else is also not enough, because a lot of stuff rely on the actual file size for various reasons (I go into more detail on https://gerrit.cloudera.org/c/18025/8/src/kudu/util/file_cache-test.cc#107).
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/env_util.cc
File src/kudu/util/env_util.cc:
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/env_util.cc@250
PS8, Line 250: uint64_t size;
> Cool! Could you please add a comment with explaining that right in this co
Done
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/file_cache-test.cc
File src/kudu/util/file_cache-test.cc:
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/file_cache-test.cc@103
PS8, Line 103: unique_ptr<RWFile> f;
: RWFileOptions opts;
: opts.is_sensitive = true;
: RETURN_NOT_OK(env_->NewRWFile(opts, name, &f));
: RETURN_NOT_OK(f->Write(f->GetEncryptionHeaderSize(), data));
> Thank you for the explanation. I'm not sure I saw that in one of patch set
PS3 was like this, in that version I add the header size to the offset on reads and writes behind the scenes. Maybe I used a bad abstraction, but I still can't really find a better one.
Let's take a WAL segment as an example. It has a fixed size footer, so the way the offset is calculated is to take the file size and subtract the footer size from it. If the file size is logical, this read will be off by 64 bytes.
Another example is the LBM container file, which requires blocks to be aligned to FS blocks, which are 4k. So if I simply add 64 bytes to the specified offset, nothing will be actually aligned to the FS blocks. The first block would start at offset 64, the next block could start at something like 4160 or 8256. This would make hole punching impossible, as it can only punch whole FS blocks, so for the alignments to work, LBM needs to be aware of the encryption headers.
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 10
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Wed, 12 Jan 2022 19:22:12 +0000
Gerrit-HasComments: Yes
[kudu-CR] [security] KUDU-3316 Add encrypted file keys
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has removed a vote on this change.
Change subject: [security] KUDU-3316 Add encrypted file keys
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 13
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [encryption] KUDU-3316 Add encrypted file keys to files
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
zchovan@cloudera.com has posted comments on this change. ( http://gerrit.cloudera.org:8080/18025 )
Change subject: [encryption] KUDU-3316 Add encrypted file keys to files
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env_posix.cc@1461
PS3, Line 1461: zeross
> nit: IMO this reads like a typo. Consider zero_slice or zero_slc or somethi
+1 nit, maybe call it padding?
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 3
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <zc...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 17 Nov 2021 18:02:01 +0000
Gerrit-HasComments: Yes
[kudu-CR] [encryption] KUDU-3316 Add encrypted file keys
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18025 )
Change subject: [encryption] KUDU-3316 Add encrypted file keys
......................................................................
Patch Set 4:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/18025/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18025/4//COMMIT_MSG@29
PS4, Line 29: 64-
40-byte?
http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/consensus/log.cc
File src/kudu/consensus/log.cc:
http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/consensus/log.cc@552
PS4, Line 552: auto file = active_segment_->file();
: RETURN_NOT_OK(file->Truncate(active_segment_
Merge lines? This makes a copy
http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/fs/log_block_manager.cc@749
PS4, Line 749:
nit: spacing
http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/tserver/tablet_copy_source_session-test.cc
File src/kudu/tserver/tablet_copy_source_session-test.cc:
http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/tserver/tablet_copy_source_session-test.cc@a346
PS4, Line 346:
Why this change? Aren't they both either encrypted or not?
http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/util/env.h
File src/kudu/util/env.h:
http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/util/env.h@378
PS4, Line 378: uint8_t
nit: maybe use size_t? GSG suggests erring away from unsigned except in certain scenarios https://google.github.io/styleguide/cppguide.html#Integer_Types
http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env_posix.cc@2256
PS3, Line 2256: return IOError("stat", errno);
: }
> Yea, startup should/will fail in this case. Do you have any tips on how to
Will ponder further, though I suppose it's unlikely a file will be <40 bytes
http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/util/env_posix.cc@602
PS4, Line 602: //
Uncomment?
http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/util/env_posix.cc@1083
PS4, Line 1083: header_written_
Missing a ! ? Does that mean that we're getting double headers for temp files?
http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/util/env_posix.cc@1104
PS4, Line 1104: {filesize_, pre_allocated_size_}
Why this change? Not a huge change, but the initializer list does add some instructions and changes the called function https://godbolt.org/z/o6bjP3Wsc
push rbp
mov rbp, rsp
sub rsp, 32
mov DWORD PTR [rbp-20], edi
mov eax, DWORD PTR [rbp-20]
add eax, 1
mov DWORD PTR [rbp-4], eax
lea rdx, [rbp-4]
lea rax, [rbp-20]
mov rsi, rdx
mov rdi, rax
call int const& std::max<int>(int const&, int const&)
mov eax, DWORD PTR [rax]
leave
ret
vs
push rbp
mov rbp, rsp
push rbx
sub rsp, 40
mov DWORD PTR [rbp-36], edi
mov ecx, DWORD PTR [rbp-36]
mov DWORD PTR [rbp-24], ecx
mov ecx, DWORD PTR [rbp-36]
add ecx, 1
mov DWORD PTR [rbp-20], ecx
lea rcx, [rbp-24]
mov rax, rcx
mov edx, 2
mov rcx, rax
mov rbx, rdx
mov rax, rdx
mov rdi, rcx
mov rsi, rax
call int std::max<int>(std::initializer_list<int>)
mov rbx, QWORD PTR [rbp-8]
leave
ret
http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/util/env_posix.cc@1657
PS4, Line 1657: , true
I may be missing something, but it seems with its current usage, we could get by removing this, and conditioning whether to write based on 'encrypt', since we seem to always be writing the header for encrypted files before creating the new file. Is that the case?
http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/util/env_posix.cc@2323
PS4, Line 2323: if (file_size < kEncryptionHeaderSize && encrypt) {
: RETURN_NOT_OK(GenerateHeader(&eh));
: RETURN_NOT_OK(WriteEncryptionHeader(fd, fname, eh));
: header_written = true;
: file_size = kEncryptionHeaderSize;
: } else if (encrypt) {
: RETURN_NOT_OK(ReadEncryptionHeader(fd, fname, &eh));
: header_written = true;
: }
nit: maybe less error prone as done elsewhere?
if (encrypt) {
if (file_size < kEncryptionHeaderSize) {
...
} else {
...
}
header_written = true;
}
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Fri, 07 Jan 2022 01:28:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] [security] KUDU-3316 Add encrypted file keys
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Zoltan Chovan, Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18025
to look at the new patch set (#9).
Change subject: [security] KUDU-3316 Add encrypted file keys
......................................................................
[security] KUDU-3316 Add encrypted file keys
The previous patches in the data at rest encryption saga add the ability
to encrypt data at rest, but files were encrypted using the same hard-coded
key.
This patch adds an extra header to encrypted files to store the
encryption algorithm used and the encrypted file key. For now, the file
keys are encrypted with the same dummy encryption key which was
previously used to encrypt the files.
The header is a bit different from the one described in the design doc:
The encryption algorithm and key length was changed to be stored in
1 byte instead of 2 for easier handling and the magic string is
"kuduenc" instead of "kuduen".
This patch also introduces a new flag which is hidden for now:
--encryption_key_length. This can be set to any valid AES key length as
per its specification (128, 192, or 256 bits), as only AES encryption is
supported for now, and there are no plans to support anything else in
the foreseeable future.
As we add a 64-byte header to encrypted files, some changes had to be
made to code that handles files and relies on sizes and offsets,
including in tests.
This commit also changes the PBC tool to check if a file is encrypted
based on the encryption header instead of the file name.
I ran the full test suite manually with encryption enabled to make sure
turning on encryption doesn't break anything.
Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
---
M src/kudu/consensus/consensus_meta-test.cc
M src/kudu/consensus/log-test.cc
M src/kudu/consensus/log_index.cc
M src/kudu/consensus/log_util.cc
M src/kudu/fs/dir_util.cc
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/log_block_manager-test-util.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_pbc.cc
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_service-test.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/tablet_copy_source_session.cc
M src/kudu/util/env-test.cc
M src/kudu/util/env.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/env_util.cc
M src/kudu/util/env_util.h
M src/kudu/util/file_cache-stress-test.cc
M src/kudu/util/file_cache-test.cc
M src/kudu/util/file_cache.cc
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
27 files changed, 698 insertions(+), 249 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/18025/9
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 9
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [security] KUDU-3316 Add encrypted file keys
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18025 )
Change subject: [security] KUDU-3316 Add encrypted file keys
......................................................................
Patch Set 11:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/18025/11/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/18025/11/src/kudu/fs/log_block_manager.cc@662
PS11, Line 662: Should only be called
: // when a block is fully written,
nit: mind updating this?
http://gerrit.cloudera.org:8080/#/c/18025/11/src/kudu/fs/log_block_manager.cc@749
PS11, Line 749: uint8_t
nit: auto or size_t?
http://gerrit.cloudera.org:8080/#/c/18025/11/src/kudu/fs/log_block_manager.cc@750
PS11, Line 750: if (encryption_header_size > 0) {
Please add a comment describing why this is here.
Would be also good to mention in the commit message that there are some special considerations for LBM container files, since this patch is quite opinionated about how we handle them: each container "loses" its first FS-block-size-worth of data, right?
http://gerrit.cloudera.org:8080/#/c/18025/11/src/kudu/fs/log_block_manager.cc@937
PS11, Line 937: a
nit: const?
http://gerrit.cloudera.org:8080/#/c/18025/11/src/kudu/fs/log_block_manager.cc@1095
PS11, Line 1095: record->length() > 0
I'm curious how this changed? What in this patch results in 0-length records?
http://gerrit.cloudera.org:8080/#/c/18025/11/src/kudu/fs/log_block_manager.cc@1318
PS11, Line 1318: if (block_start_offset < FLAGS_log_container_preallocate_bytes) {
Can you add a comment for this too?
http://gerrit.cloudera.org:8080/#/c/18025/11/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
http://gerrit.cloudera.org:8080/#/c/18025/11/src/kudu/util/env_posix.cc@243
PS11, Line 243: AES128ECB = 0xFD,
: AES192ECB = 0XFE,
: AES256ECB = 0xFF,
What's the rationale behind these values, vs just continuing at 0x03?
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/file_cache-test.cc
File src/kudu/util/file_cache-test.cc:
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/file_cache-test.cc@103
PS8, Line 103: unique_ptr<RWFile> f;
: RWFileOptions opts;
: opts.is_sensitive = true;
: RETURN_NOT_OK(env_->NewRWFile(opts, name, &f));
: RETURN_NOT_OK(f->Write(f->GetEncryptionHeaderSize(), data));
> Well, it's not exactly the same, but your proposed approach and PS3 share t
Looking through this again, it seems like whenever we deal with Read() and Write() calls, we are usually supplying logical offsets. In that regard, it seems for those particular APIs we might be able to get by with using logical size.
Re: WALs, perhaps we could extend the file API to have some OffsetFromEnd(int bytes_from_end) that gives a byte offset usable via Read/Write calls.
LBM though, I agree, seems like a different beast entirely, because we persist the physical offsets in log block metadata, and thus cannot use a logical-offset-based scheme. I do think the comments in this patch don't thoroughly explain this. It'd certainly help readers present and future better understand how and why it isn't so straightforward to ubiquitously use logical offsets.
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 11
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Tue, 25 Jan 2022 02:26:51 +0000
Gerrit-HasComments: Yes
[kudu-CR] [security] KUDU-3316 Add encrypted file keys
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18025 )
Change subject: [security] KUDU-3316 Add encrypted file keys
......................................................................
Patch Set 3:
(7 comments)
I took a quick look and since I see the size of the encryption is still floating around, may be it makes sense to re-evaluate the approach I already pointed at https://gerrit.cloudera.org/#/c/18025/3/src/kudu/util/env.h@194 ?
What do you think?
http://gerrit.cloudera.org:8080/#/c/18025/8//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18025/8//COMMIT_MSG@36
PS8, Line 36:
Probably, the encryption stuff is not one of those things which might be affected, but anyways: I found that sometimes it makes sense to check how that works at nodes with inferior hardware to help pin-pointing implicit assumptions in test scenarios.
In that context, does it makes sense to run the suite using dist-test with encryption enabled?
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/consensus/log_index.cc
File src/kudu/consensus/log_index.cc:
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/consensus/log_index.cc@124
PS8, Line 124: Read(sizeof(PhysicalEntry) * entry_in
For the RWFile::Read() and RWFile::Write() below: does it make sense to hide the offset behind the API, i.e. introducing a class derived from RWFile which takes care of the offset?
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/env.h
File src/kudu/util/env.h:
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/env.h@378
PS8, Line 378: al string in
What's the idea behind add 'const' to the size_t output type which is returned by value anyways? I'm not sure this makes much sense.
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/env.h@398
PS8, Line 398:
Drop const?
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/env.h@698
PS8, Line 698:
Is it expected to have a non-const implementation of this method in any of the derived classes? If not, consider making this method constant.
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/env_util.cc
File src/kudu/util/env_util.cc:
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/env_util.cc@250
PS8, Line 250: opts.is_sensitive = false;
This looks a bit strange: WritableFileOptions are unconditionally overwritten. Instead, does it seem feasible to specify proper options at corresponding call sites?
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/file_cache-test.cc
File src/kudu/util/file_cache-test.cc:
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/file_cache-test.cc@103
PS8, Line 103: unique_ptr<RWFile> f;
: RWFileOptions opts;
: opts.is_sensitive = true;
: RETURN_NOT_OK(env_->NewRWFile(opts, name, &f));
: RETURN_NOT_OK(f->Write(0, data));
Instead of passing around the information on the enc header size and offset, did you consider an alternative to introduce a new sub-type of RWFile (say, EncryptedRWFile), so that NewWritableFile() would return a proper instance via the shared pointer? Since Read() and Write() are virtual methods in RWFile class, those might be overridden as necessary in EncryptedRWFile, so it would not be necessary to pass around the size of the encrypted header on the upper level.
Same approach might be used for RandomAccessFile and others.
What do you think?
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 3
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Wed, 12 Jan 2022 03:21:58 +0000
Gerrit-HasComments: Yes
[kudu-CR] [security] KUDU-3316 Add encrypted file keys
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Zoltan Chovan, Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18025
to look at the new patch set (#13).
Change subject: [security] KUDU-3316 Add encrypted file keys
......................................................................
[security] KUDU-3316 Add encrypted file keys
The previous patches in the data at rest encryption saga add the ability
to encrypt data at rest, but files were encrypted using the same hard-coded
key.
This patch adds an extra header to encrypted files to store the
encryption algorithm used and the encrypted file key. For now, the file
keys are encrypted with the same dummy encryption key which was
previously used to encrypt the files.
The header is a bit different from the one described in the design doc:
The encryption algorithm and key length was changed to be stored in
1 byte instead of 2 for easier handling and the magic string is
"kuduenc" instead of "kuduen".
This patch also introduces a new flag which is hidden for now:
--encryption_key_length. This can be set to any valid AES key length as
per its specification (128, 192, or 256 bits), as only AES encryption is
supported for now, and there are no plans to support anything else in
the foreseeable future.
As we add a 64-byte header to encrypted files, some changes had to be
made to code that handles files and relies on sizes and offsets,
including in tests. One of these changes is in the LogBlockManager,
which expects blocks to be aligned to file system block boundaries,
which is necessary for hole punching. With encryption enabled, extra
steps are necessary to align the first block, and aligning blocks makes
it impractical to hide encryption header size within Env and use
logical file sizes outside of it.
This commit also changes the PBC tool to check if a file is encrypted
based on the encryption header instead of the file name.
I ran the full test suite manually with encryption enabled to make sure
turning on encryption doesn't break anything.
Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
---
M src/kudu/consensus/consensus_meta-test.cc
M src/kudu/consensus/log-test.cc
M src/kudu/consensus/log_index.cc
M src/kudu/consensus/log_util.cc
M src/kudu/fs/dir_util.cc
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/log_block_manager-test-util.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_pbc.cc
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_service-test.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/tablet_copy_source_session.cc
M src/kudu/util/env-test.cc
M src/kudu/util/env.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/env_util.cc
M src/kudu/util/env_util.h
M src/kudu/util/file_cache-stress-test.cc
M src/kudu/util/file_cache-test.cc
M src/kudu/util/file_cache.cc
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
27 files changed, 715 insertions(+), 255 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/18025/13
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 13
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [security] KUDU-3316 Add encrypted file keys
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/18025 )
Change subject: [security] KUDU-3316 Add encrypted file keys
......................................................................
Patch Set 18:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18025/17/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
http://gerrit.cloudera.org:8080/#/c/18025/17/src/kudu/util/env_posix.cc@849
PS17, Line 849: (key_size + 15) & -16)
> Well, and actually BitUtil::Ceil() does a different thing: it divides the n
BitUtil::Ceil() was pretty close to my original approach without having to use doubles, so it would've been an optimization. I think my current approach only works for powers of 2, so it wouldn't help with a general approach like Ceil() anyway.
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 18
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Sun, 30 Jan 2022 13:25:34 +0000
Gerrit-HasComments: Yes
[kudu-CR] [security] KUDU-3316 Add encrypted file keys
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/18025 )
Change subject: [security] KUDU-3316 Add encrypted file keys
......................................................................
Patch Set 18:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/18025/17/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/18025/17/src/kudu/fs/log_block_manager.cc@749
PS17, Line 749: // If we have an encryption header, we need to align the next offset to the
: // next file system block.
: if (auto encryption_header_size = data_file_->GetEncryptionHeaderSize();
: encryption_header_size > 0) {
> nit: since it's C++17 code, it's possible to put the related variable under
Wow I wasn't aware of this feature, thanks for the tip!
http://gerrit.cloudera.org:8080/#/c/18025/17/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
http://gerrit.cloudera.org:8080/#/c/18025/17/src/kudu/util/env_posix.cc@849
PS17, Line 849: (key_size + 15) & -16)
> nit: there is BitUtil::Ceil() for that if you'd like to give the code a bit
It seems the difference between BitUtil::Ceil() and this bitwise trick is non-trivial (gcc 11.2 with -O3):
Bitwise magic
lea eax, [rdi+15]
and eax, -16
BitUtil::Ceil()
xor eax, eax
test dil, 15
lea edx, [rdi+15]
setne al
test edi, edi
cmovns edx, edi
sar edx, 4
add eax, edx
sal eax, 4
ret
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 18
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Sat, 29 Jan 2022 23:54:41 +0000
Gerrit-HasComments: Yes
[kudu-CR] [encryption] KUDU-3316 Add encrypted file keys to files
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18025 )
Change subject: [encryption] KUDU-3316 Add encrypted file keys to files
......................................................................
Patch Set 3:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/18025/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18025/3//COMMIT_MSG@30
PS3, Line 30: File sizes as returned by *File APIs are logical, so on disk they're 40
: bytes larger. The GetFileSize() can return both the logical and the
: physical file size, but GetFileSizeOnDisk() actually uses block count
: and must be physical. Some of the tests rely on this and make
: assumptions based on the numbers reported here, these tests are not run
: with encryption enabled.
I'm curious what the policy is for using physical vs logical size then. I would have thought we'd want to use physical size everywhere we possibly can, since it's as accurate for an end-user with regards to what is using space. Though it seems like that's not the case for some files? eg consensus and tablet metadata?
http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/consensus/consensus_meta.cc
File src/kudu/consensus/consensus_meta.cc:
http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/consensus/consensus_meta.cc@416
PS3, Line 416: false
Shouldn't this show the header too?
http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env_posix.cc@1431
PS3, Line 1431: EncryptionHeader
Should be a const ref to avoid a copy
http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env_posix.cc@1432
PS3, Line 1432: "kuduenc"
nit: consider defining this once.
Also extra semicolon
http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env_posix.cc@1460
PS3, Line 1460: uint8_t zeros[16] = {0};
Could be const and static?
http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env_posix.cc@1461
PS3, Line 1461: zeross
nit: IMO this reads like a typo. Consider zero_slice or zero_slc or something?
http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env_posix.cc@1582
PS3, Line 1582: ;;
nit: extra semicolon
http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env_posix.cc@1710
PS3, Line 1710: exclude_encryption_header
nit: consider some WITH_HEADER enum?
http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env_posix.cc@2256
PS3, Line 2256: RETURN_NOT_OK(GenerateHeader(&eh));
: RETURN_NOT_OK(WriteEncryptionHeader(fd, eh));
Is there any concern that a user accidentally switches between encryption and no encryption, and overwrites some data in this way?
I guess not, since presumably we'd fail to start up if the instance files differ in encryption status before doing any real damage, but still worth thinking about.
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 3
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <zc...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 16 Nov 2021 21:10:24 +0000
Gerrit-HasComments: Yes
[kudu-CR] [security] KUDU-3316 Add encrypted file keys
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Zoltan Chovan, Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18025
to look at the new patch set (#12).
Change subject: [security] KUDU-3316 Add encrypted file keys
......................................................................
[security] KUDU-3316 Add encrypted file keys
The previous patches in the data at rest encryption saga add the ability
to encrypt data at rest, but files were encrypted using the same hard-coded
key.
This patch adds an extra header to encrypted files to store the
encryption algorithm used and the encrypted file key. For now, the file
keys are encrypted with the same dummy encryption key which was
previously used to encrypt the files.
The header is a bit different from the one described in the design doc:
The encryption algorithm and key length was changed to be stored in
1 byte instead of 2 for easier handling and the magic string is
"kuduenc" instead of "kuduen".
This patch also introduces a new flag which is hidden for now:
--encryption_key_length. This can be set to any valid AES key length as
per its specification (128, 192, or 256 bits), as only AES encryption is
supported for now, and there are no plans to support anything else in
the foreseeable future.
As we add a 64-byte header to encrypted files, some changes had to be
made to code that handles files and relies on sizes and offsets,
including in tests. One of these changes is in the LogBlockManager,
which expects blocks to be aligned to file system block boundaries,
which is necessary for hole punching. With encryption enabled, extra
steps are necessary to align the first block, and aligning blocks makes
it impractical to hide encryption header size within Env and use
logical file sizes outside of it.
This commit also changes the PBC tool to check if a file is encrypted
based on the encryption header instead of the file name.
I ran the full test suite manually with encryption enabled to make sure
turning on encryption doesn't break anything.
Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
---
M src/kudu/consensus/consensus_meta-test.cc
M src/kudu/consensus/log-test.cc
M src/kudu/consensus/log_index.cc
M src/kudu/consensus/log_util.cc
M src/kudu/fs/dir_util.cc
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/log_block_manager-test-util.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_pbc.cc
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_service-test.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/tablet_copy_source_session.cc
M src/kudu/util/env-test.cc
M src/kudu/util/env.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/env_util.cc
M src/kudu/util/env_util.h
M src/kudu/util/file_cache-stress-test.cc
M src/kudu/util/file_cache-test.cc
M src/kudu/util/file_cache.cc
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
27 files changed, 714 insertions(+), 254 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/18025/12
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 12
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [security] KUDU-3316 Add encrypted file keys
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Zoltan Chovan, Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18025
to look at the new patch set (#15).
Change subject: [security] KUDU-3316 Add encrypted file keys
......................................................................
[security] KUDU-3316 Add encrypted file keys
The previous patches in the data at rest encryption saga add the ability
to encrypt data at rest, but files were encrypted using the same hard-coded
key.
This patch adds an extra header to encrypted files to store the
encryption algorithm used and the encrypted file key. For now, the file
keys are encrypted with the same dummy encryption key which was
previously used to encrypt the files.
The header is a bit different from the one described in the design doc:
The encryption algorithm and key length was changed to be stored in
1 byte instead of 2 for easier handling and the magic string is
"kuduenc" instead of "kuduen".
This patch also introduces a new flag which is hidden for now:
--encryption_key_length. This can be set to any valid AES key length as
per its specification (128, 192, or 256 bits), as only AES encryption is
supported for now, and there are no plans to support anything else in
the foreseeable future.
As we add a 64-byte header to encrypted files, some changes had to be
made to code that handles files and relies on sizes and offsets,
including in tests. One of these changes is in the LogBlockManager,
which expects blocks to be aligned to file system block boundaries,
which is necessary for hole punching. With encryption enabled, extra
steps are necessary to align the first block, and aligning blocks makes
it impractical to hide encryption header size within Env and use
logical file sizes outside of it.
This commit also changes the PBC tool to check if a file is encrypted
based on the encryption header instead of the file name.
I ran the full test suite manually locally and on dist-test with
encryption enabled to make sure turning on encryption doesn't break
anything:
http://dist-test.cloudera.org/job?job_id=abukor.1643215963.60435
Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
---
M build-support/dist_test.py
M src/kudu/consensus/consensus_meta-test.cc
M src/kudu/consensus/log-test.cc
M src/kudu/consensus/log_index.cc
M src/kudu/consensus/log_util.cc
M src/kudu/fs/dir_util.cc
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/log_block_manager-test-util.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_pbc.cc
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_service-test.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/tablet_copy_source_session.cc
M src/kudu/util/env-test.cc
M src/kudu/util/env.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/env_util.cc
M src/kudu/util/env_util.h
M src/kudu/util/file_cache-stress-test.cc
M src/kudu/util/file_cache-test.cc
M src/kudu/util/file_cache.cc
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
28 files changed, 716 insertions(+), 255 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/18025/15
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 15
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [security] KUDU-3316 Add encrypted file keys
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/18025 )
Change subject: [security] KUDU-3316 Add encrypted file keys
......................................................................
Patch Set 14:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/18025/14//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18025/14//COMMIT_MSG@33
PS14, Line 33: With encryption enabled, extra
: steps are necessary to align the first block, and aligning blocks makes
: it impractical to hide encryption header size within Env and use
: logical file sizes outside of it.
> One thought here is: could we still do the extra steps mentioned, and still
Technically, I think it would be possible. I started to work on a POC to see what this would look like, but unfortunately, so far it looks like we would gain nothing with this approach, as it would simplify some parts, but complicate others. Not sure if it's worth pursuing this, at least not right now. As this is not part of the public API, we can change the contract later if we decide to refactor it.
http://gerrit.cloudera.org:8080/#/c/18025/14/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/18025/14/src/kudu/tools/kudu-tool-test.cc@2167
PS14, Line 2167:
> nit: there should be 4 spaces after the previous lines for continuations
Done
http://gerrit.cloudera.org:8080/#/c/18025/14/src/kudu/tools/kudu-tool-test.cc@2209
PS14, Line 2209: ?
> nit: funny spacing
Done
http://gerrit.cloudera.org:8080/#/c/18025/14/src/kudu/tserver/tablet_copy_service-test.cc
File src/kudu/tserver/tablet_copy_service-test.cc:
http://gerrit.cloudera.org:8080/#/c/18025/14/src/kudu/tserver/tablet_copy_service-test.cc@395
PS14, Line 395:
> nit: spacing
Done
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 14
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Sat, 29 Jan 2022 00:03:40 +0000
Gerrit-HasComments: Yes
[kudu-CR] [security] KUDU-3316 Add encrypted file keys
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18025 )
Change subject: [security] KUDU-3316 Add encrypted file keys
......................................................................
Patch Set 17:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18025/17/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
http://gerrit.cloudera.org:8080/#/c/18025/17/src/kudu/util/env_posix.cc@849
PS17, Line 849: (key_size + 15) & -16)
nit: there is BitUtil::Ceil() for that if you'd like to give the code a bit more readability (I guess the compiler is able to perform proper optimizations based on the divisor)
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 17
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Sat, 29 Jan 2022 21:45:39 +0000
Gerrit-HasComments: Yes
[kudu-CR] [security] KUDU-3316 Add encrypted file keys
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/18025 )
Change subject: [security] KUDU-3316 Add encrypted file keys
......................................................................
Patch Set 14:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18025/8//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18025/8//COMMIT_MSG@36
PS8, Line 36: of it.
> Thanks, I'll try that.
Ran it on dist-test, it worked fine. Thanks for the suggestion.
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 14
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Wed, 26 Jan 2022 17:18:58 +0000
Gerrit-HasComments: Yes
[kudu-CR] [security] KUDU-3316 Add encrypted file keys
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18025 )
Change subject: [security] KUDU-3316 Add encrypted file keys
......................................................................
Patch Set 17: Code-Review+2
(1 comment)
Overall looks good to me! It's worth it checking if Andrew has some extra feedback.
http://gerrit.cloudera.org:8080/#/c/18025/17/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/18025/17/src/kudu/fs/log_block_manager.cc@749
PS17, Line 749: auto encryption_header_size = data_file_->GetEncryptionHeaderSize();
: // If we have an encryption header, we need to align the next offset to the
: // next file system block.
: if (encryption_header_size > 0) {
nit: since it's C++17 code, it's possible to put the related variable under 'if ()' clause as well: you can see https://github.com/apache/kudu/blob/c2e3efdcefd1de65d805749ba3198adfe9dba79d/src/kudu/client/client.cc#L1793-L1798 as an example and https://en.cppreference.com/w/cpp/language/if for the reference (check the 'init-statement' part)
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 17
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Sat, 29 Jan 2022 21:31:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] [security] KUDU-3316 Add encrypted file keys
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18025 )
Change subject: [security] KUDU-3316 Add encrypted file keys
......................................................................
Patch Set 15: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18025/8//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18025/8//COMMIT_MSG@36
PS8, Line 36: of it.
> Ran it on dist-test, it worked fine. Thanks for the suggestion.
Thank you for verifying that!
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 15
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Sat, 29 Jan 2022 00:08:25 +0000
Gerrit-HasComments: Yes
[kudu-CR] [security] KUDU-3316 Add encrypted file keys
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18025 )
Change subject: [security] KUDU-3316 Add encrypted file keys
......................................................................
Patch Set 18: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18025/17/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
http://gerrit.cloudera.org:8080/#/c/18025/17/src/kudu/util/env_posix.cc@849
PS17, Line 849: (key_size + 15) & -16)
> It seems the difference between BitUtil::Ceil() and this bitwise trick is n
Ah, indeed! Thanks for checking -- it seems there is a room for improvement in BitUtil::Ceil()
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 18
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Sun, 30 Jan 2022 02:43:38 +0000
Gerrit-HasComments: Yes
[kudu-CR] [encryption] KUDU-3316 Add encrypted file keys
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Zoltan Chovan, Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18025
to look at the new patch set (#7).
Change subject: [encryption] KUDU-3316 Add encrypted file keys
......................................................................
[encryption] KUDU-3316 Add encrypted file keys
The previous patches in the data at rest encryption saga add the ability
to encrypt data at rest, but files were encrypted using the same hard-coded
key.
This patch adds an extra header to encrypted files to store the
encryption algorithm used and the encrypted file key. For now, the file
keys are encrypted with the same dummy encryption key which was
previously used to encrypt the files.
The header is a bit different from the one described in the design doc:
The encryption algorithm and key length was changed to be stored in
1 byte instead of 2 for easier handling and the magic string is
"kuduenc" instead of "kuduen".
This patch also introduces a new flag which is hidden for now:
--encryption_key_length. This can be set to any valid AES key length as
per its specification (128, 192, or 256 bits), as only AES encryption is
supported for now, and there are no plans to support anything else in
the foreseeable future.
As we add a 64-byte header to encrypted files, some changes had to be
made to code that handles files and relies on sizes and offsets,
including in tests.
This commit also changes the PBC tool to check if a file is encrypted
based on the encryption header instead of the file name.
I ran the full test suite manually with encryption enabled to make sure
turning on encryption doesn't break anything.
Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
---
M src/kudu/consensus/log-test.cc
M src/kudu/consensus/log_index.cc
M src/kudu/consensus/log_util.cc
M src/kudu/fs/dir_util.cc
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/log_block_manager-test-util.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_pbc.cc
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_service-test.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/tablet_copy_source_session.cc
M src/kudu/util/env-test.cc
M src/kudu/util/env.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/env_util.cc
M src/kudu/util/env_util.h
M src/kudu/util/file_cache-stress-test.cc
M src/kudu/util/file_cache-test.cc
M src/kudu/util/file_cache.cc
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
26 files changed, 687 insertions(+), 246 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/18025/7
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 7
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [security] KUDU-3316 Add encrypted file keys
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Zoltan Chovan, Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18025
to look at the new patch set (#8).
Change subject: [security] KUDU-3316 Add encrypted file keys
......................................................................
[security] KUDU-3316 Add encrypted file keys
The previous patches in the data at rest encryption saga add the ability
to encrypt data at rest, but files were encrypted using the same hard-coded
key.
This patch adds an extra header to encrypted files to store the
encryption algorithm used and the encrypted file key. For now, the file
keys are encrypted with the same dummy encryption key which was
previously used to encrypt the files.
The header is a bit different from the one described in the design doc:
The encryption algorithm and key length was changed to be stored in
1 byte instead of 2 for easier handling and the magic string is
"kuduenc" instead of "kuduen".
This patch also introduces a new flag which is hidden for now:
--encryption_key_length. This can be set to any valid AES key length as
per its specification (128, 192, or 256 bits), as only AES encryption is
supported for now, and there are no plans to support anything else in
the foreseeable future.
As we add a 64-byte header to encrypted files, some changes had to be
made to code that handles files and relies on sizes and offsets,
including in tests.
This commit also changes the PBC tool to check if a file is encrypted
based on the encryption header instead of the file name.
I ran the full test suite manually with encryption enabled to make sure
turning on encryption doesn't break anything.
Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
---
M src/kudu/consensus/log-test.cc
M src/kudu/consensus/log_index.cc
M src/kudu/consensus/log_util.cc
M src/kudu/fs/dir_util.cc
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/log_block_manager-test-util.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_pbc.cc
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_service-test.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/tablet_copy_source_session.cc
M src/kudu/util/env-test.cc
M src/kudu/util/env.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/env_util.cc
M src/kudu/util/env_util.h
M src/kudu/util/file_cache-stress-test.cc
M src/kudu/util/file_cache-test.cc
M src/kudu/util/file_cache.cc
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
26 files changed, 687 insertions(+), 246 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/18025/8
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 8
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [security] KUDU-3316 Add encrypted file keys
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Zoltan Chovan, Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18025
to look at the new patch set (#10).
Change subject: [security] KUDU-3316 Add encrypted file keys
......................................................................
[security] KUDU-3316 Add encrypted file keys
The previous patches in the data at rest encryption saga add the ability
to encrypt data at rest, but files were encrypted using the same hard-coded
key.
This patch adds an extra header to encrypted files to store the
encryption algorithm used and the encrypted file key. For now, the file
keys are encrypted with the same dummy encryption key which was
previously used to encrypt the files.
The header is a bit different from the one described in the design doc:
The encryption algorithm and key length was changed to be stored in
1 byte instead of 2 for easier handling and the magic string is
"kuduenc" instead of "kuduen".
This patch also introduces a new flag which is hidden for now:
--encryption_key_length. This can be set to any valid AES key length as
per its specification (128, 192, or 256 bits), as only AES encryption is
supported for now, and there are no plans to support anything else in
the foreseeable future.
As we add a 64-byte header to encrypted files, some changes had to be
made to code that handles files and relies on sizes and offsets,
including in tests.
This commit also changes the PBC tool to check if a file is encrypted
based on the encryption header instead of the file name.
I ran the full test suite manually with encryption enabled to make sure
turning on encryption doesn't break anything.
Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
---
M src/kudu/consensus/consensus_meta-test.cc
M src/kudu/consensus/log-test.cc
M src/kudu/consensus/log_index.cc
M src/kudu/consensus/log_util.cc
M src/kudu/fs/dir_util.cc
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/log_block_manager-test-util.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_pbc.cc
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_service-test.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/tablet_copy_source_session.cc
M src/kudu/util/env-test.cc
M src/kudu/util/env.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/env_util.cc
M src/kudu/util/env_util.h
M src/kudu/util/file_cache-stress-test.cc
M src/kudu/util/file_cache-test.cc
M src/kudu/util/file_cache.cc
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
27 files changed, 702 insertions(+), 249 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/18025/10
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 10
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [security] KUDU-3316 Add encrypted file keys
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18025 )
Change subject: [security] KUDU-3316 Add encrypted file keys
......................................................................
Patch Set 8:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/18025/9/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
http://gerrit.cloudera.org:8080/#/c/18025/9/src/kudu/util/env_posix.cc@1196
PS9, Line 1196: retur
style nit here and elsewhere with overridden virtual methods: drop 'virtual' since 'override' is already enough to express that the method is virtual. Not sure whether it's better to do so in this or in a separate changelist since there are other methods in those interfaces which have extra 'virtual' fluff along with the 'override' attribute'
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/file_cache-test.cc
File src/kudu/util/file_cache-test.cc:
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/file_cache-test.cc@103
PS8, Line 103: unique_ptr<RWFile> f;
: RWFileOptions opts;
: opts.is_sensitive = true;
: RETURN_NOT_OK(env_->NewRWFile(opts, name, &f));
: RETURN_NOT_OK(f->Write(f->GetEncryptionHeaderSize(), data));
> PS3 was like this, in that version I add the header size to the offset on r
I don't see that PS3 implements the proposed approach -- the decision whether to add or not the offset is always sort of manual. But frankly, I didn't try implementing the proposed approach myself, so most likely I'm missing a few important points there (maybe that's about the difference between logical/on-disk/etc sizes, etc.).
OK, if you are sure those issues cannot be handled properly by the approach suggested, I'm fine with the alternative once you are sure the code is more maintainable this way.
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 8
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Fri, 14 Jan 2022 06:20:27 +0000
Gerrit-HasComments: Yes
[kudu-CR] [encryption] KUDU-3316 Add encrypted file keys to files
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18025 )
Change subject: [encryption] KUDU-3316 Add encrypted file keys to files
......................................................................
Patch Set 3:
(3 comments)
just one high-level question for now
http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env.h
File src/kudu/util/env.h:
http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env.h@194
PS3, Line 194: exclude_encryption_header
Looking at proliferation of extra parameters for the methods like this, I started thinking: maybe deriving special EncryptedEnv from Env and using it to access encrypted files could be an alternative here? Did you explore this alternative approach?
http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env.h@730
PS3, Line 730: WriteStringToFileSyncEncrypted
nit: maybe, name this WriteStringToFileEncryptedSync()? It seems the naming convention is to have Xxx() and XxxSync() counterpart, where the Sync prefix changes the behavior by calling Sync() after the operation.
http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env_posix.cc@919
PS3, Line 919: *size = st.st_size - (encrypted_ ? kEncryptionHeaderSize : 0);
Consider adding DCHECK()/CHECK() on st.st_size being greater or equal to kEncryptionHeaderSize
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 3
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Wed, 24 Nov 2021 17:33:47 +0000
Gerrit-HasComments: Yes
[kudu-CR] [encryption] KUDU-3316 Add encrypted file keys
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/18025 )
Change subject: [encryption] KUDU-3316 Add encrypted file keys
......................................................................
Patch Set 4:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/18025/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18025/4//COMMIT_MSG@29
PS4, Line 29: 64-
> 40-byte?
I actually ended up using 64 bytes after all. It's not needed for alignments as specified in the design doc, but it's still good to have some bytes reserved for future use in case we want too add support for IV or other algorithms using larger keys in the future. Also, it's easier to see where the header ends and the data starts when looking at a hexdump :)
http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/consensus/log.cc
File src/kudu/consensus/log.cc:
http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/consensus/log.cc@552
PS4, Line 552: auto file = active_segment_->file();
: RETURN_NOT_OK(file->Truncate(active_segment_
> Merge lines? This makes a copy
Done
http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/fs/log_block_manager.cc@749
PS4, Line 749:
> nit: spacing
what's wrong with the spacing here?
http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/tserver/tablet_copy_source_session-test.cc
File src/kudu/tserver/tablet_copy_source_session-test.cc:
http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/tserver/tablet_copy_source_session-test.cc@a346
PS4, Line 346:
> Why this change? Aren't they both either encrypted or not?
Yes, but tablet_block_size is the size of only a block, session_block_size is the whole file size of a single-block temp file. Changed it to account for the difference instead of removing the assertion.
http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/util/env.h
File src/kudu/util/env.h:
http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/util/env.h@378
PS4, Line 378: uint8_t
> nit: maybe use size_t? GSG suggests erring away from unsigned except in cer
Done
http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/util/env_posix.cc@602
PS4, Line 602: //
> Uncomment?
Done
http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/util/env_posix.cc@1083
PS4, Line 1083: header_written_
> Missing a ! ? Does that mean that we're getting double headers for temp fil
This was part of another way I tried to write headers (during the first append to a file instead of when opening), but I ended up not using this. Removed it, thanks for bringing my attention to it.
http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/util/env_posix.cc@1104
PS4, Line 1104: {filesize_, pre_allocated_size_}
> Why this change? Not a huge change, but the initializer list does add some
Yea, this is not actually needed now (I needed the max of 3 integers when I was trying to hide the true file size).
http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/util/env_posix.cc@1657
PS4, Line 1657: , true
> I may be missing something, but it seems with its current usage, we could g
Yes, you're right, thanks for bringing this up, left this in by mistake.
http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/util/env_posix.cc@2323
PS4, Line 2323: if (file_size < kEncryptionHeaderSize && encrypt) {
: RETURN_NOT_OK(GenerateHeader(&eh));
: RETURN_NOT_OK(WriteEncryptionHeader(fd, fname, eh));
: header_written = true;
: file_size = kEncryptionHeaderSize;
: } else if (encrypt) {
: RETURN_NOT_OK(ReadEncryptionHeader(fd, fname, &eh));
: header_written = true;
: }
> nit: maybe less error prone as done elsewhere?
Done
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Fri, 07 Jan 2022 10:08:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] [security] KUDU-3316 Add encrypted file keys
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Zoltan Chovan, Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18025
to look at the new patch set (#11).
Change subject: [security] KUDU-3316 Add encrypted file keys
......................................................................
[security] KUDU-3316 Add encrypted file keys
The previous patches in the data at rest encryption saga add the ability
to encrypt data at rest, but files were encrypted using the same hard-coded
key.
This patch adds an extra header to encrypted files to store the
encryption algorithm used and the encrypted file key. For now, the file
keys are encrypted with the same dummy encryption key which was
previously used to encrypt the files.
The header is a bit different from the one described in the design doc:
The encryption algorithm and key length was changed to be stored in
1 byte instead of 2 for easier handling and the magic string is
"kuduenc" instead of "kuduen".
This patch also introduces a new flag which is hidden for now:
--encryption_key_length. This can be set to any valid AES key length as
per its specification (128, 192, or 256 bits), as only AES encryption is
supported for now, and there are no plans to support anything else in
the foreseeable future.
As we add a 64-byte header to encrypted files, some changes had to be
made to code that handles files and relies on sizes and offsets,
including in tests.
This commit also changes the PBC tool to check if a file is encrypted
based on the encryption header instead of the file name.
I ran the full test suite manually with encryption enabled to make sure
turning on encryption doesn't break anything.
Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
---
M src/kudu/consensus/consensus_meta-test.cc
M src/kudu/consensus/log-test.cc
M src/kudu/consensus/log_index.cc
M src/kudu/consensus/log_util.cc
M src/kudu/fs/dir_util.cc
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/log_block_manager-test-util.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_pbc.cc
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_service-test.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/tablet_copy_source_session.cc
M src/kudu/util/env-test.cc
M src/kudu/util/env.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/env_util.cc
M src/kudu/util/env_util.h
M src/kudu/util/file_cache-stress-test.cc
M src/kudu/util/file_cache-test.cc
M src/kudu/util/file_cache.cc
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
27 files changed, 702 insertions(+), 249 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/18025/11
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 11
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [security] KUDU-3316 Add encrypted file keys
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/18025 )
Change subject: [security] KUDU-3316 Add encrypted file keys
......................................................................
Patch Set 11:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/18025/9/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
http://gerrit.cloudera.org:8080/#/c/18025/9/src/kudu/util/env_posix.cc@1196
PS9, Line 1196: size_t
> style nit here and elsewhere with overridden virtual methods: drop 'virtual
Done
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/file_cache-test.cc
File src/kudu/util/file_cache-test.cc:
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/file_cache-test.cc@103
PS8, Line 103: unique_ptr<RWFile> f;
: RWFileOptions opts;
: opts.is_sensitive = true;
: RETURN_NOT_OK(env_->NewRWFile(opts, name, &f));
: RETURN_NOT_OK(f->Write(f->GetEncryptionHeaderSize(), data));
> I don't see that PS3 implements the proposed approach -- the decision wheth
Well, it's not exactly the same, but your proposed approach and PS3 share that the offsets are treated as an implementation detail internal to env, which wouldn't work for the reasons I described above.
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 11
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Fri, 14 Jan 2022 08:44:30 +0000
Gerrit-HasComments: Yes
[kudu-CR] [security] KUDU-3316 Add encrypted file keys
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18025 )
Change subject: [security] KUDU-3316 Add encrypted file keys
......................................................................
Patch Set 11:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/18025/8//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18025/8//COMMIT_MSG@36
PS8, Line 36: manually with encryption enabled
> I don't think it would be necessary, but I guess i wouldn't hurt. Is there
Well, I guess the regular way should work there, like
KUDU_ALLOW_SLOW_TESTS=1 ../../build-support/dist_test.py <other_cmd_line_flags>
If that doesn't work for you, then maybe update the code to run with encryption by default and run the newly built code :)
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/env_posix.cc@827
PS8, Line 827:
> Yea, I agree this is weird, do you have a suggestion for a cleaner approach
One alternative might be passing 'eh' as const pointer and removing the 'decrypt' parameter for DoReadV(). Passing null 'eh' would automatically mean no decryption is needed.
http://gerrit.cloudera.org:8080/#/c/18025/11/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
http://gerrit.cloudera.org:8080/#/c/18025/11/src/kudu/util/env_posix.cc@243
PS11, Line 243: AES128ECB = 0xFD,
: AES192ECB = 0XFE,
: AES256ECB = 0xFF,
> What's the rationale behind these values, vs just continuing at 0x03?
nit: I'd also expect just adding some bit (e.g. the MSB) for ECB vs CTR, like
AES128ECB = 0x80,
AES192ECB = 0x81,
AES256ECB = 0x82,
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 11
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Tue, 25 Jan 2022 05:59:15 +0000
Gerrit-HasComments: Yes
[kudu-CR] [security] KUDU-3316 Add encrypted file keys
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18025 )
Change subject: [security] KUDU-3316 Add encrypted file keys
......................................................................
Patch Set 17:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18025/17/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
http://gerrit.cloudera.org:8080/#/c/18025/17/src/kudu/util/env_posix.cc@849
PS17, Line 849: (key_size + 15) & -16)
> Ah, indeed! Thanks for checking -- it seems there is a room for improvemen
Well, and actually BitUtil::Ceil() does a different thing: it divides the number by divisor, actually :) Sorry for the noise.
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 17
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Sun, 30 Jan 2022 04:24:12 +0000
Gerrit-HasComments: Yes
[kudu-CR] [security] KUDU-3316 Add encrypted file keys
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18025 )
Change subject: [security] KUDU-3316 Add encrypted file keys
......................................................................
Patch Set 14: Code-Review+1
(5 comments)
Looks fine, barring some nits and one more thought regarding logical size implementation
http://gerrit.cloudera.org:8080/#/c/18025/14//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18025/14//COMMIT_MSG@33
PS14, Line 33: With encryption enabled, extra
: steps are necessary to align the first block, and aligning blocks makes
: it impractical to hide encryption header size within Env and use
: logical file sizes outside of it.
One thought here is: could we still do the extra steps mentioned, and still use logical size? E.g. write the header and update the next block to be logical offset (4k - header_size)?
http://gerrit.cloudera.org:8080/#/c/18025/14/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/18025/14/src/kudu/tools/kudu-tool-test.cc@2167
PS14, Line 2167:
nit: there should be 4 spaces after the previous lines for continuations
http://gerrit.cloudera.org:8080/#/c/18025/14/src/kudu/tools/kudu-tool-test.cc@2209
PS14, Line 2209: ?
nit: funny spacing
http://gerrit.cloudera.org:8080/#/c/18025/14/src/kudu/tserver/tablet_copy_service-test.cc
File src/kudu/tserver/tablet_copy_service-test.cc:
http://gerrit.cloudera.org:8080/#/c/18025/14/src/kudu/tserver/tablet_copy_service-test.cc@395
PS14, Line 395:
nit: spacing
http://gerrit.cloudera.org:8080/#/c/18025/14/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
http://gerrit.cloudera.org:8080/#/c/18025/14/src/kudu/util/env_posix.cc@850
PS14, Line 850: ceil(key_size / 16.) * 16)
Seems we should be able to do this with some bitwise operations.
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 14
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Fri, 28 Jan 2022 19:23:28 +0000
Gerrit-HasComments: Yes
[kudu-CR] [encryption] KUDU-3316 Add encrypted file keys to files
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, zchovan@cloudera.com, Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18025
to look at the new patch set (#2).
Change subject: [encryption] KUDU-3316 Add encrypted file keys to files
......................................................................
[encryption] KUDU-3316 Add encrypted file keys to files
The previous patches in the data at rest encryption saga add the ability
to encrypt data at rest, but files were encrypted using the same hard-coded
key.
This patch adds an extra header to encrypted files to store the
encryption algorithm used and the encrypted file key. For now, the file
keys are encrypted with the same dummy encryption key.
The header is a bit different from the one described in the design doc,
because the writes don't have to be aligned to the block size after all,
so I saw no reason to pad to 64 bytes, so the header is now only 40
bytes. The encryption algorithm and key length was also changed to be
stored in 1 byte instead of 2 for easier handling. The magic string is
"kuduenc" instead of "kuduen".
This patch also introduces a new flag which is hidden for now:
--encryption_key_length. This can be set to any valid AES key length as
per its specification (128, 192, or 256 bits), as only AES encryption is
supported for now, and there are no plans to support anything else in
the foreseeable future.
File sizes as returned by *File APIs are logical, so on disk they're 40
bytes larger. The GetFileSize() can return both the logical and the
physical file size, but GetFileSizeOnDisk() actually uses block count
and must be physical. Some of the tests rely on this and make
assumptions based on the numbers reported here, these tests are not run
with encryption enabled.
Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
---
M src/kudu/consensus/consensus_meta.cc
M src/kudu/consensus/log-test-base.h
M src/kudu/fs/dir_util.cc
M src/kudu/fs/log_block_manager-test-util.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/util/env-test.cc
M src/kudu/util/env.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/env_util.cc
M src/kudu/util/pb_util-test.cc
M src/kudu/util/rolling_log-test.cc
15 files changed, 367 insertions(+), 99 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/18025/2
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 2
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <zc...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [security] KUDU-3316 Add encrypted file keys
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Zoltan Chovan, Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18025
to look at the new patch set (#16).
Change subject: [security] KUDU-3316 Add encrypted file keys
......................................................................
[security] KUDU-3316 Add encrypted file keys
The previous patches in the data at rest encryption saga add the ability
to encrypt data at rest, but files were encrypted using the same hard-coded
key.
This patch adds an extra header to encrypted files to store the
encryption algorithm used and the encrypted file key. For now, the file
keys are encrypted with the same dummy encryption key which was
previously used to encrypt the files.
The header is a bit different from the one described in the design doc:
The encryption algorithm and key length was changed to be stored in
1 byte instead of 2 for easier handling and the magic string is
"kuduenc" instead of "kuduen".
This patch also introduces a new flag which is hidden for now:
--encryption_key_length. This can be set to any valid AES key length as
per its specification (128, 192, or 256 bits), as only AES encryption is
supported for now, and there are no plans to support anything else in
the foreseeable future.
As we add a 64-byte header to encrypted files, some changes had to be
made to code that handles files and relies on sizes and offsets,
including in tests. One of these changes is in the LogBlockManager,
which expects blocks to be aligned to file system block boundaries,
which is necessary for hole punching. With encryption enabled, extra
steps are necessary to align the first block, and aligning blocks makes
it impractical to hide encryption header size within Env and use
logical file sizes outside of it.
This commit also changes the PBC tool to check if a file is encrypted
based on the encryption header instead of the file name.
I ran the full test suite manually locally and on dist-test with
encryption enabled to make sure turning on encryption doesn't break
anything:
http://dist-test.cloudera.org/job?job_id=abukor.1643215963.60435
Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
---
M build-support/dist_test.py
M src/kudu/consensus/consensus_meta-test.cc
M src/kudu/consensus/log-test.cc
M src/kudu/consensus/log_index.cc
M src/kudu/consensus/log_util.cc
M src/kudu/fs/dir_util.cc
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/log_block_manager-test-util.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_pbc.cc
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_service-test.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/tablet_copy_source_session.cc
M src/kudu/util/env-test.cc
M src/kudu/util/env.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/env_util.cc
M src/kudu/util/env_util.h
M src/kudu/util/file_cache-stress-test.cc
M src/kudu/util/file_cache-test.cc
M src/kudu/util/file_cache.cc
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
28 files changed, 715 insertions(+), 255 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/18025/16
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 16
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [security] KUDU-3316 Add encrypted file keys
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/18025 )
Change subject: [security] KUDU-3316 Add encrypted file keys
......................................................................
Patch Set 16:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18025/14//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18025/14//COMMIT_MSG@33
PS14, Line 33: With encryption enabled, extra
: steps are necessary to align the first block, and aligning blocks makes
: it impractical to hide encryption header size within Env and use
: logical file sizes outside of it.
> Sure. I'm curious though, is the LBM the main part that would be complicate
I didn't go through every failure, but both LBM and log seemed to be more complicated than initially expected. In LBM, there was some weirdness that I couldn't figure yet figure out causing most of the tests fail, and in log, it wasn't only reading the footer where the offset had to be calculated using the size, but there's also the read_up_to, which is used when reading entries without a footer. Even after adjusting this, there were still corruption failures in most tests. I'm sure it would be possible to fix all of these, but based on how many changes I had to do, it wouldn't really make a difference in the complexity. I guess it's just that no one has foreseen that there could be differences in how size and offsets are being calculated.
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 16
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Sat, 29 Jan 2022 08:21:29 +0000
Gerrit-HasComments: Yes
[kudu-CR] [security] KUDU-3316 Add encrypted file keys
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Zoltan Chovan, Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18025
to look at the new patch set (#14).
Change subject: [security] KUDU-3316 Add encrypted file keys
......................................................................
[security] KUDU-3316 Add encrypted file keys
The previous patches in the data at rest encryption saga add the ability
to encrypt data at rest, but files were encrypted using the same hard-coded
key.
This patch adds an extra header to encrypted files to store the
encryption algorithm used and the encrypted file key. For now, the file
keys are encrypted with the same dummy encryption key which was
previously used to encrypt the files.
The header is a bit different from the one described in the design doc:
The encryption algorithm and key length was changed to be stored in
1 byte instead of 2 for easier handling and the magic string is
"kuduenc" instead of "kuduen".
This patch also introduces a new flag which is hidden for now:
--encryption_key_length. This can be set to any valid AES key length as
per its specification (128, 192, or 256 bits), as only AES encryption is
supported for now, and there are no plans to support anything else in
the foreseeable future.
As we add a 64-byte header to encrypted files, some changes had to be
made to code that handles files and relies on sizes and offsets,
including in tests. One of these changes is in the LogBlockManager,
which expects blocks to be aligned to file system block boundaries,
which is necessary for hole punching. With encryption enabled, extra
steps are necessary to align the first block, and aligning blocks makes
it impractical to hide encryption header size within Env and use
logical file sizes outside of it.
This commit also changes the PBC tool to check if a file is encrypted
based on the encryption header instead of the file name.
I ran the full test suite manually locally and on dist-test with
encryption enabled to make sure turning on encryption doesn't break
anything:
http://dist-test.cloudera.org/job?job_id=abukor.1643215963.60435
Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
---
M build-support/dist_test.py
M src/kudu/consensus/consensus_meta-test.cc
M src/kudu/consensus/log-test.cc
M src/kudu/consensus/log_index.cc
M src/kudu/consensus/log_util.cc
M src/kudu/fs/dir_util.cc
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/log_block_manager-test-util.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_pbc.cc
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_service-test.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/tablet_copy_source_session.cc
M src/kudu/util/env-test.cc
M src/kudu/util/env.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/env_util.cc
M src/kudu/util/env_util.h
M src/kudu/util/file_cache-stress-test.cc
M src/kudu/util/file_cache-test.cc
M src/kudu/util/file_cache.cc
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
28 files changed, 716 insertions(+), 255 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/18025/14
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 14
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [security] KUDU-3316 Add encrypted file keys
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Zoltan Chovan, Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18025
to look at the new patch set (#18).
Change subject: [security] KUDU-3316 Add encrypted file keys
......................................................................
[security] KUDU-3316 Add encrypted file keys
The previous patches in the data at rest encryption saga add the ability
to encrypt data at rest, but files were encrypted using the same hard-coded
key.
This patch adds an extra header to encrypted files to store the
encryption algorithm used and the encrypted file key. For now, the file
keys are encrypted with the same dummy encryption key which was
previously used to encrypt the files.
The header is a bit different from the one described in the design doc:
The encryption algorithm and key length was changed to be stored in
1 byte instead of 2 for easier handling and the magic string is
"kuduenc" instead of "kuduen".
This patch also introduces a new flag which is hidden for now:
--encryption_key_length. This can be set to any valid AES key length as
per its specification (128, 192, or 256 bits), as only AES encryption is
supported for now, and there are no plans to support anything else in
the foreseeable future.
As we add a 64-byte header to encrypted files, some changes had to be
made to code that handles files and relies on sizes and offsets,
including in tests. One of these changes is in the LogBlockManager,
which expects blocks to be aligned to file system block boundaries,
which is necessary for hole punching. With encryption enabled, extra
steps are necessary to align the first block, and aligning blocks makes
it impractical to hide encryption header size within Env and use
logical file sizes outside of it.
This commit also changes the PBC tool to check if a file is encrypted
based on the encryption header instead of the file name.
I ran the full test suite manually locally and on dist-test with
encryption enabled to make sure turning on encryption doesn't break
anything:
http://dist-test.cloudera.org/job?job_id=abukor.1643215963.60435
To make running dist-test with encryption enabled possible, this commit
also adds forwarding the KUDU_ENCRYPT_DATA_IN_TESTS environment variable
to dist_test.py.
Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
---
M build-support/dist_test.py
M src/kudu/consensus/consensus_meta-test.cc
M src/kudu/consensus/log-test.cc
M src/kudu/consensus/log_index.cc
M src/kudu/consensus/log_util.cc
M src/kudu/fs/dir_util.cc
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/log_block_manager-test-util.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_pbc.cc
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_service-test.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/tablet_copy_source_session.cc
M src/kudu/util/env-test.cc
M src/kudu/util/env.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/env_util.cc
M src/kudu/util/env_util.h
M src/kudu/util/file_cache-stress-test.cc
M src/kudu/util/file_cache-test.cc
M src/kudu/util/file_cache.cc
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
28 files changed, 715 insertions(+), 255 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/18025/18
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 18
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [encryption] KUDU-3316 Add encrypted file keys
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Zoltan Chovan, Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18025
to look at the new patch set (#5).
Change subject: [encryption] KUDU-3316 Add encrypted file keys
......................................................................
[encryption] KUDU-3316 Add encrypted file keys
The previous patches in the data at rest encryption saga add the ability
to encrypt data at rest, but files were encrypted using the same hard-coded
key.
This patch adds an extra header to encrypted files to store the
encryption algorithm used and the encrypted file key. For now, the file
keys are encrypted with the same dummy encryption key which was
previously used to encrypt the files.
The header is a bit different from the one described in the design doc:
The encryption algorithm and key length was changed to be stored in
1 byte instead of 2 for easier handling and the magic string is
"kuduenc" instead of "kuduen".
This patch also introduces a new flag which is hidden for now:
--encryption_key_length. This can be set to any valid AES key length as
per its specification (128, 192, or 256 bits), as only AES encryption is
supported for now, and there are no plans to support anything else in
the foreseeable future.
As we add a 64-byte header to encrypted files, some changes had to be
made to code that handles files and relies on sizes and offsets,
including in tests.
I ran the full test suite manually with encryption enabled to make sure
turning on encryption doesn't break anything.
Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
---
M src/kudu/consensus/log-test.cc
M src/kudu/consensus/log_index.cc
M src/kudu/consensus/log_util.cc
M src/kudu/fs/dir_util.cc
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/log_block_manager-test-util.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_service-test.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/tablet_copy_source_session.cc
M src/kudu/util/env-test.cc
M src/kudu/util/env.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/env_util.cc
M src/kudu/util/env_util.h
M src/kudu/util/file_cache-stress-test.cc
M src/kudu/util/file_cache-test.cc
M src/kudu/util/file_cache.cc
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
25 files changed, 678 insertions(+), 242 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/18025/5
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 5
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [security] KUDU-3316 Add encrypted file keys
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/18025 )
Change subject: [security] KUDU-3316 Add encrypted file keys
......................................................................
Patch Set 8:
(15 comments)
> Patch Set 3:
>
> (7 comments)
>
> I took a quick look and since I see the size of the encryption is still floating around, may be it makes sense to re-evaluate the approach I already pointed at https://gerrit.cloudera.org/#/c/18025/3/src/kudu/util/env.h@194 ?
>
> What do you think?
What do you mean the by the size floating around and how would your suggestion address that?
http://gerrit.cloudera.org:8080/#/c/18025/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18025/4//COMMIT_MSG@29
PS4, Line 29: 64-
> Maybe I'm missing it -- in env_posix I only see the headers using 40: 7 for
Yea we right-pad it here: https://gerrit.cloudera.org/c/18025/4/src/kudu/util/env_posix.cc#800
Seems I forgot to add this to the EncryptionHeader comment though.
http://gerrit.cloudera.org:8080/#/c/18025/8//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18025/8//COMMIT_MSG@36
PS8, Line 36: manually with encryption enabled
> Probably, the encryption stuff is not one of those things which might be af
I don't think it would be necessary, but I guess i wouldn't hurt. Is there a way to pass env vars to dist-test?
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/consensus/log_index.cc
File src/kudu/consensus/log_index.cc:
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/consensus/log_index.cc@124
PS8, Line 124: Read(file_->GetEncryptionHeaderSize()
> For the RWFile::Read() and RWFile::Write() below: does it make sense to hid
Answered this on https://gerrit.cloudera.org/c/18025/8/src/kudu/util/file_cache-test.cc#107
http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/fs/log_block_manager.cc@749
PS4, Line 749:
> Whitespace after initializer lists aren't treated the same with regards to
Done
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/tools/tool_action_pbc.cc
File src/kudu/tools/tool_action_pbc.cc:
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/tools/tool_action_pbc.cc@107
PS8, Line 107: if (!env->IsEncryptionEnabled()) {
: return false;
: }
> This seems like it doesn't do the right thing -- just because 'env' isn't e
This is only the pbc tool though, this code is never part of any servers. If the file was written by an encrypted server, we won't be able to decrypt it with encryption disabled anyway, so this check should still be okay. This condition is there basically to avoid reading the file twice if we don't have to.
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/env.h
File src/kudu/util/env.h:
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/env.h@378
PS8, Line 378: const size_t
> What's the idea behind add 'const' to the size_t output type which is retur
Done
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/env.h@398
PS8, Line 398: const
> Drop const?
Done
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/env.h@698
PS8, Line 698: IsEncrypted()
> Is it expected to have a non-const implementation of this method in any of
Done
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/env_posix.cc@491
PS8, Line 491: //memcpy(ciphertext[i].mutable_data(), cleartext[i].data(), cleartext[i].size());
> nit: remove?
Done
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/env_posix.cc@777
PS8, Line 777: // As the keys are encrypted in ECB mode which requires padding, we need
: // 32 bytes to encrypt and write a 192-bit key.
> nit: I'm pretty sure this comment is explaining why we are using 32 _instea
Sorry, I thought the 192-bit key part makes it clear enough.
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/env_posix.cc@800
PS8, Line 800: 56
> nit: define this as a constexpr constant with a kVariableName.
Done
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/env_posix.cc@827
PS8, Line 827: *eh)
> nit: seems kind of confusing to be passing this as an argument, giving we'r
Yea, I agree this is weird, do you have a suggestion for a cleaner approach?
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/env_posix.cc@833
PS8, Line 833: case 0:
> nit: consider assigning values to these enums and using those instead? May
Done
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/env_util.cc
File src/kudu/util/env_util.cc:
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/env_util.cc@250
PS8, Line 250: opts.is_sensitive = false;
> This looks a bit strange: WritableFileOptions are unconditionally overwritt
When we copy a file, it doesn't make sense to decrypt and re-encrypt it with a different key. Treating the file as insensitive allows us to copy encrypted files as-is.
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/file_cache-test.cc
File src/kudu/util/file_cache-test.cc:
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/file_cache-test.cc@103
PS8, Line 103: unique_ptr<RWFile> f;
: RWFileOptions opts;
: opts.is_sensitive = true;
: RETURN_NOT_OK(env_->NewRWFile(opts, name, &f));
: RETURN_NOT_OK(f->Write(f->GetEncryptionHeaderSize(), data));
> Instead of passing around the information on the enc header size and offset
That's effectively what the original approach was (hide the header size everywhere), but that leads to a lot of different confusion. As Andrew pointed out, it's hard to draw the line where we use physical vs logical size: https://gerrit.cloudera.org/c/18025/3//COMMIT_MSG#35
It also required a ton of hacks. We also rely on specific offsets a lot, we don't just start writing to files at 0, but we sometimes need to read the footer of a file, so we need to be able to calculate the offset where the footer begins based on the file size. LBM relies on FS blocks for hole punching, so all blocks in a container file need to be aligned. Hiding the file size would mess with these alignments.
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 8
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Wed, 12 Jan 2022 17:33:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] [security] KUDU-3316 Add encrypted file keys
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18025 )
Change subject: [security] KUDU-3316 Add encrypted file keys
......................................................................
Patch Set 8:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/18025/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18025/4//COMMIT_MSG@29
PS4, Line 29: 64-
> I actually ended up using 64 bytes after all. It's not needed for alignment
Maybe I'm missing it -- in env_posix I only see the headers using 40: 7 for magic, 1 for the algorithm, and 32 for the file key. Is there another 24 used elsewhere?
http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/fs/log_block_manager.cc@749
PS4, Line 749:
> what's wrong with the spacing here?
Whitespace after initializer lists aren't treated the same with regards to subsequent lines: https://google.github.io/styleguide/cppguide.html#Constructor_Initializer_Lists
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/tools/tool_action_pbc.cc
File src/kudu/tools/tool_action_pbc.cc:
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/tools/tool_action_pbc.cc@107
PS8, Line 107: if (!env->IsEncryptionEnabled()) {
: return false;
: }
This seems like it doesn't do the right thing -- just because 'env' isn't encrypted doesn't mean the file was written on an unencrypted env, right? Would it make sense to set FLAGS_encrypt_data_at_rest=true here in a flag saver and then check if we can open the file?
I suppose it only matters in cases where the env with which we read is different from the env in which we write. But that's possible (though problematic) e.g. in the case a user incorrectly starts without encryption args, when the data is already encrypted, and vice versa.
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/env_posix.cc@491
PS8, Line 491: //memcpy(ciphertext[i].mutable_data(), cleartext[i].data(), cleartext[i].size());
nit: remove?
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/env_posix.cc@777
PS8, Line 777: // As the keys are encrypted in ECB mode which requires padding, we need
: // 32 bytes to encrypt and write a 192-bit key.
nit: I'm pretty sure this comment is explaining why we are using 32 _instead of_ 24, as the key size below is specified. Can you explicitly call that out? It's not obvious without reading the ReadEncryptionHeader code
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/env_posix.cc@800
PS8, Line 800: 56
nit: define this as a constexpr constant with a kVariableName.
Also why 56? Mind adding a comment?
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/env_posix.cc@827
PS8, Line 827: *eh)
nit: seems kind of confusing to be passing this as an argument, giving we're not decrypting anything in this readv call.
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/env_posix.cc@833
PS8, Line 833: case 0:
nit: consider assigning values to these enums and using those instead? May be less error prone.
i.e.
enum class EncryptionAlgorithm {
AES128CTR = 0,
...
};
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 8
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Wed, 12 Jan 2022 06:30:23 +0000
Gerrit-HasComments: Yes
[kudu-CR] [security] KUDU-3316 Add encrypted file keys
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18025 )
Change subject: [security] KUDU-3316 Add encrypted file keys
......................................................................
Patch Set 8:
(1 comment)
> (15 comments)
>
> > Patch Set 3:
> >
> > (7 comments)
> >
> > I took a quick look and since I see the size of the encryption is
> still floating around, may be it makes sense to re-evaluate the
> approach I already pointed at https://gerrit.cloudera.org/#/c/18025/3/src/kudu/util/env.h@194
> ?
> >
> > What do you think?
>
> What do you mean the by the size floating around and how would your
> suggestion address that?
There are more details at https://gerrit.cloudera.org/#/c/18025/8/src/kudu/util/file_cache-test.cc@107
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/consensus/log_index.cc
File src/kudu/consensus/log_index.cc:
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/consensus/log_index.cc@124
PS8, Line 124: Read(file_->GetEncryptionHeaderSize()
> Answered this on https://gerrit.cloudera.org/c/18025/8/src/kudu/util/file_c
I don't think I got a proper answer there. You mentioned that you started with that approach, but after a while for the reason you didn't not exactly specify you decided to switch to passing around the offset and the header size all over at the higher levels. It would be great if you provide the exact reason.
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 8
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Wed, 12 Jan 2022 17:56:54 +0000
Gerrit-HasComments: Yes
[kudu-CR] [security] KUDU-3316 Add encrypted file keys
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18025 )
Change subject: [security] KUDU-3316 Add encrypted file keys
......................................................................
Patch Set 8:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/consensus/log_index.cc
File src/kudu/consensus/log_index.cc:
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/consensus/log_index.cc@124
PS8, Line 124: Read(file_->GetEncryptionHeaderSize()
> I don't think I got a proper answer there. You mentioned that you started
OK, it seems at https://gerrit.cloudera.org/c/18025/3/COMMIT_MSG#35 there is a discussion on how to draw a line where to use logical vs physical offsets.
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/env_util.cc
File src/kudu/util/env_util.cc:
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/env_util.cc@250
PS8, Line 250: opts.is_sensitive = false;
> When we copy a file, it doesn't make sense to decrypt and re-encrypt it wit
Cool! Could you please add a comment with explaining that right in this code? It could help with better understanding the code.
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/file_cache-test.cc
File src/kudu/util/file_cache-test.cc:
http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/file_cache-test.cc@103
PS8, Line 103: unique_ptr<RWFile> f;
: RWFileOptions opts;
: opts.is_sensitive = true;
: RETURN_NOT_OK(env_->NewRWFile(opts, name, &f));
: RETURN_NOT_OK(f->Write(f->GetEncryptionHeaderSize(), data));
> That's effectively what the original approach was (hide the header size eve
Thank you for the explanation. I'm not sure I saw that in one of patch sets published for this review item. Did I miss it somehow?
I didn't see the PS for the approach you referred, but I guess that the ton of hack were mostly due to not very good abstraction or a bug somewhere. So my ask is to try re-evaluate your original approach, if possible. If you are sure it's not going to work that way, I guess I'm OK with the current approach of passing the offsets in many places at the higher level.
--
To view, visit http://gerrit.cloudera.org:8080/18025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd
Gerrit-Change-Number: 18025
Gerrit-PatchSet: 8
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Wed, 12 Jan 2022 18:23:47 +0000
Gerrit-HasComments: Yes