eclipse-jgit / jgit

JGit, the Java implementation of git
https://www.eclipse.org/jgit/
Other
148 stars 41 forks source link

Unconditional addition of BouncyCastle provider breaks usage of other Providers #33

Closed mcowger closed 2 months ago

mcowger commented 8 months ago

https://github.com/eclipse-jgit/jgit/blob/1a654d3db6e5bd667dd5218a55084545538519a2/org.eclipse.jgit.gpg.bc/src/org/eclipse/jgit/gpg/bc/internal/BouncyCastleGpgSigner.java#L58-L60

In many cases, users explicitly manage the JSPs that exist in their environment to meet various compliance related requirements that BouncyCastle may not be able to fulfill (certain parts of FIPS-140, for example).

By forcibly adding bouncy castle as a provider on these lines, jgit breaks the ability of developers to control the JSPs in use, and thus are unable to meet some of these compliance requirements.

Rather than forcible addition of BouncyCastle as a provider, jgit should instead check for a viable provider of the needed algorithms and use that (in most cases, BouncyCastle will be available by default. If not available, an exception should be thrown.

Alternatively, an option could be added to the relevant constructor to disable this behavior for users that need to control their JSPs.

msohn commented 8 months ago

The library org.eclipse.jgit.gpg.bc uses bouncycastle to implement GPG signing in JGit. Since it uses bouncycastle APIs I don't see a way how this library could work with a different Java Security Provider which most probably doesn not implement bouncycastle APIs. If you don't want to use bouncycastle simply don't deploy this library.

Hence I think if you want to use a different GPG implementation you would need to create a different subclass of GpgSigner in another library.

popmonkey commented 8 months ago

I don't think the issue is that BouncyCastle is used but rather that it is being added to the Security provider chain. I would like to see a change where BC is memoized and then used explicitly without being inserted in this way.

msohn commented 8 months ago

I don't understand what you want. Maybe you can implement your idea and push it for review ?

tomaswolf commented 8 months ago

The OP does raise a valid question: is this unconditional registration of the BouncyCastle provider necessary?

The code has no direct dependency on it. GPG does use a few crypto algorithms that are not present in standard providers, for instance AES/OCB/NoPadding used for some encrypted private keys. The code uses the .jcajce crypto packages of BC, and thus it can use such algorithms if the BC security provider is installed. But maybe using the BC light-weight APIs through the .bc packages instead of the jcajce ones might be an alternative. The BC light-weight API stuff should work without installing the BouncyCastleProvider.

Whether that helps if a client has to meet FIPS requirements is unclear to me. Does FIPS even know/allow AES/OCB? (Doesn't look like it does to me, but I'm no FIPS expert.)

msohn commented 2 months ago

fixed by https://review.gerrithub.io/c/eclipse-jgit/jgit/+/1199821 in 7.0