[PR #2975] [MERGED] Further TSIG related cleanups #3464

New issue

Closed

opened 2026-03-16 11:44:59 +03:00 by kerem · 0 comments

kerem commented

2026-03-16 11:44:59 +03:00

Owner

📋 Pull Request Information

Original PR: https://github.com/hickory-dns/hickory-dns/pull/2975
Author: @cpu
Created: 5/6/2025
Status: ✅ Merged
Merged: 5/6/2025
Merged by: @djc

Base: main ← Head: cpu-tsig-msg-read_dev

📝 Commits (10+)

2af3a37 proto: doc signed_bitmessage_to_buf first_message arg
315e2ca proto: split signed_bitmessage_to_buf Message::read_records
8dd62b4 proto: refactor/tidy signed_bitmessage_to_buf
d7c1870 proto: clarify TSIG message_tbs rustdoc
47b1c86 proto: remove superfluous TSIG message_tbs type annotations
9d0d3f5 proto: remove TSIG message_tbs previous message arg
b5be2cd proto: flip message/previous_hash order for TSIG fns
4a403e7 proto: clarify TSIG verify_message_byte rustdoc
6948959 proto: flip verify_message_byte range/ts return arg order
4604805 proto: adjust TSIG RFC comments

📊 Changes

11 files changed (+187 additions, -163 deletions)

View changed files

📝 bin/tests/integration/named_metrics_tests.rs (+2 -2)
📝 crates/client/src/client/client.rs (+3 -3)
📝 crates/proto/src/dnssec/rdata/tsig.rs (+50 -46)
📝 crates/proto/src/dnssec/signer.rs (+5 -5)
📝 crates/proto/src/dnssec/tsig.rs (+56 -59)
📝 crates/proto/src/op/message.rs (+54 -29)
📝 crates/proto/src/op/mod.rs (+1 -3)
📝 crates/proto/src/udp/udp_client_stream.rs (+6 -6)
📝 crates/proto/src/xfer/dns_multiplexer.rs (+6 -6)
📝 tests/compatibility-tests/tests/integration/tsig_tests.rs (+2 -2)
📝 tests/integration-tests/tests/integration/client_tests.rs (+2 -2)

📄 Description

This is a follow-up to https://github.com/hickory-dns/hickory-dns/pull/2964

Outside of documentation cleanups, there were a couple more meaningful changes;

The TSIG signed_bitmessage_to_buf() fn was reworked to lean more heavily on Message::read_records(), and to correctly specify is_additional. This in turn allows making Message::read_records() enforce that OPT records are only found in the additional section (See this comment thread for more information).
The TSIG message_tbs() fn loses its previous_hash argument; it was dead code as there's no use-case where we need to consider a previous message digest when signing a message.
I changed the order of some functions arguments and return values for clarity.
The MessageFinalizer trait was overly generic, supporting adding any number of Records to the end of a to-be-finalized Message. In practice there are only two meaningful implementations: TSIG and SIG(0). In these cases the only thing we want to do is append a single MessageSignature, so let's make the trait support that specifically. This is less general/flexible, but I don't think other use-cases exist to justify the flexibility.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/hickory-dns/hickory-dns/pull/2975 **Author:** [@cpu](https://github.com/cpu) **Created:** 5/6/2025 **Status:** ✅ Merged **Merged:** 5/6/2025 **Merged by:** [@djc](https://github.com/djc) **Base:** `main` ← **Head:** `cpu-tsig-msg-read_dev` --- ### 📝 Commits (10+) - [`2af3a37`](https://github.com/hickory-dns/hickory-dns/commit/2af3a37e956365f2fa406c9f1580fad47ff4052b) proto: doc signed_bitmessage_to_buf first_message arg - [`315e2ca`](https://github.com/hickory-dns/hickory-dns/commit/315e2ca36b23d60680a888d536e4d594095892b0) proto: split signed_bitmessage_to_buf Message::read_records - [`8dd62b4`](https://github.com/hickory-dns/hickory-dns/commit/8dd62b4c13a1d29dccd4e7adbc6dc5040c6e0a3d) proto: refactor/tidy signed_bitmessage_to_buf - [`d7c1870`](https://github.com/hickory-dns/hickory-dns/commit/d7c187073f2168466c53314906dacf5ca1816d1a) proto: clarify TSIG message_tbs rustdoc - [`47b1c86`](https://github.com/hickory-dns/hickory-dns/commit/47b1c864310bb6eba2f692073383b3a1a937dbdd) proto: remove superfluous TSIG message_tbs type annotations - [`9d0d3f5`](https://github.com/hickory-dns/hickory-dns/commit/9d0d3f543655184fdd3d42d81c5c330504d4e9ab) proto: remove TSIG message_tbs previous message arg - [`b5be2cd`](https://github.com/hickory-dns/hickory-dns/commit/b5be2cdb5c76f175d708a30b4e0594c90cd7bb94) proto: flip message/previous_hash order for TSIG fns - [`4a403e7`](https://github.com/hickory-dns/hickory-dns/commit/4a403e7195f53d2689089e38616dbc5108f1536a) proto: clarify TSIG verify_message_byte rustdoc - [`6948959`](https://github.com/hickory-dns/hickory-dns/commit/6948959b1d5c9c1e85072c546662359df45c8463) proto: flip verify_message_byte range/ts return arg order - [`4604805`](https://github.com/hickory-dns/hickory-dns/commit/4604805892bcb66c993fda2aa98ff37e41f454b1) proto: adjust TSIG RFC comments ### 📊 Changes **11 files changed** (+187 additions, -163 deletions) <details> <summary>View changed files</summary> 📝 `bin/tests/integration/named_metrics_tests.rs` (+2 -2) 📝 `crates/client/src/client/client.rs` (+3 -3) 📝 `crates/proto/src/dnssec/rdata/tsig.rs` (+50 -46) 📝 `crates/proto/src/dnssec/signer.rs` (+5 -5) 📝 `crates/proto/src/dnssec/tsig.rs` (+56 -59) 📝 `crates/proto/src/op/message.rs` (+54 -29) 📝 `crates/proto/src/op/mod.rs` (+1 -3) 📝 `crates/proto/src/udp/udp_client_stream.rs` (+6 -6) 📝 `crates/proto/src/xfer/dns_multiplexer.rs` (+6 -6) 📝 `tests/compatibility-tests/tests/integration/tsig_tests.rs` (+2 -2) 📝 `tests/integration-tests/tests/integration/client_tests.rs` (+2 -2) </details> ### 📄 Description This is a follow-up to https://github.com/hickory-dns/hickory-dns/pull/2964 Outside of documentation cleanups, there were a couple more meaningful changes; 1. The TSIG `signed_bitmessage_to_buf()` fn was reworked to lean more heavily on `Message::read_records()`, and to correctly specify `is_additional`. This in turn allows making `Message::read_records()` enforce that `OPT` records are only found in the additional section (See [this comment thread](https://github.com/hickory-dns/hickory-dns/pull/2964#discussion_r2072663936) for more information). 2. The TSIG `message_tbs()` fn loses its `previous_hash` argument; it was dead code as there's no use-case where we need to consider a previous message digest when signing a message. 3. I changed the order of some functions arguments and return values for clarity. 4. The `MessageFinalizer` trait was overly generic, supporting adding any number of `Record`s to the end of a to-be-finalized `Message`. In practice there are only two meaningful implementations: TSIG and SIG(0). In these cases the only thing we want to do is append a single `MessageSignature`, so let's make the trait support that specifically. This is less general/flexible, but I don't think other use-cases exist to justify the flexibility. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>