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: data encryption support more plugins #8487

Merged
merged 8 commits into from
Dec 19, 2022

Conversation

tzssangglass
Copy link
Member

Description

Fixes #8407

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)

@tzssangglass tzssangglass marked this pull request as ready for review December 9, 2022 02:32
@soulbird
Copy link
Contributor

soulbird commented Dec 9, 2022

It is best to update the documentation of each plugin to indicate the fields that will be encrypted.

@tzssangglass
Copy link
Member Author

It is best to update the documentation of each plugin to indicate the fields that will be encrypted.

done

apisix/plugin.lua Outdated Show resolved Hide resolved
docs/en/latest/plugin-develop.md Outdated Show resolved Hide resolved
docs/zh/latest/plugins/openid-connect.md Outdated Show resolved Hide resolved
docs/en/latest/plugins/openid-connect.md Outdated Show resolved Hide resolved
return
end

-- we only support two levels
Copy link
Contributor

@monkeyDluffy6017 monkeyDluffy6017 Dec 13, 2022

Choose a reason for hiding this comment

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

too many limitations, maybe we should optimize this feature later when we have time

Copy link
Contributor

Choose a reason for hiding this comment

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

Why add this limitation, can we parse the conf recursively

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd like to optimize when the schema must be expressed in more than two levels.
For now, I think two levels are enough(In fact, I hate nesting more than two levels in a schema, it's unreadable.)

@@ -41,6 +41,8 @@ This Plugin considers the `GET`, `HEAD` and `OPTIONS` methods to be safe operati
| expires | number | False | `7200` | Expiration time in seconds of the CSRF cookie. Set to `0` to skip checking expiration time. |
| key | string | True | | Secret key used to encrypt the cookie. |

NOTE: `encrypt_fields = {"key"}` is also defined in the schema, which means that the field will be stored encrypted in etcd. See [encrypted storage fields](../plugin-develop.md#encrypted-storage-fields).
Copy link
Contributor

Choose a reason for hiding this comment

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

The field will be encrypted and stored in etcd ?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, what is stored in etcd is the encrypted

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the sentence better The field will be encrypted and stored in etcd

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess there is no difference? For this field, the only encrypted state is in etcd. They are decrypted when APISIX is running and when fetched via the admin API.
I think the way I wrote it better conveys the idea that it is only encrypted in etcd.

soulbird
soulbird previously approved these changes Dec 14, 2022
apisix/plugin.lua Outdated Show resolved Hide resolved
docs/en/latest/plugins/key-auth.md Show resolved Hide resolved
docs/zh/latest/plugins/key-auth.md Show resolved Hide resolved
@spacewander spacewander merged commit 152ea80 into apache:master Dec 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: APISIX Supports Global Data Encryption
4 participants