search

opensearch-project / security

🔐 Secure your cluster with TLS, numerous authentication backends, data masking, audit logging as well as role-based access control on indices, documents, and fields
https://opensearch.org/docs/latest/security-plugin/index/
Apache License 2.0
199 stars 278 forks source link

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

Open dancristiancecoi opened 5 days ago

dancristiancecoi commented 5 days ago

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 https://github.com/opensearch-project/security/issues/3420 https://github.com/elastic/elasticsearch/issues/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

cwperks commented 5 days ago

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

dancristiancecoi commented 5 days ago

@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 commented 4 days ago

@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 commented 4 days ago

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 commented 3 days ago

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.