Will happily look at this, already have a few questions. Will chime in later this week.

I think I have found it, the documentation isn't really good in this case for both SSL_CTX_load_verify_locations() and SSL_CTX_set_default_verify_paths(). If a hashed dir is passed it boils down to https://github.com/openssl/openssl/blob/ccaa754b5f66cc50d8ecbac48b38268e2acd715e/crypto/x509/x509_d2.c#L73-L76 where the manpage says:

X509_LOOKUP_add_dir() passes a directory specification from which certificates and CRLs are loaded on demand into the associated X509_STORE. type indicates what type of object is expected. This can only be used with a lookup using the implementation X509_LOOKUP_hash_dir(3).

Do you copy?

In D47433#1082177, @michaelo wrote:

I think I have found it, the documentation isn't really good in this case for both SSL_CTX_load_verify_locations() and SSL_CTX_set_default_verify_paths(). If a hashed dir is passed it boils down to https://github.com/openssl/openssl/blob/ccaa754b5f66cc50d8ecbac48b38268e2acd715e/crypto/x509/x509_d2.c#L73-L76 where the manpage says:

X509_LOOKUP_add_dir() passes a directory specification from which certificates and CRLs are loaded on demand into the associated X509_STORE. type indicates what type of object is expected. This can only be used with a lookup using the implementation X509_LOOKUP_hash_dir(3).

Do you copy?

If you are saying SSL_CRL_FILE and SSL_CRL_VERIFY are separate cases then yes. I'm unsure if they are mutually exclusive. Otherwise I'm unsure of your concern.

While testing this, do you intend to add a flag to fetch(1) as well? E.g., --crl-verify?

I have now played around with the patch and one of our intermediate CAs:

$ openssl s_client -connect dw-eng-rsc.innomotics.net:443
CONNECTED(00000003)
depth=2 C = DE, ST = Bayern, L = Muenchen, O = Siemens, serialNumber = ZZZZZZA1, OU = Siemens Trust Center, CN = Siemens Root CA V3.0 2016
verify return:1
depth=1 C = DE, ST = Bayern, L = Muenchen, O = Siemens, serialNumber = ZZZZZZE7, CN = Siemens Issuing CA Intranet Server 2022
verify return:1
depth=0 C = DE, O = Siemens, OU = IN HVM DW, CN = dw-eng-rsc.innomotics.net
verify return:1

It works with a CRL file as well as hashed in /etc/ssl/certs, but certctl(8) is totally unusable here (read broken), had to do it manually. Obtaining the CRLs from all CAs in that chain, convert from DER to text is a pain and technically needs to happen periodically. As being completely off topic: I consider CRLs as a total pain, cannot tell whether OCSP is any better, but that is a different discussion. I have only concerns with the verbose output which I will add inline.

In D47433#1082467, @michaelo wrote:

While testing this, do you intend to add a flag to fetch(1) as well? E.g., --crl-verify?

Happy to do that, but I wanted to avoid the work until this is in an acceptable stage. I think D47449 is an essential piece of the puzzle in real world use as well.

In D47433#1082468, @michaelo wrote:

I have now played around with the patch and one of our intermediate CAs:

$ openssl s_client -connect dw-eng-rsc.innomotics.net:443
CONNECTED(00000003)
depth=2 C = DE, ST = Bayern, L = Muenchen, O = Siemens, serialNumber = ZZZZZZA1, OU = Siemens Trust Center, CN = Siemens Root CA V3.0 2016
verify return:1
depth=1 C = DE, ST = Bayern, L = Muenchen, O = Siemens, serialNumber = ZZZZZZE7, CN = Siemens Issuing CA Intranet Server 2022
verify return:1
depth=0 C = DE, O = Siemens, OU = IN HVM DW, CN = dw-eng-rsc.innomotics.net
verify return:1

It works with a CRL file as well as hashed in /etc/ssl/certs, but certctl(8) is totally unusable here (read broken), had to do it manually. Obtaining the CRLs from all CAs in that chain, convert from DER to text is a pain and technically needs to happen periodically. As being completely off topic: I consider CRLs as a total pain, cannot tell whether OCSP is any better, but that is a different discussion. I have only concerns with the verbose output which I will add inline.

You know I'm all for improving certctl(8) long term. ;)

I do agree on the sentiment about how CRLs are put in the hands of the user and letting them deal with all of the complexity involved (i.e. try pulling a CRL off a public LDAP server with base tools). The OpenSSL verification side isn't too bad as testing showed but its use is limited. In our particular case the certification process for LINCE requires that updates over HTTPS are checked against existing CRLs which is achieved hereby mechanically -- but I should say pkg doesn't make this easy with its outdated embedded libfetch and then also straying from it with libcurl recently but that is a side quest.

OCSP is likely better but I haven't seen anything useful in libfetch so far and it's not part of any requirement on our side at the moment so that's another story entirely.

I see inconsistency in env vars and in output:

• Env vars: Those which are for verification are named SSL{,_NO}_VERIFY_{X} with a non-empty value. I'd expect here the same: SSL_VERIFY_CRL
• The output: "Using XXX" is used for concrete values, not for boolean flags like "Peer verification enabled". My understanding is that whether the CRL information comes from a custom bundle, default or an explicit CRL file, the library should print "Certificate revocation verification enabled":

Peer verification enabled
Using OpenSSL default CA cert file and path/Using CA....
Certificate revocation verification enabled

or

Peer verification enabled
Using OpenSSL default CA cert file and path/Using CA....
Using CRL file: ...
Certificate revocation verification enabled

WDYT?

In D47433#1082483, @michaelo wrote:

WDYT?

I'm ok with that, maybe with brevity in mind just this:

CRL verification enabled

But I don't mind either way.

Oh about SSL_VERIFY or SSL_CRL I'm not sure. Keeping it closer to SSL_CRL_FILE may be more beneficial also with SSL_CRL_OPTIONAL in mind later. Don't want these vars too long if it can be avoided and cluster all CRL into SSL_CRL prefix?

In D47433#1082484, @franco_opnsense.org wrote:

In D47433#1082483, @michaelo wrote:

WDYT?

I'm ok with that, maybe with brevity in mind just this:

CRL verification enabled

But I don't mind either way.

Like fine, but then CR, not CRL because we don't verify the list, do we? :-D Since it is a *verbose* flag I don't mind being verbose literally.

In D47433#1082486, @michaelo wrote:

Like fine, but then CR, not CRL because we don't verify the list, do we? :-D Since it is a *verbose* flag I don't mind being verbose literally.

Technically the list's signature and expiry is verified as well but we could also call it a "check" but then the env var should be renamed for clarity as well? Already expected the naming aspect of it to be difficult but I agree that it should be as good as it can be since it will likely stay that way.

In D47433#1082485, @franco_opnsense.org wrote:

Oh about SSL_VERIFY or SSL_CRL I'm not sure. Keeping it closer to SSL_CRL_FILE may be more beneficial also with SSL_CRL_OPTIONAL in mind later. Don't want these vars too long if it can be avoided and cluster all CRL into SSL_CRL prefix?

Well, then maybe SSL_VERIFY_CRL should not be boolean, but rather an enum? E.g, optional, yes, much like https://httpd.apache.org/docs/current/mod/mod_ssl.html#sslverifyclient because it the end it will require more and more flags. Default value would be none/NULL.

In D47433#1082488, @michaelo wrote:

Well, then maybe SSL_VERIFY_CRL should not be boolean, but rather an enum? E.g, optional, yes, much like https://httpd.apache.org/docs/current/mod/mod_ssl.html#sslverifyclient because it the end it will require more and more flags. Default value would be none/NULL.

Also doable, but personally I dislike the fuzzy matching on the value to act according to user (case sensitivity and ambiguity of yes and no etc and garbage input). The vars in libfetch are set and forget, if referencing a file or dir letting other parts deal with the complexity of the validation too.

In D47433#1082487, @franco_opnsense.org wrote:

In D47433#1082486, @michaelo wrote:

Like fine, but then CR, not CRL because we don't verify the list, do we? :-D Since it is a *verbose* flag I don't mind being verbose literally.

Technically the list's signature and expiry is verified as well but we could also call it a "check" but then the env var should be renamed for clarity as well? Already expected the naming aspect of it to be difficult but I agree that it should be as good as it can be since it will likely stay that way.

You convinced me by sticking to "CRL verification enabled" since the API says the same: https://docs.openssl.org/master/man3/X509_verify/. I'd stick and not really deviate from.

In D47433#1082489, @franco_opnsense.org wrote:

In D47433#1082488, @michaelo wrote:

Well, then maybe SSL_VERIFY_CRL should not be boolean, but rather an enum? E.g, optional, yes, much like https://httpd.apache.org/docs/current/mod/mod_ssl.html#sslverifyclient because it the end it will require more and more flags. Default value would be none/NULL.

Also doable, but personally I dislike the fuzzy matching on the value to act according to user (case sensitivity and ambiguity of yes and no etc and garbage input). The vars in libfetch are set and forget, if referencing a file or dir letting other parts deal with the complexity of the validation too.

I don't disagree, but introducing multiple vars for the same config isn't better either in my opinion. Consider you want to expose that to the CLI for fetch(1), do you want to introduce multiple switches?

In D47433#1082491, @michaelo wrote:

I don't disagree, but introducing multiple vars for the same config isn't better either in my opinion. Consider you want to expose that to the CLI for fetch(1), do you want to introduce multiple switches?

For historic context: right now handling of X509_V_ERR_UNABLE_TO_GET_CRL / D47449 is unconditional for OPNsense due to lack of the scope of this patch here. For FreeBSD inclusion I pondered the side effect of introducing this breaking standard verification behaviour of SSL_CRL_FILE and there it would also be beneficial.

If D47449 isn't a controversial addition I might better add it to this review in one batch, but we need to consider the side effect on SSL_CRL_FILE as well when folding the optional setting into SSL_CRL_VERIFY. According to OpenSSL it will try to find the most suitable CRL but if older CRLs are lingering in the system trust store than given in SSL_CRL_FILE an error might be raised for e.g. X509_V_ERR_CRL_HAS_EXPIRED but the overlap in CRL selection is speculation. I haven't tested interaction of both file and trust store (and maybe it's better discouraged anyway).

In D47433#1082495, @franco_opnsense.org wrote:

In D47433#1082491, @michaelo wrote:

I don't disagree, but introducing multiple vars for the same config isn't better either in my opinion. Consider you want to expose that to the CLI for fetch(1), do you want to introduce multiple switches?

For historic context: right now handling of X509_V_ERR_UNABLE_TO_GET_CRL / D47449 is unconditional for OPNsense due to lack of the scope of this patch here. For FreeBSD inclusion I pondered the side effect of introducing this breaking standard verification behaviour of SSL_CRL_FILE and there it would also be beneficial.

In my opinion, if done whisely, it won't break anything.

If D47449 isn't a controversial addition I might better add it to this review in one batch, but we need to consider the side effect on SSL_CRL_FILE as well when folding the optional setting into SSL_CRL_VERIFY. According to OpenSSL it will try to find the most suitable CRL but if older CRLs are lingering in the system trust store than given in SSL_CRL_FILE an error might be raised for e.g. X509_V_ERR_CRL_HAS_EXPIRED but the overlap in CRL selection is speculation. I haven't tested interaction of both file and trust store (and maybe it's better discouraged anyway).

First of all, an inconsistent CRL setup on the client isn't our concern. We cannot solve that. I still think that SSL_VERIFY_CRL=(require|optional|none) can also apply to an explicit CRL file, nothing is wrong with that. fetch --verify-crl=(require|optional|none) looks reasonable to me. I don't know who we could ad to the dicussion to broaden the discussion. @jrm , @emaste, any opinions here?

So I avoided the "none" case for lack of functionality and added the "leaf" (could call it "one") case for flexibility. "opt" or "optional" .. I'm not attached either way. I also added the error number to the default message which has bugged me for a while now while testing this and the optional message that a CRL was not provided now goes to stderr which attempts to make sure it is seen by the user. In the pkg case fetch_info appeared to be suppressed somewhere. What do you think? :)

FTR: https://httpd.apache.org/docs/2.4/mod/mod_ssl.html#sslcarevocationcheck with chain, leaf, none.

"all" to "chain" is also fine. We can do "none" but it will just add conditionals to the code and the libfetch style works with implicit defaults everywhere. Your choice :)

And it looks like "no_crl_for_cert_ok" is the "opt" part of this so it looks like the general direction is good.

Will look again next couple of days.