Skip to content

Comments

OSSL_STORE additions#11756

Closed
levitte wants to merge 11 commits intoopenssl:masterfrom
levitte:OSSL_STORE-additions-20200507
Closed

OSSL_STORE additions#11756
levitte wants to merge 11 commits intoopenssl:masterfrom
levitte:OSSL_STORE-additions-20200507

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 branch: master Applies to master branch approval: review pending This pull request needs review by a committer labels May 7, 2020
@levitte levitte added this to the 3.0.0 milestone May 7, 2020
@levitte levitte requested a review from DDvO May 7, 2020 11:12
@levitte
Copy link
Member Author

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 commented May 7, 2020

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

DDvO
DDvO reviewed May 7, 2020
View reviewed changes
DDvO
DDvO requested changes May 7, 2020
View reviewed changes
DDvO
DDvO reviewed May 8, 2020
View reviewed changes
@DDvO
Copy link
Contributor

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.

This was referenced May 8, 2020
Closed
Closed
@DDvO
Copy link
Contributor

DDvO commented May 8, 2020

Is my review sufficient in this case?

@levitte
Copy link
Member Author

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 commented May 8, 2020

This is now out of WIP

@DDvO
Copy link
Contributor

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 added 8 commits May 12, 2020 11:26
@levitte
OSSL_STORE: Make it possible to attach an OSSL_STORE to an opened BIO
837e862 
This capability existed internally, and is now made public.
@levitte
OSSL_STORE: Better information when prompting for pass phrases
4f676a7 
The prompt includes the URI, to make it clear which object needs a
pass phrase.
@levitte
OSSL_STORE: Make the 'file' scheme loader handle MSBLOB and PVK files
145586e 
This involves exposing two pvkfmt.c functions, but only internally.
@levitte
fixup! OSSL_STORE: Make it possible to attach an OSSL_STORE to an ope…
e142c3a 
…ned BIO
@levitte
fixup! OSSL_STORE: Make the 'file' scheme loader handle MSBLOB and PV…
a9c99aa 
…K files
@levitte
fixup! OSSL_STORE: Make it possible to attach an OSSL_STORE to an ope…
e4bf64b 
…ned BIO
@levitte
fixup! OSSL_STORE: Make it possible to attach an OSSL_STORE to an ope…
4d1ffa4 
…ned BIO
@levitte
fixup! OSSL_STORE: Make it possible to attach an OSSL_STORE to an ope…
31de025 
…ned BIO
@levitte levitte force-pushed the OSSL_STORE-additions-20200507 branch from 7e5675b to 842986c Compare May 12, 2020 09:27
@DDvO
Copy link
Contributor

DDvO commented May 12, 2020 •
edited
Loading

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 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 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 commented May 12, 2020 •
edited
Loading

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 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 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
View reviewed changes
Copy link
Contributor

@DDvO DDvO left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@DDvO DDvO added approval: done This pull request has the required number of approvals approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: review pending This pull request needs review by a committer labels May 13, 2020
@DDvO
Copy link
Contributor

DDvO commented May 13, 2020

@levitte, I think this can be merged now.

@levitte levitte removed the approval: done This pull request has the required number of approvals label May 13, 2020
openssl-machine pushed a commit that referenced this pull request May 13, 2020
@levitte
OSSL_STORE: Make it possible to attach an OSSL_STORE to an opened BIO
6ab6ecf 
This capability existed internally, and is now made public.

Reviewed-by: David von Oheimb <[email protected]>
(Merged from #11756)
openssl-machine pushed a commit that referenced this pull request May 13, 2020
@levitte
OSSL_STORE: Better information when prompting for pass phrases
bac4bff 
The prompt includes the URI, to make it clear which object needs a
pass phrase.

Reviewed-by: David von Oheimb <[email protected]>
(Merged from #11756)
openssl-machine pushed a commit that referenced this pull request May 13, 2020
@levitte
OSSL_STORE: Make the 'file' scheme loader handle MSBLOB and PVK files
f55838f 
This involves exposing two pvkfmt.c functions, but only internally.

Reviewed-by: David von Oheimb <[email protected]>
(Merged from #11756)
@levitte
Copy link
Member Author

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 OSSL_STORE-additions-20200507 branch May 13, 2020 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DDvO DDvO DDvO approved these changes

Assignees

No one assigned

Labels

approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Applies to master branch

Projects

None yet

Milestone

3.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@levitte @DDvO @mattcaswell