strongSwan: parse/validate X.509 objects in strongSwan's Botan plugin

[reneme]

StrongSwan contains a plugin for using Botan as a crypto provider for quite some time already. We're currently working on extending this plugin with functionality for parsing/validating X.509 certificates and CRLs. To access the relevant bits in Botan, a number of FFI functions have to be added. This issue is meant to provide an overview of the work in progress.

In the best-case scenario we'd like to have most of these additions available in the upcoming Botan 3.11.0 release so that we can start integrating this into strongSwan upstream. Locally, we already have a working prototype.

Required new Functionality in the FFI

• querying various data fields from certificates and CRLs (as applicable)
  (made available via FFI: Generic getter(s) for X.509 objects #5188)
  • serial number
  • subject/issuer DN as DER encoding
  • subject/issuer key identifiers
  • subject public key as PKCS#8 container
  • "to be signed" data for signature validation
  • the object's signature as raw bits
  • the signature algorithm identifier as DER encoding
  • CRL distribution point URLs
  • OCSP responder URLs (see also X509: Multiple OCSP Responders #5231)
• basic/extended constraints
  (made available via FFI: Allow querying more X.509 Certificate Constraints #5221)
  • is_CA bit
  • path length constraint
  • extended key usage bits
• subject/issuer alternative names
  (see FFI: Query Certificate Subject/Issuer Alternative Names and Name Constraints #5217)
• name constraint definitions
  (see FFI: Query Certificate Subject/Issuer Alternative Names and Name Constraints #5217)
• IP address range information (from the IPAddrBlock extension)
  (see below for some existing unmerged code for this)
• CRL validity interval (this update, next update)
  (see FFI: Allow querying validity interval of X.509 CRLs #5222)
• enumeration of individual CRL entries
  (see FFI: Enumeration of entries in an X.509 CRL #5220)

Pull Requests related to this

• FFI: Generic getter(s) for X.509 objects #5188
• Fix Inconsistency of X509 Name Constraint Matching #5189
• FFI: Query Certificate Subject/Issuer Alternative Names and Name Constraints #5217
• FFI: Enumeration of entries in an X.509 CRL #5220
• FFI: Allow querying more X.509 Certificate Constraints #5221
  • FIX: botan_x509_cert_is_ca() should return "1" for true #5236
• FFI: Allow querying validity interval of X.509 CRLs #5222
• X509: Multiple OCSP Responders #5231
• FFI: add *_count() functions for some FFI enumerators #5232

Currently Known Limitations

• IPv6 addresses
  Botan currently doesn't support handling IPv6 addresses in subject/issuer alternative names as well as name constraints. Any such IPv6 address entries would currently not be visible to strongSwan when using the Botan plugin
• Delta-CRLs
  To the best of my understanding Botan currently does not provide support for Delta CRLs. Therefore, we won't be able to support those via the strongSwan plugin either.
• OCSP Response/Request
  We don't require any OCSP-based certificate revocation checks at the moment. Integration with Botan's OCSP functionality is left for future work

/cc @tobiasbrunner

[randombit]

Re IPv6 names and Delta CRLs you are correct these are not currently supported, I'd be open to adding support if needed.

Re the currently open PRs I owe you some reviews but I don't think there are any showstoppers that would prevent these being merged in time for 3.11

[reneme]

I owe you some reviews but I don't think there are any showstoppers that would prevent these being merged in time for 3.11

These PRs don't really build on top of each other, because they (mostly) add independent functionality to the FFI (+ their tests, + their documentation). Though, almost certainly they'll cause massive merge conflicts when trying to merge them one by one.

Let's review/iterate on each of them independently and once they are all green-lighted I'm going to integrate them in one session instead of re-rebasing them again and again.

[arckoor]

Btw, the getters for the IpAddrBlocks ext I've already written once
They probably need some cleanup / adaption to the new "entry style" ffi, but perhaps you can reuse at least some parts.

May I also humbly request that functions iterating over "something" include a way to figure out how many entries there are? (i.e. some *_get_count or similar)
I will probably be the one to implement this in botan-rs at some point, and the error codes are not very ergonomic:

pub fn issuer_dn(&self, key: &str) -> Result<Vec<String>> {
    let key = make_cstr(key)?;
    let mut entries = Vec::new();
    while true { // not a fan of while true loops tbh
        let res: Result<String, Error> = call_botan_ffi_returning_string(0, &|out_buf, out_len| unsafe {
            botan_x509_cert_get_issuer_dn(self.obj, key.as_ptr(), i, out_buf, out_len)
        });

        match res {
            Ok(item) => entries.push(item),
            Err(err) => { // we got an error
                if (err.error_type() == ErrorType::BadParameter) { // it's an out of range error, so success
                    return Ok(entries);
                } else {
                    return Err(err); // something else, propagate the error
                }
            }
        }
    }
}

That's a getter that functions similarly, only here it's worse, because it doesn't even have the out of range error, it's just BadParameter
I have to while true, and explicitly unpack the error everytime to check if it's "the correct one", and else repack the error to throw
if it had a _count method, it would look like so:

pub fn issuer_dn(&self, key: &str) -> Result<Vec<String>> {
    let mut count = 0;
    let key = make_cstr(key)?;
    botan_call!(
        botan_x509_cert_get_issuer_dn_count,
        self.obj,
        key.as_ptr(),
        &mut count
    )?; // how many are there?
    let mut entries = Vec::new();
    for i in 0..count {
        let item = call_botan_ffi_returning_string(0, &|out_buf, out_len| unsafe {
            botan_x509_cert_get_issuer_dn(self.obj, key.as_ptr(), i, out_buf, out_len)
        })?; // ? means propagate the error - if this errors, something is wrong, since the index must be valid
        entries.push(item);
    }
    Ok(entries)
}

Which I think is much nicer, and much more obvious to others. It doesn't even really incur a penalty, because it makes the same number of calls, just that here the extra call is the first and not the last (and doesn't error)

[reneme]

Btw, the getters for the IpAddrBlocks ext I've already written once
They probably need some cleanup / adaption to the new "entry style" ffi, but perhaps you can reuse at least some parts.

Thanks a lot! We'll hopefully be able to integrate that before the 3.11.0 release.

May I also humbly request that functions iterating over "something" include a way to figure out how many entries there are?

I appreciate the feedback on this! In fact, I feared that "just the error code" would be too implicit. It does work nicely for strongSwan, but they use a generic "enumerator" idiom all over the code base that also just "enumerates until the end" without necessarily knowing how much there will be. I'll look into it next week.

[arckoor]

Just discovered that #5217 and #5232 lead to not being able to build botan without x509 (but with ffi) anymore:

./configure --disable=modules=x509
make cli -j`nproc`
...
src/lib/ffi/ffi_cert.cpp:384:86: error: ‘Botan::GeneralName’ has not been declared
  384 | std::optional<botan_x509_general_name_types> to_botan_x509_general_name_types(Botan::GeneralName::NameType gn_type) {
      |                                                                                      ^~~~~~~~~~~
...
src/lib/ffi/ffi_cert.cpp:616:44: error: ‘AlternativeName’ in namespace ‘Botan’ does not name a type
  616 | size_t count_general_names_in(const Botan::AlternativeName& alt_names) {
      |                                            ^~~~~~~~~~~~~~~

The namespaces are missing guards @reneme. Maybe we could make a big namespace at the top, I'm introducing one for #5166 too, so it might make sense to combine them all into one instead of having them sprinkled in between the C code?