Skip to content

Validate Authorization header in Azure test fixture #111242

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

Merged
DaveCTurner merged 2 commits into from
Jul 24, 2024
Merged

Validate Authorization header in Azure test fixture #111242

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
26205e6
Validate `Authorization` header in Azure test fixture
DaveCTurner Jul 24, 2024
6b0b55f
Merge branch 'main' into 2024/07/24/azure-test-fixture-validate-auth
DaveCTurner Jul 24, 2024
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
3 changes: 1 addition & 2 deletions ...lClusterTest/java/org/elasticsearch/repositories/azure/AzureBlobStoreRepositoryTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,9 +153,8 @@ long getUploadBlockSize() {

@SuppressForbidden(reason = "this test uses a HttpHandler to emulate an Azure endpoint")
private static class AzureBlobStoreHttpHandler extends AzureHttpHandler implements BlobStoreHttpHandler {

AzureBlobStoreHttpHandler(final String account, final String container) {
super(account, container);
super(account, container, null /* no auth header validation */);
Copy link
Copy Markdown
Contributor

@DiannaHohensee DiannaHohensee Jul 24, 2024

Choose a reason for hiding this comment

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

might help to explain why we don't want to do header validation here?

}
}

Expand Down
7 changes: 5 additions & 2 deletions ...terTest/java/org/elasticsearch/repositories/azure/AzureStorageCleanupThirdPartyTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,14 @@
public class AzureStorageCleanupThirdPartyTests extends AbstractThirdPartyRepositoryTestCase {
private static final boolean USE_FIXTURE = Booleans.parseBoolean(System.getProperty("test.azure.fixture", "true"));

private static final String AZURE_ACCOUNT = System.getProperty("test.azure.account");

@ClassRule
public static AzureHttpFixture fixture = new AzureHttpFixture(
USE_FIXTURE ? AzureHttpFixture.Protocol.HTTP : AzureHttpFixture.Protocol.NONE,
System.getProperty("test.azure.account"),
System.getProperty("test.azure.container")
AZURE_ACCOUNT,
System.getProperty("test.azure.container"),
AzureHttpFixture.startsWithPredicate("SharedKey " + AZURE_ACCOUNT + ":")
);

@Override
Expand Down
3 changes: 2 additions & 1 deletion ...tTest/java/org/elasticsearch/repositories/azure/RepositoryAzureClientYamlTestSuiteIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@ public class RepositoryAzureClientYamlTestSuiteIT extends ESClientYamlSuiteTestC
private static AzureHttpFixture fixture = new AzureHttpFixture(
USE_FIXTURE ? AzureHttpFixture.Protocol.HTTPS : AzureHttpFixture.Protocol.NONE,
AZURE_TEST_ACCOUNT,
AZURE_TEST_CONTAINER
AZURE_TEST_CONTAINER,
AzureHttpFixture.startsWithPredicate("SharedKey " + AZURE_TEST_ACCOUNT + ":")
);

private static TestTrustStore trustStore = new TestTrustStore(
Expand Down
24 changes: 21 additions & 3 deletions test/fixtures/azure-fixture/src/main/java/fixture/azure/AzureHttpFixture.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import java.security.cert.Certificate;
import java.util.List;
import java.util.Objects;
import java.util.function.Predicate;

import javax.net.ssl.KeyManager;
import javax.net.ssl.SSLContext;
Expand All @@ -38,6 +39,8 @@ public class AzureHttpFixture extends ExternalResource {
private final Protocol protocol;
private final String account;
private final String container;
private final Predicate<String> authHeaderPredicate;

private HttpServer server;

public enum Protocol {
Expand All @@ -46,10 +49,25 @@ public enum Protocol {
HTTPS
}

public AzureHttpFixture(Protocol protocol, String account, String container) {
public static Predicate<String> startsWithPredicate(String expectedPrefix) {
return new Predicate<>() {
@Override
public boolean test(String s) {
return s.startsWith(expectedPrefix);
}

@Override
public String toString() {
return "startsWith[" + expectedPrefix + "]";
}
};
}

public AzureHttpFixture(Protocol protocol, String account, String container, Predicate<String> authHeaderPredicate) {
this.protocol = protocol;
this.account = account;
this.container = container;
this.authHeaderPredicate = authHeaderPredicate;
}

private String scheme() {
Expand All @@ -72,7 +90,7 @@ protected void before() {
}
case HTTP -> {
server = HttpServer.create(new InetSocketAddress(InetAddress.getLoopbackAddress(), 0), 0);
server.createContext("/" + account, new AzureHttpHandler(account, container));
server.createContext("/" + account, new AzureHttpHandler(account, container, authHeaderPredicate));
server.start();
}
case HTTPS -> {
Expand All @@ -93,7 +111,7 @@ protected void before() {
new SecureRandom()
);
httpsServer.setHttpsConfigurator(new HttpsConfigurator(sslContext));
httpsServer.createContext("/" + account, new AzureHttpHandler(account, container));
httpsServer.createContext("/" + account, new AzureHttpHandler(account, container, authHeaderPredicate));
httpsServer.start();
}
}
Expand Down
58 changes: 57 additions & 1 deletion test/fixtures/azure-fixture/src/main/java/fixture/azure/AzureHttpHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,12 @@
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.io.Streams;
import org.elasticsearch.common.regex.Regex;
import org.elasticsearch.core.Nullable;
import org.elasticsearch.core.SuppressForbidden;
import org.elasticsearch.rest.RestStatus;
import org.elasticsearch.rest.RestUtils;
import org.elasticsearch.xcontent.XContentBuilder;
import org.elasticsearch.xcontent.XContentType;

import java.io.ByteArrayOutputStream;
import java.io.IOException;
Expand All @@ -32,6 +35,7 @@
import java.util.Objects;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.function.Predicate;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

Expand All @@ -45,15 +49,67 @@ public class AzureHttpHandler implements HttpHandler {
private final Map<String, BytesReference> blobs;
private final String account;
private final String container;
private final Predicate<String> authHeaderPredicate;

public AzureHttpHandler(final String account, final String container) {
public AzureHttpHandler(final String account, final String container, @Nullable Predicate<String> authHeaderPredicate) {
this.account = Objects.requireNonNull(account);
this.container = Objects.requireNonNull(container);
this.authHeaderPredicate = authHeaderPredicate;
this.blobs = new ConcurrentHashMap<>();
}

private static List<String> getAuthHeader(HttpExchange exchange) {
return exchange.getRequestHeaders().get("Authorization");
}

private boolean isValidAuthHeader(HttpExchange exchange) {
if (authHeaderPredicate == null) {
return true;
}

final var authHeader = getAuthHeader(exchange);
if (authHeader == null) {
return false;
}

if (authHeader.size() != 1) {
return false;
}
Comment on lines +70 to +77
Copy link
Copy Markdown
Contributor

@DiannaHohensee DiannaHohensee Jul 24, 2024

Choose a reason for hiding this comment

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

So it's OK to not have a header predicate, but it is not OK to skip sending one? What are these decisions based on 🤔


return authHeaderPredicate.test(authHeader.get(0));
}

@Override
public void handle(final HttpExchange exchange) throws IOException {
if (isValidAuthHeader(exchange) == false) {
try (exchange; var xcb = XContentBuilder.builder(XContentType.JSON.xContent())) {
Copy link
Copy Markdown
Contributor

@DiannaHohensee DiannaHohensee Jul 24, 2024

Choose a reason for hiding this comment

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

I'd understand 'builder' more rapidly than discerning what 'xcb' stands for, my two cents.

xcb.startObject();
xcb.field("method", exchange.getRequestMethod());
xcb.field("uri", exchange.getRequestURI().toString());
xcb.field("predicate", authHeaderPredicate.toString());
xcb.field("authorization", Objects.toString(getAuthHeader(exchange)));
xcb.startObject("headers");
for (final var header : exchange.getRequestHeaders().entrySet()) {
if (header.getValue() == null) {
xcb.nullField(header.getKey());
} else {
xcb.startArray(header.getKey());
for (final var value : header.getValue()) {
xcb.value(value);
}
xcb.endArray();
}
}
xcb.endObject();
xcb.endObject();
final var responseBytes = BytesReference.bytes(xcb);
exchange.getResponseHeaders().add("Content-Type", "application/json; charset=utf-8");
exchange.sendResponseHeaders(RestStatus.FORBIDDEN.getStatus(), responseBytes.length());
responseBytes.writeTo(exchange.getResponseBody());
return;
}
}

final String request = exchange.getRequestMethod() + " " + exchange.getRequestURI().toString();
if (request.startsWith("GET") || request.startsWith("HEAD") || request.startsWith("DELETE")) {
int read = exchange.getRequestBody().read();
Expand Down
3 changes: 2 additions & 1 deletion ...java/org/elasticsearch/xpack/repositories/metering/azure/AzureRepositoriesMeteringIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@ public class AzureRepositoriesMeteringIT extends AbstractRepositoriesMeteringAPI
private static AzureHttpFixture fixture = new AzureHttpFixture(
USE_FIXTURE ? AzureHttpFixture.Protocol.HTTPS : AzureHttpFixture.Protocol.NONE,
AZURE_TEST_ACCOUNT,
AZURE_TEST_CONTAINER
AZURE_TEST_CONTAINER,
AzureHttpFixture.startsWithPredicate("SharedKey " + AZURE_TEST_ACCOUNT + ":")
Copy link
Copy Markdown
Contributor

@DiannaHohensee DiannaHohensee Jul 24, 2024

Choose a reason for hiding this comment

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

where do the headers get generated that we're trying to match here by expecting "SharedKey ....:"?

);

private static TestTrustStore trustStore = new TestTrustStore(
Expand Down
3 changes: 2 additions & 1 deletion ...RestTest/java/org/elasticsearch/xpack/searchablesnapshots/AzureSearchableSnapshotsIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@ public class AzureSearchableSnapshotsIT extends AbstractSearchableSnapshotsRestT
private static AzureHttpFixture fixture = new AzureHttpFixture(
USE_FIXTURE ? AzureHttpFixture.Protocol.HTTPS : AzureHttpFixture.Protocol.NONE,
AZURE_TEST_ACCOUNT,
AZURE_TEST_CONTAINER
AZURE_TEST_CONTAINER,
AzureHttpFixture.startsWithPredicate("SharedKey " + AZURE_TEST_ACCOUNT + ":")
);

private static TestTrustStore trustStore = new TestTrustStore(
Expand Down
3 changes: 2 additions & 1 deletion ...rg/elasticsearch/xpack/snapshotbasedrecoveries/recovery/AzureSnapshotBasedRecoveryIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@ public class AzureSnapshotBasedRecoveryIT extends AbstractSnapshotBasedRecoveryR
private static AzureHttpFixture fixture = new AzureHttpFixture(
USE_FIXTURE ? AzureHttpFixture.Protocol.HTTPS : AzureHttpFixture.Protocol.NONE,
AZURE_TEST_ACCOUNT,
AZURE_TEST_CONTAINER
AZURE_TEST_CONTAINER,
AzureHttpFixture.startsWithPredicate("SharedKey " + AZURE_TEST_ACCOUNT + ":")
);

private static TestTrustStore trustStore = new TestTrustStore(
Expand Down
3 changes: 2 additions & 1 deletion ...est/java/org/elasticsearch/repositories/blobstore/testkit/AzureSnapshotRepoTestKitIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ public class AzureSnapshotRepoTestKitIT extends AbstractSnapshotRepoTestKitRestT
private static AzureHttpFixture fixture = new AzureHttpFixture(
USE_FIXTURE ? AzureHttpFixture.Protocol.HTTPS : AzureHttpFixture.Protocol.NONE,
AZURE_TEST_ACCOUNT,
AZURE_TEST_CONTAINER
AZURE_TEST_CONTAINER,
AzureHttpFixture.startsWithPredicate("SharedKey " + AZURE_TEST_ACCOUNT + ":")
);

private static TestTrustStore trustStore = new TestTrustStore(
Expand Down