core: Add full TSIG verification in DoH transport#8013
Conversation
This This PR add full TSIG verification in DoH using dns.TsigVerify() 7943 Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
| func forgedTSIGMsg() *dns.Msg { | ||
| m := new(dns.Msg) | ||
| m.SetQuestion("example.com.", dns.TypeA) | ||
|
|
||
| m.Extra = append(m.Extra, &dns.TSIG{ | ||
| Hdr: dns.RR_Header{ | ||
| Name: "bogus-key.", | ||
| Rrtype: dns.TypeTSIG, | ||
| Class: dns.ClassANY, | ||
| Ttl: 0, | ||
| }, | ||
| Algorithm: dns.HmacSHA256, | ||
| TimeSigned: uint64(time.Now().Unix()), | ||
| Fudge: 300, | ||
| MACSize: 32, | ||
| MAC: strings.Repeat("00", 32), | ||
| OrigId: m.Id, | ||
| Error: dns.RcodeSuccess, | ||
| }) | ||
| return m | ||
| } |
There was a problem hiding this comment.
forgedTSIGMsg() uses key name "bogus-key." which doesn't exist in the server's tsigSecret map. I think this means dns.TsigVerify() is never reached, and only the map lookup fails. Consider using mustPackSignedTSIGQuery (from tsig_test.go) with the right key name but a different secret, so the test actually exercises the MAC verification path that RequestToMsgWire was introduced to support.
There was a problem hiding this comment.
Can we add a test using mustPackSignedTSIGQuery with matching key name and secret, then asserting RcodeSuccess? Kind of a happy-path test like in the gRPC and QUIC implementations
| func (d *DoHWriter) TsigStatus() error { | ||
| return nil | ||
| return d.tsigStatus |
There was a problem hiding this comment.
nit: low hanging fruit for a test, kind of like TestDoQWriterTsigStatusReturnsStoredStatus from QUIC
|
Thanks @thevilledev PR updated. |
thevilledev
left a comment
There was a problem hiding this comment.
Thanks @yongtang, LGTM
1. Why is this pull request needed and what does it do?
This This PR add full TSIG verification in DoH using dns.TsigVerify()
2. Which issues (if any) are related?
#7943
3. Which documentation changes (if any) need to be made?
n/a
4. Does this introduce a backward incompatible change or deprecation?
n/a