HSM 2: Persistent signer key mappings #686

ximon18 · 2021-10-12T09:15:38Z

This PR builds on #679 "KMIP walking skeleton". The purpose of this PR is to add support for remembering which signer backend possesses a given Krill KeyIdentifier and which internal HSM key identifier(s) correspond to a given Krill KeyIdentifier.

This PR has been marked ready for review because PRs that come later and depend on this PR have been marked ready for review. As it stands as an apparently suitable base for later PRs it is apparently usable to build upon and should thus be reviewed.

As noted in the comment below this PR moves files around which makes it harder to review. With the git defaults it shows files as added and deleted which have actually been moved and edited. Below I try to guide you through the changes to make it easier to review this PR.

Easy changes to review

.github/workflows/ci.yml: extends the HSM tests to test feature flag hsm in isolation, not just when used with the hsm-tests flag, just to make sure the build isn't broken.
Cargo.lock and Cargo.toml: bump the KMIP dependency to a newer version, and some ease of reading improvements were made to the order of feature flags and with some minor additional comments.
doc/development/hsm/xxx.md: files were updated. However, it might be worth skipping ahead to the versions in PKCS#11 walking skeleton #689 which have had considerably more work done on them.
src/commons/api/ca.rs: minor tweaks to the tests to pass additional arguments into OpenSslSigner::build() (a signer name used for logging/error reporting to distinguish reporting by one signer from another when running multiple concurrent signers, and an optional SignerMapper instance which is required when using more than one signer so that the owning signer for a key can be looked up, but in a test that uses only a single OpenSslSigner the SignerMapper can be None as the one signer is being used directly by the test, there is no signer dispatch involved).
signers/kmip/connpool.rs, signers/kmip/signer.rs, signers/util.rs: Unchanged files moved from src/commons/crypto/ to src/commons/crypto/signing/
src/commons/crypto/mod.rs, src/commons/error.rs: Minor updates related to moved modules.
signers/kmip/mod.rs and signers/kmip/keymap.rs: keymap.rs was deleted and removed from the module (as this PR replaces it with something else)
signers/errors.rs: Adds some new signer error variants. I'm not happy about the added "Other(String)", I think it was added for a case where there is no context to give a better error but it feels ... undesirable. Note that later PR PKCS#11 walking skeleton #689 renames the other two variants from SignerUnavailable -> TemporarilyUnavailable and from SignerUnusable -> PermanentlyUnusable.
src/constants.rs: Added a named constant for an existing hardcoded path and added a new constant for the new '<work_dir>/signers` directory that the new signer mapper mechanism will store signer and key mapping data in.

The new signer and key mapping mechanism

src/commons/crypto/signing/dispatch/signerinfo.rs adds a new event sourcing aggregate store for storing details about known signers and the keys that they own and the mapping for those keys from Krill KeyIdentifier to internal signer identifier(s).

It was based on the test aggregate store defined in src/commons/eventsourcing/mod.rs. There are a lot of structs but this is just how the event sourcing code in Krill requires it to be. In essence it's actually quite basic: there is a new high level struct SignerMapper that is used by the signing functionality to:

Record new signers and their identity details.
Record that a particular Krill key is owned by a given signer and corresponds to a particular signer specific internal key identifer.
Change the name and/or info of a particular signer.
Query the set of signers.
Query which signer owns a given Krill key.

The identity details for a signer consist of three parts:

A human readable name intended to be set by the operator in the config file which can be changed later if discovered to be incorrect or misleading or outdated.
Some arbitrary info string intended to be populated with details about the signer backend retrieved from the backend itself, e.g. its make and model and anything other interesting and/or identifying properties.
A public key.

The public key requires some more explanation.

In the HSM prototype code signers were mapped to KeyIdentifiers and signer backend internal key IDs in a simple CSV file structure. This added new storage engine code to Krill rather than re-using its existing storage abilities, code that would need testing and documenting and wouldn't benefit from any later extension to the core storage engine to for example work with a networked key value store instead of local disk files. The CSV file was indexed by signer name, so if the operator edited the name of the signer in the config file then Krill would no longer be able to locate which signer owned a given key.

This version of the code changes this in several ways:

Use the existing Krill storage engine instead of inventing a new one.
Record backend signer info and benefit from the store tracking those changes over time, e.g. you can later see that the signer backend was upgraded or changed in some way.
Detach the signer records from the operator given name and associate them with a unique handle instead so that the name can be changed.
Don't require signers to be online and usable when Krill starts up.
Do common checks on signer backends to verify that they work with Krill.
Determine which signer backend corresponds to which signer configuration without requiring the operator to keep names or signer order static or to manage an ID per signer.

These last twos point are where the public key comes in. We want anyway to know if a signer backend is actually capable of creating a signing key and signing data with it correctly and to do this we need to ask the backend to create a new key pair which we would then discard, assuming we didn't hit a problem trying to delete this temporary key.

Instead of discarding the created key we can remember the internal ID of the private key and save the public key. When we see the contact the signer backend on a subsequent restart of Krill we attempt to locate the signing key for each known signer. Internal IDs might not be unique, e.g. they could be integers that rise by one, so finding that the signer backend says it has a particular key isn't proof that it owns the key that we created in it earlier. So we ask the signer to sign a challenge using that private key and we verify the created signature using the stored public key. In this way we can determine if a particular signer backend is the one a subset of our signer store records relate to, i.e. that we think it owns certain keys that we created earlier.

With this the operator doesn't have to correctly manage signer names, IDs or order of definition, and we verify that the signer can create keys and sign with them. It also doesn't matter if the signer backend is upgraded or replaced entirely by a new one or if the means of connecting to it changes from KMIP to PKCS#11 or connection details or credentials change, if it has our keys we will be able to detect that. Admittedly we assume that not just the one identity key was migrated to a new signer or that other keys were not deleted by an operator, but that is always outside of our control.

commons/crypto/signing.rs: KrillSigner was moved and split out into commons/crypto/signing/dispatch/krillsigner.rs and other structs such as the SignerRouter and SignerProvider were created in new files in the same module.

PR PKCS#11 walking skeleton #689 greatly updates the developer docs/xxx.md files relating to these changes and is worth reading at this point.
commons/crypto/signing/dispatch/signerrouter.rs: Logic to discover signers and register them in the SignerMapper (defined in signerrouter.rs) and to dispatch requests to the signer that owns a key (by looking it up in the SignerMapper).
commons/crypto/signing/dispatch/signerprovider.rs: An enum based dispatch wrapper around multiple different signer implementations.
commons/crypto/signers/softsigner.rs: The OpenSSL soft signer was updated to support recording of keys that it creates in the SignerMapper and to implement new functions relating to creating the identity key and signing a challenge using it. Code to create a key was factored out into new function build_key() so that it could also be used to create the identity key.
commons/crypto/signers/kmip/internal.rs: Updated to support creation of the new identity key and signing a challenge and to use the new SignerMapper instead of the simple KeyMap placeholder in-memory mechanism.

…, and support in principle selecting which signer to use for which purpose. (#539)

- Add a dependency on the backoff crate for retry support. - Add a dependency on the r2d2 crate for connection pooling support. - Uses GitHub versions of the bcder and rpki crates for the DER Unsigned Integer support needed by the KMIP signer. - Refactor signers to crypto::signers and replace the Dummy signer with a KMIP signer. - Added a "hsmtest" job to the GitHub Actions CI workflow that runs all Krill tests using the KMIP signer against PyKMIP. - Added a "hsm-tests" Cargo feature flag for configuring Krill to use ONLY KMIP as a signer, not OpenSSL at all. Currently building without the "hsm-tests" feature flag set will fail if the "hsm" feature flag is set. Krill isn't ready to be used in "hsm" mode yet. - Changes SignerProvider to implement the Signer trait so that it can be passed to builders so that their invocation of a signer also goes via SignerProvider dispatching to the correct signer.

… the write lock while switching to using the server as part of finishing a successful probe.

…mplementation

… PyKMIP.

…er. Previously some Krill logic when invoked was given the same signer as handling the current purpose to invoke later even if for a different purpose. If the initial purpose required the KMIP signer as the key owning signer but the later purpose was one-off signing then that should be able to be routed if desired to the OpenSslSigner, for example. Introduces another layer of indirection: RouterSigner.

"router", not "dispatcher", and don't lock the entire SignerRouter for create/delete key operations.

…ner and from KeyIdentifier to Signer specific key id. Stores mapping using a new SignerInfo AggregateStore impl backed by a 'signers' subdirectory of the Krill data dir.. Krill can now be built with the `hsm` feature active without also requiring the `hsm-tests` feature to be active. Needs code cleanup and tests and docs.

… in HSM test usage mode (use the HSM as much as possible).

…r operation when using the PyKMIP signer instead of OpenSSL doesn't invoke refresh single too soon.

…asn't intended to securely guarantee that and other mechanisms for that exist such as TLS server certificate verification.

…der crypto::signing.

ximon18 · 2021-10-12T20:20:00Z

I'm moving some files of the crypto signing files around and splitting them up as I find it too hard to read and follow as it is now given the changes I've made in this branch. However, while Git is capable of detecting copies and renames it by default only detects them if more than 50% of the files has NOT changed. In the case of signing.rs I split it into several files and Git doesn't see this by default. GitHub seems to use similar or the same defaults for its PR UI and so suffers from the same problem.

On the command line Git can be told to use a different threshold. If I tell Git to detect copies and renames even for files for which only 1% of the content is unchanged the diff then shows this better:

git diff $(git merge-base --fork-point issue-566-implement-krill-kmip-based-signer-implementation) --stat -M1% -C1%
 .github/workflows/ci.yml                                                          |   5 +-
 doc/development/hsm/architecture.md                                               |  77 +++++++++--
 doc/development/hsm/overview.md                                                   |   4 +
 src/cli/options.rs                                                                |   2 +-
 src/commons/api/ca.rs                                                             |  12 +-
 src/commons/crypto/cert.rs                                                        |   2 +-
 src/commons/crypto/cms.rs                                                         |   4 +-
 src/commons/crypto/mod.rs                                                         |   2 -
 src/commons/crypto/{signing.rs => signing/dispatch/krillsigner.rs}                | 529 +++-------------------------------------------------------------------------
 src/commons/crypto/{signers => signing/dispatch}/mod.rs                           |  10 +-
 src/{daemon/ca/keys.rs => commons/crypto/signing/dispatch/signerinfo.rs}          | 826 +++++++++++++++++++++++++++++++++++++++++++++++-----------------------------------------------------------------------
 src/commons/crypto/{signers/kmip/signer.rs => signing/dispatch/signerprovider.rs} | 174 +++++++++++++++++--------
 src/commons/crypto/{signing.rs => signing/dispatch/signerrouter.rs}               | 956 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------------------------------------------------------------------------
 src/commons/crypto/{signing.rs => signing/misc.rs}                                | 367 ++---------------------------------------------------
 src/commons/crypto/{ => signing}/mod.rs                                           |  14 +-
 src/commons/crypto/{ => signing}/signers/error.rs                                 |   2 +
 src/commons/crypto/{ => signing}/signers/kmip/connpool.rs                         |   0
 src/commons/crypto/{ => signing}/signers/kmip/internal.rs                         | 151 +++++++++++++++-------
 src/commons/crypto/{ => signing}/signers/kmip/keymap.rs                           |   0
 src/commons/crypto/{ => signing}/signers/kmip/mod.rs                              |   0
 src/commons/crypto/{ => signing}/signers/kmip/signer.rs                           |   0
 src/commons/crypto/{ => signing}/signers/mod.rs                                   |   1 +
 src/commons/crypto/{ => signing}/signers/softsigner.rs                            | 173 +++++++++++++++++++++----
 src/commons/crypto/{ => signing}/signers/util.rs                                  |   0
 src/constants.rs                                                                  |   3 +
 src/daemon/ca/certauth.rs                                                         |   4 +-
 src/daemon/ca/commands.rs                                                         |   2 +-
 src/daemon/ca/events.rs                                                           |   2 +-
 src/daemon/ca/keys.rs                                                             |   2 +-
 src/daemon/ca/manager.rs                                                          |   2 +-
 src/daemon/ca/publishing.rs                                                       |   2 +-
 src/daemon/ca/rc.rs                                                               |   5 +-
 src/daemon/ca/routes.rs                                                           |   2 +-
 src/daemon/krillserver.rs                                                         |   2 +-
 src/pubd/events.rs                                                                |   2 +-
 src/pubd/manager.rs                                                               |   2 +-
 src/pubd/repository.rs                                                            |   2 +-
 src/test.rs                                                                       |   2 +-
 src/upgrades/mod.rs                                                               |   2 +-
 src/upgrades/v0_9_0/ca_objects_migration.rs                                       |   2 +-
 tests/suspend.rs                                                                  |   2 +-

Here we see for example that signing.rs was split out to multiple separate files while the GitHub PR UI shows signing.rs as having been deleted.

You can also see from the above diff that lots of files were barely touched and that's because it was just the minor updates required to import the dependent modules at their new paths.

If using GitLens: Compare References in VisualStudio Code to view the changes in the branch you can cause it to use an adjusted rename/copy threshold by setting the setting GitLens: Advanced: Similarity Threshold in the settings UI, e.g. set it to to 10 to detect more renames/copies than the default value used by Git which is 50.

…crate version for new ItemNotFound suberror. Make the signer registration public key non-optional. Remove no longer used get_handle() signer fn. Deduplicate signers added to the pending set. Improvements to the binding process: bind by same name first, then try other signer store public keys; detect fatal failures and abort testing ready signers; detect key not found separately to other KMIP errors; cleanup the logic; don't panic if KMIP signer doesn't yet have a signer handle; just unwrap() locks consist with other Krill code.

… suberror code.

…cessor PR #688 which is now redundant because those changes are now present in the 'hsm-persistent-signer-key-mappings' branch which this PR targets.