Skip to content

Comments

Make no-autoload-config option work again#21621

Closed
paulidale wants to merge 5 commits intoopenssl:masterfrom
paulidale:no-autoload
Closed

Make no-autoload-config option work again#21621
paulidale wants to merge 5 commits intoopenssl:masterfrom
paulidale:no-autoload

Conversation

@paulidale
Copy link
Contributor

@paulidale paulidale commented Aug 2, 2023

The no-autoload-config option stopped working after 1.1.1.
It should work still.

  • documentation is added or updated
  • tests are added or updated

@paulidale paulidale added branch: master Applies to master branch triaged: bug The issue/pr is/fixes a bug branch: 3.0 Applies to openssl-3.0 branch severity: regression The issue/pr is a regression from previous released version branch: 3.1 Applies to openssl-3.1 (EOL) tests: present The PR has suitable tests present labels Aug 2, 2023
@paulidale paulidale self-assigned this Aug 2, 2023
@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Aug 2, 2023
@paulidale paulidale added approval: review pending This pull request needs review by a committer approval: otc review pending labels Aug 2, 2023
@paulidale paulidale mentioned this pull request Aug 2, 2023
Closed
@evgenykotkov
Copy link

evgenykotkov commented Aug 2, 2023

Hi Paul!

Thanks for taking care of this regression. As a small side opinion: maybe introducing a helper such as ossl_ensure_config_loaded() would make the code easier to maintain?

This helper could be made no-op if OPENSSL_NO_AUTOLOAD_CONFIG is defined and would allow to:

  • Remove the potentially error-prone #if !defined … && !defined … conditions
  • Skip having to check for OPENSSL_NO_AUTOLOAD_CONFIG everywhere where it's needed now and in the future

@paulidale
Copy link
Contributor Author

paulidale commented Aug 3, 2023

@evgenykotkov, I did contemplate adding such a call but while making the changes it didn't seem worthwhile given the other code I could remove. Some of the other conditional checks looked like being expensive and they can also be avoided by this approach.

Still, such a call might be worth introducing at some point.

@paulidale
Copy link
Contributor Author

paulidale commented Aug 3, 2023

Ping @openssl/committers for second review please.

slontis
slontis reviewed Aug 3, 2023
View reviewed changes
slontis
slontis reviewed Aug 3, 2023
View reviewed changes
@slontis slontis 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 Aug 3, 2023
openssl-machine pushed a commit that referenced this pull request Aug 4, 2023
@paulidale
testutil: allow a failure return from setup_tests that doesn't print …
47429d9 
…help

Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Shane Lontis <[email protected]>
(Merged from #21621)

(cherry picked from commit badf3c1)
openssl-machine pushed a commit that referenced this pull request Aug 4, 2023
@paulidale
PBE test: load providers if auto config load is turned off
7bb4f22 
Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Shane Lontis <[email protected]>
(Merged from #21621)

(cherry picked from commit 52ea255)
openssl-machine pushed a commit that referenced this pull request Aug 4, 2023
@paulidale
provider test: don't run configuration based tests if configuration i…
234bb3e 
…sn't loaded

Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Shane Lontis <[email protected]>
(Merged from #21621)

(cherry picked from commit a9dde74)
openssl-machine pushed a commit that referenced this pull request Aug 4, 2023
@paulidale
test: skip FIPS config auto loading based tests if feature is disabled
4a4d84a 
Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Shane Lontis <[email protected]>
(Merged from #21621)

(cherry picked from commit 9a255aa)
openssl-machine pushed a commit that referenced this pull request Aug 4, 2023
@paulidale
no_autoload: make the no-autoload-config option work again.
cea699c 
Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Shane Lontis <[email protected]>
(Merged from #21621)

(cherry picked from commit cb8e641)
openssl-machine pushed a commit that referenced this pull request Aug 4, 2023
@paulidale
testutil: allow a failure return from setup_tests that doesn't print …
badf3c1 
…help

Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Shane Lontis <[email protected]>
(Merged from #21621)
openssl-machine pushed a commit that referenced this pull request Aug 4, 2023
@paulidale
PBE test: load providers if auto config load is turned off
52ea255 
Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Shane Lontis <[email protected]>
(Merged from #21621)
openssl-machine pushed a commit that referenced this pull request Aug 4, 2023
@paulidale
provider test: don't run configuration based tests if configuration i…
a9dde74 
…sn't loaded

Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Shane Lontis <[email protected]>
(Merged from #21621)
openssl-machine pushed a commit that referenced this pull request Aug 4, 2023
@paulidale
test: skip FIPS config auto loading based tests if feature is disabled
9a255aa 
Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Shane Lontis <[email protected]>
(Merged from #21621)
openssl-machine pushed a commit that referenced this pull request Aug 4, 2023
@paulidale
no_autoload: make the no-autoload-config option work again.
cb8e641 
Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Shane Lontis <[email protected]>
(Merged from #21621)
@paulidale
Copy link
Contributor Author

paulidale commented Aug 4, 2023

Merged to all three branches. OpenSSL 3.0 required a manual fix but it was fairly trivial.
Thanks for the reviews.

@paulidale paulidale closed this Aug 4, 2023
openssl-machine pushed a commit that referenced this pull request Aug 4, 2023
@paulidale
testutil: allow a failure return from setup_tests that doesn't print …
21f0c0d 
…help

Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Shane Lontis <[email protected]>
(Merged from #21621)

(cherry picked from commit badf3c1)
openssl-machine pushed a commit that referenced this pull request Aug 4, 2023
@paulidale
PBE test: load providers if auto config load is turned off
61eafa8 
Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Shane Lontis <[email protected]>
(Merged from #21621)

(cherry picked from commit 52ea255)
openssl-machine pushed a commit that referenced this pull request Aug 4, 2023
@paulidale
provider test: don't run configuration based tests if configuration i…
9d097d5 
…sn't loaded

Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Shane Lontis <[email protected]>
(Merged from #21621)

(cherry picked from commit a9dde74)
openssl-machine pushed a commit that referenced this pull request Aug 4, 2023
@paulidale
test: skip FIPS config auto loading based tests if feature is disabled
e0d1221 
Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Shane Lontis <[email protected]>
(Merged from #21621)

(cherry picked from commit 9a255aa)
openssl-machine pushed a commit that referenced this pull request Aug 4, 2023
@paulidale
no_autoload: make the no-autoload-config option work again.
386c4ed 
Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Shane Lontis <[email protected]>
(Merged from #21621)

(cherry picked from commit cb8e641)
vszakats added a commit to curl/curl-for-win that referenced this pull request Aug 7, 2023
@vszakats
openssl.sh: set no-autoload-config option [ci skip]
60c34ae 
Added in 2018-04-17 [1]. Released in v1.1.1. Broken since v3.x and
received a fix just recently [2].

[1]
openssl/openssl@dbabc86
openssl/openssl#5959

[2]
openssl/openssl@cb8e641
openssl/openssl#21621
@t8m
Copy link
Member

t8m commented Aug 7, 2023

I think this deserves a CHANGES.md entry.

xl32 pushed a commit to xl32/openssl that referenced this pull request Sep 29, 2023
@paulidale @xl32
testutil: allow a failure return from setup_tests that doesn't print …
78e1b85 
…help

Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Shane Lontis <[email protected]>
(Merged from openssl#21621)

(cherry picked from commit badf3c1)
xl32 pushed a commit to xl32/openssl that referenced this pull request Sep 29, 2023
@paulidale @xl32
PBE test: load providers if auto config load is turned off
8759eea 
Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Shane Lontis <[email protected]>
(Merged from openssl#21621)

(cherry picked from commit 52ea255)
xl32 pushed a commit to xl32/openssl that referenced this pull request Sep 29, 2023
@paulidale @xl32
provider test: don't run configuration based tests if configuration i…
3c33515 
…sn't loaded

Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Shane Lontis <[email protected]>
(Merged from openssl#21621)

(cherry picked from commit a9dde74)
xl32 pushed a commit to xl32/openssl that referenced this pull request Sep 29, 2023
@paulidale @xl32
test: skip FIPS config auto loading based tests if feature is disabled
af965bf 
Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Shane Lontis <[email protected]>
(Merged from openssl#21621)

(cherry picked from commit 9a255aa)
xl32 pushed a commit to xl32/openssl that referenced this pull request Sep 29, 2023
@paulidale @xl32
no_autoload: make the no-autoload-config option work again.
4f4b1ec 
Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Shane Lontis <[email protected]>
(Merged from openssl#21621)

(cherry picked from commit cb8e641)
@slontis slontis mentioned this pull request Dec 6, 2023
Closed
@paulidale paulidale deleted the no-autoload branch May 22, 2024 00:07
@vszakats vszakats mentioned this pull request May 29, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattcaswell mattcaswell mattcaswell approved these changes

@slontis slontis slontis approved these changes

Assignees

@paulidale paulidale

Labels

approval: done This pull request has the required number of approvals branch: master Applies to master branch branch: 3.0 Applies to openssl-3.0 branch branch: 3.1 Applies to openssl-3.1 (EOL) severity: fips change The pull request changes FIPS provider sources severity: regression The issue/pr is a regression from previous released version tests: present The PR has suitable tests present triaged: bug The issue/pr is/fixes a bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@paulidale @evgenykotkov @t8m @mattcaswell @slontis