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

Migrate from BC to BCFIPS libraries #1087

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

cwperks
Copy link
Member

@cwperks cwperks commented Mar 21, 2025

Description

bouncycastle was removed from the gradle version catalog in https://github.com/opensearch-project/OpenSearch/pull/17507/files#diff-697f70cdd88ba88fe77eebda60c7e143f6ad1286bca75017421e93ad84fb87df.

This PR migrates from BC to BCFIPS libraries for this repo

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Craig Perkins <[email protected]>
Copy link

codecov bot commented Mar 21, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.30%. Comparing base (b0d1ecc) to head (e494942).

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #1087   +/-   ##
=========================================
  Coverage     76.30%   76.30%           
  Complexity     1076     1076           
=========================================
  Files           101      101           
  Lines          5276     5276           
  Branches        504      504           
=========================================
  Hits           4026     4026           
  Misses         1001     1001           
  Partials        249      249           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Signed-off-by: Craig Perkins <[email protected]>
@dbwiddis
Copy link
Member

dbwiddis commented Mar 21, 2025

Hey @cwperks we previously had 1.8.0 which caused jar hell. We downgraded to the version catalog to avoid that. (See #1072)

But I expect our dependency manager will suggest an upgrade again.

Can you be more clear on the reason for this downgrade if it is not for consistency with other dependencies? Can we not make this 1.8.0 again by reverting #1078?

@cwperks
Copy link
Member Author

cwperks commented Mar 21, 2025

@dbwiddis I can try to update the version, but the purpose of this PR is to put the version inline instead of referencing the version catalog because versions.bouncycastle was removed from the version catalog.

@beanuwave has been working on a large effort around FIPS-140-3 compliance for OpenSearch which includes swapping BC non-FIPS jars with FIPS jars so non-FIPS references were removed from the core.

Currently the jar swap is only for the core and has not been performed in the plugins. Plugins that referenced the version of bouncycastle from core's version catalog now need to put the version inline.

Signed-off-by: Craig Perkins <[email protected]>
@cwperks
Copy link
Member Author

cwperks commented Mar 21, 2025

@dbwiddis I think the update will work, but core also changed the qualifier to beta1 and we now need to wait for all of FF's dependent plugins to update accordingly.

Switching back to 1.78 in the interim

Signed-off-by: Craig Perkins <[email protected]>
@cwperks
Copy link
Member Author

cwperks commented Mar 21, 2025

@dbwiddis I think the update will work, but core also changed the qualifier to beta1 and we now need to wait for all of FF's dependent plugins to update accordingly.

Confirmed it would work.

When checking out core you need to specify the qualifier when building. i.e. ./gradlew publishToMavenLocal -Dbuild.version_qualifier=alpha1

@dbwiddis
Copy link
Member

The bigger question is why we are not aligning on a common version. I’d like to prevent future churn.

@cwperks
Copy link
Member Author

cwperks commented Mar 21, 2025

One option could be to do the swap in this repo from non-FIPS -> FIPS deps for BC dependencies.

CC @beanuwave

@dbwiddis
Copy link
Member

One option could be to do the swap in this repo from non-FIPS -> FIPS deps for BC dependencies.

CC @beanuwave

Sure, happy to look into this. Still trying to understand what happened here.

@cwperks
Copy link
Member Author

cwperks commented Mar 22, 2025

Was whatever was causing the jar hell removed and 1.80 will work fine?

It used to collide with the dependencies in opensearch-test-framework here. With this PR those deps changed to BCFIPS.

> Task :dependencyInsight
Dependency resolution failed because of conflict on the following module:
   - org.bouncycastle:bcprov-jdk18on between versions 1.80 and 1.78

org.bouncycastle:bcprov-jdk18on:1.80
  Variant compile:
    | Attribute Name                 | Provided | Requested    |
    |--------------------------------|----------|--------------|
    | org.gradle.status              | release  |              |
    | org.gradle.category            | library  | library      |
    | org.gradle.libraryelements     | jar      | classes      |
    | org.gradle.usage               | java-api | java-api     |
    | org.gradle.dependency.bundling |          | external     |
    | org.gradle.jvm.environment     |          | standard-jvm |
    | org.gradle.jvm.version         |          | 21           |
   Selection reasons:
      - By conflict resolution: between versions 1.80 and 1.78

org.bouncycastle:bcprov-jdk18on:1.80
\--- testCompileClasspath

org.bouncycastle:bcprov-jdk18on:1.78 -> 1.80
\--- org.opensearch.test:framework:3.0.0-alpha1-SNAPSHOT:20250319.180915-157
     \--- testCompileClasspath

@dbwiddis
Copy link
Member

ncies in opensearch-test-framework here. With this PR those deps changed to BCFIPS.

So I think the best action for us is to do the same

@cwperks cwperks changed the title Set bouncycastle version inline Remove unused bouncycastle dependency Mar 23, 2025
@cwperks cwperks changed the title Remove unused bouncycastle dependency Migrate from BC to BCFIPS libraries Mar 23, 2025
cwperks added 2 commits March 22, 2025 21:24
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
build.gradle Outdated
@@ -177,7 +177,8 @@ dependencies {
implementation "software.amazon.cryptography:aws-cryptographic-material-providers:1.9.0"
implementation "org.dafny:DafnyRuntime:4.10.0"
implementation "software.amazon.smithy.dafny:conversion:0.1.1"
implementation "org.bouncycastle:bcprov-jdk18on:${versions.bouncycastle}"
// needed by aws-encryption-sdk-java
implementation "org.bouncycastle:bc-fips:2.0.0"
Copy link
Member Author

Choose a reason for hiding this comment

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

Switch this to implementation "org.bouncycastle:bc-fips:${versions.bouncycastle_jce}" when FF updates to beta1 qualifier.

This is added in the version catalog after the cut to beta1 and is not available in the alpha1 artifacts.

Signed-off-by: Craig Perkins <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x backport PRs to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants