mmodzelewski commented on code in PR #4504: URL: https://github.com/apache/iggy/pull/4504#discussion_r4271792324
########## foreign/java/java-sdk/src/main/java/org/apache/iggy/builder/TcpConnectionString.java: ########## @@ -0,0 +1,194 @@ +/* + * 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.iggy.builder; + +import org.apache.iggy.exception.IggyInvalidArgumentException; + +import java.util.HashSet; +import java.util.Set; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +/** + * A parsed {@code iggy://} connection string for the TCP transport. + * + * <p>The format is {@code iggy://<username>:<password>@<host>:<port>?<options>}, where + * {@code iggy+tcp://} is accepted as well and the host may be an IPv6 address in brackets. + * The supported options are {@code tls=true|false} and {@code tls_ca_file=<path>}, each at most once. + * Error messages never include the connection string, because it contains the password. + * + * @param host the server host + * @param port the server port + * @param username the username to log in with + * @param password the password to log in with + * @param tls whether TLS is enabled + * @param tlsCaFile the path of the PEM file that validates the server certificate, or null + */ +public record TcpConnectionString( + String host, int port, String username, String password, boolean tls, String tlsCaFile) { + + private static final String DEFAULT_SCHEME = "iggy"; + private static final String SCHEME_PREFIX = "iggy+"; + private static final String TCP_TRANSPORT = "tcp"; + private static final int MAX_PORT = 65535; + private static final String TLS_OPTION = "tls"; + private static final String TLS_CA_FILE_OPTION = "tls_ca_file"; + + // Matches both "[ipv6]:port" and "host:port", where an unbracketed host may not + // contain a colon. Multi-colon authorities such as "2001:db8::1:8090" are rejected, + // and so are hosts with whitespace or control characters. + private static final Pattern ADDRESS = + Pattern.compile("^(?:\\[([^\\]\\s\\p{Cc}]+)\\]|([^:\\[\\]\\s\\p{Cc}]+)):(\\d{1,5})$"); + + // A transport name is echoed in errors only when it is plain letters, because a + // malformed scheme can contain the credentials. + private static final Pattern TRANSPORT_NAME = Pattern.compile("^[A-Za-z]+$"); + + /** + * Parses a TCP connection string. + * + * @param connectionString the connection string + * @return the parsed connection string + * @throws IggyInvalidArgumentException if the connection string is invalid or uses an unsupported option + */ + public static TcpConnectionString parse(String connectionString) { + if (connectionString == null || connectionString.isEmpty()) { + throw new IggyInvalidArgumentException("Connection string must not be empty"); + } + + String[] schemeAndRest = connectionString.split("://", -1); + if (schemeAndRest.length != 2) { + throw invalid("expected <scheme>://<username>:<password>@<host>:<port>"); + } + parseScheme(schemeAndRest[0]); + + String[] credentialsAndServer = schemeAndRest[1].split("@", -1); + if (credentialsAndServer.length != 2) { + throw invalid("expected exactly one '@' between the credentials and the server address"); + } + String[] credentials = parseCredentials(credentialsAndServer[0]); + + String[] addressAndOptions = credentialsAndServer[1].split("\\?", -1); + if (addressAndOptions.length > 2) { + throw invalid("expected at most one '?' before the options"); + } + Address address = parseAddress(addressAndOptions[0]); + Options options = addressAndOptions.length == 2 ? parseOptions(addressAndOptions[1]) : Options.NONE; + + return new TcpConnectionString( + address.host(), address.port(), credentials[0], credentials[1], options.tls(), options.tlsCaFile()); + } + + private static void parseScheme(String scheme) { + if (DEFAULT_SCHEME.equals(scheme)) { + return; + } + if (!scheme.startsWith(SCHEME_PREFIX)) { + throw invalid("scheme must be iggy:// or iggy+tcp://"); + } + String transport = scheme.substring(SCHEME_PREFIX.length()); + if (TCP_TRANSPORT.equals(transport)) { + return; + } + if (!TRANSPORT_NAME.matcher(transport).matches()) { + throw invalid("scheme must be iggy:// or iggy+tcp://"); + } + throw new IggyInvalidArgumentException( + "Unsupported transport '" + transport + "' in connection string, only tcp is supported"); + } + + private static String[] parseCredentials(String credentials) { + String[] usernameAndPassword = credentials.split(":", -1); + if (usernameAndPassword.length == 1) { + throw invalid("personal access tokens are not supported, use <username>:<password>"); + } + if (usernameAndPassword.length != 2 || usernameAndPassword[0].isEmpty() || usernameAndPassword[1].isEmpty()) { + throw invalid("credentials must be <username>:<password>"); + } + return usernameAndPassword; + } + + private static Address parseAddress(String serverAddress) { + Matcher address = ADDRESS.matcher(serverAddress); + if (!address.matches()) { + throw invalid("server address must be <host>:<port> or [<ipv6>]:<port>"); + } + int port = Integer.parseInt(address.group(3)); + if (port > MAX_PORT) { + throw invalid("port must be at most " + MAX_PORT); + } + String host = address.group(1) != null ? address.group(1) : address.group(2); + return new Address(host, port); + } + + private static Options parseOptions(String options) { + boolean tls = false; + String tlsCaFile = null; + Set<String> seen = new HashSet<>(); + for (String option : options.split("&", -1)) { + String[] nameAndValue = option.split("=", -1); + if (nameAndValue.length != 2) { + throw invalid("options must be <name>=<value> pairs separated by '&'"); + } + String name = nameAndValue[0]; + String value = nameAndValue[1]; + if (TLS_OPTION.equals(name)) { + tls = parseBoolean(name, value); + } else if (TLS_CA_FILE_OPTION.equals(name)) { Review Comment: Warning: `tls_ca_file` without `tls=true` parses cleanly, then the client connects in plaintext and ignores the CA file. Reject `tls_ca_file` unless `tls=true` is set, since the comment at line 160 promises that no option is silently dropped. ########## foreign/java/java-sdk/src/main/java/org/apache/iggy/client/async/tcp/AsyncIggyTcpClientBuilder.java: ########## @@ -173,6 +174,30 @@ public AsyncIggyTcpClientBuilder tlsCertificate(String tlsCertificatePath) { return this; } + /** + * Sets the host, port, credentials and TLS options from a connection string. + * + * <p>The format is {@code iggy://<username>:<password>@<host>:<port>?<options>}, with + * {@code tls=true|false} and {@code tls_ca_file=<path>} as the supported options. + * Settings that are not in the connection string keep their current values. + * + * @param connectionString the connection string, for example {@code iggy://iggy:iggy@localhost:8090?tls=true} + * @return this builder + * @throws IggyInvalidArgumentException if the connection string is invalid + * @see TcpConnectionString + */ + public AsyncIggyTcpClientBuilder fromConnectionString(String connectionString) { + TcpConnectionString parsed = TcpConnectionString.parse(connectionString); + host(parsed.host()); + port(parsed.port()); + credentials(parsed.username(), parsed.password()); + tls(parsed.tls()); Review Comment: Critical: `tls(parsed.tls())` runs even when the string has no `tls` option, so `enableTls().fromConnectionString("iggy://u:p@host:8090")` turns TLS off and `login()` sends the password in cleartext. Call `tls(...)` only when the string sets `tls`, and add a test for that call order. Also at `IggyTcpClientBuilder.java:231`. ########## foreign/java/java-sdk/src/main/java/org/apache/iggy/client/blocking/tcp/IggyTcpClientBuilder.java: ########## @@ -210,6 +211,30 @@ public IggyTcpClientBuilder tlsCertificate(String tlsCertificatePath) { return this; } + /** + * Sets the host, port, credentials and TLS options from a connection string. + * + * <p>The format is {@code iggy://<username>:<password>@<host>:<port>?<options>}, with + * {@code tls=true|false} and {@code tls_ca_file=<path>} as the supported options. + * Settings that are not in the connection string keep their current values. + * + * @param connectionString the connection string, for example {@code iggy://iggy:iggy@localhost:8090?tls=true} + * @return this builder + * @throws org.apache.iggy.exception.IggyInvalidArgumentException if the connection string is invalid + * @see TcpConnectionString + */ + public IggyTcpClientBuilder fromConnectionString(String connectionString) { Review Comment: Simplification: This body repeats the async builder mapping, so every option change has to land twice. Delegate to `asyncBuilder.fromConnectionString(connectionString)` and set only `username` and `password` here. ########## foreign/java/java-sdk/src/main/java/org/apache/iggy/builder/TcpConnectionString.java: ########## @@ -0,0 +1,194 @@ +/* + * 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.iggy.builder; + +import org.apache.iggy.exception.IggyInvalidArgumentException; + +import java.util.HashSet; +import java.util.Set; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +/** + * A parsed {@code iggy://} connection string for the TCP transport. + * + * <p>The format is {@code iggy://<username>:<password>@<host>:<port>?<options>}, where + * {@code iggy+tcp://} is accepted as well and the host may be an IPv6 address in brackets. + * The supported options are {@code tls=true|false} and {@code tls_ca_file=<path>}, each at most once. + * Error messages never include the connection string, because it contains the password. + * + * @param host the server host + * @param port the server port + * @param username the username to log in with + * @param password the password to log in with + * @param tls whether TLS is enabled + * @param tlsCaFile the path of the PEM file that validates the server certificate, or null + */ +public record TcpConnectionString( + String host, int port, String username, String password, boolean tls, String tlsCaFile) { + + private static final String DEFAULT_SCHEME = "iggy"; + private static final String SCHEME_PREFIX = "iggy+"; + private static final String TCP_TRANSPORT = "tcp"; + private static final int MAX_PORT = 65535; + private static final String TLS_OPTION = "tls"; + private static final String TLS_CA_FILE_OPTION = "tls_ca_file"; + + // Matches both "[ipv6]:port" and "host:port", where an unbracketed host may not + // contain a colon. Multi-colon authorities such as "2001:db8::1:8090" are rejected, + // and so are hosts with whitespace or control characters. + private static final Pattern ADDRESS = + Pattern.compile("^(?:\\[([^\\]\\s\\p{Cc}]+)\\]|([^:\\[\\]\\s\\p{Cc}]+)):(\\d{1,5})$"); + + // A transport name is echoed in errors only when it is plain letters, because a + // malformed scheme can contain the credentials. + private static final Pattern TRANSPORT_NAME = Pattern.compile("^[A-Za-z]+$"); + + /** + * Parses a TCP connection string. + * + * @param connectionString the connection string + * @return the parsed connection string + * @throws IggyInvalidArgumentException if the connection string is invalid or uses an unsupported option + */ + public static TcpConnectionString parse(String connectionString) { + if (connectionString == null || connectionString.isEmpty()) { + throw new IggyInvalidArgumentException("Connection string must not be empty"); + } + + String[] schemeAndRest = connectionString.split("://", -1); + if (schemeAndRest.length != 2) { + throw invalid("expected <scheme>://<username>:<password>@<host>:<port>"); + } + parseScheme(schemeAndRest[0]); + + String[] credentialsAndServer = schemeAndRest[1].split("@", -1); + if (credentialsAndServer.length != 2) { + throw invalid("expected exactly one '@' between the credentials and the server address"); + } + String[] credentials = parseCredentials(credentialsAndServer[0]); + + String[] addressAndOptions = credentialsAndServer[1].split("\\?", -1); + if (addressAndOptions.length > 2) { + throw invalid("expected at most one '?' before the options"); + } + Address address = parseAddress(addressAndOptions[0]); + Options options = addressAndOptions.length == 2 ? parseOptions(addressAndOptions[1]) : Options.NONE; + + return new TcpConnectionString( + address.host(), address.port(), credentials[0], credentials[1], options.tls(), options.tlsCaFile()); + } + + private static void parseScheme(String scheme) { + if (DEFAULT_SCHEME.equals(scheme)) { + return; + } + if (!scheme.startsWith(SCHEME_PREFIX)) { + throw invalid("scheme must be iggy:// or iggy+tcp://"); + } + String transport = scheme.substring(SCHEME_PREFIX.length()); + if (TCP_TRANSPORT.equals(transport)) { + return; + } + if (!TRANSPORT_NAME.matcher(transport).matches()) { + throw invalid("scheme must be iggy:// or iggy+tcp://"); + } + throw new IggyInvalidArgumentException( + "Unsupported transport '" + transport + "' in connection string, only tcp is supported"); + } + + private static String[] parseCredentials(String credentials) { + String[] usernameAndPassword = credentials.split(":", -1); + if (usernameAndPassword.length == 1) { + throw invalid("personal access tokens are not supported, use <username>:<password>"); + } + if (usernameAndPassword.length != 2 || usernameAndPassword[0].isEmpty() || usernameAndPassword[1].isEmpty()) { + throw invalid("credentials must be <username>:<password>"); + } + return usernameAndPassword; + } + + private static Address parseAddress(String serverAddress) { + Matcher address = ADDRESS.matcher(serverAddress); + if (!address.matches()) { + throw invalid("server address must be <host>:<port> or [<ipv6>]:<port>"); + } + int port = Integer.parseInt(address.group(3)); + if (port > MAX_PORT) { + throw invalid("port must be at most " + MAX_PORT); + } + String host = address.group(1) != null ? address.group(1) : address.group(2); + return new Address(host, port); + } + + private static Options parseOptions(String options) { + boolean tls = false; + String tlsCaFile = null; + Set<String> seen = new HashSet<>(); + for (String option : options.split("&", -1)) { + String[] nameAndValue = option.split("=", -1); + if (nameAndValue.length != 2) { + throw invalid("options must be <name>=<value> pairs separated by '&'"); + } + String name = nameAndValue[0]; + String value = nameAndValue[1]; + if (TLS_OPTION.equals(name)) { + tls = parseBoolean(name, value); + } else if (TLS_CA_FILE_OPTION.equals(name)) { + if (value.isEmpty()) { Review Comment: Nit: A blank `tls_ca_file` value passes here, and `tlsCertificate(String)` then clears any certificate set earlier. Reject it with `value.isBlank()`. ########## foreign/java/java-sdk/src/main/java/org/apache/iggy/builder/TcpConnectionString.java: ########## @@ -0,0 +1,194 @@ +/* + * 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.iggy.builder; + +import org.apache.iggy.exception.IggyInvalidArgumentException; + +import java.util.HashSet; +import java.util.Set; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +/** + * A parsed {@code iggy://} connection string for the TCP transport. + * + * <p>The format is {@code iggy://<username>:<password>@<host>:<port>?<options>}, where + * {@code iggy+tcp://} is accepted as well and the host may be an IPv6 address in brackets. + * The supported options are {@code tls=true|false} and {@code tls_ca_file=<path>}, each at most once. + * Error messages never include the connection string, because it contains the password. + * + * @param host the server host + * @param port the server port + * @param username the username to log in with + * @param password the password to log in with + * @param tls whether TLS is enabled + * @param tlsCaFile the path of the PEM file that validates the server certificate, or null + */ +public record TcpConnectionString( Review Comment: Warning: The PR description calls this record package-private, but it is `public`, so adding an option later breaks its canonical constructor for callers. Make it a `final class` with a private constructor and update the PR description. ########## foreign/java/java-sdk/src/main/java/org/apache/iggy/client/async/tcp/AsyncIggyTcpClientBuilder.java: ########## @@ -24,6 +24,7 @@ import io.netty.channel.nio.NioIoHandle; import io.netty.util.concurrent.EventExecutor; import org.apache.commons.lang3.StringUtils; +import org.apache.iggy.builder.TcpConnectionString; Review Comment: Nit: This import adds a package cycle, because `org.apache.iggy.builder` already imports both TCP client packages. Move `TcpConnectionString` to `org.apache.iggy.config`, which both builders already import. Also at `IggyTcpClientBuilder.java:23`. -- 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]
