Skip to content

Conversation

levitte
Copy link
Member

@levitte levitte commented May 4, 2025

This is a backport of #27529 to OpenSSL 3.3

@levitte levitte added 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 branch: 3.3 Merge to openssl-3.3 labels May 4, 2025
@levitte levitte marked this pull request as draft May 4, 2025 08:14
@levitte
Copy link
Member Author

levitte commented May 4, 2025

Making this a draft for the moment. Need to check the test suite

levitte and others added 3 commits May 5, 2025 10:38
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 <david.von.oheimb@siemens.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#27529)

(cherry picked from commit 0c48ee2)
Originally from openssl#27507, with some
changes.

Co-authored-by: Richard Levitte <levitte@openssl.org>

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Richard Levitte <levitte@openssl.org>
(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 <david.von.oheimb@siemens.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#27529)

(cherry picked from commit af5952d)
@levitte
Copy link
Member Author

levitte commented May 5, 2025

I've just verified that this cherry-picks cleanly to OpenSSL 3.2

@levitte
Copy link
Member Author

levitte commented May 5, 2025

I've also verified that this cherry-picks cleanly to OpenSSL 3.0,
except for a trivial adjustment of the use plan => {nnn} line in
test/recipes/25-test_verify.t

@levitte levitte added branch: 3.0 Merge to openssl-3.0 branch branch: 3.2 Merge to openssl-3.2 style: waived exempted from style checks labels May 5, 2025
@levitte
Copy link
Member Author

levitte commented May 5, 2025

Style needed to be waived here too, for the same reason as in #27529

@levitte levitte marked this pull request as ready for review May 5, 2025 08:46
@levitte
Copy link
Member Author

levitte commented May 5, 2025

close/open to kick the workflows

@DDvO
Copy link
Contributor

DDvO commented May 16, 2025

I had a look at the changes and would have liked to approve them, but feel that I do not have sufficient familiarity with
crypto/x509/by_store.c to do so.
Therefore asking other @openssl/committers for 2nd approval.

@t8m t8m requested review from a team and mattcaswell May 16, 2025 09:15
Copy link
Member

@beldmit beldmit left a comment

Choose a reason for hiding this comment

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

LGTM

@beldmit beldmit 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 May 19, 2025
@openssl-machine 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 May 20, 2025
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@levitte
Copy link
Member Author

levitte commented May 20, 2025

Thank you @openssl-machine. Merging now

openssl-machine pushed a commit that referenced this pull request May 20, 2025
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

(cherry picked from commit 0c48ee2)

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #27551)
openssl-machine pushed a commit that referenced this pull request May 20, 2025
Originally from #27507, with some
changes.

Co-authored-by: Richard Levitte <levitte@openssl.org>

(cherry picked from commit 927deba)

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #27551)
openssl-machine pushed a commit that referenced this pull request May 20, 2025
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.

(cherry picked from commit af5952d)

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #27551)
openssl-machine pushed a commit that referenced this pull request May 20, 2025
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

(cherry picked from commit 0c48ee2)

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #27551)

(cherry picked from commit 08220ef)
openssl-machine pushed a commit that referenced this pull request May 20, 2025
Originally from #27507, with some
changes.

Co-authored-by: Richard Levitte <levitte@openssl.org>

(cherry picked from commit 927deba)

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #27551)

(cherry picked from commit 6143e70)
openssl-machine pushed a commit that referenced this pull request May 20, 2025
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.

(cherry picked from commit af5952d)

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #27551)

(cherry picked from commit 8bc0f67)
@levitte
Copy link
Member Author

levitte commented May 20, 2025

Merged

3.3:

08220ef Rework the "by store" X509_LOOKUP method to open the given URI early
6143e70 Add test_verify tests
8bc0f67 Drop "by store"'s by_store_subject_ex()

3.2:

1432e85 Rework the "by store" X509_LOOKUP method to open the given URI early
aed9fab Add test_verify tests
6a379f6 Drop "by store"'s by_store_subject_ex()

3.0:

340383f Rework the "by store" X509_LOOKUP method to open the given URI early
a468bdb Add test_verify tests
7141330 Drop "by store"'s by_store_subject_ex()

@levitte levitte closed this May 20, 2025
openssl-machine pushed a commit that referenced this pull request May 20, 2025
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

(cherry picked from commit 0c48ee2)

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #27551)

(cherry picked from commit 08220ef)
openssl-machine pushed a commit that referenced this pull request May 20, 2025
Originally from #27507, with some
changes.

Co-authored-by: Richard Levitte <levitte@openssl.org>

(cherry picked from commit 927deba)

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #27551)

(cherry picked from commit 6143e70)
openssl-machine pushed a commit that referenced this pull request May 20, 2025
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.

(cherry picked from commit af5952d)

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #27551)

(cherry picked from commit 8bc0f67)
@levitte levitte deleted the fix-27461-3.3 branch May 20, 2025 12:14
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: 3.0 Merge to openssl-3.0 branch branch: 3.2 Merge to openssl-3.2 branch: 3.3 Merge to openssl-3.3 style: waived exempted from style checks tests: present The PR has suitable tests present triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants