On Mon, 2021-08-09 at 11:10 -0400, Stefan Berger wrote:
> From: Stefan Berger <stefanb@xxxxxxxxxxxxx>
>
> Add support for pkcs11 private keys for signing a v2 hash.
>
> Signed-off-by: Stefan Berger <stefanb@xxxxxxxxxxxxx>
> ---
> README | 1 +
> src/evmctl.c | 1 +
> src/libimaevm.c | 59 ++++++++++++++++++++++++++++++++++++++++-------
> --
> 3 files changed, 50 insertions(+), 11 deletions(-)
>
> diff --git a/README b/README
> index 1cc027f..2bb363c 100644
> --- a/README
> +++ b/README
> @@ -48,6 +48,7 @@ OPTIONS
> --xattr-user store xattrs in user namespace (for testing
> purposes)
> --rsa use RSA key type and signing scheme v1
> -k, --key path to signing key (default:
> /etc/keys/{privkey,pubkey}_evm.pem)
> + or a pkcs11 URI
> --keyid n overwrite signature keyid with a 32-bit value
> in hex (for signing)
> --keyid-from-cert file
> read keyid value from SKID of a x509 cert file
> diff --git a/src/evmctl.c b/src/evmctl.c
> index 58f8e66..2e85f8b 100644
> --- a/src/evmctl.c
> +++ b/src/evmctl.c
> @@ -2503,6 +2503,7 @@ static void usage(void)
> " --xattr-user store xattrs in user namespace
> (for testing purposes)\n"
> " --rsa use RSA key type and signing
> scheme v1\n"
> " -k, --key path to signing key (default:
> /etc/keys/{privkey,pubkey}_evm.pem)\n"
> + " or a pkcs11 URI\n"
> " --keyid n overwrite signature keyid with a
> 32-bit value in hex (for signing)\n"
> " --keyid-from-cert file\n"
> " read keyid value from SKID of a
> x509 cert file\n"
> diff --git a/src/libimaevm.c b/src/libimaevm.c
> index 8e96157..b84e5b8 100644
> --- a/src/libimaevm.c
> +++ b/src/libimaevm.c
> @@ -60,6 +60,7 @@
> #include <openssl/x509.h>
> #include <openssl/x509v3.h>
> #include <openssl/err.h>
> +#include <openssl/engine.h>
>
> #include "imaevm.h"
> #include "hash_info.h"
> @@ -803,21 +804,57 @@ static EVP_PKEY *read_priv_pkey(const char
> *keyfile, const char *keypass)
> {
> FILE *fp;
> EVP_PKEY *pkey;
> + ENGINE *e;
>
> - fp = fopen(keyfile, "r");
> - if (!fp) {
> - log_err("Failed to open keyfile: %s\n", keyfile);
> - return NULL;
> - }
> - pkey = PEM_read_PrivateKey(fp, NULL, NULL, (void *)keypass);
> - if (!pkey) {
> - log_err("Failed to PEM_read_PrivateKey key file: %s\n",
> - keyfile);
> - output_openssl_errors();
> + if (!strncmp(keyfile, "pkcs11:", 7)) {
> + if (!imaevm_params.keyid) {
> + log_err("When using a pkcs11 URI you must
> provide the keyid with an option\n");
> + return NULL;
> + }
> +
> + ENGINE_load_builtin_engines();
> + e = ENGINE_by_id("pkcs11");
> + if (!e) {
> + log_err("Failed to load pkcs11 engine\n");
> + goto err_pkcs11;
> + }
> + if (!ENGINE_init(e)) {
> + log_err("Failed to initialize the pkcs11
> engine\n");
> + goto err_pkcs11;
> + }
> + if (keypass) {
> + if (!ENGINE_ctrl_cmd_string(e, "PIN", keypass,
> 0)) {
> + log_err("Failed to set the PIN for the
> private key\n");
> + goto err_pkcs11;
> + }
> + }
> + pkey = ENGINE_load_private_key(e, keyfile, NULL, NULL);
> + if (!pkey) {
> + log_err("Failed to load private key %s\n",
> keyfile);
> + goto err_pkcs11;
> + }
> + } else {
> + fp = fopen(keyfile, "r");
> + if (!fp) {
> + log_err("Failed to open keyfile: %s\n",
> keyfile);
> + return NULL;
> + }
> + pkey = PEM_read_PrivateKey(fp, NULL, NULL, (void
> *)keypass);
> + if (!pkey) {
> + log_err("Failed to PEM_read_PrivateKey key
> file: %s\n",
> + keyfile);
> + output_openssl_errors();
> + }
> +
> + fclose(fp);
This looks a bit narrow. Isn't the problem that *any* engine key might
be a specifier not a file? In which case the generic fix is not to
validate file existence if an engine is present on the command line.
That way you can specify any engine key and the pkcs11 key simply
becomes a special case of this.
If you insist on not having to specify --engine for pkcs11 keys then
you do the prefix check earlier on and set the engine to pkcs11 (if
it's not already set to something else).
James
