Repository: miekg/dns
Pull request #1317 (2021-12-14 12:37:08)

Fix edns keepalive
Added tests for EDNS0_TCP_KEEPALIVE pack/unpack as requested in the original PR.

Closes #1294

miekg: this succeeds https://github.com/miekg/dns/pull/1294 ? and that can be closed?
ameshkov: @miekg I think if you merge this one #1294 will be automatically merged as well since this PR contains the commit by @rs 

Patch:
diff --git a/edns.go b/edns.go
index d070e41b..862e5343 100644
--- a/edns.go
+++ b/edns.go
@@ -98,6 +98,8 @@ func (rr *OPT) String() string {
 			s += "\n; SUBNET: " + o.String()
 		case *EDNS0_COOKIE:
 			s += "\n; COOKIE: " + o.String()
+		case *EDNS0_TCP_KEEPALIVE:
+			s += "\n; KEEPALIVE: " + o.String()
 		case *EDNS0_UL:
 			s += "\n; UPDATE LEASE: " + o.String()
 		case *EDNS0_LLQ:
@@ -657,57 +659,52 @@ func (e *EDNS0_LOCAL) unpack(b []byte) error {
 // EDNS0_TCP_KEEPALIVE is an EDNS0 option that instructs the server to keep
 // the TCP connection alive. See RFC 7828.
 type EDNS0_TCP_KEEPALIVE struct {
-	Code    uint16 // Always EDNSTCPKEEPALIVE
-	Length  uint16 // the value 0 if the TIMEOUT is omitted, the value 2 if it is present;
-	Timeout uint16 // an idle timeout value for the TCP connection, specified in units of 100 milliseconds, encoded in network byte order.
+	Code uint16 // Always EDNSTCPKEEPALIVE
+
+	// Timeout is an idle timeout value for the TCP connection, specified in
+	// units of 100 milliseconds, encoded in network byte order. If set to 0,
+	// pack will return a nil slice.
+	Timeout uint16
+
+	// Length is the option's length.
+	// Deprecated: this field is deprecated and is always equal to 0.
+	Length uint16
 }
 
 // Option implements the EDNS0 interface.
 func (e *EDNS0_TCP_KEEPALIVE) Option() uint16 { return EDNS0TCPKEEPALIVE }
 
 func (e *EDNS0_TCP_KEEPALIVE) pack() ([]byte, error) {
-	if e.Timeout != 0 && e.Length != 2 {
-		return nil, errors.New("dns: timeout specified but length is not 2")
-	}
-	if e.Timeout == 0 && e.Length != 0 {
-		return nil, errors.New("dns: timeout not specified but length is not 0")
+	if e.Timeout > 0 {
+		b := make([]byte, 2)
+		binary.BigEndian.PutUint16(b, e.Timeout)
+		return b, nil
 	}
-	b := make([]byte, 4+e.Length)
-	binary.BigEndian.PutUint16(b[0:], e.Code)
-	binary.BigEndian.PutUint16(b[2:], e.Length)
-	if e.Length == 2 {
-		binary.BigEndian.PutUint16(b[4:], e.Timeout)
-	}
-	return b, nil
+	return nil, nil
 }
 
 func (e *EDNS0_TCP_KEEPALIVE) unpack(b []byte) error {
-	if len(b) < 4 {
-		return ErrBuf
-	}
-	e.Length = binary.BigEndian.Uint16(b[2:4])
-	if e.Length != 0 && e.Length != 2 {
-		return errors.New("dns: length mismatch, want 0/2 but got " + strconv.FormatUint(uint64(e.Length), 10))
-	}
-	if e.Length == 2 {
-		if len(b) < 6 {
-			return ErrBuf
-		}
-		e.Timeout = binary.BigEndian.Uint16(b[4:6])
+	switch len(b) {
+	case 0:
+	case 2:
+		e.Timeout = binary.BigEndian.Uint16(b)
+	default:
+		return fmt.Errorf("dns: length mismatch, want 0/2 but got %d", len(b))
 	}
 	return nil
 }
 
-func (e *EDNS0_TCP_KEEPALIVE) String() (s string) {
-	s = "use tcp keep-alive"
-	if e.Length == 0 {
+func (e *EDNS0_TCP_KEEPALIVE) String() string {
+	s := "use tcp keep-alive"
+	if e.Timeout == 0 {
 		s += ", timeout omitted"
 	} else {
 		s += fmt.Sprintf(", timeout %dms", e.Timeout*100)
 	}
-	return
+	return s
 }
-func (e *EDNS0_TCP_KEEPALIVE) copy() EDNS0 { return &EDNS0_TCP_KEEPALIVE{e.Code, e.Length, e.Timeout} }
+
+func (e *EDNS0_TCP_KEEPALIVE) copy() EDNS0 { return &EDNS0_TCP_KEEPALIVE{e.Code, e.Timeout, e.Length} }
 
 // EDNS0_PADDING option is used to add padding to a request/response. The default
 // value of padding SHOULD be 0x0 but other values MAY be used, for instance if
diff --git a/tsig.go b/tsig.go
index b49562d8..55ca7521 100644
--- a/tsig.go
+++ b/tsig.go
@@ -162,20 +162,29 @@ func tsigGenerateProvider(m *Msg, provider TsigProvider, requestMAC string, time
 	if err != nil {
 		return nil, "", err
 	}
+
 	buf, err := tsigBuffer(mbuf, rr, requestMAC, timersOnly)
 	if err != nil {
 		return nil, "", err
 	}
 
 	t := new(TSIG)
-	// Copy all TSIG fields except MAC and its size, which are filled using the computed digest.
+	// Copy all TSIG fields except MAC, its size, and time signed which are filled when signing.
 	*t = *rr
-	mac, err := provider.Generate(buf, rr)
-	if err != nil {
-		return nil, "", err
+	t.TimeSigned = 0
+	t.MAC = ""
+	t.MACSize = 0
+
+	// Sign unless there is a key or MAC validation error (RFC 8945 5.3.2)
+	if rr.Error != RcodeBadKey && rr.Error != RcodeBadSig {
+		mac, err := provider.Generate(buf, rr)
+		if err != nil {
+			return nil, "", err
+		}
+		t.TimeSigned = rr.TimeSigned
+		t.MAC = hex.EncodeToString(mac)
+		t.MACSize = uint16(len(t.MAC) / 2) // Size is half!
 	}
-	t.MAC = hex.EncodeToString(mac)
-	t.MACSize = uint16(len(t.MAC) / 2) // Size is half!
 
 	tbuf := make([]byte, Len(t))
 	off, err := PackRR(t, tbuf, 0, nil, false)
diff --git a/version.go b/version.go
index 622c69a1..8863a035 100644
--- a/version.go
+++ b/version.go
@@ -3,7 +3,7 @@ package dns
 import "fmt"
 
 // Version is current version of this library.
-var Version = v{1, 1, 43}
+var Version = v{1, 1, 44}
 
 // v holds the version of this library.
 type v struct {


Test patch:
diff --git a/edns_test.go b/edns_test.go
index dc4ea7b3..b7c15f7e 100644
--- a/edns_test.go
+++ b/edns_test.go
@@ -1,6 +1,7 @@
 package dns
 
 import (
+	"bytes"
 	"net"
 	"testing"
 )
@@ -192,3 +193,87 @@ func TestEDNS0_ESU(t *testing.T) {
 		t.Errorf("unpacked option is different; expected %v, got %v", expect, esu.Uri)
 	}
 }
+
+func TestEDNS0_TCP_KEEPALIVE_unpack(t *testing.T) {
+	cases := []struct {
+		name        string
+		b           []byte
+		expected    uint16
+		expectedErr bool
+	}{
+		{
+			name:     "empty",
+			b:        []byte{},
+			expected: 0,
+		},
+		{
+			name:     "timeout 1",
+			b:        []byte{0, 1},
+			expected: 1,
+		},
+		{
+			name:        "invalid",
+			b:           []byte{0, 1, 3},
+			expectedErr: true,
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			e := &EDNS0_TCP_KEEPALIVE{}
+			err := e.unpack(tc.b)
+			if err != nil && !tc.expectedErr {
+				t.Error("failed to unpack, expected no error")
+			}
+			if err == nil && tc.expectedErr {
+				t.Error("unpacked, but expected an error")
+			}
+			if e.Timeout != tc.expected {
+				t.Errorf("invalid timeout, actual: %d, expected: %d", e.Timeout, tc.expected)
+			}
+		})
+	}
+}
+
+func TestEDNS0_TCP_KEEPALIVE_pack(t *testing.T) {
+	cases := []struct {
+		name     string
+		edns     *EDNS0_TCP_KEEPALIVE
+		expected []byte
+	}{
+		{
+			name: "empty",
+			edns: &EDNS0_TCP_KEEPALIVE{
+				Code:    EDNS0TCPKEEPALIVE,
+				Timeout: 0,
+			},
+			expected: nil,
+		},
+		{
+			name: "timeout 1",
+			edns: &EDNS0_TCP_KEEPALIVE{
+				Code:    EDNS0TCPKEEPALIVE,
+				Timeout: 1,
+			},
+			expected: []byte{0, 1},
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			b, err := tc.edns.pack()
+			if err != nil {
+				t.Error("expected no error")
+			}
+
+			if tc.expected == nil && b != nil {
+				t.Errorf("invalid result, expected nil")
+			}
+
+			res := bytes.Compare(b, tc.expected)
+			if res != 0 {
+				t.Errorf("invalid result, expected: %v, actual: %v", tc.expected, b)
+			}
+		})
+	}
+}
diff --git a/tsig_test.go b/tsig_test.go
index 138ec87b..25a3127b 100644
--- a/tsig_test.go
+++ b/tsig_test.go
@@ -55,6 +55,62 @@ func TestTsigCase(t *testing.T) {
 	}
 }
 
+func TestTsigErrorResponse(t *testing.T) {
+	for _, rcode := range []uint16{RcodeBadSig, RcodeBadKey} {
+		m := newTsig(strings.ToUpper(HmacSHA256))
+		m.IsTsig().Error = rcode
+		buf, _, err := TsigGenerate(m, "pRZgBrBvI4NAHZYhxmhs/Q==", "", false)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		err = m.Unpack(buf)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		mTsig := m.IsTsig()
+		if mTsig.MAC != "" {
+			t.Error("Expected empty MAC")
+		}
+		if mTsig.MACSize != 0 {
+			t.Error("Expected 0 MACSize")
+		}
+		if mTsig.TimeSigned != 0 {
+			t.Errorf("Expected TimeSigned to be 0, got %v", mTsig.TimeSigned)
+		}
+	}
+}
+
+func TestTsigBadTimeResponse(t *testing.T) {
+	clientTime := uint64(time.Now().Unix()) - 3600
+	m := newTsig(strings.ToUpper(HmacSHA256))
+	m.IsTsig().Error = RcodeBadTime
+	m.IsTsig().TimeSigned = clientTime
+
+	buf, _, err := TsigGenerate(m, "pRZgBrBvI4NAHZYhxmhs/Q==", "", false)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	err = m.Unpack(buf)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	mTsig := m.IsTsig()
+	if mTsig.MAC == "" {
+		t.Error("Expected non-empty MAC")
+	}
+	if int(mTsig.MACSize) != len(mTsig.MAC)/2 {
+		t.Errorf("Expected MACSize %v, got %v", len(mTsig.MAC)/2, mTsig.MACSize)
+	}
+
+	if mTsig.TimeSigned != clientTime {
+		t.Errorf("Expected TimeSigned %v to be retained, got %v", clientTime, mTsig.TimeSigned)
+	}
+}
+
 const (
 	// A template wire-format DNS message (in hex form) containing a TSIG RR.
 	// Its time signed field will be filled by tests.