You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2023/06/20 22:30:25 UTC
[kudu-CR] [util] fix at-rest {en,de}cryption for OpenSSL 3
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/20100
Change subject: [util] fix at-rest {en,de}cryption for OpenSSL 3
......................................................................
[util] fix at-rest {en,de}cryption for OpenSSL 3
It seems that with OpenSSL 3.0 padding is enabled by default [1].
However, the code in env-posix.cc was relying on an incorrect assumption
that the padding would be always disabled for AES-CTR ciphers, and there
were no calls to EVP_{En,De}cryptFinal_ex() in Do{En,De}cryptV()
correspondingly.
This patch addresses the issue, disabling padding for the cipher
context explicitly. I also updated the order of the expected/actual
arguments for ASSERT_EQ() because the error message was a bit hard
to read and comprehend if the corresponding test failed.
The motivation for this patch were reports on failed env-test if built
and run on RHEL9.
Probably, adding calls to EVP_{En,De}cryptFinal_ex() into
Do{En,De}cryptV() and making sure they return nothing would be a good
invariant to check in DEBUG builds. I added corresponding TODOs in the
code since it's better to address that in a separate patch.
This is a follow-up to de02a34390f9bdb722be39f96ef9ad9573c0eeeb.
[1] https://www.openssl.org/docs/man3.0/man3/EVP_EncryptUpdate.html
Change-Id: Ia49a51488a0a39953249f24598daa8219583d575
---
M src/kudu/util/env-test.cc
M src/kudu/util/env_posix.cc
2 files changed, 22 insertions(+), 12 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/00/20100/1
--
To view, visit http://gerrit.cloudera.org:8080/20100
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia49a51488a0a39953249f24598daa8219583d575
Gerrit-Change-Number: 20100
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <al...@apache.org>
[kudu-CR] [util] fix at-rest {en,de}cryption for OpenSSL 3
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.
Change subject: [util] fix at-rest {en,de}cryption for OpenSSL 3
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/20100
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Ia49a51488a0a39953249f24598daa8219583d575
Gerrit-Change-Number: 20100
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
[kudu-CR] [util] fix at-rest {en,de}cryption for OpenSSL 3
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20100 )
Change subject: [util] fix at-rest {en,de}cryption for OpenSSL 3
......................................................................
Patch Set 1: Verified+1
unrelated test failure in ReplicatedAlterTableTest.AlterReplicationFactorWhileWriting
--
To view, visit http://gerrit.cloudera.org:8080/20100
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia49a51488a0a39953249f24598daa8219583d575
Gerrit-Change-Number: 20100
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Comment-Date: Tue, 20 Jun 2023 23:25:01 +0000
Gerrit-HasComments: No
[kudu-CR] [util] fix at-rest {en,de}cryption for OpenSSL 3
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/20100 )
Change subject: [util] fix at-rest {en,de}cryption for OpenSSL 3
......................................................................
Patch Set 1: Code-Review+2
Nice catch! Funnily enough, I recall adding these checks for *Final_ex() originally, but then I decided against committing it, because I thought there was no way these checks could ever be useful with CTR mode of operation. Lesson learned, I guess.
--
To view, visit http://gerrit.cloudera.org:8080/20100
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia49a51488a0a39953249f24598daa8219583d575
Gerrit-Change-Number: 20100
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Comment-Date: Tue, 20 Jun 2023 22:51:32 +0000
Gerrit-HasComments: No
[kudu-CR] [util] fix at-rest {en,de}cryption for OpenSSL 3
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/20100 )
Change subject: [util] fix at-rest {en,de}cryption for OpenSSL 3
......................................................................
[util] fix at-rest {en,de}cryption for OpenSSL 3
It seems that with OpenSSL 3.0 padding is enabled by default [1].
However, the code in env-posix.cc was relying on an incorrect assumption
that the padding would be always disabled for AES-CTR ciphers, and there
were no calls to EVP_{En,De}cryptFinal_ex() in Do{En,De}cryptV()
correspondingly.
This patch addresses the issue, disabling padding for the cipher
context explicitly. I also updated the order of the expected/actual
arguments for ASSERT_EQ() because the error message was a bit hard
to read and comprehend if the corresponding test failed.
The motivation for this patch were reports on failed env-test if built
and run on RHEL9.
Probably, adding calls to EVP_{En,De}cryptFinal_ex() into
Do{En,De}cryptV() and making sure they return nothing would be a good
invariant to check in DEBUG builds. I added corresponding TODOs in the
code since it's better to address that in a separate patch.
This is a follow-up to de02a34390f9bdb722be39f96ef9ad9573c0eeeb.
[1] https://www.openssl.org/docs/man3.0/man3/EVP_EncryptUpdate.html
Change-Id: Ia49a51488a0a39953249f24598daa8219583d575
Reviewed-on: http://gerrit.cloudera.org:8080/20100
Reviewed-by: Attila Bukor <ab...@apache.org>
Tested-by: Alexey Serbin <al...@apache.org>
---
M src/kudu/util/env-test.cc
M src/kudu/util/env_posix.cc
2 files changed, 22 insertions(+), 12 deletions(-)
Approvals:
Attila Bukor: Looks good to me, approved
Alexey Serbin: Verified
--
To view, visit http://gerrit.cloudera.org:8080/20100
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ia49a51488a0a39953249f24598daa8219583d575
Gerrit-Change-Number: 20100
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>