Skip to content

Comments

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

Open
DDvO wants to merge 8 commits intoopenssl:masterfrom
siemens:add_missing_cert_store_URI_check
Open

OSSL_STORE, HTTP, apps/: fix URI scheme parsing and URI-related doc#27507
DDvO wants to merge 8 commits intoopenssl:masterfrom
siemens:add_missing_cert_store_URI_check

Conversation

@DDvO
Copy link
Contributor

@DDvO DDvO commented Apr 26, 2025

This fixes the remaining issues from #27461:

When registering a store URI or filename via SSL_CTX_set_default_verify_store(), X509_STORE_load_store{,_ex}(), or X509_LOOKUP_add_store{,_ex}(), which are used for the -CAstore etc. options of several apps:

  • At first (e.g., during app startup) the URI or filename is just registered but on actively used. So far, in contrast to the plain file and directory cases, no feedback is given the caller/user in case anything is wrong with the filename/URI.
  • When the URI or filename is actually used later to retrieve trust anchor certs, all errors about the URI and its contents get lost.
    There is just the very indirect feedback that not issuer cert could be found, but this is very blurry because this can also be due to many other reasons apart from an error related to the filename/URI.

The fix adds the missing check to by_store_ctrl_ex() for the case that the URI may be interpreted as a local file (path) name, i.e., if there is no scheme or the scheme is file:.
This motivated factoring out a new slightly generalized, corrected, and simplified function OSSL_file_stat() from file_store.c:file_open() and e_loader_attic.c:file_open_ex()

Fix the parsing of URI schemes in OSSL_parse_url(), OSSL_STORE_open_ex(), and ossl_store_register_loader_int(),
factoring out a new internal macro OSSL_SKIP_SCHEME().
So far, the check were too weak, allowing invalid chars and/or an empty scheme.

This caught an invalid scheme name in test/fake_rsa, which is now renamed to 'fake-rsa'.

Fix the documentation of the mentioned function and CLI option parameters,
for which so far it was not mentioned that they support not only URIs but also plain file (path) names,
and partly not mentioned that certs in those stores are loaded only on demand.

@DDvO DDvO 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 triaged: documentation The issue/pr deals with documentation (errors) branch: 3.0 Applies to openssl-3.0 branch tests: present The PR has suitable tests present branch: 3.2 Applies to openssl-3.2 (EOL) branch: 3.3 Applies to openssl-3.3 branch: 3.4 Applies to openssl-3.4 branch: 3.5 Applies to openssl-3.5 labels Apr 26, 2025
@github-actions github-actions bot added severity: fips change The pull request changes FIPS provider sources severity: ABI change This pull request contains ABI changes labels Apr 26, 2025
@DDvO DDvO force-pushed the add_missing_cert_store_URI_check branch from 059997e to f28af72 Compare April 26, 2025 13:53
@DDvO DDvO requested a review from Copilot April 26, 2025 13:53

This comment was marked as off-topic.

@DDvO
Copy link
Contributor Author

DDvO commented Apr 26, 2025

BTW, the recent relaxation in #27482 allowed for some further simplifications,
which I included in the carved out function OSSL_file_stat().

@DDvO
Copy link
Contributor Author

DDvO commented Apr 26, 2025

In partial contrast to what I had suggested yesterday,
in case two stat() calls are performed, this function retains any error queue entries for both of them,
in order to indicate on error all variants tried out. For instance, on

apps/openssl verify -CAstore file:non-existing -CAfile test/certs/root-cert.pem test/certs/root-cert.pem 

the output is

Error loading store URI file:non-existing
80CC5EF401000000:error:80000002:system library:OSSL_file_stat:No such file or directory:crypto/store/store_lib.c:1110:calling stat(file:non-existing)
80CC5EF401000000:error:80000002:system library:OSSL_file_stat:No such file or directory:crypto/store/store_lib.c:1150:calling stat(non-existing)

@DDvO DDvO requested review from levitte and t8m April 26, 2025 19:50
@DDvO DDvO removed branch: 3.0 Applies to openssl-3.0 branch branch: 3.2 Applies to openssl-3.2 (EOL) branch: 3.3 Applies to openssl-3.3 branch: 3.4 Applies to openssl-3.4 branch: 3.5 Applies to openssl-3.5 labels Apr 27, 2025
@DDvO DDvO force-pushed the add_missing_cert_store_URI_check branch 3 times, most recently from 676da4b to 0705b36 Compare April 28, 2025 06:22
@DDvO
Copy link
Contributor Author

DDvO commented Apr 28, 2025

Polished the extraction of the new helper function by slightly generalizing it and renaming it to OSSL_file_stat().

@levitte
Copy link
Member

levitte commented Sep 25, 2025

Apart from those little remarks, this looks fine. Sorry for taking so long to get back on this, @DDvO

@DDvO
Copy link
Contributor Author

DDvO commented Oct 1, 2025

Apart from those little remarks, this looks fine. Sorry for taking so long to get back on this, @DDvO

Pleased to hear. And sorry for slow responding this week, where I'm on vacation.
I've just done the changes requested.

@DDvO
Copy link
Contributor Author

DDvO commented Oct 1, 2025

Sigh, will need to provide extra PRs for backporting this to 3.4 and below.

@DDvO DDvO added branch: 3.6 Applies to openssl-3.6 and removed branch: 3.2 Applies to openssl-3.2 (EOL) branch: 3.3 Applies to openssl-3.3 branch: 3.4 Applies to openssl-3.4 branch: 3.0 Applies to openssl-3.0 branch labels Oct 1, 2025
@DDvO DDvO requested a review from levitte October 2, 2025 09:04
t8m
t8m previously approved these changes Oct 2, 2025
@DDvO
Copy link
Contributor Author

DDvO commented Oct 11, 2025

@levitte can you please approve/reconfirm.

@openssl-machine
Copy link
Collaborator

This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago

@openssl-machine
Copy link
Collaborator

This PR is in a state where it requires action by @openssl/committers but the last update was 61 days ago

@DDvO DDvO force-pushed the add_missing_cert_store_URI_check branch from 83792d7 to c7c05ae Compare January 5, 2026 17:36
@DDvO
Copy link
Contributor Author

DDvO commented Jan 5, 2026

Rebased on latest master and updated to clang-format.
For this reason, backporting to 3.5 (and 3.6) will require conflict resolution.

@DDvO
Copy link
Contributor Author

DDvO commented Jan 27, 2026

Rebased to fix merge conflicts after switch to clang-format

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approval: review pending This pull request needs review by a committer branch: master Applies to master branch severity: fips change The pull request changes FIPS provider sources tests: present The PR has suitable tests present triaged: bug The issue/pr is/fixes a bug triaged: documentation The issue/pr deals with documentation (errors)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants