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: As a user, I want to protect the sensitive information of config.yaml , so that move the data to the Vault #9915

Closed
Sn0rt opened this issue Jul 27, 2023 · 18 comments · Fixed by #10127
Assignees

Comments

@Sn0rt
Copy link
Contributor

Sn0rt commented Jul 27, 2023

this part of #8319

Description

sensitive information in config.yaml supports vault (need to implement)

Through analysis, the sensitive information in config.yaml that meets the definition of secret is:

  1. Admin Key
  2. etcd password
  3. etcd certificate
  4. Admin API certificate
  5. information that may exist in the plugin_attr

The above-mentioned sensitive information needs to be designed and implemented according to priority, and stored in the vault.

Original configuration method

The admin key and etcd password are configured in clear text, and the certificate is the path

deployment:
  admin:
    admin_key:
      - name: admin
        key: edd1c9f034335f136f87ad84b625c8f1
        role: admin
      - name: viewer
        key: 4054f7cf07e344346cd3f287985e76a2
        role: viewer
    admin_api_mtls:
      admin_ssl_cert: ""
      admin_ssl_cert_key: ""
      admin_ssl_ca_cert: ""
  etcd:
    password: 5tHkHhYkjr6cQY
    tls:
      cert: /path/to/cert
      key: /path/to/key

Among them, in the plugin_attr of config-default, there is no special need to be stored in vault for the time being. If there is, add it later.

Add configuration items
An example is as follows:

deployment:
    secret_vault:
        enable: true
        host: vault.vault:8200
        prefix: "kv/apisix"
        token: "${{VAULT_TOKEN}}"

When deployment.secret_vault .enable is true, APISIX will allow values in the form $secret://$secret_name/$key :

  1. Key value under deployment.admin.admin_key
  2. Three certificate entries under deployment.admin.admin_api_mtls
  3. deployment.etcd.password
  4. Two certificate entries under deployment.etcd.tls

The vault token is taken from the environment variables.

Example configuration:

deployment:
  admin:
    admin_key:
      - name: admin
        key: "$secret://apisix_config/admin_key"
        role: admin
      - name: viewer
        key: "$secret://apisix_config/viewer_key"
        role: viewer
    admin_api_mtls:
      admin_ssl_cert: "$secret://apisix_config/admin_ssl_cert"
      admin_ssl_cert_key: "$secret://apisix_config/admin_ssl_cert_key"
      admin_ssl_ca_cert: "$secret://apisix_config/admin_ssl_ca_cert"
  etcd:
    password: "$secret://apisix_config/etcd_password"
    tls:
      cert: "$secret://apisix_config/etcd_cert"
      key: "$secret://apisix_config/etcd_cert_key"

When deployment.secret_vault .enable is not true, the original string is kept.

If the return value of Vault is empty (the key does not exist), log and keep the original string.

@nfrankel
Copy link
Contributor

I think we need to be a bit more general here.

  1. It's not only config.yaml but also apisix.yaml for standalone-mode
  2. It's not only Vault but it could be environment variables

@Sn0rt Sn0rt changed the title feat: As a user, I want to protect the sensitive information of config.yaml , so that move the data to the Vault feat: As a user, I want to protect the sensitive information of config.yaml , so that move the data to the Vault Jul 27, 2023
@rodman10
Copy link
Contributor

Hi, I'd like to work on this. Is this feature works like the following?

  • read vault config from secret_vault.
  • before render the config, get the config from the vault with the conf from secret_vault.
  • then replace the fields in sys_conf according to the response.
  • finally render the conf with the new sys_conf.

@Sn0rt
Copy link
Contributor Author

Sn0rt commented Jul 28, 2023

  • sys_conf

Great! thank you help!

sorry . I misunderstand what's is sys_conf ? if want take over this task . can you confirm the tech design with me again ?

read vault config from secret_vault.

correcty. before start the process . the value of secret store in vault.

before render the config, get the config from the vault with the conf from secret_vault.

yep.

then replace the fields in sys_conf according to the response.

replace the refence with a value which query from vault.

finally render the conf with the new sys_conf.

yep. but i not very understand what's is sys_conf. btw the way , the finally config file only render in memory. no modify the file in disk.

@rodman10
Copy link
Contributor

rodman10 commented Jul 28, 2023

@Sn0rt Sorry, my bad, I means the variable name here https://github.com/apache/apisix/blob/master/apisix/cli/ops.lua#L741

@Sn0rt
Copy link
Contributor Author

Sn0rt commented Jul 28, 2023

replace the refence with a value which query from vault.

It looks like this. In this function, make a judgment whether the vault is used. If so, you need to take the value from the vault to render the actual configuration file.

But you can write the test file first, we are confirming?

@rodman10
Copy link
Contributor

Ok, I will try it. Whether should I open a pr with test file only or just paste the test code snippet here? @Sn0rt

@Sn0rt
Copy link
Contributor Author

Sn0rt commented Jul 28, 2023

Ok, I will try it. Whether should I open a pr with test file only or just paste the test code snippet here? @Sn0rt

thank you . it's very useful for APISIX.

@rodman10
Copy link
Contributor

rodman10 commented Jul 30, 2023

@Sn0rt Please check this, I have just supported api-key field basically. And I have two questions:

  • Should we use the rule $secret://$manager/$id/$secret_name/$key like existed secret?
  • Whether fetch from vault every time or store in cache locally?

@Sn0rt
Copy link
Contributor Author

Sn0rt commented Jul 31, 2023

@Sn0rt Please check this, I have just supported api-key field basically. And I have two questions:

  • Should we use the rule $secret://$manager/$id/$secret_name/$key like existed secret?
  • Whether fetch from vault every time or store in cache locally?

it's look good to me.
not need cache.

@rodman10
Copy link
Contributor

rodman10 commented Jul 31, 2023

  • Should we use the rule $secret://$manager/$id/$secret_name/$key like existed secret?

What about this? For ssl, we will add a new handler for admin which manually fetch from vault then set ctx without reusing the existed radix_sni ? @Sn0rt

@Sn0rt
Copy link
Contributor Author

Sn0rt commented Jul 31, 2023

  • Whether fetch from vault every time or store in cache locally?

This is not a secret, so it does not need to be stored according to the previous scheme.
It is enough to store it in the vault with config as a prefix.

@rodman10
Copy link
Contributor

rodman10 commented Aug 4, 2023

@Sn0rt Hi, I have implemented admin relative secret vault, but when trying to move etcd secret into vault, I met some problems, it seems that ssl infos are all used as path, no direct way to use text fetched from vault. Whether I misunderstood the feature requirement?

@Sn0rt
Copy link
Contributor Author

Sn0rt commented Aug 4, 2023

@Sn0rt Hi, I have implemented admin relative secret vault, but when trying to move etcd secret into vault, I met some problems, it seems that ssl infos are all used as path, no direct way to use text fetched from vault. Whether I misunderstood the feature requirement?

I understand what you mean.

  1. The original configuration is the file content of the read path
  2. Now it has changed to read the valut path.

My idea is that the implementation can be distinguished according to the prefix difference, because the semantics are consistent to read the resources of the specified path.

@rodman10
Copy link
Contributor

rodman10 commented Aug 4, 2023

My idea is that the implementation can be distinguished according to the prefix difference, because the semantics are consistent to read the resources of the specified path.

I have looked into resty.etcd and found options only support read ssl info from disk (maybe I miss something), and the following code also use file path as parameters:
https://github.com/apache/apisix/blob/master/apisix/core/etcd.lua#L65-L68
I think making read behaviour consistent need to modify the deps ?

@Sn0rt
Copy link
Contributor Author

Sn0rt commented Aug 4, 2023

Indeed, I've seen that etcd's underlying library only supports pass the path of file.

Then I think the tasks of the current issues can be divided into multiple stages to achieve.

  1. Submit the part except etcd first.
  2. Discuss the path of etcd tls in one issue after another. (There are not many tls configurations in practice, and I think the priority of this task is a bit low.

@rodman10
Copy link
Contributor

rodman10 commented Aug 4, 2023

Indeed, I've seen that etcd's underlying library only supports pass the path of file.

Then I think the tasks of the current issues can be divided into multiple stages to achieve.

  1. Submit the part except etcd first.
  2. Discuss the path of etcd tls in one issue after another. (There are not many tls configurations in practice, and I think the priority of this task is a bit low.

Ok, I will add more tests and finish in these days :).

@Sn0rt
Copy link
Contributor Author

Sn0rt commented Aug 4, 2023

Indeed, I've seen that etcd's underlying library only supports pass the path of file.
Then I think the tasks of the current issues can be divided into multiple stages to achieve.

  1. Submit the part except etcd first.
  2. Discuss the path of etcd tls in one issue after another. (There are not many tls configurations in practice, and I think the priority of this task is a bit low.

Ok, I will add more tests and finish in these days :).

thx your help. and we need create a new issues for track the etcd tls feature.

@Sn0rt
Copy link
Contributor Author

Sn0rt commented Aug 23, 2023

Indeed, I've seen that etcd's underlying library only supports pass the path of file.
Then I think the tasks of the current issues can be divided into multiple stages to achieve.

  1. Submit the part except etcd first.
  2. Discuss the path of etcd tls in one issue after another. (There are not many tls configurations in practice, and I think the priority of this task is a bit low.

Ok, I will add more tests and finish in these days :).

thx your help. and we need create a new issues for track the etcd tls feature.

I will create another PR for the below feature

  etcd:
    password: "$secret://apisix_config/etcd_password"
    tls:
      cert: "$secret://apisix_config/etcd_cert"
      key: "$secret://apisix_config/etcd_cert_key"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
3 participants