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

[WIP] Add BouncyCastleFipsProvider as test dependency #41024

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -152,9 +152,8 @@ class BuildPlugin implements Plugin<Project> {
runtimeJavaVersionDetails = findJavaVersionDetails(project, runtimeJavaHome)
runtimeJavaVersionEnum = JavaVersion.toVersion(findJavaSpecificationVersion(project, runtimeJavaHome))
}

String inFipsJvmScript = 'print(java.security.Security.getProviders()[0].name.toLowerCase().contains("fips"));'
boolean inFipsJvm = Boolean.parseBoolean(runJavaAsScript(project, runtimeJavaHome, inFipsJvmScript))
// Java home name checking is fragile, but we control the environment
boolean inFipsJvm = runtimeJavaHome.contains("fips")
Copy link
Member Author

@jkakavas jkakavas Apr 9, 2019

Choose a reason for hiding this comment

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

Previously, the Provider was statically added as a system wide dependency ( in lib/ext of a jdk8 JVM ) so we could evaluate the getProviders() result here. Now the preference order is still set in java.security but the dependency is not yet available so the provider won't be loaded


// Build debugging info
println '======================================='
Expand Down Expand Up @@ -590,6 +589,16 @@ class BuildPlugin implements Plugin<Project> {
url "https://s3.amazonaws.com/download.elasticsearch.org/lucenesnapshots/${revision}"
}
}
String compilerJavaHome = findCompilerJavaHome()
String runtimeJavaHome = findRuntimeJavaHome(compilerJavaHome)
if (runtimeJavaHome.contains("fips")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we take this opportunity to clean things up and have something like -Dfips.enabled=true or -Dtests/fips=true rather than rely on the runtime java home name ?
My understanding is that we no longer need to make modifications to that location, so it will only really act as a flag.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, good idea ! What we would need to do is to bundle the java.policy and java.security (one for each jdk version) as test resources and adjust the system properties ( -Djava.security.policy and -Djava.security.properties IIRC) the same way we do for the keystore password. Would that be ok ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's better as it will have all the fips config contained in our build.
You'll probably want to look at the ExportElasticsearchBuildResourcesTask tasks , there's already one created for each project.

repos.ivy {
url "https://downloads.bouncycastle.org"
patternLayout {
artifact 'fips-java/[module]-[revision].[ext]'
}
}
}
}

/**
Expand Down Expand Up @@ -906,6 +915,10 @@ class BuildPlugin implements Plugin<Project> {
File heapdumpDir = new File(project.buildDir, 'heapdump')

project.tasks.withType(Test) { Test test ->
RepositoryHandler repos = project.repositories
jkakavas marked this conversation as resolved.
Show resolved Hide resolved
if (project.ext.inFipsJvm) {
project.dependencies.add('testRuntimeOnly', "org.bouncycastle:bc-fips:1.0.1:jar")
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we carry this classpath tot he clusters running in the rest tests, so these will probably not be running with fips enabled. We should have a smoke test to verify that fips is really enabled everywhere we think it is when we build.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe I missed this but it seems this won't for rest tests as the tests themselves will run on fips jvm but the cluster will not.

Good catch, thanks

How about the integTest clusters? Those are covered by the above, correct ?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes internal clusters run in the same jvm as tests so would are covered.

}
File testOutputDir = new File(test.reports.junitXml.getDestination(), "output")

doFirst {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,11 @@ class PrecommitTasks {
}
task.dependsOn(project.sourceSets.test.classesTaskName)
task.javaHome = project.runtimeJavaHome

// Java home name checking is fragile, but we control the environment
if (project.runtimeJavaHome.contains("fips")){
task.enabled = false
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's worth to have a comment here explaining why we can't have jar hell in fips.

}
return task
}

Expand Down
6 changes: 0 additions & 6 deletions distribution/tools/plugin-cli/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,3 @@ test {
// TODO: find a way to add permissions for the tests in this module
systemProperty 'tests.security.manager', 'false'
}

if (project.inFipsJvm) {
// FIPS JVM includes manny classes from bouncycastle which count as jar hell for the third party audit,
// rather than provide a long list of exclusions, disable the check on FIPS.
thirdPartyAudit.enabled = false
}
1 change: 1 addition & 0 deletions modules/reindex/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ dependencies {
// Issue tracked in https://github.com/elastic/elasticsearch/issues/40904
if (project.inFipsJvm) {
integTest.enabled = false
testingConventions.enabled = false
}

if (Os.isFamily(Os.FAMILY_WINDOWS)) {
Expand Down
11 changes: 2 additions & 9 deletions modules/transport-netty4/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ thirdPartyAudit {
'org.bouncycastle.cert.jcajce.JcaX509v3CertificateBuilder',
'org.bouncycastle.jce.provider.BouncyCastleProvider',
'org.bouncycastle.operator.jcajce.JcaContentSignerBuilder',
'org.bouncycastle.asn1.x500.X500Name',

// from io.netty.handler.ssl.JettyNpnSslEngine (netty)
'org.eclipse.jetty.npn.NextProtoNego$ClientProvider',
Expand Down Expand Up @@ -169,12 +170,4 @@ thirdPartyAudit {
'io.netty.util.internal.shaded.org.jctools.util.UnsafeRefArrayAccess',
'io.netty.handler.ssl.util.OpenJdkSelfSignedCertGenerator'
)
}

if (project.inFipsJvm == false) {
// BouncyCastleFIPS provides this class, so the exclusion is invalid when running CI in
// a FIPS JVM with BouncyCastleFIPS Provider
thirdPartyAudit.ignoreMissingClasses (
'org.bouncycastle.asn1.x500.X500Name'
)
}
}
9 changes: 8 additions & 1 deletion plugins/discovery-azure-classic/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ dependencies {
compile 'org.codehaus.jackson:jackson-core-asl:1.9.2'
compile 'org.codehaus.jackson:jackson-mapper-asl:1.9.2'
compile 'org.codehaus.jackson:jackson-jaxrs:1.9.2'
compile 'org.codehaus.jackson:jackson-xc:1.9.2'
compile 'org.codehaus.jackson:jackson-xc:1.9.2'

// HACK: javax.xml.bind was removed from default modules in java 9, so we pull the api in here,
// and whitelist this hack in JarHell
Expand All @@ -63,6 +63,7 @@ File keystore = new File(project.buildDir, 'keystore/test-node.jks')

// generate the keystore
task createKey(type: LoggedExec) {
onlyIf { inFipsJvm == false }
Copy link
Member Author

Choose a reason for hiding this comment

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

this task would fail as the JVM is setup for FIPS (BCFKS keystores, etc) but the dependency is not added when this is evaluated so the necessary classes are not available. We don't use the produced keystore eitherway as this can only work with a JKS keystore which is not available in FIPS - and this is why the integTest task is also disabled

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I follow. If integTest is disabled, what would try to run this task in FIPS mode?

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 guess it's because of processTestResources.dependsOn(createKey) below. I'd be happy to explore alternatives if you have suggestions!

doFirst {
project.delete(keystore.parentFile)
keystore.parentFile.mkdirs()
Expand Down Expand Up @@ -133,3 +134,9 @@ thirdPartyAudit.ignoreMissingClasses (
'com.sun.xml.fastinfoset.stax.StAXDocumentParser',
'com.sun.xml.fastinfoset.stax.StAXDocumentSerializer'
)

if (inFipsJvm) {
// We do not run integ tests in FIPS mode as these use a JKS keystore for the azure settings
integTest.enabled = false
testingConventions.enabled = false
}
8 changes: 1 addition & 7 deletions plugins/ingest-attachment/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -82,10 +82,4 @@ forbiddenPatterns {

thirdPartyAudit{
ignoreMissingClasses()
}

if (project.inFipsJvm) {
// FIPS JVM includes manny classes from bouncycastle which count as jar hell for the third party audit,
// rather than provide a long list of exclusions, disable the check on FIPS.
thirdPartyAudit.enabled = false
}
}
10 changes: 2 additions & 8 deletions plugins/transport-nio/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ thirdPartyAudit {
'org.bouncycastle.cert.jcajce.JcaX509v3CertificateBuilder',
'org.bouncycastle.jce.provider.BouncyCastleProvider',
'org.bouncycastle.operator.jcajce.JcaContentSignerBuilder',
'org.bouncycastle.asn1.x500.X500Name',

// from io.netty.handler.ssl.JettyNpnSslEngine (netty)
'org.eclipse.jetty.npn.NextProtoNego$ClientProvider',
Expand Down Expand Up @@ -147,11 +148,4 @@ thirdPartyAudit {

'io.netty.handler.ssl.util.OpenJdkSelfSignedCertGenerator'
)
}
if (project.inFipsJvm == false) {
// BouncyCastleFIPS provides this class, so the exclusion is invalid when running CI in
// a FIPS JVM with BouncyCastleFIPS Provider
thirdPartyAudit.ignoreMissingClasses (
'org.bouncycastle.asn1.x500.X500Name'
)
}
}
2 changes: 1 addition & 1 deletion x-pack/plugin/ml/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -113,4 +113,4 @@ gradle.projectsEvaluated {
// also add an "alias" task to make typing on the command line easier
task icTest {
dependsOn internalClusterTest
}
}
2 changes: 1 addition & 1 deletion x-pack/plugin/ml/qa/native-multi-node-tests/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -69,4 +69,4 @@ integTestCluster {
retries: 10)
return tmpFile.exists()
}
}
}
3 changes: 1 addition & 2 deletions x-pack/plugin/security/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -325,5 +325,4 @@ gradle.projectsEvaluated {
.subprojects
.findAll { it.path.startsWith(project.path + ":qa") }
.each { check.dependsOn it.check }
}

}
4 changes: 0 additions & 4 deletions x-pack/plugin/security/cli/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,4 @@ if (project.inFipsJvm) {
tasks.withType(CheckForbiddenApis) {
bundledSignatures -= "jdk-non-portable"
}
// FIPS JVM includes many classes from bouncycastle which count as jar hell for the third party audit,
// rather than provide a long list of exclusions, disable the check on FIPS.
thirdPartyAudit.enabled = false

}
3 changes: 1 addition & 2 deletions x-pack/plugin/security/qa/tls-basic/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -44,5 +44,4 @@ integTestCluster {
http.setCertificateAuthorities(caFile)
return http.wait(5000)
}
}

}
2 changes: 1 addition & 1 deletion x-pack/plugin/sql/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -146,4 +146,4 @@ task regen {
patternset(includes: 'SqlBase*.java')
}
}
}
}
13 changes: 11 additions & 2 deletions x-pack/plugin/sql/qa/security/with-ssl/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -208,8 +208,7 @@ integTestCluster {
}
}
Closure notRunningFips = {
Boolean.parseBoolean(BuildPlugin.runJavaAsScript(project, project.runtimeJavaHome,
'print(java.security.Security.getProviders()[0].name.toLowerCase().contains("fips"));')) == false
inFipsJvm == false
}

// Do not attempt to form a cluster in a FIPS JVM, as doing so with a JKS keystore will fail.
Expand All @@ -219,6 +218,16 @@ tasks.matching({ it.name == "integTestCluster#init" }).all { onlyIf notRunningFi
tasks.matching({ it.name == "integTestCluster#start" }).all { onlyIf notRunningFips }
tasks.matching({ it.name == "integTestCluster#wait" }).all { onlyIf notRunningFips }
tasks.matching({ it.name == "integTestRunner" }).all { onlyIf notRunningFips }
tasks.matching({ it.name == "createNodeKeyStore" }).all { onlyIf notRunningFips }
tasks.matching({ it.name == "createClientKeyStore" }).all { onlyIf notRunningFips }
tasks.matching({ it.name == "exportNodeCertificate" }).all { onlyIf notRunningFips }
tasks.matching({ it.name == "exportClientCertificate" }).all { onlyIf notRunningFips }
tasks.matching({ it.name == "importNodeCertificateInClientKeyStore" }).all { onlyIf notRunningFips}
tasks.matching({ it.name == "importClientCertificateInNodeKeyStore" }).all { onlyIf notRunningFips}

if (project.inFipsJvm) {
testingConventions.enabled = false
}

/** A lazy evaluator to find the san to use for certificate generation. */
class SanEvaluator {
Expand Down
3 changes: 3 additions & 0 deletions x-pack/plugin/sql/qa/security/without-ssl/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,6 @@ integTestCluster {
return tmpFile.exists()
}
}
if (project.inFipsJvm) {
testingConventions.enabled = false
}
2 changes: 1 addition & 1 deletion x-pack/qa/core-rest-tests-with-security/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -43,4 +43,4 @@ integTestCluster {
retries: 10)
return tmpFile.exists()
}
}
}
2 changes: 1 addition & 1 deletion x-pack/qa/evil-tests/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,4 @@ dependencies {
test {
systemProperty 'tests.security.manager', 'false'
include '**/*Tests.class'
}
}
2 changes: 1 addition & 1 deletion x-pack/qa/full-cluster-restart/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -232,4 +232,4 @@ task copyXPackPluginProps(type: Copy) {
from project(xpackModule('core')).tasks.pluginProperties
into outputDir
}
project.sourceSets.test.output.dir(outputDir, builtBy: copyXPackPluginProps)
project.sourceSets.test.output.dir(outputDir, builtBy: copyXPackPluginProps)
3 changes: 1 addition & 2 deletions x-pack/qa/kerberos-tests/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -73,5 +73,4 @@ task copyKeytabToGeneratedResources(type: Copy) {
into generatedResources
dependsOn project(':test:fixtures:krb5kdc-fixture').postProcessFixture
}
project.sourceSets.test.output.dir(generatedResources, builtBy:copyKeytabToGeneratedResources)

project.sourceSets.test.output.dir(generatedResources, builtBy:copyKeytabToGeneratedResources)
2 changes: 1 addition & 1 deletion x-pack/qa/openldap-tests/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,4 @@ task copyIdpTrust(type: Copy) {
from idpFixtureProject.file('openldap/certs/ca_server.pem');
into outputDir
}
project.sourceSets.test.output.dir(outputDir, builtBy: copyIdpTrust)
project.sourceSets.test.output.dir(outputDir, builtBy: copyIdpTrust)
2 changes: 1 addition & 1 deletion x-pack/qa/reindex-tests-with-security/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -58,4 +58,4 @@ integTestCluster {
http.setPassword("x-pack-test-password")
return http.wait(5000)
}
}
}
2 changes: 1 addition & 1 deletion x-pack/qa/saml-idp-tests/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -111,4 +111,4 @@ thirdPartyAudit {
ignoreMissingClasses (
'com.ibm.icu.lang.UCharacter'
)
}
}
2 changes: 1 addition & 1 deletion x-pack/qa/security-tools-tests/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,4 @@ forbiddenPatterns {
}

// these are just tests, no need to audit
thirdPartyAudit.enabled = false
thirdPartyAudit.enabled = false
3 changes: 1 addition & 2 deletions x-pack/qa/third-party/active-directory/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -30,5 +30,4 @@ test {
systemProperty 'es.set.netty.runtime.available.processors', 'false'
include '**/*IT.class'
include '**/*Tests.class'
}

}