New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix TsigGenerate for non-0 TSIG error or non-empty other data #1138
Conversation
@@ -131,15 +131,11 @@ func TsigGenerate(m *Msg, secret, requestMAC string, timersOnly bool) ([]byte, s | |||
return nil, "", ErrKeyAlg | |||
} | |||
h.Write(buf) | |||
// Copy all TSIG fields expect MAC and its size, which are filled using the computed digest. | |||
*t = *rr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could alternatively copy the error and other data (plus its length) fields separately below. I chose this way as it seemed more concise and less error prone (it's clear no fields are forgotten and all fields are an exact copy of the original TSIG RR), but I don't mind taking the alternative approach.
t.MAC = hex.EncodeToString(h.Sum(nil)) | ||
t.MACSize = uint16(len(t.MAC) / 2) // Size is half! | ||
|
||
t.Hdr = RR_Header{Name: rr.Hdr.Name, Rrtype: TypeTSIG, Class: ClassANY, Ttl: 0} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: technically we can't always assume TTL is 0 here; the TSIG in the passed message could have a non-0 value (RFC2845 states it's 0 but doesn't prohibit using a different value). The fix in this PR preserves the passed TTL.
t.Fudge = rr.Fudge | ||
t.TimeSigned = rr.TimeSigned | ||
t.Algorithm = rr.Algorithm | ||
t.OrigId = m.Id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is also not always correct and should be rr.OrigId
. The fix in this PR makes it correct, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure of the RFC semantics, but seems reasonable and code looks fine. LGTM.
@tmthrgd thank you for the prompt review and approval. Just to let you know I've just noticed one minor typo in a comment line and corrected it. |
[ Quoting <notifications@github.com> in "Re: [miekg/dns] fix TsigGenerate fo..." ]
@tmthrgd approved this pull request.
Not sure of the RFC semantics, but seems reasonable and code looks fine. LGTM.
/merge
|
…1138) Automatically submitted.
The current implementation of
TsigGenerate
ignores the TSIG error and other data fields of the TSIG RR in the passed DNS message for generating the signed binary-format message (it uses these fields correctly for computing the MAC). So the resulting message would fail to be verified at the recipient. This PR is a straightforward fix to this problem.