Upgrade to OpenSAML 4.3.0 (shadowed) #98199

albertzaharovits · 2023-08-04T10:30:29Z

This commit upgrades to OpenSAML v4.3.0

Versions of OpenSAML ≥ 4.1 have a hard dependency on the non-FIPS release of BouncyCastle.
This would prevent ES from being able to run in a JVM where BC-FIPS is configured as the security provider.

Closes: #71983

Co-authored-by: Tim Vernum tim@adjective.org

This commit upgrades to OpenSAML v4.3.0 Versions of OpenSAML ≥ 4.1 have a hard dependency on the non-FIPS release of BouncyCastle. This would prevent ES from being able to run in a JVM where BC-FIPS is configured as the security provider. In order to overcome this, we shadow OpenSAML and patch two classes to remove their dependency on BouncyCastle

albertzaharovits · 2023-08-04T13:42:06Z

@elasticsearchmachine run elasticsearch-ci/part-2

albertzaharovits · 2023-08-04T16:58:13Z

@tvernum This is your work passing CI 🎊

I'm going to try to reduce the surface of the changes by:

use a non-empty org.elasticsearch.xpack.security.authc.saml.OpenSamlConfig implementation that sets properties.setProperty("opensaml.config.ecdh.defaultKDF","PBKDF2") to avoid loading the KDF code that refers BC
remove the service implementation META-INF/services/org.opensaml.security.crypto.ec.NamedCurve form the opensaml-security-api-4.3.0.jar to avoid changing the AbstractNamedCurve class.

If all goes well there should be only one shadowed jar, which just removes the META-INF/services part from the original jar.
I'll ping you when I sort it out. (if it doesn't work out, I think this here is OK too, so I'll revert back to it).

albertzaharovits · 2023-08-10T17:00:42Z

These changes effectively bypass the BC dependency. Tests do pass, but I'm not 100% that somehow, via some obscure protocal/parameter combination, our SAML SP and IDP implementations won't try to use functionality that has hence been eliminated.
To learn what functionality has been eliminated, apply the following 2 patches and run any SAML test:

diff --git a/x-pack/libs/es-opensaml-security-api/build.gradle b/x-pack/libs/es-opensaml-security-api/build.gradle
index 56fd6b81df6..104bb51f1de 100644
--- a/x-pack/libs/es-opensaml-security-api/build.gradle
+++ b/x-pack/libs/es-opensaml-security-api/build.gradle
@@ -27,5 +27,5 @@ tasks.named("shadowJar").configure {
   manifest {
     attributes 'Automatic-Module-Name': 'org.opensaml.security'
   }
-  exclude 'META-INF/services/org.opensaml.security.crypto.ec.NamedCurve'
+  //exclude 'META-INF/services/org.opensaml.security.crypto.ec.NamedCurve'
 }

diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/OpenSamlXpackSecurityConfigurationPropertiesSource.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/OpenSamlXpackSecurityConfigurationPropertiesSource.java
index 0e885dd0daa..45243df6baf 100644
--- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/OpenSamlXpackSecurityConfigurationPropertiesSource.java
+++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/OpenSamlXpackSecurityConfigurationPropertiesSource.java
@@ -16,7 +16,7 @@ public class OpenSamlXpackSecurityConfigurationPropertiesSource implements Confi
     @Override
     public Properties getProperties() {
         Properties properties = new Properties();
-        properties.setProperty("opensaml.config.ecdh.defaultKDF", "PBKDF2");
+        //properties.setProperty("opensaml.config.ecdh.defaultKDF", "PBKDF2");
         return properties;
     }
 }

 ./gradlew ':x-pack:plugin:security:test' --tests "org.elasticsearch.xpack.security.authc.saml.SamlSpMetadataBuilderTests"

albertzaharovits · 2023-08-10T17:06:42Z

@mark-vieira I think it is possible to put the OpenSamlXpackSecurityConfigurationPropertiesSource service provider inside the shadowed jar, rather than put it in the ES code that uses the code that depends on the service. That's because the same code that uses the shadowed jar also uses the library code that depends on the service implementation.
But I'm not sure if this is better/preferable?
Also I couldn't figure out what the module declaration inside the shadowed jar should look like in order to be able to expose the ConfigurationPropertiesSource implementation (that's going to be used by the code that depends on the shadowed jar).

elasticsearchmachine · 2023-08-10T17:07:22Z

Pinging @elastic/es-security (Team:Security)

mark-vieira

Yeah, I don't think we want to duplicate the property source. Sticking it in a library is probably the best way to go. It's still not clear to me that we need to repackage the dependency though.

mark-vieira · 2023-08-10T18:16:42Z

x-pack/libs/es-opensaml-security-api/build.gradle

+  manifest {
+    attributes 'Automatic-Module-Name': 'org.opensaml.security'
+  }
+  exclude 'META-INF/services/org.opensaml.security.crypto.ec.NamedCurve'


Is this the only modification to this JAR we're making? Typically a "shadow" JAR is repackaging several dependencies, or relocating them under a different package, etc. Is this effectively identical to the original JAR, just excluding this file?

Pretty much.

OpenSAML out of the box doesn't work on FIPS. My first attempt to solve it required patching 2 classes. Albert worked out that we can just avoid ever loading those classes by manipulating the set of services - one of them can be handled by stripping this service, the other by implementing our own properties resolver so that a default property can be changed.

It does seem like overkill to repackage just to drop one services file, but I'm not aware of a simpler option.
We could just strip the file as part of the distribution, but I'm not a fan of shipping modified files JAR files that claim to be the originals.

Gotcha, makes sense. I would suggest we go with the pattern we used for log4j though (look in libs/log4j/build.gradle rather than use the Shadow plugin. It's not really the intended use case, and it's more complex than necessary for this, plus the log4j hack plays more nicely with IDEs.

++
The original attempt modified a couple of classes, so it was based on the approach we used for Jackson.
Now that we've settled on just stripping ~~classes~~ files, we can simplify it.

@mark-vieira I think it is possible to put the OpenSamlXpackSecurityConfigurationPropertiesSource service provider inside the shadowed jar, rather than put it in the ES code that uses the code that depends on the service. That's because the same code that uses the shadowed jar also uses the library code that depends on the service implementation.
But I'm not sure if this is better/preferable?

I think it's preferable to co-locate it with the shadowed jar. If for no other reason then it removes the existing duplication.

I've pushed 404298c and c179601 to incoporate the ConfigurationPropertiesSource service implementation in the shadowed jar, in order to avoid having two identical such implementations in the x-pack/plugin/security and x-pack/plugin/identity-provider projects (it works because the same code that must not see the NamedCurve implementations, also needs to see the new ConfigurationPropertiesSource implementation). So now the shadowed jar effectively removes one service implementation and adds another different one (a single new class).
I think this is now a fair usage of the "shadow" plugin?

When we say "pull in" what does that mean? Is it a transitive dependency or is it shadowed in the OpenSAML jar?

It is a runtime transitive dependency of OpenSAML (since version >4.1).

We manually manage transitive dependencies so we can use whatever bouncy castle jar we want, or omit it entirely.

We wish to omit it entirely because it contains crypto algorithms that are not FIPS compliant (there is a different bouncy castle with FIPS compliant algorithms), and that's a problem because we don't want the Elasticsearch FIPS distribution to ship with non-FIPS compliant crypto on the classpath (or really, no Elasticsearch distrib should have any crypto impl on the classpath. Instead we opt to delegate to the JCA API, where the JVM is configured for FIPS or not.).

Ok, then I don't quite understand why we need this solution. We disable transitive dependencies by default in our build. The transitive BC jar shouldn't be brought in by this dependency. How does this relate to omitting that service file? From what I can tell that's part of the OpenSAML jar, not BC.

If we don't put the BC on the classpath and we don't shadow jar the OpenSAML lib, then ES throws a class not found exception when it starts up, see this issue: #71983 .

Got it. So there is a runtime dependency on BC and stripping out that service avoids that issue. Understood.

Yes, OpenSAML has had a transitive dependency on BouncyCastle for a while, but we've just avoided those parts.
In 4.1 the dependency become harder to avoid - a couple of classes that are eagerly loaded (with the the default services/properties config) have a direct dependency.

tvernum

LGTM

tvernum · 2023-08-15T10:10:07Z

x-pack/libs/es-opensaml-security-api/build.gradle

+  manifest {
+    attributes 'Automatic-Module-Name': 'org.opensaml.security'
+  }
+  exclude 'META-INF/services/org.opensaml.security.crypto.ec.NamedCurve'


Yes, OpenSAML has had a transitive dependency on BouncyCastle for a while, but we've just avoided those parts.
In 4.1 the dependency become harder to avoid - a couple of classes that are eagerly loaded (with the the default services/properties config) have a direct dependency.

tvernum · 2023-08-21T07:40:40Z

@mark-vieira Do you want a chance to review this again before it's merged?

mark-vieira

Minor comment about re-enabling some of our precommit checks but otherwise LGTM.

mark-vieira · 2023-08-22T15:28:17Z

x-pack/libs/es-opensaml-security-api/build.gradle

+  compileOnly "org.opensaml:opensaml-core:${versions.opensaml}"
+}
+
+['jarHell', 'thirdPartyAudit', 'forbiddenApisMain', 'splitPackagesAudit', 'checkstyleMain', 'licenseHeaders', 'spotlessJavaCheck'].each {


I think we can (and should) re-enable things like forbiddenApis, checkstyle and spotless here since this module actually has some code (albeit minimal) of its own.

@mark-vieira I see that forbiddenApisMain fails with:

> Task :x-pack:libs:es-opensaml-security-api:forbiddenApisMain FAILED > Task :libs:elasticsearch-core:compileJava Note: Some input files use or override a deprecated API that is marked for removal. Note: Recompile with -Xlint:removal for details. FAILURE: Build failed with an exception. * What went wrong: Execution failed for task ':x-pack:libs:es-opensaml-security-api:forbiddenApisMain'. > Parsing signatures failed: Class 'org.apache.lucene.util.IOUtils' not found on classpath while parsing signature: org.apache.lucene.util.IOUtils

Is it a real problem? I suspect it's just the forbidden task itself missing a dependency classs?

I've only reenable checkstyle and spotless for now

Yeah, this looks to be because some of our forbidden api rules reference classes that aren't on the classpath here. We can disable this task here for now.

albertzaharovits · 2023-08-22T18:57:33Z

@tvernum @mark-vieira any objections to merging this post FF in 8.10?

albertzaharovits · 2023-08-22T19:10:34Z

@elasticsearchmachine run elasticsearch-ci/example-plugins

mark-vieira · 2023-08-22T19:16:35Z

You can ignore the example-plugin build failures for now.

tvernum · 2023-08-23T00:50:22Z

any objections to merging this post FF in 8.10?

None from me. It's the type of upgrade we would consider backporting to a 8.10.x patch release any way, in which case it would be acceptable to merge post FF (since FF only applies to non-patch features)

This commit upgrades to OpenSAML v4.3.0 Versions of OpenSAML ≥ 4.1 have a hard dependency on the non-FIPS release of BouncyCastle. This would prevent ES from being able to run in a JVM where BC-FIPS is configured as the security provider. Closes: elastic#71983 Co-authored-by: Tim Vernum tim@adjective.org

This commit upgrades to OpenSAML v4.3.0 Versions of OpenSAML ≥ 4.1 have a hard dependency on the non-FIPS release of BouncyCastle. This would prevent ES from being able to run in a JVM where BC-FIPS is configured as the security provider. Backport of: #98199 Closes: #71983 Co-authored-by: Tim Vernum tim@adjective.org

…#98785)" This reverts commit 07aedfd.

apache.santuario.xmlsec version 2.1.4 is documented vulnerable. We should update to mitigate the vulnerabilities. But apache.santuario.xmlsec is a dependency of opensaml version 3.*. However, in a patch release of elasticsearch (i.e. 7.17.*) it's best we avoid updating dependencies across major versions (i.e. opensaml from version 3.* to version 4.*), particularly for such a complex dependency as opensaml (we did update the opensaml dep in this way, but in a minor elasticsearch 8.* release, i.e. #98199). The latest opensaml 3.* release (i.e. 3.4.6) still requires a vulnerable apache.santuario.xmlsec dep: https://mvnrepository.com/artifact/org.opensaml/opensaml-xmlsec-impl/3.4.6). In this case, our best hope is to find a non-vulnerable version of apache.santuario.xmlsec that is still on the same major version as the version listed in the deps of opensaml (i.e. 2.*). That's version 2.2.6: https://mvnrepository.com/artifact/org.apache.santuario/xmlsec/2.2.6 , which is not vulnerable This PR updates apache.santuario.xmlsec from the existing 2.1.4 version to the 2.2.6 version. The release notes of the 2.2.0 version from https://santuario.apache.org/javareleasenotes.html look simple, and the dependencies differences (from https://mvnrepository.com/artifact/org.apache.santuario/xmlsec/2.1.4) are minimal as well (hopefully optional dependencies, which we don't pull in, stay optional in the same way in the new version). So, it looks to me that the dep update is relatively safe (and it also passes CI)!

elasticsearchmachine added the v8.10.0 label Aug 4, 2023

albertzaharovits added 5 commits August 4, 2023 14:10

Licenses and spotless

397abc0

More licenses

c49b700

identity-provider misssing classes

d43aa55

:x-pack:libs:es-opensaml-xmlsec-impl', configuration: 'shadow'

381f7f5

thirdPartyAudit

818e660

idp empty configuration properties

41527f4

albertzaharovits added 9 commits August 9, 2023 17:20

Merge branch 'main' into tim-upgrade-opensaml-4.3

002e1dc

Use opensaml.config.ecdh.defaultKDF in xpack.plugin.security

8ee1af9

xml-sec-impl instead of security-api

08e7a29

xml-sec for the idp project as well

55c36bd

ConcatenationKDFGenerator

d488a36

Remove abstract named curve

002e0d7

Works?

842ff7b

Cleanup build.gradle

7398e96

Merge branch 'main' into tim-upgrade-opensaml-4.3

186c932

albertzaharovits marked this pull request as ready for review August 10, 2023 17:06

albertzaharovits requested a review from tvernum August 10, 2023 17:06

albertzaharovits added the :Security/Security Security issues without another label label Aug 10, 2023

albertzaharovits self-assigned this Aug 10, 2023

albertzaharovits requested a review from mark-vieira August 10, 2023 17:07

albertzaharovits added the >non-issue label Aug 10, 2023

elasticsearchmachine added the Team:Security Meta label for security team label Aug 10, 2023

mark-vieira reviewed Aug 10, 2023

View reviewed changes

albertzaharovits added 3 commits August 14, 2023 12:55

Package rename

d6e652c

Apache License

a76db0f

Remove no longer missing classes

4917d12

albertzaharovits requested a review from tvernum August 14, 2023 10:06

tvernum approved these changes Aug 15, 2023

View reviewed changes

quux00 added v8.11.0 and removed v8.10.0 labels Aug 16, 2023

mark-vieira approved these changes Aug 22, 2023

View reviewed changes

albertzaharovits added 2 commits August 22, 2023 21:45

Merge branch 'main' into tim-upgrade-opensaml-4.3

c4c9624

Reenable checkstyle and spotless

1835738

albertzaharovits added 2 commits August 23, 2023 09:27

Merge branch 'main' into tim-upgrade-opensaml-4.3

ef352a3

Merge branch 'main' into tim-upgrade-opensaml-4.3

4f5955b

albertzaharovits merged commit 1b468a0 into elastic:main Aug 23, 2023

albertzaharovits deleted the tim-upgrade-opensaml-4.3 branch August 23, 2023 09:59

albertzaharovits mentioned this pull request Aug 23, 2023

[8.10] Upgrade to OpenSAML 4.3.0 (shadowed) #98785

Merged

dnhatn added a commit to dnhatn/elasticsearch that referenced this pull request Sep 5, 2023

Revert "Upgrade to OpenSAML 4.3.0 (shadowed) (elastic#98199) (elastic…

c8f26f0

…#98785)" This reverts commit 07aedfd.

jakelandis added the v8.10.0 label Sep 14, 2023

ChrisHegarty mentioned this pull request Nov 24, 2023

Add org.opensaml.security to X-Pack Security module-info #102589

Merged

albertzaharovits mentioned this pull request Aug 22, 2024

Update apache.santuario.xmlsec dep from 2.1.4 to 2.2.6 #112022

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to OpenSAML 4.3.0 (shadowed) #98199

Upgrade to OpenSAML 4.3.0 (shadowed) #98199

albertzaharovits commented Aug 4, 2023 •

edited

Loading

albertzaharovits commented Aug 4, 2023

albertzaharovits commented Aug 4, 2023

albertzaharovits commented Aug 10, 2023

albertzaharovits commented Aug 10, 2023

elasticsearchmachine commented Aug 10, 2023

mark-vieira left a comment

mark-vieira Aug 10, 2023

tvernum Aug 11, 2023

mark-vieira Aug 11, 2023

tvernum Aug 11, 2023 •

edited

Loading

albertzaharovits Aug 11, 2023

albertzaharovits Aug 14, 2023

mark-vieira Aug 14, 2023

albertzaharovits Aug 14, 2023

mark-vieira Aug 14, 2023

tvernum Aug 15, 2023

tvernum left a comment

tvernum Aug 15, 2023

tvernum commented Aug 21, 2023

mark-vieira left a comment

mark-vieira Aug 22, 2023

albertzaharovits Aug 22, 2023

albertzaharovits Aug 22, 2023

mark-vieira Aug 22, 2023

albertzaharovits commented Aug 22, 2023

albertzaharovits commented Aug 22, 2023

mark-vieira commented Aug 22, 2023

tvernum commented Aug 23, 2023

Upgrade to OpenSAML 4.3.0 (shadowed) #98199

Upgrade to OpenSAML 4.3.0 (shadowed) #98199

Conversation

albertzaharovits commented Aug 4, 2023 • edited Loading

albertzaharovits commented Aug 4, 2023

albertzaharovits commented Aug 4, 2023

albertzaharovits commented Aug 10, 2023

albertzaharovits commented Aug 10, 2023

elasticsearchmachine commented Aug 10, 2023

mark-vieira 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

tvernum Aug 11, 2023 • 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

tvernum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvernum commented Aug 21, 2023

mark-vieira 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

albertzaharovits commented Aug 22, 2023

albertzaharovits commented Aug 22, 2023

mark-vieira commented Aug 22, 2023

tvernum commented Aug 23, 2023

albertzaharovits commented Aug 4, 2023 •

edited

Loading

tvernum Aug 11, 2023 •

edited

Loading