Support TSIG for authenticated dynamic DNS updates about hickory-dns

[bluejekyll]

After reviewing TSIG RFCs this is a bigger feature than I realized. I also am thinking that it might be redundant with DNSCrypt possibly, which seems better to implement.

[rotty]

Regarding the ease of implementation: I was talking only about the client side, I've not thought about what a server-side implementation would entail.

As an additional data point: it seems the PowerDNS authorative nameserver supports only TSIG-authenticated updates; at least I could not find any information about using SIG(0) with PowerDNS in a quick search. If that's correct, I think TSIG support, at least on the client side, can be considered important.

[dsvensson]

Browsing around a bit to see how much work it would be. Hoping to hear some comments. I'm assuming this could be tied into the Signer somehow, hoping that's not a dead end :)

Here seems to be a filter on OpCode::Update that would prevent AXFR/IXFR queries.
https://github.com/bluejekyll/trust-dns/blob/5682ba174ca0c7173b022ff0601bcd6b5972ee4a/crates/proto/src/xfer/dns_multiplexer.rs#L343-L351

Here the message is finalized, including passing timestamp:
https://github.com/bluejekyll/trust-dns/blob/5682ba174ca0c7173b022ff0601bcd6b5972ee4a/crates/proto/src/op/message.rs#L635-L645

Just like add_sig0, there would be an add_tsig.

And then a TSIG version of:
https://github.com/bluejekyll/trust-dns/blob/5682ba174ca0c7173b022ff0601bcd6b5972ee4a/crates/client/src/rr/dnssec/signer.rs#L518

And a TSIG version of:
https://github.com/bluejekyll/trust-dns/blob/5682ba174ca0c7173b022ff0601bcd6b5972ee4a/crates/proto/src/rr/dnssec/rdata/sig.rs#L183

There is a readable and well tested (CoreDNS etc) implementation here in Go:

https://github.com/miekg/dns/blob/d16ecb693e3f8d524769fefce2192a4c92207ff9/tsig.go#L98

@bluejekyll sounds like you've banged your head against this.. any thoughts on pitfalls? My usecase is only queries.

[bluejekyll]

The reason I backed away from TSIG was mainly due to having implemented SIG0 and then reading rfc2845, since what I needed was covered by SIG0, I didn't feel a need to build it out. Getting the signing process correct for RRSIG and SIG0 was a pain, and so by estimation, I realized that TSIG would be a similar amount. That's where my estimate of this being a lot of work comes from.

Here seems to be a filter on OpCode::Update that would prevent AXFR/IXFR queries.

SIG0 is only required right now for Update. We should have a flag for all queries that would allow the signing logic to kick in, I can't remember at the moment (need to review the code) if that was ever implemented. I did think about it being useful. We might want to make it a flag on the Client.

Here the message is finalized, including passing timestamp:
Just like add_sig0, there would be an add_tsig.
And then a TSIG version of:

MessageFinalizer was created as a means to allow for the proto crate to be split out of the client crate. We might not want a different implementation all together, but instead a separate method on Signer for performing this. I'm open to other ideas though. Signer itself my have too much logic in it, so maybe we want separate SIG0 and TSIG MessageFinalizers? I intended the logic in that to allow MessageFinalizers to be chained so that multiple SIG0's could be performed.

There is a readable and well tested (CoreDNS etc) implementation here in Go:

As a matter of practice I tend to not look at other implementations to keep the IP totally separate, but if that gives you good guidance then that's excellent.

sounds like you've banged your head against this.

I didn't actually bang my head against this, instead I just didn't feel a pressing need to implement it, and so never added it to my own backlog. So I don't have a strong set of guidance. It does look like what you've identified as a rough plan is reasonable.

I'm happy to help, though I won't be available until next week.

[bluejekyll]

FYI, my current plan is to implement #278 (mTLS) and not TSIG. SIG0 is already supported for a standard dynamic update auth option.

[dsvensson]

Seems to be some TSIG action going on here, https://github.com/NLnetLabs/domain

[dsvensson]

@bluejekyll Been experimenting with this a bit and it turned out somewhat painful. Thought I could reuse more infrastructure. Think this requires a bit more refactoring to get TSIG in, without duplicating too much. Should get back with what sharp corners I found at some point.

With that conclusion I instead removed the mentioned OpCode::Update filter for the signer, and tried out SIG(0) protected AXFR's which worked like a charm towards BIND.

[bluejekyll]

If you happen to have any notes from the issues you ran into, we could capture those here?

And yes, I've found SIG0 to be a much more straightforward implementation and generally suits my needs.

[rotty]

JFYI: I've added TSIG support in tdns-cli on top of trust-dns. The only issue I ran into was that adding TSIG RR to the DNS query will conflict with the EDNS handling, as the latter will add an additional RR during message rendering; this required me to duplicate the update_message.rs code from trust-dns inside tdns-cli.

It would be nice if trust-dns had support for the RR record type, and attaching a TSIG signature to a DNS message; this could be done without requiring the actual signature algorithm to reside in trust-dns, I think, if that is preferred.

I'd be willing to prepare a PR based on my implementation, if that is deemed in scope for the project. Also, I'm wondering in what ways SIG0 is more straightforward to implement than TSIG -- I found it TSIG to be quite easy to implement.

[bluejekyll]

@rotty Yes, I'd be very happy to accept TSIG into the library. I think that would be a great addition. If it's not implemented in the trust-dns-server, then my concern would be that we don't have a good method of testing the feature. There is a compatibility suite of tests built on top of BIND9, so it would be a good idea to implement a test in that suite for TSIG so that we have coverage over the feature and we don't lose it due to other breaking changes. As an example, here are the SIG0 tests: https://github.com/bluejekyll/trust-dns/blob/master/tests/compatibility-tests/tests/sig0_tests.rs

In terms of straightforwardness, let's just say that when I was implementing all of this, after DNSSEC and SIG0, I had just lost some energy in doing it, and it wasn't vital to my needs.

I will say that you might want to hold off on a PR atm, I have a very large refactor coming with #849 so we might want to hold off until I land that. I'm hoping to get it in this weekend.

[rotty]

Ok, I'll look into providing a client and server implementation, but it could take a while, as I need to get familiar with the codebase first. It's great that a server-side implementation in scope -- let's see how this goes!

Thanks for the heads-up regarding #849; very nice that this is almost ready! I'm looking forward to switch to async/await in tdns-cli.

[bluejekyll]

Oh, sorry, I wasn't suggesting you need to implement the server side, only that I want to make sure we have test coverage, and that the best way to do that would be to use the compatibility test suite for providing that coverage.

If you do want to add the support to the server, that would be great, but I don't think it's necessary.

[rotty]

Ok, thanks for the clarification.

[dsvensson]

@rotty Please deal with that if let OpCode::Update = request.op_code() filter mentioned above while at it to allow TSIG'd AXFRs and IXFRs!