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

Improve error diagnostics of X509V3 conf parsing, CMP, and other apps #12296

Closed

Conversation

@DDvO
Copy link
Contributor

@DDvO DDvO commented Jun 27, 2020

This fixes #12268
and various other small issues I recently found using the CMP CLI and other apps:

  • Error messages on parsing X.509v3 extension specs in config files where often weird. For instance for
extendedKeyUsage = clientAuth, serverAuth, wrong

the diagnostics was:

asn1 encoding routines:crypto/asn1/a_object.c:73:CMP error: first num too large
X509 V3 routines:crypto/x509/v3_extku.c:98:CMP error: invalid object identifier : section:<NULL>,name:unknown_key_usage_name,value:<NULL>
X509 V3 routines:crypto/x509/v3_conf.c:50:CMP error: error in extension : name=extendedKeyUsage, value=clientAuth, serverAuth, unknown_key_usage_name

With the fixes done here, this becomes:

object identifier routines:crypto/objects/obj_dat.c:376:CMP error: unknown object name
X509 V3 routines:crypto/x509/v3_extku.c:98:CMP error: invalid object identifier : unknown_key_usage_name
X509 V3 routines:crypto/x509/v3_conf.c:50:CMP error: error in extension : section=myKeyUsages, name=extendedKeyUsage, value=clientAuth, serverAuth, unknown_key_usage_name
  • Correct and extend the diagnostics of parse_name() in apps/lib/apps.c
  • Defer the so far premature diagnostic output of apps/cmp.c on server+proxy to be contacted.
apps/lib/apps.c Outdated Show resolved Hide resolved
crypto/objects/obj_dat.c Outdated Show resolved Hide resolved
crypto/x509/v3_cpols.c Outdated Show resolved Hide resolved
include/openssl/x509v3.h Outdated Show resolved Hide resolved
apps/lib/apps.c Outdated Show resolved Hide resolved
@DDvO
Copy link
Contributor Author

@DDvO DDvO commented Jul 1, 2020

Thanks @paulidale for reviewing this.
Dues this look okay now?

@DDvO
Copy link
Contributor Author

@DDvO DDvO commented Jul 1, 2020

The Travis builds have been queued, for whatever strange reason, for two days.
I've just rebased, squashed, and force-pushed on this PR; hope Travis will do better this time.

@DDvO DDvO force-pushed the mpeylo:improve_X509V3_conf_CMP_app_diagnostics branch to 8c5076f Jul 1, 2020
@DDvO
Copy link
Contributor Author

@DDvO DDvO commented Jul 3, 2020

@paulidale, can you still have a quick look at the changes you requested and my reaction and approve that before you leave?

In any case, have a nice and restful vacation!

@openssl-machine
Copy link

@openssl-machine openssl-machine commented Aug 3, 2020

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

Copy link
Contributor

@paulidale paulidale left a comment

LGTM. Apologies for the delay, another slipped through the cracks.

openssl-machine pushed a commit that referenced this pull request Aug 4, 2020
Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #12296)
openssl-machine pushed a commit that referenced this pull request Aug 4, 2020
Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #12296)
openssl-machine pushed a commit that referenced this pull request Aug 4, 2020
…ion:<NULL>' etc.

Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #12296)
openssl-machine pushed a commit that referenced this pull request Aug 4, 2020
…Y_NAME

Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #12296)
openssl-machine pushed a commit that referenced this pull request Aug 4, 2020
…s appropriate

Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #12296)
openssl-machine pushed a commit that referenced this pull request Aug 4, 2020
Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #12296)
openssl-machine pushed a commit that referenced this pull request Aug 4, 2020
Fixes #12268

Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #12296)
@DDvO
Copy link
Contributor Author

@DDvO DDvO commented Aug 4, 2020

Merged - thanks @paulidale

@DDvO DDvO closed this Aug 4, 2020
@DDvO DDvO deleted the mpeylo:improve_X509V3_conf_CMP_app_diagnostics branch Aug 4, 2020
swenkeratmicrosoft pushed a commit to swenkeratmicrosoft/openssl that referenced this pull request Sep 1, 2020
Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from openssl#12296)
swenkeratmicrosoft pushed a commit to swenkeratmicrosoft/openssl that referenced this pull request Sep 1, 2020
Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from openssl#12296)
swenkeratmicrosoft pushed a commit to swenkeratmicrosoft/openssl that referenced this pull request Sep 1, 2020
…ion:<NULL>' etc.

Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from openssl#12296)
swenkeratmicrosoft pushed a commit to swenkeratmicrosoft/openssl that referenced this pull request Sep 1, 2020
…Y_NAME

Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from openssl#12296)
swenkeratmicrosoft pushed a commit to swenkeratmicrosoft/openssl that referenced this pull request Sep 1, 2020
…s appropriate

Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from openssl#12296)
swenkeratmicrosoft pushed a commit to swenkeratmicrosoft/openssl that referenced this pull request Sep 1, 2020
Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from openssl#12296)
swenkeratmicrosoft pushed a commit to swenkeratmicrosoft/openssl that referenced this pull request Sep 1, 2020
Fixes openssl#12268

Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from openssl#12296)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants