You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Joe McDonnell (Code Review)" <ge...@cloudera.org> on 2025/05/16 21:43:09 UTC
[kudu-CR] PROTOTYPE: Support certificates signed with RSASSA-PSS for channel bindings
Joe McDonnell has uploaded this change for review. ( http://gerrit.cloudera.org:8080/22910
Change subject: PROTOTYPE: Support certificates signed with RSASSA-PSS for channel bindings
......................................................................
PROTOTYPE: Support certificates signed with RSASSA-PSS for channel bindings
The existing code to determine the hash algorithm for a certificate
does not handle RSASSA-PSS signatures as the hash algorithm is
configurable for RSASSA-PSS. OpenSSL 1.1.1 introduced the
x509_get_signature_info() function, which is able to determine the
hash algorithm even for RSASSA-PSS. This uses x509_get_signature_info()
whenever building against OpenSSL 1.1.1 or above. This is similar to
the fix used in Postgres when faced with the same issue.
Testing:
- Added a test certificate that uses RSASSA-PSS and a test case in
cert-test to verify that it can determine the hash algorithm used
Change-Id: I26a25a43d778fd2f2fcf293ecb199133c675212b
---
M src/kudu/security/cert-test.cc
M src/kudu/security/cert.cc
M src/kudu/security/cert.h
M src/kudu/security/test/test_certs.cc
M src/kudu/security/test/test_certs.h
5 files changed, 123 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/10/22910/1
--
To view, visit http://gerrit.cloudera.org:8080/22910
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I26a25a43d778fd2f2fcf293ecb199133c675212b
Gerrit-Change-Number: 22910
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
[kudu-CR] PROTOTYPE: Support certificates signed with RSASSA-PSS for channel bindings
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22910 )
Change subject: PROTOTYPE: Support certificates signed with RSASSA-PSS for channel bindings
......................................................................
Patch Set 2:
Build Started http://jenkins.kudu.apache.org/job/pre_commit/1377/
--
To view, visit http://gerrit.cloudera.org:8080/22910
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I26a25a43d778fd2f2fcf293ecb199133c675212b
Gerrit-Change-Number: 22910
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 16 May 2025 22:05:00 +0000
Gerrit-HasComments: No
[kudu-CR] PROTOTYPE: Support certificates signed with RSASSA-PSS for channel bindings
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22910 )
Change subject: PROTOTYPE: Support certificates signed with RSASSA-PSS for channel bindings
......................................................................
Patch Set 2: Code-Review+1
(6 comments)
http://gerrit.cloudera.org:8080/#/c/22910/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/22910/2//COMMIT_MSG@7
PS2, Line 7: PROTOTYPE
> This should be IMPALA-14038
I guess Joe was going to file a Kudu upstream JIRA for this as he mentioned in his comment for PS1.
http://gerrit.cloudera.org:8080/#/c/22910/2/src/kudu/security/cert-test.cc
File src/kudu/security/cert-test.cc:
http://gerrit.cloudera.org:8080/#/c/22910/2/src/kudu/security/cert-test.cc@20
PS2, Line 20: #include <openssl/crypto.h>
> Is this include needed?
Yes, it is: https://jenkins.kudu.apache.org/job/build_and_test/8600/console
http://gerrit.cloudera.org:8080/#/c/22910/2/src/kudu/security/cert.h
File src/kudu/security/cert.h:
http://gerrit.cloudera.org:8080/#/c/22910/2/src/kudu/security/cert.h@79
PS2, Line 79: WARN_UNUSED_RESULT
nit: in Kudu this is no longer needed in 1.18 and newer codebase since the 'Status' class now has [[nodiscard]] attribute and any non-handled Status results in compiler error
http://gerrit.cloudera.org:8080/#/c/22910/2/src/kudu/security/cert.cc
File src/kudu/security/cert.cc:
http://gerrit.cloudera.org:8080/#/c/22910/2/src/kudu/security/cert.cc@28
PS2, Line 28: #include <cstddef>
> Is this include needed?
Yes, it is: https://jenkins.kudu.apache.org/job/build_and_test/8600/console
http://gerrit.cloudera.org:8080/#/c/22910/2/src/kudu/security/cert.cc@198
PS2, Line 198: NULL
nit for here and below: consider changing to 'nullptr' to match the rest of the code in this file
http://gerrit.cloudera.org:8080/#/c/22910/2/src/kudu/security/cert.cc@200
PS2, Line 200: OBJ_find_sigid_algs(signature_nid, &digest_nid, NULL);
> I would like to see a better error message returned here if digest_nid is N
I'm not sure it's worth it trying to add such better error message:
* IIUC, pre-1.1.1 versions of the OpenSSL library do even have a notion of RSASSA-PSS to tell what's going on
* at least for Kudu, if the bits are built the standard way with pre-1.1.1 OpenSSL library, that means they are for RHEL/CentOS 7.0 or earlier. RHEL/CentOS 7.4 comes with OpenSSL 1.1.1 already. And RHEL/CentOS 7.4 is EOL already.
--
To view, visit http://gerrit.cloudera.org:8080/22910
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I26a25a43d778fd2f2fcf293ecb199133c675212b
Gerrit-Change-Number: 22910
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 19 May 2025 17:42:23 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3663: Support certificates signed with RSASSA-PSS for channel bindings
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/22910 )
Change subject: KUDU-3663: Support certificates signed with RSASSA-PSS for channel bindings
......................................................................
KUDU-3663: Support certificates signed with RSASSA-PSS for channel bindings
The existing code to determine the hash algorithm for a certificate
does not handle RSASSA-PSS signatures as the hash algorithm is
configurable for RSASSA-PSS. OpenSSL 1.1.1 introduced the
x509_get_signature_info() function, which is able to determine the
hash algorithm even for RSASSA-PSS. This uses x509_get_signature_info()
whenever building against OpenSSL 1.1.1 or above. This is similar to
the fix used in Postgres when faced with the same issue.
Testing:
- Added a test certificate that uses RSASSA-PSS and a test case in
cert-test to verify that it can determine the hash algorithm used
Change-Id: I26a25a43d778fd2f2fcf293ecb199133c675212b
Reviewed-on: http://gerrit.cloudera.org:8080/22910
Reviewed-by: Alexey Serbin <al...@apache.org>
Tested-by: Alexey Serbin <al...@apache.org>
---
M src/kudu/security/cert-test.cc
M src/kudu/security/cert.cc
M src/kudu/security/cert.h
M src/kudu/security/test/test_certs.cc
M src/kudu/security/test/test_certs.h
5 files changed, 132 insertions(+), 5 deletions(-)
Approvals:
Alexey Serbin: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/22910
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I26a25a43d778fd2f2fcf293ecb199133c675212b
Gerrit-Change-Number: 22910
Gerrit-PatchSet: 4
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] PROTOTYPE: Support certificates signed with RSASSA-PSS for channel bindings
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22910 )
Change subject: PROTOTYPE: Support certificates signed with RSASSA-PSS for channel bindings
......................................................................
Patch Set 2:
(1 comment)
Thank you for taking care of this!
LGTM overall, just a few nits from my side.
http://gerrit.cloudera.org:8080/#/c/22910/2/src/kudu/security/cert.cc
File src/kudu/security/cert.cc:
http://gerrit.cloudera.org:8080/#/c/22910/2/src/kudu/security/cert.cc@200
PS2, Line 200: OBJ_find_sigid_algs(signature_nid, &digest_nid, NULL);
> I'm not sure it's worth it trying to add such better error message:
A small correction:
* openssl on RHEL7 is originally based on openssl-1.0.1e but was rebased to openssl-1.0.2k with RHEL7.4
* openssl on RHEL8 is originally based on openssl-1.1.1 already
However, my point stays as is: one shouldn't expect to see Kudu bits built with pre-1.1.1 OpenSSL version on non-EOL Linux OS distributions. Kudu bits on EOL Linux distros are getting more and more rare, and since such OSes are EOL, we shouldn't spend time and effort supporting such configurations unless that's some specific contract.
--
To view, visit http://gerrit.cloudera.org:8080/22910
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I26a25a43d778fd2f2fcf293ecb199133c675212b
Gerrit-Change-Number: 22910
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 19 May 2025 17:52:57 +0000
Gerrit-HasComments: Yes
[kudu-CR] PROTOTYPE: Support certificates signed with RSASSA-PSS for channel bindings
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Jason Fehr, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22910
to look at the new patch set (#2).
Change subject: PROTOTYPE: Support certificates signed with RSASSA-PSS for channel bindings
......................................................................
PROTOTYPE: Support certificates signed with RSASSA-PSS for channel bindings
The existing code to determine the hash algorithm for a certificate
does not handle RSASSA-PSS signatures as the hash algorithm is
configurable for RSASSA-PSS. OpenSSL 1.1.1 introduced the
x509_get_signature_info() function, which is able to determine the
hash algorithm even for RSASSA-PSS. This uses x509_get_signature_info()
whenever building against OpenSSL 1.1.1 or above. This is similar to
the fix used in Postgres when faced with the same issue.
Testing:
- Added a test certificate that uses RSASSA-PSS and a test case in
cert-test to verify that it can determine the hash algorithm used
Change-Id: I26a25a43d778fd2f2fcf293ecb199133c675212b
---
M src/kudu/security/cert-test.cc
M src/kudu/security/cert.cc
M src/kudu/security/cert.h
M src/kudu/security/test/test_certs.cc
M src/kudu/security/test/test_certs.h
5 files changed, 125 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/10/22910/2
--
To view, visit http://gerrit.cloudera.org:8080/22910
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I26a25a43d778fd2f2fcf293ecb199133c675212b
Gerrit-Change-Number: 22910
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] PROTOTYPE: Support certificates signed with RSASSA-PSS for channel bindings
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/22910 )
Change subject: PROTOTYPE: Support certificates signed with RSASSA-PSS for channel bindings
......................................................................
Patch Set 2:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/22910/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/22910/2//COMMIT_MSG@7
PS2, Line 7: PROTOTYPE
> I guess Joe was going to file a Kudu upstream JIRA for this as he mentioned
Filed KUDU-3663 and updated this message
http://gerrit.cloudera.org:8080/#/c/22910/2/src/kudu/security/cert.h
File src/kudu/security/cert.h:
http://gerrit.cloudera.org:8080/#/c/22910/2/src/kudu/security/cert.h@79
PS2, Line 79: WARN_UNUSED_RESULT
> nit: in Kudu this is no longer needed in 1.18 and newer codebase since the
If you don't mind, I might leave this here to make it easier to backport.
http://gerrit.cloudera.org:8080/#/c/22910/2/src/kudu/security/cert.cc
File src/kudu/security/cert.cc:
http://gerrit.cloudera.org:8080/#/c/22910/2/src/kudu/security/cert.cc@28
PS2, Line 28: #include <cstddef>
> I wonder what triggered this. Maybe NULL? Let's see what happens when I swi
Switching NULL to nullptr eliminates the need for this, so this is gone.
http://gerrit.cloudera.org:8080/#/c/22910/2/src/kudu/security/cert.cc@198
PS2, Line 198: NULL
> nit for here and below: consider changing to 'nullptr' to match the rest of
Done
http://gerrit.cloudera.org:8080/#/c/22910/2/src/kudu/security/cert.cc@200
PS2, Line 200: OBJ_find_sigid_algs(signature_nid, &digest_nid, NULL);
> Yeah, I think it is possible to implement similar support for OpenSSL 1.0.2
Added a better error message for RSASSA-PSS for this case.
http://gerrit.cloudera.org:8080/#/c/22910/2/src/kudu/security/cert.cc@210
PS2, Line 210: if (digest_nid == NID_undef) {
> Since we are making changed here, we should wrap this if condition with UNL
Added PREDICT_FALSE to this condition (Kudu calls this PREDICT_FALSE rather than UNLIKELY).
http://gerrit.cloudera.org:8080/#/c/22910/2/src/kudu/security/cert.cc@213
PS2, Line 213: "server certificate using $0 has no signature digest (hash) algorithm",
> Nit: this error would be clearer if the string said "server certificate usi
Done
--
To view, visit http://gerrit.cloudera.org:8080/22910
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I26a25a43d778fd2f2fcf293ecb199133c675212b
Gerrit-Change-Number: 22910
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 19 May 2025 19:52:59 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3663: Support certificates signed with RSASSA-PSS for channel bindings
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22910 )
Change subject: KUDU-3663: Support certificates signed with RSASSA-PSS for channel bindings
......................................................................
Patch Set 3:
Build Started http://jenkins.kudu.apache.org/job/pre_commit/1382/
--
To view, visit http://gerrit.cloudera.org:8080/22910
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I26a25a43d778fd2f2fcf293ecb199133c675212b
Gerrit-Change-Number: 22910
Gerrit-PatchSet: 3
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 19 May 2025 19:54:38 +0000
Gerrit-HasComments: No
[kudu-CR] PROTOTYPE: Support certificates signed with RSASSA-PSS for channel bindings
Posted by "Jason Fehr (Code Review)" <ge...@cloudera.org>.
Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/22910 )
Change subject: PROTOTYPE: Support certificates signed with RSASSA-PSS for channel bindings
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/22910/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/22910/2//COMMIT_MSG@7
PS2, Line 7: PROTOTYPE
This should be IMPALA-14038
--
To view, visit http://gerrit.cloudera.org:8080/22910
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I26a25a43d778fd2f2fcf293ecb199133c675212b
Gerrit-Change-Number: 22910
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 19 May 2025 15:27:40 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3663: Support certificates signed with RSASSA-PSS for channel bindings
Posted by "Jason Fehr (Code Review)" <ge...@cloudera.org>.
Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/22910 )
Change subject: KUDU-3663: Support certificates signed with RSASSA-PSS for channel bindings
......................................................................
Patch Set 3:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/22910/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/22910/2//COMMIT_MSG@7
PS2, Line 7: KUDU-3663
> Filed KUDU-3663 and updated this message
Done
http://gerrit.cloudera.org:8080/#/c/22910/2/src/kudu/security/cert.cc
File src/kudu/security/cert.cc:
http://gerrit.cloudera.org:8080/#/c/22910/2/src/kudu/security/cert.cc@28
PS2, Line 28: #include <functional>
> BTW, you could see why IWYU asked for that by running
Done
http://gerrit.cloudera.org:8080/#/c/22910/2/src/kudu/security/cert.cc@200
PS2, Line 200: // Return a better error message for RSASSA-PSS, because we know that
> Added a better error message for RSASSA-PSS for this case.
Done
http://gerrit.cloudera.org:8080/#/c/22910/2/src/kudu/security/cert.cc@210
PS2, Line 210: // uses multiple hash functions, then this channel binding type's channel
> Added PREDICT_FALSE to this condition (Kudu calls this PREDICT_FALSE rather
Done
http://gerrit.cloudera.org:8080/#/c/22910/2/src/kudu/security/cert.cc@213
PS2, Line 213: //
> Done
Done
--
To view, visit http://gerrit.cloudera.org:8080/22910
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I26a25a43d778fd2f2fcf293ecb199133c675212b
Gerrit-Change-Number: 22910
Gerrit-PatchSet: 3
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 19 May 2025 22:36:20 +0000
Gerrit-HasComments: Yes
[kudu-CR] PROTOTYPE: Support certificates signed with RSASSA-PSS for channel bindings
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/22910 )
Change subject: PROTOTYPE: Support certificates signed with RSASSA-PSS for channel bindings
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/22910/2/src/kudu/security/cert.cc
File src/kudu/security/cert.cc:
http://gerrit.cloudera.org:8080/#/c/22910/2/src/kudu/security/cert.cc@200
PS2, Line 200: OBJ_find_sigid_algs(signature_nid, &digest_nid, NULL);
> I just took a look at OpenSSL 1.0.2 source code and it indeed has NID_rsass
Yeah, I think it is possible to implement similar support for OpenSSL 1.0.2 with enough transplanted logic. I avoided doing that, because it was a lot of transplanted logic and OpenSSL 1.0.2 is used by distributions that are EOL or almost EOL. RSASSA-PSS is exotic and the issue that prompted this was on modern OpenSSL.
I'll add a better error message for RSASSA-PSS on OpenSSL 1.0.2.
--
To view, visit http://gerrit.cloudera.org:8080/22910
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I26a25a43d778fd2f2fcf293ecb199133c675212b
Gerrit-Change-Number: 22910
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 19 May 2025 18:09:53 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3663: Support certificates signed with RSASSA-PSS for channel bindings
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Jason Fehr, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22910
to look at the new patch set (#3).
Change subject: KUDU-3663: Support certificates signed with RSASSA-PSS for channel bindings
......................................................................
KUDU-3663: Support certificates signed with RSASSA-PSS for channel bindings
The existing code to determine the hash algorithm for a certificate
does not handle RSASSA-PSS signatures as the hash algorithm is
configurable for RSASSA-PSS. OpenSSL 1.1.1 introduced the
x509_get_signature_info() function, which is able to determine the
hash algorithm even for RSASSA-PSS. This uses x509_get_signature_info()
whenever building against OpenSSL 1.1.1 or above. This is similar to
the fix used in Postgres when faced with the same issue.
Testing:
- Added a test certificate that uses RSASSA-PSS and a test case in
cert-test to verify that it can determine the hash algorithm used
Change-Id: I26a25a43d778fd2f2fcf293ecb199133c675212b
---
M src/kudu/security/cert-test.cc
M src/kudu/security/cert.cc
M src/kudu/security/cert.h
M src/kudu/security/test/test_certs.cc
M src/kudu/security/test/test_certs.h
5 files changed, 132 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/10/22910/3
--
To view, visit http://gerrit.cloudera.org:8080/22910
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I26a25a43d778fd2f2fcf293ecb199133c675212b
Gerrit-Change-Number: 22910
Gerrit-PatchSet: 3
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3663: Support certificates signed with RSASSA-PSS for channel bindings
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22910 )
Change subject: KUDU-3663: Support certificates signed with RSASSA-PSS for channel bindings
......................................................................
Patch Set 3: Verified-1
Build Failed
http://jenkins.kudu.apache.org/job/pre_commit/1382/ : FAILURE
--
To view, visit http://gerrit.cloudera.org:8080/22910
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I26a25a43d778fd2f2fcf293ecb199133c675212b
Gerrit-Change-Number: 22910
Gerrit-PatchSet: 3
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 19 May 2025 21:38:35 +0000
Gerrit-HasComments: No
[kudu-CR] PROTOTYPE: Support certificates signed with RSASSA-PSS for channel bindings
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/22910 )
Change subject: PROTOTYPE: Support certificates signed with RSASSA-PSS for channel bindings
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/22910/2/src/kudu/security/cert-test.cc
File src/kudu/security/cert-test.cc:
http://gerrit.cloudera.org:8080/#/c/22910/2/src/kudu/security/cert-test.cc@20
PS2, Line 20: #include <openssl/crypto.h>
> Yes, it is: https://jenkins.kudu.apache.org/job/build_and_test/8600/console
For Jason: Kudu uses include-what-you-use, which calculates the right set of headers to include and forces code to match that list. It's pretty cool, and something I wish Impala did. In this case, I think it wants crypto.h due to OPENSSL_VERSION_NUMBER.
http://gerrit.cloudera.org:8080/#/c/22910/2/src/kudu/security/cert.cc
File src/kudu/security/cert.cc:
http://gerrit.cloudera.org:8080/#/c/22910/2/src/kudu/security/cert.cc@28
PS2, Line 28: #include <cstddef>
> Yes, it is: https://jenkins.kudu.apache.org/job/build_and_test/8600/console
I wonder what triggered this. Maybe NULL? Let's see what happens when I switch to nullptr.
--
To view, visit http://gerrit.cloudera.org:8080/22910
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I26a25a43d778fd2f2fcf293ecb199133c675212b
Gerrit-Change-Number: 22910
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 19 May 2025 18:58:32 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3663: Support certificates signed with RSASSA-PSS for channel bindings
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/22910 )
Change subject: KUDU-3663: Support certificates signed with RSASSA-PSS for channel bindings
......................................................................
Patch Set 3:
I'm running builds for this on Redhat 7/8/9 just to make sure everything builds with various OpenSSL versions.
--
To view, visit http://gerrit.cloudera.org:8080/22910
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I26a25a43d778fd2f2fcf293ecb199133c675212b
Gerrit-Change-Number: 22910
Gerrit-PatchSet: 3
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 19 May 2025 20:57:41 +0000
Gerrit-HasComments: No
[kudu-CR] PROTOTYPE: Support certificates signed with RSASSA-PSS for channel bindings
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22910 )
Change subject: PROTOTYPE: Support certificates signed with RSASSA-PSS for channel bindings
......................................................................
Patch Set 1: Verified-1
Build Failed
http://jenkins.kudu.apache.org/job/pre_commit/1376/ : FAILURE
--
To view, visit http://gerrit.cloudera.org:8080/22910
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I26a25a43d778fd2f2fcf293ecb199133c675212b
Gerrit-Change-Number: 22910
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 16 May 2025 21:46:03 +0000
Gerrit-HasComments: No
[kudu-CR] PROTOTYPE: Support certificates signed with RSASSA-PSS for channel bindings
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/22910 )
Change subject: PROTOTYPE: Support certificates signed with RSASSA-PSS for channel bindings
......................................................................
Patch Set 1:
This is the Kudu side fix for IMPALA-14038. If this looks good, I'll file a Kudu jira
--
To view, visit http://gerrit.cloudera.org:8080/22910
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I26a25a43d778fd2f2fcf293ecb199133c675212b
Gerrit-Change-Number: 22910
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 16 May 2025 21:51:38 +0000
Gerrit-HasComments: No
[kudu-CR] PROTOTYPE: Support certificates signed with RSASSA-PSS for channel bindings
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22910 )
Change subject: PROTOTYPE: Support certificates signed with RSASSA-PSS for channel bindings
......................................................................
Patch Set 1:
Build Started http://jenkins.kudu.apache.org/job/pre_commit/1376/
--
To view, visit http://gerrit.cloudera.org:8080/22910
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I26a25a43d778fd2f2fcf293ecb199133c675212b
Gerrit-Change-Number: 22910
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 16 May 2025 21:43:15 +0000
Gerrit-HasComments: No
[kudu-CR] PROTOTYPE: Support certificates signed with RSASSA-PSS for channel bindings
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22910 )
Change subject: PROTOTYPE: Support certificates signed with RSASSA-PSS for channel bindings
......................................................................
Patch Set 2: Verified-1
Build Failed
http://jenkins.kudu.apache.org/job/pre_commit/1377/ : FAILURE
--
To view, visit http://gerrit.cloudera.org:8080/22910
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I26a25a43d778fd2f2fcf293ecb199133c675212b
Gerrit-Change-Number: 22910
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 16 May 2025 22:47:41 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3663: Support certificates signed with RSASSA-PSS for channel bindings
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22910 )
Change subject: KUDU-3663: Support certificates signed with RSASSA-PSS for channel bindings
......................................................................
Patch Set 3: Verified+1
unrelated failures in known flaky test kudu-admin-test (TSAN)
--
To view, visit http://gerrit.cloudera.org:8080/22910
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I26a25a43d778fd2f2fcf293ecb199133c675212b
Gerrit-Change-Number: 22910
Gerrit-PatchSet: 3
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 20 May 2025 00:32:50 +0000
Gerrit-HasComments: No
[kudu-CR] PROTOTYPE: Support certificates signed with RSASSA-PSS for channel bindings
Posted by "Jason Fehr (Code Review)" <ge...@cloudera.org>.
Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/22910 )
Change subject: PROTOTYPE: Support certificates signed with RSASSA-PSS for channel bindings
......................................................................
Patch Set 2:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/22910/2/src/kudu/security/cert-test.cc
File src/kudu/security/cert-test.cc:
http://gerrit.cloudera.org:8080/#/c/22910/2/src/kudu/security/cert-test.cc@20
PS2, Line 20: #include <openssl/crypto.h>
Is this include needed?
http://gerrit.cloudera.org:8080/#/c/22910/2/src/kudu/security/cert.cc
File src/kudu/security/cert.cc:
http://gerrit.cloudera.org:8080/#/c/22910/2/src/kudu/security/cert.cc@28
PS2, Line 28: #include <cstddef>
Is this include needed?
http://gerrit.cloudera.org:8080/#/c/22910/2/src/kudu/security/cert.cc@200
PS2, Line 200: OBJ_find_sigid_algs(signature_nid, &digest_nid, NULL);
I would like to see a better error message returned here if digest_nid is NID_undef and signature is RSASSA-PSS NID_rsassaPss. Something to the effect of "signature type unsupported on OpenSSL versions less than 1.1.1). That way, the issue is easily determined.
http://gerrit.cloudera.org:8080/#/c/22910/2/src/kudu/security/cert.cc@210
PS2, Line 210: if (digest_nid == NID_undef) {
Since we are making changed here, we should wrap this if condition with UNLIKELY().
http://gerrit.cloudera.org:8080/#/c/22910/2/src/kudu/security/cert.cc@213
PS2, Line 213: "server certificate using $0 has no signature digest (hash) algorithm",
Nit: this error would be clearer if the string said "server certificate using '$0' signature algorithm has no signature digest (hash) algorithm"
--
To view, visit http://gerrit.cloudera.org:8080/22910
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I26a25a43d778fd2f2fcf293ecb199133c675212b
Gerrit-Change-Number: 22910
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 19 May 2025 15:26:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] PROTOTYPE: Support certificates signed with RSASSA-PSS for channel bindings
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22910 )
Change subject: PROTOTYPE: Support certificates signed with RSASSA-PSS for channel bindings
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/22910/2/src/kudu/security/cert.cc
File src/kudu/security/cert.cc:
http://gerrit.cloudera.org:8080/#/c/22910/2/src/kudu/security/cert.cc@200
PS2, Line 200: OBJ_find_sigid_algs(signature_nid, &digest_nid, NULL);
> A small correction:
I just took a look at OpenSSL 1.0.2 source code and it indeed has NID_rsassaPss ID.
So, then is shouldn't be a big deal to implement Jason's suggestion, I guess. I think we can omit adding a test for that use-case, maybe just running it once and manually verifying it works as expected should be OK. Let me know if I can help there.
With that, I vote +1 for the suggestion added by Jason.
--
To view, visit http://gerrit.cloudera.org:8080/22910
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I26a25a43d778fd2f2fcf293ecb199133c675212b
Gerrit-Change-Number: 22910
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 19 May 2025 18:01:04 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3663: Support certificates signed with RSASSA-PSS for channel bindings
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.
Change subject: KUDU-3663: Support certificates signed with RSASSA-PSS for channel bindings
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/22910
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I26a25a43d778fd2f2fcf293ecb199133c675212b
Gerrit-Change-Number: 22910
Gerrit-PatchSet: 3
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3663: Support certificates signed with RSASSA-PSS for channel bindings
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22910 )
Change subject: KUDU-3663: Support certificates signed with RSASSA-PSS for channel bindings
......................................................................
Patch Set 3: Code-Review+2
(2 comments)
http://gerrit.cloudera.org:8080/#/c/22910/2/src/kudu/security/cert.h
File src/kudu/security/cert.h:
http://gerrit.cloudera.org:8080/#/c/22910/2/src/kudu/security/cert.h@79
PS2, Line 79: WARN_UNUSED_RESULT
> If you don't mind, I might leave this here to make it easier to backport.
Sure, let's keep it here if it helps in back-porting or anything. It's just a nit :)
http://gerrit.cloudera.org:8080/#/c/22910/2/src/kudu/security/cert.cc
File src/kudu/security/cert.cc:
http://gerrit.cloudera.org:8080/#/c/22910/2/src/kudu/security/cert.cc@28
PS2, Line 28: #include <functional>
> I wonder what triggered this. Maybe NULL? Let's see what happens when I swi
BTW, you could see why IWYU asked for that by running
$KUDU_HOME/build-support/iwyu.py --from-git --verbose
--
To view, visit http://gerrit.cloudera.org:8080/22910
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I26a25a43d778fd2f2fcf293ecb199133c675212b
Gerrit-Change-Number: 22910
Gerrit-PatchSet: 3
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 19 May 2025 20:45:18 +0000
Gerrit-HasComments: Yes