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

p11_child: add OCSP and CRL check ot the OpenSSL version #674

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@sumit-bose
Contributor

sumit-bose commented Oct 11, 2018

Two methods to check if certificates are revoked are added.

Since the two patches are only about certificate validation they can be tested
by calling p11child with the --verification and --certificate option to just
verify the given certificate. OCSP is used by default if there is the URL of a
responder in the certificate. the CRL check must be enabled by specifying a PEM
CRL file with the new crl_file sub-option.

Related to https://pagure.io/SSSD/sssd/issue/3489

}
if (p11_ctx->cert_verify_opts->ocsp_default_responder != NULL) {
url_str = p11_ctx->cert_verify_opts->ocsp_default_responder;

This comment has been minimized.

@jhrozek

jhrozek Oct 15, 2018

Contributor

Is it correct to skip the OCSP validation if the certificate has no OCSP URI defined in itself, but the ocsp_default_responder option is set?

goto done;
}
issuer_name = X509_get_issuer_name(cert);

This comment has been minimized.

@jhrozek

jhrozek Oct 15, 2018

Contributor

Would it make sense to check issuer_name for NULL or is NULL a valid value?

This comment has been minimized.

@sumit-bose

sumit-bose Oct 15, 2018

Contributor

Afaik, it should never happen that issuer_name is NULL here, but who knows, I added a check.

rv = BIO_do_connect(cbio);
if ((rv <= 0) && ((req_timeout == -1) || !BIO_should_retry(cbio))) {

This comment has been minimized.

@jhrozek

jhrozek Oct 15, 2018

Contributor

Why do we call should_retry if we never retry? Is it for future extension?

This comment has been minimized.

@sumit-bose

sumit-bose Oct 15, 2018

Contributor

I took inspiration from the related code from apps/ocsp.c form the OpenSSL source code. If I understood the man page correctly there might be cases when SSL is used where retry is returned but can be ignored.

STACK_OF(OPENSSL_STRING) *ocsp_urls = NULL;
char *url_str;
X509 *issuer = NULL;
int req_timeout = -1;

This comment has been minimized.

@jhrozek

jhrozek Oct 15, 2018

Contributor

I don't see where is req_timeout set, is all the handling for req_timeout != -1 for future extension?

This comment has been minimized.

@sumit-bose

sumit-bose Oct 15, 2018

Contributor

Yes, currently the PAM responder handles timeouts by just killing p11_child. But my plan is to add an option to sssctl to validate certificates and here I'd prefer to use a timeout instead of hard killing p11_child.

@jhrozek

Mostly LGTM, I just have a couple of questions

@jhrozek jhrozek self-assigned this Oct 15, 2018

@sumit-bose

This comment has been minimized.

Contributor

sumit-bose commented Oct 15, 2018

The new version fixes the two issues I mentioned above, fixed an issue with too strict option checking in the OpenSSL build and adds a test for the new crl_file suboption.

@jhrozek

LGTM. Coverity, CI and some basic tests need to be done before accepting.

sumit-bose added some commits Oct 10, 2018

p11_child: add crl_file option for the OpenSSL build
In the NSS build a Certificate Revocation List (CRL) can just be added
to the NSS database. For OpenSSL a separate file is needed.

Related to https://pagure.io/SSSD/sssd/issue/3489
@sumit-bose

This comment has been minimized.

Contributor

sumit-bose commented Oct 15, 2018

The latest version fixes some issues related to handle different OpenSSL versions. I must have run my OpenSSL-1.1 test on the wrong system where an older version was installed and I was misled by a typo in the OpenSSL documentation.

@jhrozek

This comment has been minimized.

Contributor

jhrozek commented Oct 15, 2018

OK, this builds even with a newer OpenSSL, looks like even in our internal CI.

@jhrozek jhrozek added the Accepted label Oct 15, 2018

@jhrozek

This comment has been minimized.

Contributor

jhrozek commented Oct 15, 2018

@jhrozek jhrozek closed this Oct 15, 2018

@jhrozek jhrozek added the Pushed label Oct 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment