Verify signatures on official plugins #30800

jasontedor · 2018-05-23T04:09:38Z

We sign our official plugins yet this is not well-advertised and not at all consumed during plugin installation. For plugins that are installed over the intertubes, verifying that the downloaded artifact is signed by our signing key would establish both integrity and validity of the downloaded artifact. The chain of trust here is simple: our installable artifacts (archive and package distributions) are signed so that if a user trusts our packages via their signatures, and our plugin installer (which would be executing trusted code) verifies the downloaded plugin, then the user can trust the downloaded plugin too. This commit adds verification of official plugins downloaded during installation. We do not add verification for offline plugin installs; a user can download our signatures and verify the artifacts themselves.

This commit also needs to solve a few interesting challenges. One of these is that we want the bouncy castle JARs on the classpath only for the plugin installer, but not for the runtime Elasticsearch. Additionally, we want these JARs to not be present for the JAR hell checks. To address this, we shift these JARs into a sub-directory of lib (lib/tools/plugin-cli) that is only loaded for the plugin installer, and in the plugin installer we filter any JARs in this directory from the JAR hell check.

We sign our official plugins yet this is not well-advertised and not at all consumed during plugin installation. For plugins that are installed over the intertubes, verifying that the downloaded artifact is signed by our signing key would establish both integrity and validity of the downloaded artifact. The chain of trust here is simple: our installable artifacts (archive and package distributions) so that if a user trusts our packages via their signatures, and our plugin installer (which would be executing trusted code) verifies the downloaded plugin, then the user can trust the downloaded plugin too. This commit adds verification of official plugins downloaded during installation. We do not add verification for offline plugin installs; a user can download our signatures and verify the artifacts themselves. This commit also needs to solve a few interesting challenges. One of these is that we want the bouncy castle JARs on the classpath only for the plugin installer, but not for the runtime Elasticsearch. Additionally, we want these JARs to not be present for the JAR hell checks. To address this, we shift these JARs into a sub-directory of lib (lib/tools/plugin-cli) that is only loaded for the plugin installer, and in the plugin installer we filter any JARs in this directory from the JAR hell check.

elasticmachine · 2018-05-23T04:09:40Z

Pinging @elastic/es-core-infra

jasontedor · 2018-05-23T04:10:49Z

Could I ask someone from @elastic/es-security to review this PR too?

rjernst

LGTM overall. I left a bunch of nits and minor requests

rjernst · 2018-05-23T05:23:09Z

distribution/build.gradle

-      from { project(':server').jar }
-      from { project(':server').configurations.runtime }
-      from { project(':libs:plugin-classloader').jar }
-      // delay add tools using closures, since they have not yet been configured, so no jar task exists yet


This comment is important to keep in some form. It is the reason all these from statements use a closure.

I added this comment back.

rjernst · 2018-05-23T05:32:05Z

distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java

+    void verifySignature(final Path zip, final String urlString) throws IOException, PGPException {
+        final String ascUrlString = urlString + ".asc";
+        final URL ascUrl = openUrl(ascUrlString);
+        try (// fin is a file stream over the downloaded plugin zip whose signature to verify


nit: try on its own line please

I made this change.

rjernst · 2018-05-23T05:33:37Z

distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java

+        return Files.newInputStream(zip);
+    }
+
+    String getPublicKeyId() {


At least a one line javadoc on each of these methods explaining these are not constants so tests can override would be nice

rjernst · 2018-05-23T05:33:51Z

distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java

+        Security.addProvider(new BouncyCastleProvider());
+    }
+
+    void verifySignature(final Path zip, final String urlString) throws IOException, PGPException {


a simple one line javadoc would be nice here

I added a Javadoc.

rjernst · 2018-05-23T05:38:39Z

distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java

@@ -605,11 +707,21 @@ private PluginInfo loadPluginInfo(Terminal terminal, Path pluginRoot, Environmen
        return info;
    }

+    private static final String LIB_TOOLS_PLUGIN_CLI_CLASSPATH_JAR;


nit: please put this at the top of the file with other constants/static blocks

rjernst · 2018-05-23T05:43:22Z

server/src/test/java/org/elasticsearch/common/hash/MessageDigestsTests.java

@@ -21,9 +21,11 @@

 import org.elasticsearch.test.ESTestCase;

+import java.io.File;


This file seems untouched except for added imports?

I am not sure what happened here, I reverted the changes.

rjernst · 2018-05-23T05:45:42Z

distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java

+    private static final String LIB_TOOLS_PLUGIN_CLI_CLASSPATH_JAR;
+
+    static {
+        LIB_TOOLS_PLUGIN_CLI_CLASSPATH_JAR =


nit: shouldn't this be jars, plural (ie it matches many jars, not just one)?

It is the pattern for matching a JAR under the lib/tools/plugin-cli path, it is only that there can be multiple matches.

tvernum

It looks right to me, but it's been years since I've used the BC PGP API, so I'll need to do another pass when I'm a bit more awake (unless someone else beats me to it).

tvernum · 2018-05-23T11:28:47Z

distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java

+    }
+
+    /**
+     * Downlaods a ZIP from the URL. This method also validates the downloaded plugin ZIP via the following means:


Nit: s/Downlaods/Downloads/

tvernum · 2018-05-23T11:40:17Z

...tion/tools/plugin-cli/src/test/java/org/elasticsearch/plugins/InstallPluginCommandTests.java

+                "https://artifacts.elastic.co/downloads/elasticsearch-plugins/analysis-icu/" + icu + "-" + Version.CURRENT + ".zip";
+        final MessageDigest digest = MessageDigest.getInstance("SHA-512");
+        /*
+         * To setup a situation where the expected public key ID does not match the public key ID used for singing, we generate a new public


s/sing/sign/

jaymode

Overall this looks good. I left some comments about the use of the BC provider; I'd prefer if we did not have to install it and we only use it for what we need to (the PGP stuff).

jaymode · 2018-05-23T13:36:41Z

distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java

        }

        return zip;
    }

+    static {
+        Security.addProvider(new BouncyCastleProvider());


I'm not sure if these BC apis will allow for this, but is it possible that we just use the provider directly rather that install it?

@jaymode Would you explain the problem with the simplest approach here, this is only in the CLI tool?

This could be considered paranoia on my part, but I just am not a fan of adding a security provider to be available globally even if it is just in a CLI tool. The provider being available allows for code that does not need bouncy castle to obtain a crypto primitive, like a SHA-256 message digest, from this provider. If the BC provider has a security bug in its implementation, this drives the need for us to ship a new version quickly; whereas if we rely on the JVMs configuration then the bug would most likely not necessitate a new release of our software.

As to a simple approach, here is a diff that removes the installation of the provider and instead uses an instance of it. gradle check passes on the plugin-cli project.

diff --git a/distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java b/distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java index 1cfa9b20b75..8e0daa49d7a 100644 --- a/distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java +++ b/distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java @@ -526,9 +526,7 @@ class InstallPluginCommand extends EnvironmentAwareCommand { return zip; } - static { - Security.addProvider(new BouncyCastleProvider()); - } + static final BouncyCastleProvider BOUNCY_CASTLE_PROVIDER = new BouncyCastleProvider(); /** * Verify the signature of the downloaded plugin ZIP. The signature is obtained from the source of the downloaded plugin by appending @@ -561,7 +559,7 @@ class InstallPluginCommand extends EnvironmentAwareCommand { // compute the signature of the downloaded plugin zip final PGPPublicKeyRingCollection collection = new PGPPublicKeyRingCollection(pin, new JcaKeyFingerprintCalculator()); final PGPPublicKey key = collection.getPublicKey(signature.getKeyID()); - signature.init(new JcaPGPContentVerifierBuilderProvider().setProvider("BC"), key); + signature.init(new JcaPGPContentVerifierBuilderProvider().setProvider(BOUNCY_CASTLE_PROVIDER), key); final byte[] buffer = new byte[1024]; int read; while ((read = fin.read(buffer)) != -1) { diff --git a/distribution/tools/plugin-cli/src/test/java/org/elasticsearch/plugins/InstallPluginCommandTests.java b/distribution/tools/plugin-cli/src/test/java/org/elasticsearch/plugins/InstallPluginCommandTests.java index 89c747b6c2a..da6b2a11a2a 100644 --- a/distribution/tools/plugin-cli/src/test/java/org/elasticsearch/plugins/InstallPluginCommandTests.java +++ b/distribution/tools/plugin-cli/src/test/java/org/elasticsearch/plugins/InstallPluginCommandTests.java @@ -1143,7 +1143,7 @@ public class InstallPluginCommandTests extends ESTestCase { } public PGPSecretKey newSecretKey() throws NoSuchAlgorithmException, NoSuchProviderException, PGPException { - final KeyPairGenerator kpg = KeyPairGenerator.getInstance("RSA", "BC"); + final KeyPairGenerator kpg = KeyPairGenerator.getInstance("RSA"); kpg.initialize(2048); final KeyPair pair = kpg.generateKeyPair(); final PGPDigestCalculator sha1Calc = new JcaPGPDigestCalculatorProviderBuilder().build().get(HashAlgorithmTags.SHA1); @@ -1156,7 +1156,9 @@ public class InstallPluginCommandTests extends ESTestCase { null, null, new JcaPGPContentSignerBuilder(pkp.getPublicKey().getAlgorithm(), HashAlgorithmTags.SHA1), - new JcePBESecretKeyEncryptorBuilder(PGPEncryptedData.CAST5, sha1Calc).setProvider("BC").build("passphrase".toCharArray())); + new JcePBESecretKeyEncryptorBuilder(PGPEncryptedData.CAST5, sha1Calc) + .setProvider(InstallPluginCommand.BOUNCY_CASTLE_PROVIDER) + .build("passphrase".toCharArray())); } private Function<byte[], String> checksum(final MessageDigest digest) {

Thanks for the explanation. That works for me. I pushed 50fdd05. Would you take another look @jaymode?

jaymode · 2018-05-23T13:41:37Z

...tion/tools/plugin-cli/src/test/java/org/elasticsearch/plugins/InstallPluginCommandTests.java

+    }
+
+    public PGPSecretKey newSecretKey() throws NoSuchAlgorithmException, NoSuchProviderException, PGPException {
+        final KeyPairGenerator kpg = KeyPairGenerator.getInstance("RSA", "BC");


do we need to use the BC provider generating a RSA key pair?

* master: (25 commits) [DOCS] Splits auditing.asciidoc into smaller files Reintroduce mandatory http pipelining support (elastic#30820) Painless: Types Section Clean Up (elastic#30283) Add support for indexed shape routing in geo_shape query (elastic#30760) [test] java tests for archive packaging (elastic#30734) Revert "Make http pipelining support mandatory (elastic#30695)" (elastic#30813) [DOCS] Fix more edit URLs in Stack Overview (elastic#30704) Use correct cluster state version for node fault detection (elastic#30810) Change serialization version of doc-value fields. [DOCS] Fixes broken link for native realm [DOCS] Clarified audit.index.client.hosts (elastic#30797) [TEST] Don't expect acks when isolating nodes Add a `format` option to `docvalue_fields`. (elastic#29639) Fixes UpdateSettingsRequestStreamableTests mutate bug Mustes {p0=snapshot.get_repository/10_basic/*} YAML test Revert "Mutes MachineLearningTests.testNoAttributes_givenSameAndMlEnabled" Only allow x-pack metadata if all nodes are ready (elastic#30743) Mutes MachineLearningTests.testNoAttributes_givenSameAndMlEnabled Use original settings on full-cluster restart (elastic#30780) Only ack cluster state updates successfully applied on all nodes (elastic#30672) ...

rjernst

LGTM

rjernst · 2018-05-24T19:31:42Z

distribution/packages/build.gradle

+          eachFile { FileCopyDetails fcp ->
+            String[] segments = fcp.relativePath.segments
+            for (int i = segments.length - 2; i > 0 && segments[i] != 'lib'; --i) {
+              System.out.println(segments[0..i])


leftover println

rjernst · 2018-05-24T19:32:11Z

distribution/packages/build.gradle

          eachFile { FileCopyDetails fcp ->
            String[] segments = fcp.relativePath.segments
            for (int i = segments.length - 2; i > 0 && segments[i] != 'modules'; --i) {
+              System.out.println(segments[0..i])


leftover println

* master: Update the version checks around ip_range bucket keys, now that the change was backported. Mute IndexMasterFailoverIT.testMasterFailoverDuringIndexingWithMappingChanges Use geohash cell instead of just a corner in geo_bounding_box (elastic#30698) Limit user to single concurrent auth per realm (elastic#30794) [Tests] Move templated _rank_eval tests (elastic#30679) Security: fix dynamic mapping updates with aliases (elastic#30787) Ensure that ip_range aggregations always return bucket keys. (elastic#30701) Use remote client in TransportFieldCapsAction (elastic#30838) Move Watcher versioning setting to meta field (elastic#30832) [Docs] Explain incomplete dates in range queries (elastic#30689) Move persistent task registrations to core (elastic#30755) Decouple ClusterStateTaskListener & ClusterApplier (elastic#30809) Send client headers from TransportClient (elastic#30803) Packaging: Ensure upgrade_is_oss flag file is always deleted (elastic#30732) Force stable file modes for built packages (elastic#30823) [DOCS] Fixes typos in security settings Fix GeoShapeQueryBuilder serialization after backport

jaymode

LGTM

* es/master: [TEST] Mute {p0=snapshot.get_repository/10_basic/Verify created repository} YAML test Verify signatures on official plugins (#30800) [Docs] Add reindex.remote.whitelist example (#30828)

We sign our official plugins yet this is not well-advertised and not at all consumed during plugin installation. For plugins that are installed over the intertubes, verifying that the downloaded artifact is signed by our signing key would establish both integrity and validity of the downloaded artifact. The chain of trust here is simple: our installable artifacts (archive and package distributions) so that if a user trusts our packages via their signatures, and our plugin installer (which would be executing trusted code) verifies the downloaded plugin, then the user can trust the downloaded plugin too. This commit adds verification of official plugins downloaded during installation. We do not add verification for offline plugin installs; a user can download our signatures and verify the artifacts themselves. This commit also needs to solve a few interesting challenges. One of these is that we want the bouncy castle JARs on the classpath only for the plugin installer, but not for the runtime Elasticsearch. Additionally, we want these JARs to not be present for the JAR hell checks. To address this, we shift these JARs into a sub-directory of lib (lib/tools/plugin-cli) that is only loaded for the plugin installer, and in the plugin installer we filter any JARs in this directory from the JAR hell check.

* 6.x: Fix double semicolon in import statement [TEST] Fix minor random bug from #30794 Enabling testing against an external cluster (#30885) SQL: Remove the last remaining server dependencies from jdbc (#30771) Add public key header/footer (#30877) Include size of snapshot in snapshot metadata (#29602) QA: Test template creation during rolling restart (#30850) REST high-level client: add put ingest pipeline API (#30793) Do not serialize basic license exp in x-pack info (#30848) [docs] explainer for java packaging tests (#30825) Verify signatures on official plugins (#30800) [DOCS] Document index name limitations (#30826) [Docs] Add reindex.remote.whitelist example (#30828)

jasontedor added review release highlight :Core/Infra/Plugins Plugin API and infrastructure v7.0.0 v6.4.0 labels May 23, 2018

jasontedor requested a review from rjernst May 23, 2018 04:09

Adjust imports

a75d4a0

rjernst approved these changes May 23, 2018

View reviewed changes

tvernum reviewed May 23, 2018

View reviewed changes

jaymode reviewed May 23, 2018

View reviewed changes

jasontedor added 12 commits May 23, 2018 18:47

Fix regular expression

22767a4

Inline method

ca53357

Preserve comment

62f5e93

Put try on its own line

c99106b

Javadocs

f73c59b

Fix Windows plugin script

d95f2e9

Fix unintended import changes

163e1c9

Fix typos

1f7b1ba

Fix tools sub-dir

701d36a

Remove sout

a9350f3

Remove adding provider

50fdd05

rjernst approved these changes May 24, 2018

View reviewed changes

jasontedor added 2 commits May 24, 2018 18:05

Fix location

ac911dc

jaymode approved these changes May 25, 2018

View reviewed changes

jasontedor merged commit d31e10a into elastic:master May 25, 2018

jasontedor deleted the verify-official-plugin-signature branch May 25, 2018 15:43

jasontedor added the >enhancement label Aug 20, 2018

xeraa mentioned this pull request Aug 30, 2018

Feature request: Could the docker image allow more customization so that we don’t have to keep making custom images based on this one Elasticsearch elastic/elasticsearch-docker#189

Closed

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

jkakavas mentioned this pull request Apr 16, 2019

Use RSA/DSA/EC instead of PGP signatures for official plugins #41263

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Verify signatures on official plugins #30800

Verify signatures on official plugins #30800

jasontedor commented May 23, 2018 •

edited

Loading

elasticmachine commented May 23, 2018

jasontedor commented May 23, 2018

rjernst left a comment

rjernst May 23, 2018

jasontedor May 24, 2018

rjernst May 23, 2018

jasontedor May 24, 2018

rjernst May 23, 2018

rjernst May 23, 2018

jasontedor May 24, 2018

rjernst May 23, 2018

rjernst May 23, 2018

jasontedor May 24, 2018

rjernst May 23, 2018

jasontedor May 24, 2018

tvernum left a comment

tvernum May 23, 2018

tvernum May 23, 2018

jaymode left a comment

jaymode May 23, 2018

jasontedor May 24, 2018

jaymode May 24, 2018

jasontedor May 24, 2018

jaymode May 23, 2018

rjernst left a comment

rjernst May 24, 2018

rjernst May 24, 2018

jaymode left a comment

		@@ -21,9 +21,11 @@

		import org.elasticsearch.test.ESTestCase;

		import java.io.File;

Verify signatures on official plugins #30800

Verify signatures on official plugins #30800

Conversation

jasontedor commented May 23, 2018 • edited Loading

elasticmachine commented May 23, 2018

jasontedor commented May 23, 2018

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvernum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaymode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaymode left a comment

Choose a reason for hiding this comment

jasontedor commented May 23, 2018 •

edited

Loading