Rework the "by store" X509_LOOKUP method to open the given URI early by levitte · Pull Request #27529 · openssl/openssl

levitte · 2025-04-30T09:53:17Z

The cached X509_LOOKUP method data is no longer just the URI, but now
includes the OSSL_STORE_CTX pointer, and required parameters to reopen
the URI at any time. cache_objects() is modified to handle this, and
only (re)open the URI when it wasn't previously opened, or when it was
closed by an earlier call.

This way, we can call OSSL_STORE_open_ex() in by_store_ctrl_ex(), and
get to see possible errors when the URI is loaded.

This assumes that if the URI could be opened once, it can be opened
again.

Fixes #27461
Alternative to #27507

levitte · 2025-04-30T10:00:57Z

Side note: for anyone caring about the source format checker, there was an interesting false ~~negative~~ positive, see https://github.com/openssl/openssl/actions/runs/14751802024/job/41410783642?pr=27529 (@DDvO, @nhorman)

mattcaswell

Not sure I really understand why this fixes #27461 which seems to be about absolute paths for the file scheme...or is it just the silent error part of #27461?

crypto/x509/by_store.c

DDvO · 2025-04-30T10:58:28Z

Not sure I really understand why this fixes #27461 which seems to be about absolute paths for the file scheme...or is it just the silent error part of #27461?

@mattcaswell this new PR is about the silent error part.
The relative path extension has already been done.

DDvO · 2025-04-30T11:42:23Z

Fixes #27461 Alternative to #27507

On one hand, it provides a more thorough fix,
on the other, it does not contain fixes for related minor bugs still covered by #27507.
One of those causes failure of the new verify test case "CAstore file://path":
https://github.com/openssl/openssl/actions/runs/14751802017/job/41410786941?pr=27529

levitte · 2025-04-30T11:51:07Z

... it does not contain fixes for related minor bugs. One of those causes failure of the new verify test case "CAstore file://path": https://github.com/openssl/openssl/actions/runs/14751802017/job/41410786941?pr=27529

That's because file://path is a URI with the authority path and no path. Will fix that check.

levitte · 2025-04-30T11:52:05Z

... it does not contain fixes for related minor bugs. One of those causes failure of the new verify test case "CAstore file://path": https://github.com/openssl/openssl/actions/runs/14751802017/job/41410786941?pr=27529

That's because file://path is a URI with the authority path and no path. Will fix that check.

... or huh??? hold on, something's weird with that one

levitte · 2025-04-30T11:56:08Z

Ah, I see. Yeah, please read RFC 8089, file://C:/what/ever.pem is non-standard, it should be file:///C:/what/ever.pem, i.e. the full URI path part should start with /C:

levitte · 2025-04-30T12:03:46Z

Not sure I really understand why this fixes #27461 which seems to be about absolute paths for the file scheme...or is it just the silent error part of #27461?

@mattcaswell this new PR is about the silent error part. The relative path extension has already been done.

Yeah. It's unfortunate that it got treated in the same issue, but quite frankly, this is at a point where raising a new issue "just 'cause" seems like unnecessary churn.

DDvO · 2025-04-30T12:13:28Z

Ah, I see. Yeah, please read RFC 8089, file://C:/what/ever.pem is non-standard, it should be file:///C:/what/ever.pem, i.e. the full URI path part should start with /C:

So it turns out that the Perl function abs_path() is wrong, since it did not put a / before the drive letter.

DDvO · 2025-04-30T12:19:08Z

BTW, while fixing 25-test_verify.t,
could you please also add a test case for a relative cert file name that does not have a scheme-like prefix, e.g.

my $relative_file = "cert.pem";
copy($rootcert, $relative_file);
ok(vfy_root("-CAstore", $relative_file), "CAstore relative_filename");

levitte · 2025-04-30T12:42:49Z

Ah, I see. Yeah, please read RFC 8089, file://C:/what/ever.pem is non-standard, it should be file:///C:/what/ever.pem, i.e. the full URI path part should start with /C:

So it turns out that the Perl function abs_path() is wrong, since it did not put a / before the drive letter.

It's perfectly functional, but requires a small tweak when used for a file: URI

levitte · 2025-04-30T12:47:03Z

BTW, while fixing 25-test_verify.t, could you please also add a test case for a relative cert file name that does not have a scheme-like prefix, e.g.
my $relative_file = "cert.pem";
copy($rootcert, $relative_file);
ok(vfy_root("-CAstore", $relative_file), "CAstore relative_filename");

But... we already have tests for relative paths!

t8m

One minor thing

crypto/x509/by_store.c

The cached X509_LOOKUP method data is no longer just the URI, but now includes the OSSL_STORE_CTX pointer, and required parameters to reopen the URI at any time. cache_objects() is modified to handle this, and only (re)open the URI when it wasn't previously opened, or when it was closed by an earlier call. This way, we can call OSSL_STORE_open_ex() in by_store_ctrl_ex(), and get to see possible errors when the URI is loaded. This assumes that if the URI could be opened once, it can be opened again. Fixes openssl#27461

levitte · 2025-04-30T13:59:10Z

@mattcaswell, please re-approve.

The cached X509_LOOKUP method data is no longer just the URI, but now includes the OSSL_STORE_CTX pointer, and required parameters to reopen the URI at any time. cache_objects() is modified to handle this, and only (re)open the URI when it wasn't previously opened, or when it was closed by an earlier call. This way, we can call OSSL_STORE_open_ex() in by_store_ctrl_ex(), and get to see possible errors when the URI is loaded. This assumes that if the URI could be opened once, it can be opened again. Fixes openssl#27461 Reviewed-by: David von Oheimb <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#27529) (cherry picked from commit 0c48ee2)

Originally from openssl#27507, with some changes. Co-authored-by: Richard Levitte <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Richard Levitte <[email protected]> (Merged from openssl#27529) (cherry picked from commit 927deba)

It was used to pass libctx and propq, which would override the corresponding values passed to by_store_ctrl_ex(). This wasn't really reasonable to do either way, as it could potentially be a surprise to the user, who can reasonably expect that the URI is opened with the libctx and propq that was passed with the URI, and not with those passed later. Reviewed-by: David von Oheimb <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#27529) (cherry picked from commit af5952d)

The cached X509_LOOKUP method data is no longer just the URI, but now includes the OSSL_STORE_CTX pointer, and required parameters to reopen the URI at any time. cache_objects() is modified to handle this, and only (re)open the URI when it wasn't previously opened, or when it was closed by an earlier call. This way, we can call OSSL_STORE_open_ex() in by_store_ctrl_ex(), and get to see possible errors when the URI is loaded. This assumes that if the URI could be opened once, it can be opened again. Fixes openssl#27461 Reviewed-by: David von Oheimb <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#27529) (cherry picked from commit 0c48ee2)

Originally from openssl#27507, with some changes. Co-authored-by: Richard Levitte <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Richard Levitte <[email protected]> (Merged from openssl#27529) (cherry picked from commit 927deba)

It was used to pass libctx and propq, which would override the corresponding values passed to by_store_ctrl_ex(). This wasn't really reasonable to do either way, as it could potentially be a surprise to the user, who can reasonably expect that the URI is opened with the libctx and propq that was passed with the URI, and not with those passed later. Reviewed-by: David von Oheimb <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#27529) (cherry picked from commit af5952d)

DDvO · 2025-05-05T08:40:51Z

So backporting will have to happen in separate PRs.

I just did for 3.4 in #27558.

On 3.3 and older, there are code differences in crypto/x509/by_store.c that require an effort to get right, more than a simple cherry-pick allows.

I tried to come up with a respective backport PR,
but as the conflicts are not straightforward to resolve, I don't dare to do with my limited overview of OSSL_STORE.

levitte · 2025-05-05T08:58:34Z

I've done the backports in:

for 3.4: Rework the "by store" X509_LOOKUP method to open the given URI early [3.4] #27550
for 3.3, 3.2 and 3.0: Rework the "by store" X509_LOOKUP method to open the given URI early [3.3] #27551

mattcaswell · 2025-05-30T09:23:46Z

FYI, this seems to have had a significant positive benefit to the x509storeissuer performance test for the 3.0, 3.2 and 3.3 backports. There is a marked drop in the time taken to run that test in those branches on the date that the backport of this PR was merged:

https://openssl-library.org/performance.html

The cached X509_LOOKUP method data is no longer just the URI, but now includes the OSSL_STORE_CTX pointer, and required parameters to reopen the URI at any time. cache_objects() is modified to handle this, and only (re)open the URI when it wasn't previously opened, or when it was closed by an earlier call. This way, we can call OSSL_STORE_open_ex() in by_store_ctrl_ex(), and get to see possible errors when the URI is loaded. This assumes that if the URI could be opened once, it can be opened again. Fixes openssl#27461 Reviewed-by: David von Oheimb <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#27529)

Originally from openssl#27507, with some changes. Co-authored-by: Richard Levitte <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Richard Levitte <[email protected]> (Merged from openssl#27529)

It was used to pass libctx and propq, which would override the corresponding values passed to by_store_ctrl_ex(). This wasn't really reasonable to do either way, as it could potentially be a surprise to the user, who can reasonably expect that the URI is opened with the libctx and propq that was passed with the URI, and not with those passed later. Reviewed-by: David von Oheimb <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#27529)

The cached X509_LOOKUP method data is no longer just the URI, but now includes the OSSL_STORE_CTX pointer, and required parameters to reopen the URI at any time. cache_objects() is modified to handle this, and only (re)open the URI when it wasn't previously opened, or when it was closed by an earlier call. This way, we can call OSSL_STORE_open_ex() in by_store_ctrl_ex(), and get to see possible errors when the URI is loaded. This assumes that if the URI could be opened once, it can be opened again. Fixes openssl#27461 Reviewed-by: David von Oheimb <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#27529)

Originally from openssl#27507, with some changes. Co-authored-by: Richard Levitte <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Richard Levitte <[email protected]> (Merged from openssl#27529)

It was used to pass libctx and propq, which would override the corresponding values passed to by_store_ctrl_ex(). This wasn't really reasonable to do either way, as it could potentially be a surprise to the user, who can reasonably expect that the URI is opened with the libctx and propq that was passed with the URI, and not with those passed later. Reviewed-by: David von Oheimb <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#27529)

The cached X509_LOOKUP method data is no longer just the URI, but now includes the OSSL_STORE_CTX pointer, and required parameters to reopen the URI at any time. cache_objects() is modified to handle this, and only (re)open the URI when it wasn't previously opened, or when it was closed by an earlier call. This way, we can call OSSL_STORE_open_ex() in by_store_ctrl_ex(), and get to see possible errors when the URI is loaded. This assumes that if the URI could be opened once, it can be opened again. Fixes openssl#27461 Reviewed-by: David von Oheimb <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#27529)

Originally from openssl#27507, with some changes. Co-authored-by: Richard Levitte <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Richard Levitte <[email protected]> (Merged from openssl#27529)

It was used to pass libctx and propq, which would override the corresponding values passed to by_store_ctrl_ex(). This wasn't really reasonable to do either way, as it could potentially be a surprise to the user, who can reasonably expect that the URI is opened with the libctx and propq that was passed with the URI, and not with those passed later. Reviewed-by: David von Oheimb <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#27529)

Previously openssl#27529 made a change to `by_store_ctrl_ex` in order to open the OSSL_STORE early. The reason given in that PR is: "This way, we can call OSSL_STORE_open_ex() in by_store_ctrl_ex(), and get to see possible errors when the URI is loaded" That PR then kept the store open until cache_objects is called and then reused it. Unfortunately by the time cache_objects() is called we could be in a multi-threaded scenario where the X509_STORE is being shared by multiple threads. We then get a race condition where multiple threads are all using (and ultimately closing) the same `OSSL_STORE_CTX`. The purpose of keeping the `OSSL_STORE` object between by_store_ctrl_ex() and `cache_objects` is presumably an optimisation to avoid having to open the store twice. But this does not work because of the above issue. We just take the hit and open it again. Fixes openssl#28171

Previously #27529 made a change to `by_store_ctrl_ex` in order to open the OSSL_STORE early. The reason given in that PR is: "This way, we can call OSSL_STORE_open_ex() in by_store_ctrl_ex(), and get to see possible errors when the URI is loaded" That PR then kept the store open until cache_objects is called and then reused it. Unfortunately by the time cache_objects() is called we could be in a multi-threaded scenario where the X509_STORE is being shared by multiple threads. We then get a race condition where multiple threads are all using (and ultimately closing) the same `OSSL_STORE_CTX`. The purpose of keeping the `OSSL_STORE` object between by_store_ctrl_ex() and `cache_objects` is presumably an optimisation to avoid having to open the store twice. But this does not work because of the above issue. We just take the hit and open it again. Fixes #28171 Reviewed-by: Saša Nedvědický <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #28198)

Previously #27529 made a change to `by_store_ctrl_ex` in order to open the OSSL_STORE early. The reason given in that PR is: "This way, we can call OSSL_STORE_open_ex() in by_store_ctrl_ex(), and get to see possible errors when the URI is loaded" That PR then kept the store open until cache_objects is called and then reused it. Unfortunately by the time cache_objects() is called we could be in a multi-threaded scenario where the X509_STORE is being shared by multiple threads. We then get a race condition where multiple threads are all using (and ultimately closing) the same `OSSL_STORE_CTX`. The purpose of keeping the `OSSL_STORE` object between by_store_ctrl_ex() and `cache_objects` is presumably an optimisation to avoid having to open the store twice. But this does not work because of the above issue. We just take the hit and open it again. Fixes #28171 Reviewed-by: Saša Nedvědický <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #28198) (cherry picked from commit 08951fb)

Previously openssl#27529 made a change to `by_store_ctrl_ex` in order to open the OSSL_STORE early. The reason given in that PR is: "This way, we can call OSSL_STORE_open_ex() in by_store_ctrl_ex(), and get to see possible errors when the URI is loaded" That PR then kept the store open until cache_objects is called and then reused it. Unfortunately by the time cache_objects() is called we could be in a multi-threaded scenario where the X509_STORE is being shared by multiple threads. We then get a race condition where multiple threads are all using (and ultimately closing) the same `OSSL_STORE_CTX`. The purpose of keeping the `OSSL_STORE` object between by_store_ctrl_ex() and `cache_objects` is presumably an optimisation to avoid having to open the store twice. But this does not work because of the above issue. We just take the hit and open it again. Fixes openssl#28171

Previously #27529 made a change to `by_store_ctrl_ex` in order to open the OSSL_STORE early. The reason given in that PR is: "This way, we can call OSSL_STORE_open_ex() in by_store_ctrl_ex(), and get to see possible errors when the URI is loaded" That PR then kept the store open until cache_objects is called and then reused it. Unfortunately by the time cache_objects() is called we could be in a multi-threaded scenario where the X509_STORE is being shared by multiple threads. We then get a race condition where multiple threads are all using (and ultimately closing) the same `OSSL_STORE_CTX`. The purpose of keeping the `OSSL_STORE` object between by_store_ctrl_ex() and `cache_objects` is presumably an optimisation to avoid having to open the store twice. But this does not work because of the above issue. We just take the hit and open it again. Fixes #28171 Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Saša Nedvědický <[email protected]> (Merged from #28385)

levitte added branch: master Applies to master branch approval: review pending This pull request needs review by a committer triaged: bug The issue/pr is/fixes a bug tests: present The PR has suitable tests present labels Apr 30, 2025

levitte mentioned this pull request Apr 30, 2025

OSSL_STORE, HTTP, apps/: fix URI scheme parsing and URI-related doc #27507

Open

levitte added the style: waived exempted from style checks label Apr 30, 2025

mattcaswell requested changes Apr 30, 2025

View reviewed changes

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

crypto/x509/by_store.c Show resolved Hide resolved

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

DDvO requested changes Apr 30, 2025

View reviewed changes

levitte force-pushed the fix-27461 branch from 5c2e80e to a2421cd Compare April 30, 2025 12:23

mattcaswell previously approved these changes Apr 30, 2025

View reviewed changes

t8m requested changes Apr 30, 2025

View reviewed changes

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

levitte dismissed mattcaswell’s stale review via 440f337 April 30, 2025 13:58

levitte force-pushed the fix-27461 branch from ae60f7e to 440f337 Compare April 30, 2025 13:58

mattcaswell previously approved these changes Apr 30, 2025

View reviewed changes

t8m previously approved these changes Apr 30, 2025

View reviewed changes

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 30, 2025

mattcaswell mentioned this pull request Jul 8, 2025

Test 25-test_verify.t fails in 3.5.1 on Windows #27984

Closed

mattcaswell mentioned this pull request Aug 7, 2025

Don't keep the store open in by_store_ctrl_ex #28198

Closed

mattcaswell mentioned this pull request Aug 29, 2025

Don't keep the store open in by_store_ctrl_ex - 3.0 #28385

Closed

Uh oh!

Comments

Conversation

levitte commented Apr 30, 2025

Uh oh!

levitte commented Apr 30, 2025 • edited by DDvO Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattcaswell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DDvO commented Apr 30, 2025

Uh oh!

DDvO commented Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

levitte commented Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

levitte commented Apr 30, 2025

Uh oh!

levitte commented Apr 30, 2025

Uh oh!

levitte commented Apr 30, 2025

Uh oh!

DDvO commented Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DDvO commented Apr 30, 2025

Uh oh!

levitte commented Apr 30, 2025

Uh oh!

levitte commented Apr 30, 2025

Uh oh!

t8m left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

levitte commented Apr 30, 2025

Uh oh!

DDvO commented May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

levitte commented May 5, 2025

Uh oh!

mattcaswell commented May 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

levitte commented Apr 30, 2025 •

edited by DDvO

Loading

DDvO commented Apr 30, 2025 •

edited

Loading

levitte commented Apr 30, 2025 •

edited

Loading

DDvO commented Apr 30, 2025 •

edited

Loading

DDvO commented May 5, 2025 •

edited

Loading