-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature: pkcs11-tool based HSM support for factory #278
Conversation
I took a part of the refactoring made by @detsch from #270 and replaced the HSM implementation with calls to pkcs11-tool. The result is a working solution as confirmed by a unit test, which is able to generate a valid certificate. Steps to test on Ubuntu:
|
Test produced a valid certificate based on softhsm keys for me locally:
|
If the tool is available on all required platforms then it seems like a good way forward. I thought about building our own tool around pkcs11/openssl, since suhc the tool already exists then why not. |
What I find even better in this solution is that we don't need to wrap Not as good as directly talking to the vendor specific PKCS11 library, but saves us a lot of cross-compilation headache.
|
|
8e3b8d9
to
72660c5
Compare
@doanac @mike-sul @detsch @StealthyCoder @camilamacedo86 Full testing will be done tomorrow, but this is kind of ready to go. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any use keeping the bashpki? I don't think we'll ever test or ship it. So its basically just us carrying around unsupported code, right?
Is this the correct command to store key/cert in HSM?
I tried it, no any errors, however nothing is stored in the HSM, all items are in file system. |
Added unit tests and all fixes I found while testing. |
@mike-sul there was a bug introduced while I was shuffling code around. |
@doanac while testing this stuff I found out quite a few discrepancies in the Golang based PKI implementation. That said, after I added unit tests which apply exactly the same test flow to all 3 PKI implementations, I came to the conclusion that it is worth to keep all of them. Please, see the example test results: |
I suppose the question whether to allow storing both factory and local/offline CA keys in HSM, or just one of them in out of the scope of the given PR. |
I suppose getting of the factory CA key from HSM to sign a new device offline CA csr is out of the scope of this PR, and the same for device CSRs signing by an offline CA key stored in HSM? |
The happy path works on my host, the factory root CA priv key is stored in the HSM.
|
When I ran the I am not sure that this potential issue should be discussed in the scope of the given PR at all, just wanna mention for the future consideration. |
Yes, the only things in scope are:
All other improvements need discussion, and risk growing this PR indefinitely.
This sounds like a bug, the command should have failed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
This is a quite big change. I've reviewed it without detailed analyses of the PKI keys/certs generation details for each implementation types.
I suppose we assume that the unit tests guarantee that the keys/certs are correct.
In any case, I believe it makes sense to test happy path e2e for the default case. I've just checked it partially. Testing of adding a new offline device CA and signing device CSR with the offline CA is gonna take some time.
We have already tested this implementation on Windows in the field. Also tested it on Linux using a local build. Signed-off-by: Volodymyr Khoroz <[email protected]>
This small discrepancy was found by the newly added unit tests. Signed-off-by: Volodymyr Khoroz <[email protected]>
Although this is not critical for the CSR which is warranted to be properly formatted by our API, it is really a problem for the certificate file loaded from the user filesystem. Signed-off-by: Volodymyr Khoroz <[email protected]>
This might seem contradicting, but allows for some benefits: - No shell script files need to be created; they are piped into the Bash in-memory. - All temporary csr files are created/removed in /tmp; only necessary files stay in PKI directory. - Eventually, we can remove these Bash scripts from the API code base. - Interested users have direct OS access to these scripts allowing them to build their custom PKI solution. I copied these scripts verbatim from the API and only made minimal changes to make them work: - substituted template parameters with script parameters. - removed $here variable, as scripts are executed inside the certs dir. - removed sign-csr execution from create_device_ca, and instead chained these 2 scripts inside Golang. The resulting implementation was tested in MEDS factory. To test, build the fioctl as this: ``` CGO_ENABLED=0 go build -ldflags "-X=github.com/foundriesio/fioctl/subcommands/version.Commit=v0.36-14-g2cd3815+dirty" -tags bashpki -o bin/fioctl-linux-amd64-bash main.go ``` After that, the following command creates PKI using Bash scripts: ``` bin/fioctl-linux-amd64-bash -c fioed-bash.yml keys ca create pki/fioed-bash --local-ca --online-ca ``` Maybe, later on we will decide to strip this. For now, I prefer to keep this as a backup solution. Signed-off-by: Volodymyr Khoroz <[email protected]>
This is the missing part which rendered the Bash-based PKI support inoperable. Now all the pieces seem to be fine. Signed-off-by: Volodymyr Khoroz <[email protected]>
This is a preparation for the HSM support: - Use crypto.PublicKey instead of any. - User crypto.Signer instead of *ecdsa.PrivateKey. Signed-off-by: Volodymyr Khoroz <[email protected]>
…module This makes it easier to introduce the HSM support Signed-off-by: Volodymyr Khoroz <[email protected]>
This allows to have different key storage implementations. This PR immediately adds the filesystem storage for Golang implementation. It also adds both filesystem and HSM storage for Bash implementation. The pkcs11-tool based implementation is dummy and will be added in the next commit. Signed-off-by: Volodymyr Khoroz <[email protected]>
This allows to decouple the PKCS11 vendor library loading from the Fioctl process. As a result, we can safely continue building static binaries. This fully replaces all features of our Bash based PKI implementation with the Golang native approach. Signed-off-by: Volodymyr Khoroz <[email protected]>
This requires dynamic library linking of Libc, hence, not compatible with our static releases. But, it was fairly simple to implement, so can be interesting for some users. Signed-off-by: Volodymyr Khoroz <[email protected]>
This fixes two problems: - A part of PKI files were created by x509 modules, and part by ca_create. This is not only non-uniform, but also makes writing tests harder. A small nuissance also goes from different error messages. - Because of the above, different files got different permissions. This was true for both Bash based on Golang based PKI implementation. This commit unifies file writing method: - It is the x509 package obligation to write files as necessary. - All PKI files receive read-only (0400) rights to protect them from unintended access. Signed-off-by: Volodymyr Khoroz <[email protected]>
The default value we provided for this parameter is kind of misleading. Furthermore, the user might forget to specify a correct label, resulting into misconfigured HSM module. It is safer to require a user to provide the token label explicitly. Signed-off-by: Volodymyr Khoroz <[email protected]>
These are the first tests for Fioctl, and not something we usually do. But, I was rather worried about switching from Bash based to Golang based PKI. Especially, given a fact that we need to also migrate the HSM support which was broken for ages. I also worry that it is way too easy to break a feature which is used so rarely, albeit being so important. So, I would prefer these tests to be run as a part of our CI, meaning that this code is being executed regularly. This first commit adds non-HSM tests. As you might notice, it helped me to find a couple of bugs in the Golang based PKI. It also helped verifying that the new Golang based PKI is identical to our legacy Bash based PKI. Finally, that both implementations work. Signed-off-by: Volodymyr Khoroz <[email protected]>
This test allowed to find few more fixes. It requires the following packages in order to run: - openssl, opensc, softhsm2, libengine-pkcs11-openssl It does all the necessary initialization and cleanup. Signed-off-by: Volodymyr Khoroz <[email protected]>
Make sure to verify all PKI implementations at CI time Signed-off-by: Volodymyr Khoroz <[email protected]>
98a7d6b
to
6badab6
Compare
Rebased on main to pull all linter/testing enhancements and cleaned the commit history. Will re-test this whole thing tomorrow. |
This is a small yet tempting improvement to make a smaller number of calls to the pkcs11-tool. It improved the test run time from 0.4 to 0.2 seconds (a negligible diff which simply makes me feel more professional). Signed-off-by: Volodymyr Khoroz <[email protected]>
LGTM. I think it's important to test happy path for all PKI usage use-cases. |
Signed-off-by: Volodymyr Khoroz <[email protected]>
@doanac @mike-sul please, give it a final cut. I tested the following happy-path scenaria:
Please, let me know if you have any other test scenario in mind. |
An idea is to continue building Fioctl statically, and provide HSM support by wrapping pkcs11-tool.
The tool is available for Linux, Windows, and Darwin; a user can also build from source.
The PR contains 3 implementations:
I find it reasonable to keep all 3 implementations for reference.