You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Zoltan Chovan (Code Review)" <ge...@cloudera.org> on 2023/04/06 18:53:59 UTC
[kudu-CR] [jwt] Verify JWKS URL server TLS certificate by default
Zoltan Chovan has uploaded this change for review. ( http://gerrit.cloudera.org:8080/19709
Change subject: [jwt] Verify JWKS URL server TLS certificate by default
......................................................................
[jwt] Verify JWKS URL server TLS certificate by default
This commit is to pull IMPALA-11922 code into the Kudu jwt handling,
with some modifications.
This change introduces:
1. verification of JWKS server TLS certificate by default
2. jwks_verify_server_certificate Kudu startup flag
Instead of introducing a new flag such as 'jwks_ca_certificate' the
already existing 'trusted_certificate_file' flag is reused.
The tls certificate verification is not used in unit-tests, however
security-itest is set up with the verification enabled.
Change-Id: I0fd7b53d651786bbe57642dd14cd477055b80c78
(cherry picked from commit f3f27dc81cb67496ec3ebb62324c2396a7180af1)
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/security-itest.cc
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/server/server_base.cc
M src/kudu/util/curl_util.cc
M src/kudu/util/jwt-util-internal.h
M src/kudu/util/jwt-util-test.cc
M src/kudu/util/jwt-util.cc
M src/kudu/util/jwt-util.h
M src/kudu/util/mini_oidc.cc
M src/kudu/util/mini_oidc.h
12 files changed, 163 insertions(+), 54 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/19709/1
--
To view, visit http://gerrit.cloudera.org:8080/19709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I0fd7b53d651786bbe57642dd14cd477055b80c78
Gerrit-Change-Number: 19709
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [jwt] Verify JWKS URL server TLS certificate by default
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19709 )
Change subject: [jwt] Verify JWKS URL server TLS certificate by default
......................................................................
Patch Set 1:
(19 comments)
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/integration-tests/security-itest.cc
File src/kudu/integration-tests/security-itest.cc:
PS1:
Once certificate verification for JWKS server is in place, does it make sense to add a 'negative' test case where the certificate of the JWKS server is (a) not valid and (b) not trusted, and that leads to a failure while communicating with such JWKS servers?
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/integration-tests/security-itest.cc@537
PS1, Line 537:
nit: misaligned indent?
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/mini-cluster/external_mini_cluster.cc@280
PS1, Line 280:
nit: remove this extra empty line?
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/mini-cluster/external_mini_cluster.cc@283
PS1, Line 283:
nit: wrong indent?
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/server/server_base.cc@277
PS1, Line 277: without
The name of the flag signals it should be 'with'?
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/server/server_base.cc@282
PS1, Line 282: if
nit: drop 'if'?
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/curl_util.cc
File src/kudu/util/curl_util.cc:
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/curl_util.cc@167
PS1, Line 167: todo(zchovan) consolidate these two checks
Why?
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/jwt-util.h
File src/kudu/util/jwt-util.h:
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/jwt-util.h@72
PS1, Line 72: bool is_local_file)
How is this 'is_local_file' relevant once having these 'jwks_uri', 'jwks_ca_certificate' parameters for the signature of this method?
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/jwt-util.h@71
PS1, Line 71: bool jwks_verify_server_certificate,
: const std::string& jwks_ca_certificate
nit: maybe, switch the order of the 'jwks_verify_server_certificate' and 'jwks_ca_certificate' parameters?
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/jwt-util.h@144
PS1, Line 144: bool jwks_verify_server_certificate_
Could this be 'const' as well?
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/jwt-util.cc@794
PS1, Line 794: false, ""
style nit: add the comments with the names of the parameters for the arguments at this call site
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/jwt-util.cc@798
PS1, Line 798:
nit: wrong indent
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/jwt-util.cc@801
PS1, Line 801:
nit: wrong indent
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/jwt-util.cc@949
PS1, Line 949: false, ""
style nit: add the comments with the names of the parameters for 'false' and '""' arguments
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/jwt-util.cc@1025
PS1, Line 1025: false
style nit: keep the name of the parameter in the comment
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/jwt-util.cc@1042
PS1, Line 1042: false
style nit: keep the name of the parameter in the comment
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/mini_oidc.h
File src/kudu/util/mini_oidc.h:
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/mini_oidc.h@43
PS1, Line 43: std::string server_certificate;
nit: could be great to document the newly added fields in-line as the rest of the fields of MiniOidcOptions
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/mini_oidc.cc
File src/kudu/util/mini_oidc.cc:
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/mini_oidc.cc@131
PS1, Line 131: bound_addrs
nit: consider renaming this variable since this is no longer a bound address?
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/mini_oidc.cc@134
PS1, Line 134: RETURN_NOT_OK(addr.ParseString(bound_addrs[0].host(), bound_addrs[0].port()));
Perhaps, add a comment that calling ParseString() is just to verify the address components?
--
To view, visit http://gerrit.cloudera.org:8080/19709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0fd7b53d651786bbe57642dd14cd477055b80c78
Gerrit-Change-Number: 19709
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Fri, 07 Apr 2023 05:41:38 +0000
Gerrit-HasComments: Yes
[kudu-CR] [jwt] Verify JWKS URL server TLS certificate by default
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/19709 )
Change subject: [jwt] Verify JWKS URL server TLS certificate by default
......................................................................
Patch Set 1:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/19709/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19709/1//COMMIT_MSG@19
PS1, Line 19: tls
nit: TLS
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/server/server_base.cc@281
PS1, Line 281:
nit: extra space
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/server/server_base.cc@815
PS1, Line 815:
nit: wrong indent
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/curl_util.cc
File src/kudu/util/curl_util.cc:
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/curl_util.cc@168
PS1, Line 168: FLAGS_trusted_certificate_file
EasyCurl class is common utility class. It could be used to download files from multiple sites in future. It's better to save certificate_file in member variable, not to access global flag variable directly.
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/jwt-util.cc@606
PS1, Line 606: jwks_ca_certificate
it's unused
--
To view, visit http://gerrit.cloudera.org:8080/19709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0fd7b53d651786bbe57642dd14cd477055b80c78
Gerrit-Change-Number: 19709
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Fri, 07 Apr 2023 07:15:56 +0000
Gerrit-HasComments: Yes
[kudu-CR] [jwt] Verify JWKS URL server TLS certificate by default
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Attila Bukor, Kudu Jenkins, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19709
to look at the new patch set (#4).
Change subject: [jwt] Verify JWKS URL server TLS certificate by default
......................................................................
[jwt] Verify JWKS URL server TLS certificate by default
This commit is to pull IMPALA-11922 code into the Kudu jwt handling,
with some modifications.
This change introduces:
1. verification of JWKS server TLS certificate by default
2. jwks_verify_server_certificate Kudu startup flag
Instead of introducing a new flag such as 'jwks_ca_certificate' the
already existing 'trusted_certificate_file' flag is reused.
The TLS certificate verification is not used in unit-tests, however
security-itest is set up with the verification enabled.
Change-Id: I0fd7b53d651786bbe57642dd14cd477055b80c78
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/security-itest.cc
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/server/server_base.cc
M src/kudu/util/jwt-util-internal.h
M src/kudu/util/jwt-util-test.cc
M src/kudu/util/jwt-util.cc
M src/kudu/util/jwt-util.h
M src/kudu/util/mini_oidc.cc
M src/kudu/util/mini_oidc.h
11 files changed, 273 insertions(+), 72 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/19709/4
--
To view, visit http://gerrit.cloudera.org:8080/19709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0fd7b53d651786bbe57642dd14cd477055b80c78
Gerrit-Change-Number: 19709
Gerrit-PatchSet: 4
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [jwt] Verify JWKS URL server TLS certificate by default
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/19709 )
Change subject: [jwt] Verify JWKS URL server TLS certificate by default
......................................................................
[jwt] Verify JWKS URL server TLS certificate by default
This commit is to pull IMPALA-11922 code into the Kudu jwt handling,
with some modifications.
This change introduces:
1. verification of JWKS server TLS certificate by default
2. jwks_verify_server_certificate Kudu startup flag
Instead of introducing a new flag such as 'jwks_ca_certificate' the
already existing 'trusted_certificate_file' flag is reused.
The TLS certificate verification is not used in unit-tests, however
security-itest is set up with the verification enabled.
Change-Id: I0fd7b53d651786bbe57642dd14cd477055b80c78
Reviewed-on: http://gerrit.cloudera.org:8080/19709
Reviewed-by: Attila Bukor <ab...@apache.org>
Tested-by: Kudu Jenkins
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/security-itest.cc
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/security/test/test_certs.cc
M src/kudu/security/test/test_certs.h
M src/kudu/server/server_base.cc
M src/kudu/util/jwt-util-internal.h
M src/kudu/util/jwt-util-test.cc
M src/kudu/util/jwt-util.cc
M src/kudu/util/jwt-util.h
M src/kudu/util/mini_oidc.cc
M src/kudu/util/mini_oidc.h
13 files changed, 421 insertions(+), 52 deletions(-)
Approvals:
Attila Bukor: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/19709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I0fd7b53d651786bbe57642dd14cd477055b80c78
Gerrit-Change-Number: 19709
Gerrit-PatchSet: 11
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
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: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [jwt] Verify JWKS URL server TLS certificate by default
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Attila Bukor, Kudu Jenkins, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19709
to look at the new patch set (#2).
Change subject: [jwt] Verify JWKS URL server TLS certificate by default
......................................................................
[jwt] Verify JWKS URL server TLS certificate by default
This commit is to pull IMPALA-11922 code into the Kudu jwt handling,
with some modifications.
This change introduces:
1. verification of JWKS server TLS certificate by default
2. jwks_verify_server_certificate Kudu startup flag
Instead of introducing a new flag such as 'jwks_ca_certificate' the
already existing 'trusted_certificate_file' flag is reused.
The TLS certificate verification is not used in unit-tests, however
security-itest is set up with the verification enabled.
Change-Id: I0fd7b53d651786bbe57642dd14cd477055b80c78
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/security-itest.cc
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/server/server_base.cc
M src/kudu/util/curl_util.cc
M src/kudu/util/jwt-util-internal.h
M src/kudu/util/jwt-util-test.cc
M src/kudu/util/jwt-util.cc
M src/kudu/util/jwt-util.h
M src/kudu/util/mini_oidc.cc
M src/kudu/util/mini_oidc.h
12 files changed, 247 insertions(+), 58 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/19709/2
--
To view, visit http://gerrit.cloudera.org:8080/19709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0fd7b53d651786bbe57642dd14cd477055b80c78
Gerrit-Change-Number: 19709
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [jwt] Verify JWKS URL server TLS certificate by default
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Attila Bukor, Kudu Jenkins, Abhishek Chennaka, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19709
to look at the new patch set (#7).
Change subject: [jwt] Verify JWKS URL server TLS certificate by default
......................................................................
[jwt] Verify JWKS URL server TLS certificate by default
This commit is to pull IMPALA-11922 code into the Kudu jwt handling,
with some modifications.
This change introduces:
1. verification of JWKS server TLS certificate by default
2. jwks_verify_server_certificate Kudu startup flag
Instead of introducing a new flag such as 'jwks_ca_certificate' the
already existing 'trusted_certificate_file' flag is reused.
The TLS certificate verification is not used in unit-tests, however
security-itest is set up with the verification enabled.
Change-Id: I0fd7b53d651786bbe57642dd14cd477055b80c78
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/security-itest.cc
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/server/server_base.cc
M src/kudu/util/jwt-util-internal.h
M src/kudu/util/jwt-util-test.cc
M src/kudu/util/jwt-util.cc
M src/kudu/util/jwt-util.h
M src/kudu/util/mini_oidc.cc
M src/kudu/util/mini_oidc.h
11 files changed, 270 insertions(+), 52 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/19709/7
--
To view, visit http://gerrit.cloudera.org:8080/19709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0fd7b53d651786bbe57642dd14cd477055b80c78
Gerrit-Change-Number: 19709
Gerrit-PatchSet: 7
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
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: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [jwt] Verify JWKS URL server TLS certificate by default
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/19709 )
Change subject: [jwt] Verify JWKS URL server TLS certificate by default
......................................................................
Patch Set 6:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/19709/5/src/kudu/integration-tests/security-itest.cc
File src/kudu/integration-tests/security-itest.cc:
http://gerrit.cloudera.org:8080/#/c/19709/5/src/kudu/integration-tests/security-itest.cc@552
PS5, Line 552: const auto configure_builder_for =
nit: looks like the indentation got misaligned.
http://gerrit.cloudera.org:8080/#/c/19709/5/src/kudu/integration-tests/security-itest.cc@631
PS5, Line 631: ca_certificate_file = kudu::security::kCaExpiredCert;
It's the certificate_file (and the corresponding private_key_file) that should be expired, not the CA certificate. Now, this is just an untrusted cert, not an invalid one.
http://gerrit.cloudera.org:8080/#/c/19709/5/src/kudu/integration-tests/security-itest.cc@694
PS5, Line 694: cluster_opts_.extra_master_flags.push_back("--jwks_verify_server_certificate=true");
Is this necessary? Isn't this the default value? Also, can you remove the next line instead of commenting it out?
--
To view, visit http://gerrit.cloudera.org:8080/19709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0fd7b53d651786bbe57642dd14cd477055b80c78
Gerrit-Change-Number: 19709
Gerrit-PatchSet: 6
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
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: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Thu, 13 Apr 2023 21:21:21 +0000
Gerrit-HasComments: Yes
[kudu-CR] [jwt] Verify JWKS URL server TLS certificate by default
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Attila Bukor, Kudu Jenkins, Abhishek Chennaka, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19709
to look at the new patch set (#10).
Change subject: [jwt] Verify JWKS URL server TLS certificate by default
......................................................................
[jwt] Verify JWKS URL server TLS certificate by default
This commit is to pull IMPALA-11922 code into the Kudu jwt handling,
with some modifications.
This change introduces:
1. verification of JWKS server TLS certificate by default
2. jwks_verify_server_certificate Kudu startup flag
Instead of introducing a new flag such as 'jwks_ca_certificate' the
already existing 'trusted_certificate_file' flag is reused.
The TLS certificate verification is not used in unit-tests, however
security-itest is set up with the verification enabled.
Change-Id: I0fd7b53d651786bbe57642dd14cd477055b80c78
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/security-itest.cc
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/security/test/test_certs.cc
M src/kudu/security/test/test_certs.h
M src/kudu/server/server_base.cc
M src/kudu/util/jwt-util-internal.h
M src/kudu/util/jwt-util-test.cc
M src/kudu/util/jwt-util.cc
M src/kudu/util/jwt-util.h
M src/kudu/util/mini_oidc.cc
M src/kudu/util/mini_oidc.h
13 files changed, 421 insertions(+), 52 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/19709/10
--
To view, visit http://gerrit.cloudera.org:8080/19709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0fd7b53d651786bbe57642dd14cd477055b80c78
Gerrit-Change-Number: 19709
Gerrit-PatchSet: 10
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
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: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [jwt] Verify JWKS URL server TLS certificate by default
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/19709 )
Change subject: [jwt] Verify JWKS URL server TLS certificate by default
......................................................................
Patch Set 2:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/19709/2/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/19709/2/src/kudu/server/server_base.cc@285
PS2, Line 285: TAG_FLAG(jwks_verify_server_certificate, experimental);
how about tagging this as unsafe instead of experimental?
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/curl_util.cc
File src/kudu/util/curl_util.cc:
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/curl_util.cc@168
PS1, Line 168: L_RETURN_NOT_OK(curl_easy_seto
> We still use this flag, but pass the flag as parameter to this class to mak
That is a good point, but I think it's outside the scope of this change as it's an existing flag. Maybe a follow-up change could address this.
http://gerrit.cloudera.org:8080/#/c/19709/2/src/kudu/util/curl_util.cc
File src/kudu/util/curl_util.cc:
PS2:
What are these changes for? Are these needed/relevant?
http://gerrit.cloudera.org:8080/#/c/19709/2/src/kudu/util/jwt-util-internal.h
File src/kudu/util/jwt-util-internal.h:
http://gerrit.cloudera.org:8080/#/c/19709/2/src/kudu/util/jwt-util-internal.h@371
PS2, Line 371: std::string jwks_ca_certificate_;
Is this still used?
http://gerrit.cloudera.org:8080/#/c/19709/2/src/kudu/util/jwt-util-test.cc
File src/kudu/util/jwt-util-test.cc:
http://gerrit.cloudera.org:8080/#/c/19709/2/src/kudu/util/jwt-util-test.cc@950
PS2, Line 950: false
nit: prepend with comment here and below
--
To view, visit http://gerrit.cloudera.org:8080/19709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0fd7b53d651786bbe57642dd14cd477055b80c78
Gerrit-Change-Number: 19709
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Wed, 12 Apr 2023 07:15:07 +0000
Gerrit-HasComments: Yes
[kudu-CR] [jwt] Verify JWKS URL server TLS certificate by default
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/19709 )
Change subject: [jwt] Verify JWKS URL server TLS certificate by default
......................................................................
Patch Set 5:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/19709/2/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/19709/2/src/kudu/server/server_base.cc@285
PS2, Line 285: TAG_FLAG(jwks_verify_server_certificate, experimental);
> Sounds good to me.
Ack
http://gerrit.cloudera.org:8080/#/c/19709/2/src/kudu/util/curl_util.cc
File src/kudu/util/curl_util.cc:
PS2:
> I think they're unrelated to this change and I'm not sure they're doing any
Okay, I'm reverting them
http://gerrit.cloudera.org:8080/#/c/19709/2/src/kudu/util/jwt-util-internal.h
File src/kudu/util/jwt-util-internal.h:
http://gerrit.cloudera.org:8080/#/c/19709/2/src/kudu/util/jwt-util-internal.h@371
PS2, Line 371: // will be atomically swapped.
> It looks like it's just being passed around to be set in different places,
you are right, I removed it
--
To view, visit http://gerrit.cloudera.org:8080/19709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0fd7b53d651786bbe57642dd14cd477055b80c78
Gerrit-Change-Number: 19709
Gerrit-PatchSet: 5
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Thu, 13 Apr 2023 20:02:47 +0000
Gerrit-HasComments: Yes
[kudu-CR] [jwt] Verify JWKS URL server TLS certificate by default
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Attila Bukor, Kudu Jenkins, Abhishek Chennaka, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19709
to look at the new patch set (#6).
Change subject: [jwt] Verify JWKS URL server TLS certificate by default
......................................................................
[jwt] Verify JWKS URL server TLS certificate by default
This commit is to pull IMPALA-11922 code into the Kudu jwt handling,
with some modifications.
This change introduces:
1. verification of JWKS server TLS certificate by default
2. jwks_verify_server_certificate Kudu startup flag
Instead of introducing a new flag such as 'jwks_ca_certificate' the
already existing 'trusted_certificate_file' flag is reused.
The TLS certificate verification is not used in unit-tests, however
security-itest is set up with the verification enabled.
Change-Id: I0fd7b53d651786bbe57642dd14cd477055b80c78
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/security-itest.cc
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/server/server_base.cc
M src/kudu/util/jwt-util-internal.h
M src/kudu/util/jwt-util-test.cc
M src/kudu/util/jwt-util.cc
M src/kudu/util/jwt-util.h
M src/kudu/util/mini_oidc.cc
M src/kudu/util/mini_oidc.h
11 files changed, 287 insertions(+), 67 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/19709/6
--
To view, visit http://gerrit.cloudera.org:8080/19709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0fd7b53d651786bbe57642dd14cd477055b80c78
Gerrit-Change-Number: 19709
Gerrit-PatchSet: 6
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
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: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [jwt] Verify JWKS URL server TLS certificate by default
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/19709 )
Change subject: [jwt] Verify JWKS URL server TLS certificate by default
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/19709/2/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/19709/2/src/kudu/server/server_base.cc@285
PS2, Line 285: TAG_FLAG(jwks_verify_server_certificate, experimental);
> I think we could tag it wit both experimental and unsafe, we did the same f
Sounds good to me.
http://gerrit.cloudera.org:8080/#/c/19709/2/src/kudu/util/curl_util.cc
File src/kudu/util/curl_util.cc:
PS2:
> these are coming from the Impala version of curl_util.cc
I think they're unrelated to this change and I'm not sure they're doing anything useful to us, so I think this change should be removed.
http://gerrit.cloudera.org:8080/#/c/19709/2/src/kudu/util/jwt-util-internal.h
File src/kudu/util/jwt-util-internal.h:
http://gerrit.cloudera.org:8080/#/c/19709/2/src/kudu/util/jwt-util-internal.h@371
PS2, Line 371: std::string jwks_ca_certificate_;
> yes, e.g in JWKSMgr::UpdateJWKSThread()
It looks like it's just being passed around to be set in different places, but it's not actually used anywhere, can you please double check?
--
To view, visit http://gerrit.cloudera.org:8080/19709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0fd7b53d651786bbe57642dd14cd477055b80c78
Gerrit-Change-Number: 19709
Gerrit-PatchSet: 3
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Thu, 13 Apr 2023 14:48:16 +0000
Gerrit-HasComments: Yes
[kudu-CR] [jwt] Verify JWKS URL server TLS certificate by default
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/19709 )
Change subject: [jwt] Verify JWKS URL server TLS certificate by default
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/19709/2/src/kudu/integration-tests/security-itest.cc
File src/kudu/integration-tests/security-itest.cc:
http://gerrit.cloudera.org:8080/#/c/19709/2/src/kudu/integration-tests/security-itest.cc@642
PS2, Line 642: TestJwtMiniClusterWithUntrustedCert
Except name, this unit test code looks same as TestJwtMiniClusterWithInvalidCert
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/curl_util.cc
File src/kudu/util/curl_util.cc:
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/curl_util.cc@168
PS1, Line 168: L_RETURN_NOT_OK(curl_easy_seto
> We already use this flag to verify certificates when communicating with Ran
We still use this flag, but pass the flag as parameter to this class to make this class generic. Impala also use this class, but Impala use different file name.
--
To view, visit http://gerrit.cloudera.org:8080/19709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0fd7b53d651786bbe57642dd14cd477055b80c78
Gerrit-Change-Number: 19709
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Tue, 11 Apr 2023 20:09:07 +0000
Gerrit-HasComments: Yes
[kudu-CR] [jwt] Verify JWKS URL server TLS certificate by default
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/19709 )
Change subject: [jwt] Verify JWKS URL server TLS certificate by default
......................................................................
Patch Set 1:
(22 comments)
http://gerrit.cloudera.org:8080/#/c/19709/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19709/1//COMMIT_MSG@19
PS1, Line 19: tls
> nit: TLS
Done
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/integration-tests/security-itest.cc
File src/kudu/integration-tests/security-itest.cc:
PS1:
> Once certificate verification for JWKS server is in place, does it make sen
Yes, I agree. There are no invalid/untrusted certificates in 'test_certs.cc', however I found that we can generate certificates on the fly with the methods in 'cert_management.cc'.
What would you suggest, should I create new methods in test_certs to have invalid/untrusted ready to use, or should I generate them using the cert_managment tools?
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/integration-tests/security-itest.cc@537
PS1, Line 537:
> nit: misaligned indent?
Done
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/mini-cluster/external_mini_cluster.cc@280
PS1, Line 280:
> nit: remove this extra empty line?
Done
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/mini-cluster/external_mini_cluster.cc@283
PS1, Line 283:
> nit: wrong indent?
Done
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/server/server_base.cc@277
PS1, Line 277: without
> The name of the flag signals it should be 'with'?
Done
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/server/server_base.cc@281
PS1, Line 281:
> nit: extra space
Done
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/server/server_base.cc@282
PS1, Line 282: if
> nit: drop 'if'?
Done
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/server/server_base.cc@815
PS1, Line 815:
> nit: wrong indent
Done
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/curl_util.cc
File src/kudu/util/curl_util.cc:
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/curl_util.cc@167
PS1, Line 167: todo(zchovan) consolidate these two checks
> Why?
sorry, that was left in
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/jwt-util.h
File src/kudu/util/jwt-util.h:
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/jwt-util.h@72
PS1, Line 72: bool is_local_file)
> How is this 'is_local_file' relevant once having these 'jwks_uri', 'jwks_ca
you can use this to validate a local file that is found at 'jwks_uri', in this case there is no data going through https so there is no need to validate certificates
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/jwt-util.h@144
PS1, Line 144: bool jwks_verify_server_certificate_
> Could this be 'const' as well?
Done
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/jwt-util.cc@606
PS1, Line 606: jwks_ca_certificate
> it's unused
Done
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/jwt-util.cc@794
PS1, Line 794: false, ""
> style nit: add the comments with the names of the parameters for the argum
Done
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/jwt-util.cc@798
PS1, Line 798:
> nit: wrong indent
Done
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/jwt-util.cc@801
PS1, Line 801:
> nit: wrong indent
Done
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/jwt-util.cc@949
PS1, Line 949: false, ""
> style nit: add the comments with the names of the parameters for 'false' a
Done
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/jwt-util.cc@1025
PS1, Line 1025: false
> style nit: keep the name of the parameter in the comment
Done
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/jwt-util.cc@1042
PS1, Line 1042: false
> style nit: keep the name of the parameter in the comment
Done
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/mini_oidc.h
File src/kudu/util/mini_oidc.h:
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/mini_oidc.h@43
PS1, Line 43: std::string server_certificate;
> nit: could be great to document the newly added fields in-line as the rest
Done
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/mini_oidc.cc
File src/kudu/util/mini_oidc.cc:
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/mini_oidc.cc@131
PS1, Line 131: bound_addrs
> nit: consider renaming this variable since this is no longer a bound addres
Done
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/mini_oidc.cc@134
PS1, Line 134: RETURN_NOT_OK(addr.ParseString(bound_addrs[0].host(), bound_addrs[0].port()));
> Perhaps, add a comment that calling ParseString() is just to verify the add
Done
--
To view, visit http://gerrit.cloudera.org:8080/19709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0fd7b53d651786bbe57642dd14cd477055b80c78
Gerrit-Change-Number: 19709
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Tue, 11 Apr 2023 19:22:23 +0000
Gerrit-HasComments: Yes
[kudu-CR] [jwt] Verify JWKS URL server TLS certificate by default
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Attila Bukor, Kudu Jenkins, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19709
to look at the new patch set (#5).
Change subject: [jwt] Verify JWKS URL server TLS certificate by default
......................................................................
[jwt] Verify JWKS URL server TLS certificate by default
This commit is to pull IMPALA-11922 code into the Kudu jwt handling,
with some modifications.
This change introduces:
1. verification of JWKS server TLS certificate by default
2. jwks_verify_server_certificate Kudu startup flag
Instead of introducing a new flag such as 'jwks_ca_certificate' the
already existing 'trusted_certificate_file' flag is reused.
The TLS certificate verification is not used in unit-tests, however
security-itest is set up with the verification enabled.
Change-Id: I0fd7b53d651786bbe57642dd14cd477055b80c78
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/security-itest.cc
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/server/server_base.cc
M src/kudu/util/jwt-util-internal.h
M src/kudu/util/jwt-util-test.cc
M src/kudu/util/jwt-util.cc
M src/kudu/util/jwt-util.h
M src/kudu/util/mini_oidc.cc
M src/kudu/util/mini_oidc.h
11 files changed, 287 insertions(+), 67 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/19709/5
--
To view, visit http://gerrit.cloudera.org:8080/19709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0fd7b53d651786bbe57642dd14cd477055b80c78
Gerrit-Change-Number: 19709
Gerrit-PatchSet: 5
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [jwt] Verify JWKS URL server TLS certificate by default
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/19709 )
Change subject: [jwt] Verify JWKS URL server TLS certificate by default
......................................................................
Patch Set 6:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/19709/5/src/kudu/integration-tests/security-itest.cc
File src/kudu/integration-tests/security-itest.cc:
http://gerrit.cloudera.org:8080/#/c/19709/5/src/kudu/integration-tests/security-itest.cc@552
PS5, Line 552: const auto configure_builder_for =
> nit: looks like the indentation got misaligned.
Done
http://gerrit.cloudera.org:8080/#/c/19709/5/src/kudu/integration-tests/security-itest.cc@631
PS5, Line 631: ca_certificate_file = kudu::security::kCaExpiredCert;
> It's the certificate_file (and the corresponding private_key_file) that sho
if the ca_cert is expired, then there is no trust that would be able to verify the server certificate, making the setup invalid
http://gerrit.cloudera.org:8080/#/c/19709/5/src/kudu/integration-tests/security-itest.cc@694
PS5, Line 694: cluster_opts_.extra_master_flags.push_back("--jwks_verify_server_certificate=true");
> Is this necessary? Isn't this the default value? Also, can you remove the n
okay, added an explanation comment to clear it up, for readability
--
To view, visit http://gerrit.cloudera.org:8080/19709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0fd7b53d651786bbe57642dd14cd477055b80c78
Gerrit-Change-Number: 19709
Gerrit-PatchSet: 6
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
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: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Thu, 13 Apr 2023 21:51:31 +0000
Gerrit-HasComments: Yes
[kudu-CR] [jwt] Verify JWKS URL server TLS certificate by default
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Attila Bukor, Kudu Jenkins, Abhishek Chennaka, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19709
to look at the new patch set (#8).
Change subject: [jwt] Verify JWKS URL server TLS certificate by default
......................................................................
[jwt] Verify JWKS URL server TLS certificate by default
This commit is to pull IMPALA-11922 code into the Kudu jwt handling,
with some modifications.
This change introduces:
1. verification of JWKS server TLS certificate by default
2. jwks_verify_server_certificate Kudu startup flag
Instead of introducing a new flag such as 'jwks_ca_certificate' the
already existing 'trusted_certificate_file' flag is reused.
The TLS certificate verification is not used in unit-tests, however
security-itest is set up with the verification enabled.
Change-Id: I0fd7b53d651786bbe57642dd14cd477055b80c78
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/security-itest.cc
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/security/test/test_certs.cc
M src/kudu/security/test/test_certs.h
M src/kudu/server/server_base.cc
M src/kudu/util/jwt-util-internal.h
M src/kudu/util/jwt-util-test.cc
M src/kudu/util/jwt-util.cc
M src/kudu/util/jwt-util.h
M src/kudu/util/mini_oidc.cc
M src/kudu/util/mini_oidc.h
13 files changed, 422 insertions(+), 52 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/19709/8
--
To view, visit http://gerrit.cloudera.org:8080/19709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0fd7b53d651786bbe57642dd14cd477055b80c78
Gerrit-Change-Number: 19709
Gerrit-PatchSet: 8
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
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: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [jwt] Verify JWKS URL server TLS certificate by default
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/19709 )
Change subject: [jwt] Verify JWKS URL server TLS certificate by default
......................................................................
Patch Set 3:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/19709/2/src/kudu/integration-tests/security-itest.cc
File src/kudu/integration-tests/security-itest.cc:
http://gerrit.cloudera.org:8080/#/c/19709/2/src/kudu/integration-tests/security-itest.cc@642
PS2, Line 642: TestJwtMiniClusterWithUntrustedCert
> Except name, this unit test code looks same as TestJwtMiniClusterWithInvali
yes, these two tests are just stubs now, until I figure out how to properly provide the untrusted/invalid certs to them
http://gerrit.cloudera.org:8080/#/c/19709/2/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/19709/2/src/kudu/server/server_base.cc@285
PS2, Line 285: TAG_FLAG(jwks_verify_server_certificate, experimental);
> how about tagging this as unsafe instead of experimental?
I think we could tag it wit both experimental and unsafe, we did the same for 'jwt_alllow_without_tls'
http://gerrit.cloudera.org:8080/#/c/19709/2/src/kudu/util/curl_util.cc
File src/kudu/util/curl_util.cc:
PS2:
> What are these changes for? Are these needed/relevant?
these are coming from the Impala version of curl_util.cc
http://gerrit.cloudera.org:8080/#/c/19709/2/src/kudu/util/jwt-util-internal.h
File src/kudu/util/jwt-util-internal.h:
http://gerrit.cloudera.org:8080/#/c/19709/2/src/kudu/util/jwt-util-internal.h@371
PS2, Line 371: std::string jwks_ca_certificate_;
> Is this still used?
yes, e.g in JWKSMgr::UpdateJWKSThread()
http://gerrit.cloudera.org:8080/#/c/19709/2/src/kudu/util/jwt-util-test.cc
File src/kudu/util/jwt-util-test.cc:
http://gerrit.cloudera.org:8080/#/c/19709/2/src/kudu/util/jwt-util-test.cc@950
PS2, Line 950:
> nit: prepend with comment here and below
Done
--
To view, visit http://gerrit.cloudera.org:8080/19709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0fd7b53d651786bbe57642dd14cd477055b80c78
Gerrit-Change-Number: 19709
Gerrit-PatchSet: 3
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Thu, 13 Apr 2023 14:05:50 +0000
Gerrit-HasComments: Yes
[kudu-CR] [jwt] Verify JWKS URL server TLS certificate by default
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/19709 )
Change subject: [jwt] Verify JWKS URL server TLS certificate by default
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/curl_util.cc
File src/kudu/util/curl_util.cc:
http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/curl_util.cc@168
PS1, Line 168: FLAGS_trusted_certificate_file
> EasyCurl class is common utility class. It could be used to download files
We already use this flag to verify certificates when communicating with Ranger KMS. Ideally, both Ranger KMS and JWKS server should present a certificate that is signed by a CA that is trusted globally on all servers. If this is not the case, they'd still likely use a local CA that signs all internal certificates, and if they don't install it on each server in /etc/ssl/certs, they can point this flag to a file containing this CA certificate. Worst case, they can use a single file containing multiple certificates in PEM format, and point this flag to that file. I disagree that each server we connect to should use its own trusted certificate file, that seems to go against how certificates should normally work.
--
To view, visit http://gerrit.cloudera.org:8080/19709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0fd7b53d651786bbe57642dd14cd477055b80c78
Gerrit-Change-Number: 19709
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Tue, 11 Apr 2023 09:50:15 +0000
Gerrit-HasComments: Yes
[kudu-CR] [jwt] Verify JWKS URL server TLS certificate by default
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Attila Bukor, Kudu Jenkins, Abhishek Chennaka, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19709
to look at the new patch set (#9).
Change subject: [jwt] Verify JWKS URL server TLS certificate by default
......................................................................
[jwt] Verify JWKS URL server TLS certificate by default
This commit is to pull IMPALA-11922 code into the Kudu jwt handling,
with some modifications.
This change introduces:
1. verification of JWKS server TLS certificate by default
2. jwks_verify_server_certificate Kudu startup flag
Instead of introducing a new flag such as 'jwks_ca_certificate' the
already existing 'trusted_certificate_file' flag is reused.
The TLS certificate verification is not used in unit-tests, however
security-itest is set up with the verification enabled.
Change-Id: I0fd7b53d651786bbe57642dd14cd477055b80c78
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/security-itest.cc
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/security/test/test_certs.cc
M src/kudu/security/test/test_certs.h
M src/kudu/server/server_base.cc
M src/kudu/util/jwt-util-internal.h
M src/kudu/util/jwt-util-test.cc
M src/kudu/util/jwt-util.cc
M src/kudu/util/jwt-util.h
M src/kudu/util/mini_oidc.cc
M src/kudu/util/mini_oidc.h
13 files changed, 423 insertions(+), 52 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/19709/9
--
To view, visit http://gerrit.cloudera.org:8080/19709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0fd7b53d651786bbe57642dd14cd477055b80c78
Gerrit-Change-Number: 19709
Gerrit-PatchSet: 9
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
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: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [jwt] Verify JWKS URL server TLS certificate by default
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/19709 )
Change subject: [jwt] Verify JWKS URL server TLS certificate by default
......................................................................
Patch Set 10: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/19709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0fd7b53d651786bbe57642dd14cd477055b80c78
Gerrit-Change-Number: 19709
Gerrit-PatchSet: 10
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
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: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Fri, 14 Apr 2023 14:15:12 +0000
Gerrit-HasComments: No
[kudu-CR] [jwt] Verify JWKS URL server TLS certificate by default
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Attila Bukor, Kudu Jenkins, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19709
to look at the new patch set (#3).
Change subject: [jwt] Verify JWKS URL server TLS certificate by default
......................................................................
[jwt] Verify JWKS URL server TLS certificate by default
This commit is to pull IMPALA-11922 code into the Kudu jwt handling,
with some modifications.
This change introduces:
1. verification of JWKS server TLS certificate by default
2. jwks_verify_server_certificate Kudu startup flag
Instead of introducing a new flag such as 'jwks_ca_certificate' the
already existing 'trusted_certificate_file' flag is reused.
The TLS certificate verification is not used in unit-tests, however
security-itest is set up with the verification enabled.
Change-Id: I0fd7b53d651786bbe57642dd14cd477055b80c78
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/security-itest.cc
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/server/server_base.cc
M src/kudu/util/curl_util.cc
M src/kudu/util/jwt-util-internal.h
M src/kudu/util/jwt-util-test.cc
M src/kudu/util/jwt-util.cc
M src/kudu/util/jwt-util.h
M src/kudu/util/mini_oidc.cc
M src/kudu/util/mini_oidc.h
12 files changed, 254 insertions(+), 58 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/19709/3
--
To view, visit http://gerrit.cloudera.org:8080/19709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0fd7b53d651786bbe57642dd14cd477055b80c78
Gerrit-Change-Number: 19709
Gerrit-PatchSet: 3
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>