Add a codec AES_128_GCM_SIV for encrypting columns on disk #19896

depressed-pho · 2021-02-01T03:39:47Z

While this is implemented as a compression codec, it does not actually compress data. It instead encrypts data on disk. The key is obtained by executing a user-specified command at the server startup, or if it's not specified the codec refuses to process any data.

Changelog category (leave one):

New Feature

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Added a compression codec AES_128_GCM_SIV which encrypts columns instead of compressing them.

Detailed description / Documentation draft:

Included in the commit.

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

alex-zaitsev · 2021-02-01T07:57:58Z

@depressed-pho , this is nice, but can we have it more generic? I.e. instead of AES codec define it as Encrypted with parameters. That will allow to make it more generic, as current 'encrypt' function: https://clickhouse.tech/docs/en/sql-reference/functions/encryption-functions/

For now it will only support AES_128_GCM_SIV, but we can extend later to other options.

depressed-pho · 2021-02-01T08:07:35Z

Like Codec(Default, Encrypted('AES-128-GCM-SIV'))? Okay, I will do it.

alexey-milovidov · 2021-02-01T08:21:23Z

It should be conditionally compiled under #if USE_SSL, see examples in code.

Enmk · 2021-02-01T08:24:36Z

@depressed-pho thanks, a very nice feature! I noticed that there are no tests, could you please add some? To simplify this, you may want to reuse the unit-test suite from other codes, please see CodecTestCompatibility for example.

alexey-milovidov · 2021-02-01T08:53:15Z

Unit tests are optional but functional tests are mandatory.

depressed-pho · 2021-02-01T09:12:06Z

But I'm not sure how to write a functional test for this. Would it be enough to just add something like <encryption><key_command>echo "Some static key in base64"</key_command></encryption> to tests/server-test.xml and a stateless SQL test using the codec?

alexey-milovidov · 2021-02-01T09:17:24Z

@depressed-pho Yes, you can add a new config with a script for static key, see tests/config/README.md

vzakaznikov · 2021-02-01T14:37:02Z

If we make it more generic, it would nice for the encryption parameters to the codec to match our current encrypt function (parameter names, order and values).

depressed-pho · 2021-02-02T02:51:16Z

Updated my patch. It's now named Encrypted(cipher) where cipher has to be a literal 'AES-128-GCM-SIV' for now. The code is guarded by #if USE_SSL, and has a stateless SQL test.

depressed-pho · 2021-02-02T08:21:16Z

Amended it again to fix a build failure occuring when !USE_SSL.

depressed-pho · 2021-02-04T04:45:21Z

Fixed !USE_SSL again.

depressed-pho · 2021-02-04T06:05:46Z

Marked the test as being skipped in fasttest, where SSL is disabled.

depressed-pho · 2021-02-04T08:11:59Z

Fixed several build errors found by the build check.

depressed-pho · 2021-02-05T01:27:28Z

And now only the Yandex-exclusive checks are failing. I can't do anything about that.

depressed-pho · 2021-02-18T01:44:33Z

Rebased and resolved a conflict.

nikitamikhaylov · 2021-02-25T09:58:14Z

@depressed-pho Please, run a script generate-ya-make from utils folder to solve an issue with Yandex synchronization check. Then commit add all ya.make files to this PR.

depressed-pho · 2021-02-26T02:20:04Z

@depressed-pho Please, run a script generate-ya-make from utils folder to solve an issue with Yandex synchronization check. Then commit add all ya.make files to this PR.

I did, but the script updated no ya.make files except for the one already included in this commit. I rebased the commit anyway.

While this is implemented as a compression codec, it does not actually compress data. It instead encrypts data on disk. The key is obtained by executing a user-specified command at the server startup, or if it's not specified the codec refuses to process any data. For now the only supported cipher is 'AES-128-GCM-SIV'.

alexey-milovidov · 2021-05-19T00:09:02Z

(deleted) This was wrong.

alexey-milovidov · 2021-07-27T05:08:40Z

This codec is actually mostly safe to use but some considerations exist:

the information that is revealed is when some blocks of data are the same: it makes it unsafe if ClickHouse is used for multiple tenants and some of them may have access to encrypted data on disk - then one tenant can find that other tenant have the same sequences of data;
also the size of compressed data is revealed if it is applied after compression - it makes bruteforce attacks slightly easier.

I think we can merge it and highlight these considerations in docs.

PS. I did not know that AES-GCM-SIV is a well known technique, initially I thought it is some dirty hack :)

alexey-milovidov · 2021-07-27T05:14:04Z

@alex-zaitsev

For now it will only support AES_128_GCM_SIV, but we can extend later to other options.

For other modes of operation it will be "instant death" if we will not generate and store random IVs. But if we do it, replicas will argue about mismatched checksum of "compressed" data and download data parts from each other excessively.

So, I think we should not allow more modes of operation here.

alexey-milovidov · 2021-07-30T09:21:00Z

docs/en/operations/server-configuration-parameters/settings.md

+For Linux with systemd:
+
+```xml
+<encryption>


This configuration can be confused with the configuration of entrypted disks.

Yes, probably a longer path in xml would be better, for example

<codecs> <aes_128_gcm_siv> <key_hex>...</key_hex> </aes_128_gcm_siv> </codecs>

alexey-milovidov · 2021-07-30T09:22:30Z

programs/server/Server.cpp

+
+    // turbob64 doesn't like whitespace characters in input. Strip
+    // them before decoding.
+    std::erase_if(b64_key, [](char c)


It has an inconsistency: in this place, the encryption key is in base64 while for encrypted disks it is in hex or binary.

And I think, it's better to read it in hex.

alexey-milovidov · 2021-07-30T09:24:46Z

programs/server/Server.cpp

+{
+#if USE_BASE64 && USE_SSL && USE_INTERNAL_SSL_LIBRARY
+
+    auto process = ShellCommand::execute(key_command);


Another inconsistency - here the key is loaded by a command placed in config,
while for encrypted disks we don't allow to put a script in config file, we expect the key can be imported from env variable with from_env=... attribute in config.

alexey-milovidov · 2021-07-30T09:25:46Z

src/Compression/CompressionCodecEncrypted.cpp

+    {
+        // Fixed nonce. Yes this is unrecommended, but we have to live
+        // with it.
+        std::string_view nonce("\0\0\0\0\0\0\0\0\0\0\0\0", 12);


It is actually ok for AES_GCM_SIV?

alexey-milovidov · 2021-07-30T09:29:36Z

Need help from @vitlibar for further polishing.

* master: (482 commits) ru version colon en version colon Attempt to fix flaky 00705_drop_create_merge_tree Help with ClickHouse#26424 Adjust 00537_quarters to be timezone independent Add a codec AES_128_GCM_SIV for encrypting columns on disk (ClickHouse#19896) Update BaseDaemon.cpp Update PULL_REQUEST_TEMPLATE.md edits after review small fix in links Make the test parallelizable ru version edit ru version en version Fix 01600_quota_by_forwarded_ip typo Remove trailing whitespaces from docs Remove trailing whitespaces from docs fix system.zookeeper_log initialization Move formatBlock to its own file ... # Conflicts: # programs/server/Server.cpp

vitlibar · 2021-08-05T09:07:00Z

My thoughts about that:

About syntax. I believe Codec(AES-128-GCM-SIV) would be better than Codec(Encrypted('AES-128-GCM-SIV')) because the first one is shorter and more consistent with other codecs. I mean for ordinary compression codec we don't write Codec(Compression('ZSTD')), do we? And I think if we later decide to support other encryption algorithm, for example AES-128-GCM with storing random nonce in output stream, we will just create a new codec.
I suppose we should support multiple keys, because sometimes we may wish to switch to a new key but still allow using previous keys for decryption. Of course to support multiple keys we should add a key id into an output stream so the decryptor later will know which key to use in case if in the future the current key is changed.
As already said, the representation of keys in the config file looks confusing because we also have encrypted disks.
I suggest to set keys in the config file like this:

<encryption_codecs>
    <!-- just key in hex, not some command -->
    <!-- the code should use directly this key and throw an exception if its length is not 16 bytes -->
    <key_hex>...</key_hex>
</encryption_codecs>

or, for multiple keys

<encryption_codecs>
        <!-- just key in hex, not some command -->
        <!-- the code should use this key and throw an exception if its length is not 16 bytes -->
        <key_hex id="0">...</key_hex>
        <key_hex id="1">...</key_hex>
        <current_key_id>1</current_key_id>
</encryption_codecs>

or

<encryption_codecs>
        <!-- just key in hex, not some command -->
        <!-- the code should use this key and throw an exception if its length is not 16 bytes -->
        <key_hex id="0">...</key_hex>
        <key_hex id="1">...</key_hex>
    <aes_128_gcm_siv>
        <current_key_id>1</current_key_id>
    </aes_128_gcm_siv>
</encryption_codecs>

Also we should change the documentation and say there that though encryption keys can be stored in the configuration file that is NOT recommended because it's not secure enough. It's better either to use some environmental variables (see "from_env" tag) or move the keys into a separate config file on a secure disk and put a symlink to that config file to config.d/ folder.
About implementation. It seems to be wrong. It says that it uses aes_128_gcm_siv but actually uses aes_128_gcm. Also I don't think we should use nonces containing only zeroes, a random hardcoded nonce would look better. With a corresponding comment about nonce misuse resistance. And if we require the keys in the config file to be 16 bytes long it seems to be unnecessary to call HKDF.

alexey-milovidov · 2021-08-05T10:39:13Z

I believe Codec(AES-128-GCM-SIV) would be better than Codec(Encrypted('AES-128-GCM-SIV'))

Yes, 100%. It was just some false advice...

alexey-milovidov · 2021-08-05T10:40:17Z

I suppose we should support multiple keys, because sometimes we may wish to switch to a new key but still allow using previous keys for decryption. Of course to support multiple keys we should add a key id into an output stream so the decryptor later will know which key to use in case if in the future the current key is changed.

+1, it will be consistent with full disk encryption that we have implemented.

alexey-milovidov · 2021-08-05T10:42:02Z

And if we require the keys in the config file to be 16 bytes long it seems to be unnecessary to call HKDF.

+1.

Using KDF will make it inconsistent with full disk encryption.
Otherwise we can add a separate alternative way to specify key, when KDF will be applied.

robot-clickhouse added doc-alert pr-feature Pull request with new product feature labels Feb 1, 2021

alexey-milovidov added the can be tested label Feb 1, 2021

depressed-pho force-pushed the encryption-codec branch from 178b908 to d7957ec Compare February 2, 2021 02:48

depressed-pho force-pushed the encryption-codec branch from d7957ec to 90dc94c Compare February 2, 2021 08:20

depressed-pho force-pushed the encryption-codec branch from 90dc94c to 0ce27f6 Compare February 4, 2021 04:44

depressed-pho force-pushed the encryption-codec branch from 0ce27f6 to 8959ad8 Compare February 4, 2021 06:05

depressed-pho force-pushed the encryption-codec branch from 8959ad8 to 88824b7 Compare February 4, 2021 08:11

depressed-pho force-pushed the encryption-codec branch from 88824b7 to a13e56c Compare February 18, 2021 01:43

depressed-pho force-pushed the encryption-codec branch from a13e56c to c8fedc1 Compare February 26, 2021 02:17

depressed-pho force-pushed the encryption-codec branch from c8fedc1 to 1b337bc Compare March 23, 2021 02:25

depressed-pho force-pushed the encryption-codec branch from 1b337bc to 089cdcb Compare March 31, 2021 06:22

depressed-pho mentioned this pull request Apr 1, 2021

Use a server-supplied key for functions encrypt()/decrypt() when their key argument is omitted #22423

Closed

Merge branch 'master' into encryption-codec

84e0a5a

FArthur-cmd self-assigned this Jul 28, 2021

FArthur-cmd added 2 commits July 29, 2021 12:41

Merge branch 'master' into encryption-codec

a1de2ad

Remove trailing whitespaces.

852ccc5

FArthur-cmd approved these changes Jul 29, 2021

View reviewed changes

FArthur-cmd merged commit 6425dd0 into ClickHouse:master Jul 30, 2021

alexey-milovidov reviewed Jul 30, 2021

View reviewed changes

FArthur-cmd mentioned this pull request Aug 13, 2021

Improve codec for encr 19896 #27476

Merged

8 tasks

alexey-milovidov mentioned this pull request Sep 13, 2021

Intern tasks 2020/2021 #15065

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a codec AES_128_GCM_SIV for encrypting columns on disk #19896

Add a codec AES_128_GCM_SIV for encrypting columns on disk #19896

depressed-pho commented Feb 1, 2021

alex-zaitsev commented Feb 1, 2021

depressed-pho commented Feb 1, 2021

alexey-milovidov commented Feb 1, 2021

Enmk commented Feb 1, 2021

alexey-milovidov commented Feb 1, 2021

depressed-pho commented Feb 1, 2021

alexey-milovidov commented Feb 1, 2021 •

edited

Loading

vzakaznikov commented Feb 1, 2021

depressed-pho commented Feb 2, 2021 •

edited

Loading

depressed-pho commented Feb 2, 2021

depressed-pho commented Feb 4, 2021

depressed-pho commented Feb 4, 2021

depressed-pho commented Feb 4, 2021

depressed-pho commented Feb 5, 2021

depressed-pho commented Feb 18, 2021

nikitamikhaylov commented Feb 25, 2021

depressed-pho commented Feb 26, 2021

alexey-milovidov commented May 19, 2021 •

edited

Loading

alexey-milovidov commented Jul 27, 2021 •

edited

Loading

alexey-milovidov commented Jul 27, 2021

alexey-milovidov Jul 30, 2021

vitlibar Jul 30, 2021 •

edited

Loading

alexey-milovidov Jul 30, 2021

alexey-milovidov Jul 30, 2021

alexey-milovidov Jul 30, 2021

alexey-milovidov Jul 30, 2021

alexey-milovidov commented Jul 30, 2021

vitlibar commented Aug 5, 2021 •

edited

Loading

alexey-milovidov commented Aug 5, 2021 •

edited

Loading

alexey-milovidov commented Aug 5, 2021

alexey-milovidov commented Aug 5, 2021

Add a codec AES_128_GCM_SIV for encrypting columns on disk #19896

Add a codec AES_128_GCM_SIV for encrypting columns on disk #19896

Conversation

depressed-pho commented Feb 1, 2021

alex-zaitsev commented Feb 1, 2021

depressed-pho commented Feb 1, 2021

alexey-milovidov commented Feb 1, 2021

Enmk commented Feb 1, 2021

alexey-milovidov commented Feb 1, 2021

depressed-pho commented Feb 1, 2021

alexey-milovidov commented Feb 1, 2021 • edited Loading

vzakaznikov commented Feb 1, 2021

depressed-pho commented Feb 2, 2021 • edited Loading

depressed-pho commented Feb 2, 2021

depressed-pho commented Feb 4, 2021

depressed-pho commented Feb 4, 2021

depressed-pho commented Feb 4, 2021

depressed-pho commented Feb 5, 2021

depressed-pho commented Feb 18, 2021

nikitamikhaylov commented Feb 25, 2021

depressed-pho commented Feb 26, 2021

alexey-milovidov commented May 19, 2021 • edited Loading

alexey-milovidov commented Jul 27, 2021 • edited Loading

alexey-milovidov commented Jul 27, 2021

alexey-milovidov Jul 30, 2021

Choose a reason for hiding this comment

vitlibar Jul 30, 2021 • edited Loading

Choose a reason for hiding this comment

alexey-milovidov Jul 30, 2021

Choose a reason for hiding this comment

alexey-milovidov Jul 30, 2021

Choose a reason for hiding this comment

alexey-milovidov Jul 30, 2021

Choose a reason for hiding this comment

alexey-milovidov Jul 30, 2021

Choose a reason for hiding this comment

alexey-milovidov commented Jul 30, 2021

vitlibar commented Aug 5, 2021 • edited Loading

alexey-milovidov commented Aug 5, 2021 • edited Loading

alexey-milovidov commented Aug 5, 2021

alexey-milovidov commented Aug 5, 2021

alexey-milovidov commented Feb 1, 2021 •

edited

Loading

depressed-pho commented Feb 2, 2021 •

edited

Loading

alexey-milovidov commented May 19, 2021 •

edited

Loading

alexey-milovidov commented Jul 27, 2021 •

edited

Loading

vitlibar Jul 30, 2021 •

edited

Loading

vitlibar commented Aug 5, 2021 •

edited

Loading

alexey-milovidov commented Aug 5, 2021 •

edited

Loading