Use OSSL_STORE in apps, generalizing and simplifying load_key() etc. by DDvO · Pull Request #11755 · openssl/openssl

DDvO · 2020-05-07T09:45:18Z

This is the continuation of #11745, a further spin-off of PR #11470 according to #11470 (comment).

This adds a flexible apps-internal function load_key_cert_crl() now used load_key(), load_pubkey(), and load_cert(), making use of OSSL_STORE.

This also adds the app-internal functions get_passwd(), cleanse(), and clear_free() to be used also in apps/cmp.c etc.

This PR also updates the help texts and the documentation of those apps using load-key() etc.

doc/man3/OSSL_STORE_open.pod

DDvO · 2020-05-07T11:33:47Z

Using OSSL_STORE for loading private keys in DER format gives a couple of strange failures of
make test TESTS=test_ec
at least for the following ED25519 Private-Key:

The loader reports on apps/openssl pkey -in pkey-f.d -inform der -text ambigous results with a matchcount of 10 and the following console output:

Unable to load key
C0:B7:87:51:D2:7F:00:00:error:rsa routines:(unknown function):RSA lib:crypto/rsa/rsa_ameth.c:142:
C0:B7:87:51:D2:7F:00:00:error:asn1 encoding routines:(unknown function):wrong tag:crypto/asn1/tasn_dec.c:1135:
C0:B7:87:51:D2:7F:00:00:error:asn1 encoding routines:(unknown function):nested asn1 error:crypto/asn1/tasn_dec.c:698:
C0:B7:87:51:D2:7F:00:00:error:asn1 encoding routines:(unknown function):nested asn1 error:crypto/asn1/tasn_dec.c:630:Field=params.p, Type=DSA
C0:B7:87:51:D2:7F:00:00:error:dsa routines:(unknown function):interrupted or cancelled:crypto/dsa/dsa_ameth.c:416:
C0:B7:87:51:D2:7F:00:00:error:asn1 encoding routines:(unknown function):wrong tag:crypto/asn1/tasn_dec.c:1135:
C0:B7:87:51:D2:7F:00:00:error:asn1 encoding routines:(unknown function):nested asn1 error:crypto/asn1/tasn_dec.c:698:
C0:B7:87:51:D2:7F:00:00:error:asn1 encoding routines:(unknown function):nested asn1 error:crypto/asn1/tasn_dec.c:630:Field=privateKey, Type=EC_PRIVATEKEY
C0:B7:87:51:D2:7F:00:00:error:elliptic curve routines:(unknown function):EC lib:crypto/ec/ec_asn1.c:991:
C0:B7:87:51:D2:7F:00:00:error:elliptic curve routines:(unknown function):decode error:crypto/ec/ec_ameth.c:455:
C0:B7:87:51:D2:7F:00:00:error:asn1 encoding routines:(unknown function):ASN1 lib:crypto/asn1/d2i_pr.c:62:
C0:B7:87:51:D2:7F:00:00:error:asn1 encoding routines:(unknown function):ASN1 lib:crypto/asn1/d2i_pr.c:62:
C0:B7:87:51:D2:7F:00:00:error:asn1 encoding routines:(unknown function):ASN1 lib:crypto/asn1/d2i_pr.c:62:
C0:B7:87:51:D2:7F:00:00:error:asn1 encoding routines:(unknown function):ASN1 lib:crypto/asn1/d2i_pr.c:62:
C0:B7:87:51:D2:7F:00:00:error:STORE routines:(unknown function):ambiguous content type:crypto/store/loader_file.c:1386:

How can this be fixed?

levitte · 2020-05-07T13:16:31Z

Oh, what a surprise! It turns out that d2i_PrivateKey (and thereby d2i_PrivateKey) doen't work as advertised for PKCS#8-wrapped keys! The manual says this (italics mine, for emphasis):

d2i_PrivateKey() decodes a private key using algorithm type. It
attempts to use any key specific format or PKCS#8 unencrypted
PrivateKeyInfo format. The type parameter should be a public key
algorithm constant such as EVP_PKEY_RSA. An error occurs if the decoded
key does not match type. d2i_PublicKey() does the same for public
keys.

However, looking at actual code, you'll notice that when it has decoded a PKCS#8 structure, *it doesn't care about type at all!

openssl/crypto/asn1/d2i_pr.c

Lines 46 to 65 in 90fc2c2

    
           if (!ret->ameth->old_priv_decode || 
        
               !ret->ameth->old_priv_decode(ret, &p, length)) { 
        
               if (ret->ameth->priv_decode != NULL 
        
                       || ret->ameth->priv_decode_with_libctx != NULL) { 
        
                   EVP_PKEY *tmp; 
        
                   PKCS8_PRIV_KEY_INFO *p8 = NULL; 
        
                   p8 = d2i_PKCS8_PRIV_KEY_INFO(NULL, &p, length); 
        
                   if (p8 == NULL) 
        
                       goto err; 
        
                   tmp = evp_pkcs82pkey_int(p8, libctx, propq); 
        
                   PKCS8_PRIV_KEY_INFO_free(p8); 
        
                   if (tmp == NULL) 
        
                       goto err; 
        
                   EVP_PKEY_free(ret); 
        
                   ret = tmp; 
        
               } else { 
        
                   ASN1err(0, ERR_R_ASN1_LIB); 
        
                   goto err; 
        
               } 
        
           }

Of course, the file loader is implemented according to documentation 😉

levitte · 2020-05-07T13:20:37Z

Try and see what happens if you add this patch:

diff --git a/crypto/asn1/d2i_pr.c b/crypto/asn1/d2i_pr.c
index c7346f5424..3ddc56d408 100644
--- a/crypto/asn1/d2i_pr.c
+++ b/crypto/asn1/d2i_pr.c
@@ -58,6 +58,8 @@ EVP_PKEY *d2i_PrivateKey_ex(int type, EVP_PKEY **a, const unsigned char **pp,
                 goto err;
             EVP_PKEY_free(ret);
             ret = tmp;
+            if (EVP_PKEY_type(type) != EVP_PKEY_base_id(ret))
+                goto err;
         } else {
             ASN1err(0, ERR_R_ASN1_LIB);
             goto err;

DDvO · 2020-05-07T17:52:41Z

Try and see what happens if you add this patch:

Great, this fixes the test failures - thanks @levitte

apps/ca.c

DDvO · 2020-05-08T12:57:13Z

@levitte, hope you can finish up #11756 providing OSSL_STORE_attach() soon
such that this PR can be finalized and merged also in #11470.

crypto/asn1/d2i_pr.c

crypto/store/loader_file.c

doc/man1/openssl-ca.pod.in

doc/man3/OSSL_STORE_open.pod

apps/include/opt.h

apps/lib/apps.c

DDvO · 2020-05-09T18:31:47Z

This PR meanwhile reflects also the extended load_key() and load_pubkey() features in the apps and their documentation.

@FdaSilvaYY, thanks for your yesterday's review. Maybe you wanna have a look also at this addendum.

…penssl#11755)

DDvO · 2020-05-12T12:55:08Z

@levitte, if you can approve this today we should be able to get this along with #11756 into alpha2.
It would also simplify #11470.

DDvO · 2020-05-12T13:58:34Z

I've partly squashed the commits and
simplified the TODOs to be executed when rebasing on #11756.

DDvO · 2020-05-12T15:11:07Z

I again caught the unrelated Travis failure "No space left on device",
which is annoying. Looks like the machine needs some disk cleanup.

levitte · 2020-05-13T09:40:05Z

Unfortunately, I was not well yesterday, and won't be able to look before this evening today. Considering that #11756 has only been approved today, time is running very short anyway.
I will look this evening.

levitte · 2020-05-13T09:43:02Z

That being said, we've internally decided to make fairly frequent alphas, with about three weeks' interval between them, so the wait for alpha3 isn't going to be huge. I think it's better to iron out whatever there is to iron out than to rush it in for alpha2. Do you agree?

DDvO · 2020-05-13T14:15:39Z

Thanks @levitte for your comments on the timing of this PR.

I agree that it's not a problem if it does not make it into alpha2, mostly because it's not strictly needed anymore for #11470, which I'll be able to merge in about two hours.

levitte · 2020-05-14T07:49:34Z

I like your approach very much. Isn't it worth having this API not on the apps level but on the level of the libcrypto?

Please no.

beldmit · 2020-05-14T07:58:41Z

I like your approach very much. Isn't it worth having this API not on the apps level but on the level of the libcrypto?

Please no.

It just causes the cut'n'pasting these functions when needed :(

DDvO · 2020-05-14T07:59:33Z

I like your approach very much. Isn't it worth having this API not on the apps level but on the level of the libcrypto?

Please no.

What would make sense IHMO is a slight abstraction (without the stdin feature, and more general password input) of

int load_key_cert_crl(const char *uri, int maybe_stdin,
                      const char *pass, const char *desc,
                      EVP_PKEY **ppkey, X509 **pcert, X509_CRL **pcrl)

for convenient library-level use of the OSSL_STORE API, for instance:

int OSSL_STORE_load_key_cert_crl(const char *uri, BIO *bio,
                                 const UI_METHOD *ui_method, void *ui_data, const char *desc,
                                 EVP_PKEY **ppkey, X509 **pcert, X509_CRL **pcrl)

where either the uri or the bio would be used as input.

DDvO · 2020-05-14T08:10:18Z

I like your approach very much. Isn't it worth having this API not on the apps level but on the level of the libcrypto?

Please no.

It just causes the cut'n'pasting these functions when needed :(

Indeed. Any why not even more directly usable

EVP_PKEY *OSSL_STORE_load_key(const char *uri, BIO *bio, const char *desc,
                              const UI_METHOD *ui_method, void *ui_data);
X509 *OSSL_STORE_load_cert(const char *uri, BIO *bio, const char *desc,
                           const UI_METHOD *ui_method, void *ui_data);
X509_CRL *OSSL_STORE_load_crl(const char *uri, BIO *bio, const char *desc);

DDvO · 2020-05-14T08:17:07Z

Then we can also obsolete/replace/integrate

X509 *X509_load_http(const char *url, BIO *bio, BIO *rbio, int timeout);
X509_CRL *X509_CRL_load_http(const char *url, BIO *bio, BIO *rbio, int timeout);

DDvO · 2020-05-14T08:19:12Z

BTW, is it possible with OSSL_STORE to set a timeout value in case HTTP-based fetching is used?

levitte · 2020-05-14T08:46:18Z

BTW, is it possible with OSSL_STORE to set a timeout value in case HTTP-based fetching is used?

There is a ctrl function, isn't there?

levitte · 2020-05-14T08:47:29Z

I really don't think that application specific routines belong in libcrypto. If you absolutely want to make them into a public library, it should be a separate one.

beldmit · 2020-05-14T08:54:56Z

I really don't think that application specific routines belong in libcrypto. If you absolutely want to make them into a public library, it should be a separate one.

I agree with this position.

DDvO · 2020-05-15T12:56:07Z

I really don't think that application specific routines belong in libcrypto.
If you absolutely want to make them into a public library, it should be a separate one.

This brings us back to a discussion started pretty exactly four years back: #6986.

In my view also the OSSL_STORE would better be placed in such an application-level lib.
And of course various other rather high-level features that do not really belong to libcrypto,
such as S/MIME, TS, HTTP, OCSP, and CMP.

openssl-machine · 2020-05-15T13:00:24Z

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

levitte · 2020-05-15T13:13:40Z

In my view also the OSSL_STORE would better be placed in such an application-level lib.

I disagree, especially in light if it being used internally in a few places in libcrypto, but also because it's a central ENGINE (and soon provider) interface.

And of course various other rather high-level features that do not really belong to libcrypto,
such as S/MIME, TS, HTTP, OCSP, and CMP.

I agree completely! There's a lot that's been shoved into libcrypto that's more like protocol libraries (interestingly, we have a separate libssl, go figure!) or application utility libraries.

DDvO · 2020-05-15T13:26:09Z

In my view also the OSSL_STORE would better be placed in such an application-level lib.

I disagree, especially in light if it being used internally in a few places in libcrypto, but also because it's a central ENGINE (and soon provider) interface.

Given this insight I agree.

…ib/apps.c

…warning

This also adds the more flexible and general load_key_cert_crl() as well as helper functions get_passwd(), cleanse(), and clear_free() to be used also in apps/cmp.c etc.

…ib/apps.c Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: David von Oheimb <[email protected]> (Merged from #11755)

…warning Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: David von Oheimb <[email protected]> (Merged from #11755)

This also adds the more flexible and general load_key_cert_crl() as well as helper functions get_passwd(), cleanse(), and clear_free() to be used also in apps/cmp.c etc. Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: David von Oheimb <[email protected]> (Merged from #11755)

DDvO · 2020-05-15T18:22:43Z

Pushed - thanks!

DDvO mentioned this pull request May 7, 2020

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

Closed

levitte reviewed May 7, 2020

View reviewed changes

doc/man3/OSSL_STORE_open.pod Outdated Show resolved Hide resolved

levitte mentioned this pull request May 7, 2020

OSSL_STORE additions #11756

Closed

DDvO force-pushed the use_OSSL_STORE_in_apps branch from 477e358 to 2a16046 Compare May 7, 2020 20:08

DDvO changed the title ~~Use OSSL_STORE in apps, generalizing and simplifying load_key() etc.~~ WIP: Use OSSL_STORE in apps, generalizing and simplifying load_key() etc. May 7, 2020

DDvO added this to the 3.0.0 milestone May 7, 2020

slontis reviewed May 7, 2020

View reviewed changes

apps/ca.c Outdated Show resolved Hide resolved

DDvO force-pushed the use_OSSL_STORE_in_apps branch 3 times, most recently from 8258354 to 16b6afa Compare May 8, 2020 10:34

DDvO mentioned this pull request May 8, 2020

Chunk 11 of CMP contribution to OpenSSL: CMP command-line interface #11470

Closed

FdaSilvaYY reviewed May 8, 2020

View reviewed changes

DDvO force-pushed the use_OSSL_STORE_in_apps branch from 16b6afa to 50c3db5 Compare May 9, 2020 16:52

DDvO changed the title ~~WIP: Use OSSL_STORE in apps, generalizing and simplifying load_key() etc.~~ Use OSSL_STORE in apps, generalizing and simplifying load_key() etc. May 9, 2020

levitte mentioned this pull request May 11, 2020

Fix d2i_PrivateKey_ex() to work as documented #11787

Closed

DDvO added a commit to mpeylo/cmpossl that referenced this pull request May 12, 2020

carve out dependency on creds loading improvements (PR openssl#4930, o…

a5ec2b4

…penssl#11755)

DDvO force-pushed the use_OSSL_STORE_in_apps branch from 43b7df1 to 9b0a075 Compare May 12, 2020 13:22

DDvO force-pushed the use_OSSL_STORE_in_apps branch from 9b0a075 to 0c672fe Compare May 12, 2020 14:55

levitte approved these changes May 14, 2020

View reviewed changes

levitte added approval: done This pull request has the required number of approvals branch: master Applies to master branch labels May 14, 2020

DDvO mentioned this pull request May 15, 2020

Consider a proper application-level library #6986

Closed

DDvO force-pushed the use_OSSL_STORE_in_apps branch from 11b7bad to cf2e871 Compare May 15, 2020 18:03

DDvO added 3 commits May 15, 2020 20:20

Guard use of struct tms with #ifdef __TMS like done earlier in apps/l…

e6c2d9d

…ib/apps.c

Nit-fix: remove whitespace in doc/man3/EVP_PKEY_fromdata.pod causing …

12e8865

…warning

Use OSSL_STORE for load_{,pub}key() and load_cert() in apps/lib/apps.c

a2847b1

This also adds the more flexible and general load_key_cert_crl() as well as helper functions get_passwd(), cleanse(), and clear_free() to be used also in apps/cmp.c etc.

DDvO force-pushed the use_OSSL_STORE_in_apps branch from cf2e871 to a2847b1 Compare May 15, 2020 18:20

openssl-machine pushed a commit that referenced this pull request May 15, 2020

Guard use of struct tms with #ifdef __TMS like done earlier in apps/l…

db71d31

…ib/apps.c Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: David von Oheimb <[email protected]> (Merged from #11755)

openssl-machine pushed a commit that referenced this pull request May 15, 2020

Nit-fix: remove whitespace in doc/man3/EVP_PKEY_fromdata.pod causing …

60d5331

…warning Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: David von Oheimb <[email protected]> (Merged from #11755)

DDvO closed this May 15, 2020

dcooper16 mentioned this pull request May 19, 2020

x509 can't read from stdin #11871

Closed

romen mentioned this pull request May 22, 2020

Incorrect speed output after #11755 #11922

Closed

DDvO mentioned this pull request May 25, 2020

90% drop in aes performance since commit db71d315479762eefbf2bcda8be3b44b1867133f #11903

Closed

Uh oh!

Comments

Conversation

DDvO commented May 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

DDvO commented May 7, 2020

Uh oh!

levitte commented May 7, 2020

Uh oh!

levitte commented May 7, 2020

Uh oh!

DDvO commented May 7, 2020

Uh oh!

Uh oh!

DDvO commented May 8, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DDvO commented May 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DDvO commented May 12, 2020

Uh oh!

DDvO commented May 12, 2020

Uh oh!

DDvO commented May 12, 2020

Uh oh!

levitte commented May 13, 2020

Uh oh!

levitte commented May 13, 2020

Uh oh!

DDvO commented May 13, 2020

Uh oh!

levitte commented May 14, 2020

Uh oh!

beldmit commented May 14, 2020

Uh oh!

DDvO commented May 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DDvO commented May 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DDvO commented May 14, 2020

Uh oh!

DDvO commented May 14, 2020

Uh oh!

levitte commented May 14, 2020

Uh oh!

levitte commented May 14, 2020

Uh oh!

beldmit commented May 14, 2020

Uh oh!

DDvO commented May 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openssl-machine commented May 15, 2020

Uh oh!

levitte commented May 15, 2020

Uh oh!

DDvO commented May 15, 2020

Uh oh!

DDvO commented May 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

DDvO commented May 7, 2020 •

edited

Loading

DDvO commented May 9, 2020 •

edited

Loading

DDvO commented May 14, 2020 •

edited

Loading

DDvO commented May 14, 2020 •

edited

Loading

DDvO commented May 15, 2020 •

edited

Loading