Certificate_Store_In_Memory, additional constructor?

[bazineta]

Certificate_Store_in_Memory has a convenience constructor:

      /**
      * Adds given certificate to the store.
      */
      explicit Certificate_Store_In_Memory(const X509_Certificate& cert);

I think this has the effect of signaling to the consumer that a CRL is optional, but if one's taking the policy defaults, it's not, and someone porting from ASIO's TLS can spend a lot of time in error 42 purgatory before realizing that with the default policies, it's not optional -- it can be empty, but it's not optional.

It might be helpful to have a second convenience constructor:

      /**
      * Adds given certificate and certificate revocation list (CRL) to the store.
      */
      explicit Certificate_Store_In_Memory(const X509_Certificate& cert, const X509_CRL& crl);

And modifying the comment in the first constructor to note that with default policies, the second constructor is probably the one you want. I think that as things stand now, the presence of the constructor that only takes a certificate, when combined with other documentation that speaks to the optionality of CRLs, is a bit misleading given how the policies work by default.

[bazineta]

So, here's the thing. I was looking through the TODOs at the bottom of the Certificate_Store_In_Memory class:

https://github.com/randombit/botan/blob/f7975f0dbbf71fd670141f5e02e2818566ec0789/src/lib/x509/certstor.h#L156

And I was thinking, well, the library does conditionally compile with boost, and there's multi-index containers in boost, it'd be simple to handle that with a multi-index container, could just check for boost being present, have an alternate version that'd solve those TODOs....

And then the light dawned that:

1. my use case is exactly one certificate and exactly one CRL
2. the TODOs were indicative that the class is more a demo of how things could be done, not necessarily how they had to be done
3. since I'm never storing more than one certificate and one CRL, I'd be better off just deriving my own class from Certificate_Store

That did turn out to be the short path, so probably best that I close this, but figured documenting the thought process was perhaps worthwhile.

[bazineta]

@randombit Jack, my single certificate / single CRL implementation has turned out well. I suspect this might be a reasonably common use case; we use it for private client / server TLS communications, where we generate a private CA and then issue certificates and keys from it, allowing our own clients and servers to communicate securely with one another.

Is this something you might be interested in a PR for? I can provide a gist if that'd be helpful to determine if it'd be more broadly useful, or too narrow in scope to want to consider for inclusion.

[reneme]

I'd be better off just deriving my own class from Certificate_Store

Frankly, I think that is indeed the most typical solution people land on, if they can't simply use the operating system's trust stores.

I agree that (1) this insight could be documented better and (2) it might be worthwhile to have convenient support for this exact use case where people just want to trust a single private CA or raw public key.

Are you using the ASIO wrapper, or the lower-level TLS classes in your application?

[bazineta]

Are you using the ASIO wrapper, or the lower-level TLS classes in your application?

I'm using the ASIO wrapper; this is a preexisting ASIO application that already used a lot of Botan support for custom stream encryption (embedded devices); TLS usage was our only hard dependency on ASIO's OpenSSL support remaining. Other than the task of building up the context, the ASIO wrapper is practically a drop-in replacement.

Additionally, while it's nice to have one less dependency, thread-safety of the ASIO TLS context is difficult to determine with certainty, while with Botan, it's explicit and simple to reason about. So while it does take a bit of time to knit together what's required for a context, once that's complete, the wrapper makes the change simple.

[bazineta]

it might be worthwhile to have convenient support for this exact use case where people just want to trust a single private CA or raw public key.

@reneme, I've put up a gist of the approach, perhaps that'll facilitate determining if something like this would be worth including, either as a function of the library, or as documentation.

https://gist.github.com/bazineta/823ff022084861f49fe0eec40fe5442a

The use case would be typically someone moving from boost.asio's OpenSSL-based TLS context, who has a private CA used to facilitate private client / server TLS, needs a server context equivalent to what they had previously, and would like a sample solution that addresses concurrency concerns for the typical 'small pool of ASIO worker threads' implementation.

Other than an exception on line 67 being specific to our use of exceptions (easy adjustment, that), I think the specific Certificate_Store is a generic solution for the use case that might be more broadly useful.

[reneme]

Thanks for the detailed gist. Your code looks good to me. Personally, I also felt the pain of the inexplicable "bad_certificate" error when trying to use the ASIO wrapper in some project of the first time.

Do you think #5143 helps as a first step?

[bazineta]

Do you think #5143 helps as a first step?

Yes, I think that would be helpful for the user converting from the base ASIO TLS support; the demo is setting them up with a good starting point that is likely to work out of the box, and if they proceed to a more strict policy, they'll know what they changed before the bad certificate errors start showing up. Knowing that it would have been specifically revocation checking, they'd be in a better place.