WIP: apps: Switch to using OSSL_STORE for loading keys, certs, ... by levitte · Pull Request #7390 · openssl/openssl

levitte · 2018-10-12T15:14:31Z

This makes all the format specifiers largely irrelevant, except for
the ENGINE format when a key is given as a key ID only. However, with
the engine loader, it's possible to change this:

-engine {engineid} -key {keyid} -keyformat ENGINE

To this:

-key engine://{engineid}/{keyid}

We retain the ENGINE format for the sake of compatibility. All other
input formats are simply ignored. Note that output formats is a
different story.

Fixes #1963

petrovr · 2018-10-14T13:42:46Z

Richard Levitte wrote:

This makes all the format specifiers largely irrelevant, except for the ENGINE format when a key is given as a key ID only. However, with the engine loader, it's possible to change this: -engine {engineid} -key {keyid} -keyformat ENGINE To this: -key engine://{engineid}/{keyid}

[SNIP] Proposed modification is not store specific. Let me quote in brief from PKIX-SSH manual ("http://securebox.termoneplus.com/man5/ssh_config.5.html") for option IdentityFile. (1) If file name starts with prefix ‘engine:’ instead from file identity load is redirected to “loadable cryptographic module” (engine). Name format is engine:{engineid}:{keyid} using syntax above. (2) Prefix “store:” could be used if cryptographic library supports ossl_store(7) functionality. In such case name format is store:{uri}. Engine if applicable is encoded in URI-scheme. Second case is more generic. It is store related as it allows use of URIs not supported by engine. For instance based on store functionality PKIX-SSH could use directly as identify key and certificates stored in PKCS#12 file (side effect). Without store user should unpack (extract) PKCS#12 file into text file (PEM format). Otherwise there is no big difference for PKIX-SSH: (1) ssh ... -i engine:e_nss:{keyid} vs (2) ssh ... -i store:nss:{keyid} Remark: in both cases engine require explicit configuration. And one note more: {engineid}/{keyid} is not _in store URI format._ You know engine , you know keyid, but you don't know how to build store URI. It is engine specific and is more one direction process: URI -> {{engineid}, {keyid}, {extras...}}. Regards, Roumen Petrov

levitte · 2018-10-14T16:44:26Z

Ah, there's a precedent for a engine: scheme. Sure, I can use that instead.

Why someone has implemented a store: scheme is beyond me, though. Does that mean that if I want to express a file name in uri form, I would have to write store:file:///home/levitte/foo.pem? Why isn't file:///home/levitte/foo.pem enough? I cannot see why we should support such a contraption...

petrovr · 2018-10-15T16:34:08Z

Richard Levitte wrote:

Ah, there's a précédent for a engine: scheme. Sure, I can use that instead. Why someone has implemented a store: scheme is beyond me, though. Does that mean that if I want to express a file name in uri form, I would have to write `store:file:///home/levitte/foo.pem`?

So I use prefix:data, whre prefix is engine or store just to avoid failback case into code. There is no reason OpenSSL to use such format.

Why isn't `file:///home/levitte/foo.pem` enough? I cannot see why we should support such a contraption...

Yep . Support of "Store URI" as input source should be enough . Perhaps if store fails with "invalid scheme" then code could try to open file (failback case). Drawback of "Store URI" is that engine may not be loaded. Work around is to keep "engine" argument. Regards, Roumen Petrov

levitte · 2018-10-15T16:44:07Z

Perhaps if store fails with "invalid scheme" then code could try to open file (failback case).

The current file loader does this already, i.e. it defaults to file: if no scheme is present.

levitte · 2018-10-15T16:45:12Z

Drawback of "Store URI" is that engine may not be loaded. Work around is to keep "engine" argument.

The proper workaround is to do what I do in this PR, i.e. define a engine: scheme loader. See apps/app_engine_loader.c

kaduk · 2018-10-15T22:17:53Z

Please do not start using an unregistered URI scheme: https://www.iana.org/assignments/uri-schemes/uri-schemes.xhtml

levitte · 2018-10-15T23:19:58Z

You make a good point. Earlier on, I was think that "nah, it's just private to the openssl app", and that doesn't quite fit the need to registration, and I quote from RFC 7595 section 4 "Guidelines for Provisional URI Scheme Registration":

'Provisional' registration can be used for schemes that are not part of any standard but that are intended for use (or observed to be in use) that is not limited to a private environment within a single organization.

But seeing that PKIX-SSH uses the same scheme, and uses a different syntax to boot, that might be qualification enough to make a provisional registration after all. (I don't see it ever changing status to something more ~~provisional~~permanent, btw)

levitte · 2018-10-17T09:48:14Z

Code updated to use engine:{engineid}:{keyid}

levitte · 2018-10-18T05:03:29Z

Apparently, I hadn't completed what I said. Now fixed, and tests added.

levitte · 2018-10-18T18:45:46Z

Finally, this builds and tests fine. I will fix the docs, then it will be time for a review

petrovr · 2018-10-19T18:33:32Z

Richard Levitte wrote:

Apparently, I hadn't completed what I said. Now fixed, and tests added.

Perhaps in next weeks :-( I could find time to continue discussion. Regards, Roumen

This capability existed internally, and is now made public.

The prompt includes the URI, to make it clear which object needs a pass phrase.

This involves exposing two pvkfmt.c functions, but only internally.

This helps load keys with the following scheme: engine:{engineid}:{keyid} This is legacy, but added for the time being to support keys given to the application like this: -engine {engineid} -key {keyid} -keyform ENGINE

This makes all the format specifiers largely irrelevant, except for the ENGINE format when a key is given as a key ID only. However, with the engine loader, it's possible to change this: -engine {engineid} -key {keyid} -keyformat ENGINE To this: -key engine:{engineid}:{keyid} We retain the ENGINE format for the sake of compatibility. All other input formats are simply ignored. Note that output formats is a different story. Fixes openssl#1963

This adds a bit of functionality in ossltest, so it can now be used to load PEM files. It takes the file name as key ID, but just to make sure faults aren't ignored, it requires all file names to be prefixed with 'ot:'.

…ned BIO

dwmw2 · 2019-04-25T06:23:51Z

I'll echo the "please don't use an unregistered URI scheme" comment with its converse: please do support PKCS#11 URIs directly, without them having to be prefixed or mangled. The {engine}:… form mentioned above would have worked for that, given that the name of the engine in question would be pkcs11.

If you end up with a form that doesn't Just Work for RFC7512 PKCS#11 URIs, please special-case them to use the pkcs11 engine. Thanks.

levitte · 2019-04-25T07:56:43Z

If you create a proper OSSL_STORE loader that registers the scheme "pkcs11:", then you can use that scheme directly.

The "engine:" 'scheme' is an interim construct to allow access via the older ENGINE calls to access keys; ENGINE_load_private_key and ENGINE_load_public_key. However, if the engine offers an OSSL_STORE loader, there is no reason whatsoever to use the "engine:" 'scheme'

dwmw2 · 2019-04-25T08:00:03Z

Perfect. Thank you.

mattcaswell · 2019-10-31T13:53:05Z

Needs a rebase

mattcaswell · 2019-11-04T14:53:43Z

But seeing that PKIX-SSH uses the same scheme, and uses a different syntax to boot, that might be qualification enough to make a provisional registration after all. (I don't see it ever changing status to something more provisionalpermanent, btw)

Was the scheme ever registered as suggested here?

levitte · 2019-11-04T16:04:09Z

Was the scheme ever registered as suggested here?

No, but can certainly be done. I have the impression that it's not very hard...

mattcaswell

I'd also suggest a CHANGES entry might be in order for the change in behaviour of "-inform"

mattcaswell · 2019-11-04T15:04:48Z

apps/app_engine_loader.c

+        char engineid[256];
+        size_t engineid_l = q - p;
+
+        strncpy(engineid, p, engineid_l);


Using strncpy like this seems incorrect. The point of using strncpy is to not exceed the target buffer size - but you instead pass the string length (which therefore won't copy the '\0') and then manually add \0. So this is likely to go wrong for very large engine URIs.

mattcaswell · 2019-11-04T15:06:06Z

apps/app_engine_loader.c

+        engineid[engineid_l] = '\0';
+        e = ENGINE_by_id(engineid);
+
+        keyid = OPENSSL_strdup(q + 1);


Check for NULL return?

mattcaswell · 2019-11-04T15:11:36Z

apps/app_engine_loader.c

+        || !OSSL_STORE_LOADER_set_error(loader, engine_error)
+        || !OSSL_STORE_LOADER_set_close(loader, engine_close)
+        || !OSSL_STORE_register_loader(loader)) {
+        OSSL_STORE_LOADER_free(loader);


I wonder if this loader might useful beyond the apps. Does it belong it libcrypto?

Considering we have declared ENGINE legacy, I dunno how keen I'm on that. But, we could re-evalutate that when I've finished #9389

mattcaswell · 2019-11-04T15:25:02Z

apps/apps.c

+        }
+
+        ok = load_cert_crl_http(uri, onecert, onecrl);
+        return ok;


Just return directly without the intermediate ok

mattcaswell · 2019-11-04T15:28:39Z

apps/apps.c

+        }
        unbuffer(stdin);
-        cert = dup_bio_in(format);
+        in = dup_bio_in(0);      /* Binary work best */


Should use an explicit macro format here...0 is actually FORMAT_UNDEF. The comment suggests you might have meant FORMAT_BINARY...although as far as I can tell with the dup_bio_in function either will do the same thing.

mattcaswell · 2019-11-04T15:44:11Z

apps/crl.c

 const OPTIONS crl_options[] = {
    {"help", OPT_HELP, '-', "Display this summary"},
-    {"inform", OPT_INFORM, 'F', "Input format; default PEM"},
+    {"inform", OPT_INFORM, 'F', "DEPRECATED AND IGNORED"},


Not sure there is a need to shout. This will draw attention to this particular option in any "-help" listing, which seems unwarranted. Same comment for other similar occurrences.

mattcaswell · 2019-11-04T16:02:37Z

crypto/store/loader_file.c

                                               int *matchcount,
                                               const UI_METHOD *ui_method,
-                                               void *ui_data);
+                                               void *ui_data, const char *uri);


The comment above here needs updating

mattcaswell · 2019-11-04T16:14:59Z

doc/man1/openssl.pod

+
+ -key engine:eng:something
+
+This is a temporary measure until engines have learned to provide an


I somehow suspect this won't be temporary and this wording will still be here in 10 years time. Perhaps reword?

mattcaswell · 2019-11-04T16:18:14Z

util/libcrypto.num

 EVP_CIPHER_fetch                        4748	3_0_0	EXIST::FUNCTION:
 EVP_CIPHER_mode                         4749	3_0_0	EXIST::FUNCTION:
+OSSL_STORE_attach                       4750	3_0_0	EXIST::FUNCTION:
+OSSL_STORE_LOADER_set_attach            4751	3_0_0	EXIST::FUNCTION:


These functions don't seem to be documented.

mattcaswell · 2019-11-04T16:21:35Z

test/recipes/90-test_store.t

+		ok(run(app(["openssl", "rsa", "-text", "-noout", @pubin,
+			    "-in", "engine:ossltest:ot:$file"])));
+		ok(run(app(["openssl", "rsa", "-text", "-noout", @pubin,
+			    "-in", "engine:ossltest:ot:".to_abs_file($file)])));


Given the extent of the option changes in the apps this seems very light testing

levitte · 2019-11-04T19:44:03Z

Needs a rebase

Getting at it now

levitte · 2019-11-04T21:36:10Z

The more I looked at the branch, the more I realised it's not quite ready for final review. So I put it back in WIP.

Comments are still welcome!

t-j-h · 2020-02-13T13:08:16Z

This is still a WIP but placing a hold so it doesn't end up in OpenSSL 3.0 without a discussion and decision on the points already raised in the feedback.

openssl-machine · 2020-03-15T00:08:08Z

Automated Ping: This issue is in a state where it requires action by @openssl/omc but the last update was 30 days ago

DDvO · 2020-10-09T10:12:37Z

This is related to #12647 and likely further more recent PRs..

t8m · 2021-07-22T15:42:00Z

Isn't this change already implemented in master/3.0?

levitte · 2021-07-22T15:44:57Z

Isn't this change already implemented in master/3.0?

Almost, there were nuances that are not caught by @DDvO's changes.
I've left this unchanged for a while for the sake of priority, but intended to rebase this when I've got a chunk of time, to see what there is left.

mattcaswell · 2022-01-19T08:27:37Z

OMC: There is no decision to be made here at this stage.

t8m · 2024-06-28T14:52:47Z

This is no longer relevant. The feature is already implemented.

levitte force-pushed the apps-switch-to-store branch from 45651b9 to cd85f3f Compare October 17, 2018 09:47

levitte added 6 commits April 21, 2019 11:53

OSSL_STORE: Make it possible to attach an OSSL_STORE to an opened BIO

db87a66

This capability existed internally, and is now made public.

OSSL_STORE: Better information when prompting for pass phrases

f5324fb

The prompt includes the URI, to make it clear which object needs a pass phrase.

OSSL_STORE: Make the 'file' scheme loader handle MSBLOB and PVK files

c3d9306

This involves exposing two pvkfmt.c functions, but only internally.

apps: Add OSSL_STORE loader for engine keys

262f681

This helps load keys with the following scheme: engine:{engineid}:{keyid} This is legacy, but added for the time being to support keys given to the application like this: -engine {engineid} -key {keyid} -keyform ENGINE

Add test to demonstrate the app's new engine key loading

34d8735

This adds a bit of functionality in ossltest, so it can now be used to load PEM files. It takes the file name as key ID, but just to make sure faults aren't ignored, it requires all file names to be prefixed with 'ot:'.

levitte force-pushed the apps-switch-to-store branch from c055e63 to 34d8735 Compare April 21, 2019 09:54

levitte mentioned this pull request Apr 22, 2019

WIP: OpenSSL PKCS#11 Engine #8200

Closed

2 tasks

levitte added 4 commits April 22, 2019 13:41

fixup! apps: Switch to using OSSL_STORE for loading keys, certs, ...

3775b34

fixup! apps: Switch to using OSSL_STORE for loading keys, certs, ...

5719a6a

fixup! OSSL_STORE: Make it possible to attach an OSSL_STORE to an ope…

cddfd17

…ned BIO

fixup! apps: Switch to using OSSL_STORE for loading keys, certs, ...

9e53b43

mattcaswell added the approval: review pending This pull request needs review by a committer label Oct 31, 2019

mattcaswell requested changes Nov 4, 2019

View reviewed changes

levitte changed the title ~~apps: Switch to using OSSL_STORE for loading keys, certs, ...~~ WIP: apps: Switch to using OSSL_STORE for loading keys, certs, ... Nov 4, 2019

levitte mentioned this pull request Nov 13, 2019

WIP: investigate whats missing to implement a pkcs11 provider #10265

Closed

levitte added branch: master Applies to master branch and removed approval: review pending This pull request needs review by a committer labels Jan 15, 2020

levitte mentioned this pull request Feb 13, 2020

apps x509: passing PKCS#11 URL as -signkey #11086

Closed

1 task

t-j-h added the hold: need omc decision label Feb 13, 2020

This was referenced May 6, 2020

Use OSSL_STORE for load_key() and load_pubkey() in apps/lib/apps.c #11745

Closed

Use OSSL_STORE in apps, generalizing and simplifying load_key() etc. #11755

Closed

OSSL_STORE additions #11756

Closed

levitte mentioned this pull request Aug 19, 2020

apps: make use of OSSL_STORE for generalized cert and CRLs loading #12647

Merged

2 tasks

levitte mentioned this pull request Nov 30, 2020

Allow org.openssl.engine: for each key input option (for apps supporting -engine) #13406

Closed

t8m added the triaged: feature The issue/pr requests/adds a feature label Jul 23, 2021

t8m added this to the Post 3.0.0 milestone Aug 9, 2021

mattcaswell removed the hold: need omc decision label Jan 19, 2022

t8m closed this Jun 28, 2024


		-key engine:eng:something

		This is a temporary measure until engines have learned to provide an

Uh oh!

Comments

Conversation

levitte commented Oct 12, 2018

Uh oh!

petrovr commented Oct 14, 2018 via email

Uh oh!

levitte commented Oct 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petrovr commented Oct 15, 2018 via email

Uh oh!

levitte commented Oct 15, 2018

Uh oh!

levitte commented Oct 15, 2018

Uh oh!

kaduk commented Oct 15, 2018

Uh oh!

levitte commented Oct 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

levitte commented Oct 17, 2018

Uh oh!

levitte commented Oct 18, 2018

Uh oh!

levitte commented Oct 18, 2018

Uh oh!

petrovr commented Oct 19, 2018 via email

Uh oh!

dwmw2 commented Apr 25, 2019

Uh oh!

levitte commented Apr 25, 2019

Uh oh!

dwmw2 commented Apr 25, 2019

Uh oh!

mattcaswell commented Oct 31, 2019

Uh oh!

mattcaswell commented Nov 4, 2019

Uh oh!

levitte commented Nov 4, 2019

Uh oh!

mattcaswell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

levitte commented Nov 4, 2019

Uh oh!

levitte commented Nov 4, 2019

Uh oh!

t-j-h commented Feb 13, 2020

Uh oh!

openssl-machine commented Mar 15, 2020

Uh oh!

DDvO commented Oct 9, 2020

Uh oh!

t8m commented Jul 22, 2021

Uh oh!

levitte commented Jul 22, 2021

Uh oh!

levitte commented Oct 14, 2018 •

edited

Loading

levitte commented Oct 15, 2018 •

edited

Loading