libfetch: allow use of SSL_CRL_VERIFY

[franco_opnsense.org]

Summary

Since the default store already points to /etc/ssl/certs and the
CRLs are hashed there too it is trivial to bring libfetch applications
to verifying the CRLs contained when doing a SSL connection.

[franco_opnsense.org]

lib/libfetch/common.c

1065

can probably avoid this variable since we only care about existence of the env var

[michaelo]

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

[franco_opnsense.org]

remove unused variable

[michaelo]

lib/libfetch/common.c

1095

Let me try to understand this because it partially dupliates lines 1107 to 1109: If Some trust store has been built, either default or custom and this env var is set you request the security context to read the CRL from disk/memory and enforce it for the entire chain.
As far as I understand this can only apply if:

• an explicit CRL file has been loaded: none so far since SSL_CTX_load_verify_locations() only load BEGIN/END CERTIFICATE. Unforatunately, the manpage of SSL_CTX_load_verify_locations() does not mention CRLs either.
• the default store contains any, but then this should be part of 1085, no?

My problem at the moment is that where is X509_LOOKUP_hash_dir() applied for the default case.

In general: What will happen if this is always enabled?

[michaelo]

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?

[franco_opnsense.org]

lib/libfetch/common.c

1095

an explicit CRL file has been loaded: none so far since SSL_CTX_load_verify_locations() only load BEGIN/END CERTIFICATE. Unforatunately, the manpage of SSL_CTX_load_verify_locations() does not mention CRLs either.

It does start reaching for CRLs in the default locations, SSL_CA_CERT_PATH and even SSL_CA_CERT_FILE (tested this today). You can witness this easily by enabling the flag: CLR verification immediately kicks in and fails with X509_V_ERR_UNABLE_TO_GET_CRL until you add the CRLs there which then either verifies fine or throws other CRL related errors such es signature mismatch or CRL expired.

the default store contains any, but then this should be part of 1085, no?

No, because the user needs to ensure CRLs have been added there first.

My problem at the moment is that where is X509_LOOKUP_hash_dir() applied for the default case.

Not sure I admit. Though we have tested and verified this with an external lab and this behavior is derived in part from Syslog-ng crl-dir() directive and it works as expected in CRL usage situations.

In general: What will happen if this is always enabled?

Hard verification failures mainly through X509_V_ERR_UNABLE_TO_GET_CRL -- I've been trying to address with with https://reviews.freebsd.org/D47449 but for other reasons than trying to get CRL verification enabled by default. There is too much burden on the user side to allow for generic CRL use (starting with the ability to find and fetch CRLs and hash them into the trust store).

[franco_opnsense.org]

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.

[michaelo]

lib/libfetch/common.c

1095

I somewhat fail to see where the definition says that X509_LOOKUP_load_file_ex() will load both certs *and* CRLs. At least on the source code I cannot see it, opposed to hashed dir.

[franco_opnsense.org]

lib/libfetch/common.c

1095

Real world test:

# grep BEGIN.X /etc/ssl/cert.pem
-----BEGIN X509 CRL-----
-----BEGIN X509 CRL-----
-----BEGIN X509 CRL-----
# env SSL_CA_CERT_FILE=/etc/ssl/cert.pem SSL_CRL_VERIFY=yes fetch -v https://pkg.opnsense.org
resolving server address: pkg.opnsense.org:443
SSL options: 82004850
Peer verification enabled
Using CA cert file: /etc/ssl/cert.pem
Using CRL verify: yes
Verify hostname
TLSv1.3 connection established using TLS_AES_256_GCM_SHA384
Certificate subject: /CN=pkg.opnsense.org
Certificate issuer: /C=BE/O=GlobalSign nv-sa/CN=GlobalSign GCC R3 DV TLS CA 2020
requesting https://pkg.opnsense.org/
local size / mtime: 3561 / 1730814854
remote size / mtime: 3561 / 0
pkg.opnsense.org                                      3561  B   21 MBps    00s

vs. removing one of the relevant CRLs

# grep BEGIN.X /etc/ssl/cert.pem
-----BEGIN X509 CRL-----
-----BEGIN X509 CRL-----
# env SSL_CA_CERT_FILE=/etc/ssl/cert.pem SSL_CRL_VERIFY=yes fetch -v https://pkg.opnsense.org
resolving server address: pkg.opnsense.org:443
SSL options: 82004850
Peer verification enabled
Using CA cert file: /etc/ssl/cert.pem
Using CRL verify: yes
Certificate verification failed for /C=BE/O=GlobalSign nv-sa/CN=GlobalSign GCC R3 DV TLS CA 2020
0020E16209360000:error:0A000086:SSL routines:tls_post_process_server_certificate:certificate verify failed:/usr/src/crypto/openssl/ssl/statem/statem_clnt.c:1890:
fetch: https://pkg.opnsense.org: Authentication error

[michaelo]

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

[michaelo]

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.

[franco_opnsense.org]

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.

[franco_opnsense.org]

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.

[michaelo]

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?

[franco_opnsense.org]

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.

[franco_opnsense.org]

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?

[michaelo]

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.

[franco_opnsense.org]

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.

[michaelo]

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.

[franco_opnsense.org]

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.

[michaelo]

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.

[michaelo]

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?

[franco_opnsense.org]

• lib/libfetch: feedback on previous

[franco_opnsense.org]

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).

[michaelo]

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?

[franco_opnsense.org]

• libfetch: rewrite SSL_CRL_VERIFY behaviour
• libfetch: add the error number to verify callback failure case
• libfetch: wording
• libfetch: redo docs

[franco_opnsense.org]

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? :)

[michaelo]

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

[franco_opnsense.org]

"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.

[franco_opnsense.org]

• libfetch: shuffle SSL_CRL_VERIFY options

[michaelo]

Will look again next couple of days.