Skip to content

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

Merged
juststillthinking merged 8 commits into
apache:masterfrom
RitterHou:issue-10484
Jan 25, 2024
Merged

change: unify the keyring and key_encrypt_salt fields, remove ssl:key_encrypt_salt#10771
juststillthinking merged 8 commits into
apache:masterfrom
RitterHou:issue-10484

Conversation

@RitterHou

@RitterHou RitterHou commented Jan 7, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

@juststillthinking

juststillthinking commented Jan 8, 2024

Copy link
Copy Markdown
Contributor

Thanks for contribution, we will check it later

@juststillthinking

juststillthinking commented Jan 9, 2024

Copy link
Copy Markdown
Contributor

I think you should update the related test cases too

juststillthinking
juststillthinking reviewed Jan 9, 2024
View reviewed changes
Comment thread conf/config-default.yaml Outdated
@@ -132,7 +122,7 @@ apisix:
enable: false

@juststillthinking juststillthinking Jan 9, 2024

Copy link
Copy Markdown
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

@RitterHou RitterHou Jan 9, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

@juststillthinking juststillthinking added the wait for update wait for the author's response in this issue/PR label Jan 9, 2024
@RitterHou

RitterHou commented Jan 9, 2024

Copy link
Copy Markdown
Contributor Author

I think you should update the related test cases too

i will add them

@RitterHou

RitterHou commented Jan 14, 2024

Copy link
Copy Markdown
Contributor Author

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

juststillthinking
juststillthinking reviewed Jan 15, 2024
View reviewed changes
Comment thread apisix/ssl.lua Outdated
return origin
end

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

@juststillthinking juststillthinking Jan 15, 2024

Copy link
Copy Markdown
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?

@RitterHou RitterHou Jan 15, 2024

Copy link
Copy Markdown
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?

@juststillthinking juststillthinking Jan 16, 2024

Copy link
Copy Markdown
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

@juststillthinking

juststillthinking commented Jan 16, 2024

Copy link
Copy Markdown
Contributor

please make the ci pass

@RitterHou

RitterHou commented Jan 18, 2024

Copy link
Copy Markdown
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.

@juststillthinking

juststillthinking commented Jan 18, 2024

Copy link
Copy Markdown
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

RitterHou commented Jan 18, 2024

Copy link
Copy Markdown
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.

@juststillthinking

juststillthinking commented Jan 19, 2024

Copy link
Copy Markdown
Contributor

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

@RitterHou

RitterHou commented Jan 22, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

@juststillthinking

juststillthinking commented Jan 23, 2024

Copy link
Copy Markdown
Contributor

Maybe we should add the field back: b6501cd

@RitterHou
add parameter field back
7f27eb2 
Signed-off-by: derobukal <[email protected]>
@juststillthinking juststillthinking removed the wait for update wait for the author's response in this issue/PR label Jan 24, 2024
@juststillthinking
juststillthinking merged commit 75cff5e into apache:master Jan 25, 2024
@RitterHou
RitterHou deleted the issue-10484 branch January 25, 2024 08:26
@AlinsRan AlinsRan mentioned this pull request Jun 3, 2024
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shreemaan-abhishek shreemaan-abhishek shreemaan-abhishek approved these changes
@moonming moonming Awaiting requested review from moonming
+1 more reviewer
@juststillthinking juststillthinking juststillthinking approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

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

@RitterHou @juststillthinking @shreemaan-abhishek