ebourg / jsign

Java implementation of Microsoft Authenticode for signing Windows executables, installers & scripts
https://ebourg.github.io/jsign
Apache License 2.0
250 stars 107 forks source link

Improve HashiCorpVaultSigningService to support multiple secret engines and enhance error handling #229

Closed eatasaur closed 23 hours ago

eatasaur commented 3 weeks ago
ebourg commented 3 weeks ago

Thank you for the PR. Does the Transit engine store the key in an HSM?

eatasaur commented 3 weeks ago

Hi Emmanuel,

Thank you very much for your implementation of jsign. I've been needing exactly this type of security provider in order to sign complex binaries using HashiCorp Vault.

The HashiCorp Vault - Transit Secrets Engine is the "native" cryptographic key storage engine for Vault.

The keys are meant to be stored on the designated Vault - Storage Backend. The storage backend can be internal, a data file which Vault uses as storage, or an external storage backend, (i.e., Consul, DynamoDB, etc..).

As far as I know, HashiCorp Vault only supports integrating an HSM as a means of maintaining the Vault Root Key lifecycle, and not key storage.

As for the rest of the PR, as you can see I've added a logger to debug certain edge cases. I decided to hand in the PR with it, as well, as I wouldn't know exactly where to propagate the information but believe that there is value in it. I've also added a more verbose exception handling, and for that I had to change some String expectations with the HashiCorpVaultSigningServiceTest, (Which mentioned "Google Cloud KMS" specifically). Lastly, I'm not extremely satisfied with the way I determined with an additional boolean property whether the endpoint is responding with a Google Cloud KMS or a Transit backend, so if you find that there's a more elegant solution in this case, I would love to see it.

Once again, thanks and have a great rest of the day!

ebourg commented 3 weeks ago

I agree the boolean isn't ideal. I see two solutions:

  1. Try to guess the type of the endpoint, either GCP or Transit, by looking for gcp or transit in the endpoint URI, and if it's inconclusive, call an API method that only works for one of the two engines. For example /config for GCPKMS or /config/keys for Transit.
  2. Use a different SigningService for each engine. For example HASHICORPVAULT/TRANSIT and keep HASHICORPVAULT for GCPKMS or rename it to HASHICORPVAULT/GCPKMS. Or maybe just TRANSIT since Vault has been forked as OpenBao recently, so me may as well drop the HashiCorp name.
eatasaur commented 2 weeks ago

Initially, I was wanting to add a new SigningService for Vault Transit, but ended up not to, as that was a bit more work, and I'm not fully aware of the overhead associated with allowing jsign to support a new SigningService. That would, of course, fulfil the requirement.

That being said, both solutions would be perfectly valid in my opinion, so please let me know how you'd prefer to proceed from here. If there are changes that can be implemented by me, I would try to put in the effort to do so.

Thanks!

ebourg commented 2 weeks ago

Different services for each engine is probably cleaner and easier to implement.

ebourg commented 1 week ago

@eatasaur Do you think you could squash your commits and rebase on top of the current master branch please?

ebourg commented 23 hours ago

I've merged your changes with some simplifications (less logging and error handling to keep the code short, it's still possible to analyze potential errors using the new debug mode). I've noticed that the Transit support could be further improved by loading the certificate associated to the key (using the export key endpoint).

Thank you for your contribution!