Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

APPS load_key_certs_crls(): Make file access errors much more readable #16452

Closed
wants to merge 3 commits into from

Conversation

DDvO
Copy link
Contributor

@DDvO DDvO commented Aug 27, 2021

Since the use of OSSL_STORE for file loading in apps, the output in case of file access errors has become needlessly bulky and hard to read, for instance:

apps/openssl x509 -in WRONG
Could not open file or uri for loading certificate from WRONG
00517F71887F0000:error:16000069:STORE routines:ossl_store_get0_loader_int:unregistered scheme:crypto/store/store_register.c:237:scheme=file
00517F71887F0000:error:80000002:system library:file_open:No such file or directory:providers/implementations/storemgmt/file_store.c:269:calling stat(WRONG)
Unable to load certificate

It turns out that the first error queue entry is spurious, so this PR prunes it.
The second entry is quite informative, but not nice to read for app users,
so I've made sure that the errno string is printed directly instead.
This way, the output on the above app invocation becomes

Could not open file or uri for loading certificate from WRONG: No such file or directory

@DDvO DDvO added approval: otc review pending This pull request needs review by an OTC member triaged: bug The issue/pr is/fixes a bug triaged: refactor The issue/pr requests/implements refactoring labels Aug 27, 2021
@DDvO
Copy link
Contributor Author

DDvO commented Aug 27, 2021

Doing the latter improvement in apps/lib/app.c, I found that #15670 had introduced a pretty ugly way of suppressing DH parameter loading errors.
This PR changes this to a much less invasive way of doing this: temporarily unset the bio_err variable.

@t8m
Copy link
Member

t8m commented Aug 30, 2021

Doing the latter improvement in apps/lib/app.c, I found that #15670 had introduced a pretty ugly way of suppressing DH parameter loading errors.
This PR changes this to a much less invasive way of doing this: temporarily unset the bio_err variable.

I have to disagree that your way is less ugly. 😁

@t8m t8m added the branch: master Merge to master branch label Aug 30, 2021
@t8m
Copy link
Member

t8m commented Aug 30, 2021

IMO this is not something that should be merged before 3.0 release.

@DDvO
Copy link
Contributor Author

DDvO commented Aug 30, 2021

Doing the latter improvement in apps/lib/app.c, I found that #15670 had introduced a pretty ugly way of suppressing DH parameter loading errors.
This PR changes this to a much less invasive way of doing this: temporarily unset the bio_err variable.

I have to disagree that your way is less ugly. grin

Why? It takes much less code than adding a wrapper function and making the various print function calls explicitly dependent on an explicit suppress_decode_errors parameter.

@DDvO
Copy link
Contributor Author

DDvO commented Aug 30, 2021

IMO this is not something that should be merged before 3.0 release.

I advocate for merging this before the release because it corrects issues introduced with OSSL_STORE in 3.0.

@DDvO DDvO added this to the 3.0.0 milestone Aug 30, 2021
@t8m t8m added the branch: 3.0 Merge to openssl-3.0 branch label Sep 7, 2021
@t8m t8m removed this from the 3.0.0 milestone Sep 7, 2021
@t8m t8m added this to the Post 3.0.0 milestone Sep 24, 2021
@DDvO DDvO force-pushed the fix_apps_file_load_errors branch from 50ab00e to 6756f5e Compare January 4, 2022 16:27
@DDvO
Copy link
Contributor Author

DDvO commented Jan 4, 2022

Also handling of this PR should be resumed, since we are after the 3.0 release.

@t8m t8m removed the branch: 3.0 Merge to openssl-3.0 branch label Jan 4, 2022
Copy link
Contributor

@paulidale paulidale left a comment

Choose a reason for hiding this comment

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

Just one style nit.

apps/lib/apps.c Show resolved Hide resolved
@t8m t8m added triaged: feature The issue/pr requests/adds a feature and removed triaged: bug The issue/pr is/fixes a bug labels Jan 6, 2022
openssl-machine pushed a commit that referenced this pull request Jan 7, 2022
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #16452)
openssl-machine pushed a commit that referenced this pull request Jan 7, 2022
This reverts part of commit ef04491 using a less invasive suppression.

Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #16452)
@DDvO
Copy link
Contributor Author

DDvO commented Jan 7, 2022

Merged - thanks @paulidale

@DDvO DDvO closed this Jan 7, 2022
t8m pushed a commit to t8m/openssl that referenced this pull request Nov 2, 2022
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#16452)

(cherry picked from commit 7c64ca7)
t8m pushed a commit to t8m/openssl that referenced this pull request Nov 2, 2022
This reverts part of commit ef04491 using a less invasive suppression.

Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#16452)

(cherry picked from commit 6e24994)
openssl-machine pushed a commit that referenced this pull request Nov 9, 2022
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #16452)

(cherry picked from commit 7c64ca7)
Reviewed-by: Hugo Landau <hlandau@openssl.org>
openssl-machine pushed a commit that referenced this pull request Nov 9, 2022
This reverts part of commit ef04491 using a less invasive suppression.

Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #16452)

(cherry picked from commit 6e24994)
Reviewed-by: Hugo Landau <hlandau@openssl.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: otc review pending This pull request needs review by an OTC member branch: master Merge to master branch triaged: feature The issue/pr requests/adds a feature triaged: refactor The issue/pr requests/implements refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants