Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix TsigGenerate for non-0 TSIG error or non-empty other data #1138

Merged
merged 2 commits into from Jul 21, 2020
Merged

fix TsigGenerate for non-0 TSIG error or non-empty other data #1138

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bfa1c5f
fix TsigGenerate for non-0 TSIG error or non-empty other data
jinmeiib Jul 21, 2020
dededd7
corrected a minor typo in a comment
jinmeiib Jul 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 2 additions & 6 deletions tsig.go
View file
Open in desktop
Expand Up @@ -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 except MAC and its size, which are filled using the computed digest.
*t = *rr
Copy link
Contributor Author

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}
Copy link
Contributor Author

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
Copy link
Contributor Author

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.


tbuf := make([]byte, Len(t))
off, err := PackRR(t, tbuf, 0, nil, false)
if err != nil {
Expand Down
67 changes: 67 additions & 0 deletions tsig_test.go
View file
Open in desktop
Expand Up @@ -125,3 +125,70 @@ func TestTsigErrors(t *testing.T) {
t.Errorf("expected error to contain %q, but got %v", "overflow", err)
}
}

// This test exercises some more corner cases for TsigGenerate.
func TestTsigGenerate(t *testing.T) {
// This is a template TSIG to be used for signing.
tsig := TSIG{
Hdr: RR_Header{Name: "testkey.", Rrtype: TypeTSIG, Class: ClassANY, Ttl: 0},
Algorithm: HmacSHA256,
TimeSigned: timeSigned,
Fudge: 300,
OrigId: 42,
Error: RcodeBadTime, // use a non-0 value to make sure it's indeed used
}

tests := []struct {
desc string // test description
requestMAC string // request MAC to be passed to TsigGenerate (arbitrary choice)
otherData string // other data specified in the TSIG (arbitrary choice)
expectedMAC string // pre-computed expected (correct) MAC in hex form
}{
{"with request MAC", "3684c225", "",
"c110e3f62694755c10761dc8717462431ee34340b7c9d1eee09449150757c5b1"},
{"no request MAC", "", "",
"385449a425c6d52b9bf2c65c0726eefa0ad8084cdaf488f24547e686605b9610"},
{"with other data", "3684c225", "666f6f",
"15b91571ca80b3b410a77e2b44f8cc4f35ace22b26020138439dd94803e23b5d"},
}
for _, tc := range tests {
tc := tc
t.Run(tc.desc, func(t *testing.T) {
// Build TSIG for signing from the template
testTSIG := tsig
testTSIG.OtherLen = uint16(len(tc.otherData) / 2)
testTSIG.OtherData = tc.otherData
req := &Msg{
MsgHdr: MsgHdr{Opcode: OpcodeUpdate},
Question: []Question{Question{Name: "example.com.", Qtype: TypeSOA, Qclass: ClassINET}},
Extra: []RR{&testTSIG},
}

// Call generate, and check the returned MAC against the expected value
msgData, mac, err := TsigGenerate(req, testSecret, tc.requestMAC, false)
if err != nil {
t.Error(err)
}
if mac != tc.expectedMAC {
t.Fatalf("MAC doesn't match: expected '%s', but got '%s'", tc.expectedMAC, mac)
}

// Retrieve the TSIG to be sent out, confirm the MAC in it
_, outTSIG, err := stripTsig(msgData)
if err != nil {
t.Error(err)
}
if outTSIG.MAC != tc.expectedMAC {
t.Fatalf("MAC doesn't match: expected '%s', but got '%s'", tc.expectedMAC, outTSIG.MAC)
}
// Confirm other fields of MAC.
// RDLENGTH should be valid as stripTsig succeeded, so we exclude it from comparison
outTSIG.MACSize = 0
outTSIG.MAC = ""
testTSIG.Hdr.Rdlength = outTSIG.Hdr.Rdlength
if *outTSIG != testTSIG {
t.Fatalf("TSIG RR doesn't match: expected '%v', but got '%v'", *outTSIG, testTSIG)
}
})
}
}