gnodet-bot commented on code in PR #2292: URL: https://github.com/apache/maven-resolver/pull/2292#discussion_r4505397953
########## maven-resolver-util/src/main/java/org/eclipse/aether/util/connector/transport/http/AwsSigV4RequestSigner.java: ########## @@ -0,0 +1,337 @@ +/* + * 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.eclipse.aether.util.connector.transport.http; + +import javax.crypto.Mac; +import javax.crypto.spec.SecretKeySpec; + +import java.net.URI; +import java.nio.charset.StandardCharsets; +import java.security.GeneralSecurityException; +import java.security.MessageDigest; +import java.time.Clock; +import java.time.ZoneOffset; +import java.time.format.DateTimeFormatter; +import java.util.ArrayList; +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.TreeMap; +import java.util.TreeSet; + +import static java.util.Objects.requireNonNull; + +/** + * {@link HttpRequestSigner} for <a href="https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_sigv-create-signed-request.html";>AWS + * Signature Version 4</a>, as S3 and S3-compatible object stores expect it. The payload is declared as + * {@code UNSIGNED-PAYLOAD}, which S3 accepts and which keeps the transports from reading uploads twice; the + * signed headers are {@code host}, {@code x-amz-date}, {@code x-amz-content-sha256} and, with a session token, + * {@code x-amz-security-token}. The request path is signed as sent, without re-encoding or normalization, which is + * what S3 requires. + * + * @since 2.0.26 + */ +public final class AwsSigV4RequestSigner implements HttpRequestSigner { + /** + * The {@code x-amz-content-sha256} value declaring that the payload is not part of the signature. + */ + public static final String UNSIGNED_PAYLOAD = "UNSIGNED-PAYLOAD"; + + private static final String ALGORITHM = "AWS4-HMAC-SHA256"; + + private static final String HEADER_AUTHORIZATION = "Authorization"; + + private static final String HEADER_HOST = "host"; + + private static final String HEADER_AMZ_DATE = "x-amz-date"; + + private static final String HEADER_AMZ_CONTENT_SHA256 = "x-amz-content-sha256"; + + private static final String HEADER_AMZ_SECURITY_TOKEN = "x-amz-security-token"; + + private static final Set<String> SIGNED_HEADER_NAMES; + + static { + TreeSet<String> names = new TreeSet<>(String.CASE_INSENSITIVE_ORDER); + names.add(HEADER_AUTHORIZATION); + names.add(HEADER_AMZ_DATE); + names.add(HEADER_AMZ_CONTENT_SHA256); + names.add(HEADER_AMZ_SECURITY_TOKEN); + SIGNED_HEADER_NAMES = Collections.unmodifiableSet(names); + } + + private static final DateTimeFormatter AMZ_DATE_FORMAT = + DateTimeFormatter.ofPattern("yyyyMMdd'T'HHmmss'Z'").withZone(ZoneOffset.UTC); + + private static final char[] HEX = "0123456789abcdef".toCharArray(); + + private final String accessKey; + + private final String secretKey; + + private final String sessionToken; Review Comment: ⚠️ **[MEDIUM] Credentials stored as `String` — cannot be zeroed from heap** `accessKey`, `secretKey`, and `sessionToken` are stored as `java.lang.String`. Strings in Java are immutable and interned — there is no way to zero the contents of a `String` from heap after use. This means credentials remain in memory (and in heap dumps, GC logs, etc.) for an indeterminate time after the `AwsSigV4RequestSigner` is discarded. Consider storing credentials as `char[]` or `byte[]` and explicitly zeroing them in a `close()` method (implementing `Closeable`), consistent with the security model used by e.g. `AuthenticationContext` which does the same. ```suggestion private final char[] accessKey; private final char[] secretKey; private final String sessionToken; // sessionToken can remain String if zeroing is not supported upstream ``` At minimum, document the limitation in the class-level Javadoc so callers are aware. ########## maven-resolver-transport-jdk-parent/maven-resolver-transport-jdk11/src/main/java/org/eclipse/aether/transport/jdk/JdkTransporter.java: ########## @@ -655,6 +673,29 @@ static boolean isRedirect(int statusCode) { * is not the repository origin (same scheme, case-insensitive host and effective port). Method rewriting * mirrors {@link HttpClient.Redirect#NORMAL}. */ + /** + * A redirect hop carries the previous hop's headers: replace the signature with one for the new request when + * the hop stays within the repository origin, and strip it otherwise. + */ + private HttpRequest resign(HttpRequest request) { Review Comment: 🔍 **[LOW] `resign()` insertion orphans `redirectRequest()`'s Javadoc** The original Javadoc block for `redirectRequest()` (ending with `*/` around line 675) is now followed by the new `resign()` method (with its own Javadoc) rather than by `redirectRequest()` itself. The result is that `redirectRequest()` at line 699 has no Javadoc of its own — the orphaned block above `resign()` is now associated with `resign()` by proximity. Please move `resign()` (with its own Javadoc) to a different position, or add a proper Javadoc directly above `redirectRequest()`. ########## maven-resolver-util/src/main/java/org/eclipse/aether/util/connector/transport/http/HttpTransporterUtils.java: ########## @@ -249,6 +250,77 @@ public static boolean isFollowInsecureRedirect( deprecatedTransportProperty); } + /** + * Getter for {@link ConfigurationProperties#HTTP_REQUEST_SIGNER}: the signer configured for the repository, or + * {@code null} when requests are not signed. For {@link ConfigurationProperties#HTTP_REQUEST_SIGNER_AWS_SIGV4} + * the repository's username and password are the access key and secret key; without repository authentication + * the {@code AWS_ACCESS_KEY_ID}, {@code AWS_SECRET_ACCESS_KEY} and {@code AWS_SESSION_TOKEN} environment + * variables are used. The region comes from {@link ConfigurationProperties#HTTP_AWS_REGION}, then the + * {@code AWS_REGION} and {@code AWS_DEFAULT_REGION} environment variables, then the default. + * + * @throws IllegalStateException if signing is configured but no credentials can be found + * @throws IllegalArgumentException if an unknown signer is configured + * @since 2.0.26 + */ + public static HttpRequestSigner getHttpRequestSigner(RepositorySystemSession session, RemoteRepository repository) { Review Comment: 💡 **[SUGGESTION] No warning when signing over plain `http://` — leaks `Authorization` header** `getHttpRequestSigner()` happily creates a signer for `http://` repository URLs. When signing over plain HTTP, the `Authorization` header is transmitted in cleartext, exposing HMAC-signed credentials to network observers. The ITs themselves use plain HTTP via MinIO testcontainers. Consider adding a `WARN` log when `repository.getUrl().startsWith("http:")`: ```java if (repository.getUrl().startsWith("http:")) { LOGGER.warn("Repository {} uses HTTP (not HTTPS) with AWS SigV4 signing — " + "the Authorization header will be transmitted in cleartext", repository.getId()); } ``` This is non-blocking but would significantly improve the user experience for misconfigured setups. ########## maven-resolver-transport-apache/src/test/java/org/eclipse/aether/transport/apache/AwsSigV4ApacheTransporterIT.java: ########## @@ -0,0 +1,134 @@ +/* + * 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.eclipse.aether.transport.apache; + +import java.io.IOException; +import java.net.HttpURLConnection; +import java.net.URI; +import java.nio.charset.StandardCharsets; + +import org.eclipse.aether.ConfigurationProperties; +import org.eclipse.aether.DefaultRepositorySystemSession; +import org.eclipse.aether.repository.RemoteRepository; +import org.eclipse.aether.spi.connector.transport.GetTask; +import org.eclipse.aether.spi.connector.transport.PeekTask; +import org.eclipse.aether.spi.connector.transport.PutTask; +import org.eclipse.aether.spi.connector.transport.Transporter; +import org.eclipse.aether.spi.connector.transport.TransporterFactory; +import org.eclipse.aether.spi.io.PathProcessorSupport; +import org.eclipse.aether.util.connector.transport.http.AwsSigV4RequestSigner; +import org.eclipse.aether.util.repository.AuthenticationBuilder; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; +import org.testcontainers.containers.MinIOContainer; +import org.testcontainers.junit.jupiter.Container; +import org.testcontainers.junit.jupiter.Testcontainers; +import org.testcontainers.utility.DockerImageName; + +import static org.junit.jupiter.api.Assertions.*; + +/** + * The transporter against an S3-compatible store with {@code aws-sigv4} request signing: a plain HTTP repository + * URL whose path names the bucket and prefix, and the repository username and password as access and secret key. + */ +@Testcontainers(disabledWithoutDocker = true) +class AwsSigV4ApacheTransporterIT { + private static final String BUCKET = "sigv4"; + + private static final String REGION = "us-east-1"; + + @Container + static final MinIOContainer CONTAINER = + new MinIOContainer(DockerImageName.parse("pgsty/silo:RELEASE.2026-09-03T13-18-01Z-distroless") Review Comment: 🔍 **[LOW] Third-party Docker image — supply chain risk** ```java DockerImageName.parse("pgsty/silo:RELEASE.2026-09-03T13-18-01Z-distroless").asCompatibleSubstituteFor("minio/minio") ``` `pgsty/silo` is a third-party image (not the official `minio/minio`). Using it in ITs introduces a supply chain risk: the image could change behavior, be removed, or be compromised without notice. All three IT files (`ApacheTransporterIT`, `JdkTransporterIT`, `JettyTransporterIT`) use it. Consider using the official `minio/minio` image directly, or at minimum pinning by image digest rather than by tag. ########## maven-resolver-util/src/main/java/org/eclipse/aether/util/connector/transport/http/HttpTransporterUtils.java: ########## @@ -249,6 +250,77 @@ public static boolean isFollowInsecureRedirect( deprecatedTransportProperty); } + /** + * Getter for {@link ConfigurationProperties#HTTP_REQUEST_SIGNER}: the signer configured for the repository, or + * {@code null} when requests are not signed. For {@link ConfigurationProperties#HTTP_REQUEST_SIGNER_AWS_SIGV4} + * the repository's username and password are the access key and secret key; without repository authentication + * the {@code AWS_ACCESS_KEY_ID}, {@code AWS_SECRET_ACCESS_KEY} and {@code AWS_SESSION_TOKEN} environment + * variables are used. The region comes from {@link ConfigurationProperties#HTTP_AWS_REGION}, then the + * {@code AWS_REGION} and {@code AWS_DEFAULT_REGION} environment variables, then the default. + * + * @throws IllegalStateException if signing is configured but no credentials can be found + * @throws IllegalArgumentException if an unknown signer is configured + * @since 2.0.26 + */ + public static HttpRequestSigner getHttpRequestSigner(RepositorySystemSession session, RemoteRepository repository) { + String signer = ConfigUtils.getString( + session, + ConfigurationProperties.DEFAULT_HTTP_REQUEST_SIGNER, + ConfigurationProperties.HTTP_REQUEST_SIGNER + "." + repository.getId(), + ConfigurationProperties.HTTP_REQUEST_SIGNER); + if (ConfigurationProperties.HTTP_REQUEST_SIGNER_NONE.equals(signer)) { + return null; + } + if (ConfigurationProperties.HTTP_REQUEST_SIGNER_AWS_SIGV4.equals(signer)) { + String accessKey = null; + String secretKey = null; + String sessionToken = null; + try (AuthenticationContext authContext = AuthenticationContext.forRepository(session, repository)) { + if (authContext != null) { + accessKey = authContext.get(AuthenticationContext.USERNAME); + secretKey = authContext.get(AuthenticationContext.PASSWORD); + } + } + if (accessKey == null || secretKey == null) { + accessKey = System.getenv("AWS_ACCESS_KEY_ID"); + secretKey = System.getenv("AWS_SECRET_ACCESS_KEY"); + sessionToken = emptyToNull(System.getenv("AWS_SESSION_TOKEN")); + if (accessKey == null || secretKey == null) { Review Comment: ⚠️ **[MEDIUM] Partial repo credentials silently fall through to env vars — no user warning** If the repository auth context provides `USERNAME` but not `PASSWORD` (or vice versa), the `accessKey == null || secretKey == null` check at line 284 is `true` and the code silently falls through to try environment variables — without logging any warning. The user gets a signing attempt using env-var credentials instead of their configured repo credentials, which is likely not what they intended and can be very confusing to diagnose. Suggested fix: log a `WARN` before the env-var fallback when partial credentials are detected: ```java if (accessKey == null || secretKey == null) { if (authContext != null && (authContext.get(AuthenticationContext.USERNAME) != null || authContext.get(AuthenticationContext.PASSWORD) != null)) { LOGGER.warn("Repository {} has partial AWS credentials configured (username XOR password) — " + "falling back to environment variable credentials", repository.getId()); } accessKey = System.getenv("AWS_ACCESS_KEY_ID"); ``` ########## maven-resolver-transport-jdk-parent/maven-resolver-transport-jdk11/src/main/java/org/eclipse/aether/transport/jdk/JdkTransporter.java: ########## @@ -511,8 +519,12 @@ protected void implPut(PutTask task) throws Exception { } private void prepare(HttpRequest.Builder requestBuilder) { - if (preemptiveAuth - || (preemptivePutAuth && requestBuilder.build().method().equals("PUT"))) { + HttpRequest built = requestBuilder.build(); + if (requestSigner != null) { + // all other headers are set by now; the signature covers the request as built + requestSigner.sign(built.method(), built.uri()).forEach(requestBuilder::setHeader); + } + if (preemptiveAuth || (preemptivePutAuth && built.method().equals("PUT"))) { Review Comment: ⚠️ **[MEDIUM] JDK `prepare()` missing `requestSigner == null` guard — inconsistent with Apache transport** The Apache transport at line 505 uses: ```java if (requestSigner == null && (preemptiveAuth || (preemptivePutAuth && put))) { ``` The JDK transport at this line uses: ```java if (preemptiveAuth || (preemptivePutAuth && built.method().equals("PUT"))) { ``` The missing `requestSigner == null` guard is safe at runtime because `serverAuthentication` is only populated via `if (requestSigner == null ...)` (around line 787), so `serverAuthentication` will always be `null` when `requestSigner` is set. However, the inconsistency is a maintenance hazard — a future refactor could break that invariant silently. The Apache transport's defensive guard is the correct pattern to follow. ```suggestion if (requestSigner == null && (preemptiveAuth || (preemptivePutAuth && built.method().equals("PUT")))) { ``` -- 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]
