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/04/25 06:10:10 UTC
[kudu-CR] [util] fix non-handled OpenSSL errors in JWT code
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/19799
Change subject: [util] fix non-handled OpenSSL errors in JWT code
......................................................................
[util] fix non-handled OpenSSL errors in JWT code
While looking at ~50% flakiness stats for the newly added JWT test,
I found the root cause of the issue was a non-handled error left
at the error stack of the OpenSSL library.
Further, while looking at the JWT code, I found that the code that
uses OpenSSL API was fragile and corresponding errors were either not
handled at all or handled improperly.
While this patch doesn't fix the flakiness in the bespoken JWT test,
it addresses the issue with the funny code in the JWT wrapper where
OpenSSL errors were not handled.
Change-Id: Iac5142c1dcaeec5042217e1c90f8e8770b36a670
---
M src/kudu/util/jwt-util-internal.h
M src/kudu/util/jwt-util.cc
2 files changed, 124 insertions(+), 81 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/19799/1
--
To view, visit http://gerrit.cloudera.org:8080/19799
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iac5142c1dcaeec5042217e1c90f8e8770b36a670
Gerrit-Change-Number: 19799
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <al...@apache.org>
[kudu-CR] [util] fix non-handled OpenSSL errors in JWT code
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19799 )
Change subject: [util] fix non-handled OpenSSL errors in JWT code
......................................................................
Patch Set 1: Verified+1
unrelated test failure in TxnCommitITest.TestBasicCommits (TSAN)
--
To view, visit http://gerrit.cloudera.org:8080/19799
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iac5142c1dcaeec5042217e1c90f8e8770b36a670
Gerrit-Change-Number: 19799
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Tue, 25 Apr 2023 17:47:00 +0000
Gerrit-HasComments: No
[kudu-CR] [util] handle OpenSSL errors in JWT code
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/19799 )
Change subject: [util] handle OpenSSL errors in JWT code
......................................................................
Patch Set 5: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/19799
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iac5142c1dcaeec5042217e1c90f8e8770b36a670
Gerrit-Change-Number: 19799
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Wed, 26 Apr 2023 22:50:02 +0000
Gerrit-HasComments: No
[kudu-CR] [util] handle OpenSSL errors in JWT code
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Zoltan Chovan, Yuqi Du, Attila Bukor, Kudu Jenkins, Abhishek Chennaka, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19799
to look at the new patch set (#5).
Change subject: [util] handle OpenSSL errors in JWT code
......................................................................
[util] handle OpenSSL errors in JWT code
While looking at ~50% flakiness stats for the newly added JWT test,
I found that the root cause of the issue was a non-handled error left
at the OpenSSL's error stack.
When looking at the code in jwt-util.cc, it turned out the usage of the
OpenSSL API in ConvertJwkToPem() methods was a bit fragile: possible
errors returned by OpenSSL functions were not properly handled.
While this patch doesn't fix the flakiness in the JWT test, it addresses
the issue with unhandled OpenSSL errors in the JWT wrapper code. I also
updated the code to enable larger RSA and EC keys to be converted into
PEM format, and detect if the provided JWK keys are too big.
Change-Id: Iac5142c1dcaeec5042217e1c90f8e8770b36a670
---
M src/kudu/util/jwt-util-internal.h
M src/kudu/util/jwt-util.cc
M src/kudu/util/jwt_test_certs.cc
3 files changed, 139 insertions(+), 105 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/19799/5
--
To view, visit http://gerrit.cloudera.org:8080/19799
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iac5142c1dcaeec5042217e1c90f8e8770b36a670
Gerrit-Change-Number: 19799
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [util] handle OpenSSL errors in JWT code
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/19799 )
Change subject: [util] handle OpenSSL errors in JWT code
......................................................................
Patch Set 5: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19799/5/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/19799/5/src/kudu/util/jwt-util.cc@564
PS5, Line 564: EC_KEY_new_by_curve_name
> Yes, it makes sense and it's done already in the line below.
Great, thanks for clarifying that.
--
To view, visit http://gerrit.cloudera.org:8080/19799
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iac5142c1dcaeec5042217e1c90f8e8770b36a670
Gerrit-Change-Number: 19799
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Fri, 28 Apr 2023 23:09:31 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] handle OpenSSL errors in JWT code
Posted by "Yuqi Du (Code Review)" <ge...@cloudera.org>.
Yuqi Du has posted comments on this change. ( http://gerrit.cloudera.org:8080/19799 )
Change subject: [util] handle OpenSSL errors in JWT code
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19799/4/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/19799/4/src/kudu/util/jwt-util.cc@446
PS4, Line 446: std::size(desc) - 1
'const constexpr size_t kKeyMaxSize = std::size(desc);' is ok?
--
To view, visit http://gerrit.cloudera.org:8080/19799
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iac5142c1dcaeec5042217e1c90f8e8770b36a670
Gerrit-Change-Number: 19799
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Wed, 26 Apr 2023 07:30:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] fix non-handled OpenSSL errors in JWT code
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19799 )
Change subject: [util] fix non-handled OpenSSL errors in JWT code
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/19799/1/src/kudu/util/jwt-util-internal.h
File src/kudu/util/jwt-util-internal.h:
http://gerrit.cloudera.org:8080/#/c/19799/1/src/kudu/util/jwt-util-internal.h@222
PS1, Line 222: const std::string& base64_n, const std::string& base64_e, std::string& pub_key);
> warning: non-const reference parameter 'pub_key', make it const or use a po
I'm planning to take care of that in a separate changelist.
http://gerrit.cloudera.org:8080/#/c/19799/1/src/kudu/util/jwt-util-internal.h@236
PS1, Line 236: std::string& pub_key);
> warning: non-const reference parameter 'pub_key', make it const or use a po
I'm planning to take care of that in a separate changelist.
--
To view, visit http://gerrit.cloudera.org:8080/19799
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iac5142c1dcaeec5042217e1c90f8e8770b36a670
Gerrit-Change-Number: 19799
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Tue, 25 Apr 2023 17:47:48 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] handle OpenSSL errors in JWT code
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Zoltan Chovan, Kudu Jenkins, Abhishek Chennaka, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19799
to look at the new patch set (#3).
Change subject: [util] handle OpenSSL errors in JWT code
......................................................................
[util] handle OpenSSL errors in JWT code
While looking at ~50% flakiness stats for the newly added JWT test,
I found that the root cause of the issue was a non-handled error left
at the OpenSSL's error stack.
When looking at the code in jwt-util.cc, it turned out the usage of the
OpenSSL API in ConvertJwkToPem() methods was a bit fragile: possible
errors returned by OpenSSL functions were not properly handled.
While this patch doesn't fix the flakiness in the JWT test, it addresses
the issue with unhandled OpenSSL errors in the JWT wrapper code.
Change-Id: Iac5142c1dcaeec5042217e1c90f8e8770b36a670
---
M src/kudu/util/jwt-util-internal.h
M src/kudu/util/jwt-util.cc
2 files changed, 124 insertions(+), 81 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/19799/3
--
To view, visit http://gerrit.cloudera.org:8080/19799
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iac5142c1dcaeec5042217e1c90f8e8770b36a670
Gerrit-Change-Number: 19799
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [util] handle OpenSSL errors in JWT code
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19799 )
Change subject: [util] handle OpenSSL errors in JWT code
......................................................................
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/19799/3/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/19799/3/src/kudu/util/jwt-util.cc@81
PS3, Line 81: using std::string;
> nit: move this line in front of line 80
Done
http://gerrit.cloudera.org:8080/#/c/19799/3/src/kudu/util/jwt-util.cc@412
PS3, Line 412: string str_n;
> nit: don't need
Done
http://gerrit.cloudera.org:8080/#/c/19799/4/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/19799/4/src/kudu/util/jwt-util.cc@446
PS4, Line 446: std::size(desc) - 1
> 'const constexpr size_t kKeyMaxSize = std::size(desc);' is ok?
That's a good point: once there isn't trailing '\0' in the buffer, there is no need to dance around that.
In addition, I realized this boilerplate code isn't needed since we have this functionality implemented properly in libsecurity.
Thanks a lot for your input!
--
To view, visit http://gerrit.cloudera.org:8080/19799
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iac5142c1dcaeec5042217e1c90f8e8770b36a670
Gerrit-Change-Number: 19799
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Wed, 26 Apr 2023 20:19:37 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] handle OpenSSL errors in JWT code
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/19799 )
Change subject: [util] handle OpenSSL errors in JWT code
......................................................................
Patch Set 5: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/19799
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iac5142c1dcaeec5042217e1c90f8e8770b36a670
Gerrit-Change-Number: 19799
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Thu, 27 Apr 2023 06:27:56 +0000
Gerrit-HasComments: No
[kudu-CR] [util] handle OpenSSL errors in JWT code
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/19799 )
Change subject: [util] handle OpenSSL errors in JWT code
......................................................................
Patch Set 3: Code-Review+1
(2 comments)
looks good to me, just two minor nits
http://gerrit.cloudera.org:8080/#/c/19799/3/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/19799/3/src/kudu/util/jwt-util.cc@81
PS3, Line 81: using std::shared_ptr;
nit: move this line in front of line 80
http://gerrit.cloudera.org:8080/#/c/19799/3/src/kudu/util/jwt-util.cc@412
PS3, Line 412: pub_key.clear();
nit: don't need
--
To view, visit http://gerrit.cloudera.org:8080/19799
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iac5142c1dcaeec5042217e1c90f8e8770b36a670
Gerrit-Change-Number: 19799
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Tue, 25 Apr 2023 21:09:30 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] handle OpenSSL errors in JWT code
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19799 )
Change subject: [util] handle OpenSSL errors in JWT code
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19799/5/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/19799/5/src/kudu/util/jwt-util.cc@564
PS5, Line 564: EC_KEY_new_by_curve_name
> nit: Does it makes sense to check the return value of this method as well?
Yes, it makes sense and it's done already in the line below.
--
To view, visit http://gerrit.cloudera.org:8080/19799
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iac5142c1dcaeec5042217e1c90f8e8770b36a670
Gerrit-Change-Number: 19799
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Fri, 28 Apr 2023 21:48:46 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] handle OpenSSL errors in JWT code
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/19799 )
Change subject: [util] handle OpenSSL errors in JWT code
......................................................................
Patch Set 5: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19799/5/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/19799/5/src/kudu/util/jwt-util.cc@564
PS5, Line 564: EC_KEY_new_by_curve_name
nit: Does it makes sense to check the return value of this method as well?
--
To view, visit http://gerrit.cloudera.org:8080/19799
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iac5142c1dcaeec5042217e1c90f8e8770b36a670
Gerrit-Change-Number: 19799
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Fri, 28 Apr 2023 21:16:07 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] handle OpenSSL errors in JWT code
Posted by "Yuqi Du (Code Review)" <ge...@cloudera.org>.
Yuqi Du has posted comments on this change. ( http://gerrit.cloudera.org:8080/19799 )
Change subject: [util] handle OpenSSL errors in JWT code
......................................................................
Patch Set 5: Code-Review+1
(1 comment)
LGTM.
http://gerrit.cloudera.org:8080/#/c/19799/4/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/19799/4/src/kudu/util/jwt-util.cc@446
PS4, Line 446:
> That's a good point: once there isn't trailing '\0' in the buffer, there is
Good.
--
To view, visit http://gerrit.cloudera.org:8080/19799
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iac5142c1dcaeec5042217e1c90f8e8770b36a670
Gerrit-Change-Number: 19799
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Thu, 27 Apr 2023 02:42:40 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] handle OpenSSL errors in JWT code
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Zoltan Chovan, Kudu Jenkins, Abhishek Chennaka, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19799
to look at the new patch set (#2).
Change subject: [util] handle OpenSSL errors in JWT code
......................................................................
[util] handle OpenSSL errors in JWT code
While looking at ~50% flakiness stats for the newly added JWT test,
I that found the root cause of the issue was a non-handled error left
at the error stack of the OpenSSL library.
When looking at the code in jwt-util.cc, it turned out the usage of the
OpenSSL API in ConvertJwkToPem() methods was a bit fragile: possible
errors returned by OpenSSL functions were not properly handled.
While this patch doesn't fix the flakiness in the JWT test, it addresses
the issue with the unhandled OpenSSL errors in the JWT wrapper code.
Change-Id: Iac5142c1dcaeec5042217e1c90f8e8770b36a670
---
M src/kudu/util/jwt-util-internal.h
M src/kudu/util/jwt-util.cc
2 files changed, 124 insertions(+), 81 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/19799/2
--
To view, visit http://gerrit.cloudera.org:8080/19799
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iac5142c1dcaeec5042217e1c90f8e8770b36a670
Gerrit-Change-Number: 19799
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [util] handle OpenSSL errors in JWT code
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19799 )
Change subject: [util] handle OpenSSL errors in JWT code
......................................................................
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/19799/5/src/kudu/util/jwt-util-internal.h
File src/kudu/util/jwt-util-internal.h:
http://gerrit.cloudera.org:8080/#/c/19799/5/src/kudu/util/jwt-util-internal.h@222
PS5, Line 222: const std::string& base64_n, const std::string& base64_e, std::string& pub_key);
> warning: non-const reference parameter 'pub_key', make it const or use a po
I'm planning to take care of that in a separate changelist.
http://gerrit.cloudera.org:8080/#/c/19799/5/src/kudu/util/jwt-util-internal.h@236
PS5, Line 236: std::string& pub_key);
> warning: non-const reference parameter 'pub_key', make it const or use a po
I'm planning to take care of that in a separate changelist.
--
To view, visit http://gerrit.cloudera.org:8080/19799
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iac5142c1dcaeec5042217e1c90f8e8770b36a670
Gerrit-Change-Number: 19799
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Wed, 26 Apr 2023 22:04:26 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] handle OpenSSL errors in JWT code
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/19799 )
Change subject: [util] handle OpenSSL errors in JWT code
......................................................................
[util] handle OpenSSL errors in JWT code
While looking at ~50% flakiness stats for the newly added JWT test,
I found that the root cause of the issue was a non-handled error left
at the OpenSSL's error stack.
When looking at the code in jwt-util.cc, it turned out the usage of the
OpenSSL API in ConvertJwkToPem() methods was a bit fragile: possible
errors returned by OpenSSL functions were not properly handled.
While this patch doesn't fix the flakiness in the JWT test, it addresses
the issue with unhandled OpenSSL errors in the JWT wrapper code. I also
updated the code to enable larger RSA and EC keys to be converted into
PEM format, and detect if the provided JWK keys are too big.
Change-Id: Iac5142c1dcaeec5042217e1c90f8e8770b36a670
Reviewed-on: http://gerrit.cloudera.org:8080/19799
Tested-by: Kudu Jenkins
Reviewed-by: Wenzhe Zhou <wz...@cloudera.com>
Reviewed-by: Yuqi Du <sh...@gmail.com>
Reviewed-by: Zoltan Chovan <zc...@cloudera.com>
Reviewed-by: Abhishek Chennaka <ac...@cloudera.com>
---
M src/kudu/util/jwt-util-internal.h
M src/kudu/util/jwt-util.cc
M src/kudu/util/jwt_test_certs.cc
3 files changed, 139 insertions(+), 105 deletions(-)
Approvals:
Kudu Jenkins: Verified
Wenzhe Zhou: Looks good to me, but someone else must approve
Yuqi Du: Looks good to me, but someone else must approve
Zoltan Chovan: Looks good to me, but someone else must approve
Abhishek Chennaka: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/19799
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Iac5142c1dcaeec5042217e1c90f8e8770b36a670
Gerrit-Change-Number: 19799
Gerrit-PatchSet: 6
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [util] fix non-handled OpenSSL errors in JWT code
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.
Change subject: [util] fix non-handled OpenSSL errors in JWT code
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/19799
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Iac5142c1dcaeec5042217e1c90f8e8770b36a670
Gerrit-Change-Number: 19799
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [util] handle OpenSSL errors in JWT code
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/19799 )
Change subject: [util] handle OpenSSL errors in JWT code
......................................................................
Patch Set 4: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/19799
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iac5142c1dcaeec5042217e1c90f8e8770b36a670
Gerrit-Change-Number: 19799
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Wed, 26 Apr 2023 03:19:55 +0000
Gerrit-HasComments: No
[kudu-CR] [util] handle OpenSSL errors in JWT code
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Zoltan Chovan, Kudu Jenkins, Abhishek Chennaka, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19799
to look at the new patch set (#4).
Change subject: [util] handle OpenSSL errors in JWT code
......................................................................
[util] handle OpenSSL errors in JWT code
While looking at ~50% flakiness stats for the newly added JWT test,
I found that the root cause of the issue was a non-handled error left
at the OpenSSL's error stack.
When looking at the code in jwt-util.cc, it turned out the usage of the
OpenSSL API in ConvertJwkToPem() methods was a bit fragile: possible
errors returned by OpenSSL functions were not properly handled.
While this patch doesn't fix the flakiness in the JWT test, it addresses
the issue with unhandled OpenSSL errors in the JWT wrapper code. I also
updated the code to enable larger RSA and EC keys to be converted into
PEM format, and detect if the provided JWK keys are too big.
Change-Id: Iac5142c1dcaeec5042217e1c90f8e8770b36a670
---
M src/kudu/util/jwt-util-internal.h
M src/kudu/util/jwt-util.cc
2 files changed, 134 insertions(+), 84 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/19799/4
--
To view, visit http://gerrit.cloudera.org:8080/19799
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iac5142c1dcaeec5042217e1c90f8e8770b36a670
Gerrit-Change-Number: 19799
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>