Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OSSL_STORE additions #11756

Closed
wants to merge 11 commits into from
Closed

Conversation

@levitte
Copy link
Member

@levitte levitte commented May 7, 2020

Additions that I've had lying around in #7390 but are hopefully interesting on their own.

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

This capability existed internally, and is now made public.

OSSL_STORE: Better information when prompting for pass phrases

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

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

@levitte levitte added this to the 3.0.0 milestone May 7, 2020
@levitte levitte requested a review from DDvO May 7, 2020
@levitte
Copy link
Member Author

@levitte levitte commented May 7, 2020

@DDvO, I think this might interesting for your effort in #11755

@levitte levitte changed the title OSSL_STORE additions [WIP] OSSL_STORE additions May 7, 2020
@levitte
Copy link
Member Author

@levitte levitte commented May 7, 2020

I made this WIP for the moment, to clear the Travis failures

crypto/err/openssl.txt Outdated Show resolved Hide resolved
crypto/err/openssl.txt Outdated Show resolved Hide resolved
crypto/include/internal/pem_int.h Show resolved Hide resolved
crypto/store/loader_file.c Show resolved Hide resolved
crypto/store/loader_file.c Outdated Show resolved Hide resolved
crypto/store/loader_file.c Show resolved Hide resolved
crypto/store/loader_file.c Show resolved Hide resolved
crypto/store/loader_file.c Show resolved Hide resolved
crypto/store/loader_file.c Outdated Show resolved Hide resolved
crypto/store/store_lib.c Outdated Show resolved Hide resolved
crypto/store/store_lib.c Show resolved Hide resolved
include/openssl/store.h Outdated Show resolved Hide resolved
include/openssl/store.h Show resolved Hide resolved
@DDvO
Copy link
Contributor

@DDvO DDvO commented May 8, 2020

Thanks @levitte for quickly de-coupling these changes for use in #11755.
I've meanwhile finished my review of this PR. I found just minor (potential and actual) issues with it.

I appreciate the flexibility it provides w.r.t. custom BIO input and the added MSBLOB and PVK support, which will allow further strong simplification of the loading functions adapted in #11755,
and also the improved user interation on password input.

@DDvO
Copy link
Contributor

@DDvO DDvO commented May 8, 2020

Is my review sufficient in this case?

@levitte
Copy link
Member Author

@levitte levitte commented May 8, 2020

Is my review sufficient in this case?

It that's about my lack of interaction, I had some personal time during the day... 😉

@levitte levitte changed the title [WIP] OSSL_STORE additions OSSL_STORE additions May 8, 2020
@levitte
Copy link
Member Author

@levitte levitte commented May 8, 2020

This is now out of WIP

@DDvO
Copy link
Contributor

@DDvO DDvO commented May 9, 2020

Great that you meanwhile fixed those (mostly minor) issues.
Now just two very minor doc-nits remain to be fixed.
Apart from that the PR looks very good to me.

@levitte levitte force-pushed the levitte:OSSL_STORE-additions-20200507 branch to 842986c May 12, 2020
@DDvO
Copy link
Contributor

@DDvO DDvO commented May 12, 2020

Still the NAME section of OSSL_STORE_LOADER.pod needs to be extended - doc-nits says:

doc/man3/OSSL_STORE_LOADER.pod:1: OSSL_STORE_attach_fn missing from NAME section
doc/man3/OSSL_STORE_LOADER.pod:1: OSSL_STORE_LOADER_set_attach missing from NAME section
@levitte
Copy link
Member Author

@levitte levitte commented May 12, 2020

Still the NAME section of OSSL_STORE_LOADER.pod needs to be extended - doc-nits says:

Ah, thanks. Fixed

@DDvO
Copy link
Contributor

@DDvO DDvO commented May 12, 2020

Sigh, now doc-nits mourns:

OSSL_STORE_attach_fn(3) is supposedly internal but is documented as public

I think it needs to be listed in util/other.syms.

It's advisable (but I also tend to forget) to run make doc-nits before pushing.

@levitte
Copy link
Member Author

@levitte levitte commented May 12, 2020

Sigh, now doc-nits mourns

Silly find-doc-nits can't properly parse typedefs... :-/

I added a line in other.syms

@DDvO
Copy link
Contributor

@DDvO DDvO commented May 12, 2020

The changes I had requested are meanwhile done. So I wanted to change my review outcome to "approved", but GitHub seems to have lost my review state :-(
Nevertheless I'm uncertain if an approval from my side has a formal value since I'm not an OTC member.

@mattcaswell
Copy link
Member

@mattcaswell mattcaswell commented May 12, 2020

Nevertheless I'm uncertain if an approval from my side has a formal value since I'm not an OTC member.

Yes, your approval counts. PRs need one OTC approval and one commiter approval - and the author "counts". Since, @levitte is an OTC member he just needs one committer approval.

In any case "extra" approvals are useful to show what review a PR has gone throug.

@DDvO
DDvO approved these changes May 12, 2020
Copy link
Contributor

@DDvO DDvO left a comment

LGTM

@DDvO
Copy link
Contributor

@DDvO DDvO commented May 13, 2020

@levitte, I think this can be merged now.

@levitte levitte removed the approval: done label May 13, 2020
openssl-machine pushed a commit that referenced this pull request May 13, 2020
This capability existed internally, and is now made public.

Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from #11756)
openssl-machine pushed a commit that referenced this pull request May 13, 2020
The prompt includes the URI, to make it clear which object needs a
pass phrase.

Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from #11756)
openssl-machine pushed a commit that referenced this pull request May 13, 2020
This involves exposing two pvkfmt.c functions, but only internally.

Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from #11756)
@levitte
Copy link
Member Author

@levitte levitte commented May 13, 2020

Merged

6ab6ecf OSSL_STORE: Make it possible to attach an OSSL_STORE to an opened BIO
bac4bff OSSL_STORE: Better information when prompting for pass phrases
f55838f OSSL_STORE: Make the 'file' scheme loader handle MSBLOB and PVK files

@levitte levitte closed this May 13, 2020
@levitte levitte deleted the levitte:OSSL_STORE-additions-20200507 branch May 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants