You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2022/09/15 08:15:25 UTC
[GitHub] [apisix] monkeyDluffy6017 opened a new pull request, #7925: feat: support ssl key-encrypt-salt rotation
monkeyDluffy6017 opened a new pull request, #7925:
URL: https://github.com/apache/apisix/pull/7925
### 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
- [x] I have explained the need for this PR and the problem it solves
- [x] I have explained the changes or the new features added to this PR
- [x] I have added tests corresponding to this change
- [ ] I have updated the documentation to reflect this change
- [x] I have verified that this change is backward compatible (If not, please discuss on the [APISIX mailing list](https://github.com/apache/apisix/tree/master#community) first)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] soulbird commented on a diff in pull request #7925: feat: support ssl key-encrypt-salt rotation
Posted by GitBox <gi...@apache.org>.
soulbird commented on code in PR #7925:
URL: https://github.com/apache/apisix/pull/7925#discussion_r973813897
##########
t/admin/ssl4.t:
##########
@@ -0,0 +1,358 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
+
+log_level('debug');
+no_root_location();
+
+add_block_preprocessor( sub{
+ my ($block) = @_;
+
+ if (!$block->request) {
+ $block->set_value("request", "GET /t");
+ }
+
+ if ((!defined $block->error_log) && (!defined $block->no_error_log)) {
+ $block->set_value("no_error_log", "[error]");
+ }
+
+ my $TEST_NGINX_HTML_DIR ||= html_dir();
+
+ my $config = <<_EOC_;
+listen unix:$TEST_NGINX_HTML_DIR/nginx.sock ssl;
+
+location /t {
+ content_by_lua_block {
+ -- etcd sync
+ ngx.sleep(0.2)
+
+ do
+ local sock = ngx.socket.tcp()
+
+ sock:settimeout(2000)
+
+ local ok, err = sock:connect("unix:$TEST_NGINX_HTML_DIR/nginx.sock")
+ if not ok then
+ ngx.say("failed to connect: ", err)
+ return
+ end
+
+ ngx.say("connected: ", ok)
+
+ local sess, err = sock:sslhandshake(nil, "www.test.com", true)
+ if not sess then
+ ngx.say("failed to do SSL handshake: ", err)
+ return
+ end
+
+ ngx.say("ssl handshake: ", sess ~= nil)
+
+ local req = "GET /hello HTTP/1.0\\r\\nHost: www.test.com\\r\\nConnection: close\\r\\n\\r\\n"
+ local bytes, err = sock:send(req)
+ if not bytes then
+ ngx.say("failed to send http request: ", err)
+ return
+ end
+
+ ngx.say("sent http request: ", bytes, " bytes.")
+
+ while true do
+ local line, err = sock:receive()
+ if not line then
+ -- ngx.say("failed to receive response status line: ", err)
+ break
+ end
+
+ ngx.say("received: ", line)
+ end
+
+ local ok, err = sock:close()
+ ngx.say("close: ", ok, " ", err)
+ end -- do
+ -- collectgarbage()
+ }
+}
+_EOC_
+
+ if (!$block->config) {
+ $block->set_value("config", $config)
+ }
+}
+
+);
+
+
+run_tests;
+
+__DATA__
+
+=== TEST 1: set ssl(sni: www.test.com), encrypt with the first key_encrypt_salt
+--- yaml_config
+apisix:
+ node_listen: 1984
+ ssl:
+ key_encrypt_salt:
+ - edd1c9f0985e76a1
+ - edd1c9f0985e76a2
+--- config
+location /t {
+ content_by_lua_block {
+ local core = require("apisix.core")
+ local t = require("lib.test_admin")
+
+ local ssl_cert = t.read_file("t/certs/apisix.crt")
+ local ssl_key = t.read_file("t/certs/apisix.key")
+ local data = {cert = ssl_cert, key = ssl_key, sni = "www.test.com"}
+
+ local code, body = t.test('/apisix/admin/ssls/1',
+ ngx.HTTP_PUT,
+ core.json.encode(data),
+ [[{
+ "value": {
+ "sni": "www.test.com"
+ },
+ "key": "/apisix/ssls/1"
+ }]]
+ )
+
+ ngx.status = code
+ ngx.say(body)
+ }
+}
+--- response_body
+passed
+
+
+
+=== TEST 2: set route(id: 1)
+--- yaml_config
+apisix:
+ node_listen: 1984
+ ssl:
+ key_encrypt_salt: "edd1c9f0985e76a1"
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 3: client request with the old style key_encrypt_salt
+--- yaml_config
+apisix:
+ node_listen: 1984
+ ssl:
+ key_encrypt_salt: "edd1c9f0985e76a1"
+--- response_body eval
Review Comment:
The judgment of the response body does not seem necessary.
##########
t/admin/ssl4.t:
##########
@@ -0,0 +1,358 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
+
+log_level('debug');
+no_root_location();
+
+add_block_preprocessor( sub{
+ my ($block) = @_;
+
+ if (!$block->request) {
+ $block->set_value("request", "GET /t");
+ }
+
+ if ((!defined $block->error_log) && (!defined $block->no_error_log)) {
+ $block->set_value("no_error_log", "[error]");
+ }
+
+ my $TEST_NGINX_HTML_DIR ||= html_dir();
+
+ my $config = <<_EOC_;
+listen unix:$TEST_NGINX_HTML_DIR/nginx.sock ssl;
+
+location /t {
+ content_by_lua_block {
+ -- etcd sync
+ ngx.sleep(0.2)
+
+ do
+ local sock = ngx.socket.tcp()
+
+ sock:settimeout(2000)
+
+ local ok, err = sock:connect("unix:$TEST_NGINX_HTML_DIR/nginx.sock")
+ if not ok then
+ ngx.say("failed to connect: ", err)
+ return
+ end
+
+ ngx.say("connected: ", ok)
+
+ local sess, err = sock:sslhandshake(nil, "www.test.com", true)
+ if not sess then
+ ngx.say("failed to do SSL handshake: ", err)
+ return
+ end
+
+ ngx.say("ssl handshake: ", sess ~= nil)
+
+ local req = "GET /hello HTTP/1.0\\r\\nHost: www.test.com\\r\\nConnection: close\\r\\n\\r\\n"
+ local bytes, err = sock:send(req)
+ if not bytes then
+ ngx.say("failed to send http request: ", err)
+ return
+ end
+
+ ngx.say("sent http request: ", bytes, " bytes.")
+
+ while true do
+ local line, err = sock:receive()
+ if not line then
+ -- ngx.say("failed to receive response status line: ", err)
Review Comment:
Please remove redundant comments.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #7925: feat: support ssl key-encrypt-salt rotation
Posted by GitBox <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #7925:
URL: https://github.com/apache/apisix/pull/7925#discussion_r972858802
##########
apisix/ssl.lua:
##########
@@ -86,32 +101,32 @@ function _M.aes_encrypt_pkey(origin)
end
-local function decrypt_priv_pkey(iv, key)
- local decoded_key = ngx_decode_base64(key)
- if not decoded_key then
- core.log.error("base64 decode ssl key failed. key[", key, "] ")
- return nil
+local function aes_decrypt_pkey(origin)
+ if core.string.has_prefix(origin, "---") then
+ return origin
end
- local decrypted = iv:decrypt(decoded_key)
- 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
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #7925: feat: support ssl key-encrypt-salt rotation
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7925:
URL: https://github.com/apache/apisix/pull/7925#discussion_r973711612
##########
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
local function get_aes_128_cbc_with_iv()
- if _aes_128_cbc_with_iv == false then
+ if _aes_128_cbc_with_iv_tbl == false then
+ _aes_128_cbc_with_iv_tbl = core.table.new(2, 0)
local local_conf = core.config.local_conf()
- local iv = core.table.try_read_attr(local_conf, "apisix", "ssl", "key_encrypt_salt")
- if type(iv) =="string" and #iv == 16 then
- _aes_128_cbc_with_iv = assert(aes:new(iv, nil, aes.cipher(128, "cbc"), {iv = iv}))
- else
- _aes_128_cbc_with_iv = nil
+ local ivs = core.table.try_read_attr(local_conf, "apisix", "ssl", "key_encrypt_salt")
+ local type_ivs = type(ivs)
+
+ if type_ivs == "table" then
+ for _, iv in ipairs(ivs) do
+ 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
Review Comment:
We can remove the `#ivs == 16` check as we already checked it in the schema.
##########
apisix/cli/schema.lua:
##########
@@ -222,7 +222,25 @@ local config_schema = {
}
}
}
- }
+ },
+ key_encrypt_salt = {
+ anyOf = {
+ {
+ type = "array",
+ items = {
+ type = "string",
+ minLength = 16,
+ maxLength = 16
+ }
+ },
+ {
+ type = "string",
+ minLength = 16,
+ maxLength = 16
+ }
+ }
+
Review Comment:
Why add an extra blank line?
##########
apisix/cli/schema.lua:
##########
@@ -222,7 +222,25 @@ local config_schema = {
}
}
}
- }
+ },
+ key_encrypt_salt = {
+ anyOf = {
+ {
+ type = "array",
Review Comment:
Let's add length check for the array
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #7925: feat: support ssl key-encrypt-salt rotation
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7925:
URL: https://github.com/apache/apisix/pull/7925#discussion_r978204300
##########
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
Review Comment:
Better to avoid using boolean value as the default value of a table?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #7925: feat: support ssl key-encrypt-salt rotation
Posted by GitBox <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #7925:
URL: https://github.com/apache/apisix/pull/7925#discussion_r972671790
##########
conf/config-default.yaml:
##########
@@ -114,9 +114,10 @@ apisix:
ssl_session_tickets: false # disable ssl_session_tickets by default for 'ssl_session_tickets' would make Perfect Forward Secrecy useless.
# ref: https://github.com/mozilla/server-side-tls/issues/135
- 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.
Review Comment:
I have test case about this case
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a diff in pull request #7925: feat: support ssl key-encrypt-salt rotation
Posted by GitBox <gi...@apache.org>.
membphis commented on code in PR #7925:
URL: https://github.com/apache/apisix/pull/7925#discussion_r972643689
##########
conf/config-default.yaml:
##########
@@ -114,9 +114,10 @@ apisix:
ssl_session_tickets: false # disable ssl_session_tickets by default for 'ssl_session_tickets' would make Perfect Forward Secrecy useless.
# ref: https://github.com/mozilla/server-side-tls/issues/135
- 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.
Review Comment:
Is this a breaking change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #7925: feat: support ssl key-encrypt-salt rotation
Posted by GitBox <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #7925:
URL: https://github.com/apache/apisix/pull/7925#discussion_r974265958
##########
t/admin/ssl4.t:
##########
@@ -0,0 +1,358 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
+
+log_level('debug');
+no_root_location();
+
+add_block_preprocessor( sub{
+ my ($block) = @_;
+
+ if (!$block->request) {
+ $block->set_value("request", "GET /t");
+ }
+
+ if ((!defined $block->error_log) && (!defined $block->no_error_log)) {
+ $block->set_value("no_error_log", "[error]");
+ }
+
+ my $TEST_NGINX_HTML_DIR ||= html_dir();
+
+ my $config = <<_EOC_;
+listen unix:$TEST_NGINX_HTML_DIR/nginx.sock ssl;
+
+location /t {
+ content_by_lua_block {
+ -- etcd sync
+ ngx.sleep(0.2)
+
+ do
+ local sock = ngx.socket.tcp()
+
+ sock:settimeout(2000)
+
+ local ok, err = sock:connect("unix:$TEST_NGINX_HTML_DIR/nginx.sock")
+ if not ok then
+ ngx.say("failed to connect: ", err)
+ return
+ end
+
+ ngx.say("connected: ", ok)
+
+ local sess, err = sock:sslhandshake(nil, "www.test.com", true)
+ if not sess then
+ ngx.say("failed to do SSL handshake: ", err)
+ return
+ end
+
+ ngx.say("ssl handshake: ", sess ~= nil)
+
+ local req = "GET /hello HTTP/1.0\\r\\nHost: www.test.com\\r\\nConnection: close\\r\\n\\r\\n"
+ local bytes, err = sock:send(req)
+ if not bytes then
+ ngx.say("failed to send http request: ", err)
+ return
+ end
+
+ ngx.say("sent http request: ", bytes, " bytes.")
+
+ while true do
+ local line, err = sock:receive()
+ if not line then
+ -- ngx.say("failed to receive response status line: ", err)
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #7925: feat: support ssl key-encrypt-salt rotation
Posted by GitBox <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #7925:
URL: https://github.com/apache/apisix/pull/7925#discussion_r972872446
##########
apisix/ssl.lua:
##########
@@ -55,23 +56,37 @@ function _M.server_name()
end
-local _aes_128_cbc_with_iv = false
+local _aes_128_cbc_with_iv_tbl = false
local function get_aes_128_cbc_with_iv()
- if _aes_128_cbc_with_iv == false then
+ if _aes_128_cbc_with_iv_tbl == false then
+ _aes_128_cbc_with_iv_tbl = core.table.new(2, 0)
local local_conf = core.config.local_conf()
- local iv = core.table.try_read_attr(local_conf, "apisix", "ssl", "key_encrypt_salt")
- if type(iv) =="string" and #iv == 16 then
- _aes_128_cbc_with_iv = assert(aes:new(iv, nil, aes.cipher(128, "cbc"), {iv = iv}))
- else
- _aes_128_cbc_with_iv = nil
+ local ivs = core.table.try_read_attr(local_conf, "apisix", "ssl", "key_encrypt_salt")
+ local type_ivs = type(ivs)
+
+ if type_ivs == "table" then
+ for index, iv in ipairs(ivs) do
+ if type(iv) == "string" and #iv == 16 then
Review Comment:
we need backward compatibility, how to support both table and string if use schema?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #7925: feat: support ssl key-encrypt-salt rotation
Posted by GitBox <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #7925:
URL: https://github.com/apache/apisix/pull/7925#discussion_r973753150
##########
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
local function get_aes_128_cbc_with_iv()
- if _aes_128_cbc_with_iv == false then
+ if _aes_128_cbc_with_iv_tbl == false then
+ _aes_128_cbc_with_iv_tbl = core.table.new(2, 0)
local local_conf = core.config.local_conf()
- local iv = core.table.try_read_attr(local_conf, "apisix", "ssl", "key_encrypt_salt")
- if type(iv) =="string" and #iv == 16 then
- _aes_128_cbc_with_iv = assert(aes:new(iv, nil, aes.cipher(128, "cbc"), {iv = iv}))
- else
- _aes_128_cbc_with_iv = nil
+ local ivs = core.table.try_read_attr(local_conf, "apisix", "ssl", "key_encrypt_salt")
+ local type_ivs = type(ivs)
+
+ if type_ivs == "table" then
+ for _, iv in ipairs(ivs) do
+ 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
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a diff in pull request #7925: feat: support ssl key-encrypt-salt rotation
Posted by GitBox <gi...@apache.org>.
membphis commented on code in PR #7925:
URL: https://github.com/apache/apisix/pull/7925#discussion_r972643689
##########
conf/config-default.yaml:
##########
@@ -114,9 +114,10 @@ apisix:
ssl_session_tickets: false # disable ssl_session_tickets by default for 'ssl_session_tickets' would make Perfect Forward Secrecy useless.
# ref: https://github.com/mozilla/server-side-tls/issues/135
- 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.
Review Comment:
Is this an incompatible change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #7925: feat: support ssl key-encrypt-salt rotation
Posted by GitBox <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #7925:
URL: https://github.com/apache/apisix/pull/7925#discussion_r974335153
##########
apisix/cli/schema.lua:
##########
@@ -222,7 +222,25 @@ local config_schema = {
}
}
}
- }
+ },
+ key_encrypt_salt = {
+ anyOf = {
Review Comment:
But this is the conventional way in APISIX, it's used everywhere, so i think i should follow this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tokers commented on a diff in pull request #7925: feat: support ssl key-encrypt-salt rotation
Posted by GitBox <gi...@apache.org>.
tokers commented on code in PR #7925:
URL: https://github.com/apache/apisix/pull/7925#discussion_r973815301
##########
apisix/cli/schema.lua:
##########
@@ -222,7 +222,25 @@ local config_schema = {
}
}
}
- }
+ },
+ key_encrypt_salt = {
+ anyOf = {
Review Comment:
I know `anyOf` can satisfy the schema check but it should be `oneOf` in this scene?
##########
conf/config-default.yaml:
##########
@@ -114,9 +114,10 @@ apisix:
ssl_session_tickets: false # disable ssl_session_tickets by default for 'ssl_session_tickets' would make Perfect Forward Secrecy useless.
# ref: https://github.com/mozilla/server-side-tls/issues/135
- 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.
+ - 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.
Review Comment:
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`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #7925: feat: support ssl key-encrypt-salt rotation
Posted by GitBox <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #7925:
URL: https://github.com/apache/apisix/pull/7925#discussion_r974270947
##########
conf/config-default.yaml:
##########
@@ -114,9 +114,10 @@ apisix:
ssl_session_tickets: false # disable ssl_session_tickets by default for 'ssl_session_tickets' would make Perfect Forward Secrecy useless.
# ref: https://github.com/mozilla/server-side-tls/issues/135
- 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.
+ - 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.
Review Comment:
Should we only recommend using array？
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tokers commented on a diff in pull request #7925: feat: support ssl key-encrypt-salt rotation
Posted by GitBox <gi...@apache.org>.
tokers commented on code in PR #7925:
URL: https://github.com/apache/apisix/pull/7925#discussion_r972818851
##########
conf/config-default.yaml:
##########
@@ -114,9 +114,10 @@ apisix:
ssl_session_tickets: false # disable ssl_session_tickets by default for 'ssl_session_tickets' would make Perfect Forward Secrecy useless.
# ref: https://github.com/mozilla/server-side-tls/issues/135
- 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.
+ - 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.
Review Comment:
The comment is not accurate to describe the field, since the comment says it "must be a string of length 16".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #7925: feat: support ssl key-encrypt-salt rotation
Posted by GitBox <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #7925:
URL: https://github.com/apache/apisix/pull/7925#discussion_r972859020
##########
t/admin/ssl4.t:
##########
@@ -0,0 +1,382 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
+
+log_level('debug');
+no_root_location();
+
+add_block_preprocessor( sub{
+ my ($block) = @_;
+
+ my $TEST_NGINX_HTML_DIR ||= html_dir();
+
+ my $config = <<_EOC_;
+listen unix:$TEST_NGINX_HTML_DIR/nginx.sock ssl;
+
+location /t {
+ content_by_lua_block {
+ -- etcd sync
+ ngx.sleep(0.2)
+
+ do
+ local sock = ngx.socket.tcp()
+
+ sock:settimeout(2000)
+
+ local ok, err = sock:connect("unix:$TEST_NGINX_HTML_DIR/nginx.sock")
+ if not ok then
+ ngx.say("failed to connect: ", err)
+ return
+ end
+
+ ngx.say("connected: ", ok)
+
+ local sess, err = sock:sslhandshake(nil, "www.test.com", true)
+ if not sess then
+ ngx.say("failed to do SSL handshake: ", err)
+ return
+ end
+
+ ngx.say("ssl handshake: ", sess ~= nil)
+
+ local req = "GET /hello HTTP/1.0\\r\\nHost: www.test.com\\r\\nConnection: close\\r\\n\\r\\n"
+ local bytes, err = sock:send(req)
+ if not bytes then
+ ngx.say("failed to send http request: ", err)
+ return
+ end
+
+ ngx.say("sent http request: ", bytes, " bytes.")
+
+ while true do
+ local line, err = sock:receive()
+ if not line then
+ -- ngx.say("failed to receive response status line: ", err)
+ break
+ end
+
+ ngx.say("received: ", line)
+ end
+
+ local ok, err = sock:close()
+ ngx.say("close: ", ok, " ", err)
+ end -- do
+ -- collectgarbage()
+ }
+}
+_EOC_
+
+ if (!$block->config) {
+ $block->set_value("config", $config)
+ }
+}
+
+);
+
+
+run_tests;
+
+__DATA__
+
+=== TEST 1: set ssl(sni: www.test.com), encrypt with the first key_encrypt_salt
+--- yaml_config
+apisix:
+ node_listen: 1984
+ ssl:
+ key_encrypt_salt:
+ - edd1c9f0985e76a1
+ - edd1c9f0985e76a2
+--- config
+location /t {
+ content_by_lua_block {
+ local core = require("apisix.core")
+ local t = require("lib.test_admin")
+
+ local ssl_cert = t.read_file("t/certs/apisix.crt")
+ local ssl_key = t.read_file("t/certs/apisix.key")
+ local data = {cert = ssl_cert, key = ssl_key, sni = "www.test.com"}
+
+ local code, body = t.test('/apisix/admin/ssls/1',
+ ngx.HTTP_PUT,
+ core.json.encode(data),
+ [[{
+ "value": {
+ "sni": "www.test.com"
+ },
+ "key": "/apisix/ssls/1"
+ }]]
+ )
+
+ ngx.status = code
+ ngx.say(body)
+ }
+}
+--- request
Review Comment:
Done
##########
apisix/ssl.lua:
##########
@@ -55,23 +56,37 @@ function _M.server_name()
end
-local _aes_128_cbc_with_iv = false
+local _aes_128_cbc_with_iv_tbl = false
local function get_aes_128_cbc_with_iv()
- if _aes_128_cbc_with_iv == false then
+ if _aes_128_cbc_with_iv_tbl == false then
+ _aes_128_cbc_with_iv_tbl = core.table.new(2, 0)
local local_conf = core.config.local_conf()
- local iv = core.table.try_read_attr(local_conf, "apisix", "ssl", "key_encrypt_salt")
- if type(iv) =="string" and #iv == 16 then
- _aes_128_cbc_with_iv = assert(aes:new(iv, nil, aes.cipher(128, "cbc"), {iv = iv}))
- else
- _aes_128_cbc_with_iv = nil
+ local ivs = core.table.try_read_attr(local_conf, "apisix", "ssl", "key_encrypt_salt")
+ local type_ivs = type(ivs)
+
+ if type_ivs == "table" then
+ for index, iv in ipairs(ivs) do
+ if type(iv) == "string" and #iv == 16 then
+ 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 "
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #7925: feat: support ssl key-encrypt-salt rotation
Posted by GitBox <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #7925:
URL: https://github.com/apache/apisix/pull/7925#discussion_r978218225
##########
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
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #7925: feat: support ssl key-encrypt-salt rotation
Posted by GitBox <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #7925:
URL: https://github.com/apache/apisix/pull/7925#discussion_r974265169
##########
t/admin/ssl4.t:
##########
@@ -0,0 +1,358 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
+
+log_level('debug');
+no_root_location();
+
+add_block_preprocessor( sub{
+ my ($block) = @_;
+
+ if (!$block->request) {
+ $block->set_value("request", "GET /t");
+ }
+
+ if ((!defined $block->error_log) && (!defined $block->no_error_log)) {
+ $block->set_value("no_error_log", "[error]");
+ }
+
+ my $TEST_NGINX_HTML_DIR ||= html_dir();
+
+ my $config = <<_EOC_;
+listen unix:$TEST_NGINX_HTML_DIR/nginx.sock ssl;
+
+location /t {
+ content_by_lua_block {
+ -- etcd sync
+ ngx.sleep(0.2)
+
+ do
+ local sock = ngx.socket.tcp()
+
+ sock:settimeout(2000)
+
+ local ok, err = sock:connect("unix:$TEST_NGINX_HTML_DIR/nginx.sock")
+ if not ok then
+ ngx.say("failed to connect: ", err)
+ return
+ end
+
+ ngx.say("connected: ", ok)
+
+ local sess, err = sock:sslhandshake(nil, "www.test.com", true)
+ if not sess then
+ ngx.say("failed to do SSL handshake: ", err)
+ return
+ end
+
+ ngx.say("ssl handshake: ", sess ~= nil)
+
+ local req = "GET /hello HTTP/1.0\\r\\nHost: www.test.com\\r\\nConnection: close\\r\\n\\r\\n"
+ local bytes, err = sock:send(req)
+ if not bytes then
+ ngx.say("failed to send http request: ", err)
+ return
+ end
+
+ ngx.say("sent http request: ", bytes, " bytes.")
+
+ while true do
+ local line, err = sock:receive()
+ if not line then
+ -- ngx.say("failed to receive response status line: ", err)
+ break
+ end
+
+ ngx.say("received: ", line)
+ end
+
+ local ok, err = sock:close()
+ ngx.say("close: ", ok, " ", err)
+ end -- do
+ -- collectgarbage()
+ }
+}
+_EOC_
+
+ if (!$block->config) {
+ $block->set_value("config", $config)
+ }
+}
+
+);
+
+
+run_tests;
+
+__DATA__
+
+=== TEST 1: set ssl(sni: www.test.com), encrypt with the first key_encrypt_salt
+--- yaml_config
+apisix:
+ node_listen: 1984
+ ssl:
+ key_encrypt_salt:
+ - edd1c9f0985e76a1
+ - edd1c9f0985e76a2
+--- config
+location /t {
+ content_by_lua_block {
+ local core = require("apisix.core")
+ local t = require("lib.test_admin")
+
+ local ssl_cert = t.read_file("t/certs/apisix.crt")
+ local ssl_key = t.read_file("t/certs/apisix.key")
+ local data = {cert = ssl_cert, key = ssl_key, sni = "www.test.com"}
+
+ local code, body = t.test('/apisix/admin/ssls/1',
+ ngx.HTTP_PUT,
+ core.json.encode(data),
+ [[{
+ "value": {
+ "sni": "www.test.com"
+ },
+ "key": "/apisix/ssls/1"
+ }]]
+ )
+
+ ngx.status = code
+ ngx.say(body)
+ }
+}
+--- response_body
+passed
+
+
+
+=== TEST 2: set route(id: 1)
+--- yaml_config
+apisix:
+ node_listen: 1984
+ ssl:
+ key_encrypt_salt: "edd1c9f0985e76a1"
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 3: client request with the old style key_encrypt_salt
+--- yaml_config
+apisix:
+ node_listen: 1984
+ ssl:
+ key_encrypt_salt: "edd1c9f0985e76a1"
+--- response_body eval
Review Comment:
Just a double check, i prefer to leave it there
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tokers commented on a diff in pull request #7925: feat: support ssl key-encrypt-salt rotation
Posted by GitBox <gi...@apache.org>.
tokers commented on code in PR #7925:
URL: https://github.com/apache/apisix/pull/7925#discussion_r972816224
##########
apisix/ssl.lua:
##########
@@ -55,23 +56,37 @@ function _M.server_name()
end
-local _aes_128_cbc_with_iv = false
+local _aes_128_cbc_with_iv_tbl = false
local function get_aes_128_cbc_with_iv()
- if _aes_128_cbc_with_iv == false then
+ if _aes_128_cbc_with_iv_tbl == false then
+ _aes_128_cbc_with_iv_tbl = core.table.new(2, 0)
local local_conf = core.config.local_conf()
- local iv = core.table.try_read_attr(local_conf, "apisix", "ssl", "key_encrypt_salt")
- if type(iv) =="string" and #iv == 16 then
- _aes_128_cbc_with_iv = assert(aes:new(iv, nil, aes.cipher(128, "cbc"), {iv = iv}))
- else
- _aes_128_cbc_with_iv = nil
+ local ivs = core.table.try_read_attr(local_conf, "apisix", "ssl", "key_encrypt_salt")
+ local type_ivs = type(ivs)
+
+ if type_ivs == "table" then
+ for index, iv in ipairs(ivs) do
+ if type(iv) == "string" and #iv == 16 then
Review Comment:
+1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tzssangglass commented on a diff in pull request #7925: feat: support ssl key-encrypt-salt rotation
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #7925:
URL: https://github.com/apache/apisix/pull/7925#discussion_r973850799
##########
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
local function get_aes_128_cbc_with_iv()
- if _aes_128_cbc_with_iv == false then
+ if _aes_128_cbc_with_iv_tbl == false then
+ _aes_128_cbc_with_iv_tbl = core.table.new(2, 0)
local local_conf = core.config.local_conf()
- local iv = core.table.try_read_attr(local_conf, "apisix", "ssl", "key_encrypt_salt")
- if type(iv) =="string" and #iv == 16 then
- _aes_128_cbc_with_iv = assert(aes:new(iv, nil, aes.cipher(128, "cbc"), {iv = iv}))
- else
- _aes_128_cbc_with_iv = nil
+ local ivs = core.table.try_read_attr(local_conf, "apisix", "ssl", "key_encrypt_salt")
+ local type_ivs = type(ivs)
+
+ if type_ivs == "table" then
+ for _, iv in ipairs(ivs) do
+ 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" then
+ local aes_with_iv = assert(aes:new(ivs, nil, aes.cipher(128, "cbc"), {iv = ivs}))
+ core.table.insert(_aes_128_cbc_with_iv_tbl, aes_with_iv)
Review Comment:
how about `type_ivs` is not `table` or `string`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #7925: feat: support ssl key-encrypt-salt rotation
Posted by GitBox <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #7925:
URL: https://github.com/apache/apisix/pull/7925#discussion_r973752475
##########
apisix/cli/schema.lua:
##########
@@ -222,7 +222,25 @@ local config_schema = {
}
}
}
- }
+ },
+ key_encrypt_salt = {
+ anyOf = {
+ {
+ type = "array",
Review Comment:
minItems = 1? why need this check?
##########
apisix/cli/schema.lua:
##########
@@ -222,7 +222,25 @@ local config_schema = {
}
}
}
- }
+ },
+ key_encrypt_salt = {
+ anyOf = {
+ {
+ type = "array",
+ items = {
+ type = "string",
+ minLength = 16,
+ maxLength = 16
+ }
+ },
+ {
+ type = "string",
+ minLength = 16,
+ maxLength = 16
+ }
+ }
+
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #7925: feat: support ssl key-encrypt-salt rotation
Posted by GitBox <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #7925:
URL: https://github.com/apache/apisix/pull/7925#discussion_r972671447
##########
conf/config-default.yaml:
##########
@@ -114,9 +114,10 @@ apisix:
ssl_session_tickets: false # disable ssl_session_tickets by default for 'ssl_session_tickets' would make Perfect Forward Secrecy useless.
# ref: https://github.com/mozilla/server-side-tls/issues/135
- 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.
Review Comment:
it's backward compatible, we could use 2 ways at the same time
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #7925: feat: support ssl key-encrypt-salt rotation
Posted by GitBox <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #7925:
URL: https://github.com/apache/apisix/pull/7925#discussion_r974429233
##########
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
local function get_aes_128_cbc_with_iv()
- if _aes_128_cbc_with_iv == false then
+ if _aes_128_cbc_with_iv_tbl == false then
+ _aes_128_cbc_with_iv_tbl = core.table.new(2, 0)
local local_conf = core.config.local_conf()
- local iv = core.table.try_read_attr(local_conf, "apisix", "ssl", "key_encrypt_salt")
- if type(iv) =="string" and #iv == 16 then
- _aes_128_cbc_with_iv = assert(aes:new(iv, nil, aes.cipher(128, "cbc"), {iv = iv}))
- else
- _aes_128_cbc_with_iv = nil
+ local ivs = core.table.try_read_attr(local_conf, "apisix", "ssl", "key_encrypt_salt")
+ local type_ivs = type(ivs)
+
+ if type_ivs == "table" then
+ for _, iv in ipairs(ivs) do
+ 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" then
+ local aes_with_iv = assert(aes:new(ivs, nil, aes.cipher(128, "cbc"), {iv = ivs}))
+ core.table.insert(_aes_128_cbc_with_iv_tbl, aes_with_iv)
Review Comment:
It will raise an error because the limitation in schema.lua.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #7925: feat: support ssl key-encrypt-salt rotation
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7925:
URL: https://github.com/apache/apisix/pull/7925#discussion_r972672185
##########
apisix/ssl.lua:
##########
@@ -55,23 +56,37 @@ function _M.server_name()
end
-local _aes_128_cbc_with_iv = false
+local _aes_128_cbc_with_iv_tbl = false
local function get_aes_128_cbc_with_iv()
- if _aes_128_cbc_with_iv == false then
+ if _aes_128_cbc_with_iv_tbl == false then
+ _aes_128_cbc_with_iv_tbl = core.table.new(2, 0)
local local_conf = core.config.local_conf()
- local iv = core.table.try_read_attr(local_conf, "apisix", "ssl", "key_encrypt_salt")
- if type(iv) =="string" and #iv == 16 then
- _aes_128_cbc_with_iv = assert(aes:new(iv, nil, aes.cipher(128, "cbc"), {iv = iv}))
- else
- _aes_128_cbc_with_iv = nil
+ local ivs = core.table.try_read_attr(local_conf, "apisix", "ssl", "key_encrypt_salt")
+ local type_ivs = type(ivs)
+
+ if type_ivs == "table" then
+ for index, iv in ipairs(ivs) do
+ if type(iv) == "string" and #iv == 16 then
+ 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 "
Review Comment:
We can use `,` instead of `..` as separator
##########
apisix/ssl.lua:
##########
@@ -55,23 +56,37 @@ function _M.server_name()
end
-local _aes_128_cbc_with_iv = false
+local _aes_128_cbc_with_iv_tbl = false
local function get_aes_128_cbc_with_iv()
- if _aes_128_cbc_with_iv == false then
+ if _aes_128_cbc_with_iv_tbl == false then
+ _aes_128_cbc_with_iv_tbl = core.table.new(2, 0)
local local_conf = core.config.local_conf()
- local iv = core.table.try_read_attr(local_conf, "apisix", "ssl", "key_encrypt_salt")
- if type(iv) =="string" and #iv == 16 then
- _aes_128_cbc_with_iv = assert(aes:new(iv, nil, aes.cipher(128, "cbc"), {iv = iv}))
- else
- _aes_128_cbc_with_iv = nil
+ local ivs = core.table.try_read_attr(local_conf, "apisix", "ssl", "key_encrypt_salt")
+ local type_ivs = type(ivs)
+
+ if type_ivs == "table" then
+ for index, iv in ipairs(ivs) do
+ if type(iv) == "string" and #iv == 16 then
Review Comment:
Better to check this via schema in https://github.com/apache/apisix/blob/master/apisix/cli/schema.lua
##########
apisix/ssl.lua:
##########
@@ -86,32 +101,32 @@ function _M.aes_encrypt_pkey(origin)
end
-local function decrypt_priv_pkey(iv, key)
- local decoded_key = ngx_decode_base64(key)
- if not decoded_key then
- core.log.error("base64 decode ssl key failed. key[", key, "] ")
- return nil
+local function aes_decrypt_pkey(origin)
+ if core.string.has_prefix(origin, "---") then
+ return origin
end
- local decrypted = iv:decrypt(decoded_key)
- 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
Review Comment:
Use `#` would be better as the table must be array
##########
t/admin/ssl4.t:
##########
@@ -0,0 +1,382 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
+
+log_level('debug');
+no_root_location();
+
+add_block_preprocessor( sub{
+ my ($block) = @_;
+
+ my $TEST_NGINX_HTML_DIR ||= html_dir();
+
+ my $config = <<_EOC_;
+listen unix:$TEST_NGINX_HTML_DIR/nginx.sock ssl;
+
+location /t {
+ content_by_lua_block {
+ -- etcd sync
+ ngx.sleep(0.2)
+
+ do
+ local sock = ngx.socket.tcp()
+
+ sock:settimeout(2000)
+
+ local ok, err = sock:connect("unix:$TEST_NGINX_HTML_DIR/nginx.sock")
+ if not ok then
+ ngx.say("failed to connect: ", err)
+ return
+ end
+
+ ngx.say("connected: ", ok)
+
+ local sess, err = sock:sslhandshake(nil, "www.test.com", true)
+ if not sess then
+ ngx.say("failed to do SSL handshake: ", err)
+ return
+ end
+
+ ngx.say("ssl handshake: ", sess ~= nil)
+
+ local req = "GET /hello HTTP/1.0\\r\\nHost: www.test.com\\r\\nConnection: close\\r\\n\\r\\n"
+ local bytes, err = sock:send(req)
+ if not bytes then
+ ngx.say("failed to send http request: ", err)
+ return
+ end
+
+ ngx.say("sent http request: ", bytes, " bytes.")
+
+ while true do
+ local line, err = sock:receive()
+ if not line then
+ -- ngx.say("failed to receive response status line: ", err)
+ break
+ end
+
+ ngx.say("received: ", line)
+ end
+
+ local ok, err = sock:close()
+ ngx.say("close: ", ok, " ", err)
+ end -- do
+ -- collectgarbage()
+ }
+}
+_EOC_
+
+ if (!$block->config) {
+ $block->set_value("config", $config)
+ }
+}
+
+);
+
+
+run_tests;
+
+__DATA__
+
+=== TEST 1: set ssl(sni: www.test.com), encrypt with the first key_encrypt_salt
+--- yaml_config
+apisix:
+ node_listen: 1984
+ ssl:
+ key_encrypt_salt:
+ - edd1c9f0985e76a1
+ - edd1c9f0985e76a2
+--- config
+location /t {
+ content_by_lua_block {
+ local core = require("apisix.core")
+ local t = require("lib.test_admin")
+
+ local ssl_cert = t.read_file("t/certs/apisix.crt")
+ local ssl_key = t.read_file("t/certs/apisix.key")
+ local data = {cert = ssl_cert, key = ssl_key, sni = "www.test.com"}
+
+ local code, body = t.test('/apisix/admin/ssls/1',
+ ngx.HTTP_PUT,
+ core.json.encode(data),
+ [[{
+ "value": {
+ "sni": "www.test.com"
+ },
+ "key": "/apisix/ssls/1"
+ }]]
+ )
+
+ ngx.status = code
+ ngx.say(body)
+ }
+}
+--- request
Review Comment:
Let's move the common part to the top like other test files
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #7925: feat: support ssl key-encrypt-salt rotation
Posted by GitBox <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #7925:
URL: https://github.com/apache/apisix/pull/7925#discussion_r972878226
##########
conf/config-default.yaml:
##########
@@ -114,9 +114,10 @@ apisix:
ssl_session_tickets: false # disable ssl_session_tickets by default for 'ssl_session_tickets' would make Perfect Forward Secrecy useless.
# ref: https://github.com/mozilla/server-side-tls/issues/135
- 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.
+ - 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.
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #7925: feat: support ssl key-encrypt-salt rotation
Posted by GitBox <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #7925:
URL: https://github.com/apache/apisix/pull/7925#discussion_r972877912
##########
apisix/ssl.lua:
##########
@@ -55,23 +56,37 @@ function _M.server_name()
end
-local _aes_128_cbc_with_iv = false
+local _aes_128_cbc_with_iv_tbl = false
local function get_aes_128_cbc_with_iv()
- if _aes_128_cbc_with_iv == false then
+ if _aes_128_cbc_with_iv_tbl == false then
+ _aes_128_cbc_with_iv_tbl = core.table.new(2, 0)
local local_conf = core.config.local_conf()
- local iv = core.table.try_read_attr(local_conf, "apisix", "ssl", "key_encrypt_salt")
- if type(iv) =="string" and #iv == 16 then
- _aes_128_cbc_with_iv = assert(aes:new(iv, nil, aes.cipher(128, "cbc"), {iv = iv}))
- else
- _aes_128_cbc_with_iv = nil
+ local ivs = core.table.try_read_attr(local_conf, "apisix", "ssl", "key_encrypt_salt")
+ local type_ivs = type(ivs)
+
+ if type_ivs == "table" then
+ for index, iv in ipairs(ivs) do
+ if type(iv) == "string" and #iv == 16 then
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander merged pull request #7925: feat: support ssl key-encrypt-salt rotation
Posted by GitBox <gi...@apache.org>.
spacewander merged PR #7925:
URL: https://github.com/apache/apisix/pull/7925
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kayx23 commented on a diff in pull request #7925: feat: support ssl key-encrypt-salt rotation
Posted by "kayx23 (via GitHub)" <gi...@apache.org>.
kayx23 commented on code in PR #7925:
URL: https://github.com/apache/apisix/pull/7925#discussion_r1223790244
##########
conf/config-default.yaml:
##########
@@ -114,9 +114,10 @@ apisix:
ssl_session_tickets: false # disable ssl_session_tickets by default for 'ssl_session_tickets' would make Perfect Forward Secrecy useless.
# ref: https://github.com/mozilla/server-side-tls/issues/135
- 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.
+ - 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.
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org