randombit
Improve RFC 3779 extension api
#4699 added x509 extensions for RFC 3779. While working on #4877 I realized that the api for actually using them is kind of horrible.
I'm not sure why we didn't do it this way in the first place, sorry about that. I think doing it this way is way nicer for the user, because they don't need to understand the semantic of a random std::nullopt somewhere, they can just say "please inherit". I'm still a little unsure how to do it cleanly for the IPAddressBlocks extension with its high usage of templates, but I'll see what comes up.
coverage: 90.627% (+0.05%) from 90.576% when pulling 691bc671f8d81bb2b350b443763dd5a08f24223a on arckoor:improve-rfc3779-api into 93e9e1d365f52407821711da60a82e31e84345ce on randombit:master.
@randombit I'm currently redoing all the tests, and I can test the encode by comparing the expected bits to cert.v3_extensions().get_extension_bits(IPAddressBlocks::static_oid()).
Can I somehow do something similar for decoding? I'd like to give it a DER hex string / vec<uint8_t> and have it decode that into an extension (or fail), but all the relevant functionality that I could find, that is the extensions decode_inner() function as well as all the Extensions:: functions, are private. Is there a way I can get around that?
Of course I'd much prefer to just unit test the encoding / decoding functions of the individual classes, but that runs into the same private function problems.
The API I'm mostly happy with now, though I'm still debating whether there should be helper methods for things like "inherited", or if the user should interpret the std::nullopt themselves. As for the tests, they're better now, they test encoding and decoding separately, but the decode side could be much better with proper access to the decoding methods.
I'll leave it as it is for now, maybe someone has better ideas.
@arckoor Thanks I'll take a look asap. Can you please start by rebasing against latest master? There has been recently introduced some new checks in CI (most notably clang-tidy runs against headers now, while previously it only ran on cpp files)
:thinking: clang-tidy doesn't seem to check the x509_ext.h file. I also tried running it locally, same thing, but maybe I'm also just doing it wrong / missing something
@randombit can I please get a review of this? The next release is drawing closer, and we need to either merge this or revert #4699. We cannot leave it as is.
@randombit fair points, I'll add the old constructors back in, but I'd really like to keep even the mutable methods like restrict_*, without them FFI would be very painful. In #4877 I already encountered the "extension should not be mutable when coming from a cert"-problem, and tried to work around it with a self.__writable: bool on the python side. Can I introduce a m_writable that gets set when decoding, and it just throws when you try to write to it anyway?
but I'd really like to keep even the mutable methods like restrict_*, without them FFI would be very painful.
Yes that's fine.
Can I introduce a m_writable that gets set when decoding, and it just throws when you try to write to it anyway?
That would work but perhaps that's not necessary in practice. In decoding , Extensions returns only a const pointer from its getters, so these mutating functions can't be called in any case.
@randombit should be ready to take a look again. The only changed test I kept as it was is the decode test, since I find straight values more easily readable than hex. It also tests something different, since the logic was just wrong there.
~~Re the m_writable: yes in cpp it's not a problem, but the FFI can't know if the ext comes from being freshly constructed or from a cert, when a user gets an ext from a cert I have to copy the entire ext, because I can't guarantee that the user will destroy the ext before destroying the cert.
So I have an intended read-only ext, that is also writable. It of course won't change the cert it belongs to if you write to it, but is still kind of a footgun if the user expects to be able to modify the cert somehow. I can enforce the read-only-ness at the client, that is the python / rust binding, or I can enforce it in cpp, with an m_writable for IPAddressBlocks / ASBlocks. Whatever you prefer~~ Nevermind that, I'll just wrap it in a custom ffi struct, I forgot I could do that