Add the STORE module take 2, for loaded certs, keys and more given a URI (cont) #3542
Conversation
|
Copying remaining big issues from #2011:
I'm currently working on 2. 1 is waiting on team decision (I want it to become policy if we decide for |
|
We spoke of the desire to keep your test cases lifted from OpenConnect, in sync with the ones I have. So I thought I should let you know I added a new one that you should incorporate: |
|
@dwmw2, hmmm, that will take some work to adapt, and especially since I try to be platform agnostic... |
|
Of the listed things to fix, I realised I forgot to add one, @t-j-h's concern with opening up internal APIs, so I'll add that here:
I feel unsure about (3)... exposing things internally within libcrypto is already done for other things, I don't quite see why this one is a bad thing. @t-j-h, are you very attached to an unreachable |
levitte
force-pushed
the
store2-continued
branch
2 times, most recently
from
29d5e71 to
ff95ce4
Compare
The test case itself, yeah. I need to find a Windows box (suspect Wine won't cut it here) and work that out. But having an expectation that it should work... that's a different story (qv). |
|
Just attempting to work out what this looks like for PKCS#11... Given a RFC7512 URI string (which is actually a search term that may produce more than result), we call I think that works sanely, and fits the same usage model as when there is more than one object within a given file. Will we eventually have a The only thing I think I'm missing is explicit documentation of the lifetime of the |
|
@dwmw2 wrote:
Yes. Basically, it's roughly modeled from stdio, with the difference that we start with a URI in place of a file name.
Obviously, you haven't seen #2696 and #2697... They're pretty rad, especially #2697.
I would expect I would expect that any condition specific to an engine will be documented with said engine |
|
(previous comment removed... I can't believe I wrote that out loud...) |
|
So... If a pkcs11 STORE backend might exist one day and if my users might one day give me a string which happens to reference an object therein, as the cert they want to use... Then I need to have read that documentation before I write my application today? I would be much happier with lifetime rules for the ui_data warning up front that some back ends may keep references to it for as long as keys exist. Those other PRs... Neat. Need to think about how we handle that for system trust, especially w.r.t. |
|
Do you mean the |
Well, it would seem appropriate to read docs that come with the backends you're about to use, if there is any, no? I don't understand why that would be questionable... I find it as natural as having to read
Mm, good point. I think I can write something generic enough. I'll get back to that on Monday. |
|
Yes I'm thinking of that |
|
For the other thing... I write my application to use the STORE interface long before the back end in question even exists. A sysadmin installs the PKCS#11 back end once it's written, and a user chooses a cert with a PKCS#11 URI. It wasn't "me" who used that back end. |
|
Ah, I get your point. The normal thing, at least from the usual OpenSSL perspective, would be to have the backend duplicate the UI data... which is, of course, difficult with the current UI API. But then, I could easily add something that would do exactly that. I would think that's a saner way of doing things. |
|
"duplicate the ui_data"? This is an opaque (to OpenSSL) datum which ends up being passed back to the application in order to request the password. It contains GUI context or something, and may have a chain of other refcounted objects hanging off it that OpenSSL can know nothing about.... |
|
See #3575. |
|
My thought is that if the ui_data can't be duplicated (regardless of reason), then your backend can't support things like |
|
Ack, that works. Thanks. A nice obvious failure mode if an app forgets to provide a dup method -- even today with just the file backend where it isn't strictly needed -- is extra helpful. |
|
I'm starting to wonder if a callback function to get the password might be better than a UI. People build without UI support (e.g. MacOS) and do we want it to be a dependency? The "user" prompts we offer are never going to be perfectly suited for presenting directly to the user anyway (with translations and referring to the object being unlockedj with the terminology that the app+user use for it. Using a callback also lets us punt on the charset question and insist we only take UTF-8. Because now there is app code between the STORE and the We also get to convey other flags like "wrong PIN" and "last chance" with our own callback, in machine readable way (i.e. so the app can modify its behaviour and stop using the cached pin, etc.) |
Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #3542)
This command can be used to view the contents of any supported type of information fetched from a URI, and output them in PEM format. Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #3542)
Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #3542)
Containers are objects that are containers for a bunch of other objects with types we recognise but aren't readable in a stream. Such containers are read and parsed, and their content is cached, to be served one object at a time. This extends the FILE_HANDLER type to include a function to destroy the cache and a function to simulate the EOF check. Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #3542)
Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #3542)
Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #3542)
Some containers might very simply decode into something new that deserves to be considered as new (embedded) data. With the help of a special OSSL_STORE_INFO type, make that new data available to the loader functions so they can start over. Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #3542)
…start Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #3542)
Add a separate handler for encrypted PKCS#8 data. This uses the new restart functionality. Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #3542)
This has it recognised when the given path is a directory. In that case, the file loader will give back a series of names, all as URI formatted as possible given the incoming URI. Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #3542)
Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #3542)
Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #3542)
…dlers Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #3542)
Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #3542)
Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #3542)
These tests were inspired by OpenConnect and incorporated by permission of David Woodhouse under CLA Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #3542)
Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #3542)
Sometimes, 'file_load' couldn't really distinguish if a file handler matched the data and produced an error or if it didn't match the data at all. Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #3542)
| if (a->scheme != NULL && b->scheme != NULL) | ||
| return strcmp(a->scheme, b->scheme); | ||
| else if (a->scheme == b->scheme) | ||
| return 0; |
There was a problem hiding this comment.
I thought we super-double-plus ensured that the scheme field was never NULL, so most of this is redundant.
|
|
||
| int OSSL_STORE_error(OSSL_STORE_CTX *ctx) | ||
| { | ||
| return ctx->loader->error(ctx->loader_ctx); |
There was a problem hiding this comment.
These functions seem like a giant footgun, as we don't enforce in OSSL_STORE_register_loader() that all the method functions are non-NULL.
Maybe I missed some previous discussion on this pull request and this is in fact the behavior we want from our "don't check user input for NULL" policy, but my personal preference would be to reject registering an incomplete loader.
| EVP_PKEY *pkey = NULL; | ||
|
|
||
| if (pem_name != NULL) { | ||
| if (strcmp(pem_name, PEM_STRING_PUBLIC) != 0) |
There was a problem hiding this comment.
Do things break if this is pem_check_suffix() instead of strcmp()?
There was a problem hiding this comment.
Yes. PEM_STRING_PUBLIC identifies PKCS#8 public keys, which has the algo OID as part of the contents. As far as I know, that's what we actively support. Is there are reason you'd like us to support public keys that are typed in the PEM name?
There was a problem hiding this comment.
No; I wasn't quite sure on the difference between the forms, and noted that we have a couple examples of "RSA PUBLIC KEY" in documentation, so wondered what the difference was. Thanks for clarifying.
| } else if (strncmp(&uri[5], "///", 3) == 0) { | ||
| path = &uri[7]; | ||
| } else if (strncmp(&uri[5], "//", 2) != 0) { | ||
| path = &uri[5]; |
There was a problem hiding this comment.
No one complained about magic numbers in pointer arithmetic? ;)
There was a problem hiding this comment.
Obviously not... but then, considering the comparison with "file:" just above, this isn't much of a mystery, is it?
| const char *path = NULL; | ||
|
|
||
| if (strncasecmp(uri, "file:", 5) == 0) { | ||
| if (strncmp(&uri[5], "//localhost/", 12) == 0) { |
There was a problem hiding this comment.
by the way, isn't the DNS part of an URI case-insensitive?
| * There is also a OSSL_STORE_open_file() that can be used for file paths | ||
| * that can't be represented as URIs, such as Windows backslashes | ||
| */ | ||
| OSSL_STORE_CTX *ctx = OSSL_STORE_open("file:/foo/bar/data.pem"); |
There was a problem hiding this comment.
I have issue with this example provided. The first time I saw this function it hasn't this prototype: https://github.com/openssl/openssl/blame/4fd397821139723fd4e51a03e92df33e9a9fadcc/include/openssl/store.h#L55-L58 I will use code from storeutl to help me.
This is a new pull request to replace #2011, which has encountered github performance issues.
This STORE module adds the following functionality:
a URI and helps loading the supported objects (PKEYs, CERTs and CRLs
for the moment) from it.
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
Checklist