Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
Merged
4 changes: 4 additions & 0 deletions apisix/cli/file.lua
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,10 @@ local function path_is_multi_type(path, type_val)
return true
end

if path == "apisix->ssl->key_encrypt_salt" then
return true
end

return false
end

Expand Down
20 changes: 19 additions & 1 deletion apisix/cli/schema.lua
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,25 @@ local config_schema = {
}
}
}
}
},
key_encrypt_salt = {
anyOf = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
type = "array",
minItems = 1,
items = {
type = "string",
minLength = 16,
maxLength = 16
}
},
{
type = "string",
minLength = 16,
maxLength = 16
}
}
},
}
},
}
Expand Down
66 changes: 38 additions & 28 deletions apisix/ssl.lua
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ local aes = require("resty.aes")
local str_lower = string.lower
local assert = assert
local type = type
local ipairs = ipairs


local cert_cache = core.lrucache.new {
Expand Down Expand Up @@ -55,23 +56,32 @@ function _M.server_name()
end


local _aes_128_cbc_with_iv = false
local _aes_128_cbc_with_iv_tbl = false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

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)
tzssangglass marked this conversation as resolved.
Show resolved Hide resolved
end
end
return _aes_128_cbc_with_iv

return _aes_128_cbc_with_iv_tbl
end


function _M.aes_encrypt_pkey(origin)
local aes_128_cbc_with_iv = get_aes_128_cbc_with_iv()
local aes_128_cbc_with_iv_tbl = get_aes_128_cbc_with_iv()
local aes_128_cbc_with_iv = aes_128_cbc_with_iv_tbl[1]
if aes_128_cbc_with_iv ~= nil and core.string.has_prefix(origin, "---") then
local encrypted = aes_128_cbc_with_iv:encrypt(origin)
if encrypted == nil then
Expand All @@ -86,32 +96,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 #aes_128_cbc_with_iv_tbl == 0 then
return origin
end

return decrypted
end


local function aes_decrypt_pkey(origin)
if core.string.has_prefix(origin, "---") then
return origin
local decoded_key = ngx_decode_base64(origin)
if not decoded_key then
core.log.error("base64 decode ssl key failed. key[", origin, "] ")
return nil
end

local aes_128_cbc_with_iv = get_aes_128_cbc_with_iv()
if aes_128_cbc_with_iv ~= nil then
return decrypt_priv_pkey(aes_128_cbc_with_iv, origin)
for _, aes_128_cbc_with_iv in ipairs(aes_128_cbc_with_iv_tbl) do
local decrypted = aes_128_cbc_with_iv:decrypt(decoded_key)
if decrypted then
return decrypted
end
end
return origin

core.log.error("decrypt ssl key failed")

return nil
end


Expand Down
7 changes: 4 additions & 3 deletions conf/config-default.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

@membphis membphis Sep 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a breaking change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have test case about this case

- edd1c9f0985e76a2 # If set this, must be a array filled with 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we only recommend using array?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


#fallback_sni: "my.default.domain" # If set this, when the client doesn't send SNI during handshake, the fallback SNI will be used instead
enable_control: true
Expand Down
Loading