feat: support ssl key-encrypt-salt rotation by juststillthinking · Pull Request #7925 · apache/apisix

juststillthinking · 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

-    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

+                    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

-    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

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


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

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

+
+        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

+    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

+                        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

+                                    maxLength = 16
+                                }
+                            }
+


Why add an extra blank line?

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

+                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

+    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

+            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

-                        }
+                        },
+                        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

+    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.

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



-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

juststillthinking 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

Comment thread apisix/ssl.lua

fix: resolve review comments

c096363

spacewander reviewed Sep 23, 2022

View reviewed changes

juststillthinking 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

juststillthinking requested review from soulbird, spacewander, tokers and tzssangglass and removed request for soulbird, spacewander 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



		local _aes_128_cbc_with_iv = false
		local _aes_128_cbc_with_iv_tbl = false

Uh oh!

Conversation

juststillthinking commented Sep 15, 2022

Description

Checklist

Uh oh!

membphis Sep 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

membphis Sep 16, 2022 •

edited

Loading