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

chain_build(): Call verify_cb_cert() if a preliminary error has become final #14157

Closed
wants to merge 1 commit into from

Conversation

@DDvO
Copy link
Contributor

@DDvO DDvO commented Feb 11, 2021

In the context of #14094 the idea came up that it might be interesting to call the verification callback function (if provided) already during the chain construction phase of certificate verification when something is likely to go wrong.

A typical example is check_issued() in x509_vfy.c, which meanwhile can produce preliminary errors during chain building, such as X509_V_ERR_AKID_ISSUER_SERIAL_MISMATCH. At this point verify_cb_cert(ctx, x, ctx->error_depth, err) might be called, which not only would give an application the chance to learn about this specific problem but even to influence the chain building. Even without the callback being involved, the error may become final, may get overridden by some other error, or maybe the chain building succeeds on some other way such that in the end no error is thrown.

@t8m wrote about this:

The callback must be called only when the chain building process is coming to a dead end. It should not ever be called for any intermediate failures that can be overcome by considering further certificates in the untrusted or trusted set.

This sounds reasonable, but note that we already have the situation that the verification callback may be used to waive errors regarding certificate validity period restrictions:
X509_V_ERR_ERROR_IN_CERT_NOT_BEFORE_FIELD
X509_V_ERR_ERROR_IN_CERT_NOT_AFTER_FIELD
X509_V_ERR_CERT_NOT_YET_VALID
X509_V_ERR_CERT_HAS_EXPIRED

I tend to agree that any other errors such as the one mentioned above likely better should not be given to the verification callback. So I've modified my original proposal such that the callback is called only after chain building has failed and some preliminary error (except for the four ones regarding the certificate validity period restrictions) was thrown and has become final at this point. So the application can learn about the specific reason why chain building failed while so far this was possible only for the more general chain building errors:
X509_V_ERR_CERT_CHAIN_TOO_LONG
X509_V_ERR_DANE_NO_MATCH
X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT
X509_V_ERR_SELF_SIGNED_CERT_IN_CHAIN
X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT
X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY

This modified idea is implemented in this PR.

@kroeckx
Copy link
Member

@kroeckx kroeckx commented Feb 16, 2021

I'm not sure how useful this really is to applications. One of the problem is, as you say yourself, it will likely turn into a real error, but it might also just work. It's also not obvious what an application can do with it.

@DDvO
Copy link
Contributor Author

@DDvO DDvO commented Feb 17, 2021

Yes, but note that the very same is already done for other errors (due to cert validity period checking), so this PR brings consistency.~

@DDvO
Copy link
Contributor Author

@DDvO DDvO commented Feb 17, 2021

No, the adapted idea as implemented in this PR invokes the callback only after the error has become final.

@t8m
t8m approved these changes Feb 17, 2021
@openssl-machine
Copy link

@openssl-machine openssl-machine commented Feb 18, 2021

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

openssl-machine pushed a commit that referenced this pull request Feb 18, 2021
…e final

Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #14157)
@DDvO
Copy link
Contributor Author

@DDvO DDvO commented Feb 18, 2021

Merged - thanks @t8m

@DDvO DDvO closed this Feb 18, 2021
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.

None yet

4 participants