Improve RFC 3779 extension api

[arckoor]

#4699 added x509 extensions for RFC 3779. While working on #4877 I realized that the api for actually using them is kind of horrible.

[arckoor]

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.

[coveralls]

coverage: 90.627% (+0.05%) from 90.576%
when pulling 691bc67 on arckoor:improve-rfc3779-api
into 93e9e1d on randombit:master.

[arckoor]

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

[arckoor]

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.

[randombit]

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

[arckoor]

🤔 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

[arckoor]

@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]

@arckoor Thanks for the ping I'll review soon

[randombit]

@arckoor So the changes to x509_ext.{h,cpp} seem basically fine but tbh this PR is very hard to review because it both changes the API and modifies the tests both in terms of API usage but also what is being tested. The diff is basically unreadable.

I can see how for heavy usage the new API might be preferable; you are obviously more invested in this being ergonomic than I am. That said I generally prefer avoiding mutable interfaces (eg how restrict_rdi deletes previously entered state) especially since with X.509 extensions the only time the data is really (logically) mutable is during creation rather than after.

So I'd propose this

• You removed or made private various constructors (eg ASBlocks) that had a fairly verbose but IMO usable interface. Bring these back.
• Don't remove any of the existing tests (using the old, currently removed, interfaces).
• Add new tests using your new interface

That admittedly leaves us with two slightly different ways of accomplishing creating an ASBlocks but TBH given the choice between the two I would prefer the verbose way of creating ASIdentifier list. And it should make then the diff comprehensible/reviewable.

[arckoor]

@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?

[randombit]

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.

[arckoor]

@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

[randombit]

This looks good to me. Can you clean up the commit history? Just squash to 1 if you like or try to split into API improvements vs new tests, however you prefer.

[randombit]

There is also a merge conflict

[Copilot]

Pull Request Overview

This PR refactors and enhances the RFC 3779 extension API by adding builder-style methods, improving parsing/merging logic, and updating tests to use the new API.

• Add helper overload in tests.h for comparing std::array<uint8_t, N> buffers.
• Introduce add_address, restrict, and inherit methods for IPAddressBlocks and analogous methods for ASBlocks.
• Update decoding routines to automatically sort and merge entries, and refresh all tests to use the new API patterns.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/tests/tests.h	Added test_eq overload for std::array<uint8_t, N>
src/tests/test_x509_rpki.cpp	Added builder-style tests and refactored existing ones
src/lib/x509/x509_ext.h	Exposed builder methods and made constructors explicit
src/lib/x509/x509_ext.cpp	Invoke sort_and_merge after decoding and updated encode/decode routines
src/tests/data/x509/x509test/*.pem	Added new test certificates

Comments suppressed due to low confidence (3)

src/tests/test_x509_rpki.cpp:278

• [nitpick] The message says "block 24" but this is checking index 4. Update the description to "block 4 has correct safi" for clarity.

      result.test_eq("block 24 has correct safi", addr_blocks[4].safi(), std::optional<uint8_t>{1});

src/tests/test_x509_rpki.cpp:1500

• [nitpick] This test is for ASBlocks, but the description says "IP Address Blocks". Change it to "X509 AS Blocks encode (builder)".

   Test::Result result("X509 IP Address Blocks encode (builder)");

src/lib/x509/x509_ext.h:738

• [nitpick] The method name restrict may be confused with a C/C++ keyword. Consider renaming to restrict_addresses or similar for clearer intent.

      void restrict(std::optional<uint8_t> safi = std::nullopt) {

[Copilot]

[nitpick] The builder does not merge or sort overlapping AS ranges after adding. Consider invoking a merge step (e.g., sort_and_merge_ranges) to maintain a normalized, sorted list.