Adding classless IN-ADDR.ARPA (IPv4) and IP6.ARPA (IPv6) delegations (reverse lookups) #12

BitlyTwiser · 2024-09-20T14:43:56Z

Good day!

I have been aware of your marvelous works for some time! I followed the thread for this package when it was moving its way towards the Zig stdlib 👏 I am excited to see the package progress and move towards stdlib merging.

I am a fan of these works and found them most intriguing, thus, when I had an opportunity to use this package I dived right in! 😄

I was working on another project of mine when I realized I needed reverse DNS lookups to ascertain host records from IP addresses in Zig and your work seemed like the perfect place to attempt to add such things! I hope you do not mind the PR to extend the API functionality of zigdig just a smidge! 🙃

I also apologize if this is a long PR message for little reason, I am an excitable fellow and enjoy this stuff immensely 🙏

Notes on the PR:
The primary innards of the zigdig package were mostly left untouched. I added reverse.zig and address.zig (as you can obviously see) to facilitate the needs of the lookup and the IP Address work needed to get IP's into ARPA formation for PTR DNS queries.

All tests pass and look good. I did need to extend max_label_size: usize = 35 from 32->35 else it would cause an overflow for the more robust label sizes needed for Ipv6 parsing which seemed acceptable enough.

For testing (outside of the actual code) I also used Wireshark and dig to capture packets to ensure everything looked good (aside from getting a valid return, which one does, I wanted to ensure packet structure looked perfect as well).

Dig commands for testing:

dig -x <ip_address> +noadditional +nostats +nocomments +answer +noedns +noadflag

Running the code and capturing traffic on Lo interface the packets are identical and the reverse lookup succeeds with the proper addresses/records returned:
(left packet is the dig command, Right packet is the packet captured from program runtime)
Request:

Response (same order as above respectively):

Now with all that said, I am super open to feedback, criticism, and the like 😄

If this is entirely unwanted I can keep the works in my work and work from there, if you think this is nifty/sensible and could enhance the package that is marvelous! Whatever feedback on the code you have I am all 👂 and 👀

Thanks for your time! 😃

…onse from opaque data. Need to simplify and formulate a better way to do this

…rflow when serializing the name.

…omments, adjusting static ints to constants for simple uddates in reverse.zig

…ook at catching error instead of magic number?

BitlyTwiser · 2024-10-13T16:35:30Z

Good day @lun-4,

I was curious if you yet had an opportunity to review the above PR in any fashion 🙏

Hopefully it does not take too much of your time and I always appreciate feedback 😃

lun-4 · 2024-10-13T16:40:16Z

oops, thanks for the reminder, should've said something once the PR was open but these past weeks have been keeping me busy. I plan to review this once I'm able to get around to it.

regarding stdlib upstreaming, given the API is living outside of the main address resolution API, I'd be fine with merging it (after review) and just not upstreaming that specific part (given I think stdlib does not need reverse DNS lookups itself given that functionality wasn't there)

BitlyTwiser · 2024-10-14T18:13:05Z

Oh certainly understandable there! The hectic days can certainly keep us busy, take all the time you need 😄 I just appreciate the time and review! 🙇

Most certainly understandable on the passing of the reverse code upstream to stdlib. During curation, I mostly assumed all of this would remain behind within the base code of zigdig since its primary purpose is a bit outside of stdlibs needs 🤣

Mostly selfishly making the alterations since I needed them myself for other projects and I was already aware of/a fan of the zigdig code itself 👍

Let me know of any changes you wish to see when you get around to the review and I shall be happy for any and all feedback!

lun-4 · 2024-12-07T18:37:45Z

README.md

+### Reverse lookups examples
+``` 
+    // Ipv4
+    const name = "dns.google.";


name is not being used in this example, why is that?

lun-4 · 2024-12-07T18:42:45Z

src/address.zig

+        };
+    }
+
+    /// Creates an IpAddress from a []const u8 address


AddressMeta.fromString doesn't return IpAddress

lun-4 · 2024-12-07T18:43:35Z

src/address.zig

+
+const Errors = error{InvalidIP};
+
+pub const AddressMeta = union(enum) {


why isn't std.net.Address used directly?

lun-4 · 2024-12-07T18:47:12Z

src/address.zig

+};
+
+// RFC reference for ARPA address formation: https://www.ietf.org/rfc/rfc2317.txt
+pub const IpAddress = struct {


this probably should be renamed to ArpaDomain rather than IpAddress

lun-4 · 2024-12-07T18:48:03Z

src/address.zig

+    arpa_suffix: []const u8 = ".in-addr.arpa",
+    arpa_suffix_ipv6: []const u8 = "ip6.arpa",
+
+    allocator: std.mem.Allocator,


the design of zigdig allows for library users to interact with it without memory allocation, is there a way to do such here?

lun-4 · 2024-12-07T19:09:27Z

src/reverse.zig

+                );
+
+                //fromOpaque read the data, so if we utilize any standard reader/writer we have access to the []const u8 opaque data value. So we just allocprint here
+                const dns_name = try std.fmt.allocPrint(self.allocator, "{s}", .{resource_data});


you should verify that the returned resource_data is of type PTR instead of relying on the dynamic format within dns.ResourceData. a malformed response would return malformed results to library clients instead of an error or empty slice.

lun-4 · 2024-12-07T19:09:50Z

src/reverse.zig

+test "reverse lookup of Ipv4 address" {
+    const name = "dns.google.";
+    const test_address = "8.8.4.4";
+    var reverse = try ReverseLookup.init(std.heap.page_allocator, test_address, 123);


use std.testing.allocator for memory leak checking (applies to all tests here as well)

lun-4 · 2024-12-07T19:10:58Z

src/reverse.zig

+    var reverse = try ReverseLookup.init(std.heap.page_allocator, test_address, 123);
+    const names = try reverse.lookupIpv4();
+
+    assert(names.len > 0);


use std.testing.expect to fail the test with an error instead of panic'ing the entire executable

lun-4 · 2024-12-07T19:11:22Z

src/reverse.zig

+    const names = try reverse.lookupIpv4();
+
+    assert(names.len > 0);
+    assert(std.mem.eql(u8, names[0], name));


use std.testing.expectEqualStrings for the same reason, plus gives nice diff output when it fails

lun-4 · 2024-12-07T19:12:35Z

src/test.zig

@@ -295,3 +296,41 @@ test "rdata serialization" {
    const encoded_result = encodeBase64(&encode_buffer, serialized_result);
    try std.testing.expectEqualStrings(PACKET_WITH_RDATA, encoded_result);
 }
+
+// Recreation of tests from reverse.zig to show basic API usage for reverse lookups
+test "reverse address lookup Ipv4" {


these tests are copy-pasted from src/reverse.zig and it seems on purpose, I would strongly suggest against copy-pasting them to decrease maintenance burden.

BitlyTwiser added 13 commits September 17, 2024 15:35

Initial push for reverse lookup code (messy)

c25d9b8

Some silly things to get the DNS (reversed) name from the packet resp…

493a5df

…onse from opaque data. Need to simplify and formulate a better way to do this

Moving reverse over into its own module

e79b9fb

Starting ipv6 work

a94301d

More ipv6 work

5a210f8

Getting closer. Ipv6 now returns the correct data, just woring on ove…

f8838b4

…rflow when serializing the name.

Most things all working now

dce7462

Moving a few things around for a more linear code flow, adding some c…

0cc970c

…omments, adjusting static ints to constants for simple uddates in reverse.zig

Not very happy with the transmuteName fix for ipv6 address. Perhaps l…

45f4d27

…ook at catching error instead of magic number?

Removing this as it was awful

61557b8

Perhaps this works better for now

2c30d36

Moving the protection from error further up the stack

348a80f

Adding some test copies to test.zig for API display and adjusting README

c5531e9

lun-4 requested changes Dec 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding classless IN-ADDR.ARPA (IPv4) and IP6.ARPA (IPv6) delegations (reverse lookups) #12

Adding classless IN-ADDR.ARPA (IPv4) and IP6.ARPA (IPv6) delegations (reverse lookups) #12

BitlyTwiser commented Sep 20, 2024

BitlyTwiser commented Oct 13, 2024

lun-4 commented Oct 13, 2024

BitlyTwiser commented Oct 14, 2024

lun-4 Dec 7, 2024

lun-4 Dec 7, 2024

lun-4 Dec 7, 2024

lun-4 Dec 7, 2024

lun-4 Dec 7, 2024

lun-4 Dec 7, 2024

lun-4 Dec 7, 2024

lun-4 Dec 7, 2024

lun-4 Dec 7, 2024

lun-4 Dec 7, 2024


		const Errors = error{InvalidIP};

		pub const AddressMeta = union(enum) {

Adding classless IN-ADDR.ARPA (IPv4) and IP6.ARPA (IPv6) delegations (reverse lookups) #12

Are you sure you want to change the base?

Adding classless IN-ADDR.ARPA (IPv4) and IP6.ARPA (IPv6) delegations (reverse lookups) #12

Conversation

BitlyTwiser commented Sep 20, 2024

BitlyTwiser commented Oct 13, 2024

lun-4 commented Oct 13, 2024

BitlyTwiser commented Oct 14, 2024

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