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

Add EC2 credential test for repository-s3 #31918

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
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 @@ -137,7 +137,7 @@ class ClusterConfiguration {
this.project = project
}

Map<String, String> systemProperties = new HashMap<>()
Map<String, Object> systemProperties = new HashMap<>()
Copy link
Contributor

Choose a reason for hiding this comment

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

The difference is easy to overlook , maybe it would be good to have a comment explaining why Object is needed.
We might be able to have Map<String, GString> to make the intention cleaner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

💯


Map<String, Object> settings = new HashMap<>()

Expand All @@ -157,7 +157,7 @@ class ClusterConfiguration {
List<Object> dependencies = new ArrayList<>()

@Input
void systemProperty(String property, String value) {
void systemProperty(String property, Object value) {
systemProperties.put(property, value)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,7 @@ class ClusterFormationTasks {
setup = configureCreateKeystoreTask(taskName(prefix, node, 'createKeystore'), project, setup, node)
setup = configureAddKeystoreSettingTasks(prefix, project, setup, node)
setup = configureAddKeystoreFileTasks(prefix, project, setup, node)
setup = configureESJavaOpts(prefix, project, setup, node)

if (node.config.plugins.isEmpty() == false) {
if (node.nodeVersion == VersionProperties.elasticsearch) {
Expand Down Expand Up @@ -412,6 +413,31 @@ class ClusterFormationTasks {
return parentTask
}

/** Configure ES JAVA OPTS - adds system properties, assertion flags, remote debug etc */
static Task configureESJavaOpts(String parent, Project project, Task setup, NodeInfo node) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need a separate task for it, especially that the result is only stored in memory.
I would add this to the doFirst we already run on the start task.
We can just set ES_JAVA_OPTS before the start task, or concatenate it if already set.
The builds script could also set this. None of the currently do, but we should either concatenate to it if set, or fail if it's set if we don;t want to support it. I'm not saying we shouldn't just speculating on what the initial intent could have been.

Copy link
Contributor Author

@vladimirdolzhenko vladimirdolzhenko Jul 10, 2018

Choose a reason for hiding this comment

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

@atorok do you mean to put it to

project.logger.info("Starting node in ${node.clusterName} distribution: ${node.config.distribution}")
?

Copy link
Contributor

Choose a reason for hiding this comment

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

exactly

return project.tasks.create(name: taskName(parent, node, 'configureESJavaOpts'), type: DefaultTask, dependsOn: setup) {
doLast {
String collectedSystemProperties = node.config.systemProperties.collect { key, value -> "-D${key}=${value}" }.join(" ")
List<String> esJavaOpts = [collectedSystemProperties]
esJavaOpts.add(node.config.jvmArgs)
if (Boolean.parseBoolean(System.getProperty('tests.asserts', 'true'))) {
// put the enable assertions options before other options to allow
// flexibility to disable assertions for specific packages or classes
// in the cluster-specific options
esJavaOpts.add("-ea")
esJavaOpts.add("-esa")
}
// we must add debug options inside the closure so the config is read at execution time, as
// gradle task options are not processed until the end of the configuration phase
if (node.config.debug) {
println 'Running elasticsearch in debug mode, suspending until connected on port 8000'
esJavaOpts.add('-agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=8000')
}
node.env['ES_JAVA_OPTS'] = esJavaOpts.join(" ")
}
}
}

static Task configureExtraConfigFilesTask(String name, Project project, Task setup, NodeInfo node) {
if (node.config.extraConfigFiles.isEmpty()) {
return setup
Expand Down Expand Up @@ -624,13 +650,6 @@ class ClusterFormationTasks {
node.writeWrapperScript()
}

// we must add debug options inside the closure so the config is read at execution time, as
// gradle task options are not processed until the end of the configuration phase
if (node.config.debug) {
println 'Running elasticsearch in debug mode, suspending until connected on port 8000'
node.env['ES_JAVA_OPTS'] = '-agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=8000'
}

node.getCommandString().eachLine { line -> logger.info(line) }

if (logger.isInfoEnabled() || node.config.daemonize == false) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -180,15 +180,7 @@ class NodeInfo {
}

args.addAll("-E", "node.portsfile=true")
String collectedSystemProperties = config.systemProperties.collect { key, value -> "-D${key}=${value}" }.join(" ")
String esJavaOpts = config.jvmArgs.isEmpty() ? collectedSystemProperties : collectedSystemProperties + " " + config.jvmArgs
if (Boolean.parseBoolean(System.getProperty('tests.asserts', 'true'))) {
// put the enable assertions options before other options to allow
// flexibility to disable assertions for specific packages or classes
// in the cluster-specific options
esJavaOpts = String.join(" ", "-ea", "-esa", esJavaOpts)
}
env = ['ES_JAVA_OPTS': esJavaOpts]
env = [:]
Copy link
Contributor

Choose a reason for hiding this comment

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

this can be removed, the map is already initialized.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, map is null, here is the output if I drop this line:

Cannot invoke method put() on null object

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad, this class initializes in the constructor, this is ok as it is.

for (Map.Entry<String, String> property : System.properties.entrySet()) {
if (property.key.startsWith('tests.es.')) {
args.add("-E")
Expand Down
8 changes: 7 additions & 1 deletion plugins/repository-s3/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,9 @@ Map<String, Object> expansions = [
'permanent_bucket': s3PermanentBucket,
'permanent_base_path': s3PermanentBasePath,
'temporary_bucket': s3TemporaryBucket,
'temporary_base_path': s3TemporaryBasePath
'temporary_base_path': s3TemporaryBasePath,
'ec2_bucket': s3TemporaryBucket,
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean for this to be the same as the entry for temporary_bucket, and similarly for ec2_base_path? I think they should be different.

'ec2_base_path': s3TemporaryBasePath
]

processTestResources {
Expand All @@ -309,6 +311,10 @@ integTestCluster {
/* Use a closure on the string to delay evaluation until tests are executed */
setting 's3.client.integration_test_permanent.endpoint', "http://${-> s3Fixture.addressAndPort}"
setting 's3.client.integration_test_temporary.endpoint', "http://${-> s3Fixture.addressAndPort}"
setting 's3.client.integration_test_ec2.endpoint', "http://${-> s3Fixture.addressAndPort}"

// to redirect InstanceProfileCredentialsProvider to custom auth point
systemProperty "com.amazonaws.sdk.ec2MetadataServiceEndpointOverride", "http://${-> s3Fixture.addressAndPort}"
} else {
println "Using an external service to test the repository-s3 plugin"
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.concurrent.TimeUnit;
import java.util.function.Function;

import static java.nio.charset.StandardCharsets.UTF_8;

Expand Down Expand Up @@ -70,17 +72,24 @@ private AmazonS3Fixture(final String workingDir, final String permanentBucketNam

@Override
protected Response handle(final Request request) throws IOException {
final RequestHandler handler = handlers.retrieve(request.getMethod() + " " + request.getPath(), request.getParameters());
final String nonAuthorizedPath = "* " + request.getMethod() + " " + request.getPath();
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a big fan of the * vs A thing, at least not as literals. Could you introduce constants so these things have more descriptive names?

final RequestHandler nonAuthorizedHandler = handlers.retrieve(nonAuthorizedPath, request.getParameters());
if (nonAuthorizedHandler != null) {
return nonAuthorizedHandler.handle(request);
}

final String authorizedPath = "A " + request.getMethod() + " " + request.getPath();
final RequestHandler handler = handlers.retrieve(authorizedPath, request.getParameters());
if (handler != null) {
final String authorization = request.getHeader("Authorization");
final String permittedBucket;
if (authorization.contains("s3_integration_test_permanent_access_key")) {
if (authorization != null && authorization.contains("s3_integration_test_permanent_access_key")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we handle the authorisation == null case first to avoid these null checks proliferating? It's also helpful to distinguish no authorisation header vs a bad one, which this would achieve.

final String sessionToken = request.getHeader("x-amz-security-token");
if (sessionToken != null) {
return newError(request.getId(), RestStatus.FORBIDDEN, "AccessDenied", "Unexpected session token", "");
}
permittedBucket = permanentBucketName;
} else if (authorization.contains("s3_integration_test_temporary_access_key")) {
} else if (authorization != null && authorization.contains("s3_integration_test_temporary_access_key")) {
final String sessionToken = request.getHeader("x-amz-security-token");
if (sessionToken == null) {
return newError(request.getId(), RestStatus.FORBIDDEN, "AccessDenied", "No session token", "");
Expand All @@ -89,6 +98,15 @@ protected Response handle(final Request request) throws IOException {
return newError(request.getId(), RestStatus.FORBIDDEN, "AccessDenied", "Bad session token", "");
}
permittedBucket = temporaryBucketName;
} else if (authorization != null && authorization.contains("securitycredentials42_KEYID")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we extract securitycredentials42 out as a named constant so it's clear why it is what it is?

final String sessionToken = request.getHeader("x-amz-security-token");
if (sessionToken == null) {
return newError(request.getId(), RestStatus.FORBIDDEN, "AccessDenied", "No session token", "");
}
if (sessionToken.equals("securitycredentials42_TKN") == false) {
return newError(request.getId(), RestStatus.FORBIDDEN, "AccessDenied", "Bad session token", "");
}
permittedBucket = temporaryBucketName;
Copy link
Contributor

Choose a reason for hiding this comment

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

As commented above, I think this should have its own bucket.

} else {
return newError(request.getId(), RestStatus.FORBIDDEN, "AccessDenied", "Bad access key", "");
}
Expand Down Expand Up @@ -129,7 +147,7 @@ private static PathTrie<RequestHandler> defaultHandlers(final Map<String, Bucket
// HEAD Object
//
// https://docs.aws.amazon.com/AmazonS3/latest/API/RESTObjectHEAD.html
objectsPaths("HEAD /{bucket}").forEach(path ->
objectsPaths("A HEAD /{bucket}").forEach(path ->
handlers.insert(path, (request) -> {
final String bucketName = request.getParam("bucket");

Expand All @@ -151,7 +169,7 @@ private static PathTrie<RequestHandler> defaultHandlers(final Map<String, Bucket
// PUT Object
//
// https://docs.aws.amazon.com/AmazonS3/latest/API/RESTObjectPUT.html
objectsPaths("PUT /{bucket}").forEach(path ->
objectsPaths("A PUT /{bucket}").forEach(path ->
handlers.insert(path, (request) -> {
final String destBucketName = request.getParam("bucket");

Expand Down Expand Up @@ -201,7 +219,7 @@ private static PathTrie<RequestHandler> defaultHandlers(final Map<String, Bucket
// DELETE Object
//
// https://docs.aws.amazon.com/AmazonS3/latest/API/RESTObjectDELETE.html
objectsPaths("DELETE /{bucket}").forEach(path ->
objectsPaths("A DELETE /{bucket}").forEach(path ->
handlers.insert(path, (request) -> {
final String bucketName = request.getParam("bucket");

Expand All @@ -219,7 +237,7 @@ private static PathTrie<RequestHandler> defaultHandlers(final Map<String, Bucket
// GET Object
//
// https://docs.aws.amazon.com/AmazonS3/latest/API/RESTObjectGET.html
objectsPaths("GET /{bucket}").forEach(path ->
objectsPaths("A GET /{bucket}").forEach(path ->
handlers.insert(path, (request) -> {
final String bucketName = request.getParam("bucket");

Expand All @@ -240,7 +258,7 @@ private static PathTrie<RequestHandler> defaultHandlers(final Map<String, Bucket
// HEAD Bucket
//
// https://docs.aws.amazon.com/AmazonS3/latest/API/RESTBucketHEAD.html
handlers.insert("HEAD /{bucket}", (request) -> {
handlers.insert("A HEAD /{bucket}", (request) -> {
String bucket = request.getParam("bucket");
if (Strings.hasText(bucket) && buckets.containsKey(bucket)) {
return new Response(RestStatus.OK.getStatus(), TEXT_PLAIN_CONTENT_TYPE, EMPTY_BYTE);
Expand All @@ -252,7 +270,7 @@ private static PathTrie<RequestHandler> defaultHandlers(final Map<String, Bucket
// GET Bucket (List Objects) Version 1
//
// https://docs.aws.amazon.com/AmazonS3/latest/API/RESTBucketGET.html
handlers.insert("GET /{bucket}/", (request) -> {
handlers.insert("A GET /{bucket}/", (request) -> {
final String bucketName = request.getParam("bucket");

final Bucket bucket = buckets.get(bucketName);
Expand All @@ -270,7 +288,7 @@ private static PathTrie<RequestHandler> defaultHandlers(final Map<String, Bucket
// Delete Multiple Objects
//
// https://docs.aws.amazon.com/AmazonS3/latest/API/multiobjectdeleteapi.html
handlers.insert("POST /", (request) -> {
handlers.insert("A POST /", (request) -> {
final List<String> deletes = new ArrayList<>();
final List<String> errors = new ArrayList<>();

Expand Down Expand Up @@ -312,6 +330,40 @@ private static PathTrie<RequestHandler> defaultHandlers(final Map<String, Bucket
return newInternalError(request.getId(), "Something is wrong with this POST multiple deletes request");
});

// non-authorized requests

Function<String, Response> credentialResponseFunction = prefix -> {
final Date expiration = new Date(new Date().getTime() + TimeUnit.DAYS.toMillis(1));
final String response = "{"
+ "\"AccessKeyId\": \"" + prefix + "_KEYID" + "\","
+ "\"Expiration\": \"" + DateUtils.formatISO8601Date(expiration) + "\","
+ "\"RoleArn\": \"" + prefix + "_ROLE" + "\","
+ "\"SecretAccessKey\": \"" + prefix + "_SCR_KEY" + "\","
+ "\"Token\": \"" + prefix + "_TKN" + "\""
+ "}";

final Map<String, String> headers = new HashMap<>(contentType("application/json"));
return new Response(RestStatus.OK.getStatus(), headers, response.getBytes(UTF_8));
};

// GET
//
// http://docs.aws.amazon.com/AWSEC2/latest/UserGuide/iam-roles-for-amazon-ec2.html
handlers.insert("* GET /latest/meta-data/iam/security-credentials/", (request) -> {
final String response = "securitycredentials42";

final Map<String, String> headers = new HashMap<>(contentType("text/plain"));
return new Response(RestStatus.OK.getStatus(), headers, response.getBytes(UTF_8));
});

// GET
//
// http://docs.aws.amazon.com/AWSEC2/latest/UserGuide/iam-roles-for-amazon-ec2.html
handlers.insert("* GET /latest/meta-data/iam/security-credentials/{credentials}", (request) -> {
final String credentials = request.getParam("credentials");
return credentialResponseFunction.apply(credentials);
Copy link
Contributor

Choose a reason for hiding this comment

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

On deeper thought, this seems unduly lenient: it should only return credentials for the role that GET /latest/meta-data/iam/security-credentials/ returned, and should return 404 otherwise.

Also I think credentialResponseFunction can be inlined, it's only used in one place.

Also also we could prevent cheating slightly more by inventing random credentials when the service starts up, rather than synthesising them from the role name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I extract it for doing ECS part as well - it has to return similar json,

for randomization ... I bit confused here in terms of how to reproduce it in case of failure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DaveCTurner s3 perm/temp authorization tokens have to be randomized as well - should we ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we cannot (yet) do this with auth tokens received externally because they need to be passed to the test suite as well as this fixture, so we've had to settle on just using the same (long) string in both places. It would indeed be nicer if there were no magic strings in the fixture at all.

For deterministic testing, maybe you can pass a seed in from Gradle somehow? Or just hard-code them, it's not that important. I'd rather they weren't synthesised as they are now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it sounds like it is better to provide some kind of properties file that contains tokens, paths etc - esp. keeping in mind coming ECS test

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, good plan.

});

return handlers;
}

Expand Down
Loading