Remove duplicate ssl setup in sql/qa projects #57319

breskeby · 2020-05-28T21:45:09Z

This is follow up on #57197 as we run ssl cert certification for each with-ssl subproject with identical parameters. This PR introduces an elasticsearch.test-with-ssl plugin that configures projects to use pregenerated certificates for testing that are shipped with buildSrc.

Removes duplicate code from with-ssl test projects
Removes expensive duplicate dynamic creation of ssl certificate generation during precommit run
Documents how the certificates can be recreated
Used certs are static and not generated dynamically anymore

Here's a comparison of running precommit locally before(https://gradle-enterprise.elastic.co/s/lhoqf5yix5g24/performance/build) and after(https://gradle-enterprise.elastic.co/s/djogrpm6ci7fs/performance/build)

elasticmachine · 2020-05-28T21:45:11Z

Pinging @elastic/es-core-infra (:Core/Infra/Build)

rjernst

One general comment is I wonder why we are still generating these files on the fly. In other projects (eg :x-pack:qa:smoke-test-plugins-ssl) we have moved away from generating the ssl support files and instead check them in. What if we were to move them to a shared location in buildSrc and have a plugin that copies them into projects and does the configuration. That would also mean the sql projects here could support fips testing, which I believe is why we moved to static files in the first place.

cc @jkakavas

rjernst · 2020-05-28T21:56:33Z

x-pack/plugin/sql/qa/server/security/with-ssl/build.gradle

+  }
+}
+
+tasks.withType(org.elasticsearch.gradle.testclusters.TestClustersAware){


Won't this aggressively realize all tasks? I think we need configureEach?

good catch. 👀 . Fixed

jkakavas · 2020-05-29T07:01:14Z

One general comment is I wonder why we are still generating these files on the fly. In other projects (eg :x-pack:qa:smoke-test-plugins-ssl) we have moved away from generating the ssl support files and instead check them in.

The reason is that the SQL client doesn't yet support to be configured with certificate/key material in PEM format and thus we can handle this in the same way we do in other qa projects. That's why we have this :

integTest.runner {
  dependsOn(importClientCertificateInNodeKeyStore)
  onlyIf {
    // Do not attempt to form a cluster in a FIPS JVM, as doing so with a JKS keystore will fail.
    // TODO Revisit this when SQL CLI client can handle key/certificate instead of only Keystores.
    // https://github.com/elastic/elasticsearch/issues/32306
    BuildParams.inFipsJvm == false
  }
}

What if we were to move them to a shared location in buildSrc and have a plugin that copies them into projects and does the configuration. That would also mean the sql projects here could support fips testing, which I believe is why we moved to static files in the first place.

This is a step in the right direction IMHO, but we can always defer that to when we will be able to actually use the key/cert material in the SQL projects

breskeby · 2020-05-29T07:04:56Z

One general comment is I wonder why we are still generating these files on the fly. In other projects (eg :x-pack:qa:smoke-test-plugins-ssl) we have moved away from generating the ssl support files and instead check them in. What if we were to move them to a shared location in buildSrc and have a plugin that copies them into projects and does the configuration. That would also mean the sql projects here could support fips testing, which I believe is why we moved to static files in the first place.

cc @jkakavas

From what I can see there are two dynamic variables in this generation:

san(SubjectAlternativeName)
dname(Distinguished Name).

At least dname seems to possible to be made a fixed string I think. for san I'm not sure how it is used and I honestly don't know if this can be removed or alternated to be static so we could generate them once and ship as you described. Would be great if we can. as generating those files is like ~3-4s on each execution (and part of precommit atm)

breskeby · 2020-05-29T08:19:53Z

@jkakavas so I wonder is this dynamic in the creation of those files as described here: #57319 (comment) required or could we remove that so we can have those files generate once, checked in and reused in all environments (local devs, ci, etc)?

jkakavas · 2020-05-29T08:40:06Z

No, we don't need to generate those dynamically. We can use catch-all SANs that would work for all

 X509v3 Subject Alternative Name: 
                DNS:localhost, DNS:localhost.localdomain, DNS:localhost4, DNS:localhost4.localdomain4, DNS:localhost6, DNS:localhost6.localdomain6, IP Address:127.0.0.1, IP Address:0:0:0:0:0:0:0:1

We have README files in most(wanted to say all but I noticed I didn't put one in :x-pack:qa:smoke-test-plugins-ssl ) of the places we use pre-generated keys and certificates describing how these were generated , i.e. : https://github.com/jkakavas/elasticsearch/blob/75a5a64427bed323f76f055050b42d5b05c3a5a7/x-pack/plugin/security/qa/tls-basic/src/test/resources/ssl/README.asciidoc

We also have an issue open to clean these up and do deduplication: #41646

breskeby · 2020-05-29T09:01:14Z

@jkakavas perfect. That should make things simpler and we can remove the overhead on every build run. I look into this.

breskeby · 2020-05-29T15:41:55Z

@elasticsearchmachine test this please

breskeby · 2020-06-02T08:16:02Z

@elasticsearchmachine test this please

jkakavas

I love this, it will help us untangle much of the setup in the security tests. I added some comments and questions on whether we can make this plugin slightly more generic so that it can handle other test use cases

jkakavas · 2020-06-03T12:46:03Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/test/TestWithSslPlugin.java

+        File keyStoreDir = new File(project.getBuildDir(), "keystore");
+        TaskProvider<ExportElasticsearchBuildResourcesTask> exportKeyStore = project.getTasks()
+            .register("copyTestCertificates", ExportElasticsearchBuildResourcesTask.class, (t) -> {
+                t.copy("test/ssl/test-client.cert");


nit: It doesn't really matter but can we rename the suffices to crt instead ?

jkakavas · 2020-06-03T12:54:40Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/test/TestWithSslPlugin.java

+                t.copy("test/ssl/test-client.cert");
+                t.copy("test/ssl/test-client.jks");
+                t.copy("test/ssl/test-node.cert");
+                t.copy("test/ssl/test-node.jks");


Can we also get the private keys into test/ssl/test-X.pem ? In general , given the applicability that this plugin has in so many projects in our security and core tests, I wonder if we can do a little more up front:

Setup a CA key and certificate

Generate test-node private key and certificate signed by that CA

Generate test-client private key and certificate signed by that CA

Make all the above available as PEM files ( x.crt and x.key ) and in JKS / PKCS12 keystores

Copy all of them as extra config files

@jkakavas I think we should be able to do that. Can you raise a separate issue for this? I'd like to keep this PR small and focussed.

raised #57606

jkakavas · 2020-06-03T12:55:19Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/test/TestWithSslPlugin.java

+                .getByName(TestClustersPlugin.EXTENSION_NAME);
+            clusters.all(c -> {
+                // ceremony to set up ssl
+                c.setting("xpack.security.transport.ssl.keystore.path", "test-node.jks");


Can I override/overwrite this in the build.gradle file of a subproject where I use this new plugin ?

yes, you can override them in the build script after applying the plugin

jkakavas · 2020-06-03T12:57:27Z

buildSrc/src/main/resources/test/ssl/README.md

+The certificates are generated using catch-all SAN in the following procedure:
+
+1. Generate the node's keystore:
+   `keytool -genkey -alias test-node -keystore test-node.jks -keyalg RSA -keysize 2048 -validity 712 -dname CN="Elasticsearch Build Test Infrastructure" -keypass keypass -storepass keypass -ext san=dns:localhost,dns:localhost.localdomain,dns:localhost4,dns:localhost4.localdomain4,dns:localhost6,dns:localhost6.localdomain6,ip:127.0.0.1,ip:0:0:0:0:0:0:0:1`


I think we can go with something closer to 10 years here instead of 2. We don't gain anything by shorter lifetime. Maybe also add a note in the README with the date these certificates expire so that is more probable someone catches this and updates them before they expire and tests start failing?

jkakavas · 2020-06-03T13:02:50Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/test/TestWithSslPlugin.java

+            NamedDomainObjectContainer<ElasticsearchCluster> clusters = (NamedDomainObjectContainer<ElasticsearchCluster>) project
+                .getExtensions()
+                .getByName(TestClustersPlugin.EXTENSION_NAME);
+            clusters.all(c -> {


Could we also have a conditional here that if we are inFipsJvm() it would use key and certificate PEM files to set up TLS instead of keystores so that it can work even when the subproject that uses this plugin is running tests in fips mode ? i.e.

*.ssl.key *.ssl.secure_key_passphrase *.ssl.certificate

instead of

*.ssl.keystore.path *.ssl.keystore.secure_password

and

*.ssl.certificate_authorities

instead of

*.ssl.keystore.path

or

*.ssl.truststore.path

@jkakavas same as above. Would like to tackle this in a separate story

I'm fine with that as long as we don't apply this plugin to any other subproject until we have the fips handling because it will start failing in CI. ( sql is fine because we mute the tests in fips eitherway )

- make them valid for 10 years - Add note to readme about date of expiration

buildSrc/build.gradle

jkakavas · 2020-06-03T14:42:39Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/test/TestWithSslPlugin.java

+            NamedDomainObjectContainer<ElasticsearchCluster> clusters = (NamedDomainObjectContainer<ElasticsearchCluster>) project
+                .getExtensions()
+                .getByName(TestClustersPlugin.EXTENSION_NAME);
+            clusters.all(c -> {


I'm fine with that as long as we don't apply this plugin to any other subproject until we have the fips handling because it will start failing in CI. ( sql is fine because we mute the tests in fips eitherway )

breskeby · 2020-06-03T15:13:59Z

@elasticmachine test this please

mark-vieira

Man this is about 1000% better. 👍

mark-vieira · 2020-06-03T16:42:39Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/test/TestWithSslPlugin.java

+                t.setOutputDir(keyStoreDir);
+            });
+
+        project.getPlugins().withType(StandaloneRestTestPlugin.class).configureEach(restTestPlugin -> {


How does this compare to project.getPluginManager().withPlugin()?

getPluginManager() doesn't allow configuration by plugin type (class) which I usually prefer when coding. also depending on your environment it used to be better when doing safer when refactorings etc instead of relying on plugin ids. PluginManager provides access to AppliedPlugin which we do not need here.

mark-vieira · 2020-06-03T16:43:27Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/test/TestWithSslPlugin.java

+
+        project.getPlugins().withType(StandaloneRestTestPlugin.class).configureEach(restTestPlugin -> {
+            SourceSetContainer sourceSets = project.getExtensions().getByType(SourceSetContainer.class);
+            SourceSet testSourceSet = sourceSets.getByName("test");


We can use Util.getJavaTestSourceSet().

mark-vieira · 2020-06-03T16:44:23Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/test/TestWithSslPlugin.java

+            testSourceSet.compiledBy(exportKeyStore);
+
+            project.getTasks()
+                .withType(org.elasticsearch.gradle.testclusters.TestClustersAware.class)


Do we have to use the fully qualified name here? Can we add an import?

mark-vieira · 2020-06-03T16:45:47Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/test/TestWithSslPlugin.java

+
+            // Tell the tests we're running with ssl enabled
+            project.getTasks()
+                .withType(RestIntegTestTask.class)


We should instead configure all tasks of RestTestRunnerTask type. There can be instances where we create these directly w/o creating a RestIntegTestTask. Also, the latter will hopefully dissapear soon.

Also, would we not want this for unit tests as well? Or is this very specific to integration tests?

changed to use RestTestRunnerTask. So far I don't think we have unit tests for this kind of testing. I don't see how this would look like tbh.

- Minor cleanup

* Remove duplicate ssl setup in sql/qa projects * Fix enforcement of task instances * Use static data for cert generation * Move ssl testing logic into a plugin * Document test cert creation

breskeby self-assigned this May 28, 2020

breskeby added :Delivery/Build Build or test infrastructure Team:Core/Infra Meta label for core/infra team v7.7.2 v7.8.1 v7.9.0 v8.0.0 labels May 28, 2020

breskeby marked this pull request as draft May 28, 2020 21:45

rjernst reviewed May 28, 2020

View reviewed changes

breskeby force-pushed the remove-duplicate-ssl-setup-in-qa branch from 4a77c93 to 86c96e6 Compare June 2, 2020 08:26

breskeby added 8 commits June 2, 2020 11:10

Remove duplicate ssl setup in sql/qa projects

97901d3

Fix enforcement of task instances

2f0aa85

Fix imports

ce6869f

Fix another import

90d2458

Fix configuration refernce

c66d11d

Use static data for cert generation

d2cfb33

Some fixing in the ssl setup task plumbing

b6205f1

Put certs on test runtime classpath

63aea79

breskeby force-pushed the remove-duplicate-ssl-setup-in-qa branch from 86c96e6 to 63aea79 Compare June 2, 2020 09:23

breskeby added 3 commits June 2, 2020 15:29

Resolve certs for testing directly

1126d89

Move ssl testing logic into a plugin

7d1779d

Fix imports

d525f1d

breskeby marked this pull request as ready for review June 3, 2020 10:25

breskeby requested a review from jkakavas June 3, 2020 10:26

jkakavas reviewed Jun 3, 2020

View reviewed changes

breskeby added 2 commits June 3, 2020 16:17

Apply feedback: Rename *.cert to *.crt files

f5adb44

Apply feedback regarding generated test certs

4a7782b

- make them valid for 10 years - Add note to readme about date of expiration

jkakavas approved these changes Jun 3, 2020

View reviewed changes

jkakavas mentioned this pull request Jun 3, 2020

Make elasticserach.test-with-ssl plugin more generic #57606

Open

breskeby added 2 commits June 3, 2020 16:44

Fix forbiddenPattern task after cert -> crt renaming

42de63b

Another fix after cert -> crt renaming

3e7240b

mark-vieira approved these changes Jun 3, 2020

View reviewed changes

breskeby added 2 commits June 3, 2020 19:11

Fix generated certificates

11bbb65

Apply review feedback

aaa29ad

- Minor cleanup

breskeby merged commit 78cb9fa into elastic:master Jun 4, 2020

breskeby mentioned this pull request Jun 4, 2020

Remove duplicate ssl setup in sql/qa projects (7.x backport) #57643

Merged

breskeby added the backport pending label Jun 4, 2020

breskeby mentioned this pull request Jun 4, 2020

Remove duplicate ssl setup in sql/qa projects (7.8 backport) #57646

Merged

breskeby mentioned this pull request Jun 4, 2020

Remove duplicate ssl setup in sql/qa projects (7.7 backport) #57647

Merged

breskeby deleted the remove-duplicate-ssl-setup-in-qa branch June 4, 2020 08:36

breskeby removed the backport pending label Jun 9, 2020

mark-vieira added Team:Delivery Meta label for Delivery team and removed Team:Core/Infra Meta label for core/infra team labels Nov 11, 2020

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Remove duplicate ssl setup in sql/qa projects #57319

Remove duplicate ssl setup in sql/qa projects #57319

Conversation

breskeby commented May 28, 2020 • edited Loading

elasticmachine commented May 28, 2020

rjernst left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkakavas commented May 29, 2020

breskeby commented May 29, 2020

breskeby commented May 29, 2020

jkakavas commented May 29, 2020

breskeby commented May 29, 2020

breskeby commented May 29, 2020

breskeby commented Jun 2, 2020

jkakavas 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

Choose a reason for hiding this comment

breskeby commented Jun 3, 2020

mark-vieira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

breskeby Jun 3, 2020 • edited Loading

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

breskeby commented May 28, 2020 •

edited

Loading

rjernst left a comment •

edited

Loading

breskeby Jun 3, 2020 •

edited

Loading