-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Add support for AWS session tokens #30414
Changes from 9 commits
2ccaeaa
0fbe039
0299a75
c93e568
ee41de6
ed62728
85737f6
2eea829
d431f28
ff9f8a1
2602b32
9fd5d44
9db4a59
c1e0a97
552e852
e583a0c
87b6c58
7eabb56
6915c47
1475a26
439b763
2a93e94
60e7b83
9de75c6
63caae2
be7b51c
d6abddc
aafb0aa
ff29696
86fa97a
34ffc72
da791b3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,16 +39,37 @@ forbiddenApisTest { | |
|
||
boolean useFixture = false | ||
|
||
String s3AccessKey = System.getenv("amazon_s3_access_key") | ||
String s3SecretKey = System.getenv("amazon_s3_secret_key") | ||
String s3Bucket = System.getenv("amazon_s3_bucket") | ||
String s3BasePath = System.getenv("amazon_s3_base_path") | ||
|
||
if (!s3AccessKey && !s3SecretKey && !s3Bucket && !s3BasePath) { | ||
s3AccessKey = 's3_integration_test_access_key' | ||
s3SecretKey = 's3_integration_test_secret_key' | ||
s3Bucket = 'bucket_test' | ||
s3BasePath = 'integration_test' | ||
// We test against two repositories, one which uses the usual two-part "permanent" credentials and | ||
// the other which uses three-part "temporary" or "session" credentials. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hopefully we'll split this into two QA projects in a follow up PR, so for now I'd prefer to not rename the existing env vars. I understand the permanent/temporary renaming for repositories or buckets but I still think that the env vars names should reflect the feature implemented, ie keeping There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have reverted the changes to the names of the existing environment variables. |
||
|
||
String s3PermanentAccessKey = System.getenv("amazon_s3_access_key_permanent") | ||
String s3PermanentSecretKey = System.getenv("amazon_s3_secret_key_permanent") | ||
String s3PermanentBucket = System.getenv("amazon_s3_bucket_permanent") | ||
String s3PermanentBasePath = System.getenv("amazon_s3_base_path_permanent") | ||
|
||
String s3TemporaryAccessKey = System.getenv("amazon_s3_access_key_temporary") | ||
String s3TemporarySecretKey = System.getenv("amazon_s3_secret_key_temporary") | ||
String s3TemporarySessionToken = System.getenv("amazon_s3_session_token_temporary") | ||
String s3TemporaryBucket = System.getenv("amazon_s3_bucket_temporary") | ||
String s3TemporaryBasePath = System.getenv("amazon_s3_base_path_temporary") | ||
|
||
// If all these variables are missing then we are testing against the internal fixture instead, which has the following | ||
// credentials hard-coded in. | ||
|
||
if (!s3PermanentAccessKey && !s3PermanentSecretKey && !s3PermanentBucket && !s3PermanentBasePath | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we set the defaults in separate condition blocks? So that if a set [amazon_s3_access_key/amazon_s3_secret_key/amazon_s3_base_path/amazon_s3_bucket] is set it runs against the real service whatever is set for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As it currently stands, I think it makes more sense to either test entirely against the fake fixture or entirely against the real service (and therefore not run the fake fixture). If there is a followup that allows these two cases to run separately then hopefully it's not too hard to disentangle this then. |
||
&& !s3TemporaryAccessKey && !s3TemporarySecretKey && !s3TemporaryBucket && !s3TemporaryBasePath && !s3TemporarySessionToken) { | ||
|
||
s3PermanentAccessKey = 's3_integration_test_permanent_access_key' | ||
s3PermanentSecretKey = 's3_integration_test_permanent_secret_key' | ||
s3PermanentBucket = 'permanent_bucket_test' | ||
s3PermanentBasePath = 'integration_test' | ||
|
||
s3TemporaryAccessKey = 's3_integration_test_temporary_access_key' | ||
s3TemporarySecretKey = 's3_integration_test_temporary_secret_key' | ||
s3TemporaryBucket = 'temporary_bucket_test' | ||
s3TemporaryBasePath = 'integration_test' | ||
s3TemporarySessionToken = 's3_integration_test_temporary_session_token' | ||
|
||
useFixture = true | ||
} | ||
|
||
|
@@ -57,26 +78,34 @@ task s3Fixture(type: AntFixture) { | |
dependsOn compileTestJava | ||
env 'CLASSPATH', "${ -> project.sourceSets.test.runtimeClasspath.asPath }" | ||
executable = new File(project.runtimeJavaHome, 'bin/java') | ||
args 'org.elasticsearch.repositories.s3.AmazonS3Fixture', baseDir, s3Bucket | ||
args 'org.elasticsearch.repositories.s3.AmazonS3Fixture', baseDir | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd prefer if we continue to pass the bucket names to the fixture so that we can reuse it in different projects more easily. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I've reverted that. There is, however, already some other coupling (duplication of string values) between the |
||
} | ||
|
||
Map<String, Object> expansions = [ | ||
'bucket': s3Bucket, | ||
'base_path': s3BasePath | ||
'permanent_bucket': s3PermanentBucket, | ||
'permanent_base_path': s3PermanentBasePath, | ||
'temporary_bucket': s3TemporaryBucket, | ||
'temporary_base_path': s3TemporaryBasePath | ||
] | ||
processTestResources { | ||
inputs.properties(expansions) | ||
MavenFilteringHack.filter(it, expansions) | ||
} | ||
|
||
integTestCluster { | ||
keystoreSetting 's3.client.integration_test.access_key', s3AccessKey | ||
keystoreSetting 's3.client.integration_test.secret_key', s3SecretKey | ||
keystoreSetting 's3.client.integration_test_permanent.access_key', s3PermanentAccessKey | ||
keystoreSetting 's3.client.integration_test_permanent.secret_key', s3PermanentSecretKey | ||
|
||
keystoreSetting 's3.client.integration_test_temporary.access_key', s3TemporaryAccessKey | ||
keystoreSetting 's3.client.integration_test_temporary.secret_key', s3TemporarySecretKey | ||
keystoreSetting 's3.client.integration_test_temporary.session_token', s3TemporarySessionToken | ||
|
||
if (useFixture) { | ||
println "Using internal test service to test the repository-s3 plugin" | ||
dependsOn s3Fixture | ||
/* Use a closure on the string to delay evaluation until tests are executed */ | ||
setting 's3.client.integration_test.endpoint', "http://${-> s3Fixture.addressAndPort}" | ||
setting 's3.client.integration_test_permanent.endpoint', "http://${-> s3Fixture.addressAndPort}" | ||
setting 's3.client.integration_test_temporary.endpoint', "http://${-> s3Fixture.addressAndPort}" | ||
} else { | ||
println "Using an external service to test the repository-s3 plugin" | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -53,8 +53,8 @@ | |
public class AmazonS3Fixture { | ||
|
||
public static void main(String[] args) throws Exception { | ||
if (args == null || args.length != 2) { | ||
throw new IllegalArgumentException("AmazonS3Fixture <working directory> <bucket>"); | ||
if (args == null || args.length != 1) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we revert this please? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We now pass the bucket names in on the command line again. |
||
throw new IllegalArgumentException("AmazonS3Fixture <working directory>"); | ||
} | ||
|
||
final InetSocketAddress socketAddress = new InetSocketAddress(InetAddress.getLoopbackAddress(), 0); | ||
|
@@ -72,7 +72,8 @@ public static void main(String[] args) throws Exception { | |
// Emulates S3 | ||
final String storageUrl = "http://" + addressAndPort; | ||
final AmazonS3TestServer storageTestServer = new AmazonS3TestServer(storageUrl); | ||
storageTestServer.createBucket(args[1]); | ||
storageTestServer.createBucket("permanent_bucket_test"); | ||
storageTestServer.createBucket("temporary_bucket_test"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we revert this please, and pass the bucket names as program args? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We now pass the bucket names in on the command line again. |
||
|
||
httpServer.createContext("/", new ResponseHandler(storageTestServer)); | ||
httpServer.start(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -106,13 +106,38 @@ public Response handle(final String method, | |
} | ||
|
||
final List<String> authorizations = headers.get("Authorization"); | ||
if (authorizations == null | ||
|| (authorizations.isEmpty() == false & authorizations.get(0).contains("s3_integration_test_access_key") == false)) { | ||
return newError(requestId, RestStatus.FORBIDDEN, "AccessDenied", "Access Denied", ""); | ||
if (authorizations == null || authorizations.size() != 1) { | ||
return newError(requestId, RestStatus.FORBIDDEN, "AccessDenied", "No authorization", ""); | ||
} | ||
final String authorization = authorizations.get(0); | ||
final String permittedBucket; | ||
if (authorization.contains("s3_integration_test_permanent_access_key")) { | ||
final List<String> sessionTokens = headers.get("x-amz-security-token"); | ||
if (sessionTokens != null) { | ||
return newError(requestId, RestStatus.FORBIDDEN, "AccessDenied", "Unexpected session token", ""); | ||
} | ||
permittedBucket = "permanent_bucket_test"; | ||
} else if (authorization.contains("s3_integration_test_temporary_access_key")) { | ||
final List<String> sessionTokens = headers.get("x-amz-security-token"); | ||
if (sessionTokens == null || sessionTokens.size() != 1) { | ||
return newError(requestId, RestStatus.FORBIDDEN, "AccessDenied", "No session token", ""); | ||
} | ||
final String sessionToken = sessionTokens.get(0); | ||
if (sessionToken.equals("s3_integration_test_temporary_session_token") == false) { | ||
return newError(requestId, RestStatus.FORBIDDEN, "AccessDenied", "Bad session token", ""); | ||
} | ||
permittedBucket = "temporary_bucket_test"; | ||
} else { | ||
return newError(requestId, RestStatus.FORBIDDEN, "AccessDenied", "Bad access key", ""); | ||
} | ||
|
||
final RequestHandler handler = handlers.retrieve(method + " " + path, params); | ||
if (handler != null) { | ||
final String bucket = params.get("bucket"); | ||
if (bucket != null && permittedBucket.equals(bucket) == false) { | ||
// allow a null bucket to support bucket-free APIs like ListBuckets? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I don't think it's needed, returning an error seems the right thing to do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is needed, but it's not |
||
return newError(requestId, RestStatus.FORBIDDEN, "AccessDenied", "Bad bucket", ""); | ||
} | ||
return handler.execute(params, headers, body, requestId); | ||
} else { | ||
return newInternalError(requestId, "No handler defined for request [method: " + method + ", path: " + path + "]"); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: An -> A