You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Jason Fehr (Code Review)" <ge...@cloudera.org> on 2023/02/15 22:54:27 UTC

[Impala-ASF-CR] IMPALA-11922 Verify JWKS URL server TLS certificate by default.

Jason Fehr has uploaded this change for review. ( http://gerrit.cloudera.org:8080/19503


Change subject: IMPALA-11922 Verify JWKS URL server TLS certificate by default.
......................................................................

IMPALA-11922 Verify JWKS URL server TLS certificate by default.

**** BREAKING CHANGE ****

JWT Auth has an option to specify the location of the
JSON Web Key Set (JWKS) using a URL.  If that URL is
accessed over https, the TLS certificate presented by the
server is not verified.

This means that Impala only requires the server to return
a TLS certificate, whether or not Impala trusts the signing
certificate chain.

The implications of this setup is that a fully secure chain
of trust cannot be established throughout the entire JWT
authentication lifecycle and thus creates an attack vector
where a bad actor could trick Impala into trusting an
actor-controlled JWKS.  The bad actor can then generate
a JWT with any claims they chose and Impala will accept it.

This change introduces:
  1. verification of JWKS server TLS certificate by default
  2. jwks_insecure_tls Impala startup flag
  3. jwks_ca_certificate Impala startup flag

1. While previously, the JWKS URL was always called without
   verifying its TLS certificate, the default is to now to
   verify that cert.  Thus, any cases where the JWKS was
   retrieved from an untrusted URL will now cause Impala
   to fail to start.

2. The new flag jwks_insecure_tls controls whether or not
   Impala verifies the TLS certificate presented by the
   JWKS server.  It defaults to "false" meaning that the
   certificate will be verified.  Setting this value to
   "true" will restore the previous behavior where
   untrusted TLS certificates are accepted.

3. The new flag jwks_ca_certificate enables specifying
   a pem bundle of certificates to trust when calling to
   the JWKS URL.

Change-Id: I5f1e887fae39b5fb82fa9a40352e4b507b7d8d35
---
M be/src/kudu/util/curl_util.cc
M be/src/kudu/util/curl_util.h
M be/src/rpc/authentication.cc
M be/src/service/impala-server.cc
M be/src/util/jwt-util-internal.h
M be/src/util/jwt-util-test.cc
M be/src/util/jwt-util.cc
M be/src/util/jwt-util.h
M fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java
9 files changed, 200 insertions(+), 56 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/03/19503/1
-- 
To view, visit http://gerrit.cloudera.org:8080/19503
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I5f1e887fae39b5fb82fa9a40352e4b507b7d8d35
Gerrit-Change-Number: 19503
Gerrit-PatchSet: 1
Gerrit-Owner: Jason Fehr <jf...@cloudera.com>

[Impala-ASF-CR] IMPALA-11922 Verify JWKS URL server TLS certificate by default.

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19503 )

Change subject: IMPALA-11922 Verify JWKS URL server TLS certificate by default.
......................................................................


Patch Set 2:

(34 comments)

http://gerrit.cloudera.org:8080/#/c/19503/2/be/src/util/jwt-util.cc
File be/src/util/jwt-util.cc:

http://gerrit.cloudera.org:8080/#/c/19503/2/be/src/util/jwt-util.cc@710
PS2, Line 710:         new_jwks->LoadKeysFromUrl(jwks_uri_, jwks_verify_server_certificate_, jwks_ca_certificate_,
line too long (99 > 90)


http://gerrit.cloudera.org:8080/#/c/19503/2/be/src/util/jwt-util.cc@760
PS2, Line 760:   RETURN_IF_ERROR(jwks_mgr_->Init(jwks_uri, jwks_verify_server_certificate, jwks_ca_certificate,
line too long (96 > 90)


http://gerrit.cloudera.org:8080/#/c/19503/2/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java
File fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java:

http://gerrit.cloudera.org:8080/#/c/19503/2/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@352
PS2, Line 352:     String certDir = setupServerAndRootCerts("testJwtAuthWithInsecureJwksHttpsUrl", "testJwtAuthWithInsecureJwksHttpsUrl Root", "localhostlocalhost");
line too long (150 > 90)


http://gerrit.cloudera.org:8080/#/c/19503/2/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@392
PS2, Line 392:    * 
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/19503/2/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@399
PS2, Line 399:     String certDir = setupServerAndRootCerts("testJwtAuthWithUntrustedJwksHttpsUrl", "testJwtAuthWithUntrustedJwksHttpsUrl Root", "localhost");
line too long (143 > 90)


http://gerrit.cloudera.org:8080/#/c/19503/2/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@439
PS2, Line 439:    * 
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/19503/2/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@447
PS2, Line 447:     String certDir = setupServerAndRootCerts("testJwtAuthWithTrustedJwksHttpsUrlInvalidCN", "testJwtAuthWithTrustedJwksHttpsUrlInvalidCN Root", certCN);
line too long (152 > 90)


http://gerrit.cloudera.org:8080/#/c/19503/2/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@448
PS2, Line 448:     Path logDir = Files.createTempDirectory("testJwtAuthWithTrustedJwksHttpsUrlInvalidCN");
line too long (91 > 90)


http://gerrit.cloudera.org:8080/#/c/19503/2/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@464
PS2, Line 464:         + "error: SSL peer certificate or SSH remote key was not OK: SSL: certificate subject name '%s' does not match target host name '%s'", jwksHttpUrl, certCN, "localhost");
line too long (177 > 90)


http://gerrit.cloudera.org:8080/#/c/19503/2/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@490
PS2, Line 490:     String certDir = setupServerAndRootCerts("testJwtAuthWithTrustedJwksHttpsUrl", "testJwtAuthWithTrustedJwksHttpsUrl Root", "localhost");
line too long (139 > 90)


http://gerrit.cloudera.org:8080/#/c/19503/2/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@518
PS2, Line 518:   private String setupServerAndRootCerts(String testName, String rootCaCertCN, String rootLeafCertCN) throws Exception {
line too long (120 > 90)


http://gerrit.cloudera.org:8080/#/c/19503/2/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java
File fe/src/test/java/org/apache/impala/testutil/X509CertChain.java:

http://gerrit.cloudera.org:8080/#/c/19503/2/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@51
PS2, Line 51:   
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/19503/2/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@53
PS2, Line 53:   private static final AlgorithmIdentifier sha256WithRSA = new AlgorithmIdentifier(PKCSObjectIdentifiers.sha256WithRSAEncryption, DERNull.INSTANCE);
line too long (148 > 90)


http://gerrit.cloudera.org:8080/#/c/19503/2/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@54
PS2, Line 54:   
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/19503/2/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@55
PS2, Line 55:   private static final KeyUsage certSignKeyUsage = new KeyUsage(KeyUsage.keyCertSign | KeyUsage.cRLSign);
line too long (105 > 90)


http://gerrit.cloudera.org:8080/#/c/19503/2/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@56
PS2, Line 56:   private static final KeyUsage serverAuthKeyUsage = new KeyUsage(KeyUsage.digitalSignature | KeyUsage.keyEncipherment);
line too long (120 > 90)


http://gerrit.cloudera.org:8080/#/c/19503/2/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@63
PS2, Line 63:   
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/19503/2/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@64
PS2, Line 64:   public X509CertChain(String rootCaCertCN, String rootLeafCertCN)  throws NoSuchAlgorithmException, NoSuchProviderException, InvalidKeyException, SignatureException, IOException, CertificateException  {
line too long (203 > 90)


http://gerrit.cloudera.org:8080/#/c/19503/2/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@73
PS2, Line 73:     leafCert = generateLeafCert(rootLeafCertCN, this.leafKp, this.rootCert, this.rootCaKp.getPrivate());
line too long (104 > 90)


http://gerrit.cloudera.org:8080/#/c/19503/2/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@100
PS2, Line 100:   public void writeRootCertAsPem(Writer w) throws CertificateEncodingException, IOException {
line too long (93 > 90)


http://gerrit.cloudera.org:8080/#/c/19503/2/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@103
PS2, Line 103:   
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/19503/2/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@110
PS2, Line 110:   public void writeLeafCertAsPem(Writer w) throws CertificateEncodingException, IOException {
line too long (93 > 90)


http://gerrit.cloudera.org:8080/#/c/19503/2/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@117
PS2, Line 117:    * 
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/19503/2/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@131
PS2, Line 131:   
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/19503/2/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@136
PS2, Line 136:   private X509Certificate generateRootCACert(String commonName, KeyPair kp) throws NoSuchAlgorithmException, NoSuchProviderException, InvalidKeyException, SignatureException, IOException, CertificateException {
line too long (210 > 90)


http://gerrit.cloudera.org:8080/#/c/19503/2/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@168
PS2, Line 168:     return (X509Certificate)CertificateFactory.getInstance("X.509", "BC").generateCertificate(new ByteArrayInputStream(new DERSequence(v).getEncoded(ASN1Encoding.DER)));
line too long (169 > 90)


http://gerrit.cloudera.org:8080/#/c/19503/2/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@171
PS2, Line 171:   private X509Certificate generateLeafCert(String commonName, KeyPair kp, X509Certificate issuerCert, PrivateKey issuerPrivateKey) throws IOException, NoSuchAlgorithmException, NoSuchProviderException, InvalidKeyException, SignatureException, CertificateException {
line too long (265 > 90)


http://gerrit.cloudera.org:8080/#/c/19503/2/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@183
PS2, Line 183:     extGenerator.addExtension(Extension.extendedKeyUsage, false, new ExtendedKeyUsage(new KeyPurposeId[]{KeyPurposeId.id_kp_serverAuth, KeyPurposeId.id_kp_clientAuth}));
line too long (169 > 90)


http://gerrit.cloudera.org:8080/#/c/19503/2/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@184
PS2, Line 184:     
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/19503/2/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@193
PS2, Line 193:     
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/19503/2/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@206
PS2, Line 206:     return (java.security.cert.X509Certificate)CertificateFactory.getInstance("X.509", "BC").generateCertificate(new ByteArrayInputStream(new DERSequence(v).getEncoded(ASN1Encoding.DER)));
line too long (188 > 90)


http://gerrit.cloudera.org:8080/#/c/19503/2/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@209
PS2, Line 209:   private void certToPem(X509Certificate cert, Writer writer) throws IOException, CertificateEncodingException {
line too long (112 > 90)


http://gerrit.cloudera.org:8080/#/c/19503/2/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@216
PS2, Line 216:   private String certToPem(X509Certificate cert) throws IOException, CertificateEncodingException {
line too long (99 > 90)


http://gerrit.cloudera.org:8080/#/c/19503/2/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@223
PS2, Line 223:   
line has trailing whitespace



-- 
To view, visit http://gerrit.cloudera.org:8080/19503
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5f1e887fae39b5fb82fa9a40352e4b507b7d8d35
Gerrit-Change-Number: 19503
Gerrit-PatchSet: 2
Gerrit-Owner: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Feb 2023 02:38:30 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11922 Verify JWKS URL server TLS certificate by default.

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19503 )

Change subject: IMPALA-11922 Verify JWKS URL server TLS certificate by default.
......................................................................


Patch Set 2:

Build Failed 

https://jenkins.impala.io/job/gerrit-code-review-checks/12400/ : Initial code review checks failed. See linked job for details on the failure.


-- 
To view, visit http://gerrit.cloudera.org:8080/19503
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5f1e887fae39b5fb82fa9a40352e4b507b7d8d35
Gerrit-Change-Number: 19503
Gerrit-PatchSet: 2
Gerrit-Owner: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Feb 2023 02:57:49 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11922 Verify JWKS URL server TLS certificate by default.

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19503 )

Change subject: IMPALA-11922 Verify JWKS URL server TLS certificate by default.
......................................................................


Patch Set 3:

Build Failed 

https://jenkins.impala.io/job/gerrit-code-review-checks/12401/ : Initial code review checks failed. See linked job for details on the failure.


-- 
To view, visit http://gerrit.cloudera.org:8080/19503
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5f1e887fae39b5fb82fa9a40352e4b507b7d8d35
Gerrit-Change-Number: 19503
Gerrit-PatchSet: 3
Gerrit-Owner: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Feb 2023 02:59:34 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11922 Verify JWKS URL server TLS certificate by default.

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19503 )

Change subject: IMPALA-11922 Verify JWKS URL server TLS certificate by default.
......................................................................


Patch Set 1:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/12382/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/19503
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5f1e887fae39b5fb82fa9a40352e4b507b7d8d35
Gerrit-Change-Number: 19503
Gerrit-PatchSet: 1
Gerrit-Owner: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Wed, 15 Feb 2023 23:14:44 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11922 Verify JWKS URL server TLS certificate by default.

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19503 )

Change subject: IMPALA-11922 Verify JWKS URL server TLS certificate by default.
......................................................................


Patch Set 3:

(12 comments)

http://gerrit.cloudera.org:8080/#/c/19503/3/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java
File fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java:

http://gerrit.cloudera.org:8080/#/c/19503/3/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@393
PS3, Line 393:    * 
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/19503/3/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@441
PS3, Line 441:    * 
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/19503/3/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@452
PS3, Line 452:     Path logDir = Files.createTempDirectory("testJwtAuthWithTrustedJwksHttpsUrlInvalidCN");
line too long (91 > 90)


http://gerrit.cloudera.org:8080/#/c/19503/3/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java
File fe/src/test/java/org/apache/impala/testutil/X509CertChain.java:

http://gerrit.cloudera.org:8080/#/c/19503/3/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@51
PS3, Line 51:   
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/19503/3/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@56
PS3, Line 56:   
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/19503/3/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@67
PS3, Line 67:   
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/19503/3/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@111
PS3, Line 111:   
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/19503/3/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@126
PS3, Line 126:    * 
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/19503/3/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@140
PS3, Line 140:   
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/19503/3/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@202
PS3, Line 202:     
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/19503/3/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@211
PS3, Line 211:     
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/19503/3/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@245
PS3, Line 245:   
line has trailing whitespace



-- 
To view, visit http://gerrit.cloudera.org:8080/19503
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5f1e887fae39b5fb82fa9a40352e4b507b7d8d35
Gerrit-Change-Number: 19503
Gerrit-PatchSet: 3
Gerrit-Owner: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Feb 2023 02:39:17 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11922 Verify JWKS URL server TLS certificate by default.

Posted by "Jason Fehr (Code Review)" <ge...@cloudera.org>.
Jason Fehr has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/19503 )

Change subject: IMPALA-11922 Verify JWKS URL server TLS certificate by default.
......................................................................

IMPALA-11922 Verify JWKS URL server TLS certificate by default.

**** BREAKING CHANGE ****

JWT Auth has an option to specify the location of the
JSON Web Key Set (JWKS) using a URL. If that URL is
accessed over HTTPS, the TLS certificate presented by the
server is not verified.

This means that Impala only requires the server to return
a TLS certificate, whether or not Impala trusts the signing
certificate chain.

The implications of this setup is that a fully secure chain
of trust cannot be established throughout the entire JWT
authentication lifecycle and thus creates an attack vector
where a bad actor could trick Impala into trusting an
actor-controlled JWKS. The bad actor can then generate
a JWT with any claims they chose and Impala will accept it.

This change introduces:
  1. verification of JWKS server TLS certificate by default
  2. jwks_insecure_tls Impala startup flag
  3. jwks_ca_certificate Impala startup flag

1. While previously, the JWKS URL was always called without
   verifying its TLS certificate, the default is to now to
   verify that cert.  Thus, any cases where the JWKS was
   retrieved from an untrusted URL will now cause Impala
   to fail to start.

2. The new flag jwks_insecure_tls controls whether or not
   Impala verifies the TLS certificate presented by the
   JWKS server.  It defaults to "false" meaning that the
   certificate will be verified.  Setting this value to
   "true" will restore the previous behavior where
   untrusted TLS certificates are accepted.

3. The new flag jwks_ca_certificate enables specifying
   a PEM certificate bundle that contains certificates
   to trust when calling to the JWKS URL.

Testing was achieved in the front-end Java custom cluster
tests.  An existing test was modified and two new tests
were created.  The following test cases are covered:
  1. Insecurely retrieve a JWKS from a server with an
     untrusted TLS certificate.  This test case is expected
     to pass.
  2. Securely retrieve a JWKS from a server with an
     untrusted TLS certificate.  This test case is expected
     to fail.  The Impala corodinator logs are checked to
     ensure the cause was an untrusted certificate
     presented by the JWKS server.
  3. Securely retrieve a JWKS from a server with a trusted
     TLS certificate.  This test case is expected to pass.

Change-Id: I5f1e887fae39b5fb82fa9a40352e4b507b7d8d35
---
M be/src/kudu/util/curl_util.cc
M be/src/kudu/util/curl_util.h
M be/src/rpc/authentication.cc
M be/src/service/impala-server.cc
M be/src/util/jwt-util-internal.h
M be/src/util/jwt-util-test.cc
M be/src/util/jwt-util.cc
M be/src/util/jwt-util.h
M fe/pom.xml
M fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java
A fe/src/test/java/org/apache/impala/testutil/X509CertChain.java
11 files changed, 533 insertions(+), 54 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/03/19503/2
-- 
To view, visit http://gerrit.cloudera.org:8080/19503
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5f1e887fae39b5fb82fa9a40352e4b507b7d8d35
Gerrit-Change-Number: 19503
Gerrit-PatchSet: 2
Gerrit-Owner: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>

[Impala-ASF-CR] IMPALA-11922 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/19503 )

Change subject: IMPALA-11922 Verify JWKS URL server TLS certificate by default.
......................................................................


Patch Set 7: Code-Review+1


-- 
To view, visit http://gerrit.cloudera.org:8080/19503
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5f1e887fae39b5fb82fa9a40352e4b507b7d8d35
Gerrit-Change-Number: 19503
Gerrit-PatchSet: 7
Gerrit-Owner: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Feb 2023 23:01:22 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11922 Verify JWKS URL server TLS certificate by default.

Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/19503 )

Change subject: IMPALA-11922 Verify JWKS URL server TLS certificate by default.
......................................................................


Patch Set 6:

(11 comments)

Mostly comments about comments

http://gerrit.cloudera.org:8080/#/c/19503/6//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/19503/6//COMMIT_MSG@9
PS6, Line 9: **** BREAKING CHANGE ****
Maybe a summary of what is breaking would be useful here.


http://gerrit.cloudera.org:8080/#/c/19503/6//COMMIT_MSG@38
PS6, Line 38: 2. The new flag jwks_insecure_tls controls whether or not
Maybe a more descriptive name would be useful? Just insecure_tls seems very general.
jwks_allow_unverified_certificate


http://gerrit.cloudera.org:8080/#/c/19503/6/be/src/kudu/util/curl_util.cc
File be/src/kudu/util/curl_util.cc:

http://gerrit.cloudera.org:8080/#/c/19503/6/be/src/kudu/util/curl_util.cc@89
PS6, Line 89:   // Ensure the curl error buffer is large enough
Nit: add period


http://gerrit.cloudera.org:8080/#/c/19503/6/be/src/kudu/util/curl_util.cc@116
PS6, Line 116:   // across calls
Nit: add period


http://gerrit.cloudera.org:8080/#/c/19503/6/be/src/rpc/authentication.cc
File be/src/rpc/authentication.cc:

http://gerrit.cloudera.org:8080/#/c/19503/6/be/src/rpc/authentication.cc@174
PS6, Line 174:     "trust chain can be established for it and if the certificate has a common name or "
Nit: "it,"


http://gerrit.cloudera.org:8080/#/c/19503/6/be/src/rpc/authentication.cc@175
PS6, Line 175:     "SAN that matches the server's hostname.  This should only be set to false for "
Nit: double space


http://gerrit.cloudera.org:8080/#/c/19503/6/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java
File fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java:

http://gerrit.cloudera.org:8080/#/c/19503/6/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@414
PS6, Line 414:     List<String> logLines;
Nit: move to where it is used, like
List<String> logLines = Files.readAllLines(logDir.resolve("impalad.ERROR"));


http://gerrit.cloudera.org:8080/#/c/19503/6/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@465
PS6, Line 465:     List<String> logLines;
Move to where used, as above?


http://gerrit.cloudera.org:8080/#/c/19503/6/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@479
PS6, Line 479:     for (Iterator<String> i = logLines.iterator(); i.hasNext();) {
Can we say 
assertTrue(..., logLines.contains(expectedErrString)) 
?


http://gerrit.cloudera.org:8080/#/c/19503/6/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java
File fe/src/test/java/org/apache/impala/testutil/X509CertChain.java:

http://gerrit.cloudera.org:8080/#/c/19503/6/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@103
PS6, Line 103:    * @return String
Maybe delete these lines, they don't really add much


http://gerrit.cloudera.org:8080/#/c/19503/6/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@179
PS6, Line 179:     gen.setEndDate(new Time(new Date(System.currentTimeMillis() + 60 * 60 * 1000)));
Maybe comment the meaning of these dates



-- 
To view, visit http://gerrit.cloudera.org:8080/19503
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5f1e887fae39b5fb82fa9a40352e4b507b7d8d35
Gerrit-Change-Number: 19503
Gerrit-PatchSet: 6
Gerrit-Owner: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Feb 2023 19:02:55 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11922 Verify JWKS URL server TLS certificate by default.

Posted by "Jason Fehr (Code Review)" <ge...@cloudera.org>.
Jason Fehr has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/19503 )

Change subject: IMPALA-11922 Verify JWKS URL server TLS certificate by default.
......................................................................

IMPALA-11922 Verify JWKS URL server TLS certificate by default.

**** BREAKING CHANGE ****

JWT Auth has an option to specify the location of the
JSON Web Key Set (JWKS) using a URL. If that URL is
accessed over HTTPS, the TLS certificate presented by the
server is not verified.

This means that Impala only requires the server to return
a TLS certificate, whether or not Impala trusts the signing
certificate chain.

The implications of this setup is that a fully secure chain
of trust cannot be established throughout the entire JWT
authentication lifecycle and thus creates an attack vector
where a bad actor could trick Impala into trusting an
actor-controlled JWKS. The bad actor can then generate
a JWT with any claims they chose and Impala will accept it.

This change introduces:
  1. verification of JWKS server TLS certificate by default
  2. jwks_insecure_tls Impala startup flag
  3. jwks_ca_certificate Impala startup flag

1. While previously, the JWKS URL was always called without
   verifying its TLS certificate, the default is to now to
   verify that cert. Thus, any cases where the JWKS was
   retrieved from an untrusted URL will now cause Impala
   to fail to start.

2. The new flag jwks_insecure_tls controls whether or not
   Impala verifies the TLS certificate presented by the
   JWKS server. It defaults to "false" meaning that the
   certificate will be verified. Setting this value to
   "true" will restore the previous behavior where
   untrusted TLS certificates are accepted.

3. The new flag jwks_ca_certificate enables specifying
   a PEM certificate bundle that contains certificates
   to trust when calling to the JWKS URL.

Testing was achieved in the front-end Java custom cluster
tests. An existing test was modified and three new tests
were created. The following test cases are covered:
  1. Insecurely retrieve a JWKS from a server with an
     untrusted TLS certificate. This test case is expected
     to pass.
  2. Securely retrieve a JWKS from a server with an
     untrusted TLS certificate. This test case is expected
     to fail. The Impala coordinator logs are checked to
     ensure the cause was an untrusted certificate
     presented by the JWKS server.
  3. Retrieve a JWKS from a server where the root CA is
     trusted, but the cert contains the wrong CN. This
     test is expected to fail. The Impala logs are checked
     to ensure the cause was a certificate with an
     incorrect CN.
  4. Securely retrieve a JWKS from a server with a trusted
     TLS certificate. This test case is expected to pass.

Change-Id: I5f1e887fae39b5fb82fa9a40352e4b507b7d8d35
---
M be/src/kudu/util/curl_util.cc
M be/src/kudu/util/curl_util.h
M be/src/rpc/authentication.cc
M be/src/service/impala-server.cc
M be/src/util/jwt-util-internal.h
M be/src/util/jwt-util-test.cc
M be/src/util/jwt-util.cc
M be/src/util/jwt-util.h
M fe/pom.xml
M fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java
A fe/src/test/java/org/apache/impala/testutil/X509CertChain.java
11 files changed, 564 insertions(+), 54 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/03/19503/5
-- 
To view, visit http://gerrit.cloudera.org:8080/19503
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5f1e887fae39b5fb82fa9a40352e4b507b7d8d35
Gerrit-Change-Number: 19503
Gerrit-PatchSet: 5
Gerrit-Owner: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>

[Impala-ASF-CR] IMPALA-11922 Verify JWKS URL server TLS certificate by default.

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19503 )

Change subject: IMPALA-11922 Verify JWKS URL server TLS certificate by default.
......................................................................


Patch Set 6:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/12409/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/19503
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5f1e887fae39b5fb82fa9a40352e4b507b7d8d35
Gerrit-Change-Number: 19503
Gerrit-PatchSet: 6
Gerrit-Owner: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Feb 2023 17:49:50 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11922 Verify JWKS URL server TLS certificate by default.

Posted by "Jason Fehr (Code Review)" <ge...@cloudera.org>.
Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/19503 )

Change subject: IMPALA-11922 Verify JWKS URL server TLS certificate by default.
......................................................................


Patch Set 7:

(14 comments)

addressed additional comments, thanks to all the reviewers who took time to review!

http://gerrit.cloudera.org:8080/#/c/19503/6//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/19503/6//COMMIT_MSG@9
PS6, Line 9: **** BREAKING CHANGE ****
> Maybe a summary of what is breaking would be useful here.
Done


http://gerrit.cloudera.org:8080/#/c/19503/6//COMMIT_MSG@29
PS6, Line 29:  with any claims 
> this flag is already renamed
Done


http://gerrit.cloudera.org:8080/#/c/19503/6//COMMIT_MSG@38
PS6, Line 38:    verify that cert. Thus, any cases where the JWKS was
> Maybe a more descriptive name would be useful? Just insecure_tls seems very
Updated to match the new impala startup flag.


http://gerrit.cloudera.org:8080/#/c/19503/6/be/src/kudu/util/curl_util.cc
File be/src/kudu/util/curl_util.cc:

http://gerrit.cloudera.org:8080/#/c/19503/6/be/src/kudu/util/curl_util.cc@89
PS6, Line 89:   // Ensure the curl error buffer is large enough.
> Nit: add period
Done


http://gerrit.cloudera.org:8080/#/c/19503/6/be/src/kudu/util/curl_util.cc@116
PS6, Line 116:   // across calls.
> Nit: add period
Done


http://gerrit.cloudera.org:8080/#/c/19503/6/be/src/rpc/authentication.cc
File be/src/rpc/authentication.cc:

http://gerrit.cloudera.org:8080/#/c/19503/6/be/src/rpc/authentication.cc@174
PS6, Line 174:     "trust chain can be established for it, and if the certificate has a common name or "
> Nit: "it,"
Done


http://gerrit.cloudera.org:8080/#/c/19503/6/be/src/rpc/authentication.cc@175
PS6, Line 175:     "SAN that matches the server's hostname. This should only be set to false for "
> Nit: double space
Done


http://gerrit.cloudera.org:8080/#/c/19503/6/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java
File fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java:

http://gerrit.cloudera.org:8080/#/c/19503/6/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@55
PS6, Line 55: tring SERVER_KEY
> name constant variables with all upper case characters
Deleted since they are not used.


http://gerrit.cloudera.org:8080/#/c/19503/6/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@414
PS6, Line 414:         + "problem: unable to get local issuer certificate", jwksHttpUrl);
> Nit: move to where it is used, like
Done


http://gerrit.cloudera.org:8080/#/c/19503/6/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@465
PS6, Line 465:     // check in the impalad logs that the server startup failed for the expected reason
> Move to where used, as above?
Done


http://gerrit.cloudera.org:8080/#/c/19503/6/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@479
PS6, Line 479:     String certDir = setupServerAndRootCerts("testJwtAuthWithTrustedJwksHttpsUrl",
> Can we say 
Yes, leveraging a couple of hamcrest matchers to figure out if the log lines contains an item that contains the expected string within that item.


http://gerrit.cloudera.org:8080/#/c/19503/6/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java
File fe/src/test/java/org/apache/impala/testutil/X509CertChain.java:

http://gerrit.cloudera.org:8080/#/c/19503/6/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@80
PS6, Line 80: private final KeyPair rootCaKp_;
            :   private final KeyPair leafKp_;
            :   private final X509Certificate rootCert_;
            :   private final X509Certificate leafCert_
> follow the coding style to name the member variables with last character as
Done


http://gerrit.cloudera.org:8080/#/c/19503/6/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@103
PS6, Line 103:   public String rootCertAsPemString() throws CertificateEncodingException, IOException {
> Maybe delete these lines, they don't really add much
Done


http://gerrit.cloudera.org:8080/#/c/19503/6/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@179
PS6, Line 179:     gen.setSubject(cn);
> Maybe comment the meaning of these dates
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/19503
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5f1e887fae39b5fb82fa9a40352e4b507b7d8d35
Gerrit-Change-Number: 19503
Gerrit-PatchSet: 7
Gerrit-Owner: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Feb 2023 22:49:34 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11922 Verify JWKS URL server TLS certificate by default.

Posted by "Jason Fehr (Code Review)" <ge...@cloudera.org>.
Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/19503 )

Change subject: IMPALA-11922 Verify JWKS URL server TLS certificate by default.
......................................................................


Patch Set 3:

(19 comments)

Addressed comments.

http://gerrit.cloudera.org:8080/#/c/19503/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/19503/1//COMMIT_MSG@12
PS1, Line 12: I
> nit: one extra space
Done


http://gerrit.cloudera.org:8080/#/c/19503/1//COMMIT_MSG@13
PS1, Line 13: HTTPS
> nit: HTTPS
Done


http://gerrit.cloudera.org:8080/#/c/19503/1//COMMIT_MSG@24
PS1, Line 24: T
> nit: one extra space
Done


http://gerrit.cloudera.org:8080/#/c/19503/1//COMMIT_MSG@46
PS1, Line 46: PEM
> nit: PEM
Done


http://gerrit.cloudera.org:8080/#/c/19503/1//COMMIT_MSG@46
PS1, Line 46: e bundle tha
> Should those be CA certificates or non-CA certs (e.g., exact TLS server cer
It's valid to trust both CA and non-CA certificates.  All the curl lib needs to do is establish a chain of certificates between the leaf cert presented by the server and a certificate it trusts.  If curl trusts the certificate presented by the server, then a chain of trust has been established.


http://gerrit.cloudera.org:8080/#/c/19503/1//COMMIT_MSG@48
PS1, Line 48: 
> add a Testing section
Done


http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/kudu/util/curl_util.h
File be/src/kudu/util/curl_util.h:

http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/kudu/util/curl_util.h@74
PS1, Line 74: certificates
> Are these should be CA certificates or non-CA certs are also accepted?
It can be either.  I updated the comment to reflect this.


http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/kudu/util/curl_util.h@75
PS1, Line 75: CA ce
> nit: HTTPS
Done


http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/kudu/util/curl_util.cc
File be/src/kudu/util/curl_util.cc:

http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/kudu/util/curl_util.cc@122
PS1, Line 122: CHECK_EQ
> Does it make sense to switch to using CURL_RETURN_NOT_OK() here instead?
I copied this code from line 92-93.  I'm very new to Impala, so I don't have a good understanding of the difference between CHECK_EQ on the return code or CURL_RETURN_NOT_OK.  What are the differences?


http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/rpc/authentication.cc
File be/src/rpc/authentication.cc:

http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/rpc/authentication.cc@171
PS1, Line 171: jwks_verify_serve
> nit: 'jwks_insecure_tls' sounds a bit vague to me: it might be authenticati
I modeled this flag after the curl command line utility, but I like your suggestion better.  I'm going to leave out the "tls" part because that is implied when working with server certs.


http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/rpc/authentication.cc@1418
PS1, Line 1418:       return Status(
> Check jwks_ca_certificate is not empty if jwks_insecure_tls is set as false
If jwks_insecure_tls is set to false and jwks_ca_certificate is empty, then Impala will use the system trust store.  Most of the time (including CDP production), the system trust store will be enough.


http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/util/jwt-util-internal.h
File be/src/util/jwt-util-internal.h:

http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/util/jwt-util-internal.h@374
PS1, Line 374: of certs
> nit: what exactly this 'trust' covers?  Is this just to verify authenticity
I modified this comment a little bit in an attempt to clarify.  It's only used when retrieving the JWKS.  We do need to be clear since there are multiple places throughout Impala where a PEM certificate bundle could be used as a trust store.


http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/util/jwt-util.h
File be/src/util/jwt-util.h:

http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/util/jwt-util.h@59
PS1, Line 59: const std::string& jwks_file_path
> Does it make sense to make this a parameter of one of the constructors for 
I based my changes off the pattern that was already established.  Looking at the code, JWTHelper is a static singleton that gets instantiated during application startup: https://github.com/apache/impala/blob/feb4a76ed4cb5b688143eb21370f78ec93133c56/be/src/util/jwt-util.cc#L739

At this point in my C++ experience, I don't feel comfortable taking on a refactor of this nature.


http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/util/jwt-util.h@63
PS1, Line 63: Init
> ditto
see previous comment


http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/util/jwt-util.h@64
PS1, Line 64: bool is_local_file
> do we still need this variable?
Yeah.  If is_local_file is true, then the JWKS is read from the local filesystem instead of from a URL.


http://gerrit.cloudera.org:8080/#/c/19503/1/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java
File fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java:

http://gerrit.cloudera.org:8080/#/c/19503/1/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@83
PS1, Line 83: 
> for ?
ha ha, yes.


http://gerrit.cloudera.org:8080/#/c/19503/1/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@397
PS1, Line 397: 
> It's better to give a certificate which does not match the certificate retu
Good point.  Really we need to test both cases:
1. cert trust chain cannot be established but the cert CN and server hostname matches
2. cert trust chain is established but the cert CN and server hostname do not match

I added test cases for each situation.


http://gerrit.cloudera.org:8080/#/c/19503/1/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@420
PS1, Line 420: 
> nit: extra spaces
Done


http://gerrit.cloudera.org:8080/#/c/19503/1/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@440
PS1, Line 440: JWKS server is n
> Is this certificate also has the CA capability?  If not, I'm a bit surprise
The expected use case in the wild would be a trusted root CA.  Then, the server would return its leaf cert (where the server hostname matches the cert CN/SAN) and any intermediates necessary to established a trust chain back to the root.

Wenzhe made a comment up on line 397 that prompted me to rethink the test cases.  I realized I combined two test cases into one, and I need to split them apart.  Once I do that, this concern will be addressed.



-- 
To view, visit http://gerrit.cloudera.org:8080/19503
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5f1e887fae39b5fb82fa9a40352e4b507b7d8d35
Gerrit-Change-Number: 19503
Gerrit-PatchSet: 3
Gerrit-Owner: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Feb 2023 02:37:55 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11922 Verify JWKS URL server TLS certificate by default.

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19503 )

Change subject: IMPALA-11922 Verify JWKS URL server TLS certificate by default.
......................................................................


Patch Set 5:

Build Failed 

https://jenkins.impala.io/job/gerrit-code-review-checks/12403/ : Initial code review checks failed. See linked job for details on the failure.


-- 
To view, visit http://gerrit.cloudera.org:8080/19503
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5f1e887fae39b5fb82fa9a40352e4b507b7d8d35
Gerrit-Change-Number: 19503
Gerrit-PatchSet: 5
Gerrit-Owner: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Feb 2023 04:09:41 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11922 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/19503 )

Change subject: IMPALA-11922 Verify JWKS URL server TLS certificate by default.
......................................................................


Patch Set 8: Code-Review+1


-- 
To view, visit http://gerrit.cloudera.org:8080/19503
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5f1e887fae39b5fb82fa9a40352e4b507b7d8d35
Gerrit-Change-Number: 19503
Gerrit-PatchSet: 8
Gerrit-Owner: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Feb 2023 18:29:12 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11922 Verify JWKS URL server TLS certificate by default.

Posted by "Jason Fehr (Code Review)" <ge...@cloudera.org>.
Jason Fehr has uploaded a new patch set (#8). ( http://gerrit.cloudera.org:8080/19503 )

Change subject: IMPALA-11922 Verify JWKS URL server TLS certificate by default.
......................................................................

IMPALA-11922 Verify JWKS URL server TLS certificate by default.

**** BREAKING CHANGE ****
If using JWT authentication to the Impala engine and the
JWKS is retrieved from a URL, Impala now verifies the
server's TLS certificate.  Before, Impala did not verify
the trust chain nor did it verify the CN/SAN.

JWT Auth has an option to specify the location of the
JSON Web Key Set (JWKS) using a URL. If that URL is
accessed over HTTPS, the TLS certificate presented by the
server is not verified.

This means that Impala only requires the server to return
a TLS certificate, whether or not Impala trusts the signing
certificate chain.

The implications of this setup is that a fully secure chain
of trust cannot be established throughout the entire JWT
authentication lifecycle and thus creates an attack vector
where a bad actor could trick Impala into trusting an
actor-controlled JWKS. The bad actor can then generate
a JWT with any claims they chose and Impala will accept it.

This change introduces:
  1. verification of JWKS server TLS certificate by default
  2. jwks_verify_server_certificate Impala startup flag
  3. jwks_ca_certificate Impala startup flag

1. While previously, the JWKS URL was always called without
   verifying its TLS certificate, the default is to now to
   verify that cert. Thus, any cases where the JWKS was
   retrieved from an untrusted URL will now cause Impala
   to fail to start.

2. The new flag jwks_verify_server_certificate controls
   whether or not Impala verifies the TLS certificate
   presented by the JWKS server. It defaults to "false"
   meaning that the certificate will be verified. Setting
   this value to "false" will restore the previous behavior
   where untrusted TLS certificates are accepted.

3. The new flag jwks_ca_certificate enables specifying
   a PEM certificate bundle that contains certificates
   to trust when calling to the JWKS URL.

Testing was achieved in the front-end Java custom cluster
tests. An existing test was modified and three new tests
were created. The following test cases are covered:
  1. Insecurely retrieve a JWKS from a server with an
     untrusted TLS certificate. This test case is expected
     to pass.
  2. Securely retrieve a JWKS from a server with an
     untrusted TLS certificate. This test case is expected
     to fail. The Impala coordinator logs are checked to
     ensure the cause was an untrusted certificate
     presented by the JWKS server.
  3. Retrieve a JWKS from a server where the root CA is
     trusted, but the cert contains the wrong CN. This
     test is expected to fail. The Impala logs are checked
     to ensure the cause was a certificate with an
     incorrect CN.
  4. Securely retrieve a JWKS from a server with a trusted
     TLS certificate. This test case is expected to pass.

Change-Id: I5f1e887fae39b5fb82fa9a40352e4b507b7d8d35
---
M be/src/kudu/util/curl_util.cc
M be/src/kudu/util/curl_util.h
M be/src/rpc/authentication.cc
M be/src/service/impala-server.cc
M be/src/util/jwt-util-internal.h
M be/src/util/jwt-util-test.cc
M be/src/util/jwt-util.cc
M be/src/util/jwt-util.h
M fe/pom.xml
M fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java
A fe/src/test/java/org/apache/impala/testutil/X509CertChain.java
11 files changed, 563 insertions(+), 54 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/03/19503/8
-- 
To view, visit http://gerrit.cloudera.org:8080/19503
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5f1e887fae39b5fb82fa9a40352e4b507b7d8d35
Gerrit-Change-Number: 19503
Gerrit-PatchSet: 8
Gerrit-Owner: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>

[Impala-ASF-CR] IMPALA-11922 Verify JWKS URL server TLS certificate by default.

Posted by "Jason Fehr (Code Review)" <ge...@cloudera.org>.
Jason Fehr has uploaded a new patch set (#7). ( http://gerrit.cloudera.org:8080/19503 )

Change subject: IMPALA-11922 Verify JWKS URL server TLS certificate by default.
......................................................................

IMPALA-11922 Verify JWKS URL server TLS certificate by default.

**** BREAKING CHANGE ****
If using JWT authentication to the Impala engine and the
JWKS is retrieved from a URL, Impala now verifies the
server's TLS certificate.  Before, Impala did not verify
the trust chain nor did it verify the CN/SAN.

JWT Auth has an option to specify the location of the
JSON Web Key Set (JWKS) using a URL. If that URL is
accessed over HTTPS, the TLS certificate presented by the
server is not verified.

This means that Impala only requires the server to return
a TLS certificate, whether or not Impala trusts the signing
certificate chain.

The implications of this setup is that a fully secure chain
of trust cannot be established throughout the entire JWT
authentication lifecycle and thus creates an attack vector
where a bad actor could trick Impala into trusting an
actor-controlled JWKS. The bad actor can then generate
a JWT with any claims they chose and Impala will accept it.

This change introduces:
  1. verification of JWKS server TLS certificate by default
  2. jwks_verify_server_certificate Impala startup flag
  3. jwks_ca_certificate Impala startup flag

1. While previously, the JWKS URL was always called without
   verifying its TLS certificate, the default is to now to
   verify that cert. Thus, any cases where the JWKS was
   retrieved from an untrusted URL will now cause Impala
   to fail to start.

2. The new flag jwks_verify_server_certificate controls
   whether or not Impala verifies the TLS certificate
   presented by the JWKS server. It defaults to "false"
   meaning that the certificate will be verified. Setting
   this value to "false" will restore the previous behavior
   where untrusted TLS certificates are accepted.

3. The new flag jwks_ca_certificate enables specifying
   a PEM certificate bundle that contains certificates
   to trust when calling to the JWKS URL.

Testing was achieved in the front-end Java custom cluster
tests. An existing test was modified and three new tests
were created. The following test cases are covered:
  1. Insecurely retrieve a JWKS from a server with an
     untrusted TLS certificate. This test case is expected
     to pass.
  2. Securely retrieve a JWKS from a server with an
     untrusted TLS certificate. This test case is expected
     to fail. The Impala coordinator logs are checked to
     ensure the cause was an untrusted certificate
     presented by the JWKS server.
  3. Retrieve a JWKS from a server where the root CA is
     trusted, but the cert contains the wrong CN. This
     test is expected to fail. The Impala logs are checked
     to ensure the cause was a certificate with an
     incorrect CN.
  4. Securely retrieve a JWKS from a server with a trusted
     TLS certificate. This test case is expected to pass.

Change-Id: I5f1e887fae39b5fb82fa9a40352e4b507b7d8d35
---
M be/src/kudu/util/curl_util.cc
M be/src/kudu/util/curl_util.h
M be/src/rpc/authentication.cc
M be/src/service/impala-server.cc
M be/src/util/jwt-util-internal.h
M be/src/util/jwt-util-test.cc
M be/src/util/jwt-util.cc
M be/src/util/jwt-util.h
M fe/pom.xml
M fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java
A fe/src/test/java/org/apache/impala/testutil/X509CertChain.java
11 files changed, 566 insertions(+), 54 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/03/19503/7
-- 
To view, visit http://gerrit.cloudera.org:8080/19503
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5f1e887fae39b5fb82fa9a40352e4b507b7d8d35
Gerrit-Change-Number: 19503
Gerrit-PatchSet: 7
Gerrit-Owner: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>

[Impala-ASF-CR] IMPALA-11922 Verify JWKS URL server TLS certificate by default.

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19503 )

Change subject: IMPALA-11922 Verify JWKS URL server TLS certificate by default.
......................................................................


Patch Set 7:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/12412/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/19503
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5f1e887fae39b5fb82fa9a40352e4b507b7d8d35
Gerrit-Change-Number: 19503
Gerrit-PatchSet: 7
Gerrit-Owner: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Feb 2023 23:09:18 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11922 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/19503 )

Change subject: IMPALA-11922 Verify JWKS URL server TLS certificate by default.
......................................................................


Patch Set 1:

(7 comments)

Thanks to work on this and added certificate verification to kudu::EasyCurl. curl_util.h/curl_util.cc were synced from Kudu repo, we need to port these changes to Kudu repo later.

http://gerrit.cloudera.org:8080/#/c/19503/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/19503/1//COMMIT_MSG@12
PS1, Line 12:  
nit: one extra space


http://gerrit.cloudera.org:8080/#/c/19503/1//COMMIT_MSG@24
PS1, Line 24:  
nit: one extra space


http://gerrit.cloudera.org:8080/#/c/19503/1//COMMIT_MSG@48
PS1, Line 48: 
add a Testing section


http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/rpc/authentication.cc
File be/src/rpc/authentication.cc:

http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/rpc/authentication.cc@1418
PS1, Line 1418:     }
Check jwks_ca_certificate is not empty if jwks_insecure_tls is set as false.


http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/util/jwt-util.h
File be/src/util/jwt-util.h:

http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/util/jwt-util.h@64
PS1, Line 64: bool is_local_file
do we still need this variable?


http://gerrit.cloudera.org:8080/#/c/19503/1/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java
File fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java:

http://gerrit.cloudera.org:8080/#/c/19503/1/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@397
PS1, Line 397:  
It's better to give a certificate which does not match the certificate returned from server.


http://gerrit.cloudera.org:8080/#/c/19503/1/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@420
PS1, Line 420:     
nit: extra spaces



-- 
To view, visit http://gerrit.cloudera.org:8080/19503
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5f1e887fae39b5fb82fa9a40352e4b507b7d8d35
Gerrit-Change-Number: 19503
Gerrit-PatchSet: 1
Gerrit-Owner: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 16 Feb 2023 00:05:09 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11922 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/19503 )

Change subject: IMPALA-11922 Verify JWKS URL server TLS certificate by default.
......................................................................


Patch Set 1:

(13 comments)

http://gerrit.cloudera.org:8080/#/c/19503/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/19503/1//COMMIT_MSG@13
PS1, Line 13: https
nit: HTTPS


http://gerrit.cloudera.org:8080/#/c/19503/1//COMMIT_MSG@46
PS1, Line 46: pem
nit: PEM


http://gerrit.cloudera.org:8080/#/c/19503/1//COMMIT_MSG@46
PS1, Line 46: certificates
Should those be CA certificates or non-CA certs (e.g., exact TLS server certificates without CA capability) are also accepted?


http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/kudu/util/curl_util.h
File be/src/kudu/util/curl_util.h:

http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/kudu/util/curl_util.h@74
PS1, Line 74: pem
nit: PEM


http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/kudu/util/curl_util.h@74
PS1, Line 74: certificates
Are these should be CA certificates or non-CA certs are also accepted?


http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/kudu/util/curl_util.h@75
PS1, Line 75: https
nit: HTTPS


http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/kudu/util/curl_util.cc
File be/src/kudu/util/curl_util.cc:

http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/kudu/util/curl_util.cc@122
PS1, Line 122: CHECK_EQ
Does it make sense to switch to using CURL_RETURN_NOT_OK() here instead?


http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/rpc/authentication.cc
File be/src/rpc/authentication.cc:

http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/rpc/authentication.cc@171
PS1, Line 171: jwks_insecure_tls
nit: 'jwks_insecure_tls' sounds a bit vague to me: it might be authentication-only TLS channel, not verifying certs on either of the sides, using weak ciphers for the handshake, using weak ciphers to encrypt the data sent over the established channel, etc.

Maybe, something like 'jwks_verify_server_tls_cert' or similar would be more descriptive of what this flag is actually for?


http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/util/jwt-util-internal.h
File be/src/util/jwt-util-internal.h:

http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/util/jwt-util-internal.h@374
PS1, Line 374: to trust
nit: what exactly this 'trust' covers?  Is this just to verify authenticity of the JWKS server's TLS certificate or the certificates in the bundle are used for something else as well?


http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/util/jwt-util.h
File be/src/util/jwt-util.h:

http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/util/jwt-util.h@59
PS1, Line 59: const std::string& jwks_file_path
Does it make sense to make this a parameter of one of the constructors for this class and have just one Init() method with the signature

  Status Init();

?


http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/util/jwt-util.h@63
PS1, Line 63: Init
ditto


http://gerrit.cloudera.org:8080/#/c/19503/1/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java
File fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java:

http://gerrit.cloudera.org:8080/#/c/19503/1/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@83
PS1, Line 83: the
for ?


http://gerrit.cloudera.org:8080/#/c/19503/1/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@440
PS1, Line 440: webserverTLSCert
Is this certificate also has the CA capability?  If not, I'm a bit surprised a non-CA certificate is accepted here.

Overall, is it possible to pass here not the server's certificate as is, but the CA certificate that the server's cert is signed with?  I guess that would be the expected use case in the wild, no?



-- 
To view, visit http://gerrit.cloudera.org:8080/19503
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5f1e887fae39b5fb82fa9a40352e4b507b7d8d35
Gerrit-Change-Number: 19503
Gerrit-PatchSet: 1
Gerrit-Owner: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 16 Feb 2023 03:06:52 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11922 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/19503 )

Change subject: IMPALA-11922 Verify JWKS URL server TLS certificate by default.
......................................................................


Patch Set 7: Code-Review+1

(3 comments)

http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/kudu/util/curl_util.cc
File be/src/kudu/util/curl_util.cc:

http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/kudu/util/curl_util.cc@122
PS1, Line 122: CHECK_EQ
> I copied this code from line 92-93.  I'm very new to Impala, so I don't hav
You could check the definition of those two macros -- it could make it clearer.

CHECK_OK() would lead to a crash (due to SIGABRT) if the parameter of the macro isn't Status::OK, and that was the way to handle such a situation in the constructor.  Since EasyCurl::DoRequest() returns Status, now there is a way to handle a situation when curl_easy_setopt() fails gracefully, the same way as it's done for other curl_easy_setopt() invocations in this method.


http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/util/jwt-util-internal.h
File be/src/util/jwt-util-internal.h:

http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/util/jwt-util-internal.h@374
PS1, Line 374: of certs
> I modified this comment a little bit in an attempt to clarify.  It's only u
OK, great.  So, maybe then update the comment to make it more specific what the certs in the bundle are used for?


http://gerrit.cloudera.org:8080/#/c/19503/7/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java
File fe/src/test/java/org/apache/impala/testutil/X509CertChain.java:

PS7: 
Just curious: any specific reason for bringing in BouncyCastle in addition to java.security/javax.security?



-- 
To view, visit http://gerrit.cloudera.org:8080/19503
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5f1e887fae39b5fb82fa9a40352e4b507b7d8d35
Gerrit-Change-Number: 19503
Gerrit-PatchSet: 7
Gerrit-Owner: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Feb 2023 16:50:30 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11922 Verify JWKS URL server TLS certificate by default.

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19503 )

Change subject: IMPALA-11922 Verify JWKS URL server TLS certificate by default.
......................................................................


Patch Set 8:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/12422/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/19503
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5f1e887fae39b5fb82fa9a40352e4b507b7d8d35
Gerrit-Change-Number: 19503
Gerrit-PatchSet: 8
Gerrit-Owner: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Feb 2023 18:35:57 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11922 Verify JWKS URL server TLS certificate by default.

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19503 )

Change subject: IMPALA-11922 Verify JWKS URL server TLS certificate by default.
......................................................................


Patch Set 9: Verified+1


-- 
To view, visit http://gerrit.cloudera.org:8080/19503
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5f1e887fae39b5fb82fa9a40352e4b507b7d8d35
Gerrit-Change-Number: 19503
Gerrit-PatchSet: 9
Gerrit-Owner: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 23 Feb 2023 00:01:54 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11922 Verify JWKS URL server TLS certificate by default.

Posted by "Jason Fehr (Code Review)" <ge...@cloudera.org>.
Jason Fehr has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/19503 )

Change subject: IMPALA-11922 Verify JWKS URL server TLS certificate by default.
......................................................................

IMPALA-11922 Verify JWKS URL server TLS certificate by default.

**** BREAKING CHANGE ****

JWT Auth has an option to specify the location of the
JSON Web Key Set (JWKS) using a URL. If that URL is
accessed over HTTPS, the TLS certificate presented by the
server is not verified.

This means that Impala only requires the server to return
a TLS certificate, whether or not Impala trusts the signing
certificate chain.

The implications of this setup is that a fully secure chain
of trust cannot be established throughout the entire JWT
authentication lifecycle and thus creates an attack vector
where a bad actor could trick Impala into trusting an
actor-controlled JWKS. The bad actor can then generate
a JWT with any claims they chose and Impala will accept it.

This change introduces:
  1. verification of JWKS server TLS certificate by default
  2. jwks_insecure_tls Impala startup flag
  3. jwks_ca_certificate Impala startup flag

1. While previously, the JWKS URL was always called without
   verifying its TLS certificate, the default is to now to
   verify that cert. Thus, any cases where the JWKS was
   retrieved from an untrusted URL will now cause Impala
   to fail to start.

2. The new flag jwks_insecure_tls controls whether or not
   Impala verifies the TLS certificate presented by the
   JWKS server. It defaults to "false" meaning that the
   certificate will be verified. Setting this value to
   "true" will restore the previous behavior where
   untrusted TLS certificates are accepted.

3. The new flag jwks_ca_certificate enables specifying
   a PEM certificate bundle that contains certificates
   to trust when calling to the JWKS URL.

Testing was achieved in the front-end Java custom cluster
tests. An existing test was modified and three new tests
were created. The following test cases are covered:
  1. Insecurely retrieve a JWKS from a server with an
     untrusted TLS certificate. This test case is expected
     to pass.
  2. Securely retrieve a JWKS from a server with an
     untrusted TLS certificate. This test case is expected
     to fail. The Impala coordinator logs are checked to
     ensure the cause was an untrusted certificate
     presented by the JWKS server.
  3. Retrieve a JWKS from a server where the root CA is
     trusted, but the cert contains the wrong CN. This
     test is expected to fail. The Impala logs are checked
     to ensure the cause was a certificate with an
     incorrect CN.
  4. Securely retrieve a JWKS from a server with a trusted
     TLS certificate. This test case is expected to pass.

Change-Id: I5f1e887fae39b5fb82fa9a40352e4b507b7d8d35
---
M be/src/kudu/util/curl_util.cc
M be/src/kudu/util/curl_util.h
M be/src/rpc/authentication.cc
M be/src/service/impala-server.cc
M be/src/util/jwt-util-internal.h
M be/src/util/jwt-util-test.cc
M be/src/util/jwt-util.cc
M be/src/util/jwt-util.h
M fe/pom.xml
M fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java
A fe/src/test/java/org/apache/impala/testutil/X509CertChain.java
11 files changed, 563 insertions(+), 54 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/03/19503/4
-- 
To view, visit http://gerrit.cloudera.org:8080/19503
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5f1e887fae39b5fb82fa9a40352e4b507b7d8d35
Gerrit-Change-Number: 19503
Gerrit-PatchSet: 4
Gerrit-Owner: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>

[Impala-ASF-CR] IMPALA-11922 Verify JWKS URL server TLS certificate by default.

Posted by "Jason Fehr (Code Review)" <ge...@cloudera.org>.
Jason Fehr has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/19503 )

Change subject: IMPALA-11922 Verify JWKS URL server TLS certificate by default.
......................................................................

IMPALA-11922 Verify JWKS URL server TLS certificate by default.

**** BREAKING CHANGE ****

JWT Auth has an option to specify the location of the
JSON Web Key Set (JWKS) using a URL. If that URL is
accessed over HTTPS, the TLS certificate presented by the
server is not verified.

This means that Impala only requires the server to return
a TLS certificate, whether or not Impala trusts the signing
certificate chain.

The implications of this setup is that a fully secure chain
of trust cannot be established throughout the entire JWT
authentication lifecycle and thus creates an attack vector
where a bad actor could trick Impala into trusting an
actor-controlled JWKS. The bad actor can then generate
a JWT with any claims they chose and Impala will accept it.

This change introduces:
  1. verification of JWKS server TLS certificate by default
  2. jwks_insecure_tls Impala startup flag
  3. jwks_ca_certificate Impala startup flag

1. While previously, the JWKS URL was always called without
   verifying its TLS certificate, the default is to now to
   verify that cert.  Thus, any cases where the JWKS was
   retrieved from an untrusted URL will now cause Impala
   to fail to start.

2. The new flag jwks_insecure_tls controls whether or not
   Impala verifies the TLS certificate presented by the
   JWKS server.  It defaults to "false" meaning that the
   certificate will be verified.  Setting this value to
   "true" will restore the previous behavior where
   untrusted TLS certificates are accepted.

3. The new flag jwks_ca_certificate enables specifying
   a PEM certificate bundle that contains certificates
   to trust when calling to the JWKS URL.

Testing was achieved in the front-end Java custom cluster
tests.  An existing test was modified and two new tests
were created.  The following test cases are covered:
  1. Insecurely retrieve a JWKS from a server with an
     untrusted TLS certificate.  This test case is expected
     to pass.
  2. Securely retrieve a JWKS from a server with an
     untrusted TLS certificate.  This test case is expected
     to fail.  The Impala corodinator logs are checked to
     ensure the cause was an untrusted certificate
     presented by the JWKS server.
  3. Securely retrieve a JWKS from a server with a trusted
     TLS certificate.  This test case is expected to pass.

Change-Id: I5f1e887fae39b5fb82fa9a40352e4b507b7d8d35
---
M be/src/kudu/util/curl_util.cc
M be/src/kudu/util/curl_util.h
M be/src/rpc/authentication.cc
M be/src/service/impala-server.cc
M be/src/util/jwt-util-internal.h
M be/src/util/jwt-util-test.cc
M be/src/util/jwt-util.cc
M be/src/util/jwt-util.h
M fe/pom.xml
M fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java
A fe/src/test/java/org/apache/impala/testutil/X509CertChain.java
11 files changed, 563 insertions(+), 54 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/03/19503/3
-- 
To view, visit http://gerrit.cloudera.org:8080/19503
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5f1e887fae39b5fb82fa9a40352e4b507b7d8d35
Gerrit-Change-Number: 19503
Gerrit-PatchSet: 3
Gerrit-Owner: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>

[Impala-ASF-CR] IMPALA-11922 Verify JWKS URL server TLS certificate by default.

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19503 )

Change subject: IMPALA-11922 Verify JWKS URL server TLS certificate by default.
......................................................................


Patch Set 4:

(12 comments)

http://gerrit.cloudera.org:8080/#/c/19503/4/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java
File fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java:

http://gerrit.cloudera.org:8080/#/c/19503/4/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@393
PS4, Line 393:    * 
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/19503/4/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@441
PS4, Line 441:    * 
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/19503/4/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@452
PS4, Line 452:     Path logDir = Files.createTempDirectory("testJwtAuthWithTrustedJwksHttpsUrlInvalidCN");
line too long (91 > 90)


http://gerrit.cloudera.org:8080/#/c/19503/4/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java
File fe/src/test/java/org/apache/impala/testutil/X509CertChain.java:

http://gerrit.cloudera.org:8080/#/c/19503/4/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@51
PS4, Line 51:   
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/19503/4/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@56
PS4, Line 56:   
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/19503/4/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@67
PS4, Line 67:   
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/19503/4/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@111
PS4, Line 111:   
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/19503/4/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@126
PS4, Line 126:    * 
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/19503/4/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@140
PS4, Line 140:   
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/19503/4/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@202
PS4, Line 202:     
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/19503/4/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@211
PS4, Line 211:     
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/19503/4/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@245
PS4, Line 245:   
line has trailing whitespace



-- 
To view, visit http://gerrit.cloudera.org:8080/19503
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5f1e887fae39b5fb82fa9a40352e4b507b7d8d35
Gerrit-Change-Number: 19503
Gerrit-PatchSet: 4
Gerrit-Owner: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Feb 2023 02:56:51 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11922 Verify JWKS URL server TLS certificate by default.

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19503 )

Change subject: IMPALA-11922 Verify JWKS URL server TLS certificate by default.
......................................................................


Patch Set 9:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9065/ DRY_RUN=false


-- 
To view, visit http://gerrit.cloudera.org:8080/19503
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5f1e887fae39b5fb82fa9a40352e4b507b7d8d35
Gerrit-Change-Number: 19503
Gerrit-PatchSet: 9
Gerrit-Owner: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Feb 2023 18:52:57 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11922 Verify JWKS URL server TLS certificate by default.

Posted by "Jason Fehr (Code Review)" <ge...@cloudera.org>.
Jason Fehr has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/19503 )

Change subject: IMPALA-11922 Verify JWKS URL server TLS certificate by default.
......................................................................

IMPALA-11922 Verify JWKS URL server TLS certificate by default.

**** BREAKING CHANGE ****

JWT Auth has an option to specify the location of the
JSON Web Key Set (JWKS) using a URL. If that URL is
accessed over HTTPS, the TLS certificate presented by the
server is not verified.

This means that Impala only requires the server to return
a TLS certificate, whether or not Impala trusts the signing
certificate chain.

The implications of this setup is that a fully secure chain
of trust cannot be established throughout the entire JWT
authentication lifecycle and thus creates an attack vector
where a bad actor could trick Impala into trusting an
actor-controlled JWKS. The bad actor can then generate
a JWT with any claims they chose and Impala will accept it.

This change introduces:
  1. verification of JWKS server TLS certificate by default
  2. jwks_insecure_tls Impala startup flag
  3. jwks_ca_certificate Impala startup flag

1. While previously, the JWKS URL was always called without
   verifying its TLS certificate, the default is to now to
   verify that cert. Thus, any cases where the JWKS was
   retrieved from an untrusted URL will now cause Impala
   to fail to start.

2. The new flag jwks_insecure_tls controls whether or not
   Impala verifies the TLS certificate presented by the
   JWKS server. It defaults to "false" meaning that the
   certificate will be verified. Setting this value to
   "true" will restore the previous behavior where
   untrusted TLS certificates are accepted.

3. The new flag jwks_ca_certificate enables specifying
   a PEM certificate bundle that contains certificates
   to trust when calling to the JWKS URL.

Testing was achieved in the front-end Java custom cluster
tests. An existing test was modified and three new tests
were created. The following test cases are covered:
  1. Insecurely retrieve a JWKS from a server with an
     untrusted TLS certificate. This test case is expected
     to pass.
  2. Securely retrieve a JWKS from a server with an
     untrusted TLS certificate. This test case is expected
     to fail. The Impala coordinator logs are checked to
     ensure the cause was an untrusted certificate
     presented by the JWKS server.
  3. Retrieve a JWKS from a server where the root CA is
     trusted, but the cert contains the wrong CN. This
     test is expected to fail. The Impala logs are checked
     to ensure the cause was a certificate with an
     incorrect CN.
  4. Securely retrieve a JWKS from a server with a trusted
     TLS certificate. This test case is expected to pass.

Change-Id: I5f1e887fae39b5fb82fa9a40352e4b507b7d8d35
---
M be/src/kudu/util/curl_util.cc
M be/src/kudu/util/curl_util.h
M be/src/rpc/authentication.cc
M be/src/service/impala-server.cc
M be/src/util/jwt-util-internal.h
M be/src/util/jwt-util-test.cc
M be/src/util/jwt-util.cc
M be/src/util/jwt-util.h
M fe/pom.xml
M fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java
A fe/src/test/java/org/apache/impala/testutil/X509CertChain.java
11 files changed, 581 insertions(+), 54 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/03/19503/6
-- 
To view, visit http://gerrit.cloudera.org:8080/19503
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5f1e887fae39b5fb82fa9a40352e4b507b7d8d35
Gerrit-Change-Number: 19503
Gerrit-PatchSet: 6
Gerrit-Owner: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>

[Impala-ASF-CR] IMPALA-11922 Verify JWKS URL server TLS certificate by default.

Posted by "Jason Fehr (Code Review)" <ge...@cloudera.org>.
Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/19503 )

Change subject: IMPALA-11922 Verify JWKS URL server TLS certificate by default.
......................................................................


Patch Set 7:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/kudu/util/curl_util.cc
File be/src/kudu/util/curl_util.cc:

http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/kudu/util/curl_util.cc@122
PS1, Line 122: CHECK_EQ
> You could check the definition of those two macros -- it could make it clea
Thanks for that explanation.  I switched to CURL_RETURN_NOT_OK.


http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/util/jwt-util-internal.h
File be/src/util/jwt-util-internal.h:

http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/util/jwt-util-internal.h@374
PS1, Line 374: of certs
> OK, great.  So, maybe then update the comment to make it more specific what
I had updated the comment with my previous patch set.  Do you have any specific additional information you want to see?


http://gerrit.cloudera.org:8080/#/c/19503/7/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java
File fe/src/test/java/org/apache/impala/testutil/X509CertChain.java:

PS7: 
> Just curious: any specific reason for bringing in BouncyCastle in addition 
Yeah, the JDK built-in classes define the interfaces/classes to provide a structure for managing X509 certificates and RSA private keys.  Bouncycastle provides the implementation that does the actual certificate signing and private key generation.

Bouncycastle and this class are both only used in unit tests.



-- 
To view, visit http://gerrit.cloudera.org:8080/19503
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5f1e887fae39b5fb82fa9a40352e4b507b7d8d35
Gerrit-Change-Number: 19503
Gerrit-PatchSet: 7
Gerrit-Owner: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Feb 2023 18:16:08 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11922 Verify JWKS URL server TLS certificate by default.

Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/19503 )

Change subject: IMPALA-11922 Verify JWKS URL server TLS certificate by default.
......................................................................


Patch Set 8: Code-Review+1

LGTM


-- 
To view, visit http://gerrit.cloudera.org:8080/19503
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5f1e887fae39b5fb82fa9a40352e4b507b7d8d35
Gerrit-Change-Number: 19503
Gerrit-PatchSet: 8
Gerrit-Owner: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Feb 2023 18:39:26 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11922 Verify JWKS URL server TLS certificate by default.

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19503 )

Change subject: IMPALA-11922 Verify JWKS URL server TLS certificate by default.
......................................................................


Patch Set 4:

Build Failed 

https://jenkins.impala.io/job/gerrit-code-review-checks/12402/ : Initial code review checks failed. See linked job for details on the failure.


-- 
To view, visit http://gerrit.cloudera.org:8080/19503
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5f1e887fae39b5fb82fa9a40352e4b507b7d8d35
Gerrit-Change-Number: 19503
Gerrit-PatchSet: 4
Gerrit-Owner: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Feb 2023 03:16:35 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11922 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/19503 )

Change subject: IMPALA-11922 Verify JWKS URL server TLS certificate by default.
......................................................................


Patch Set 10: Code-Review+1

(2 comments)

LGTM

http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/util/jwt-util-internal.h
File be/src/util/jwt-util-internal.h:

http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/util/jwt-util-internal.h@374
PS1, Line 374: of certs
> I had updated the comment with my previous patch set.  Do you have any spec
Ah, I guess the idea was to indicate that the certificates in the bundle are used to verify the TLS certificate of the JWKS server, but if the current version looks good enough and non-ambiguous to you, then no need to update.


http://gerrit.cloudera.org:8080/#/c/19503/7/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java
File fe/src/test/java/org/apache/impala/testutil/X509CertChain.java:

PS7: 
> Yeah, the JDK built-in classes define the interfaces/classes to provide a s
Ah, sure -- I meant relying on SunJSSE/SunJCE providers would be something I expected to see, but BC is an option as well.  Was just curious why BC is the preferred provider.



-- 
To view, visit http://gerrit.cloudera.org:8080/19503
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5f1e887fae39b5fb82fa9a40352e4b507b7d8d35
Gerrit-Change-Number: 19503
Gerrit-PatchSet: 10
Gerrit-Owner: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 23 Feb 2023 02:19:33 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11922 Verify JWKS URL server TLS certificate by default.

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19503 )

Change subject: IMPALA-11922 Verify JWKS URL server TLS certificate by default.
......................................................................


Patch Set 9: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/19503
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5f1e887fae39b5fb82fa9a40352e4b507b7d8d35
Gerrit-Change-Number: 19503
Gerrit-PatchSet: 9
Gerrit-Owner: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Feb 2023 18:52:56 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11922 Verify JWKS URL server TLS certificate by default.

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/19503 )

Change subject: IMPALA-11922 Verify JWKS URL server TLS certificate by default.
......................................................................

IMPALA-11922 Verify JWKS URL server TLS certificate by default.

**** BREAKING CHANGE ****
If using JWT authentication to the Impala engine and the
JWKS is retrieved from a URL, Impala now verifies the
server's TLS certificate.  Before, Impala did not verify
the trust chain nor did it verify the CN/SAN.

JWT Auth has an option to specify the location of the
JSON Web Key Set (JWKS) using a URL. If that URL is
accessed over HTTPS, the TLS certificate presented by the
server is not verified.

This means that Impala only requires the server to return
a TLS certificate, whether or not Impala trusts the signing
certificate chain.

The implications of this setup is that a fully secure chain
of trust cannot be established throughout the entire JWT
authentication lifecycle and thus creates an attack vector
where a bad actor could trick Impala into trusting an
actor-controlled JWKS. The bad actor can then generate
a JWT with any claims they chose and Impala will accept it.

This change introduces:
  1. verification of JWKS server TLS certificate by default
  2. jwks_verify_server_certificate Impala startup flag
  3. jwks_ca_certificate Impala startup flag

1. While previously, the JWKS URL was always called without
   verifying its TLS certificate, the default is to now to
   verify that cert. Thus, any cases where the JWKS was
   retrieved from an untrusted URL will now cause Impala
   to fail to start.

2. The new flag jwks_verify_server_certificate controls
   whether or not Impala verifies the TLS certificate
   presented by the JWKS server. It defaults to "false"
   meaning that the certificate will be verified. Setting
   this value to "false" will restore the previous behavior
   where untrusted TLS certificates are accepted.

3. The new flag jwks_ca_certificate enables specifying
   a PEM certificate bundle that contains certificates
   to trust when calling to the JWKS URL.

Testing was achieved in the front-end Java custom cluster
tests. An existing test was modified and three new tests
were created. The following test cases are covered:
  1. Insecurely retrieve a JWKS from a server with an
     untrusted TLS certificate. This test case is expected
     to pass.
  2. Securely retrieve a JWKS from a server with an
     untrusted TLS certificate. This test case is expected
     to fail. The Impala coordinator logs are checked to
     ensure the cause was an untrusted certificate
     presented by the JWKS server.
  3. Retrieve a JWKS from a server where the root CA is
     trusted, but the cert contains the wrong CN. This
     test is expected to fail. The Impala logs are checked
     to ensure the cause was a certificate with an
     incorrect CN.
  4. Securely retrieve a JWKS from a server with a trusted
     TLS certificate. This test case is expected to pass.

Change-Id: I5f1e887fae39b5fb82fa9a40352e4b507b7d8d35
Reviewed-on: http://gerrit.cloudera.org:8080/19503
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/kudu/util/curl_util.cc
M be/src/kudu/util/curl_util.h
M be/src/rpc/authentication.cc
M be/src/service/impala-server.cc
M be/src/util/jwt-util-internal.h
M be/src/util/jwt-util-test.cc
M be/src/util/jwt-util.cc
M be/src/util/jwt-util.h
M fe/pom.xml
M fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java
A fe/src/test/java/org/apache/impala/testutil/X509CertChain.java
11 files changed, 563 insertions(+), 54 deletions(-)

Approvals:
  Impala Public Jenkins: Looks good to me, approved; Verified

-- 
To view, visit http://gerrit.cloudera.org:8080/19503
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I5f1e887fae39b5fb82fa9a40352e4b507b7d8d35
Gerrit-Change-Number: 19503
Gerrit-PatchSet: 10
Gerrit-Owner: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>

[Impala-ASF-CR] IMPALA-11922 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/19503 )

Change subject: IMPALA-11922 Verify JWKS URL server TLS certificate by default.
......................................................................


Patch Set 6:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/19503/6//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/19503/6//COMMIT_MSG@29
PS6, Line 29: jwks_insecure_tls
this flag is already renamed


http://gerrit.cloudera.org:8080/#/c/19503/6/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java
File fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java:

http://gerrit.cloudera.org:8080/#/c/19503/6/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@55
PS6, Line 55: webserverTLSCert
name constant variables with all upper case characters


http://gerrit.cloudera.org:8080/#/c/19503/6/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java
File fe/src/test/java/org/apache/impala/testutil/X509CertChain.java:

http://gerrit.cloudera.org:8080/#/c/19503/6/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@80
PS6, Line 80: private final KeyPair rootCaKp;
            :   private final KeyPair leafKp;
            :   private final X509Certificate rootCert;
            :   private final X509Certificate leafCert;
follow the coding style to name the member variables with last character as "_"



-- 
To view, visit http://gerrit.cloudera.org:8080/19503
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5f1e887fae39b5fb82fa9a40352e4b507b7d8d35
Gerrit-Change-Number: 19503
Gerrit-PatchSet: 6
Gerrit-Owner: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Feb 2023 19:27:16 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11922 Verify JWKS URL server TLS certificate by default.

Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/19503 )

Change subject: IMPALA-11922 Verify JWKS URL server TLS certificate by default.
......................................................................


Patch Set 8: Code-Review+2

Oh I see Alexey gave +1 so bump to +2


-- 
To view, visit http://gerrit.cloudera.org:8080/19503
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5f1e887fae39b5fb82fa9a40352e4b507b7d8d35
Gerrit-Change-Number: 19503
Gerrit-PatchSet: 8
Gerrit-Owner: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Feb 2023 18:49:16 +0000
Gerrit-HasComments: No