-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
change(jwt-auth): unify apisix/core/vault.lua and apisix/secret/vault.lua #8660
Merged
Merged
Changes from 14 commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
ac8b7ab
feat: use get method from kms module
shreemaan-abhishek b506c2e
remove vault related configuration
shreemaan-abhishek 8243851
remove unused variable and function
shreemaan-abhishek c03c8cc
delete vault.lua
shreemaan-abhishek 736453c
delete vault test file
shreemaan-abhishek 82ec4b1
remove vout
shreemaan-abhishek a6076dd
docs: remove vault related parts
shreemaan-abhishek 8e2e47a
docs: add information about KMS
shreemaan-abhishek 0860711
docs: remove trailing space
shreemaan-abhishek e4ce6c8
docs: add information about KMS
shreemaan-abhishek 09f6c59
docs: update getting started guide
shreemaan-abhishek 2adf780
remove vault related configuration
shreemaan-abhishek 116ad96
fix link
shreemaan-abhishek 44208dc
fix comment
shreemaan-abhishek 41db740
return nil when key is missing
shreemaan-abhishek File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,7 +18,6 @@ local core = require("apisix.core") | |
local jwt = require("resty.jwt") | ||
local consumer_mod = require("apisix.consumer") | ||
local resty_random = require("resty.random") | ||
local vault = require("apisix.core.vault") | ||
local new_tab = require ("table.new") | ||
|
||
local ngx_encode_base64 = ngx.encode_base64 | ||
|
@@ -71,10 +70,6 @@ local consumer_schema = { | |
type = "boolean", | ||
default = false | ||
}, | ||
vault = { | ||
type = "object", | ||
properties = {} | ||
}, | ||
lifetime_grace_period = { | ||
type = "integer", | ||
minimum = 0, | ||
|
@@ -102,19 +97,6 @@ local consumer_schema = { | |
}, | ||
required = {"public_key", "private_key"}, | ||
}, | ||
{ | ||
properties = { | ||
vault = { | ||
type = "object", | ||
properties = {} | ||
}, | ||
algorithm = { | ||
enum = {"RS256", "ES256"}, | ||
}, | ||
}, | ||
required = {"vault"}, | ||
}, | ||
|
||
} | ||
} | ||
}, | ||
|
@@ -147,11 +129,6 @@ function _M.check_schema(conf, schema_type) | |
return false, err | ||
end | ||
|
||
if conf.vault then | ||
core.log.info("skipping jwt-auth schema validation with vault") | ||
return true | ||
end | ||
|
||
if conf.algorithm ~= "RS256" and conf.algorithm ~= "ES256" and not conf.secret then | ||
conf.secret = ngx_encode_base64(resty_random.bytes(32, true)) | ||
elseif conf.base64_secret then | ||
|
@@ -161,8 +138,8 @@ function _M.check_schema(conf, schema_type) | |
end | ||
|
||
if conf.algorithm == "RS256" or conf.algorithm == "ES256" then | ||
-- Possible options are a) both are in vault, b) both in schema | ||
-- c) one in schema, another in vault. | ||
-- Possible options are a) public key is missing | ||
-- b) private key is missing | ||
if not conf.public_key then | ||
return false, "missing valid public key" | ||
end | ||
|
@@ -243,25 +220,8 @@ local function fetch_jwt_token(conf, ctx) | |
return val | ||
end | ||
|
||
|
||
local function get_vault_path(username) | ||
return "consumer/".. username .. "/jwt-auth" | ||
end | ||
|
||
|
||
local function get_secret(conf, consumer_name) | ||
local secret = conf.secret | ||
if conf.vault then | ||
local res, err = vault.get(get_vault_path(consumer_name)) | ||
if not res then | ||
return nil, err | ||
end | ||
|
||
if not res.data or not res.data.secret then | ||
return nil, "secret could not found in vault: " .. core.json.encode(res) | ||
end | ||
secret = res.data.secret | ||
end | ||
|
||
if conf.base64_secret then | ||
return ngx_decode_base64(secret) | ||
|
@@ -274,32 +234,16 @@ end | |
local function get_rsa_or_ecdsa_keypair(conf, consumer_name) | ||
local public_key = conf.public_key | ||
local private_key = conf.private_key | ||
-- if keys are present in conf, no need to query vault (fallback) | ||
|
||
if public_key and private_key then | ||
return public_key, private_key | ||
elseif public_key and not private_key then | ||
return public_key, nil, "missing private key" | ||
elseif not public_key and private_key then | ||
return nil, private_key, "missing public key" | ||
else | ||
return nil, nil, "public and private keys are missing" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't remove the elseif statements so that a better error message could be returned. Let me know if this is not up to the mark. |
||
end | ||
|
||
local vout = {} | ||
if conf.vault then | ||
local res, err = vault.get(get_vault_path(consumer_name)) | ||
if not res then | ||
return nil, nil, err | ||
end | ||
|
||
if not res.data then | ||
return nil, nil, "key pairs could not found in vault: " .. core.json.encode(res) | ||
end | ||
vout = res.data | ||
end | ||
|
||
if not public_key and not vout.public_key then | ||
return nil, nil, "missing public key, not found in config/vault" | ||
end | ||
if not private_key and not vout.private_key then | ||
return nil, nil, "missing private key, not found in config/vault" | ||
end | ||
|
||
return public_key or vout.public_key, private_key or vout.private_key | ||
end | ||
|
||
|
||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will it be a problem in https://github.com/shreemaan-abhishek/apisix/blob/09f6c5971176b6fd60b9c090aacd1c556c072d1d/apisix/plugins/jwt-auth.lua#L291?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we add a nil check there? I think we should. Even the previous implementation would return nil in some cases. Not sure why it is not being checked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apisix/apisix/plugins/jwt-auth.lua
Line 347 in 9c899b8
Here we check if public_key is nil to detect the err.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not quite sure what you are trying to say, could you please elaborate? 😅