Improve the documentation of cert path building and validation by DDvO · Pull Request #13735 · openssl/openssl

DDvO · 2020-12-23T22:34:35Z

This has been motivated initially by a recent question in the openssl-users mailing list.
Answering it, I found that some important points were missing from the documentation
of the certification path building and validation process, which I've added.

This also updates and strongly improves the documentation of trust anchors and the related concepts of "purpose", "use", and "trust" attributes
and moves documentation of EKU cert extension checking from openssl-x509.pod.in to openssl-verification-options.pod.

Still, there are many more details that would be good to add.

On this occasion, as motivated by #13735 (comment):

X509_verify_cert(): Take first element of untrusted list as default target certificate

Checklist

documentation is added or updated
tests are added or updated

doc/man1/openssl-verification-options.pod

crypto/x509/v3_purp.c

doc/man1/openssl-verification-options.pod

doc/man3/X509_STORE_CTX_new.pod

DDvO · 2021-01-02T22:22:08Z

~~The "check_update" CI failure is unrelated and should be handled elsewhere.~~
I've extended this PR as discussed in #7871; it is ready for further review.

vdukhovni

Not a thorough review, just a few nits from a drive-by read.

doc/man1/openssl-verification-options.pod

doc/man3/SSL_CTX_add1_chain_cert.pod

DDvO · 2021-01-04T07:54:09Z

I've just added the following (small) commits:

X509_STORE_CTX_new.pod and x509_vfy.h.in: rename some params for clarity, improve their doc
x509_vfy.c: Improve a couple of internally documenting comments
x509_trs.c: rename to x509_trust.c for clarity and correct comment in trust_compat()

DDvO · 2021-01-04T08:07:39Z

Thanks @vdukhovni for having a quick look
and sorry for all those typos (it was a long day yesterday). Fixed them.

DDvO · 2021-01-04T12:22:50Z

I've meanwhile updated and strongly improved the documentation of trust anchors.
This took me half a day because to this end had to understand at least to some level
the related concepts of "purpose", "use", and "trust" attributes, which are very poorly documented.
The text I've added/updated here is still not complete on these topics but should bring major improvements.

DDvO · 2021-01-06T12:52:23Z

Doing further improvements on the x509 app I just came across documentation of EKU cert extension checking in openssl-x509.pod.in.
Since this is actually of more general nature and makes much more sense in openssl-verification-options.pod I've moved it there.

DDvO · 2021-01-14T16:05:34Z

Are there any further review comments on this?

t8m · 2021-01-20T16:09:06Z

Needs rebase now

DDvO · 2021-01-21T07:58:33Z

Needs rebase now

Thanks for the notice; done.

t8m

Just some nits. Otherwise good work!

doc/man1/openssl-verification-options.pod

vdukhovni · 2021-01-25T18:32:40Z

Is it time yet to retire support for the "Netscape" certificate features? Perhaps this is all dead code that no users are relying on?
Are any CAs still creating this type of certificate?

richsalz · 2021-01-25T19:06:57Z

See #9247 #9242 #9241

Quoting a comment from #9241: "To quote email from Mozilla folks I have: "In general if there is an IETF PKIX replacement for a Netscape datatype, then you don't need to support the Netscape datatype. Therefore OpenSSL does not need to support Netscape cert types and Netscape cert sequence." "

DDvO · 2021-01-27T20:12:30Z

Good point that support for Netscape certs and the respective documentation should be removed.
This is certainly to be done in a separate PR.

DDvO · 2021-01-27T20:14:01Z

Just some nits. Otherwise good work!

Thanks @t8m. I've handled all items (as far as it makes sense within this PR).

crypto/x509/x509_vfy.c

doc/man3/X509_STORE_CTX_new.pod

vdukhovni · 2021-01-27T21:49:24Z

doc/man1/openssl-verification-options.pod

While you're here, perhaps rewrite the first (remaining) sentence. It is rather clumsy.

Indeed. I've polished this one and the following paragraph.

doc/man1/openssl-verification-options.pod

vdukhovni · 2021-01-27T21:53:18Z

doc/man1/openssl-verification-options.pod

I don't see that the subject key identifier is particularly special here. Perhaps you're thinking about checks whether the TA cert is self-signed, which may in some cases use the SKID to make that conclusion. If so, just say that OpenSSL will likely also check whether the TA cert is self-signed.

Self-signedness of TAs is usually not checked.
I referred to the SKID at this point for another reason.
I've made this more clear by:

OpenSSL checks the validity period of such certificates
and makes use of some further fields.
In particular, the subject key identifier extension, if present,
is used for matching trust anchors during chain building.

vdukhovni · 2021-01-27T21:55:02Z

doc/man1/openssl-verification-options.pod

This needs a bit more work. "... related to the purposes of using ..." is not particularly clear or well phrased.

This is indeed a bit unclear - precisely because, as I wrote, I am not fully sure about how purpose, EKUs, and trust attributes interrelate. Here is a 2nd try.

From the OpenSSL perspective, a trust anchor is a certificate
that should be augmented with an explicit designation for which
uses of a target certificate the certificate may serve as a trust anchor.
In PEM encoding, this is indicated by the C<TRUSTED CERTIFICATE> string.
Such a designation provides a set of positive trust attributes
explicitly stating trust for the listed purposes
and/or a set of negative trust attributes
explicitly rejecting the use for the listed purposes.
The purposes are encoded using the values defined for the extended key usages
(EKUs) that may be given in X.509 extensions of end-entity certificates.

vdukhovni · 2021-01-27T21:57:31Z

doc/man1/openssl-verification-options.pod

This makes it sound like certificates not in the trust store might be trusted just by matching a few fields of a trusted certificate.

I thought it was this way, but it I understand you correctly,
at least the intended semantics is as follows, and I've changed the text accordingly:

A certificate, which may be CA certificate or an end-entity certificate,
is considered a trust anchor for the given use
if and only if all the following conditions hold.

It is an an element of the trust store.

It does not have a negative trust attribute rejecting the given use.

...

doc/man1/openssl-x509.pod.in

doc/man3/SSL_CTX_load_verify_locations.pod

doc/man3/X509_STORE_CTX_new.pod

crypto/x509/x509_vfy.c

openssl-machine · 2021-05-05T00:07:11Z

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

DDvO · 2021-06-03T11:47:52Z

@openssl/otc, this PR (as well as a couple of other cert-related PRs of mine) have been stuck for many months due to missing further treatment by the OTC.

The relevance of improving the documentation of cert path validation, in particular on the details of trust vs. purpose, is exemplified by a recent discussion on https://www.mail-archive.com/[email protected]/msg89496.html.
Among others, the author of that thread complained, like I did more than a year ago, about the pretty tautological and thus useless "documentation" of X509_VERIFY_PARAM_set_trust():

X509_VERIFY_PARAM_set_trust() sets the trust setting in B<param> to B<trust>.

t8m · 2021-06-03T15:17:33Z

IMO, apart from the x509.h changes this would be mergeable after the beta release. It is also a quite big change to the documentation, so it will require some time to properly review and so it would be nice to be able to postpone this after beta. Unfortunately this is another example of PR that mixes multiple unrelated things into a single PR.

DDvO · 2021-06-03T15:22:40Z

Thanks for your response.
So when I find the time I will somehow split this PR in more digestable pieces.

DDvO · 2021-06-03T15:24:15Z

BTW, when is the beta release currently planned/expected?
Is it June 30th as indicated in the respective GitHub milestone label?

paulidale

Typographic and wording suggestions. This looks good.

include/openssl/x509_vfy.h.in

doc/man3/X509_STORE_CTX_new.pod

doc/man1/openssl-verification-options.pod

paulidale · 2021-06-04T02:37:56Z

doc/man1/openssl-verification-options.pod

Drop it is my vote.

openssl-machine · 2021-06-08T02:00:17Z

This pull request is ready to merge

Reviewed-by: Paul Dale <[email protected]> (Merged from #13735)

…ity, improve their doc Reviewed-by: Paul Dale <[email protected]> (Merged from #13735)

Reviewed-by: Paul Dale <[email protected]> (Merged from #13735)

This hopefully alleviates the fact that the name is unclear/misleading. Reviewed-by: Paul Dale <[email protected]> (Merged from #13735)

Reviewed-by: Paul Dale <[email protected]> (Merged from #13735)

DDvO · 2021-06-08T05:50:11Z

Merged. Glad this through, after long time in between.
Thanks to all who contributed and to @pauildale for giving the final thrust.

Reviewed-by: Paul Dale <[email protected]> (Merged from openssl#13735)

…ity, improve their doc Reviewed-by: Paul Dale <[email protected]> (Merged from openssl#13735)

Reviewed-by: Paul Dale <[email protected]> (Merged from openssl#13735)

This hopefully alleviates the fact that the name is unclear/misleading. Reviewed-by: Paul Dale <[email protected]> (Merged from openssl#13735)

Reviewed-by: Paul Dale <[email protected]> (Merged from openssl#13735)

DDvO added the approval: otc review pending label Dec 23, 2020

DDvO force-pushed the chain_build_verify_doc branch from 9df6957 to f5f63e0 Compare December 23, 2020 22:42

FdaSilvaYY reviewed Dec 24, 2020

View reviewed changes

doc/man1/openssl-verification-options.pod Outdated Show resolved Hide resolved

doc/man1/openssl-verification-options.pod Outdated Show resolved Hide resolved

kaduk reviewed Dec 28, 2020

View reviewed changes

DDvO mentioned this pull request Jan 2, 2021

Why is X509_V_FLAG_PARTIAL_CHAIN flag required, should it be default? #7871

Closed

DDvO force-pushed the chain_build_verify_doc branch from f191711 to 813f382 Compare January 4, 2021 07:15

vdukhovni reviewed Jan 4, 2021

View reviewed changes

DDvO mentioned this pull request Jan 5, 2021

[PKI meta] Cert path building and verification needs many further improvements #13780

Open

DDvO added this to the 3.0.0 milestone Jan 6, 2021

DDvO added the triaged: documentation The issue/pr deals with documentation (errors) label Jan 6, 2021

DDvO force-pushed the chain_build_verify_doc branch from 8d2ca69 to 9f5a89e Compare January 21, 2021 07:58

DDvO mentioned this pull request Jan 21, 2021

X509_print_ex(): Do not abort printing when notBefore/notAfter times are invalid #13714

Closed

t8m requested changes Jan 25, 2021

View reviewed changes

DDvO force-pushed the chain_build_verify_doc branch from 8be23b4 to 18a5006 Compare January 27, 2021 20:08

vdukhovni reviewed Jan 27, 2021

View reviewed changes

crypto/x509/x509_vfy.c Outdated Show resolved Hide resolved

vdukhovni reviewed Jan 27, 2021

View reviewed changes

doc/man3/X509_STORE_CTX_new.pod Outdated Show resolved Hide resolved

doc/man3/X509_STORE_CTX_new.pod Outdated Show resolved Hide resolved

doc/man3/X509_STORE_CTX_new.pod Outdated Show resolved Hide resolved

vdukhovni reviewed Jan 27, 2021

View reviewed changes

crypto/x509/x509_vfy.c Outdated Show resolved Hide resolved

DDvO force-pushed the chain_build_verify_doc branch from abe06de to 3900a21 Compare April 21, 2021 05:06

paulidale requested changes Jun 4, 2021

View reviewed changes

fixup! Improve the documentation of cert path building and validation

ea4486c

github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Jun 4, 2021

paulidale approved these changes Jun 7, 2021

View reviewed changes

paulidale added approval: done This pull request has the required number of approvals branch: master Applies to master branch and removed approval: otc review pending labels Jun 7, 2021

openssl-machine removed the approval: done This pull request has the required number of approvals label Jun 8, 2021

openssl-machine added the approval: ready to merge The 24 hour grace period has passed, ready to merge label Jun 8, 2021

openssl-machine pushed a commit that referenced this pull request Jun 8, 2021

x509_trs.c: rename to x509_trust.c and correct comment in trust_compat()

62d2386

Reviewed-by: Paul Dale <[email protected]> (Merged from #13735)

openssl-machine pushed a commit that referenced this pull request Jun 8, 2021

x509_vfy.c: Improve a couple of internally documenting comments

f1343f4

Reviewed-by: Paul Dale <[email protected]> (Merged from #13735)

openssl-machine pushed a commit that referenced this pull request Jun 8, 2021

X509_STORE_CTX_new.pod and x509_vfy.h.in: rename some params for clar…

2576b9c

…ity, improve their doc Reviewed-by: Paul Dale <[email protected]> (Merged from #13735)

openssl-machine pushed a commit that referenced this pull request Jun 8, 2021

Improve the documentation of cert path building and validation

f9ac6f6

Reviewed-by: Paul Dale <[email protected]> (Merged from #13735)

openssl-machine pushed a commit that referenced this pull request Jun 8, 2021

x509.h.in: extended 'documenting' comment on X509_TRUST_OK_ANY_EKU

3cd4e5e

This hopefully alleviates the fact that the name is unclear/misleading. Reviewed-by: Paul Dale <[email protected]> (Merged from #13735)

openssl-machine pushed a commit that referenced this pull request Jun 8, 2021

Move trust-related decls from x509.h.in to x509_vfy.h.in

09345c8

Reviewed-by: Paul Dale <[email protected]> (Merged from #13735)

DDvO closed this Jun 8, 2021

devnexen pushed a commit to devnexen/openssl that referenced this pull request Jul 7, 2021

x509_trs.c: rename to x509_trust.c and correct comment in trust_compat()

d3f3f7f

Reviewed-by: Paul Dale <[email protected]> (Merged from openssl#13735)

devnexen pushed a commit to devnexen/openssl that referenced this pull request Jul 7, 2021

x509_vfy.c: Improve a couple of internally documenting comments

3742c07

Reviewed-by: Paul Dale <[email protected]> (Merged from openssl#13735)

devnexen pushed a commit to devnexen/openssl that referenced this pull request Jul 7, 2021

X509_STORE_CTX_new.pod and x509_vfy.h.in: rename some params for clar…

5888684

…ity, improve their doc Reviewed-by: Paul Dale <[email protected]> (Merged from openssl#13735)

devnexen pushed a commit to devnexen/openssl that referenced this pull request Jul 7, 2021

Improve the documentation of cert path building and validation

ce51229

Reviewed-by: Paul Dale <[email protected]> (Merged from openssl#13735)

devnexen pushed a commit to devnexen/openssl that referenced this pull request Jul 7, 2021

Move trust-related decls from x509.h.in to x509_vfy.h.in

8b0c494

Reviewed-by: Paul Dale <[email protected]> (Merged from openssl#13735)

Uh oh!

Conversation

DDvO commented Dec 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DDvO commented Jan 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vdukhovni left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DDvO commented Jan 4, 2021

Uh oh!

DDvO commented Jan 4, 2021

Uh oh!

DDvO commented Jan 4, 2021

Uh oh!

DDvO commented Jan 6, 2021

Uh oh!

DDvO commented Jan 14, 2021

Uh oh!

t8m commented Jan 20, 2021

Uh oh!

DDvO commented Jan 21, 2021

Uh oh!

t8m left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vdukhovni commented Jan 25, 2021

Uh oh!

richsalz commented Jan 25, 2021

Uh oh!

DDvO commented Jan 27, 2021

Uh oh!

DDvO commented Jan 27, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DDvO commented Dec 23, 2020 •

edited

Loading

DDvO commented Jan 2, 2021 •

edited

Loading