Skip to content

Replumbing: pre-populate the EVP namemap with commonly known names#8984

Closed
levitte wants to merge 6 commits intoopenssl:masterfrom
levitte:prepopulating-names
Closed

Replumbing: pre-populate the EVP namemap with commonly known names#8984
levitte wants to merge 6 commits intoopenssl:masterfrom
levitte:prepopulating-names

Conversation

@levitte
Copy link
Member

@levitte levitte commented May 23, 2019

This adds ossl_namemap_empty(), to detect if a namemap is empty and
can thereby be pre-populated.

Related to #8967

@levitte levitte changed the title [WIP]Prepopulating names [WIP] Replumbing: pre-populate the EVP namemap with commonly known names May 23, 2019
@levitte levitte added the branch: master Applies to master branch label May 23, 2019
@levitte levitte force-pushed the prepopulating-names branch 2 times, most recently from 56ce553 to d8ddb20 Compare May 24, 2019 12:11
@levitte levitte force-pushed the prepopulating-names branch from d8ddb20 to c043a25 Compare June 4, 2019 09:09
@levitte levitte force-pushed the prepopulating-names branch 2 times, most recently from 6f649b4 to aa39b1e Compare June 5, 2019 07:10
@levitte levitte mentioned this pull request Aug 27, 2019
2 tasks
@levitte levitte force-pushed the prepopulating-names branch from aa39b1e to 492c002 Compare September 12, 2019 11:43
@levitte
Copy link
Member Author

levitte commented Sep 12, 2019

Rebased on a fresh master

@levitte
Copy link
Member Author

levitte commented Sep 14, 2019

@paulidale, it would probably be more constructive to discuss the technical aspects of this PR here...

@levitte
Copy link
Member Author

levitte commented Sep 21, 2019

This includes #9969, please review that one first

@levitte levitte force-pushed the prepopulating-names branch from d383eb6 to 7398856 Compare September 30, 2019 20:48
@levitte levitte added the hold label Sep 30, 2019
@levitte
Copy link
Member Author

levitte commented Sep 30, 2019

I'm putting this on hold, because it should be reworked to work together with the changes in #8985, i.e. that one needs to be merged first

@levitte
Copy link
Member Author

levitte commented Oct 31, 2019

(the hold label that I added was on a personal basis, not an OMC decision thing, so I removed it)

@levitte levitte force-pushed the prepopulating-names branch from 7398856 to b087436 Compare October 31, 2019 14:38
@levitte
Copy link
Member Author

levitte commented Oct 31, 2019

So, this is essentially finished at this point. Updating it was a breeze...

However, I wonder if this is still desirable. We have lived without a pre-populated namemap for a while now, and I still see no signs of panic or horror with regards to that, so ...

@levitte levitte changed the title [WIP] Replumbing: pre-populate the EVP namemap with commonly known names Replumbing: pre-populate the EVP namemap with commonly known names Nov 1, 2019
@mattcaswell
Copy link
Member

I think this is a valuable thing to have. We want to make it as easy as possible for people to transition from one provider to another. At the moment all of our aliases are held in the default provider - but if you don't load that then you don't get the full list of aliases. Having them pre-populated in the namemap would mean we always have them.

@mattcaswell mattcaswell added the approval: review pending This pull request needs review by a committer label Nov 1, 2019
@levitte levitte changed the title Replumbing: pre-populate the EVP namemap with commonly known names WIP: Replumbing: pre-populate the EVP namemap with commonly known names Nov 1, 2019
@mattcaswell mattcaswell removed the approval: review pending This pull request needs review by a committer label Nov 1, 2019
@levitte
Copy link
Member Author

levitte commented Nov 1, 2019

I put this in WIP because @mattcaswell and I realised that we need to keep the legacy method database (which is often wrongly called "object database") around, and I might as well use that to pre-populate the EVP namemap instead of having an intermediary array with the exact same information.

This adds ossl_namemap_empty(), to detect if a namemap is empty and
can thereby be pre-populated.

Related to openssl#8967
…ames

This changes how the initial EVP namemap is prepopulated.  Instead of
have a separate database of names, it simply pilfers the still existing
legacy method database.
@levitte levitte force-pushed the prepopulating-names branch from 9dde637 to 0027896 Compare November 11, 2019 09:19
@levitte
Copy link
Member Author

levitte commented Nov 11, 2019

There's something very odd happening with the solution to pre-populate names from the legacy method db.

… known names

This affected the way legacy NIDs are looked up in evp_cipher_from_dispatch()
and evp_md_from_dispatch().  Instead of trying to find the NID directly,
look up the legacy method structure and grab the NID from there.  The
reason is that NIDs can be aliases for other NIDs, which looked like
a clash even if wasn't really one.
@levitte
Copy link
Member Author

levitte commented Nov 11, 2019

It seems I finally pulled through all corner cases (and there were a few). If the CIs agree, I'll take this out of WIP.

@levitte levitte changed the title WIP: Replumbing: pre-populate the EVP namemap with commonly known names Replumbing: pre-populate the EVP namemap with commonly known names Nov 12, 2019
@levitte
Copy link
Member Author

levitte commented Nov 12, 2019

Not WIP any more

@levitte
Copy link
Member Author

levitte commented Nov 15, 2019

Ping

@levitte levitte added the approval: review pending This pull request needs review by a committer label Nov 15, 2019
@paulidale paulidale added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Nov 15, 2019
@levitte levitte added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Nov 18, 2019
openssl-machine pushed a commit that referenced this pull request Nov 20, 2019
This adds ossl_namemap_empty(), to detect if a namemap is empty and
can thereby be pre-populated.

This also affects the way legacy NIDs are looked up in
evp_cipher_from_dispatch() and evp_md_from_dispatch().  Instead of
trying to find the NID directly, look up the legacy method structure
and grab the NID from there.  The reason is that NIDs can be aliases
for other NIDs, which looks like a clash even if wasn't really one.

Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #8984)
@levitte
Copy link
Member Author

levitte commented Nov 20, 2019

Merged.

6a835fc Replumbing: pre-populate the EVP namemap with commonly known names

@levitte levitte closed this Nov 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants