search

indygreg / apple-platform-rs

Rust crates supporting Apple platform development
565 stars 38 forks source link

apple-codesign: add support for signing and decrypting with Windows store certificates #111

Closed ElMostafaIdrassi closed 9 months ago

ElMostafaIdrassi commented 9 months ago

rcodesign does not yet support signing / decrypting with certificates that are stored in the Windows store. This PR addresses that by allowing users to specify the certificate's SHA1 fingerprint (as it can be gathered from certmgr.msc or certlm.msc).

indygreg commented 9 months ago

I'm actively working on introducing config files support and I will likely introduce merge conflicts with this PR. If you don't want to incur the hardship of a rebase, that's fine: feel free to just edit this PR until it passes review and I'll do the rebase myself.

ElMostafaIdrassi commented 9 months ago

Hi @indygreg,

Thank you for the comments! I really appreciate you taking the time to go over this PR.

I'm not thrilled with the amount of unsafe Rust. But unless someone else has implemented a safe wrapper to the win32 APIs you are using, I suppose there's nothing we can do. I'm not going to scope bloat you to publish a safe indirection layer.

I had a hunch the use of unsafe Rust was not appropriate, but I could not find any safe alternatives. I believe this is a good start, and the code should obviously be modified once such alternative exists (maybe I will write one if I have some time, but no promises :) ).

I'm also impressed you were able to navigate all the wonky cryptography interfaces we have. Those interfaces are a mess and evolved organically, before the RustCrypto projects had more reasonable traits in place. I hope to one day clean things up and standardize on more common/reasonable traits.

Thanks you! TBH, the interfaces looked pretty straightforward to me, and the integration with the Windows store did not take me long to figure out.

I'm actively working on introducing config files support and I will likely introduce merge conflicts with this PR. If you don't want to incur the hardship of a rebase, that's fine: feel free to just edit this PR until it passes review and I'll do the rebase myself.

I will address the issues you have raised in the coming days. I will also try to rebase after you have completed your changes, although I may have to hand it over to you in case I face some major issues.

indygreg commented 9 months ago

Oh, I think most of my churn around configuration files is done. I landed that feature ~24 hours ago on main.

ElMostafaIdrassi commented 9 months ago

I have rebased and force pushed. Let me know what you think.

indygreg commented 9 months ago

I'm going to apply these commits locally along with some minor changes:

Your commit authorship has been retained. I anticipate pushing in a few minutes, at which time this PR should be marked as closed instead of merged.

Note: I'm currently traveling and my Windows machine at home went offline several days ago and I'm unable to test this new functionality. So I'm taking it on faith that things work. But the code seems pretty solid. The only thing I can think of is that the Windows APIs aren't working as I expect them too. I suppose we'll find out in due time if there are bugs. But it is like that for all software :)

Thank you again for this terrific contribution!