Make key store types extensible conveniently

[Vampire]

This is a follow-up to #258, only the top-most commit is to be reviewed.
This refactors the key store types into services.
This pulls apart the code into separate classes.
And more importantly it allows to easily extends JSign with further key store types in consumer code,
by simply providing the respective service implementation.

[ebourg]

Thank you for the PR, I've made a similar refactoring on a local branch but I was moderately satisfied with the result because many internal details leak in the public API. I'll probably reevaluate this after the 7.0 release.

[Vampire]

I might misremember, but I think I did not publish more than necessary to implement an own key store, so mainly the getters in the key store builder and the CertificateUtils class.

[ebourg]

My concern is mostly the KeyStoreType private methods that become public (reuseKeyStorePassword(), hasCertificate(), etc). I hoped I could keep these under the rug :)

[Vampire]

Well, custom implementations of JsignKeyStore should be able to provide own implementations of these methods, shouldn't they?

To keep them package-private we could move AbstractJsignKeyStore out of net.jsign.ks to net.jsign, add it a constructor argument for reuseKeyStorePassword, and then require that all custom implementations extend from AbstractJsignKeyStore. But imho those methods are part of the JsignKeyStore interface and you should not require using AbstractJsignKeyStore just like it is right now in my PR.

[Vampire]

By the way, I have just seen that between 6.0 and 7.0-SNAPSHOT you introduced jsign-crypto module.
You now have classes in net.jsign package in jsign-crypto and jsign-core which is bad for JPMS compatibility as there such split packages are not allowed.

[ebourg]

You now have classes in net.jsign package in jsign-crypto and jsign-core which is bad for JPMS compatibility as there such split packages are not allowed.

Yes I'm aware of that, but I'm trying to preserve the backward compatibility as much as possible. Jsign isn't a JPMS module yet so this can be solved later.

[Vampire]

Well, it can nevertheless be put to the module path as automatic module or transformed to be a module for example using the Gradle plugin https://github.com/gradlex-org/extra-java-module-info in consumer projects. Unless it has such split packages which then prevents it. :-)

Just wanted to make you aware of it in case you were not as many people out there are not at all JPMS aware still.

[ebourg]

I've updated my refactoring to use a SPI for the keystores and pushed it to the keystore-spi branch:

• KeyStoreType becomes an interface but retains the same fields and methods (name(), valueOf(), values()) to preserve the compatibility (at least the source compatibility, the binary compatibility is probably broken)
• the hasCertificate() and reuseKeyStorePassword() methods are gone (this change is already on the master branch)
• the pkcs11 and fileBased properties have been removed, the class hierarchy is leveraged instead (PKCS111KeystoreType and FileBasedKeyStoreType)
• The implementations are all in the net.jsign package for now to avoid making some classes public (CertificateUtils, ProviderUtils)

Some points still have to be addressed:

• the ServiceLoader result isn't cached like in your implementation. I'm not sure the performance boost is significant here
• the order of the elements returned by KeyStoreType.values() isn't deterministic, they should probably be sorted
• if several KeyStoreType implementation match the specified name, the first one is picked. This could be used to hijack the default implementations and I'm not sure this is a good idea. Maybe the implementations from the net.jsign package should have the priority
• maybe turn the KeyStoreTypes into singletons, that would require a different SPI, for example a KeyStoreTypeProvider interface returning the unique instance

[Vampire]

KeyStoreType becomes an interface but retains the same fields and methods (name(), valueOf(), values()) to preserve the compatibility (at least the source compatibility, the binary compatibility is probably broken)

In my version it should be fully compatible as I only removed package-private methods and kept it as enum. :-)

• the hasCertificate() and reuseKeyStorePassword() methods are gone (this change is already on the master branch)
• the pkcs11 and fileBased properties have been removed, the class hierarchy is leveraged instead (PKCS111KeystoreType and FileBasedKeyStoreType)

I updated my PR to also adapt these refactorings from your branch.

The implementations are all in the net.jsign package for now to avoid making some classes public (CertificateUtils, ProviderUtils)

In my version ProviderUtils was not made public but was moved along, just CertificateUtils became public as it was also used in other places and could also be useful for custom implementations.

That's also one of the pro-points of having the implementations in a separate package, as you can see what such implementations need, so that they can become public like all the KeyStoreBuilder getters. In custom implementations you will as well need those and by having the built-in implementations in a separate package, you can clearly see that. In your branch those are all still package-private and so a custom implementation would be very hard to impossible unless put to your package which is of course not the best idea.

But for better comparability for now I also moved the classes out to the net.jsign package, but I still think it would be more clean and a better idea to have them in a separate package, even without considering JPMS.

the ServiceLoader result isn't cached like in your implementation. I'm not sure the performance boost is significant here

The intention was not just a performance boost, also to have them more or less singleton, at least at normal usage, while keeping the flexibility to instantiate them when needed.

the order of the elements returned by KeyStoreType.values() isn't deterministic, they should probably be sorted

If such a custom method is used, then yes, I'd say it should at least be a deterministic order if it ist not a by-definition unordered object. But as the return value is an array, I'd say a deterministic order - even if not sorted - would be better.

if several KeyStoreType implementation match the specified name, the first one is picked. This could be used to hijack the default implementations and I'm not sure this is a good idea. Maybe the implementations from the net.jsign package should have the priority

Imho it is better to throw an exception if multiple have the same ID. Yes, this could easily break if for example I have a FOO one with 7.0 and then upgrade to 8.0 which also has a FOO one for example.
But I'm usually a fan of fail-fast.
If you say you prefer the built-in ones with the same situation, then suddenly the built-in one is used which might behave majorly different.
If you say you prefer custom ones, then a malicious dependency could easily hijack it.
And if you want to prefer the built-in ones, you would also need to explicitly list them and not just check for the package unless you create a sealed package for them. And even then that would break with the unholy bad practice of creating fat jars. Without sealed or when broken, "custom" ones could also be put to net.jsign package.
With your current branch even I would probably do it as all the things you need are package-private. (see above :-) )

maybe turn the KeyStoreTypes into singletons, that would require a different SPI, for example a KeyStoreTypeProvider interface returning the unique instance

Or use my version, there they effectively are singletons :-)