TSIG names cannot be reused across different zones.

[Fattouche]

Due to the nature of the TsigSecret in the Server{} object, it's not possible to re-use TSIG names across different zones.

For example:

TsigSecret = map[string]string{"axfr.": "so6ZGir4GPAqINNh9U5c3A=="}

Here the tsig name is axfr. which means that if any other zone wanted to use axfr. as well, it would be overwrite this one. Alternatively, the server could specify the TsigSecret to use zoneName for the TSIG name, which is better because duplicates would not occur, however this means that every client using that zone would need to have the same TSIG and also wouldn't allow TSIGs to be re-used across multiple zones which again is not great.

[tmthrgd]

In cases where the TsigSecret API is insufficient, use the TsigProvider API. It was added to handle exactly this kind of use case.

If you can’t make this work with TsigProvider, then that is a bug or design flaw we can look at.

[Fattouche]

TsigProvider comes after this point in the code.

https://github.com/miekg/dns/blob/master/xfr.go#L228-L232.

You can see here that in order to even find the correct TSIG you need to query by header name. That means that if multiple TSIG across different zones share the same name, only one can exist at once because it's in a basic Map. It then uses that Tsig to verify, which may or may not use the TsigProvider interface.

Unless I'm missing something, it's not possible to solve this problem using TsigProvider. Can you point me to an example of how that would work?

[tmthrgd]

If your problem is specifically when dealing with XFR, that’s #1300 and #1311. That’s definitely a deficiency and something with a pending fix. That’s waiting on that PR, but this should already work for other TSIG records.

[Fattouche]

A couple of things with your approach:

1. The server object doesn't support the TsigProvider interface, which your PR doesn't fix.
2. If we add TsigProvider we will need to edit these lines https://github.com/miekg/dns/blob/master/server.go#L636-L643 similar to how we do in xfr.go and client.go etc.
3. We need to do the same thing here within WriteMessage https://github.com/miekg/dns/blob/master/server.go#L723
4. All of this is fine until you realize that provider.Generate() gets

   dns/tsig.go

   Line 166 in 51afb90

   buf, err := tsigBuffer(mbuf, rr, requestMAC, timersOnly)

   this TsigBuffer response passed in. This means that even though you have a []byte and a *Tsig as params to Generate, its non trivial to pull out the actual message from the byte slice so that you can get the zone Name in order to properly separate tsigs by zones.

I've created a PR on my own repo to show this problem.
https://github.com/Fattouche/dns/pull/1/files#diff-53170cac6baf0bf02f3aaa3994259ea4cffaaf2498e768fb7e03cf9f752af96cR1351-R1354

The benefit of this approach is a couple of things:

1. The user could now async reload secrets on the server because we can create a tsigProvider struct that uses a read write lock. https://github.com/Fattouche/dns/pull/1/files#diff-53170cac6baf0bf02f3aaa3994259ea4cffaaf2498e768fb7e03cf9f752af96cR1267-R1270
2. Instead of using a secrets map[string]string, we can use map[string]map[string] where the key is the tsigName and the value is a map of zones that have that same tsigName. It's quite common for zones to use the name axfr. as their tsig name, and we cannot expect that they will change it.

If we can make a function that does the opposite of TsigBuffer, I think this whole approach would work great. Unfortunately that looks a little bit difficult IMO but maybe I'm missing something.

[Fattouche]

And just for clarity once again this would solve the other issue that I previously opened #1161.

[tmthrgd]

And just for clarity once again this would solve the other issue that I previously opened #1161.

I'm not sure how adding another map would help there at all. It's still not safe to mutate the map when the server is running.

Edit: I think I understand what you mean.

[tmthrgd]

The server object doesn't support the TsigProvider interface, which your PR doesn't fix.

That we can fix. Not sure how we overlooked that initially.

All of this is fine until you realize that provider.Generate() gets [...]

I see the issue you're having. I think implementing the opposite of tsigBuffer is probably the right approach. In a future v2 perhaps we can pass the original message unaltered.

The benefit of this approach is a couple of things:

Yep, exactly! Using TsigProvider the caller can handle these things however they might want. Using something like sync.Map or a map with sync.RWMutex is not something we want to impose on all callers.

[tmthrgd]

I'll reopen this until #1331 is merged. There is a deficiency here.

[Fattouche]

Yep, exactly! Using TsigProvider the caller can handle these things however they might want. Using something like sync.Map or a map with sync.RWMutex is not something we want to impose on all callers.

Absolutely, I was just showing how in the TsigProvider implementation you could use a RW Mutex so that anytime you need to check that map on the server TSIG verification, you could lock it to make sure that no secrets were being added removed. It's quite a beneficial setup for systems that need to constantly rotate tsig secrets.