JackOfMostTrades / aws-kms-pkcs11

PKCS#11 Provider Using AWS KMS
MIT License
39 stars 17 forks source link

[RFC] Various fixes and basic support for "pesign" via NSS #11

Closed ozbenh closed 3 years ago

ozbenh commented 3 years ago

So I'm not familiar at all with the world of PKI and details of how PKCS11 modules generally operate.. this is entirely based on experiments, debugging and whatever docs I managed to read over the last few days but ...

This allows aws-kms-pkcs11 to work as an NSS backend for use with "pesign" which is the signing tool used by most Linux distros to sign PE binaries for secure-boot. Basic operations with "certutil" also seem to work.

Some of the changes in there are convenience (Makefile), bug fixes (handle 0, attribute matching bug), but most of the rest is dealing with assumptions or behaviours of NSS, especially the business with C_CopyObject() which I suppose could be considered controversial (that said, what NSS does seem to be batshit insane to me but what do I know ...)

I hope this doesn't break any of your existing workloads (the Makefile bit about the default install path might...)

ozbenh commented 3 years ago

Actually, question: How much do you want to keep the Makefile being able to find the SDK in your home dir ? This has been problematic because when doing sudo make install, ${HOME} is wrong. This is why I had to move the test for checking if the SDK was found into the build rule, which is ... messy.

For static/dynamic, I'd like to add some extra magic to find out which libs are available to default to with an explicit override, but that will really not work with the above business vs sudo, as I would probably have to put all the logic outside of the build rule.

Is that ok with you to change the Makefile so that you have to explicitly pass AWS_SDK_PATH when using the "home dir" install ?

JackOfMostTrades commented 3 years ago

If AWS_SDK_PATH is something that can be passed in that would be fine.

ozbenh commented 3 years ago

So I've pushed a new version. I've removed the controversial C_CopyObject :-) I've also significantly reworked attributes.cpp, so make sure you test properly. You will have to pass AWS_SDK_PATH explicitly for an SDK in your home dir. I've also documented all the Makefile options including some to control static/dynamic linking of SDK libs

JackOfMostTrades commented 3 years ago

The latest changes look good to me. I'm going to make sure it builds ok for me and do some tests. If that all looks good, I'll hit merge.

JackOfMostTrades commented 3 years ago

I found that the statically linked libs have to be included in the --whole-archive part of the g++ command, so I made a tweak to separately build the STATIC_LIBS from LIBS in the Makefile. Also had to update the circleci script to set the AWS_SDK_PATH. Otherwise everything looked great, so made those changes are merged it.

Just cut release 0.0.6 as well, so if anyone prefers pulling down a prebuilt binary it should work with pesign out of the box now. Thanks for all the work!

ozbenh commented 3 years ago

Ah interesting, with my config (C runtime static, C++ dynamic) it worked. Thanks for creating this in the first place !

ozbenh commented 3 years ago

Hrm ... did you collapse all the commits into one or is that a github artifact ? It's ... not ideal, especially if one wants to bisect a regression later on

JackOfMostTrades commented 3 years ago

I think the default merge strategy on PRs for github to squash+fast forward. I suppose it lets you more easily revert a whole PR that way, but is certainly limiting if you wanted to bisect deeper. It's not something I have strong opinions on (hence the default). :shrug:

ozbenh commented 3 years ago

On Thu, 2021-10-14 at 15:19 -0700, Ian Haken wrote:

I think the default merge strategy on PRs for github to squash+fast forward. I suppose it lets you more easily revert a whole PR that way, but is certainly limiting if you wanted to bisect deeper. It's not something I have strong opinions on (hence the default). 🤷

I see. I spent 20 years in the Linux Kernel, so for me squashing commits is anathema :) But it's your project, do as you please by all means ;-)

Cheers, Ben.