repart: support OpenSSL engines for signing #29539

bluca · 2023-10-11T18:30:03Z

This uses the deprecated ENGINE APIs, because there's no documentation anywhere on how to use the new one, and lots of engines haven't been converted anyway.

I tried to look for the 'providers' equivalent of the 'engine' interface, but the documentation is abysmal, and nobody seems to have bothered to update so far, so can't see any working examples.

DaanDeMeyer

Bit obtuse but found an example here: https://youtu.be/ph3QVgxUi98?t=272, why not give that a try? Seems easy enough

bluca · 2023-10-11T21:01:57Z

Seems easy enough

...is it fuck

I can't make it work, the pkcs11 provider doesn't want to work with my yubikey. I've added it, but with a fallback to the legacy API, which actually works. This stuff is probably too new for now.

man/systemd-repart.xml

src/partition/repart.c

man/systemd-repart.xml

src/partition/repart.c

poettering · 2023-10-12T08:18:25Z

So as it turns out, @simo5 is actually quite a nice guy. @simo5 any chance you can give us a hint how to use the openssl3 provider stuff correctly to get a yubikey to sign something via your pkcs11 provider? this PR is supposed to implement that, but @bluca ran into some trouble there.

(background: this is for systemd's systemd-repart tool which among other things can generate signed verity disk images, which have various uses, such as very likely in Fedora's future initrd logic, and more)

Any help appreciated.

DaanDeMeyer · 2023-10-12T08:20:24Z

@bluca Can you post some logs here on what fails with the provider stuff with your yubikey?

bluca · 2023-10-12T09:39:32Z

OSSL_STORE_load() fails with no error - ERR_error_string() just returns a helpful error:00000000:lib(0)::reason(0). I have built and installed https://github.com/latchset/pkcs11-provider.git from latest main.

The PKCS11 URI I am using, which works with the engine, is: pkcs11:model=PKCS%2315%20emulated;manufacturer=piv_II;serial=bf18f45eb430b744;token=Secure%20Boot%20Signing the yubikey is a 5 nano, with an RSA2048 cert/key in slot 9c

src/shared/openssl-util.c

simo5 · 2023-10-12T18:35:41Z

That error looks like an openssl engine error, not a provider error ?

bluca · 2023-10-12T18:38:08Z

The engine fallback is commented out, so it's definitely from openssl or the pkcs11 provider

bluca · 2023-10-12T18:38:50Z

This is the diff I'm testing with, on top of this PR:

--- a/src/shared/openssl-util.c
+++ b/src/shared/openssl-util.c
@@ -1146,7 +1146,7 @@ static int load_key_from_provider(
 
         _cleanup_(OSSL_STORE_closep) OSSL_STORE_CTX *store = OSSL_STORE_open(
                         private_key_uri,
-                        /* ui_method= */ NULL,
+                        /* ui_method= */ UI_OpenSSL(),
                         /* ui_data= */ NULL,
                         /* post_process= */ NULL,
                         /* post_process_data= */ NULL);
@@ -1214,8 +1214,8 @@ int openssl_load_key_from_provider(
         int r;
 
         r = load_key_from_provider(provider, private_key_uri, ret_private_key);
-        if (r < 0)
-                r = load_key_from_engine(provider, private_key_uri, ret_private_key);
+        // if (r < 0)
+        //         r = load_key_from_engine(provider, private_key_uri, ret_private_key);
 
         return r;
 }

simo5 · 2023-10-12T18:40:51Z

Btw is this code compiled in a binary?
Or is it potentially going to be called from a shared library?

If the latter you should really allocate a separate libctx otherwise you will interfere with the default context used also by the main application.

That said I wonder if the yubikey is forcing a re-authentication before allowing the sign operation.
Can you take a new debug file with the pacth you have on top so I can see what error is returned from the pkcs11 driver?

bluca · 2023-10-12T18:48:34Z

Yes, the yubikey wants the pin again when signing. With the engine, I get prompted twice for the pin - once when the engine is loaded, and once per signing operation. Debug log:

https://paste.ubuntu.com/p/S5H9YNDwzP/

simo5 · 2023-10-12T18:54:43Z

Yes, the yubikey wants the pin again when signing. With the engine, I get prompted twice for the pin - once when the engine is loaded, and once per signing operation. Debug log:

https://paste.ubuntu.com/p/S5H9YNDwzP/

Ok then sadly what you need is this issue resolved:
latchset/pkcs11-provider#42

It won't work until this is implemented I am afraid.

simo5 · 2023-10-12T18:59:15Z

[interface.gen.c:677] p11prov_Sign(): Error: 0x00000101; Error returned by C_Sign
^ this is:
#define CKR_USER_NOT_LOGGED_IN 0x00000101UL

Confirming my hypothesis.

I will check if we can expedite fixing this issue, now we should have enough infrastructure in the project to make it doable, it was not easy when 42 was filed.

bluca · 2023-10-12T19:01:31Z

Ok, thanks, then I'll go ahead and merge as-is, and the fallbacks will ensure things are usable. When those 2 code changes are in the provider let me know and I'll test again, and do any required follow-up. Thanks for the help.

simo5 · 2023-10-12T19:03:19Z

Note that there is no callback signature on signing operations, so I will need to figure out how to deal with the PIN, it may be required for you to set an option to allow the provider to cache pins, which will require a setting in openssl.cnf until this code: openssl/openssl#21604 is in a release and can be used to pass configuration at dynamic load time.

simo5 · 2023-10-12T19:05:07Z

Ok, thanks, then I'll go ahead and merge as-is, and the fallbacks will ensure things are usable. When those 2 code changes are in the provider let me know and I'll test again, and do any required follow-up. Thanks for the help.

If you subscribe to latchset/pkcs11-provider#42 you will get an automatic notification when we close it as implemented.

DaanDeMeyer · 2023-10-12T19:07:01Z

src/partition/repart.c

+                if (isempty(arg_signing_provider)) {
+                        _cleanup_(erase_and_freep) char *k = NULL;
+                        size_t n = 0;
+
+                        r = read_full_file_full(
+                                        AT_FDCWD, private_key, UINT64_MAX, SIZE_MAX,
+                                        READ_FULL_FILE_SECURE|READ_FULL_FILE_WARN_WORLD_READABLE|READ_FULL_FILE_CONNECT_SOCKET,
+                                        NULL,
+                                        &k, &n);
+                        if (r < 0)
+                                return log_error_errno(r, "Failed to read key file '%s': %m", private_key);
+
+                        r = parse_private_key(k, n, &arg_private_key);
+                        if (r < 0)
+                                return r;


Can't we always go through the provider codepath? It should support regular paths as uris just fine no?

Yes you can, the store api is fully generic and will load pem files just fine, you do not need special code if you use modern openssl 3 store apis, and that is the recommended way to deal with the pkcs11-provider, rather than having custom code.
Just rely on interested admins (or the distributions to do it for them) to configure the provider in the openssl.cnf and just provide a pkcs11 uri instead of a file name for the keys and all should just work transparently.

And what about --signing-provider in that case? Does it even make sense to have such an argument? Or is it sufficient to start interpreting the --private-key= argument which currently just takes a file path as a URI. Then if we need to fallback to the engine stuff maybe we can determine the engine to use from the uri as well?

Can't we always go through the provider codepath? It should support regular paths as uris just fine no?

eh not really, given providers don't really work at the moment

eh not really, given providers don't really work at the moment

Not with your token, but with files on disk without pins they should work fine I think

I think I've done enough experimenting for a while, I just want to be able to use my yubikey

@DaanDeMeyer as I said the default path without any HW toke invovled can be changed to use the Store api by default. And then everything will work automatically as the store api select the correct provider based on the URI it gets provived with, and fo course if a URI is not handled by any provider then it will throw an error.
going that way you will be able to use also other providers without code changes like the tpm2 provider or the oqs-provider etc...

Can't we always go through the provider codepath? It should support regular paths as uris just fine no?

Our loader is a bit fancy though, i.e. it will warn about world-readable access modes, and will connect to AF_UNIX sockets if you reference one. We try to make sure all our tools that read keys work that way. I doubt that openssl supports that (or wants to support that)

bluca · 2023-10-12T19:22:33Z

Note that there is no callback signature on signing operations, so I will need to figure out how to deal with the PIN, it may be required for you to set an option to allow the provider to cache pins, which will require a setting in openssl.cnf until this code: openssl/openssl#21604 is in a release and can be used to pass configuration at dynamic load time.

mmh but how is the pkcs11 engine doing it then? I get 2 prompts with the engine, there's no caching involved, I get a prompt when the engine is loaded, and a prompt whenever a signing operation is done. No caching at all, which seems the safe thing to do, not sure how comfortable I am at leaving the PIN around...

simo5 · 2023-10-12T19:24:39Z

Note that there is no callback signature on signing operations, so I will need to figure out how to deal with the PIN, it may be required for you to set an option to allow the provider to cache pins, which will require a setting in openssl.cnf until this code: openssl/openssl#21604 is in a release and can be used to pass configuration at dynamic load time.

mmh but how is the pkcs11 engine doing it then? I get 2 prompts with the engine, there's no caching involved, I get a prompt when the engine is loaded, and a prompt whenever a signing operation is done. No caching at all, which seems the safe thing to do, not sure how comfortable I am at leaving the PIN around...

I think it just uses the default prompt via UI_method_new(), but the problem here is not getting the PIN, pkcs11-provider does not support the custom login before the signature operation at all. This is something only some PIV tokens require, most HSMs or software tokens are ok with just the session login (which happens the first time you are asked for a pin).

bluca · 2023-10-12T19:26:09Z

Note that there is no callback signature on signing operations, so I will need to figure out how to deal with the PIN, it may be required for you to set an option to allow the provider to cache pins, which will require a setting in openssl.cnf until this code: openssl/openssl#21604 is in a release and can be used to pass configuration at dynamic load time.

mmh but how is the pkcs11 engine doing it then? I get 2 prompts with the engine, there's no caching involved, I get a prompt when the engine is loaded, and a prompt whenever a signing operation is done. No caching at all, which seems the safe thing to do, not sure how comfortable I am at leaving the PIN around...

I think it just uses the default prompt via UI_method_new(), but the problem here is not getting the PIN, pkcs11-provider does not support the custom login before the signature operation at all. This is something only some PIV tokens require, most HSMs or software tokens are ok with just the session login (which happens the first time you are asked for a pin).

Right, I guess for 'unattended' HSMs it makes sense, but for all yubikeys that I've had, the signing slot was always configured to require auth on each operation

DaanDeMeyer

I still think we're using the provider API wrong. We should only require users to give us a URI, not tell us to use a specific provider. That should be communicated via the URI.

Do we need the calls to OSSL_PROVIDER_try_load() at all? The way I understand it, you just need OSSL_STORE_open() when using the provider stuff.

So I would suggest getting rid of the --signing-provider= argument and just figuring out the engine to use from the given URI provided via --private-key= if the provider stuff fails. Probably just get the string until the first colon in the URI and that should be the engine to use. Maybe @simo5 can confirm this.

bluca · 2023-10-12T19:39:56Z

OSSL_PROVIDER_try_load is needed, otherwise you need some ugly and custom configuration changes in /etc/ to make the non-default providers work at all

simo5 · 2023-10-12T19:42:55Z

OSSL_PROVIDER_try_load is needed, otherwise you need some ugly and custom configuration changes in /etc/ to make the non-default providers work at all

This is a matter of perspective, most applications should be just fine, you change the conf once and then you are good with it. In fact we are considering dropping a config snippet in automatically when you install pkcs11-provider in fedora.

If the default code path gets changed to use the store api then -signing-provider would become optional.

bluca · 2023-10-12T19:49:22Z

Nah requiring global config to be changed with some oddly specific (and architecture specific) things is not a very good user experience. I would never ship that in Debian for example, given the path changes depending on the architecture, it would cause a ton of conflicts and break multi-arch. And then these get enabled globally, rather than per-application, which one might not even want.

simo5 · 2023-10-12T20:08:26Z

Nah requiring global config to be changed with some oddly specific (and architecture specific) things is not a very good user experience. I would never ship that in Debian for example, given the path changes depending on the architecture, it would cause a ton of conflicts and break multi-arch. And then these get enabled globally, rather than per-application, which one might not even want.

Sorry I do not understand the comment, I am not sure what there is that is architecture specific or odd here, but this is not the place to discuss distribution level changes anyway.
This code will work as is, we'll improve it as needed later.

bluca · 2023-10-12T20:12:22Z

Nah requiring global config to be changed with some oddly specific (and architecture specific) things is not a very good user experience. I would never ship that in Debian for example, given the path changes depending on the architecture, it would cause a ton of conflicts and break multi-arch. And then these get enabled globally, rather than per-application, which one might not even want.

Sorry I do not understand the comment, I am not sure what there is that is architecture specific or odd here, but this is not the place to discuss distribution level changes anyway. This code will work as is, we'll improve it as needed later.

The config files need an architecture-specific path on Debian/Ubuntu: module = /usr/lib/x86_64-linux-gnu/ossl-modules/pkcs11.so

simo5 · 2023-10-12T20:14:50Z

The config files need an architecture-specific path on Debian/Ubuntu: module = /usr/lib/x86_64-linux-gnu/ossl-modules/pkcs11.so

As far as I know no path is needed, openssl will search the pkcs11.so in the openssl default path like it does for the default.so and fips.so providers.
The path is optional in case you want to load a module that is not installed in the default path.
The pkcs11-provider configure is meant to make install in the default openssl designated path.

bluca · 2023-10-13T11:16:12Z

So can we merge this? I just wanted to use my yubikey...

DaanDeMeyer · 2023-10-13T11:44:40Z

I don't want us merging code that doesn't use the provider stuff like it's intended. This is just going to lead to bug reports and complaints down the road. Like @simo5 said, the module path should not be required in the config file in which case it should default to the p11-kit proxy.

So I'd like the following changes:

Get rid of the try_load() stuff and require the config file to be configured as intended by upstream. If there's something wrong with the config file we should work with upstream to get that fixed.
Get rid of --signing-provider= and derive the engine to use from the given URI. Maybe add --private-key-uri= alongside --private-key= so that we can accurately distinguish URIs from paths. Then the engine to use can be derived from the URI and we don't need --signing-provider= at all.

bluca · 2023-10-13T12:34:49Z

alright I'll just script around repart

poettering · 2023-10-13T12:59:07Z

@bluca hmm, any reason you deleted the branch? i still think something like this should go in, no reason to just delete the whole thing.

keszybz · 2024-03-22T11:48:11Z

A variant of this was merged as #31261.

bluca added the repart label Oct 11, 2023

bluca requested a review from DaanDeMeyer October 11, 2023 18:30

github-actions bot added documentation util-lib please-review PR is ready for (re-)review by a maintainer labels Oct 11, 2023

bluca force-pushed the repart_engine branch from 693c9e2 to bad2c47 Compare October 11, 2023 18:51

DaanDeMeyer reviewed Oct 11, 2023

View reviewed changes

bluca force-pushed the repart_engine branch from bad2c47 to afd0908 Compare October 11, 2023 21:00

bluca removed the documentation label Oct 11, 2023