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

Remove BouncyCastle dependency from runtime #32193

Merged
merged 8 commits into from
Jul 20, 2018

Conversation

jkakavas
Copy link
Member

This commit introduces a new project that builds a Jar of the
classes that have a dependency on BouncyCastle and puts it in a
subdirectory of lib (/tools/security-cli) along with the
BouncyCastle jars. This directory is then passed in the
ES_ADDITIONAL_CLASSPATH_DIRECTORIES of the CLI tools that use
these classes.

BouncyCastle is removed as a runtime dependency (remains
as a compileOnly one) from x-pack core and x-pack security.

This commit introduces a new project that builds a Jar of the
classes that have a dependency on BouncyCastle and puts it in a
subdirectory of lib (/tools/security-cli) along with the
BouncyCastle jars. This directory is then passed in the
ES_ADDITIONAL_CLASSPATH_DIRECTORIES of the CLI tools that use
these classes.

BouncyCastle is removed as a runtime dependency (remains
as a compileOnly one) from x-pack core and x-pack security.
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@@ -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

}

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

Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

I agree with @jaymode's comments and left a question.

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.

- Move source for CertificateTool, CertificateGenerateTool and
CertGenUtils and associated tests to the new project
- Ensure that the genrated jar and bc dependencies are not bundled
in oss
@jkakavas
Copy link
Member Author

@jaymode @jasontedor this is ready for another round

Copy link
Member

@jaymode jaymode left a comment

Choose a reason for hiding this comment

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

LGTM. I like this better and am so happy to see BC removed from the security runtime dependencies

testCompile "junit:junit:${versions.junit}"
testCompile "org.hamcrest:hamcrest-all:${versions.hamcrest}"
testCompile 'org.elasticsearch:securemock:1.2'
testCompile("org.elasticsearch.test:framework:${version}")
Copy link
Member

Choose a reason for hiding this comment

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

Nit: can you change this to the same style used above for the elasticsearch compile dependency

Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

Looks great! I left one comment so this is approved contingent on adding a test.

from { project(':distribution:tools:plugin-cli').jar }
from { project(':distribution:tools:plugin-cli').configurations.runtime }
}
if (oss == false) {
Copy link
Member

Choose a reason for hiding this comment

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

My only ask is a packaging test for this, that these are not in the OSS distribution, and are in the default distribution. In the new style. I am happy to guide, let me know.

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'll take a swing at it and will ping for help if I find myself stumbling in the dark

@jkakavas
Copy link
Member Author

@jasontedor I added the a package test, please take a look and see if this resembles what you had in mind.

Also, can you please comment on the use of testImplementation for the dependencies closure of security-cli. It looks like the correct one for this use case (and I do get related errors running the tests with only testCompile) but we are not using it anywhere else in our build scripts so I was wondering if we're solving this in some other manner.

BouncyCastle returns as a compileOnly dependency for security as Opensaml has a compile dependency on cryptacular which hash a compile dependency on bouncycastle.

@jasontedor
Copy link
Member

Thanks @jkakavas, everything looks great, including the use of testImplementation. Thanks a lot for adding the test.

@jkakavas jkakavas merged commit aaa8f84 into elastic:master Jul 20, 2018
martijnvg added a commit that referenced this pull request Jul 21, 2018
* es/master: (23 commits)
  Switch full-cluster-restart to new style Requests (#32140)
  [DOCS] Clarified that you must remove X-Pack plugin when upgrading from pre-6.3. (#32016)
  Remove BouncyCastle dependency from runtime (#32193)
  INGEST: Extend KV Processor (#31789) (#32232)
  INGEST: Make a few Processors callable by Painless (#32170)
  Add region ISO code to GeoIP Ingest plugin (#31669)
  [Tests] Remove QueryStringQueryBuilderTests#toQuery class assertions (#32236)
  Make sure that field aliases count towards the total fields limit. (#32222)
  Switch rolling restart to new style Requests (#32147)
  muting failing test for internal auto date histogram to avoid failure before fix is merged
  MINOR: Remove unused `IndexDynamicSettings` (#32237)
  Fix multi level nested sort (#32204)
  Enhance Parent circuit breaker error message (#32056)
  [ML] Use default request durability for .ml-state index (#32233)
  Remove indices stats timeout from monitoring docs
  Rename ranking evaluation response section (#32166)
  Dependencies: Upgrade to joda time 2.10 (#32160)
  Remove aliases resolution limitations when security is enabled (#31952)
  Ensure that field aliases cannot be used in multi-fields. (#32219)
  TESTS: Check for Netty resource leaks (#31861)
  ...
jkakavas added a commit to jkakavas/elasticsearch that referenced this pull request Jul 26, 2018
* Remove BouncyCastle dependency from runtime

This commit introduces a new gradle  project that contains
 the classes that have a dependency on BouncyCastle. For
the default distribution, It builds  a jar from those and
 in puts it in a subdirectory of lib
 (/tools/security-cli) along with the BouncyCastle jars.
This directory is then passed in the
ES_ADDITIONAL_CLASSPATH_DIRECTORIES of the CLI tools
that use these classes.

BouncyCastle is removed as a runtime dependency (remains
as a compileOnly one) from x-pack core and x-pack security.
jkakavas added a commit to jkakavas/elasticsearch that referenced this pull request Jul 26, 2018
* Remove BouncyCastle dependency from runtime

This commit introduces a new gradle  project that contains
 the classes that have a dependency on BouncyCastle. For
the default distribution, It builds  a jar from those and
 in puts it in a subdirectory of lib
 (/tools/security-cli) along with the BouncyCastle jars.
This directory is then passed in the
ES_ADDITIONAL_CLASSPATH_DIRECTORIES of the CLI tools
that use these classes.

BouncyCastle is removed as a runtime dependency (remains
as a compileOnly one) from x-pack core and x-pack security.
jkakavas added a commit that referenced this pull request Jul 26, 2018
* Remove BouncyCastle dependency from runtime

This commit introduces a new gradle  project that contains
 the classes that have a dependency on BouncyCastle. For
the default distribution, It builds  a jar from those and
 in puts it in a subdirectory of lib
 (/tools/security-cli) along with the BouncyCastle jars.
This directory is then passed in the
ES_ADDITIONAL_CLASSPATH_DIRECTORIES of the CLI tools
that use these classes.

BouncyCastle is removed as a runtime dependency (remains
as a compileOnly one) from x-pack core and x-pack security.

This is a backport of #32193
jkakavas added a commit that referenced this pull request Jul 26, 2018
* Remove BouncyCastle dependency from runtime

This commit introduces a new gradle  project that contains
 the classes that have a dependency on BouncyCastle. For
the default distribution, It builds  a jar from those and
 in puts it in a subdirectory of lib
 (/tools/security-cli) along with the BouncyCastle jars.
This directory is then passed in the
ES_ADDITIONAL_CLASSPATH_DIRECTORIES of the CLI tools
that use these classes.

BouncyCastle is removed as a runtime dependency (remains
as a compileOnly one) from x-pack core and x-pack security.

This is a backport of #32193
@jkakavas jkakavas deleted the cli-project branch September 14, 2018 06:55
@mark-vieira mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Delivery/Build Build or test infrastructure >enhancement Team:Delivery Meta label for Delivery team v6.4.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants