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

[BUG] Current version of OpenSAML is incompatible with the Bouncy Castle FIPS library #4915

Open
dancristiancecoi opened this issue Nov 18, 2024 · 5 comments
Labels
bug Something isn't working triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable.

Comments

@dancristiancecoi
Copy link
Contributor

dancristiancecoi commented Nov 18, 2024

What is the bug?
Current version of OpenSAML (4.3.2) does not work in FIPS mode as it has a hard dependency (org.bouncycastle.jce.ECNamedCurveTable) on the non-FIPS distribution of Bouncy Castle

How can one reproduce the bug?
Steps to reproduce the behavior:

  1. Replace BC with BC-FIPS
  2. Run OpenSAML related tests
  3. You will see a error due to missing the ECNamedCurveTable dependency

Potential solutions

  1. Downgrade OpenSAML to 3.x
  2. Downgrade OpenSAML to 4.0 (Tests seem to pass under that version, however some untested functionality might still be broken)
  3. Replace OpenSAML with Keycloak as it supports running in FIPS 140-2 compliant mode

Relevant links
#3420
elastic/elasticsearch#71983
https://shibboleth.atlassian.net/wiki/spaces/DEV/pages/1159627167/FIPS
https://shibboleth.net/pipermail/dev/2023-August/011111.html
https://www.keycloak.org/server/fips

@dancristiancecoi dancristiancecoi added bug Something isn't working untriaged Require the attention of the repository maintainers and may need to be prioritized labels Nov 18, 2024
@cwperks cwperks added triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable. and removed untriaged Require the attention of the repository maintainers and may need to be prioritized labels Nov 18, 2024
@cwperks
Copy link
Member

cwperks commented Nov 18, 2024

[Triage] Thank you for filing an issue for this @dancristiancecoi. Should I assign this issue to you?

@dancristiancecoi
Copy link
Contributor Author

@cwperks I can give it a try, thanks!

Would also be great to get some feedback from the community on what potential solution would be the best way forward.

@shshankvikrram
Copy link

@dancristiancecoi
Removing service 'META-INF/services/org.opensaml.security.crypto.ec.NamedCurve' from opensaml will ensure EC classes are not loaded eagerly and you can remove bc non fips dependencies.

Elastic search have implemented this already, please refer to this : https://github.com/elastic/elasticsearch/pull/98199/files

@dancristiancecoi
Copy link
Contributor Author

Excellent find @shshankvikrram, thanks!

Am I correct saying that this fixes the error on start-up for Elasticsearch but the functionality that relies on EC will still be broken?

In OpenSearch's case the error does not happen on start-up but when the OpenSAML code gets called. If we used this solution and we do not rely on any EC related functionality then we might be okay.

Are we confident that we don't rely on any of that functionality and that the test coverage in that area is sufficient to prove this?

@shshankvikrram
Copy link

Yes if it relies on EC then it might break but that doesn't seem to be the case here as per my understanding. Looks like OpenSearch doesn't invoke any flow which might use EC functionality in OpenSAML though I am not 100% confident.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable.
Projects
None yet
Development

No branches or pull requests

3 participants