Avoid duplicate dir search #24140

vdukhovni · 2024-04-15T04:20:33Z

This avoids adding the default CApath to the list of sources for "store" lookups, it is already on the legacy directory search list.

Is including the default search directory (or else the SSL_CERT_DIR environment variable value) in the store lookup list required in some situations???

Note, this is based on PR #24139 that fixes tests that failed (depending on time of day), while testing this PR.

crypto/x509/x509_d2.c

vdukhovni · 2024-04-17T17:46:59Z

Performance comparison from x509storeissuer, with thread counts of 1, 10, 100 and 1000:

Baseline is master without this PR. The "no-deprecated" numbers are also without this PR, but with "no-deprecated" passed to Configure. It seems the vast majority of the performance impact is from all the logic around trying to find a legacy engine that supports the URI scheme!

IMHO the URI schemes should still be eagerly resolved to their lookup methods at the time the URI is added to the store, rather than during the lookup. Are there compelling reasons to defer the binding? In any case, perhaps we can remove the legacy engine support from the store code, sooner rather than later?

Threads	Baseline (us)	PR (us)	no-deprecated (us)
1	3107	1459	2779
10	8505	2432	4323
100	156688	2560	12286
1000	1382203	2501	4695

vdukhovni · 2024-04-17T19:48:21Z

I think this is ready for review (see updated comment above). Dropping legacy engine support from the store is I think a separate issue, that can be tackled in another PR... @levitte perhaps?

Sashan

thanks for fixing the tests. I think I've hit this bug earlier this week when running tests late in night my time.

vdukhovni · 2024-04-18T04:08:44Z

thanks for fixing the tests. I think I've hit this bug earlier this week when running tests late in night my time.

Note that the test fixes are a separate PR that is a merge-base for this one. Once that is merged, this will be rebased to contain only the last commit. The base PR has two approvals, so I hope will be merged shortly.

levitte · 2024-04-18T04:18:36Z

crypto/x509/x509_d2.c

+    /*
+     * Also search the default store URIs, if any, but
+     * DO NOT add the default search directory also to this lookup,
+     * such a second search would be slower and redundant.
+     */


At this point, is this comment still needed? Considering that adding the default search directory happened somewhere else (unknown to the code in this function), this comment is a bit confusing here. Perhaps it should be moved to crypto/x509/by_store.c, as a reminder not to remake that mistake?

Reworked and moved the comment.

levitte · 2024-04-18T04:21:26Z

I think this is ready for review (see updated comment above). Dropping legacy engine support from the store is I think a separate issue, that can be tackled in another PR... @levitte perhaps?

That would be a breaking change for anyone still using an engine with a scheme implementation. I believe that the pkcs11 engine is one of those... no? Anyway, that would defer such a change to OpenSSL 4.0 at the earliest, and should probably happen in sync with general removal of engine support (which is currently only deprecated).

levitte · 2024-04-18T04:52:08Z

IMHO the URI schemes should still be eagerly resolved to their lookup methods at the time the URI is added to the store, rather than during the lookup. Are there compelling reasons to defer the binding?

This sounds like a "what function to use" question, really. We have two functions to add a URI to a OSSL_STORE lookup:

X509_LOOKUP_add_store_ex(), which just adds the URI to an internal list, from which objects will be loaded later. This is the lazy function.
X509_LOOKUP_load_store_ex(), which loads all objects from the given URI immediately. This is the eager function.

So it seems to me that the choice is left to the user.

But, perhaps you're trying to say that we should use X509_LOOKUP_load_store_ex() in X509_STORE_set_default_paths_ex(), rather than X509_LOOKUP_add_store_ex()?

t8m · 2024-04-18T12:21:56Z

Please rebase as the test fix PR was merged.

mattcaswell · 2024-04-18T12:37:55Z

crypto/x509/by_store.c

-            argp = X509_get_default_cert_dir();
-
-        {
+        if (argp != NULL) {


So is this a change in behaviour in the case that someone only adds the store lookup? i.e. could it break someone relying on this?

Yes, they'd have to add some actual URIs to get anything to happen. I believe that's the right behaviour, but yes, it is different. One might for example ask why only the CApath and not the CAfile defaults into the store...

... One might for example ask why only the CApath and not the CAfile defaults into the store...

That was 100% arbitrary, "I have a hunch" style.

vdukhovni · 2024-04-18T14:30:39Z

Please rebase as the test fix PR was merged.

Done.

vdukhovni · 2024-04-22T14:02:58Z

Nudging reviewers... I believe this is ready.

Fixes openssl#21067

t8m

@nhorman please reconfirm

nhorman · 2024-04-24T16:28:34Z

reconfirmed, approval holds

openssl-machine · 2024-04-25T18:00:12Z

This pull request is ready to merge

t8m · 2024-04-26T07:04:14Z

Merged to the master branch. Thank you.

Fixes #21067 Reviewed-by: Neil Horman <nhorman@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #24140)

Fixes openssl#21067 Reviewed-by: Neil Horman <nhorman@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#24140)

vdukhovni requested review from levitte and mattcaswell April 15, 2024 04:20

vdukhovni linked an issue Apr 16, 2024 that may be closed by this pull request

Slow response when using the store X509_LOOKUP method #21067

Closed

nhorman approved these changes Apr 16, 2024

View reviewed changes

t8m added branch: master Merge to master branch approval: otc review pending This pull request needs review by an OTC member tests: exempted The PR is exempt from requirements for testing triaged: performance The issue/pr reports/fixes a performance concern labels Apr 16, 2024

levitte reviewed Apr 16, 2024

View reviewed changes

crypto/x509/x509_d2.c Outdated Show resolved Hide resolved

vdukhovni force-pushed the avoid-duplicate-dir-search branch 3 times, most recently from a307c9f to 9805c4f Compare April 17, 2024 17:17

Sashan approved these changes Apr 17, 2024

View reviewed changes

levitte reviewed Apr 18, 2024

View reviewed changes

mattcaswell reviewed Apr 18, 2024

View reviewed changes

vdukhovni force-pushed the avoid-duplicate-dir-search branch from 9805c4f to 6114b67 Compare April 18, 2024 14:30

vdukhovni force-pushed the avoid-duplicate-dir-search branch from 6114b67 to 6868ca1 Compare April 23, 2024 19:27

Avoid duplicate default CApath lookups

6868ca1

Fixes openssl#21067

t8m approved these changes Apr 24, 2024

View reviewed changes

t8m added approval: review pending This pull request needs review by a committer and removed approval: otc review pending This pull request needs review by an OTC member labels Apr 24, 2024

t8m 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 Apr 24, 2024

openssl-machine 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 Apr 25, 2024

t8m closed this Apr 26, 2024

openssl-machine pushed a commit that referenced this pull request Apr 26, 2024

Avoid duplicate default CApath lookups

6d01857

Fixes #21067 Reviewed-by: Neil Horman <nhorman@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #24140)

nhorman mentioned this pull request May 24, 2024

Massive performance degradation in OpenSsl 3.0 if used in a heavy multi threaded server application #17064

Open

jvdsn pushed a commit to jvdsn/openssl that referenced this pull request Jun 3, 2024

Avoid duplicate default CApath lookups

02d0e29

Fixes openssl#21067 Reviewed-by: Neil Horman <nhorman@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#24140)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid duplicate dir search #24140

Avoid duplicate dir search #24140

vdukhovni commented Apr 15, 2024

vdukhovni commented Apr 17, 2024 •

edited

Loading

vdukhovni commented Apr 17, 2024

Sashan left a comment

vdukhovni commented Apr 18, 2024

levitte Apr 18, 2024 •

edited

Loading

vdukhovni Apr 23, 2024

levitte commented Apr 18, 2024

levitte commented Apr 18, 2024 •

edited

Loading

t8m commented Apr 18, 2024

mattcaswell Apr 18, 2024

vdukhovni Apr 18, 2024

levitte Apr 18, 2024

vdukhovni commented Apr 18, 2024

vdukhovni commented Apr 22, 2024

t8m left a comment

nhorman commented Apr 24, 2024

openssl-machine commented Apr 25, 2024

t8m commented Apr 26, 2024

Avoid duplicate dir search #24140

Avoid duplicate dir search #24140

Conversation

vdukhovni commented Apr 15, 2024

vdukhovni commented Apr 17, 2024 • edited Loading

vdukhovni commented Apr 17, 2024

Sashan left a comment

Choose a reason for hiding this comment

vdukhovni commented Apr 18, 2024

levitte Apr 18, 2024 • edited Loading

Choose a reason for hiding this comment

vdukhovni Apr 23, 2024

Choose a reason for hiding this comment

levitte commented Apr 18, 2024

levitte commented Apr 18, 2024 • edited Loading

t8m commented Apr 18, 2024

mattcaswell Apr 18, 2024

Choose a reason for hiding this comment

vdukhovni Apr 18, 2024

Choose a reason for hiding this comment

levitte Apr 18, 2024

Choose a reason for hiding this comment

vdukhovni commented Apr 18, 2024

vdukhovni commented Apr 22, 2024

t8m left a comment

Choose a reason for hiding this comment

nhorman commented Apr 24, 2024

openssl-machine commented Apr 25, 2024

t8m commented Apr 26, 2024

vdukhovni commented Apr 17, 2024 •

edited

Loading

levitte Apr 18, 2024 •

edited

Loading

levitte commented Apr 18, 2024 •

edited

Loading