Update plugin-cli tool to isolate plugin Bouncy Castle from FIPS BC

[ebarlas]

The goals of this change to plugin-cli are:

• Continue using Bouncy Castle for PGP signature verification
• Upgrade to latest Bouncy Castle (1.83)
• Ensure reliable execution when the ES distribution has BC FIPS 1.x or 2.x in the lib dir

A new bc sub-project library was introduced within plugin-cli. The bc project encapsulated PGP signature verification using Bouncy Castle. The bc library is shadowed into a single uber JAR during distribution.

JAR shadowing is used to ensure that plugin-cli uses its own BC libraries rather than any in the ES lib directory.

bc/
├── build.gradle
├── licenses/
│   ├── bouncycastle-LICENSE.txt
│   └── bouncycastle-NOTICE.txt
└── src/
    └── main/
        └── java/
            └── org/
                └── elasticsearch/
                    └── plugins/
                        └── cli/
                            └── bc/
                                └── PgpSignatureVerifier.java

---

The following PR diff view shows a prior iteration that used a class loader to achieve isolation: https://github.com/elastic/elasticsearch/pull/138949/files/17a688703f62fdaa8cbdee73121465e4c5155e18

[tvernum]

There are a few things that bother me slightly, that it would be nice to fix - if we can do so without adding too much complexity.

1. BcPgpSignatureVerifier is in the standard main/java/ directory. Thus, at compile time everything looks like it has shared dependencies and can access everything else. But in fact that's not true at runtime.
   For example, the PgpSignatureVerifier interface that BcPgpSignatureVerifier implements will be a different class than the one implemented by IsolatedBcPgpSignatureVerifier. Those sorts of class loader issues always come back to seek revenge later on - some change is going to cause something is going leak in some way and it will be horribly confusing because the error will say that BcPgpSignatureVerifier can't be cast to PgpSignatureVerifier which is because of classloaders, but that's not obvious when dealing with an unexpected. runtime error.
   And likewise, it's possible for IsolatedBcPgpSignatureVerifier to just call BcPgpSignatureVerifier directly. Doing so will break when the application class loader contains BC 1.x classes, but the code will compile perfectly fine and the first time anyone will know about it is when it runs through FIPS CI with BC 1.x
   So, if we can do the work to put BcPgpSignatureVerifier into a separate source directory (with zero dependencies), that will make compilation time look a lot more like runtime.

2. For me, the names aren't quite right. We have IsolatedBcPgpSignatureVerifier and BcPgpSignatureVerifier, and "Isolated" refers to the one that isn't isolated, but loads the other one in a way that makes it isolated. I understand the naming, but I think it's also confusing. I would prefer a different naming scheme. My proposal would be to have a PgpSignatureVerifierLoader (or Factory or whatever) that dynamically loads a StandaloneBcPgpSignatureVerifier and then wraps it in an anonymous PgpSignatureVerifier implementation. I think that would be a bit more obvious.

3. I don't love using reflection to call the method (but I can live with it). We could avoid that by changing verifySignature to take 2 arguments (Path and InputStream - the url string is only needed for error handling which could be pushed up) and have it only throw a runtime exception. Then it could implement java.util.function.BiConsumer<Path,InputStream> and though we need reflection for the constructor, we could then cast it to a BiConsumer and call accept

[ebarlas]

@tvernum , I addressed all three points. The Gradle build file increased in complexity, but I think the changes are a net positive and worth the added complexity.

1. Source and classpath separation

• main cannot see BC classes or bc
• bc cannot see main or ES classes, it can only see BC classes
• test sees both main and bc

2. Naming

• IsolatedBcPgpSignatureVerifier renamed to BcPgpSignatureVerifierLoader
• PgpSignatureVerifier interface removed in favor of BiConsumer<Path, InputStream>
• BcPgpSignatureVerifier implements BiConsumer<Path, InputStream>
• IsolatedBcPgpSignatureVerifier implements Supplier<BiConsumer<Path, InputStream>>>

3. Method reflection

• BiConsumer accept method used instead of method reflection

[tvernum]

That approach looks good to me.

[elasticsearchmachine]

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

[elasticsearchmachine]

Hi @ebarlas, I've created a changelog YAML for you.

[mark-vieira]

An isolated class loading environment is used to reliably source all BC FIPS classes from the plugin-cli classpath rather than the broader ES lib classpath.

Can we elaborate more on this? Is the concern here that when customers are running in FIPS mode, they provide their own bouncycastle implementation and this would in turn leak into the plugin CLI classpath causing potential issues? If so, I think we already have a solution for this, which is using EmbeddedImplClassLoader as we do for other things we want to ensure we are using a very particular implementation such as jackson. @rjernst do my comments here line up with the intention of embedded impl jars?

[ebarlas]

Can we elaborate more on this? Is the concern here that when customers are running in FIPS mode, they provide their own bouncycastle implementation and this would in turn leak into the plugin CLI classpath causing potential issues?

Yes, the goal is for plugin-cli to work reliably in an ES distribution configured for FIPS with Bouncy Castle (BC FIPS JAR files in ES lib directory).

We evaded this issue in the past by ensuring that the plugin-cli tool has the same BC FIPS JARs that are officially supported for FIPS mode.

Now, with FIPS 140-3 and BC FIPS 2.x, the landscape is more complicated.

The isolation technique used here ensures that the ES classpath (ES lib directory) is not considered when loading BC classes.

I think we already have a solution for this, which is using EmbeddedImplClassLoader

EmbeddedImplClassLoader does look similar, but I think a plain URLClassLoader is adequate for this simple use case.

[rjernst]

We didn't disable plugin cli tests under BC 1.x, why is the situation different with 2.x?

[ebarlas]

Previously, plugin-cli and ES in FIPS mode always used the same BC FIPS 1.x JARs. Now, the landscape has changed, since ES in FIPS mode could be BC FIPS 1.x or BC FIPS 2.x.

[ebarlas]

We could expand the conditional expression as follows.

if (buildParams.inFipsJvm && buildParams.fipsMode == "140-2") {
  ...
}

[rjernst]

We never had jarhell though, even in FIPS mode when BC existed in lib (added by test CI/user) as well as under libs/tools/plugin-cli. How did that work?

[ebarlas]

There is no conflict or issue when ES lib contains org.bouncycastle:bc-fips:1.0.2.6 and plugin lib contains both org.bouncycastle:bc-fips:1.0.2.6 and org.bouncycastle:bcpg-fips:1.0.7.1. That works just fine.

[rjernst]

I think we already have a solution for this, which is using EmbeddedImplClassLoader as we do for other things

There's a key difference, EmbeddedimplClassLoader still can't conflict with anything in the boot classloader. Yet in the case of bouncycastle existing under lib when added by test CI or users, we would still have jarhell.

[mark-vieira]

Using source sets here seems unnecessarily complex and feels like a hack to tell the IDE about runtime implementation details. Couldn't we just as well put this in it's own library project to do the same thing? I know the lines on when you do which can be a bit merky but all we're trying to do is separate code based on dependencies, and separate projects/modules accomplishes the same thing, and is a much less confusing pattern. Generally we use sourcesets when the code we're talking about is highly coupled.

[mark-vieira]

Do we still need to do this? We want to test against the shadow jar, right, and then just add bouncycastle back to the test runtime classpath so we don't get classnotfound errors.

[ebarlas]

Yes, ideally we'd like to test against the shadowed JAR. But in this case, InstallPluginActionTests is deeply coupled directly with Bouncy Castle. The test defines an anonymous class that extends InstallPluginAction and manipulates signature verification logic.

Without this configuration, tests fail with NoClassDefFoundError.

[ebarlas]

I believe mixing org.bouncycastle and shadow.org.bouncycastle from the test perspective will be problematic, but I'll give it a try.

[mark-vieira]

Yeah, bouncy castle will be on the classpath twice here, but one will be shadowed. That should be fine, no?

[ebarlas]

It's problematic because of the nuances of InstallPluginActionTests.

Here's the simple diff to (1) use the default test class path with the shadowed JAR and (2) selectively add BC JARs.

 tasks.named("test").configure {
   // Use original classpath, not the shadow JAR, since test code imports non-relocated packages
-  classpath = sourceSets.test.runtimeClasspath
+  classpath += configurations.runtimeClasspath.filter {
+    it.name.startsWith("bc")
+  }

As you can see from this test failure, there is disagreement about BC classes when they are referenced from tests since InstallPluginAction uses shadow.org.bouncycastle and InstallPluginActionTests uses org.bouncycastle.

REPRODUCE WITH: ./gradlew ":distribution:tools:plugin-cli:test" --tests "org.elasticsearch.plugins.cli.InstallPluginActionTests" -Dtests.method="testSlowSignatureVerificationMessage {p0=com.google.common.jimfs.JimfsFileSystem@3138953b p1=org.elasticsearch.plugins.cli.InstallPluginActionTests$1Parameter$$Lambda/0x00000fe0003dde88@27df95e}" -Dtests.seed=23C6D41096344C33 -Dtests.locale=cy-Latn-GB -Dtests.timezone=Africa/Banjul -Druntime.java=25

'void org.elasticsearch.plugins.cli.InstallPluginAction.computeSignatureForDownloadedPlugin(java.io.InputStream, java.io.InputStream, org.bouncycastle.openpgp.PGPSignature)'
java.lang.NoSuchMethodError: 'void org.elasticsearch.plugins.cli.InstallPluginAction.computeSignatureForDownloadedPlugin(java.io.InputStream, java.io.InputStream, org.bouncycastle.openpgp.PGPSignature)'
	at __randomizedtesting.SeedInfo.seed([23C6D41096344C33:CABFD595F4D03C34]:0)
	at org.elasticsearch.plugins.cli.InstallPluginActionTests.testSlowSignatureVerificationMessage(InstallPluginActionTests.java:511)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
	at java.base/java.lang.reflect.Method.invoke(Method.java:565)
	at com.carrotsearch.randomizedtesting.RandomizedRunner.invoke(RandomizedRunner.java:1763)
	at com.carrotsearch.randomizedtesting.RandomizedRunner$8.evaluate(RandomizedRunner.java:946)
	at com.carrotsearch.randomizedtesting.RandomizedRunner$9.evaluate(RandomizedRunner.java:982)
	at com.carrotsearch.randomizedtesting.RandomizedRunner$10.evaluate(RandomizedRunner.java:996)

[rjernst]

This should be resolvable by moving the shadowing to an inner library/project of plugin-cli. That is, create a subproject like you did before which will depend on (and shade) bouncycastle. Then the plugin-cli would depend on (compile and runtime) on this lib and in InstallPluginAction use the classes from this library.

[ebarlas]

This worked well!

I reinstated the bc sub-project library. It now houses PgpSignatureVerifier, which completely encapsulates Bouncy Castle. This library gets shadowed and included in the plugin-cli distribution.

BC JARs are added explicitly to plugin-cli test class path but no collision occurs due to the shadowing of bc.

Overall, the PR amounts to a number of structural changes to get things in place. The only logic change needed was confined to a few lines in InstallPluginActionTests.

[mark-vieira]

This change shouldn't be necessary. Not sure why we use path explicitly sometimes but it's redundant, and I think it's possible Gradle will be deprecating this map-style configuration syntax at some point in the future.

[ebarlas]

Good catch. I stripped away the shadow config and left the explicit path. It's no longer needed.

[mark-vieira]

LGTM