Skip to content

Follow-up to Azure auth validation change #111252

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 1 commit into from
Jul 25, 2024
Merged

Follow-up to Azure auth validation change #111252

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion ...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 @@ -154,7 +154,7 @@ 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, null /* no auth header validation */);
super(account, container, null /* no auth header validation - sometimes it's omitted in these tests (TODO why?) */);
Copy link
Copy Markdown
Member Author

@DaveCTurner DaveCTurner Jul 24, 2024

Choose a reason for hiding this comment

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

As the (new) comment says, the Authorization header is sometimes missing in the calls the SDK makes, but apparently only in this specific test suite. I don't know why. I spent a little time investigating without success, but this is not on the critical path for what I'm actually working towards so I don't want to spend too long on it right now. Definitely worth digging deeper later on.

}
}

Expand Down
2 changes: 1 addition & 1 deletion ...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 @@ -51,7 +51,7 @@ public class AzureStorageCleanupThirdPartyTests extends AbstractThirdPartyReposi
USE_FIXTURE ? AzureHttpFixture.Protocol.HTTP : AzureHttpFixture.Protocol.NONE,
AZURE_ACCOUNT,
System.getProperty("test.azure.container"),
AzureHttpFixture.startsWithPredicate("SharedKey " + AZURE_ACCOUNT + ":")
AzureHttpFixture.sharedKeyForAccountPredicate(AZURE_ACCOUNT)
);

@Override
Expand Down
2 changes: 1 addition & 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 @@ -33,7 +33,7 @@ public class RepositoryAzureClientYamlTestSuiteIT extends ESClientYamlSuiteTestC
USE_FIXTURE ? AzureHttpFixture.Protocol.HTTPS : AzureHttpFixture.Protocol.NONE,
AZURE_TEST_ACCOUNT,
AZURE_TEST_CONTAINER,
AzureHttpFixture.startsWithPredicate("SharedKey " + AZURE_TEST_ACCOUNT + ":")
AzureHttpFixture.sharedKeyForAccountPredicate(AZURE_TEST_ACCOUNT)
);

private static TestTrustStore trustStore = new TestTrustStore(
Expand Down
12 changes: 9 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 @@ -49,16 +49,22 @@ public enum Protocol {
HTTPS
}

public static Predicate<String> startsWithPredicate(String expectedPrefix) {
/**
* @param account The name of the Azure Blob Storage account against which the request should be authorized..
* @return a predicate that matches the {@code Authorization} HTTP header that the Azure SDK sends when using shared key auth (i.e.
* using a key or SAS token).
* @see <a href="https://learn.microsoft.com/en-us/rest/api/storageservices/authorize-with-shared-key">Azure docs on shared key auth</a>
Comment on lines +53 to +56
Copy link
Copy Markdown
Member Author

@DaveCTurner DaveCTurner Jul 24, 2024

Choose a reason for hiding this comment

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

Hope this helps clarify - the header is coming from the Azure SDK, and I'm working towards a change that should cause it to emit a different kind of auth header, so I want to add these assertions now in order to better support the upcoming change.

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.

Very helpful 👍

*/
public static Predicate<String> sharedKeyForAccountPredicate(String account) {
return new Predicate<>() {
@Override
public boolean test(String s) {
return s.startsWith(expectedPrefix);
return s.startsWith("SharedKey " + account + ":");
}

@Override
public String toString() {
return "startsWith[" + expectedPrefix + "]";
return "SharedKey[" + account + "]";
}
};
}
Expand Down
28 changes: 14 additions & 14 deletions 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 @@ -82,27 +82,27 @@ private boolean isValidAuthHeader(HttpExchange exchange) {
@Override
public void handle(final HttpExchange exchange) throws IOException {
if (isValidAuthHeader(exchange) == false) {
try (exchange; var xcb = XContentBuilder.builder(XContentType.JSON.xContent())) {
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");
try (exchange; var builder = XContentBuilder.builder(XContentType.JSON.xContent())) {
Copy link
Copy Markdown
Member Author

@DaveCTurner DaveCTurner Jul 24, 2024

Choose a reason for hiding this comment

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

Renamed 👍

builder.startObject();
builder.field("method", exchange.getRequestMethod());
builder.field("uri", exchange.getRequestURI().toString());
builder.field("predicate", authHeaderPredicate.toString());
builder.field("authorization", Objects.toString(getAuthHeader(exchange)));
builder.startObject("headers");
for (final var header : exchange.getRequestHeaders().entrySet()) {
if (header.getValue() == null) {
xcb.nullField(header.getKey());
builder.nullField(header.getKey());
} else {
xcb.startArray(header.getKey());
builder.startArray(header.getKey());
for (final var value : header.getValue()) {
xcb.value(value);
builder.value(value);
}
xcb.endArray();
builder.endArray();
}
}
xcb.endObject();
xcb.endObject();
final var responseBytes = BytesReference.bytes(xcb);
builder.endObject();
builder.endObject();
final var responseBytes = BytesReference.bytes(builder);
exchange.getResponseHeaders().add("Content-Type", "application/json; charset=utf-8");
exchange.sendResponseHeaders(RestStatus.FORBIDDEN.getStatus(), responseBytes.length());
responseBytes.writeTo(exchange.getResponseBody());
Expand Down
2 changes: 1 addition & 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 @@ -31,7 +31,7 @@ public class AzureRepositoriesMeteringIT extends AbstractRepositoriesMeteringAPI
USE_FIXTURE ? AzureHttpFixture.Protocol.HTTPS : AzureHttpFixture.Protocol.NONE,
AZURE_TEST_ACCOUNT,
AZURE_TEST_CONTAINER,
AzureHttpFixture.startsWithPredicate("SharedKey " + AZURE_TEST_ACCOUNT + ":")
AzureHttpFixture.sharedKeyForAccountPredicate(AZURE_TEST_ACCOUNT)
);

private static TestTrustStore trustStore = new TestTrustStore(
Expand Down
2 changes: 1 addition & 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 @@ -32,7 +32,7 @@ public class AzureSearchableSnapshotsIT extends AbstractSearchableSnapshotsRestT
USE_FIXTURE ? AzureHttpFixture.Protocol.HTTPS : AzureHttpFixture.Protocol.NONE,
AZURE_TEST_ACCOUNT,
AZURE_TEST_CONTAINER,
AzureHttpFixture.startsWithPredicate("SharedKey " + AZURE_TEST_ACCOUNT + ":")
AzureHttpFixture.sharedKeyForAccountPredicate(AZURE_TEST_ACCOUNT)
);

private static TestTrustStore trustStore = new TestTrustStore(
Expand Down
2 changes: 1 addition & 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 @@ -31,7 +31,7 @@ public class AzureSnapshotBasedRecoveryIT extends AbstractSnapshotBasedRecoveryR
USE_FIXTURE ? AzureHttpFixture.Protocol.HTTPS : AzureHttpFixture.Protocol.NONE,
AZURE_TEST_ACCOUNT,
AZURE_TEST_CONTAINER,
AzureHttpFixture.startsWithPredicate("SharedKey " + AZURE_TEST_ACCOUNT + ":")
AzureHttpFixture.sharedKeyForAccountPredicate(AZURE_TEST_ACCOUNT)
);

private static TestTrustStore trustStore = new TestTrustStore(
Expand Down
2 changes: 1 addition & 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 @@ -30,7 +30,7 @@ public class AzureSnapshotRepoTestKitIT extends AbstractSnapshotRepoTestKitRestT
USE_FIXTURE ? AzureHttpFixture.Protocol.HTTPS : AzureHttpFixture.Protocol.NONE,
AZURE_TEST_ACCOUNT,
AZURE_TEST_CONTAINER,
AzureHttpFixture.startsWithPredicate("SharedKey " + AZURE_TEST_ACCOUNT + ":")
AzureHttpFixture.sharedKeyForAccountPredicate(AZURE_TEST_ACCOUNT)
);

private static TestTrustStore trustStore = new TestTrustStore(
Expand Down