Skip to content

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

Closed
ElMostafaIdrassi wants to merge 3 commits into
indygreg:mainfrom
ElMostafaIdrassi:windows-certificate-feature
Closed

apple-codesign: add support for signing and decrypting with Windows store certificates#111
ElMostafaIdrassi wants to merge 3 commits into
indygreg:mainfrom
ElMostafaIdrassi:windows-certificate-feature

Conversation

@ElMostafaIdrassi

Copy link
Copy Markdown
Contributor

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 indygreg left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is awesome! Thank you so much for the contribution!

I left a handful of comments. But the core of this PR looks very solid to me and I'm not seeing any obvious show stoppers.

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'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.

Comment thread apple-codesign/src/windows.rs Outdated
Comment thread apple-codesign/src/windows.rs Outdated
Comment thread apple-codesign/src/windows.rs
Comment thread apple-codesign/src/windows.rs Outdated
Comment thread apple-codesign/src/windows.rs Outdated
Comment thread apple-codesign/src/windows.rs Outdated
Comment thread apple-codesign/src/cli/certificate_source.rs Outdated
Comment thread apple-codesign/src/cli/certificate_source.rs Outdated
Comment thread apple-codesign/tests/cmd/windows-store-export-certificate-chain.trycmd Outdated
Comment thread apple-codesign/tests/cmd/windows-store-print-certificates.trycmd Outdated
@indygreg

Copy link
Copy Markdown
Owner

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

Copy link
Copy Markdown
Contributor Author

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 indygreg left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I skimmed over the changes and things are looking great! Thank you for incorporating my feedback. Let me know when you are ready for a full review / merge.

@indygreg

Copy link
Copy Markdown
Owner

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

@ElMostafaIdrassi ElMostafaIdrassi force-pushed the windows-certificate-feature branch from 8a3bd9f to a11eb05 Compare November 16, 2023 21:33
@ElMostafaIdrassi

Copy link
Copy Markdown
Contributor Author

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

@indygreg

Copy link
Copy Markdown
Owner

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

  • Add documentation.
  • Rename some fields/arguments for consistency.
  • Remove the retrieval of the certificate thumbprint since it can be derived from the certificate DER.
  • Run rustfmt
  • Add missing #[serde] annotation on the key source.
  • Few misc code 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!

@indygreg indygreg closed this in e2a7def Nov 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants