From 04392670a1e4b43d52aac085bbdda9f08687af8a Mon Sep 17 00:00:00 2001 From: Wangchong Zhou Date: Wed, 8 Nov 2023 15:16:51 +0800 Subject: [PATCH] fix(schema): validate public and private key for `keys` entity (#11923) KAG-390 --------- Co-authored-by: Datong Sun --- .../unreleased/kong/validate_private_key.yml | 3 ++ kong/db/dao/keys.lua | 10 ++++- kong/db/schema/typedefs.lua | 33 +++++++++++---- kong/plugins/acme/client.lua | 16 +------ .../01-db/01-schema/03-typedefs_spec.lua | 20 +++++++++ spec/02-integration/03-db/18-keys_spec.lua | 42 +++++++++++++++++++ 6 files changed, 100 insertions(+), 24 deletions(-) create mode 100644 changelog/unreleased/kong/validate_private_key.yml diff --git a/changelog/unreleased/kong/validate_private_key.yml b/changelog/unreleased/kong/validate_private_key.yml new file mode 100644 index 00000000000..70aa941103f --- /dev/null +++ b/changelog/unreleased/kong/validate_private_key.yml @@ -0,0 +1,3 @@ +message: Validate private and public key for `keys` entity to ensure they match each other. +type: bugfix +scope: Core diff --git a/kong/db/dao/keys.lua b/kong/db/dao/keys.lua index 1f04fadf710..8e14f0ac55b 100644 --- a/kong/db/dao/keys.lua +++ b/kong/db/dao/keys.lua @@ -76,14 +76,20 @@ local function _load_pkey(key, part) pk, err = pkey.new(key.jwk, { format = "JWK" }) end if key.pem then - if not key.pem[part] then - return nil, fmt("%s key not found.", part) + -- public key can be derived from private key, but not vice versa + if part == "private_key" and not key.pem[part] then + return nil, "could not load a private key from public key material" end pk, err = pkey.new(key.pem[part], { format = "PEM" }) end if not pk then return nil, "could not load pkey. " .. err end + + if part == "private_key" and not pk:is_private() then + return nil, "could not load a private key from public key material" + end + return pk end diff --git a/kong/db/schema/typedefs.lua b/kong/db/schema/typedefs.lua index 3838b10d10b..cd875302280 100644 --- a/kong/db/schema/typedefs.lua +++ b/kong/db/schema/typedefs.lua @@ -654,20 +654,34 @@ local function validate_pem_keys(values) local private_key = values.private_key -- unless it's a vault reference - if kong.vault.is_reference(private_key) or - kong.vault.is_reference(public_key) then + if kong and ( + kong.vault.is_reference(private_key) or + kong.vault.is_reference(public_key)) then return true end - local pk, err = openssl_pkey.new(public_key, { format = "PEM" }) - if not pk or err then - return false, "could not load public key" + local pubkey, privkey, err + + if public_key and public_key ~= null then + pubkey, err = openssl_pkey.new(public_key, { format = "PEM", type = "pu" }) + if not pubkey or err then + return false, "could not load public key" + end end - local ppk, perr = openssl_pkey.new(private_key, { format = "PEM" }) - if not ppk or perr then - return false, "could not load private key" .. (perr or "") + if private_key and private_key ~= null then + privkey, err = openssl_pkey.new(private_key, { format = "PEM", type = "pr" }) + if not privkey or err then + return false, "could not load private key" .. (err or "") + end end + + if privkey and pubkey then + if privkey:to_PEM("public") ~= pubkey:to_PEM() then + return false, "public key does not match private key" + end + end + return true end @@ -691,6 +705,9 @@ typedefs.pem = Schema.define { }, }, }, + entity_checks = { + { at_least_one_of = { "private_key", "public_key" } } + }, custom_validator = validate_pem_keys, description = "A pair of PEM-encoded public and private keys, which can be either a string or a reference to a credential in Kong Vault. If provided as strings, they must be valid PEM-encoded keys." diff --git a/kong/plugins/acme/client.lua b/kong/plugins/acme/client.lua index cb3cb3d8749..826f0a03050 100644 --- a/kong/plugins/acme/client.lua +++ b/kong/plugins/acme/client.lua @@ -234,13 +234,7 @@ local function get_account_key(conf) local key_set, key_set_err = kong.db.key_sets:select_by_name(conf.key_set) if key_set_err then - kong.log.warn("error loading keyset ", conf.key_set, " : ", key_set_err) - return nil, key_set_err - end - - if not key_set then - kong.log.warn("could not load keyset nil value was returned") - return nil, error("nil returned by key_sets:select_by_name for key_set ", conf.key_set) + return nil, "could not load keyset: " .. key_set_err end lookup.set = {id = key_set.id} @@ -250,13 +244,7 @@ local function get_account_key(conf) local key, key_err = kong.db.keys:select_by_cache_key(cache_key) if key_err then - kong.log.warn("error loading key ", kid, " : ", key_err) - return nil, key_err - end - - if not key then - kong.log.warn("could not load key nil value was returned") - return nil, error("nil returned by keys:select_by_cache_key for key ", conf.key_id) + return nil, "could not load keys: " .. key_err end return kong.db.keys:get_privkey(key) diff --git a/spec/01-unit/01-db/01-schema/03-typedefs_spec.lua b/spec/01-unit/01-db/01-schema/03-typedefs_spec.lua index cbd011d2559..1183e0858e0 100644 --- a/spec/01-unit/01-db/01-schema/03-typedefs_spec.lua +++ b/spec/01-unit/01-db/01-schema/03-typedefs_spec.lua @@ -203,4 +203,24 @@ describe("typedefs", function() assert.equal(false, uuid2.auto) end) + it("features pem", function() + local Test = Schema.new({ + fields = { + { f = typedefs.pem } + } + }) + local tmpkey = openssl_pkey.new { type = 'EC', curve = 'prime256v1' } + assert.truthy(Test:validate({ f = { public_key = tmpkey:to_PEM("public") }})) + assert.truthy(Test:validate({ f = { private_key = tmpkey:to_PEM("private") }})) + assert.falsy( Test:validate({ f = { private_key = tmpkey:to_PEM("public") }})) + assert.falsy(Test:validate({ f = { public_key = tmpkey:to_PEM("private") }})) + assert.truthy(Test:validate({ f = { public_key = tmpkey:to_PEM("public"), + private_key = tmpkey:to_PEM("private") }})) + local anotherkey = openssl_pkey.new { type = 'EC', curve = 'prime256v1' } + assert.falsy( Test:validate({ f = { public_key = anotherkey:to_PEM("public"), + private_key = tmpkey:to_PEM("private") }})) + assert.falsy( Test:validate({ f = { public_key = tmpkey:to_PEM("public"), + private_key = anotherkey:to_PEM("private") }})) +end) + end) diff --git a/spec/02-integration/03-db/18-keys_spec.lua b/spec/02-integration/03-db/18-keys_spec.lua index 737a25aaef5..5cac149a1e7 100644 --- a/spec/02-integration/03-db/18-keys_spec.lua +++ b/spec/02-integration/03-db/18-keys_spec.lua @@ -207,5 +207,47 @@ for _, strategy in helpers.all_strategies() do assert.is_not_nil(decoded_jwk.q) assert.is_not_nil(decoded_jwk.qi) end) + + it(":get_privkey errors if only got pubkey [pem]", function() + local pem_t, err = db.keys:insert { + name = "pem_key", + set = init_key_set, + kid = "999", + pem = { public_key = pem_pub } + } + assert.is_nil(err) + assert(pem_t) + + local pem_pub_t, g_err = db.keys:get_pubkey(pem_t) + assert.is_nil(g_err) + assert.matches("-----BEGIN PUBLIC KEY", pem_pub_t) + + local pem_priv, p_err = db.keys:get_privkey(pem_t) + assert.is_nil(pem_priv) + assert.matches("could not load a private key from public key material", p_err) + end) + + it(":get_privkey errors if only got pubkey [jwk]", function() + jwk.d = nil + local jwk_t, _ = db.keys:insert { + name = "jwk_key", + set = init_key_set, + kid = jwk.kid, + jwk = cjson.encode(jwk) + } + assert(jwk_t) + + local jwk_pub_t, g_err = db.keys:get_pubkey(jwk_t) + assert.is_nil(g_err) + local jwk_pub_o = cjson.decode(jwk_pub_t) + assert.is_not_nil(jwk_pub_o.e) + assert.is_not_nil(jwk_pub_o.kid) + assert.is_not_nil(jwk_pub_o.kty) + assert.is_not_nil(jwk_pub_o.n) + + local jwk_priv, p_err = db.keys:get_privkey(jwk_t) + assert.is_nil(jwk_priv) + assert.matches("could not load a private key from public key material", p_err) + end) end) end