-
Notifications
You must be signed in to change notification settings - Fork 3.9k
GH-38460: [Java][FlightRPC] Add mTLS support for Flight SQL JDBC driver #38461
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 7 commits
477e1c4
fa52fdc
6cfbe98
a41d551
c45c243
cc56b64
39ffc76
d852653
6ff25f6
953f5cd
7a04483
e9410ce
8e46cb9
40d0589
9697e50
1975786
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -114,6 +114,21 @@ parameters are: | |
| - null | ||
| - When TLS is enabled, the password for the certificate store | ||
|
|
||
| * - tlsRootCerts | ||
| - null | ||
| - Path to PEM-encoded root certificates for TLS - use this as | ||
| an alternative to ``trustStore`` | ||
|
|
||
| * - clientCertificate | ||
| - null | ||
| - Path to PEM-encoded client mTLS certificate when the Flight | ||
| SQL server requires client verification. | ||
|
|
||
| * - clientKey | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will it be necessary to provide an optional client key password argument if this client private key is password-protected?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hello – it could be added, but the Java Flight client doesn’t yet support that to my knowledge – so I just wanted to catch up to that for now. We can add it in a later PR, unless you feel strongly that it should be included here. |
||
| - null | ||
| - Path to PEM-encoded client mTLS key when the Flight | ||
prmoore77 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| SQL server requires client verification. | ||
|
|
||
| * - useEncryption | ||
| - false | ||
| - Whether to use TLS (the default is an insecure, plaintext | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,6 +34,8 @@ | |
| import java.util.concurrent.TimeUnit; | ||
| import java.util.function.Consumer; | ||
|
|
||
| import javax.net.ssl.SSLException; | ||
|
|
||
| import org.apache.arrow.flight.auth.ServerAuthHandler; | ||
| import org.apache.arrow.flight.auth.ServerAuthInterceptor; | ||
| import org.apache.arrow.flight.auth2.Auth2Constants; | ||
|
|
@@ -49,9 +51,14 @@ | |
|
|
||
| import io.grpc.Server; | ||
| import io.grpc.ServerInterceptors; | ||
| import io.grpc.netty.GrpcSslContexts; | ||
| import io.grpc.netty.NettyServerBuilder; | ||
| import io.netty.channel.EventLoopGroup; | ||
| import io.netty.channel.ServerChannel; | ||
| import io.netty.handler.ssl.ClientAuth; | ||
| import io.netty.handler.ssl.SslContext; | ||
| import io.netty.handler.ssl.SslContextBuilder; | ||
|
|
||
|
|
||
| /** | ||
| * Generic server of flight data that is customized via construction with delegate classes for the | ||
|
|
@@ -172,6 +179,8 @@ public static final class Builder { | |
| private int maxInboundMessageSize = MAX_GRPC_MESSAGE_SIZE; | ||
| private InputStream certChain; | ||
| private InputStream key; | ||
| private InputStream mTlsCACert; | ||
| private SslContext sslContext; | ||
| private final List<KeyFactory<?>> interceptors; | ||
| // Keep track of inserted interceptors | ||
| private final Set<String> interceptorKeys; | ||
|
|
@@ -245,7 +254,21 @@ public FlightServer build() { | |
| } | ||
|
|
||
| if (certChain != null) { | ||
| builder.useTransportSecurity(certChain, key); | ||
| SslContextBuilder sslContextBuilder = GrpcSslContexts | ||
| .forServer(certChain, key); | ||
|
|
||
| if (mTlsCACert != null) { | ||
| sslContextBuilder | ||
| .clientAuth(ClientAuth.REQUIRE) | ||
| .trustManager(mTlsCACert); | ||
| } | ||
| try { | ||
| sslContext = sslContextBuilder.build(); | ||
| } catch (SSLException e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
prmoore77 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| builder.sslContext(sslContext); | ||
| } | ||
|
|
||
| // Share one executor between the gRPC service, DoPut, and Handshake | ||
|
|
@@ -317,6 +340,15 @@ public Builder useTls(final File certChain, final File key) throws IOException { | |
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Enable Client Verification via mTLS on the server. | ||
| * @param mTlsCACert The CA certificate to use for verifying clients. | ||
| */ | ||
| public Builder useMTlsClientVerification(final File mTlsCACert) throws IOException { | ||
| this.mTlsCACert = new FileInputStream(mTlsCACert); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the user calls this method repeatedly we will leak File handles. Need to close the current one if it exists. Perhaps it would be better to defer opening the physical file until starting the connection, though that delays notification if there's a problem opening it.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hey @jduo - I'm not a very good Java developer - but I'm learning (thanks for your patience). Would it be like this? public Builder useMTlsClientVerification(final File mTlsCACert) throws IOException {
if (this.mTlsCACert != null) {
this.mTlsCACert.close();
}
this.mTlsCACert = new FileInputStream(mTlsCACert); Thanks for your help!
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes this would work. However I'm leaning towards deferring opening mTlsCACert until the connection is made. It's odd for builders to have more logic than setting fields.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jduo just for my knowledge and to clarify a point, would it be more easier if we use try-with-resources here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hi @jduo - this is the FlightServer - I'd rather not change the overall design of the current class's builder. It already uses this approach for
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @vibhatha , try-with-resources wouldn't help here since we need the stream to have a longer lifetime than this builder function. This is more of a case of tracking ownership.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it, thanks @jduo
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jduo - I've added small changes that detect if the InputStream attributes are not null, and if so - closes them. Could you please re-review?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @prmoore77 minor comment about another possible error scenario in useTls() that applies here. |
||
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Enable TLS on the server. | ||
| * @param certChain The certificate chain to use. | ||
|
|
@@ -328,6 +360,15 @@ public Builder useTls(final InputStream certChain, final InputStream key) { | |
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Enable mTLS on the server. | ||
| * @param mTlsCACert The CA certificate to use for verifying clients. | ||
| */ | ||
| public Builder useMTlsClientVerification(final InputStream mTlsCACert) throws IOException { | ||
| this.mTlsCACert = mTlsCACert; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar comment as above about this potentially leaking a file. Also, now it is ambiguous whether the Flight client should close() mTlsCACert (presumably it should but there should be a comment).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mirrored the existing Does that code have the issue as well?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct. It looks to have this issue too.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @vibhatha @davisusanibar can you file an issue to fix this?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @lidavidm I will file one. Will study a bit and file one.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. given that this issue will be filed separately - is it acceptable to approve/merge this PR?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Filed #38586 |
||
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Set the executor used by the server. | ||
| * | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -235,6 +235,57 @@ public static InputStream getCertificateStream(final String keyStorePath, | |||
| return getSingleCertificateInputStream(keyStore); | ||||
| } | ||||
|
|
||||
| /** | ||||
| * Generates an {@link InputStream} that contains certificates for path-based | ||||
| * TLS Root Certificates. | ||||
| * | ||||
| * @param tlsRootsCertificatesPath The path of the TLS Root Certificates. | ||||
| * @return a new {code InputStream} containing the certificates. | ||||
| * @throws GeneralSecurityException on error. | ||||
| * @throws IOException on error. | ||||
| */ | ||||
| public static InputStream getTlsRootCertificatesStream(final String tlsRootsCertificatesPath) | ||||
| throws GeneralSecurityException, IOException { | ||||
| Preconditions.checkNotNull(tlsRootsCertificatesPath, "TLS Root certificates path cannot be null!"); | ||||
|
|
||||
| return Files | ||||
| .newInputStream(Paths.get(Preconditions.checkNotNull(tlsRootsCertificatesPath))); | ||||
|
||||
| Preconditions.checkNotNull(keyStorePath, "KeyStore path cannot be null!"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should now be fixed.
Uh oh!
There was an error while loading. Please reload this page.