feat: support ssl key-encrypt-salt rotation #7925

monkeyDluffy6017 · 2022-09-15T08:15:23Z

Description

Currently, the key_encryption_salt cannot be changed once users use it to encrypt the private key, otherwise, Apache APISIX cannot decrypt the private key correctly. This may become a pain point when the user leaks the salt.
As a user, I can configure multiple key_encryption_salt for Apache APISIX, and Apache APISIX will use them to decrypt private keys in turn

Checklist

I have explained the need for this PR and the problem it solves
I have explained the changes or the new features added to this PR
I have added tests corresponding to this change
I have updated the documentation to reflect this change
I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

membphis · 2022-09-16T06:05:42Z

conf/config-default.yaml

-    key_encrypt_salt: edd1c9f0985e76a2      #  If not set, will save origin ssl key into etcd.
-                                            #  If set this, must be a string of length 16. And it will encrypt ssl key with AES-128-CBC
-                                            #  !!! So do not change it after saving your ssl, it can't decrypt the ssl keys have be saved if you change !!
+    key_encrypt_salt:             #  If not set, will save origin ssl key into etcd.


Is this a breaking change?

it's backward compatible, we could use 2 ways at the same time

I have test case about this case

spacewander · 2022-09-16T06:52:07Z

apisix/ssl.lua

+                    local aes_with_iv = assert(aes:new(iv, nil, aes.cipher(128, "cbc"), {iv = iv}))
+                    core.table.insert(_aes_128_cbc_with_iv_tbl, aes_with_iv)
+                else
+                    core.log.error("the key_encrypt_salt does not meet the "


We can use , instead of .. as separator

spacewander · 2022-09-16T06:52:48Z

apisix/ssl.lua

-    if not decrypted then
-        core.log.error("decrypt ssl key failed. key[", key, "] ")
+    local aes_128_cbc_with_iv_tbl = get_aes_128_cbc_with_iv()
+    if core.table.isempty(aes_128_cbc_with_iv_tbl) then


Use # would be better as the table must be array

spacewander · 2022-09-16T06:54:06Z

t/admin/ssl4.t

+        ngx.say(body)
+    }
+}
+--- request


Let's move the common part to the top like other test files

spacewander · 2022-09-16T06:58:28Z

apisix/ssl.lua

+
+        if type_ivs == "table" then
+            for index, iv in ipairs(ivs) do
+                if type(iv) == "string" and #iv == 16 then


Better to check this via schema in https://github.com/apache/apisix/blob/master/apisix/cli/schema.lua

tokers · 2022-09-16T09:14:48Z

conf/config-default.yaml

+    key_encrypt_salt:             #  If not set, will save origin ssl key into etcd.
+      - edd1c9f0985e76a2          #  If set this, must be a string of length 16. And it will encrypt ssl key with AES-128-CBC
+                                  #  !!! So do not change it after saving your ssl, it can't decrypt the ssl keys have be saved if you change !!
+                                  #  Only use the first key to encrypt, and decrypt in the order of the array.


The comment is not accurate to describe the field, since the comment says it "must be a string of length 16".

The comment is still wrong.

The key_encrypt_salt should either be a string whose size is 16 or an array whose elements are string, and the size is also 16.

Should we only recommend using array？

I'm updating this to a hexadecimal string of length 16. It's technically a 8 byte value in hex representation. Size isn't accurate.

spacewander · 2022-09-18T12:12:46Z

apisix/cli/schema.lua

+                        key_encrypt_salt = {
+                            anyOf = {
+                                {
+                                type = "array",


Let's add length check for the array

minItems = 1? why need this check?

spacewander · 2022-09-18T12:14:20Z

apisix/cli/schema.lua

+                                    maxLength = 16
+                                }
+                            }
+


Why add an extra blank line?

spacewander · 2022-09-18T12:15:32Z

apisix/ssl.lua

+                local aes_with_iv = assert(aes:new(iv, nil, aes.cipher(128, "cbc"), {iv = iv}))
+                core.table.insert(_aes_128_cbc_with_iv_tbl, aes_with_iv)
+            end
+        elseif type_ivs == "string" and #ivs == 16 then


We can remove the #ivs == 16 check as we already checked it in the schema.

soulbird · 2022-09-19T01:33:39Z

t/admin/ssl4.t

+    node_listen: 1984
+    ssl:
+        key_encrypt_salt: "edd1c9f0985e76a1"
+--- response_body eval


The judgment of the response body does not seem necessary.

Just a double check, i prefer to leave it there

soulbird · 2022-09-19T01:34:23Z

t/admin/ssl4.t

+            while true do
+                local line, err = sock:receive()
+                if not line then
+                    -- ngx.say("failed to receive response status line: ", err)


Please remove redundant comments.

tokers · 2022-09-19T01:40:27Z

apisix/cli/schema.lua

-                        }
+                        },
+                        key_encrypt_salt = {
+                            anyOf = {


I know anyOf can satisfy the schema check but it should be oneOf in this scene?

But this is the conventional way in APISIX, it's used everywhere, so i think i should follow this.

tokers · 2022-09-19T01:46:57Z

conf/config-default.yaml

+    key_encrypt_salt:             #  If not set, will save origin ssl key into etcd.
+      - edd1c9f0985e76a2          #  If set this, must be a string of length 16. And it will encrypt ssl key with AES-128-CBC
+                                  #  !!! So do not change it after saving your ssl, it can't decrypt the ssl keys have be saved if you change !!
+                                  #  Only use the first key to encrypt, and decrypt in the order of the array.


The comment is still wrong.

The key_encrypt_salt should either be a string whose size is 16 or an array whose elements are string, and the size is also 16.

apisix/ssl.lua

spacewander · 2022-09-23T01:35:30Z

apisix/ssl.lua

@@ -55,23 +56,32 @@ function _M.server_name()
 end


-local _aes_128_cbc_with_iv = false
+local _aes_128_cbc_with_iv_tbl = false


Better to avoid using boolean value as the default value of a table?

…nkeyDluffy6017/apisix into support-ssl-key-encrypt-salt-rotation

monkeyDluffy6017 added 5 commits September 15, 2022 16:11

feat: support ssl key-encrypt-salt rotation

0103018

test: add test cases

2ba4f77

test: add test cases

e6a8c57

Merge branch 'master' into support-ssl-key-encrypt-salt-rotation

d7632ee

test: fix failed test cases

02f838e

membphis reviewed Sep 16, 2022

View reviewed changes

spacewander reviewed Sep 16, 2022

View reviewed changes

tokers reviewed Sep 16, 2022

View reviewed changes

feat: resolve review comments

b3d6e38

spacewander reviewed Sep 18, 2022

View reviewed changes

fix: resolve review comments

8e31c0a

soulbird reviewed Sep 19, 2022

View reviewed changes

tokers reviewed Sep 19, 2022

View reviewed changes

tzssangglass reviewed Sep 19, 2022

View reviewed changes

apisix/ssl.lua Show resolved Hide resolved

fix: resolve review comments

c096363

spacewander reviewed Sep 23, 2022

View reviewed changes

monkeyDluffy6017 added 4 commits September 23, 2022 10:05

fix: fix review comments

501f475

Merge branch 'master' into support-ssl-key-encrypt-salt-rotation

457bddc

Merge branch 'master' into support-ssl-key-encrypt-salt-rotation

4cc8097

Merge branch 'support-ssl-key-encrypt-salt-rotation' of github.com:mo…

1842e6d

…nkeyDluffy6017/apisix into support-ssl-key-encrypt-salt-rotation

monkeyDluffy6017 requested review from spacewander, soulbird, tokers and tzssangglass and removed request for spacewander, soulbird and tokers September 23, 2022 02:16

spacewander approved these changes Sep 23, 2022

View reviewed changes

tzssangglass approved these changes Sep 23, 2022

View reviewed changes

spacewander merged commit 20efe29 into apache:master Sep 23, 2022

Liu-Junlin pushed a commit to Liu-Junlin/apisix that referenced this pull request Nov 4, 2022

feat: support ssl key-encrypt-salt rotation (apache#7925)

d081606

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support ssl key-encrypt-salt rotation #7925

feat: support ssl key-encrypt-salt rotation #7925

monkeyDluffy6017 commented Sep 15, 2022

membphis Sep 16, 2022 •

edited

Loading

monkeyDluffy6017 Sep 16, 2022

monkeyDluffy6017 Sep 16, 2022

spacewander Sep 16, 2022

monkeyDluffy6017 Sep 16, 2022

spacewander Sep 16, 2022

monkeyDluffy6017 Sep 16, 2022

spacewander Sep 16, 2022

monkeyDluffy6017 Sep 16, 2022

spacewander Sep 16, 2022

tokers Sep 16, 2022

monkeyDluffy6017 Sep 16, 2022

tokers Sep 16, 2022

monkeyDluffy6017 Sep 16, 2022

tokers Sep 19, 2022

monkeyDluffy6017 Sep 19, 2022

kayx23 Jun 9, 2023

spacewander Sep 18, 2022

monkeyDluffy6017 Sep 18, 2022

spacewander Sep 18, 2022

monkeyDluffy6017 Sep 18, 2022

spacewander Sep 18, 2022

monkeyDluffy6017 Sep 18, 2022

soulbird Sep 19, 2022

monkeyDluffy6017 Sep 19, 2022

soulbird Sep 19, 2022

monkeyDluffy6017 Sep 19, 2022

tokers Sep 19, 2022

monkeyDluffy6017 Sep 19, 2022

tokers Sep 19, 2022

spacewander Sep 23, 2022

monkeyDluffy6017 Sep 23, 2022

feat: support ssl key-encrypt-salt rotation #7925

feat: support ssl key-encrypt-salt rotation #7925

Conversation

monkeyDluffy6017 commented Sep 15, 2022

Description

Checklist

membphis Sep 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

membphis Sep 16, 2022 •

edited

Loading