GH-38460: [Java][FlightRPC] Add mTLS support for Flight SQL JDBC driver #38461

prmoore77 · 2023-10-25T14:10:18Z

Rationale for this change

I wanted to add additional security capabilities to the Arrow Flight SQL JDBC driver so that it catches up to ADBC. ADBC already supports mTLS - and it is a great security feature. I wanted to bring this to the JDBC driver as well.

What changes are included in this PR?

This PR adds support for mTLS (client certificate verification/authentication) to the Arrow Flight SQL JDBC driver.

Are these changes tested?

Yes, I've added tests of the new mTLS functionality - and have ensured that the change is backward compatible by verifying all existing tests pass.

Are there any user-facing changes?

Yes - but the end-user documentation for the Arrow Flight SQL JDBC driver has been updated in the PR itself.

Closes: [Java][FlightRPC] Add mTLS support to the Arrow Flight SQL JDBC driver #38460

…DBC driver.

…mTLS.

…DBC driver.

…mTLS.

…tls' into feature/flight-sql-jdbc-driver-mtls

github-actions · 2023-10-25T14:45:45Z

⚠️ GitHub issue #38460 has been automatically assigned in GitHub to PR creator.

danepitkin · 2023-10-25T15:24:17Z

Thanks @prmoore77 ! @vibhatha @davisusanibar and I can take a look this week.

...flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/ConnectionMutualTlsTest.java

docs/source/java/flight_sql_jdbc_driver.rst

jbonofre · 2023-10-26T19:20:35Z

Thanks for the PR. I'm doing a pass and testing as well.

…jduo

…jduo

danepitkin

The tests are quite thorough, nice work! I'll defer to others who have more experience on Flight SQL for approval.

danepitkin · 2023-10-26T22:44:32Z

...flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/ConnectionMutualTlsTest.java

+   * @throws Exception on error.
+   */
+  @Test
+  public void testTLSConnectionPropertyTrueIntegerCorrectCastUrlAndPropertiesUsingPutWithDriverManager()


We could use @ParameterizedTest to reduce some code duplication in the test files.

danepitkin · 2023-10-26T22:46:30Z

...flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/ConnectionMutualTlsTest.java

+    final ArrowFlightJdbcDataSource dataSource =
+        ArrowFlightJdbcDataSource.createNewDataSource(properties);
+    try (final Connection connection = dataSource.getConnection()) {
+      assert connection.isValid(300);


Out of curiosity, why is there a 300 second timeout for ArrowFlightJdbcDataSource connections, but 0 second timeouts for ArrowFlightJdbcDriver/DriverManager connections?

Very good question :) - I copied the tests from the ConnectionTlsTest.java code - while modifying the tests for mTLS of course. I'm not sure of the rationale for the timeout, but I'll research it.

jduo · 2023-10-26T23:31:58Z

java/flight/flight-core/src/main/java/org/apache/arrow/flight/FlightServer.java

+     * @param mTlsCACert The CA certificate to use for verifying clients.
+     */
+    public Builder useMTlsClientVerification(final File mTlsCACert) throws IOException {
+      this.mTlsCACert = new FileInputStream(mTlsCACert);


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.

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!

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.

@jduo just for my knowledge and to clarify a point, would it be more easier if we use try-with-resources here?

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 useTls - and I think my change is consistent with that.

@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.

Got it, thanks @jduo

@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?

Thanks @prmoore77 minor comment about another possible error scenario in useTls() that applies here.

jduo · 2023-10-26T23:34:11Z

java/flight/flight-core/src/main/java/org/apache/arrow/flight/FlightServer.java

+     * @param mTlsCACert The CA certificate to use for verifying clients.
+     */
+    public Builder useMTlsClientVerification(final InputStream mTlsCACert) throws IOException {
+      this.mTlsCACert = mTlsCACert;


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).

I mirrored the existing useTls method here

Does that code have the issue as well?

Correct. It looks to have this issue too.

@vibhatha @davisusanibar can you file an issue to fix this?

@lidavidm I will file one. Will study a bit and file one.

given that this issue will be filed separately - is it acceptable to approve/merge this PR?

@vibhatha @davisusanibar

Filed #38586

davisusanibar

Thank you for the PR. I am adding a few minor questions.

davisusanibar · 2023-10-28T04:31:59Z

docs/source/java/flight_sql_jdbc_driver.rst

+     - Path to PEM-encoded client mTLS certificate when the Flight
+       SQL server requires client verification.
+
+   * - clientKey


Will it be necessary to provide an optional client key password argument if this client private key is password-protected?

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.

...jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/client/ArrowFlightSqlClientHandler.java

vibhatha · 2023-10-30T00:49:56Z

@prmoore77 I left a few comments and nice work with the test cases and comments. Thanks for working on this issue.

… them if so) in FlightServer.java

…FlightServer.java Co-authored-by: David Li <[email protected]>

lidavidm · 2023-11-04T17:27:16Z

java/flight/flight-core/src/main/java/org/apache/arrow/flight/FlightServer.java

+     * @param mTlsCACert The CA certificate to use for verifying clients.
+     */
+    public Builder useMTlsClientVerification(final InputStream mTlsCACert) throws IOException {
+      this.mTlsCACert = mTlsCACert;


Filed #38586

jduo · 2023-11-06T15:25:23Z

@prmoore77 heads up, after #38521 a copy constructor was added to ArrowFlightSqlClientHandler.Builder
So any new fields that this PR adds will need to be added to that copy constructor.

#38580 also changes ArrowFligthSqlClientHandler to set defaults correctly and adds tests for verifying defaults, so if that goes in first, new fields will be added there too.

prmoore77 · 2023-11-06T16:46:12Z

@prmoore77 heads up, after #38521 a copy constructor was added to ArrowFlightSqlClientHandler.Builder So any new fields that this PR adds will need to be added to that copy constructor.

#38580 also changes ArrowFligthSqlClientHandler to set defaults correctly and adds tests for verifying defaults, so if that goes in first, new fields will be added there too.

Hi @jduo - I've added the new fields to the new copy constructor (from #38521) - per your recommendation.

See: 1975786 - for details.

Thanks!

jbonofre · 2023-11-06T17:48:16Z

I did a quick test and review. LGTM. Thanks !

conbench-apache-arrow · 2023-11-07T05:33:57Z

After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 4ff1a29.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 3 possible false positives for unstable benchmarks that are known to sometimes produce them.

…C driver (apache#38461) ### Rationale for this change I wanted to add additional security capabilities to the Arrow Flight SQL JDBC driver so that it catches up to ADBC. ADBC already supports mTLS - and it is a great security feature. I wanted to bring this to the JDBC driver as well. ### What changes are included in this PR? This PR adds support for mTLS (client certificate verification/authentication) to the Arrow Flight SQL JDBC driver. ### Are these changes tested? Yes, I've added tests of the new mTLS functionality - and have ensured that the change is backward compatible by verifying all existing tests pass. ### Are there any user-facing changes? Yes - but the end-user documentation for the Arrow Flight SQL JDBC driver has been updated in the PR itself. * Closes: apache#38460 Lead-authored-by: prmoore77 <[email protected]> Co-authored-by: David Li <[email protected]> Signed-off-by: David Li <[email protected]>

prmoore77 added 5 commits October 23, 2023 17:09

Got mTLS and TLS_ROOT_CERTS capability working for Arrow Flight SQL J…

477e1c4

…DBC driver.

Added support for FlightServer mTLS. Added tests for Flight SQL JDBC …

fa52fdc

…mTLS.

Got mTLS and TLS_ROOT_CERTS capability working for Arrow Flight SQL J…

6cfbe98

…DBC driver.

Added support for FlightServer mTLS. Added tests for Flight SQL JDBC …

a41d551

…mTLS.

Merge remote-tracking branch 'origin/feature/flight-sql-jdbc-driver-m…

c45c243

…tls' into feature/flight-sql-jdbc-driver-mtls

prmoore77 requested a review from lidavidm as a code owner October 25, 2023 14:10

lidavidm changed the title ~~GH-38460: [Arrow Flight JDBC Drvier] - add mTLS support~~ GH-38460: [Java][FlightRPC] Add add mTLS support for Flight SQL JDBC driver Oct 25, 2023

Minor name fix

cc56b64

github-actions bot added awaiting review Awaiting review Component: Java Component: Documentation labels Oct 25, 2023

prmoore77 changed the title ~~GH-38460: [Java][FlightRPC] Add add mTLS support for Flight SQL JDBC driver~~ GH-38460: [Java][FlightRPC] Add mTLS support for Flight SQL JDBC driver Oct 25, 2023

jduo reviewed Oct 26, 2023

View reviewed changes

...flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/ConnectionMutualTlsTest.java Outdated Show resolved Hide resolved

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Oct 26, 2023

jduo reviewed Oct 26, 2023

View reviewed changes

docs/source/java/flight_sql_jdbc_driver.rst Show resolved Hide resolved

jduo reviewed Oct 26, 2023

View reviewed changes

docs/source/java/flight_sql_jdbc_driver.rst Show resolved Hide resolved

Changed mTLS Java tests to use try-with-resources as recommended by @…

39ffc76

…jduo

danepitkin reviewed Oct 26, 2023

View reviewed changes

jduo reviewed Oct 26, 2023

View reviewed changes

davisusanibar reviewed Oct 28, 2023

View reviewed changes

vibhatha reviewed Oct 30, 2023

View reviewed changes

...jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/client/ArrowFlightSqlClientHandler.java Show resolved Hide resolved

prmoore77 added 3 commits October 30, 2023 09:11

Merge branch 'apache:main' into feature/flight-sql-jdbc-driver-mtls

d852653

Now checking if InputStream attributes are not null (and then closing…

6ff25f6

… them if so) in FlightServer.java

Added documentation to method: withTlsRootCertificates

953f5cd

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Nov 3, 2023

Update java/flight/flight-core/src/main/java/org/apache/arrow/flight/…

8e46cb9

…FlightServer.java Co-authored-by: David Li <[email protected]>

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Nov 3, 2023

Changes to address feedback on PR

40d0589

jduo approved these changes Nov 3, 2023

View reviewed changes

prmoore77 requested review from jduo and lidavidm November 3, 2023 21:42

lidavidm mentioned this pull request Nov 4, 2023

[Java][FlightRPC] FlightServer builder should store File, not InputStream #38586

Open

lidavidm approved these changes Nov 4, 2023

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting changes Awaiting changes labels Nov 4, 2023

prmoore77 added 2 commits November 6, 2023 11:29

Merge branch 'apache:main' into feature/flight-sql-jdbc-driver-mtls

9697e50

Added new fields to new copy constructor (from: apache#38521)

1975786

jduo approved these changes Nov 6, 2023

View reviewed changes

jbonofre approved these changes Nov 6, 2023

View reviewed changes

lidavidm approved these changes Nov 6, 2023

View reviewed changes

lidavidm merged commit 4ff1a29 into apache:main Nov 6, 2023
18 checks passed

lidavidm removed the awaiting merge Awaiting merge label Nov 6, 2023

prmoore77 deleted the feature/flight-sql-jdbc-driver-mtls branch November 6, 2023 20:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-38460: [Java][FlightRPC] Add mTLS support for Flight SQL JDBC driver #38461

GH-38460: [Java][FlightRPC] Add mTLS support for Flight SQL JDBC driver #38461

prmoore77 commented Oct 25, 2023 •

edited by github-actions bot

Loading

github-actions bot commented Oct 25, 2023

danepitkin commented Oct 25, 2023

jbonofre commented Oct 26, 2023

danepitkin left a comment

danepitkin Oct 26, 2023

danepitkin Oct 26, 2023 •

edited

Loading

prmoore77 Oct 27, 2023

jduo Oct 26, 2023 •

edited

Loading

prmoore77 Oct 27, 2023

jduo Oct 27, 2023

vibhatha Oct 30, 2023

prmoore77 Oct 30, 2023

jduo Oct 30, 2023

vibhatha Oct 30, 2023

prmoore77 Oct 30, 2023

jduo Nov 2, 2023

jduo Oct 26, 2023 •

edited

Loading

prmoore77 Oct 27, 2023

jduo Oct 27, 2023

lidavidm Oct 30, 2023

vibhatha Oct 30, 2023

prmoore77 Oct 31, 2023

lidavidm Nov 3, 2023

lidavidm Nov 4, 2023

davisusanibar left a comment

davisusanibar Oct 28, 2023

prmoore77 Oct 31, 2023

vibhatha commented Oct 30, 2023

lidavidm Nov 4, 2023

jduo commented Nov 6, 2023

prmoore77 commented Nov 6, 2023 •

edited

Loading

jbonofre commented Nov 6, 2023

conbench-apache-arrow bot commented Nov 7, 2023

GH-38460: [Java][FlightRPC] Add mTLS support for Flight SQL JDBC driver #38461

GH-38460: [Java][FlightRPC] Add mTLS support for Flight SQL JDBC driver #38461

Conversation

prmoore77 commented Oct 25, 2023 • edited by github-actions bot Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Oct 25, 2023

danepitkin commented Oct 25, 2023

jbonofre commented Oct 26, 2023

danepitkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danepitkin Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jduo Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jduo Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davisusanibar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vibhatha commented Oct 30, 2023

Choose a reason for hiding this comment

jduo commented Nov 6, 2023

prmoore77 commented Nov 6, 2023 • edited Loading

jbonofre commented Nov 6, 2023

conbench-apache-arrow bot commented Nov 7, 2023

prmoore77 commented Oct 25, 2023 •

edited by github-actions bot

Loading

danepitkin Oct 26, 2023 •

edited

Loading

jduo Oct 26, 2023 •

edited

Loading

jduo Oct 26, 2023 •

edited

Loading

prmoore77 commented Nov 6, 2023 •

edited

Loading