Add the STORE module take 2, for loaded certs, keys and more given a URI

[levitte]

Checklist

• documentation is added or updated
• tests are added or updated
• CLA is signed

Description of change

This is a redesign of #1962

This STORE module adds the following functionality:

• A function STORE_open(), STORE_load() and STORE_close() that accesses
  a URI and helps loading the supported objects (PKEYs, CERTs and CRLs
  for the moment) from it.
• An opaque type STORE_INFO that holds information on each loaded object.
• A few functions to retrieve desired data from a STORE_INFO reference.
• Functions to register and unregister loaders for different URI schemes. This enables dynamic addition of loaders from applications or from engines.

Also includes a loader for the "file" scheme. The goal is to have it load PEM files and raw DER files alike, transparently.

Fixes #1958, #1959

[levitte]

For the curious, I have a merge of the store2 branch (this PR) and the uri branch (#1961) being built here

[dwmw2]

This seems very much based on the concept that you can open a 'STORE', read it sequentially, and close it again. As such, it doesn't seem to apply to storage like PKCS#11. Is that something we care about?

[levitte]

This seems very much based on the concept that you can open a 'STORE', read it sequentially, and close it again. As such, it doesn't seem to apply to storage like PKCS#11. Is that something we care about?

For the file: scheme, you're entirely correct. That's the only scheme that's built in, for obvious reasons. However, the design is also to allow anyone to added their own loader for whatever sheme they see fit, and the only requirement is that this loader can simulate a sequential store of objects, given URI parameters. The backend data doesn't have to be organised in any specific way as long as the loader can simulate sequential semantics.

I haven't looked too much into the pkcs11: scheme, but I have the impression that a URI usually refers to exactly one object, so it should be fairly simple to create a loader that supports that.

[levitte]

Note: this branch now uses commits from #1961 and from #2027. If someone approves this one without looking at those, I'll consider it an approval of them as well.

[levitte]

I cleaned this up quite a bit, squashed what made sense to squash, and rebased on top of fresh master

[levitte]

I just did a huge rework of this PR.

Major new: public file scheme handlers are gone. There are a few left for the different kinds of data, but no more separate handler for each key type. This is instead handled through already existing OpenSSL functionality (most notably, EVP_PKEY_ASN1_METHODs).

[levitte]

5. The question of STORE_INFO_new_ENDOFDATA vs a ferror like function.

[dwmw2]

No, it's not a fundamental part of this API. This API is merely a user of whatever PBE APIs there are in other parts of OpenSSL (more specifically, it uses PKCS12_verify_mac and PKCS12_pbe_crypt).

In this context, that isn't a valid observation. The STORE API needs its documentation to stand alone and describe how to use it. Its use of other functions internally is purely an implementation detail.

Our UI library (which is what STORE uses to get passwords) deals with NUL-terminated char arrays, that should clarify your question. This means that we really only need to deal with the encoding (i.e. is this string UTF-8 or ISO-LATIN-{x}?).

What I'm saying is that a failure to clearly define the encoding is AS BAD AS not knowing if it's NUL-terminated chars or something else. For the new STORE API, please at least document precisely what is expected to be passed in.

And if you can't clearly specify it, with http://david.woodhou.se/draft-woodhouse-cert-best-practice.html in mind, then it's not really appropriate to pass the buck and say "but the PKCS12_* functions are broken already and it's not my fault". By simply using those and further entrenching their problems in new APIs, you are making the situation worse. Please don't do that.

We have an opportunity here to introduce a new API which applications can use to Do The Right Thing, without the hundreds of lines of code that I mentioned. Please please please let's not infest it with legacy behaviour and portability issues right from the start.

[levitte]

@dwmw2, could we at least investigate our current PBEs and what needs to be done with them?

Also, it sounds like you're asking me to reimplement our PBEs as part of this PR, and perhaps even make them part of the file scheme loader. That will not happen.

As for "The STORE API needs its documentation to stand alone and describe how to use it"... The STORE API uses UI_METHOD for password prompting.... are you saying that the UI_METHOD docs should be duplicated into the STORE docs?
In essence, the STORE API and library aren't standalone, they aren't designed that way.

[levitte]

Upon more carefull thought, the issue is even less part of the STORE API than I expressed. With regards to passphrases / passwords, the STORE API itself does nothing, by design. All it does is pass around a UI_METHOD, that the loaders can use as they please, and they can treat the result from the UI calls as they please as well.

So it would seem to me that your issue with passwords here belongs with the file scheme loader that's implemented as part of this PR.

Maybe this separation of stuff makes things clearer for you?

[dwmw2]

You don't need to duplicate the UI_METHOD docs. It's OK to refer to them, of course.

But if the UI_METHOD docs don't specify the character set, then you transitively inherit that same problem in your own code. And sure, you can pass the buck and say "the old code was already broken". But now you've added new APIs which are similarly broken, which is less OK.

And you have potentially made it worse, if we end up in a situation where one back end expects one thing (the locale charset) while others expect another (a PKCS#11 store probably really does require UTF-8 since that's specifiied by PKCS#11). So users of your new API now have to make guesses based on internal details and pass something different according to their guess.

The reason I'm pushing for this now, as a precursor to this landing, is because if we merge it with that kind of ambiguity then we end up with the API problem for ever. The API should be stable before it lands.

[levitte]

Ah, I thought I had already added references to the the UI / UI_METHOD docs. Apparently not. Will fix.

[dwmw2]

Still not good enough if the UI_METHOD docs don't specify charset handling, and if your overall resulting behaviour isn't correct given what is said about it.

[levitte]

As for the rest of what you say, @dwmw2, I repeat, let's fix the code where it's actually broken.

[dwmw2]

Sure, but typically one tries to fix such brokenness before propagating it and producing new ambiguous APIs. But as long as it gets done before an actual release with the STORE API, I suppose that's fine. We still shouldn't end up in a situation where applications have to jump through hoops to do different things for different back ends or different versions of OpenSSL.

[mattcaswell]

Based on the team discussion I consider (2) resolved (my objection about requiring different translation units can be fairly easily worked around). (4) is also resolved - OPENSSL_init_crypto() is required to ensure the atexit handlers get invoked properly.

[levitte]

So, remaining bigger issues:

1. STORE_ vs OSSL_STORE_ prefix (not yet concluded as far as I can see)
2. app name (store or storeutl)
3. ENDOFDATA vs ferror like interface.

I'm currently working on 3. 1 is waiting on team decision (I want it to become policy if we decide for OSSL_STORE_)... and quite frankly, 2 seems like a small thing, I would prefer "storeutl".

[levitte]

@dwmw2, regarding the passphrase issue, I'll have to insist on raising a separate issue for it, not in this PR. I'm pondering the whats and what nots right now.

Regarding what's done first or after, what about in parallell?

[richsalz]

I think @DWM2 makes a strong point. If the current UI is broken for non-ascii passwords, then adding a new API on top of that also seems broken.

[levitte]

You know, I figured that yes, some work needs to be done in the file scheme loader with regards to passwords. I suddenly remembered that the encoding issue was dealt with in the pkcs12 app, where a given pass phrase will be tried as is first when loading a file, and failing that, a pass phrases that's been converted from whatever encoding it's in to utf8 will be attempted instead. The same kind of operation should be attempted in the file scheme loader.

Why it should be done this way? Because there are existing keys that have been encrypted with iso-8859-* encoded pass phrases, and of course, there are also keys encrypted with utf8 encoded pass phrases.

In a similar vein, we cannot assume to know exactly what encoding each loader wants. That will be up to loader specific decisions and implementations. So for example, I would assume that a loader for PKCS#11 would make sure to convert the pass phrase it gets from UI calls to utf8. That should be a matter of what, one library call? Either OPENSSL_asc2utf8 or something similar from a more potent character conversion library.

UI, btw, will give you straight (or as straight as possible) what it was fed, with the current local encoding. It delivers NUL-terminated char *, and that's it.

[richsalz]

1. STORE_ vs OSSL_STORE_ prefix (not yet concluded as far as I can see)

I think doing a policy will be very difficult. The consensus seems to be that STORE is a generic name, and we shouldn't claim it. Can we just do with that one change? What's the policy, if not. "Don't use common names, otherwise prefix with OSSL" Our exported symbols have 73 items that start with openssl, and none that start with ossl: ; grep -i '^openssl' *num | wc 73 292 6010 ; grep -i ossl *num exit 1 ; I agree OPENSSL_STORE is big and ugly, but it *is* the convention we already have OPENSSL_LHASH _sk, _thread, etc.

2. app name (store or storeutl)

This is not a hill worth fighting over. Pick whatever name you want.

3. ENDOFDATA vs ferror like interface.

If it can look like BIO or FILE* with feof indicators, awesome. If not, so be it.

[mattcaswell]

2. app name (store or storeutl)

This is not a hill worth fighting over. Pick whatever name you want.

I prefer "store" (in my mind the utl bit doesn't add anything). But its just a preference so I will accept whatever you decide.

BTW, I am now struggling to load this PR in my browser. Github keeps throwing up a page saying it is taking too long to load.

[levitte]

@richsalz wrote thusly:

1. STORE_ vs OSSL_STORE_ prefix (not yet concluded as far as I can see)

I think doing a policy will be very difficult. The consensus seems to be that STORE is a generic name, and we shouldn't claim it.

X509 is also pretty generic (and we already have one clash). RSA as well. I could go on...

Can we just do with that one change? What's the policy, if not. "Don't use common names, otherwise prefix with OSSL" Our exported symbols have 73 items that start with openssl, and none that start with ossl:
; grep -i '^openssl' *num | wc
73 292 6010
; grep -i ossl *num
exit 1
;

I agree OPENSSL_STORE is big and ugly, but it is the convention we already have OPENSSL_LHASH _sk, _thread, etc.

Hmmm, hadn't thought of the OPENSSL_LHASH_ case... ok then.

2. app name (store or storeutl)

This is not a hill worth fighting over. Pick whatever name you want.

3. ENDOFDATA vs ferror like interface.

If it can look like BIO or FILE* with feof indicators, awesome. If not, so be it.

Well, we do have the end-of-file indicator already, but not the error indicator.

[levitte]

BTW, I am now struggling to load this PR in my browser. Github keeps throwing up a page saying it is taking too long to load.

Yup, I have the same issue. As far as I can tell, it's load related (on github's side)

[richsalz]

X509 is also pretty generic (and we already have one clash). RSA as well. I could go on...

And most, if not all, of these come from the SSLeay days. :) Things are different now, and we want to avoid naming conflicts if we can.

[kaduk]

In a similar vein, we cannot assume to know exactly what encoding each loader wants. That will be up to loader specific decisions and implementations. So for example, I would assume that a loader for PKCS#11 would make sure to convert the pass phrase it gets from UI calls to utf8. That should be a matter of what, one library call? Either OPENSSL_asc2utf8 or something similar from a more potent character conversion library.

I would probably try to reframe @dwmw2's position as "you should document exactly what format the input to the new APIs should be in, including charset/encoding/etc." (Unicode normalization form?) Ideally this would be a nice clean and consistent interface that is easy to describe, though the initial implementation might be accompanied by warnings of buggy behavior in some cases that does not match up with the clean API spec. A more "messy" API that has the encoding depend on which backend is in use might be possible to document accurately as well; I didn't look very hard. But consumers of the new functions should be able to have a very specific understanding and expectation for their behavior. If the encoding really is solely up to the UI_METHOD and that's all that STORE is passing around, then maybe we need to add some NIDs for various encodings and a way to query the UI_METHOD what is needed.

But to reiterate, I agree with @dwmw that character encoding is a giant minefield, and I think the best way we can help our consumers avoid issues is to clearly document how to correctly use our code; secondarily is to make correct usage simpler and more consistent.

[levitte]

Please move the pass phrase encoding discussion to #3531. I will refuse to answer these questions further here.

[dwmw2]

Please move the pass phrase encoding discussion to #3531.

Thank you for filing that.

I will refuse to answer these questions further here.

... except where they are directly related, I hope. Specifically:

• Please explicitly document the expectation implied by #3531, that the UI_METHOD passed in to STORE_open() is expected to provide strings in the locale charset, except where OPENSSL_WIN32_UTF8 is defined.

[mattcaswell]

Can I suggest closing this PR and reopening it with a different PR number? Github just cannot cope with loading this page now - I guess because there have been so many comments. Most times I get an error page :-(

[levitte]

Closing this PR, please redirect your attention to #3542