Skip to content
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.

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

[MRESOLVER-427] Config for expect/continue inhibition #363

Merged
merged 1 commit into from
Nov 14, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,22 @@ public final class ConfigurationProperties {
*/
public static final boolean DEFAULT_HTTP_PREEMPTIVE_PUT_AUTH = true;

/**
* Boolean flag should the HTTP transport use expect-continue handshake for PUT requests. Not all transport support
* this option. This option may be needed for some broken HTTP servers.
*
* @see #DEFAULT_HTTP_EXPECT_CONTINUE
* @since 2.0.0
*/
public static final String HTTP_EXPECT_CONTINUE = PREFIX_CONNECTOR + "http.expectContinue";

/**
* Default value if {@link #HTTP_EXPECT_CONTINUE} is not set: {@code true}.
*
* @since 2.0.0
*/
public static final boolean DEFAULT_HTTP_EXPECT_CONTINUE = true;

/**
* The mode that sets HTTPS transport "security mode": to ignore any SSL errors (certificate validity checks,
* hostname verification). The default value is {@link #HTTPS_SECURITY_MODE_DEFAULT}.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,15 @@ final class HttpTransporter extends AbstractTransporter {
builder.useSystemProperties();
}

final boolean expectContinue = ConfigUtils.getBoolean(
session,
ConfigurationProperties.DEFAULT_HTTP_EXPECT_CONTINUE,
ConfigurationProperties.HTTP_EXPECT_CONTINUE + "." + repository.getId(),
ConfigurationProperties.HTTP_EXPECT_CONTINUE);
if (expectContinue != ConfigurationProperties.DEFAULT_HTTP_EXPECT_CONTINUE) {
state.setExpectContinue(expectContinue);
}

final boolean reuseConnections = ConfigUtils.getBoolean(
session,
ConfigurationProperties.DEFAULT_HTTP_REUSE_CONNECTIONS,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -726,6 +726,27 @@ void testPut_Authenticated_ExpectContinueRejected() throws Exception {
assertEquals("upload", TestFileUtils.readString(new File(repoDir, "file.txt")));
}

@Test
void testPut_Authenticated_ExpectContinueDisabled() throws Exception {
session.setConfigProperty(ConfigurationProperties.HTTP_EXPECT_CONTINUE, false);
httpServer.setAuthentication("testuser", "testpass");
httpServer.setExpectSupport(HttpServer.ExpectContinue.FAIL); // if transport tries Expect/Continue explode
auth = new AuthenticationBuilder()
.addUsername("testuser")
.addPassword("testpass")
.build();
newTransporter(httpServer.getHttpUrl());
RecordingTransportListener listener = new RecordingTransportListener();
PutTask task =
new PutTask(URI.create("repo/file.txt")).setListener(listener).setDataString("upload");
transporter.put(task);
assertEquals(0L, listener.dataOffset);
assertEquals(6L, listener.dataLength);
assertEquals(1, listener.startedCount); // w/ expectContinue enabled would have here 2
assertTrue(listener.progressedCount > 0, "Count: " + listener.progressedCount);
assertEquals("upload", TestFileUtils.readString(new File(repoDir, "file.txt")));
}

@Test
void testPut_Authenticated_ExpectContinueRejected_ExplicitlyConfiguredHeader() throws Exception {
Map<String, String> headers = new HashMap<>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,8 @@ final class JdkHttpTransporter extends AbstractTransporter {

private final int requestTimeout;

private final boolean expectContinue;

JdkHttpTransporter(RepositorySystemSession session, RemoteRepository repository) throws NoTransporterException {
try {
URI uri = new URI(repository.getUrl()).parseServerAuthority();
Expand Down Expand Up @@ -149,6 +151,11 @@ final class JdkHttpTransporter extends AbstractTransporter {
ConfigurationProperties.DEFAULT_REQUEST_TIMEOUT,
ConfigurationProperties.REQUEST_TIMEOUT + "." + repository.getId(),
ConfigurationProperties.REQUEST_TIMEOUT);
this.expectContinue = ConfigUtils.getBoolean(
session,
ConfigurationProperties.DEFAULT_HTTP_EXPECT_CONTINUE,
ConfigurationProperties.HTTP_EXPECT_CONTINUE + "." + repository.getId(),
ConfigurationProperties.HTTP_EXPECT_CONTINUE);

this.headers = headers;
this.client = getOrCreateClient(session, repository);
Expand Down Expand Up @@ -280,8 +287,10 @@ protected void implGet(GetTask task) throws Exception {

@Override
protected void implPut(PutTask task) throws Exception {
HttpRequest.Builder request =
HttpRequest.newBuilder().uri(resolve(task)).timeout(Duration.ofMillis(requestTimeout));
HttpRequest.Builder request = HttpRequest.newBuilder()
.uri(resolve(task))
.timeout(Duration.ofMillis(requestTimeout))
.expectContinue(expectContinue);
headers.forEach(request::setHeader);
try (FileUtils.TempFile tempFile = FileUtils.newTempFile()) {
utilPut(task, Files.newOutputStream(tempFile.getPath()), true);
Expand Down
1 change: 1 addition & 0 deletions src/site/markdown/configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ Option | Type | Description | Default Value | Supports Repo ID Suffix
`aether.connector.http.cacheState` | boolean | Flag indicating whether a memory-based cache is used for user tokens, connection managers, expect continue requests and authentication schemes. | `true` | no
`aether.connector.http.connectionMaxTtl` | int | Total time to live in seconds for an HTTP connection, after that time, the connection will be dropped (no matter for how long it was idle). | `300` | yes
`aether.connector.http.credentialEncoding` | String | The encoding/charset to use when exchanging credentials with HTTP servers. | `"ISO-8859-1"` | yes
`aether.connector.http.expectContinue` | boolean | Whether to use expect/continue handshake during PUTs. Some broken HTTP servers needs this disabled. | `true` | yes
`aether.connector.http.headers` | `Map<String, String>` | The request headers to use for HTTP-based repository connectors. The headers are specified using a map of strings mapping a header name to its value. The repository-specific headers map is supposed to be complete, i.e. is not merged with the general headers map. | - | yes
`aether.connector.http.localAddress` | String | Set the outgoing interface (globally or per remote repository). Valid values are local accessible IP addresses or host names. The default will use the system's default route. Invalid addresses will result in HttpTransport creation failure. | - | yes
`aether.connector.http.maxConnectionsPerRoute` | int | The maximum concurrent connections per route HTTP client is allowed to use. | `50` | yes
Expand Down