mattcasters commented on code in PR #8401: URL: https://github.com/apache/hop/pull/8401#discussion_r4024059868
########## plugins/tech/arrow/src/test/java/org/apache/hop/arrow/flight/ArrowFlightServerAuthenticationTest.java: ########## @@ -0,0 +1,161 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package org.apache.hop.arrow.flight; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.mock; + +import java.util.Optional; +import org.apache.arrow.flight.CallOption; +import org.apache.arrow.flight.Criteria; +import org.apache.arrow.flight.FlightClient; +import org.apache.arrow.flight.FlightRuntimeException; +import org.apache.arrow.flight.FlightStatusCode; +import org.apache.arrow.flight.Location; +import org.apache.arrow.flight.grpc.CredentialCallOption; +import org.apache.arrow.memory.RootAllocator; +import org.apache.hop.core.logging.ILogChannel; +import org.apache.hop.core.variables.Variables; +import org.apache.hop.metadata.serializer.memory.MemoryMetadataProvider; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; + +/** + * Starts a real Flight server and checks that the credentials configured on it are actually + * enforced. These tests use plain gRPC: they cover the authentication half of the security options, Review Comment: **[suggestion]** The new transport-security path is untested. Auth tests explicitly use plain gRPC; `ArrowFlightSecurityTest` only checks option validation and VFS reads of dummy PEM text; nothing starts a server with `useTls` / `useMTlsClientVerification` or a client with `verifyServer`, a trusted CA, or a client certificate. A wiring mistake in `ArrowFlightServer.buildFlightServer` or `ArrowFlightDataStream.buildFlightClient` (wrong Location scheme, streams consumed at `build()` rather than `useTls()`, PKCS#8 vs trust-store behavior, mTLS `ClientAuth.REQUIRE`) would not be caught. That is a real gap for a PR whose purpose is TLS. **Suggestion:** Add an integration test that mints a short-lived self-signed (and, separately, mTLS) PEM pair, starts `ArrowFlightServer` with those files, and connects with `ArrowFlightDataStream` / `FlightClient` (verify on + trusted CA; verify off without extra certs; mTLS with client cert; anonymous client rejected when the server CA is set). ########## plugins/tech/arrow/src/main/java/org/apache/hop/arrow/datastream/flight/ArrowFlightDataStream.java: ########## @@ -192,15 +273,58 @@ private void initializeStreamWriting() throws HopException { flightClient.startPut( FlightDescriptor.path(dataStreamMeta.getName()), vectorSchemaRoot, - new AsyncPutListener()); + new AsyncPutListener(), + callOptions); } private void buildFlightClient() throws HopException { try { // Get a flight client going. // - Location location = Location.forGrpcInsecure(realHostname, realPort); - flightClient = FlightClient.builder(rootAllocator, location).build(); + Location location = + tls + ? Location.forGrpcTls(realHostname, realPort) + : Location.forGrpcInsecure(realHostname, realPort); + FlightClient.Builder builder = FlightClient.builder(rootAllocator, location); + + if (tls) { + builder.verifyServer(verifyServer); + + String realTrustedCertificates = variables.resolve(trustedCertificatesFile); + if (!Utils.isEmpty(realTrustedCertificates)) { + builder.trustedCertificates( + new ByteArrayInputStream(ArrowFlightSecurity.readPemFile(realTrustedCertificates))); Review Comment: **[suggestion]** Two client option combinations are not validated the way the server validates TLS/auth combos. (1) Arrow Flight’s `NettyClientBuilder` throws `IllegalArgumentException` if `verifyServer` is false *and* a trusted CA or client cert/key is set. The GUI encourages both “Only switch this off for testing with a self-signed certificate” and filling mTLS / trusted-CA file widgets, so a tester following the tooltips can hit a wrapped, Flight-internal error. (2) If **Use TLS** is unchecked, trusted CA and client cert/key paths are silently ignored (`if (tls) { ... }`), unlike the server which rejects mTLS without TLS. There is no widget listener to disable those fields when TLS is off, so the form will take values that do nothing. **Suggestion:** Mirror server-side `validate()`: reject `verifyServer == false` together with trusted CA or client cert/key, and reject client cert/key (or trusted CA) when TLS is off, with a HopException. Optionally disable those widgets unless **Use TLS** is checked. Document that mTLS / a custom CA requires leaving **Verify the server certificate** on and pointing **Trusted certificates file** at the server CA. ########## plugins/tech/arrow/src/main/java/org/apache/hop/arrow/flight/ArrowFlightSecurity.java: ########## @@ -0,0 +1,163 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package org.apache.hop.arrow.flight; + +import java.io.IOException; +import java.io.InputStream; +import java.nio.charset.StandardCharsets; +import java.security.MessageDigest; +import lombok.AllArgsConstructor; +import lombok.Getter; +import lombok.NoArgsConstructor; +import lombok.Setter; +import org.apache.arrow.flight.CallStatus; +import org.apache.arrow.flight.auth2.BasicCallHeaderAuthenticator; +import org.apache.arrow.flight.auth2.CallHeaderAuthenticator; +import org.apache.arrow.flight.auth2.GeneratedBearerTokenAuthenticator; +import org.apache.hop.core.exception.HopException; +import org.apache.hop.core.logging.ILogChannel; +import org.apache.hop.core.util.Utils; +import org.apache.hop.core.vfs.HopVfs; + +/** + * The transport security and authentication settings of the Hop Apache Arrow Flight server. Both + * are optional and disabled when nothing is configured, which keeps the plain gRPC behavior of + * earlier versions. + */ +@Getter +@Setter +@NoArgsConstructor +@AllArgsConstructor +public class ArrowFlightSecurity { + + /** Neither TLS nor authentication: plain gRPC on an open port. */ + public static final ArrowFlightSecurity NONE = new ArrowFlightSecurity(); + + /** The PEM file with the server certificate chain. Enables TLS together with the private key. */ + private String certificateFile; Review Comment: **[nit]** `NONE` is a public static instance of a Lombok `@Setter` class. The five-argument `ArrowFlightServer` constructor stores this singleton when `security` is null, so `server.getSecurity().setUsername(...)` (or any other setter) would mutate process-wide default security. **Suggestion:** Drop `@Setter` on this type, or make `NONE` an immutable subclass / copy-on-store so the constructor keeps a private snapshot. ########## THREAT_MODEL.md: ########## @@ -182,8 +183,12 @@ where strict outbound TLS verification is required (see §9). the `<sslConfig>` block of `hop-server.xml`, but **TLS is off by default** (the shipped config starts a plain-HTTP listener; only auth is on). Outbound JDBC TLS is fully driver-delegated; HTTP/REST/cloud transforms use the JVM trust - store unless given a per-transform truststore. Enabling TLS is an operator - responsibility (§10). + store unless given a per-transform truststore. The **Arrow Flight server** + (`hop arrow`) is likewise plaintext and unauthenticated unless started with + `--arrow-flight-tls-certificate`/`--arrow-flight-tls-key` (optionally + `--arrow-flight-tls-client-ca` for mutual TLS) and + `--arrow-flight-username`/`--arrow-flight-password`; it logs a warning when it + starts without them. Enabling TLS is an operator responsibility (§10). Review Comment: **[suggestion]** This new paragraph correctly says enabling Flight TLS is an operator responsibility (§10), but §10’s “Use TLS” bullet still only mentions the Hop Server and `javax.net.ssl.keyStore`. §11 likewise has no misuse pattern for an exposed `hop arrow` port. Operators reading only §10 will not see the new flags. **Suggestion:** Add a §10 bullet (and a §11 misuse line) to bind Flight off `0.0.0.0` unless needed, pass `--arrow-flight-tls-*` and `--arrow-flight-username`/`--arrow-flight-password`, and keep **Verify the server certificate** on for Hop clients. ########## plugins/tech/arrow/src/main/java/org/apache/hop/arrow/flight/ArrowFlightSecurity.java: ########## @@ -0,0 +1,163 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package org.apache.hop.arrow.flight; + +import java.io.IOException; +import java.io.InputStream; +import java.nio.charset.StandardCharsets; +import java.security.MessageDigest; +import lombok.AllArgsConstructor; +import lombok.Getter; +import lombok.NoArgsConstructor; +import lombok.Setter; +import org.apache.arrow.flight.CallStatus; +import org.apache.arrow.flight.auth2.BasicCallHeaderAuthenticator; +import org.apache.arrow.flight.auth2.CallHeaderAuthenticator; +import org.apache.arrow.flight.auth2.GeneratedBearerTokenAuthenticator; +import org.apache.hop.core.exception.HopException; +import org.apache.hop.core.logging.ILogChannel; +import org.apache.hop.core.util.Utils; +import org.apache.hop.core.vfs.HopVfs; + +/** + * The transport security and authentication settings of the Hop Apache Arrow Flight server. Both + * are optional and disabled when nothing is configured, which keeps the plain gRPC behavior of + * earlier versions. + */ +@Getter +@Setter +@NoArgsConstructor +@AllArgsConstructor +public class ArrowFlightSecurity { + + /** Neither TLS nor authentication: plain gRPC on an open port. */ + public static final ArrowFlightSecurity NONE = new ArrowFlightSecurity(); + + /** The PEM file with the server certificate chain. Enables TLS together with the private key. */ + private String certificateFile; + + /** The PEM file with the (PKCS#8) private key belonging to the server certificate. */ + private String privateKeyFile; + + /** + * The PEM file with the certificate authority used to verify client certificates. Enables mutual + * TLS. + */ + private String clientCertificateAuthorityFile; + + /** The user name clients need to present, or empty to accept unauthenticated clients. */ + private String username; + + /** The password belonging to {@link #username}. */ + private String password; + + public boolean isTlsEnabled() { + return !Utils.isEmpty(certificateFile) || !Utils.isEmpty(privateKeyFile); + } + + public boolean isMutualTlsEnabled() { + return !Utils.isEmpty(clientCertificateAuthorityFile); + } + + public boolean isAuthenticationEnabled() { + return !Utils.isEmpty(username); + } + + /** + * Verify that the combination of options makes sense and warn about the ones that are legal but + * unwise. + * + * @param log the channel to log warnings on + * @throws HopException in case the options can't be used to start a server + */ + public void validate(ILogChannel log) throws HopException { + if (!Utils.isEmpty(certificateFile) && Utils.isEmpty(privateKeyFile)) { + throw new HopException( + "Please also specify the TLS private key file for the Arrow Flight server certificate."); + } + if (Utils.isEmpty(certificateFile) && !Utils.isEmpty(privateKeyFile)) { + throw new HopException( + "Please also specify the TLS certificate file for the Arrow Flight server private key."); + } + if (isMutualTlsEnabled() && !isTlsEnabled()) { + throw new HopException( + "Client certificate verification needs TLS: please also specify a certificate and private key for the Arrow Flight server."); + } + if (isAuthenticationEnabled() && Utils.isEmpty(password)) { + throw new HopException( + "Please also specify a password for Arrow Flight server user '" + username + "'."); + } + + if (!isTlsEnabled()) { + if (isAuthenticationEnabled()) { + log.logMinimal( + "WARNING: the Arrow Flight server is not using TLS: credentials and data are sent in the clear."); + } else { Review Comment: **[suggestion]** `validate()` warns only when TLS is off. A server started with `--arrow-flight-tls-certificate`/`--arrow-flight-tls-key` but no `--arrow-flight-username` still accepts every client that can complete the TLS handshake; the start log says `authentication: disabled` at BASIC, but there is no MINIMAL warning comparable to the plaintext case. The same method treats authentication as username-only (`isAuthenticationEnabled()`): `--arrow-flight-password` without a username is stored and ignored, so TLS + password-without-username is an open encrypted listener with no warning. THREAT_MODEL.md overstates this as “it logs a warning when it starts without them” (TLS *and* username/password). **Suggestion:** Warn (or refuse) when authentication is off, including the password-without-username case. Align the threat-model sentence with what is actually logged. Keep the existing plaintext-credentials warning. ########## plugins/tech/arrow/src/main/java/org/apache/hop/arrow/datastream/flight/ArrowFlightDataStream.java: ########## @@ -192,15 +273,58 @@ private void initializeStreamWriting() throws HopException { flightClient.startPut( FlightDescriptor.path(dataStreamMeta.getName()), vectorSchemaRoot, - new AsyncPutListener()); + new AsyncPutListener(), + callOptions); } private void buildFlightClient() throws HopException { try { // Get a flight client going. // - Location location = Location.forGrpcInsecure(realHostname, realPort); - flightClient = FlightClient.builder(rootAllocator, location).build(); + Location location = + tls + ? Location.forGrpcTls(realHostname, realPort) + : Location.forGrpcInsecure(realHostname, realPort); + FlightClient.Builder builder = FlightClient.builder(rootAllocator, location); + + if (tls) { + builder.verifyServer(verifyServer); + + String realTrustedCertificates = variables.resolve(trustedCertificatesFile); + if (!Utils.isEmpty(realTrustedCertificates)) { + builder.trustedCertificates( + new ByteArrayInputStream(ArrowFlightSecurity.readPemFile(realTrustedCertificates))); + } + + String realClientCertificate = variables.resolve(clientCertificateFile); + String realClientKey = variables.resolve(clientKeyFile); + if (!Utils.isEmpty(realClientCertificate) || !Utils.isEmpty(realClientKey)) { + if (Utils.isEmpty(realClientCertificate) || Utils.isEmpty(realClientKey)) { + throw new HopException( + "Please specify both a client certificate and a client key to connect to the Flight server with mutual TLS."); + } + builder.clientCertificate( + new ByteArrayInputStream(ArrowFlightSecurity.readPemFile(realClientCertificate)), + new ByteArrayInputStream(ArrowFlightSecurity.readPemFile(realClientKey))); + } + } + + flightClient = builder.build(); + + // Authenticate if the server asks us to. We get a bearer token back which we then pass + // along with every call we make. + // + String realUsername = variables.resolve(username); + if (!Utils.isEmpty(realUsername)) { + String realPassword = Encr.decryptPasswordOptionallyEncrypted(variables.resolve(password)); + callOptions = + flightClient + .authenticateBasicToken(realUsername, Const.NVL(realPassword, "")) + .map(option -> new CallOption[] {option}) + .orElseGet(() -> new CallOption[0]); Review Comment: **[nit]** The comment says the client authenticates “if the server asks us to.” The code authenticates iff the Data Stream username is non-empty, independent of what the server is configured for. That misstates the fail mode: an empty username against a secured server fails later on `getInfo`/`startPut`/`getStream`, and a username against an open server still runs `authenticateBasicToken`. **Suggestion:** Replace with a short comment that a non-empty username triggers `authenticateBasicToken` and that the returned bearer token is passed on subsequent calls. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
