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.

Sign up for GitHub

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

Jump to bottom

Remove BouncyCastle dependency from runtime #32193

Merged
merged 8 commits into from
Jul 20, 2018
Merged

Remove BouncyCastle dependency from runtime #32193

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e5824d7
Remove BouncyCastle dependency from runtime
jkakavas Jul 19, 2018
15f8c15
Address feedback
jkakavas Jul 19, 2018
27286ac
Update class package in scripts
jkakavas Jul 19, 2018
aca04e2
address feedback
jkakavas Jul 19, 2018
c40137f
Add packaging test for security cli
jkakavas Jul 20, 2018
af5745a
Reintroduce compileOnly dependency for security
jkakavas Jul 20, 2018
defa749
ensure jimfs is available in test runtime
jkakavas Jul 20, 2018
c2e55c2
Fix security-cli tests
jkakavas Jul 20, 2018
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
4 changes: 4 additions & 0 deletions distribution/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,10 @@ configure(subprojects.findAll { ['archives', 'packages'].contains(it.name) }) {
from { project(':distribution:tools:plugin-cli').jar }
from { project(':distribution:tools:plugin-cli').configurations.runtime }
}
into('tools/security-cli') {
Copy link
Member

Choose a reason for hiding this comment

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

I am pretty sure this is not where we want to add this since the comment makes me think this section also applies to the oss distribution which does not have xpack.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, that makes sense. I can take advantage of the oss boolean and call with libFiles(oss) where applicable so that it only includes the jars in tools/security-cli is oss is false

Copy link
Member

Choose a reason for hiding this comment

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

Also, I think this can probably just go under tools now that I see where we are putting the jar. My suggestion for this directory yesterday was due to my outdated view of how files are laid out within the distribution.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm...I think that tools/security-cli is the right place? It mimics what we did with the plugin-cli.

Copy link
Member

Choose a reason for hiding this comment

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

derp. Ignore me and leave it as is

from { project(':x-pack:plugin:security:cli').jar }
from { project(':x-pack:plugin:security:cli').configurations.compile }
}
}

modulesFiles = { oss ->
Expand Down
6 changes: 3 additions & 3 deletions x-pack/plugin/core/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ esplugin {
}

dependencyLicenses {
mapping from: /bc.*/, to: 'bouncycastle'
mapping from: /http.*/, to: 'httpclient' // pulled in by rest client
mapping from: /commons-.*/, to: 'commons' // pulled in by rest client
}
Expand All @@ -36,8 +35,8 @@ dependencies {

// security deps
compile 'com.unboundid:unboundid-ldapsdk:3.2.0'
compile 'org.bouncycastle:bcprov-jdk15on:1.59'
compile 'org.bouncycastle:bcpkix-jdk15on:1.59'
compileOnly 'org.bouncycastle:bcprov-jdk15on:1.59'
compileOnly 'org.bouncycastle:bcpkix-jdk15on:1.59'
compile project(path: ':modules:transport-netty4', configuration: 'runtime')

testCompile 'org.elasticsearch:securemock:1.2'
Expand Down Expand Up @@ -114,6 +113,7 @@ task testJar(type: Jar) {
appendix 'test'
from sourceSets.test.output
}

artifacts {
// normal es plugins do not publish the jar but we need to since users need it for Transport Clients and extensions
archives jar
Expand Down
5 changes: 2 additions & 3 deletions x-pack/plugin/security/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ dependencies {
testCompile project(path: xpackModule('core'), configuration: 'testArtifacts')

compile 'com.unboundid:unboundid-ldapsdk:3.2.0'
compile 'org.bouncycastle:bcprov-jdk15on:1.59'
compile 'org.bouncycastle:bcpkix-jdk15on:1.59'
compileOnly 'org.bouncycastle:bcprov-jdk15on:1.59'
compileOnly 'org.bouncycastle:bcpkix-jdk15on:1.59'

// the following are all SAML dependencies - might as well download the whole internet
compile "org.opensaml:opensaml-core:3.3.0"
Expand Down Expand Up @@ -79,7 +79,6 @@ sourceSets.test.resources {
srcDir '../core/src/test/resources'
}
dependencyLicenses {
mapping from: /bc.*/, to: 'bouncycastle'
mapping from: /java-support|opensaml-.*/, to: 'shibboleth'
mapping from: /http.*/, to: 'httpclient'
}
Expand Down
22 changes: 22 additions & 0 deletions x-pack/plugin/security/cli/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
apply plugin: 'elasticsearch.build'

archivesBaseName = 'elasticsearch-security-cli'
dependencies {
compileOnly "org.elasticsearch:elasticsearch:${version}"
compile 'org.bouncycastle:bcprov-jdk15on:1.59'
compile 'org.bouncycastle:bcpkix-jdk15on:1.59'
}

jar {
from(project(':x-pack:plugin:core').sourceSets.main.output.classesDirs) {
Copy link
Member

Choose a reason for hiding this comment

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

can we not move the source files to this project? At the very least I think we should exclude these from the x-pack core jar

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you elaborate please? We're not moving source files to this project now. Am i reading your comment wrong or maybe the not in can we not move isn't meant to be there ?

I - probably falsely - assumed we do not want to move the source files to this project since we preferred this approach to the new plugin approach I tried out yesterday. It definitely makes sense to not have the classes both in this jar and in x-pack-core though - thanks.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, my comment was not clear. I'd like for us to move the source files to this project for a clearer separation.

The distinction between a separate plugin vs a jar is that the certificate generation code doesn't require a plugin and just needs to be packaged in a way that it can be loaded as a CLI tool. I suggested this way as an alternative to a completely separate plugin to avoid any confusion about seeing a security-cli plugin/module deployed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great thanks, all clear now. I'll move the source files in this project

include 'org/elasticsearch/xpack/core/ssl/CertificateTool.class'
include 'org/elasticsearch/xpack/core/ssl/CertificateGenerateTool.class'
include 'org/elasticsearch/xpack/core/ssl/CertGenUtils.class'
}
}

dependencyLicenses {
mapping from: /bc.*/, to: 'bouncycastle'
}

licenseHeaders.enabled = false
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain what is happening here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can certainly try. I started off without the licenseHeaders.enabled = false and precommit checks failed with:

> Task :x-pack:plugin:security:cli:licenseHeaders FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':x-pack:plugin:security:cli:licenseHeaders'.
> You must specify at least one file to create the report for.

I interpreted this to be an error caused by the licenseHeaders task looking for Java files in order to check the license heades in security-cli and failing because there aren't any. Depending on whether I'll bring in the source files (#32193 (comment)) , I'll remove this but what should have been the correct approach in this case?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I was unclear. I meant can you leave a comment in the build.gradle about this. Of course, this changes if we do bring in the source files.

0 ...ore/licenses/bcpkix-jdk15on-1.59.jar.sha1 → ...cli/licenses/bcpkix-jdk15on-1.59.jar.sha1
View file
Open in desktop
File renamed without changes.
0 ...ore/licenses/bcprov-jdk15on-1.59.jar.sha1 → ...cli/licenses/bcprov-jdk15on-1.59.jar.sha1
View file
Open in desktop
File renamed without changes.
0 ...in/core/licenses/bouncycastle-LICENSE.txt → ...ity/cli/licenses/bouncycastle-LICENSE.txt
View file
Open in desktop
File renamed without changes.
0 ...gin/core/licenses/bouncycastle-NOTICE.txt → ...rity/cli/licenses/bouncycastle-NOTICE.txt
View file
Open in desktop
File renamed without changes.
1 change: 1 addition & 0 deletions x-pack/plugin/security/src/main/bin/elasticsearch-certgen
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,6 @@

ES_MAIN_CLASS=org.elasticsearch.xpack.core.ssl.CertificateGenerateTool \
ES_ADDITIONAL_SOURCES="x-pack-env;x-pack-security-env" \
ES_ADDITIONAL_CLASSPATH_DIRECTORIES=lib/tools/security-cli \
"`dirname "$0"`"/elasticsearch-cli \
"$@"
1 change: 1 addition & 0 deletions x-pack/plugin/security/src/main/bin/elasticsearch-certgen.bat
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ setlocal enableextensions

set ES_MAIN_CLASS=org.elasticsearch.xpack.core.ssl.CertificateGenerateTool
set ES_ADDITIONAL_SOURCES=x-pack-env;x-pack-security-env
set ES_ADDITIONAL_CLASSPATH_DIRECTORIES=lib/tools/security-cli
call "%~dp0elasticsearch-cli.bat" ^
%%* ^
|| exit /b 1
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugin/security/src/main/bin/elasticsearch-certutil
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,6 @@

ES_MAIN_CLASS=org.elasticsearch.xpack.core.ssl.CertificateTool \
ES_ADDITIONAL_SOURCES="x-pack-env;x-pack-security-env" \
ES_ADDITIONAL_CLASSPATH_DIRECTORIES=lib/tools/security-cli \
"`dirname "$0"`"/elasticsearch-cli \
"$@"
1 change: 1 addition & 0 deletions x-pack/plugin/security/src/main/bin/elasticsearch-certutil.bat
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ setlocal enableextensions

set ES_MAIN_CLASS=org.elasticsearch.xpack.core.ssl.CertificateTool
set ES_ADDITIONAL_SOURCES=x-pack-env;x-pack-security-env
set ES_ADDITIONAL_CLASSPATH_DIRECTORIES=lib/tools/security-cli
call "%~dp0elasticsearch-cli.bat" ^
%%* ^
|| exit /b 1
Expand Down