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: allow admin key and admin api cert to be stored in vault #9930

Closed
wants to merge 11 commits into from

Conversation

rodman10
Copy link
Contributor

Description

Fixes #9915

Checklist

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

@rodman10 rodman10 force-pushed the vault branch 2 times, most recently from 62e6bbe to 09644e4 Compare July 30, 2023 11:27
@Sn0rt
Copy link
Contributor

Sn0rt commented Jul 31, 2023

the TEST case look good

@rodman10 rodman10 force-pushed the vault branch 3 times, most recently from 28f663e to adb98f2 Compare August 2, 2023 07:52
@Sn0rt
Copy link
Contributor

Sn0rt commented Aug 4, 2023

If you think you can enter the review, or you want to run a CI test first. You can remove the Draft label.

Generally, I will conduct code review after CI is completed.

@rodman10 rodman10 marked this pull request as ready for review August 6, 2023 08:26
@Sn0rt
Copy link
Contributor

Sn0rt commented Aug 10, 2023

@rodman10 are you still process this PR ?

@rodman10
Copy link
Contributor Author

I have removed the draft state and ready for review.@Sn0rt

@monkeyDluffy6017 monkeyDluffy6017 added the checking check first if this issue occurred label Aug 14, 2023
@Sn0rt
Copy link
Contributor

Sn0rt commented Aug 14, 2023

I have removed the draft state and ready for review.@Sn0rt

PTAL the CI ?

@rodman10
Copy link
Contributor Author

I have removed the draft state and ready for review.@Sn0rt

PTAL the CI ?

Ok, I will check it.

@rodman10
Copy link
Contributor Author

I have removed the draft state and ready for review.@Sn0rt

PTAL the CI ?

I have looked into the centos7 case,

企业微信截图_16920688186997

It seems failed in case t/deployment/conf_server.t, but I can't reproduce the problem with cmd prove -I../test-nginx/lib -I./ -r -s t/deployment/conf_server.t, whether I have mistaken the test method @Sn0rt ?

企业微信截图_16920685157714

@Sn0rt
Copy link
Contributor

Sn0rt commented Aug 16, 2023

@rodman10 Don't worry, sometimes ci is unstable. Re-run CI to try.

@Sn0rt
Copy link
Contributor

Sn0rt commented Aug 17, 2023

@rodman10

PTAL the CI https://github.com/apache/apisix/actions/runs/5856541181/job/15966889114?pr=9930

+ VAULT_TOKEN=root
+ VAULT_ADDR=http://0.0.0.0:8200/
+ vault kv put kv/apisix/apisix_config admin_ssl_cert=@./t/certs/mtls_server.crt admin_ssl_cert_key=@./t/certs/mtls_server.key admin_ssl_ca_cert=@./t/certs/mtls_ca.crt
./t/cli/test_admin_mtls.sh: line 76: vault: command not found

@rodman10
Copy link
Contributor Author

@rodman10

PTAL the CI https://github.com/apache/apisix/actions/runs/5856541181/job/15966889114?pr=9930

+ VAULT_TOKEN=root
+ VAULT_ADDR=http://0.0.0.0:8200/
+ vault kv put kv/apisix/apisix_config admin_ssl_cert=@./t/certs/mtls_server.crt admin_ssl_cert_key=@./t/certs/mtls_server.key admin_ssl_ca_cert=@./t/certs/mtls_ca.crt
./t/cli/test_admin_mtls.sh: line 76: vault: command not found

I have add vault dependency in ci/linux_apisix_current_luarocks_runner.sh. @Sn0rt

@Sn0rt
Copy link
Contributor

Sn0rt commented Aug 18, 2023

Can I cooperate? Give me permission

@rodman10
Copy link
Contributor Author

Can I cooperate? Give me permission

Ok.

fi

# skip
code=$(curl -i -o /dev/null -s -w %{http_code} -k -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' https://admin.apisix.dev:9180/apisix/admin/routes)
Copy link
Contributor

Choose a reason for hiding this comment

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

this test case can't pass

token: "${{VAULT_TOKEN}}"
' > conf/config.yaml

VAULT_TOKEN='root' VAULT_ADDR='http://0.0.0.0:8200' vault kv put kv/apisix/apisix_config admin_ssl_cert=@./t/certs/mtls_server.crt admin_ssl_cert_key=@./t/certs/mtls_server.key admin_ssl_ca_cert=@./t/certs/mtls_ca.crt
Copy link
Contributor

Choose a reason for hiding this comment

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

diff --git a/.github/workflows/cli.yml b/.github/workflows/cli.yml
index 7aa65540..4cde0dfe 100644
--- a/.github/workflows/cli.yml
+++ b/.github/workflows/cli.yml
@@ -53,6 +53,7 @@ jobs:
       - name: Linux launch common services
         run: |
           project_compose_ci=ci/pod/docker-compose.common.yml make ci-env-up
+          sudo ./ci/init-common-test-service.sh

@kingluo
Copy link
Contributor

kingluo commented Aug 22, 2023

The code is fine with me.

But I think it should be noted in the PR description which configuration fields support encryption:

  • Admin Key
  • Admin API certificate
  • etcd password
  • etcd certificate
  • information that may exist in the plugin_attr

Also, whether in this PR or not, the relevant documentation should also describe this change.

@Sn0rt
Copy link
Contributor

Sn0rt commented Aug 23, 2023

The code is fine with me.

But I think it should be noted in the PR description which configuration fields support encryption:

  • Admin Key
  • Admin API certificate
  • etcd password
  • etcd certificate
  • information that may exist in the plugin_attr

Also, whether in this PR or not, the relevant documentation should also describe this change.

yep. Because our underlying library can only use the path to pass in the certificate, so avoid the PR being too large. We will support this feature in other PRs.

api7/lua-resty-etcd#208

@kingluo kingluo changed the title feat: move sensitive information into vault. feat: allow admin key and admin api cert to be stored in vault Aug 23, 2023
@Sn0rt
Copy link
Contributor

Sn0rt commented Sep 19, 2023

I plan to close this PR and continue to improve this feature based on your commit and new proposal.
The new PR will contain complete information about your commit.
Do you have any suggestions?

@rodman10

@rodman10
Copy link
Contributor Author

I plan to close this PR and continue to improve this feature based on your commit and new proposal. The new PR will contain complete information about your commit. Do you have any suggestions?

@rodman10

Ok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
checking check first if this issue occurred
Projects
None yet
4 participants