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

Update to OpenSAML 4 #77012

Merged
merged 8 commits into from
Sep 22, 2021
Merged

Update to OpenSAML 4 #77012

merged 8 commits into from
Sep 22, 2021

Conversation

tvernum
Copy link
Contributor

@tvernum tvernum commented Aug 30, 2021 •
edited
Loading

This commit switches the security and identity-provider plugins to use
v4.0 of the OpenSAML library (upgraded from v3.4).

In order to facilitate this upgrade the following changes are also
made:

  • Common Codec is upgraded to v1.14 across all modules
  • Guava is upgraded to v28.2 in the 2 affected modules

Relates: #71983

@tvernum
Update to OpenSAML 4
4af6fa2 
This commit switches the security and identity-provider plugins to use
v4 of the OpenSAML library (upgraded from v3).

In order to facilitate this upgrade the following changes are also
made:
- Common Codec is upgraded to 1.15 across all modules
- Guava is upgraded to v30 in the 2 affected modules
- BouncyCastle has added of the 2 affected modules (OpenSAML4 has a
  direct dependency on BouncyCastle that we haven't found a way to
  avoid yet)
- SecureSM has been changed to support the Cleaner class in Java9, and
  the InnocuousThread more generally
@tvernum tvernum added >enhancement WIP :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) v8.0.0 :Security/IdentityProvider Identity Provider (SSO) project in X-Pack labels Aug 30, 2021
@elasticmachine elasticmachine added the Team:Security Meta label for security team label Aug 30, 2021
@elasticmachine
Copy link
Collaborator

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

@tvernum tvernum marked this pull request as draft August 30, 2021 08:44
@tvernum tvernum mentioned this pull request Aug 31, 2021
Closed
2 tasks
@tvernum
Copy link
Contributor Author

tvernum commented Aug 31, 2021

Reconfirming what we already know, adding the standard BC Jars to the classpath means that we cannot run in FIPS mode.
If I hack around the JarHell checks, we end up with:

[2021-08-31T18:47:10,868][ERROR][o.e.b.ElasticsearchUncaughtExceptionHandler] [node01] fatal error in thread [main], exiting
java.lang.NoSuchMethodError: 'byte[] org.bouncycastle.util.encoders.Hex.decodeStrict(java.lang.String)'
	at org.bouncycastle.crypto.ec.CustomNamedCurves$2.createParameters(Unknown Source) ~[?:?]
	at org.bouncycastle.asn1.x9.X9ECParametersHolder.getParameters(Unknown Source) ~[bc-fips-1.0.2.jar:?]

@tvernum
Copy link
Contributor Author

tvernum commented Aug 31, 2021

And if I remove bcprov from the modules/x-pack-security directory, we fail (as expected) with:

[2021-08-31T18:49:44,716][ERROR][o.e.b.ElasticsearchUncaughtExceptionHandler] [node01] fatal error in thread [main], exiting
java.lang.NoClassDefFoundError: org/bouncycastle/crypto/DerivationParameters
	at org.opensaml.xmlsec.config.impl.DefaultSecurityConfigurationBootstrap.buildDefaultEncryptionConfiguration(DefaultSecurityConfigurationBootstrap.java:121) ~[?:?]
	at org.opensaml.xmlsec.config.impl.GlobalSecurityConfigurationInitializer.init(GlobalSecurityConfigurationInitializer.java:36) ~[?:?]
	at org.opensaml.core.config.InitializationService.initialize(InitializationService.java:56) ~[?:?]
	at org.elasticsearch.xpack.security.authc.saml.SamlUtils.lambda$initialize$0(SamlUtils.java:92) ~[?:?]

@tvernum
Copy link
Contributor Author

tvernum commented Sep 1, 2021 •
edited
Loading

I can use a javaagent to load BCFIPS & BCJSSE on an isolated classloader and register them as security providers. That avoids the JarHell problems (both the triggering of the check and the real problem of having 2 different versions of a class within a single classloading hierarchy).

However, that still means that we would ship with a non-FIPS-certified version of BC, we just wouldn't install it as a java.security.Provider. That's not a good outcome from a FIPS point of view.

Which is to say, I think we can make it work technically, but there could still be a FIPS policy problem.

@tvernum
Downgrade to OpenSAML4.0.1
43397e4 
This resolves FIPS issues by removing the direct dependency on
BouncyCastle's bcprov jar, see
http://shibboleth.net/pipermail/users/2021-May/049989.html
@tvernum
Copy link
Contributor Author

tvernum commented Sep 2, 2021

@elasticmachine update branch

@tvernum
Copy link
Contributor Author

tvernum commented Sep 6, 2021

@elasticmachine update branch

@ChrisHegarty ChrisHegarty mentioned this pull request Sep 15, 2021
Closed
@tvernum tvernum removed the WIP label Sep 21, 2021
@tvernum
Copy link
Contributor Author

tvernum commented Sep 21, 2021

@elasticmachine run elasticsearch-ci/part-2-fips please

@tvernum tvernum marked this pull request as ready for review September 21, 2021 08:08
@tvernum tvernum requested review from jkakavas and ywangd and removed request for ywangd September 21, 2021 08:08
@tvernum
Copy link
Contributor Author

tvernum commented Sep 22, 2021

CI job failed due to #78160

@tvernum
Copy link
Contributor Author

tvernum commented Sep 22, 2021

@elasticmachine update branch

jkakavas
jkakavas reviewed Sep 22, 2021
View reviewed changes
Copy link
Member

@jkakavas jkakavas left a comment

Choose a reason for hiding this comment

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

Worked through all the Java changes, LGTM

@tvernum tvernum changed the title [DRAFT] Update to OpenSAML 4 Update to OpenSAML 4 Sep 22, 2021
@tvernum
Copy link
Contributor Author

tvernum commented Sep 22, 2021

@elasticmachine run elasticsearch-ci/part-2 please

@tvernum tvernum merged commit e73d16d into elastic:master Sep 22, 2021
tvernum added a commit to tvernum/elasticsearch that referenced this pull request Sep 22, 2021
@tvernum
Revert unintended changes from elastic#77012
cfe2df9 
This reverts some changes from
e73d16dc20cf50a5215ee8ff8cccfcbd2f0c1a7es that were incorrectly
included within elastic#77012.
@tvernum tvernum mentioned this pull request Sep 22, 2021
Merged
elasticsearchmachine pushed a commit that referenced this pull request Sep 22, 2021
@tvernum
Revert unintended changes from #77012 (#78182)
8312537 
This reverts some changes from
e73d16dc20cf50a5215ee8ff8cccfcbd2f0c1a7es that were incorrectly
included within #77012.
@tvernum tvernum mentioned this pull request Jan 14, 2022
Closed
@tvernum tvernum mentioned this pull request Jun 3, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkakavas jkakavas jkakavas left review comments

Assignees
No one assigned
Labels
>enhancement :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) :Security/IdentityProvider Identity Provider (SSO) project in X-Pack Team:Security Meta label for security team v8.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tvernum @elasticmachine @jkakavas @jakelandis