Add IPAddrBlock and ASIdentifiers extensions (RFC 3779) #4443

arckoor · 2024-11-25T15:36:14Z

This PR implements two extensions required for working with secure routing. These extensions contain IP Addresses or AS numbers, as specified by RFC 3779.

Both encoding and decoding of AS numbers, single addresses, address prefixes and min-max ranges have been tested.

coveralls · 2024-11-25T16:16:15Z

coverage: 91.294% (+0.04%) from 91.258%
when pulling 81ac46d on arckoor:master
into 09a7a98 on randombit:master.

randombit · 2024-11-26T13:48:54Z

@arckoor There are some CI failures which are relevant. Stylistically for calls to min,max where the compiler can’t deduce T we prefer std::min<T> vs use of casts.

randombit · 2024-11-27T16:07:34Z

@arckoor Remaining CI issues look related to the visibility annotations (eg BOTAN_PUBLIC_API) probably something is not being exported as a symbol.

arckoor · 2024-11-27T16:11:11Z

@randombit We're currently chipping away at the fuzzers, but the visibility is also an issue. Previously linux/clang was failing, which I resolved by moving the definition of set_from_bytes to the .h file, the other errors could probably be solved by moving the rest there too, but that does not sound like a good solution to me. All of these errors are most likely related to the use of templates, though I don't know what the cleanest / recommended way to resolve them is.

randombit · 2024-11-27T22:35:18Z

@arckoor ok thanks. I’ll try to have a look over the weekend

arckoor · 2024-12-10T10:53:54Z

@randombit Have you had the chance to take a look yet, and can I still contribute something to make your life easier?

randombit · 2024-12-20T21:02:58Z

@arckoor The main issue here is CI failing. In general this is a change that's appropriate to the library and the code from a quick look seems fine. But we certainly can't merge if it will break the build. Unfortunately I don't know the exact issue; I guess symbol visibility behaves differently on macOS/Windows vs Linux. My only suggestion would be to try adding BOTAN_PUBLIC_API macros to the various subclasses until the build succeeds.

arckoor · 2025-01-07T17:36:13Z

@randombit The clang build should now succeed. After much debugging we realized the compiler was optimizing away the IPAddress class completely. We tried various things like including a dummy function that instantiates all variations at compile time, nothing helped. We worked around this issue by defining a new macro that forces compilation, the windows and macos builds are probably still going to fail, as there the compilers seems to optimize away even more. A potential workaround would be to add the forced compilation macro everywhere, or move the entire thing to x509_ext.h.

Co-authored-by: butteronarchbtw <[email protected]>

butteronarchbtw · 2025-01-09T19:21:12Z

@randombit we've now resolved the symbol issues by instantiating the template classes in x509_ext.cpp and also did some small tweaks to fix the other checks. Does this seem like a viable solution to you?

randombit

@arckoor @butteronarchbtw Not a full review and I still need to check the tests but enough for you to get started on. Overall things seem fine with two exceptions

The various types have setters that seem to me quite easy to misuse. I would rather see values set using constructors, at least whenever possible.
The decoding functions seem really complicated considering the types involved. BER_Decoder is designed to make this easy, so either there is improvements possible in the decoding or there are improvements possible in BER_Decoder. I left some suggestions which are untested but should help get you going.

randombit · 2025-01-18T22:08:25Z

src/lib/x509/x509_ext.h

+      template <Version V>
+      class BOTAN_PUBLIC_API(3, 6) IPAddress final {
+         public:
+            void set_from_bytes(const std::vector<uint8_t>& v);


Input values should be span, here std::span<const uint8_t> v

randombit · 2025-01-18T22:09:51Z

src/lib/x509/x509_ext.h

+ * RFC 3779 X.509 Extensions for IP Addr
+ *
+*/
+class BOTAN_PUBLIC_API(3, 6) IPAddressBlocks final : public Certificate_Extension {


The BOTAN_PUBLIC_API macros should read 3, 7 since that's the next release

randombit · 2025-01-18T22:13:46Z

src/lib/x509/x509_ext.h

+
+            void set_min(const IPAddress<V>& min) { m_min = min; }
+
+            void set_max(const IPAddress<V>& max) { m_max = max; }


General comment: I'd prefer if possible (and it seems like it is from my current understanding) that values are set once in a constructor and we don't have any setters.

randombit · 2025-01-18T22:15:06Z

src/lib/x509/x509_ext.cpp

+   uint16_t afi = (m_addr_family[0] << 8) | m_addr_family[1];
+   if(1 > afi || afi > 2) {
+      throw Decoding_Error("Only AFI IPv4 and IPv6 are supported.");
+   }


Here combine the check on afi and the decoding

if(afi == 1) { ... } else if(afi == 2) { ... } else { throw Decoding_Error("Only AFI IPv4 and IPv6 are supported."); }

randombit · 2025-01-18T22:20:32Z

src/lib/x509/x509_ext.cpp

+      BER_Decoder obj_ber = BER_Decoder(obj);
+      BER_Object as_min_obj = obj_ber.get_next_object();
+      std::vector<uint8_t> bytes;
+      bytes.assign(as_min_obj.data().begin(), as_min_obj.data().end());


ASId is just an INTEGER, BER_Decoder supports this already

randombit · 2025-01-18T22:25:01Z

src/lib/x509/x509_ext.cpp

+
+void ASBlocks::ASIdOrRange::decode_from(BER_Decoder& from) {
+   BER_Object obj = from.get_next_object();
+   ASN1_Type type_tag = obj.type_tag();


I think this function can be quite a bit simpler overall, something like (untested)

ASN1_Tag next_tag = from.peek_next_object().type_tag(); if(next_tag == ASN1_Type::Integer) { from.decode(m_min); m_max = m_min; } else if(next_tag == ASN11_Type::Sequence) { from.start_sequence().decode(m_min).decode(m_max).end_cons(); } else { throw ... }

randombit · 2025-01-18T22:27:40Z

src/lib/x509/x509_ext.cpp

+
+void ASBlocks::ASIdOrRange::encode_into(Botan::DER_Encoder& into) const {
+   if(m_min == m_max) {
+      into.add_object(ASN1_Type::Integer, ASN1_Class::Universal, encode_asnum(m_min));


DER_Encoder already knows to zero prefix, all that should be required here is

into.encode(m_min)

(likewise wherever encode_asnum is called, I don't believe this function is necessary at all)

randombit · 2025-01-18T22:32:10Z

src/lib/x509/x509_ext.cpp

+   ASN1_Type type_tag = obj.type_tag();
+
+   // this can either be a prefix or a single address
+   if(type_tag == ASN1_Type::BitString) {


This function similar to ASBlocks::ASIdOrRange::decode_from can be made much simpler by using peek_next_object to decide if you are in the IPAddress or IPAddressRange case. Then you can use the builtin functionality for decoding bit strings, parsing sequences etc without having to replicate logic here.

randombit · 2025-01-18T22:33:59Z

src/lib/x509/x509_ext.cpp

+   m_addr_family = addr_family;
+   size_t addr_family_length = addr_family.size();
+   if(2 > addr_family_length || addr_family_length > 3) {
+      throw Decoding_Error("AFI/SAFI too long / too short.");


I find this surpsisingly hard to parse. Maybe just

if(addr_family.size() != 2 && addr_family.size() != 3)

?

randombit · 2025-01-18T22:41:46Z

src/lib/x509/x509_ext.cpp

+}
+
+void ASBlocks::ASIdentifiers::decode_from(Botan::BER_Decoder& from) {
+   BER_Object obj = from.get_next_object();


I think this can be done as (untested)

from.start_sequence() .decode_optional(m_asnum, ASN1_Type(0), ASN1_Class::ExplicitContextSpecific); .decode_optional(m_rdi, ASN1_Type(1), ASN1_Class::ExplicitContextSpecific); .end_cons()

If it's something much harder than that it suggests BER_Decoder needs a helper

arckoor force-pushed the master branch from 725468c to 1870a9c Compare November 26, 2024 14:35

arckoor force-pushed the master branch from 1870a9c to 81ac46d Compare November 29, 2024 19:47

randombit self-requested a review November 30, 2024 13:23

arckoor mentioned this pull request Dec 3, 2024

AVX512 Serpent amalgamation tests fail with GCC #4454

Open

arckoor force-pushed the master branch from 81ac46d to 1adb0ce Compare January 7, 2025 17:31

arckoor force-pushed the master branch 4 times, most recently from cec640d to 4986bf0 Compare January 9, 2025 16:53

Add IPAddrBlock and ASIdentifiers extensions (RFC 3779)

1ce3bc6

Co-authored-by: butteronarchbtw <[email protected]>

arckoor force-pushed the master branch from 4986bf0 to 1ce3bc6 Compare January 9, 2025 17:11

randombit requested changes Jan 18, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add IPAddrBlock and ASIdentifiers extensions (RFC 3779) #4443

Add IPAddrBlock and ASIdentifiers extensions (RFC 3779) #4443

arckoor commented Nov 25, 2024

coveralls commented Nov 25, 2024 •

edited

Loading

randombit commented Nov 26, 2024

randombit commented Nov 27, 2024

arckoor commented Nov 27, 2024

randombit commented Nov 27, 2024

arckoor commented Dec 10, 2024

randombit commented Dec 20, 2024

arckoor commented Jan 7, 2025

butteronarchbtw commented Jan 9, 2025

randombit left a comment •

edited

Loading

randombit Jan 18, 2025

randombit Jan 18, 2025

randombit Jan 18, 2025

randombit Jan 18, 2025

randombit Jan 18, 2025

randombit Jan 18, 2025

randombit Jan 18, 2025

randombit Jan 18, 2025

randombit Jan 18, 2025

randombit Jan 18, 2025


		void set_min(const IPAddress<V>& min) { m_min = min; }

		void set_max(const IPAddress<V>& max) { m_max = max; }

Add IPAddrBlock and ASIdentifiers extensions (RFC 3779) #4443

Are you sure you want to change the base?

Add IPAddrBlock and ASIdentifiers extensions (RFC 3779) #4443

Conversation

arckoor commented Nov 25, 2024

coveralls commented Nov 25, 2024 • edited Loading

randombit commented Nov 26, 2024

randombit commented Nov 27, 2024

arckoor commented Nov 27, 2024

randombit commented Nov 27, 2024

arckoor commented Dec 10, 2024

randombit commented Dec 20, 2024

arckoor commented Jan 7, 2025

butteronarchbtw commented Jan 9, 2025

randombit left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 25, 2024 •

edited

Loading

randombit left a comment •

edited

Loading