Add ability to use different TSIG algorithms

[bodgit]

This makes it possible to fix #549 and support GSS-TSIG (or in theory any other TSIG algorithms aside from HMAC-MD5, etc.). There is one breaking change in that the TsigSecret field is now a map of value interface{} rather than string. I've added two new functions TsigGenerateByAlgorithm() and TsigVerifyByAlgorithm() which allow an algorithm-specific callback and refactored the existing TsigGenerate() and TsigVerify() to just call these with callback functions that perform the HMAC-specific bits.

With this, GSS-TSIG can be done with something like the following:

const (
        GssTsig = "gss-tsig."
)

func main() {
        ...

        ctx, keyname, err := negotiateGssapiCtx(hostname)
        if err != nil {
                ...
        }

        client := &dns.Client{
                Net:           "tcp",
                TsigAlgorithm: map[string]*dns.TsigAlgorithm{GssTsig: {TsigGenerateGssapi, TsigVerifyGssapi}},
                TsigSecret:    map[string]interface{}{*keyname: ctx},
        }

        msg := new(dns.Msg)

        // Set dynamic DNS, etc.

        msg.SetTsig(*keyname, GssTsig, 300, time.Now().Unix())

        rr, _, err := client.Exchange(msg, net.JoinHostPort(addr, "53"))

        ...
}

negotiateGssapiCtx does the TKEY exchange to establish the GSSAPI context and my TsigGenerateGssapi & TsigVerifyGssapi are pretty much the same as in #549 (comment) the only difference is that the generate callback is now also passed the TSIG algorithm.

[codecov-io]

Codecov Report

Merging #626 into master will decrease coverage by 0.16%.
The diff coverage is 28.04%.

@@            Coverage Diff             @@
##           master     #626      +/-   ##
==========================================
- Coverage   58.23%   58.07%   -0.17%     
==========================================
  Files          37       37              
  Lines       10122    10161      +39     
==========================================
+ Hits         5895     5901       +6     
- Misses       3171     3202      +31     
- Partials     1056     1058       +2

Impacted Files	Coverage Δ
server.go	59.75% <0%> (-2.33%)	⬇️
client.go	56.38% <5.26%> (-2.06%)	⬇️
tsig.go	43.49% <55%> (+1.98%)	⬆️
msg.go	79.34% <0%> (-0.63%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 350cd08...2450047. Read the comment docs.

[tmthrgd]

@bodgit I doubt such a breaking change would be accepted into the library. I see no reason why the meta argument, and thus this breaking change, is needed at all. What's stopping you from just using a closure? You can just pass co.TsigSecret[t.Hdr.Name] (string) and t.Hdr.Name into TsigGenerateByAlgorithm and TsigVerifyByAlgorithm instead, and have them passed into cb.

[bodgit]

The problem is that co.TsigSecret[t.Hdr.Name] isn't always a string, it is for traditional HMAC TSIG but for GSS-TSIG it's a per-server GSSAPI context which in this case is some cgo-bound pointer into a Kerberos C library and I didn't want to have to add any GSSAPI library-specific types to this library. The actual algorithm-specific function is the only area that needs to know how to interpret the secret, hence making it an interface{}.

I agree this breaks the library (although all libraries get a major version bump eventually) so I could add another field such as client.TsigAlgorithmSecret and keep the original client.TsigSecret as-is which keeps the library working exactly the same as before (I've not changed the prototype of any existing functions here), but then you have two fields that kind of look the same although if/when you do that major version bump you can rationalise them.

[miekg]

before I actually review +1 to @tmthrgd 's comment.

Even if it's a "cgo pointer" you can probably get away with it being converted to string (however bad that is).

[miekg]

Also note: we try hard to not break backwards compat, even if that puts us in a bad state code wise.

library versioning is not a reason to start breaking compat now

[tmthrgd]

I opened bodgit#1 to demonstrate my suggestion:

Restructure pull-request to avoid breaking the API

This commit removes the interface{} 'meta' argument, which required
breaking the signature of TsigSecret, and instead passes the name
directly to the callback. If there is a need to use a non-string secret,
this can be looked up from within a closure using the name as a lookup
key.

This change still permits all the same use-cases as it's parent.

I make no particular comment on this pull-request (#626) or whether it should be merged, but it is definitely possible to achieve the desired aim without introducing any breaking changes.

[tmthrgd]

master...tmthrgd:gss-tsig would be the final diff if @bodgit accepts my suggestion.

[bodgit]

Even if it's a "cgo pointer" you can probably get away with it being converted to string (however bad that is).

If that's possible, then that could work. When I print my GSSAPI context out with "%#v" I get this:

&gssapi.CtxId{Lib:(*gssapi.Lib)(0xc42009a000), C_gss_ctx_id_t:(gssapi._Ctype_gss_ctx_id_t)(0xff8370)}

I'm struggling to work out how to serialize that to and from a string.

[bodgit]

I've merged @tmthrgd's PR into mine, so there should now be no compat breakage.

[tmthrgd]

A few other nits:

Are TsigVerifyByAlgorithm and TsigGenerateByAlgorithm called by code external to the library? If not, they should be unexported.

In bodgit#1 you mention:

The only gotcha is I have to set a dummy TSIG secret in the client so it doesn't just return dns.ErrSecret.

Perhaps the if _, ok := co.TsigSecret[t.Hdr.Name]; !ok should only exist in the HMAC TsigVerify verify case? (Are there valid cases for an empty-string "" secret?)

[tmthrgd]

Also, does this functionality also need to exist in server.go for completeness?

See

dns/server.go

Line 559 in f5ac34d

w.tsigStatus = TsigVerify(m, w.tsigSecret[secret], "", false)

and

dns/server.go

Line 647 in f5ac34d

data, w.tsigRequestMAC, err = TsigGenerate(m, w.tsigSecret[t.Hdr.Name], w.tsigRequestMAC, w.tsigTimersOnly)

[bodgit]

Are TsigVerifyByAlgorithm and TsigGenerateByAlgorithm called by code external to the library? If not, they should be unexported.

I've had a need to be able verify externally outside of the library; part of the TKEY negotiation includes a TSIG in the final response from the server which needs to be checked after the library would normally do it as the GSSAPI context needs to first be updated with the contents from that same TKEY response.

Generate less so, but maybe someone else has a use for that. The existing TsigVerify and TsigGenerate were exported so I made these new ones the same.

Perhaps the if _, ok := co.TsigSecret[t.Hdr.Name]; !ok should only exist in the HMAC TsigVerify verify case? (Are there valid cases for an empty-string "" secret?)

I don't think there are. Removing the check will mean an algorithm callback will always be passed at least an empty string so the onus is on that code to check that len(secret) > 0 or something similar. I'm completely ignoring that secret parameter in my GSS-TSIG callbacks anyway so I don't mind either way, whatever you think is best.

I will have a look at the server code now, I admit I overlooked it.

[miekg]

I'll to free up some cycles to review this - lots of other dns work pushed this backward.

[bodgit]

Anything I can do to progress this?

[miekg]

Sorry, just coming back to this after much work on CoreDNS.

Why do we need *TsigAlgorithm? That's still a breaking change...

I though in the long discussion we have about this that we could get away with a non-breaking backwards compat change and just having some closures?

I also (apparently) made it not clear enough that backward breaking changes for just this feature are not going to happen. I.e. new members in Client and Server can work, but changing existing fields and types will not fly.

[bodgit]

Can you show me where I've broken backwards compat? I don't believe I have (now). Originally I did by changing the tsigSecret map but I merged in @tmthrgd's PR which reverted that and made it work another way so all I've done is add the tsigAlgorithm member (which itself uses *TsigAlgorithm) which didn't exist previously.

[bodgit]

@miekg, can you possibly confer with @tmthrgd as I think he's happy there's no compat breakage. Other than that, I'd just like to know what to do with the server code I pointed out and if I should relax the secret check as suggested in #626 (comment)

[bodgit]

Can this be reopened? I'd still like to get this functionality in and @tmthrgd seemed happy with it. I'm not sure how many times I could point out that I'm only adding new members and not changing existing ones.

[miekg]

Sure. Didn't recall we where still discussing this...

…

On Fri, 4 Jan 2019, 09:15 Matt Dainty ***@***.*** wrote: Can this be reopened? I'd still like to get this functionality in and @tmthrgd <https://github.com/tmthrgd> seemed happy with it. I'm not sure how many times I could point out that I'm only adding new members and not changing existing ones. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#626 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAVkWyxbB53_GLw-W1AcCnnbO8d58WXsks5u_xuUgaJpZM4RZ6I_> .

[bodgit]

I'm trying to but you keep ignoring my requests for feedback 🤷‍♂️

I'm happy to bend this into whatever shape is necessary so it works, I'm not changing any existing members or types, only adding new ones. So the only other question is regarding using closures, can you give me an example of how you think that would work that's better than how I've currently done it with callbacks?

[DevKyleS]

@miekg Can this get reopened? Would like to get GSS-TSIG support

[miekg]

[ Quoting <[email protected]> in "Re: [miekg/dns] Add ability to use ..." ]

Can this get reopened? Would like to get GSS-TSIG support

yes, sure, with these caveats: * NO BACKWARDS INCOMPATIBLE changes * NO BLOATING of current datastructures to accommodate the changes Once you have a PR that does that, we can discuss further. If not, then no

[bodgit]

• NO BACKWARDS INCOMPATIBLE changes

I've asked you repeatedly to show me where such changes were in the latest version of the code, not in the first commits which were subsequently fixed thanks to @tmthrgd.

• NO BLOATING of current datastructures to accommodate the changes

Quoting yourself:

I.e. new members in Client and Server can work, but changing existing fields and types will not fly.

So can I add new members, or is that bloating things? Please make your mind up.

The problem is the generation and verification of TSIG is done inside the library so there needs to be some way of being able to inject additional TSIG algorithms because as it currently stands if the algorithm used is not one of MD5, SHA1, SHA256 or SHA512 the library will always return ErrKeyAlg which breaks the standard usage of the client. In case it's not perfectly clear, GSS-TSIG does not use one of those algorithms.

[miekg]

I'm done with this feature. Sorry.