Skip to content

GH-155: Delegate opening mTLS cert files to build() #783

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

GH-155: Delegate opening mTLS cert files to build() #783

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f75d6e5
GH-155: Delegate opening mTLS cert files to `build()`
rtadepalli Jun 5, 2025
ffb76f4
move code around to preserve closing order
rtadepalli Jun 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 40 additions & 7 deletions flight/flight-core/src/main/java/org/apache/arrow/flight/FlightServer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,9 +190,18 @@ public static final class Builder {
private int maxInboundMessageSize = MAX_GRPC_MESSAGE_SIZE;
private int maxHeaderListSize = MAX_GRPC_MESSAGE_SIZE;
private int backpressureThreshold = DEFAULT_BACKPRESSURE_THRESHOLD;

/*
TODO: Remove certChain/key/mTlsCACert as instance vars and make them local vars in build() after they are
no longer being set in the builder methods.
*/
private File certChainFile;
private InputStream certChain;
private File keyFile;
private InputStream key;
private File mTlsCACertFile;
private InputStream mTlsCACert;

private SslContext sslContext;
private final List<KeyFactory<?>> interceptors;
// Keep track of inserted interceptors
Expand All @@ -213,6 +222,16 @@ public static final class Builder {

/** Create the server for this builder. */
public FlightServer build() {
// Get TLS info in order if the server is being configured to use mTLS.
try {
prepareTlsSettings();
} catch (IOException e) {
closeMTlsCACert();
closeCertChain();
closeKey();
Comment on lines +229 to +231
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, this should be in a try-finally wrapping the entire build() call (since other exceptions in between this and the actual TLS settings will leak the handles). Either that, or this should be moved down to right before we use the file handles (and should still be in a try-finally)

throw new RuntimeException("Could not create FlightServer with mTLS", e);
}

// Add the auth middleware if applicable.
if (headerAuthenticator != CallHeaderAuthenticator.NO_OP) {
this.middleware(
Expand Down Expand Up @@ -442,11 +461,8 @@ private void closeMTlsCACert() {
* @param key The private key to use.
*/
public Builder useTls(final File certChain, final File key) throws IOException {
closeCertChain();
this.certChain = new FileInputStream(certChain);

closeKey();
this.key = new FileInputStream(key);
this.certChainFile = certChain;
this.keyFile = key;

return this;
}
Expand All @@ -457,8 +473,8 @@ public Builder useTls(final File certChain, final File key) throws IOException {
* @param mTlsCACert The CA certificate to use for verifying clients.
*/
public Builder useMTlsClientVerification(final File mTlsCACert) throws IOException {
closeMTlsCACert();
this.mTlsCACert = new FileInputStream(mTlsCACert);
this.mTlsCACertFile = mTlsCACert;

return this;
}

Expand All @@ -468,6 +484,7 @@ public Builder useMTlsClientVerification(final File mTlsCACert) throws IOExcepti
* @param certChain The certificate chain to use.
* @param key The private key to use.
*/
@Deprecated(forRemoval = true, since = "18.4.0")
public Builder useTls(final InputStream certChain, final InputStream key) throws IOException {
closeCertChain();
this.certChain = certChain;
Expand All @@ -483,6 +500,7 @@ public Builder useTls(final InputStream certChain, final InputStream key) throws
*
* @param mTlsCACert The CA certificate to use for verifying clients.
*/
@Deprecated(forRemoval = true, since = "18.4.0")
public Builder useMTlsClientVerification(final InputStream mTlsCACert) throws IOException {
closeMTlsCACert();
this.mTlsCACert = mTlsCACert;
Expand Down Expand Up @@ -552,5 +570,20 @@ public Builder producer(FlightProducer producer) {
this.producer = Preconditions.checkNotNull(producer);
return this;
}

private void prepareTlsSettings() throws IOException {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way this is written, if you set the File version then the deprecated version never takes effect, even if you set it afterwards. I suppose that's OK given the deprecation.

if (this.mTlsCACertFile != null) {
closeMTlsCACert();
this.mTlsCACert = new FileInputStream(this.mTlsCACertFile);
}
if (this.certChainFile != null) {
closeCertChain();
this.certChain = new FileInputStream(this.certChainFile);
}
if (this.keyFile != null) {
closeKey();
this.key = new FileInputStream(this.keyFile);
}
}
}
}
Loading