Internal: Add versionless alias for rest client codebase in policy files #26521

rjernst · 2017-09-06T16:35:53Z

Security manager policy files contains grants for specific codebases,
where a codebase is a jar file. We use a system property containing the
name of the jar file to resolve the jar file location when parsing the
policy file. However, this means the version of the jars must be
modified when versions of dependencies change. This is particularly
messy for elasticsearch, where we now have a dependency on the rest
client, and need to support both a snapshot version for testing and non
snapshot for release.

This commit removes the version portion of the jar file name. The
implications are we will no longer need to edit policy files when
dependencies change, and do not need any special handling for snapshot
vs release builds and their internal policy files in the core
elasticsearch jar.

versions Security manager policy files contains grants for specific codebases, where a codebase is a jar file. We use a system property containing the name of the jar file to resolve the jar file location when parsing the policy file. However, this means the version of the jars must be modified when versions of dependencies change. This is particularly messy for elasticsearch, where we now have a dependency on the rest client, and need to support both a snapshot version for testing and non snapshot for release. This commit removes the version portion of the jar file name. The implications are we will no longer need to edit policy files when dependencies change, and do not need any special handling for snapshot vs release builds and their internal policy files in the core elasticsearch jar.

s1monw

LGTM

jasontedor

I don't like this change:

it's breaking for plugin developers in a minor release, and they would not get a compile-time failure
it's introducing leniency where I think there should not be any

jasontedor · 2017-09-06T17:00:42Z

I offered an alternate proposal which is to change the grants for our JARs to ${name-of-artifact}${es.version} where we set a property es.version to Version.CURRENT(-SNAPSHOT)? (the latter only if needed) at runtime. This does not break plugin developers, solves our problem, is not lenient, and prevents us from having to update these versions every time we bump versions.

jasontedor · 2017-09-06T17:01:35Z

core/src/main/java/org/elasticsearch/bootstrap/Security.java

                        continue; // tests :(
                    }
+                    String shortName = fileName.replaceFirst("-\\d+\\.\\d+.*\\.jar", ".jar");


This assumes a version format that while fairly standard is not guaranteed.

Sure it is not guaranteed to work for all possible version formats (specifically any that might use just a single version). But this will cover 99.9% of versions out there, and the edge case would not be for our own policy files (which is currently the only place we actually use jar specific grants). If there is a version format that is different, we can tweak this in the future, but this is still progress.

rjernst · 2017-09-06T18:46:27Z

I offered an alternate proposal which is to change the grants for our JARs to ${name-of-artifact}${es.version}

This would also be breaking for plugin developers. The "name-of-artifact" is not what we produce right now. We just put the property in "codebase.filename", which includes the version and .jar extension.

rjernst · 2017-09-06T18:49:38Z

And note that "breaking" for plugin developers is debatable. None of the official plugins use jar specific grants. Nor (I believe) do our examples for plugin developers to add their policy files. And finally, we can make this fail with a build check which validates the policy file, but that would require doing a first pass parse of the policy file to search for property uses (which i would only look for the simple ${property} format, anything more advanced may be used but not would not be validated.

rjernst · 2017-09-06T18:53:35Z

it's introducing leniency where I think there should not be any

I don't see this as leniency at all. It is not the policy parsers job to find if someone dropped a different version jar into lib. If we want to validate jars, there are other ways (don't use wildcard classpath, add jar signing). This is only about the arbitrary system property names we use to pass info between ES startup and the policy files.

s1monw · 2017-09-06T19:15:53Z

can we maybe reduce the scope here and say we only stip off major.minor.bugifx[alpha|beta]-SNAPSHOT for codebases that start with elasticsearch- I think this would make it safer and we likely would want to control all others based on their actual version. We might do a followup for lucene jars to prevent the snapshots updates from manually editing stuff?

s1monw · 2017-09-06T20:07:57Z

to be honest I would be happy to really just fix this for codebase.elasticsearch-rest-client-7.0.0-alpha1-SNAPSHOT.jar for now and then we can try to find a generic solution.

jasontedor · 2017-09-06T21:03:59Z

This would also be breaking for plugin developers. The "name-of-artifact" is not what we produce right now. We just put the property in "codebase.filename", which includes the version and .jar extension.

I said for our JARs, it would not break plugins. I meant making this change only for our artifacts, it would not break external plugins at all. In the short term, since we can not easily identify our JARs:

implement the ${es.version} change that I am proposing
hack this only for elasticsearch-rest-client (elasticsearch- is too weak, it could catch plugins that start with that name)

In the long term:

carry forward the ${es.version} change that I am proposing
sign our JARs
implement this change for all JARs signed by our key

jasontedor · 2017-09-06T21:10:27Z

And note that "breaking" for plugin developers is debatable.

It's not debatable. There's nothing that precludes a plugin author from using JAR-specific grants with the syntax that we use for the core policy file.

None of the official plugins use jar specific grants.

They do not. However, the syntax is in our core policy file, in the open source in our reindex module and the transport-netty4 modules do JAR-specific grants. In closed source, we do this in X-Pack.

s1monw · 2017-09-06T21:12:38Z

another option is to read the jars manifest and if it's a jar build by our build system (maybe just check for some X-Compile we can do it automatically for all the jars we control?

jasontedor · 2017-09-06T21:34:43Z

Here's a version that implements what I'm suggesting and I think can be iterated on to work towards what @s1monw is suggesting with the manifest to identify our JARs (and eventually signed JARs):

diff --git a/core/src/main/java/org/elasticsearch/bootstrap/Elasticsearch.java b/core/src/main/java/org/elasticsearch/bootstrap/Elasticsearch.java
index cfe73459a0..372458afb7 100644
--- a/core/src/main/java/org/elasticsearch/bootstrap/Elasticsearch.java
+++ b/core/src/main/java/org/elasticsearch/bootstrap/Elasticsearch.java
@@ -81,6 +81,7 @@ class Elasticsearch extends EnvironmentAwareCommand {
             }
         });
         LogConfigurator.registerErrorListener();
+        System.setProperty("es.version", Version.CURRENT.toString() + (Build.CURRENT.isSnapshot() ? "-SNAPSHOT" : ""));
         final Elasticsearch elasticsearch = new Elasticsearch();
         int status = main(args, elasticsearch, Terminal.DEFAULT);
         if (status != ExitCodes.OK) {
diff --git a/core/src/main/java/org/elasticsearch/bootstrap/Security.java b/core/src/main/java/org/elasticsearch/bootstrap/Security.java
index e5c326b8ee..4eb6b8de94 100644
--- a/core/src/main/java/org/elasticsearch/bootstrap/Security.java
+++ b/core/src/main/java/org/elasticsearch/bootstrap/Security.java
@@ -198,7 +198,15 @@ final class Security {
                     if (shortName.endsWith(".jar") == false) {
                         continue; // tests :(
                     }
-                    String previous = System.setProperty("codebase." + shortName, url.toString());
+                    final String previous;
+                    if (shortName.startsWith("elasticsearch-rest-client")) {
+                        final String urlAsString = url.toString();
+                        final int index = urlAsString.indexOf("-" + System.getProperty("es.version") + ".jar");
+                        assert index >= 0;
+                        previous = System.setProperty(policyPropertyName(shortName), urlAsString.substring(0, index));
+                    } else {
+                        previous = System.setProperty(policyPropertyName(shortName), url.toString());
+                    }
                     if (previous != null) {
                         throw new IllegalStateException("codebase property already set: " + shortName + "->" + previous);
                     }
@@ -211,7 +219,7 @@ final class Security {
                     if (shortName.endsWith(".jar") == false) {
                         continue; // tests :(
                     }
-                    System.clearProperty("codebase." + shortName);
+                    System.clearProperty(policyPropertyName(shortName));
                 }
             }
         } catch (NoSuchAlgorithmException | URISyntaxException e) {
@@ -219,6 +227,17 @@ final class Security {
         }
     }
 
+    private static String policyPropertyName(final String shortName) {
+        // hack for now until we can identify all of our jar
+        if (shortName.startsWith("elasticsearch-rest-client")) {
+            final int index = shortName.indexOf("-" + System.getProperty("es.version") + ".jar");
+            assert index >= 0;
+            return "codebase." + shortName.substring(0, index);
+        } else {
+            return "codebase." + shortName;
+        }
+    }
+
     /** returns dynamic Permissions to configured paths and bind ports */
     static Permissions createPermissions(Environment environment) throws IOException {
         Permissions policy = new Permissions();
diff --git a/core/src/main/resources/org/elasticsearch/bootstrap/test-framework.policy b/core/src/main/resources/org/elasticsearch/bootstrap/test-framework.policy
index 79cb42214d..840f182ada 100644
--- a/core/src/main/resources/org/elasticsearch/bootstrap/test-framework.policy
+++ b/core/src/main/resources/org/elasticsearch/bootstrap/test-framework.policy
@@ -63,7 +63,7 @@ grant codeBase "${codebase.mocksocket-1.2.jar}" {
   permission java.net.SocketPermission "*", "accept,connect";
 };
 
-grant codeBase "${codebase.elasticsearch-rest-client-7.0.0-alpha1-SNAPSHOT.jar}" {
+grant codeBase "${codebase.elasticsearch-rest-client}-${es.version}.jar" {
   // rest makes socket connections for rest tests
   permission java.net.SocketPermission "*", "connect";
   // rest client uses system properties which gets the default proxy
diff --git a/modules/reindex/src/main/plugin-metadata/plugin-security.policy b/modules/reindex/src/main/plugin-metadata/plugin-security.policy
index 39c1d77277..8f63708cbb 100644
--- a/modules/reindex/src/main/plugin-metadata/plugin-security.policy
+++ b/modules/reindex/src/main/plugin-metadata/plugin-security.policy
@@ -22,7 +22,7 @@ grant {
    permission java.net.SocketPermission "*", "connect";
 };
 
-grant codeBase "${codebase.elasticsearch-rest-client-7.0.0-alpha1-SNAPSHOT.jar}" {
+grant codeBase "${codebase.elasticsearch-rest-client}-${es.version}.jar" {
   // rest client uses system properties which gets the default proxy
   permission java.net.NetPermission "getProxySelector";
 };

rjernst · 2017-09-06T21:59:43Z

@jasontedor I pushed an update using your suggestion. I do not like it, but I am ok with it as a temporary solution.

s1monw · 2017-09-06T22:18:18Z

I think the version is adding unnecessary complexity here. Lets just use grant codeBase "${codebase.elasticsearch-rest-client} and special case it. The version is unnecessary. We can just do this special case iff it's the current version such that codebase.elasticsearch-rest-client always refers to the current version. you can still do codebase.elasticsearch-rest-client-4.5.6-beta-SNAPSHOT if you want to. Lets just make codebase.elasticsearch-rest-client an alias for the current version. We can generalize this in a followup

jasontedor

I left a comment.

jasontedor · 2017-09-06T22:27:18Z

core/src/main/java/org/elasticsearch/bootstrap/Security.java

+                    }
+                    String property = "codebase." + shortName;
+                    propertiesSet.add(property);
+                    String previous = System.setProperty(property, url.toString());


This doesn't work. Note that

"${codebase.elasticsearch-rest-client}-${es.version}.jar"

translates to

file:///path/to/elasticsearch-rest-client-7.0.0-alpha1-SNAPSHOT.jar-7.0.0-alpha1-SNAPSHOT.jar

because you're replacing codebase.elasticsearch-rest-client with the full URL string, and then appending the version (from es.version). I handled this in the diff I gave.

jasontedor · 2017-09-06T22:29:42Z

I think the version is adding unnecessary complexity here.

Note that we have to do the version parsing/handling anyway, to find the name elasticsearch-rest-client from the filename of the JAR that we are handling. It's only an incremental step so that then we are granting to exactly the version that we expect at runtime.

rjernst · 2017-09-06T22:34:32Z

so that then we are granting to exactly the version that we expect at runtime.

This is what is unnecessary. It is not the policy parsing code's job to stop other jars from being dropped in place of existing deps. Let us leave that validation (which would be good) for a separate discussion.

rjernst · 2017-09-06T22:42:27Z

@s1monw @jasontedor I pushed another update.

s1monw

LGTM lets move forward with this please

s1monw · 2017-09-07T00:44:23Z

core/src/main/java/org/elasticsearch/bootstrap/Security.java

+                    if (shortName.startsWith("elasticsearch-rest-client")) {
+                        final String esVersion = Version.CURRENT + (Build.CURRENT.isSnapshot() ? "-SNAPSHOT" : "");
+                        final int index = property.indexOf("-" + esVersion + ".jar");
+                        assert index >= 0;


can you put in a message for this?

s1monw · 2017-09-07T00:45:03Z

core/src/main/java/org/elasticsearch/bootstrap/Security.java

-                    }
-                    System.clearProperty("codebase." + shortName);
+                for (String property : propertiesSet) {
+                    System.out.println("Unsetting " + property);


this is a leftover?

jasontedor

I do not like it, but I am ok with it as a temporary solution.

…les (#26521) Security manager policy files contains grants for specific codebases, where a codebase is a jar file. We use a system property containing the name of the jar file to resolve the jar file location when parsing the policy file. However, this means the version of the jars must be modified when versions of dependencies change. This is particularly messy for elasticsearch, where we now have a dependency on the rest client, and need to support both a snapshot version for testing and non snapshot for release. This commit adds an alias for the elasticsearch rest client without a version to be used in policy files. That allows the policy files to not care whether the rest client is a snapshot or release.

…ties This is a followup to elastic#26521. This commit expands the alias added for the elasticsearch client codebase to all codebases. The original full jar name property is left intact. This only adds an alias without the version, which should help ease the pain in updating any versions (ES itself or dependencies).

…ties (#26756) This is a followup to #26521. This commit expands the alias added for the elasticsearch client codebase to all codebases. The original full jar name property is left intact. This only adds an alias without the version, which should help ease the pain in updating any versions (ES itself or dependencies).

rjernst added :Internal >bug v5.6.0 v6.0.0 v6.1.0 v7.0.0 labels Sep 6, 2017

s1monw approved these changes Sep 6, 2017

View reviewed changes

jasontedor requested changes Sep 6, 2017

View reviewed changes

jasontedor reviewed Sep 6, 2017

View reviewed changes

alternative

68a84d9

jasontedor requested changes Sep 6, 2017

View reviewed changes

remove es.version

896c351

iter

f04ad17

s1monw approved these changes Sep 7, 2017

View reviewed changes

add comment and remove debug

cebf67a

jasontedor approved these changes Sep 7, 2017

View reviewed changes

rjernst merged commit c9964d1 into elastic:master Sep 7, 2017

rjernst changed the title ~~Internal: Change policy file system properties to not contain jar versions~~ Internal: Add versionless alias for rest client codebase in policy files Sep 7, 2017

rjernst deleted the grant_no_version branch September 7, 2017 01:57

colings86 added v6.0.0-rc1 and removed v6.0.0 labels Sep 22, 2017

rjernst mentioned this pull request Sep 22, 2017

Plugins: Add versionless alias to all security policy codebase properties #26756

Merged

lcawl removed the v6.1.0 label Dec 12, 2017

jpountz removed the v7.0.0 label Jan 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Internal: Add versionless alias for rest client codebase in policy files #26521

Internal: Add versionless alias for rest client codebase in policy files #26521

rjernst commented Sep 6, 2017

s1monw left a comment

jasontedor left a comment •

edited

Loading

jasontedor commented Sep 6, 2017 •

edited

Loading

jasontedor Sep 6, 2017

rjernst Sep 6, 2017

rjernst commented Sep 6, 2017

rjernst commented Sep 6, 2017

rjernst commented Sep 6, 2017

s1monw commented Sep 6, 2017

s1monw commented Sep 6, 2017

jasontedor commented Sep 6, 2017

jasontedor commented Sep 6, 2017

s1monw commented Sep 6, 2017

jasontedor commented Sep 6, 2017

rjernst commented Sep 6, 2017

s1monw commented Sep 6, 2017

jasontedor left a comment

jasontedor Sep 6, 2017

jasontedor commented Sep 6, 2017

rjernst commented Sep 6, 2017

rjernst commented Sep 6, 2017

s1monw left a comment

s1monw Sep 7, 2017

s1monw Sep 7, 2017

jasontedor left a comment

Internal: Add versionless alias for rest client codebase in policy files #26521

Internal: Add versionless alias for rest client codebase in policy files #26521

Conversation

rjernst commented Sep 6, 2017

s1monw left a comment

Choose a reason for hiding this comment

jasontedor left a comment • edited Loading

Choose a reason for hiding this comment

jasontedor commented Sep 6, 2017 • edited Loading

jasontedor Sep 6, 2017

Choose a reason for hiding this comment

rjernst Sep 6, 2017

Choose a reason for hiding this comment

rjernst commented Sep 6, 2017

rjernst commented Sep 6, 2017

rjernst commented Sep 6, 2017

s1monw commented Sep 6, 2017

s1monw commented Sep 6, 2017

jasontedor commented Sep 6, 2017

jasontedor commented Sep 6, 2017

s1monw commented Sep 6, 2017

jasontedor commented Sep 6, 2017

rjernst commented Sep 6, 2017

s1monw commented Sep 6, 2017

jasontedor left a comment

Choose a reason for hiding this comment

jasontedor Sep 6, 2017

Choose a reason for hiding this comment

jasontedor commented Sep 6, 2017

rjernst commented Sep 6, 2017

rjernst commented Sep 6, 2017

s1monw left a comment

Choose a reason for hiding this comment

s1monw Sep 7, 2017

Choose a reason for hiding this comment

s1monw Sep 7, 2017

Choose a reason for hiding this comment

jasontedor left a comment

Choose a reason for hiding this comment

jasontedor left a comment •

edited

Loading

jasontedor commented Sep 6, 2017 •

edited

Loading