change(jwt-auth): unify apisix/core/vault.lua and apisix/secret/vault.lua by shreemaan-abhishek · Pull Request #8660 · apache/apisix

shreemaan-abhishek · 2023-01-12T02:09:22Z

Description

Checklist

I have explained the need for this PR and the problem it solves. Check the issue
I have explained the changes or the new features added to this PR NA
I have added tests corresponding to this change. Not yet
I have updated the documentation to reflect this change. NA
I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

soulbird · 2023-01-12T02:29:09Z

-local vault        = require("apisix.core.vault")
+local vault        = require("apisix.core.vault") -- to be removed later
 local new_tab = require ("table.new")
+local secrets = require("apisix.admin.secrets")


All functions in apisix.admin.secrets are only used to configure secret manager. It should not be called from plugins.

If you want to use the secret manager function, you should use the get function in apisix.secret

soulbird · 2023-01-12T02:34:46Z

@@ -252,15 +253,12 @@ end
 local function get_secret(conf, consumer_name)
    local secret = conf.secret
    if conf.vault then


Just delete the vault-related configuration directly, because we have already retrieved and replaced the configuration of all auth plugins (type = "auth") in the consumer.
https://github.com/apache/apisix/blob/master/apisix/consumer.lua#L107

soulbird · 2023-01-12T05:08:02Z

After deleting a configuration, a break change is introduced in APISIX, so the title of the PR should be change: xxx, here we change the jwt-auth plugin, so it is best to change(jwt-auth): xxx

Other legitimate headers can be seen here: https://github.com/apache/apisix/blob/master/.github/workflows/semantic.yml#L23

soulbird · 2023-01-12T05:12:06Z

Should also update doc： https://github.com/apache/apisix/blob/master/docs/en/latest/plugins/jwt-auth.md

If you are interested, you can update the Chinese documentation at the same time, you can use the tool to translate.
https://github.com/apache/apisix/blob/master/docs/zh/latest/plugins/jwt-auth.md

shreemaan-abhishek · 2023-01-16T16:30:33Z

How should this be handled?

soulbird · 2023-01-17T02:59:17Z

A link should be added after the field, pointing to APISIX Secret

soulbird · 2023-01-17T03:16:21Z

How should this be handled?

As described in the comments, this code fits into three possible scenarios. So, just remove some of the comments

soulbird · 2023-01-17T05:48:56Z

Vault related links in this chapter should be updated https://github.com/apache/apisix/blob/master/docs/en/latest/getting-started.md#features
https://github.com/apache/apisix/blob/master/docs/en/latest/getting-started.md#features

spacewander · 2023-01-18T03:02:38Z

    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"


Will it be a problem in https://github.com/shreemaan-abhishek/apisix/blob/09f6c5971176b6fd60b9c090aacd1c556c072d1d/apisix/plugins/jwt-auth.lua#L291?

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.

apisix/apisix/plugins/jwt-auth.lua

Line 347 in 9c899b8

if not public_key then

Here we check if public_key is nil to detect the err.

I am not quite sure what you are trying to say, could you please elaborate? 😅

spacewander

Can we drop

apisix/conf/config-default.yaml

Line 253 in 4131fbf

    
           # HashiCorp Vault storage backend for sensitive data retrieval. The config shows an example of what APISIX expects if you

now?

shreemaan-abhishek · 2023-01-29T10:39:34Z

+    elseif not public_key and private_key then
+        return nil, nil, "missing public key"
+    else
+        return nil, nil, "public and private keys are missing"


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.

* upstream/master: (67 commits) fix: grpc-transcode plugin: fix map data population (apache#8731) change(jwt-auth): unify apisix/core/vault.lua and apisix/secret/vault.lua (apache#8660) feat: stream subsystem support consul_kv service discovery (apache#8633) fix(proxy-mirror): use with uri rewrite (apache#8718) ci: move helper script to the right dir (apache#8691) refactor(pubsub): simpify the get_cmd implementation (apache#8608) feat: stream subsystem support kubernetes service discovery (apache#8640) docs: fix deployment links (apache#8714) fix: remove backslash before slash when encoding (apache#8684) ci: kafka should register port in the zookeeper same as exposed (apache#8672) docs: fix plugin config naming (apache#8701) docs: fix code block (apache#8700) docs: rename kms to secret (apache#8697) docs: replace transparent logos with white background logos (apache#8689) fix: upgrade lua-resty-etcd to 1.10.3 (apache#8668) fix: upgrade casbin to 1.41.3 to improve performance (apache#8676) chore: make send_stream_request more clear (apache#8627) feat: stream subsystem support nacos service discovery (apache#8584) feat: stream subsystem support dns service discovery (apache#8593) refactor(admin): refactor resource routes (apache#8611) ...

feat: use get method from kms module

ac8b7ab

soulbird reviewed Jan 12, 2023

View reviewed changes

shreemaan-abhishek added 2 commits January 12, 2023 10:00

remove vault related configuration

b506c2e

remove unused variable and function

8243851

shreemaan-abhishek changed the title ~~feat: use get method from kms module~~ unify apisix/core/vault.lua and apisix/secret/vault.lua Jan 12, 2023

shreemaan-abhishek changed the title ~~unify apisix/core/vault.lua and apisix/secret/vault.lua~~ fix: unify apisix/core/vault.lua and apisix/secret/vault.lua Jan 12, 2023

delete vault.lua

c03c8cc

shreemaan-abhishek marked this pull request as ready for review January 12, 2023 05:06

shreemaan-abhishek changed the title ~~fix: unify apisix/core/vault.lua and apisix/secret/vault.lua~~ change(jwt-auth): unify apisix/core/vault.lua and apisix/secret/vault.lua Jan 12, 2023

shreemaan-abhishek added 2 commits January 12, 2023 15:05

delete vault test file

736453c

remove vout

82ec4b1

shreemaan-abhishek commented Jan 12, 2023

View reviewed changes

Comment thread apisix/plugins/jwt-auth.lua Outdated

docs: remove vault related parts

a6076dd

shreemaan-abhishek added 2 commits January 17, 2023 10:05

docs: add information about KMS

8e2e47a

docs: remove trailing space

0860711

shreemaan-abhishek added 2 commits January 17, 2023 11:51

docs: add information about KMS

e4ce6c8

docs: update getting started guide

09f6c59

soulbird requested a review from spacewander January 17, 2023 06:59

soulbird previously approved these changes Jan 18, 2023

View reviewed changes

spacewander requested changes Jan 18, 2023

View reviewed changes

spacewander reviewed Jan 18, 2023

View reviewed changes

shreemaan-abhishek added 3 commits January 18, 2023 09:33

remove vault related configuration

2adf780

fix link

116ad96

fix comment

44208dc

shreemaan-abhishek dismissed soulbird’s stale review via 44208dc January 18, 2023 04:08

shreemaan-abhishek requested review from soulbird and spacewander and removed request for soulbird and spacewander January 18, 2023 04:27

shreemaan-abhishek closed this Jan 25, 2023

shreemaan-abhishek reopened this Jan 27, 2023

soulbird previously approved these changes Jan 29, 2023

View reviewed changes

return nil when key is missing

41db740

shreemaan-abhishek dismissed soulbird’s stale review via 41db740 January 29, 2023 10:36

shreemaan-abhishek commented Jan 29, 2023

View reviewed changes

spacewander approved these changes Jan 30, 2023

View reviewed changes

soulbird approved these changes Jan 30, 2023

View reviewed changes

spacewander merged commit c8d5afb into apache:master Jan 30, 2023

shreemaan-abhishek deleted the fix#8424 branch February 23, 2023 15:44

membphis mentioned this pull request May 12, 2026

docs: CHANGELOG audit findings for v3.x — completeness, wording, and process gaps #13359

Open

Uh oh!

Conversation

shreemaan-abhishek commented Jan 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

soulbird Jan 12, 2023

Choose a reason for hiding this comment

Uh oh!

soulbird Jan 12, 2023

Choose a reason for hiding this comment

Uh oh!

soulbird commented Jan 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soulbird commented Jan 12, 2023

Uh oh!

Uh oh!

shreemaan-abhishek commented Jan 16, 2023

Uh oh!

soulbird commented Jan 17, 2023

Uh oh!

soulbird commented Jan 17, 2023

Uh oh!

soulbird commented Jan 17, 2023

Uh oh!

spacewander Jan 18, 2023

Choose a reason for hiding this comment

Uh oh!

shreemaan-abhishek Jan 18, 2023 • 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!

spacewander Jan 28, 2023

Choose a reason for hiding this comment

Uh oh!

shreemaan-abhishek Jan 29, 2023

Choose a reason for hiding this comment

Uh oh!

spacewander left a comment

Choose a reason for hiding this comment

Uh oh!

shreemaan-abhishek Jan 29, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

shreemaan-abhishek commented Jan 12, 2023 •

edited

Loading

soulbird commented Jan 12, 2023 •

edited

Loading

shreemaan-abhishek Jan 18, 2023 •

edited

Loading