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

change: unify the keyring and key_encrypt_salt fields, remove ssl:key_encrypt_salt #10771

Merged
merged 8 commits into from Jan 25, 2024

Conversation

RitterHou
Copy link
Contributor

@RitterHou RitterHou commented Jan 7, 2024

Description

This is a fix for issue 10484, the field ssl:key_encrypt_salt has been removed, all the encrypt and decrypt operations are using data_encryption:keyring field now.

key_encrypt_salt and keyring are all used for data encrypt and decrypt, but in fact the are doing the same thing, this change remove key_encrypt_salt only field keyring need to be reserved.

Fixes #10484

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

…ryption:keyring

Signed-off-by: derobukal <derobukal@gmail.com>
@monkeyDluffy6017
Copy link
Contributor

Thanks for contribution, we will check it later

@monkeyDluffy6017
Copy link
Contributor

I think you should update the related test cases too

@@ -132,7 +122,7 @@ apisix:
enable: false
Copy link
Contributor

Choose a reason for hiding this comment

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

we should put this attribute enable under the keyring, because it's only for encrypt_fields in plugin schma, and change a name, modify the comments

Copy link
Contributor Author

@RitterHou RitterHou Jan 9, 2024

Choose a reason for hiding this comment

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

how about changing enable to enable_encrypt_fields and leave it still at this place, because it's undering data_encryption and the attribute name has shown its function

@monkeyDluffy6017 monkeyDluffy6017 added the wait for update wait for the author's response in this issue/PR label Jan 9, 2024
@RitterHou
Copy link
Contributor Author

I think you should update the related test cases too

i will add them

Signed-off-by: derobukal <derobukal@gmail.com>
@RitterHou
Copy link
Contributor Author

I have updated the related test cases and fixed some problems @monkeyDluffy6017

apisix/ssl.lua Outdated
return origin
end

if not field and core.string.has_prefix(origin, "---") then
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need the field parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reserved this parameter because ssl encryption is need to check whether the origin has a --- prefix and data encryption is not. If I remove it, is the prefix check of origin should be also removed too? or just reserve the check and put it to ssl and data encryption?

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer the later one: reserve the check and put it to ssl and data encryption, the data encryption don't have a prefix like ---, so no need to consider the data encryption

@monkeyDluffy6017
Copy link
Contributor

please make the ci pass

Signed-off-by: derobukal <derobukal@gmail.com>
Signed-off-by: derobukal <derobukal@gmail.com>
Signed-off-by: derobukal <derobukal@gmail.com>
Signed-off-by: derobukal <derobukal@gmail.com>
Signed-off-by: derobukal <derobukal@gmail.com>
@RitterHou
Copy link
Contributor Author

Hi @monkeyDluffy6017, I.am feeling confused on how to fix Error: nginx: [error] open() "/home/runner/work/apisix/apisix/logs/nginx.pid" failed (2: No such file or directory) error, I have the same problem in local environment and I manual created the logs directory to fix this error. I am not sure how to fix this to pass the ci, is any prompt for fixing this error? thank you.

@monkeyDluffy6017
Copy link
Contributor

@RitterHou This error log can be ignored: Error: nginx: [error] open() "/home/runner/work/apisix/apisix/logs/nginx.pid" failed (2: No such file or directory), please pay attention to the other errors

@RitterHou
Copy link
Contributor Author

@monkeyDluffy6017 I switched the branch to master and tested t/router/radixtree-sni2.t, the tests on the master branch still have errors. I am stuck fixing the test error now, can you give me some advice to fix the errors? I would be very grateful. Thank you.

@monkeyDluffy6017
Copy link
Contributor

@RitterHou I think the error is imported by your modifications
image

@RitterHou
Copy link
Contributor Author

RitterHou commented Jan 22, 2024

I have found the reason for this error, the SSL key encryption must check whether the key is started with ---, if the key is not started with ---, the encryption will not work and the origin SSL key will be returned. However, the data encryption doesn't have this limit. If I add the check of the prefix, the SSL key check will be ok, but the data encryption will never work because it doesn't have a --- prefix. But if I remove the check, the SSL key encryption will have the error I mentioned before.

Can you help me to solve this problem? @monkeyDluffy6017 I am not sure whether I can remove the field parameter because the logic for SSL key and data encryptions are different.

@RitterHou I think the error is imported by your modifications image

@monkeyDluffy6017
Copy link
Contributor

Maybe we should add the field back: b6501cd

Signed-off-by: derobukal <derobukal@gmail.com>
@monkeyDluffy6017 monkeyDluffy6017 removed the wait for update wait for the author's response in this issue/PR label Jan 24, 2024
@monkeyDluffy6017 monkeyDluffy6017 merged commit 75cff5e into apache:master Jan 25, 2024
44 checks passed
@RitterHou RitterHou deleted the issue-10484 branch January 25, 2024 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: As a user, I want to unify the keyring and key_encrypt_salt fields
3 participants