improve padding (#4)

* ensures that padding of request URLs sends only as many characters as is required

Author: fardog

README.md

@@ -57,8 +57,6 @@ consider the following:
 ## Caveats/TODO
 
 * Currently only the following records are supported: `A, AAAA, CNAME, MX`
-* Padding is not very smart; it just always pads to 1024 characters, and fails
-  if the URL would've been larger than that
 * More thorough tests should be written
 * No caching is implemented, and probably never will. If you need caching, put
   your `secure-operator` server behind another DNS server which provides

provider_google.go

@@ -6,9 +6,12 @@ import (
 	"net/http"
 )
 
-var (
-	targetPaddedLength = 1024
-	paddingParameter   = "random_padding"
+const (
+	// DNSNameMaxBytes is the maximum number of bytes a DNS name may contain
+	DNSNameMaxBytes = 253
+	// max number of characters in a 16-bit uint integer, converted to string
+	extraPad         = 5
+	paddingParameter = "random_padding"
 )
 
 // GDNSQuestion represents a question response item from Google's DNS service
@@ -95,23 +98,24 @@ func (g GDNSProvider) Query(q DNSQuestion) (*DNSResponse, error) {
 	}
 
 	qry := httpreq.URL.Query()
+	dnsType := fmt.Sprintf("%v", q.Type)
+
+	l := len([]byte(q.Name))
+	if l > DNSNameMaxBytes {
+		return nil, fmt.Errorf("name length of %v exceeds DNS name max length", l)
+	}
 
 	qry.Add("name", q.Name)
-	qry.Add("type", fmt.Sprintf("%v", q.Type))
+	qry.Add("type", dnsType)
 	qry.Add("edns_client_subnet", "0.0.0.0/0")
 
 	httpreq.URL.RawQuery = qry.Encode()
 
-	// if padding was requested, pad to the target padding length
-	// TODO: this needs to be smarter; should be padding to more sane lengths
-	// except for very large name requests
 	if g.Pad {
-		l := len(httpreq.URL.String()) + len(paddingParameter) + 1
-
-		if l > targetPaddedLength {
-			return nil, fmt.Errorf("failed to pad; query was already of length: %v", l)
-		}
-		pad := randSeq(targetPaddedLength - l)
+		// pad to the maximum size a valid request could be. we add `1` because
+		// Google's DNS service ignores a trailing period, increasing the
+		// possible size of a name by 1
+		pad := randSeq(DNSNameMaxBytes + extraPad - l - len(dnsType) + 1)
 		qry.Add(paddingParameter, pad)
 
 		httpreq.URL.RawQuery = qry.Encode()

provider_google_test.go

@@ -5,6 +5,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"strconv"
+	"strings"
 	"testing"
 
 	"github.com/miekg/dns"
@@ -27,6 +28,7 @@ func TestQuery(t *testing.T) {
 		w.Header().Set("Content-Type", "application/json")
 		fmt.Fprint(w, gresp)
 	}))
+	defer ts.Close()
 
 	g := GDNSProvider{
 		Endpoint: ts.URL,
@@ -45,7 +47,7 @@ func TestQuery(t *testing.T) {
 	if a.Name != "example.com." {
 		t.Errorf("unexpected name %v", a.Name)
 	}
-	if a.Type != 1 {
+	if a.Type != dns.TypeA {
 		t.Errorf("unexpected type %v", a.Type)
 	}
 	if a.Data != "93.184.216.34" {
@@ -59,3 +61,69 @@ func TestQuery(t *testing.T) {
 		t.Errorf("unexpected response code %v", resp.ResponseCode)
 	}
 }
+
+func TestPadding(t *testing.T) {
+	var expected int
+	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		l := len([]byte(r.URL.String()))
+		// first request, set the expectation to the length of the first URL we
+		// see; if any others don't match it, our padding function fails
+		if expected == 0 {
+			expected = l
+		}
+
+		if l != expected {
+			t.Errorf("unexpected URL length: %v, expected: %v", l, expected)
+		}
+
+		w.WriteHeader(http.StatusOK)
+		w.Header().Set("Content-Type", "application/json")
+		fmt.Fprint(w, gresp)
+	}))
+	defer ts.Close()
+
+	g := GDNSProvider{
+		Endpoint: ts.URL,
+		Pad:      true,
+	}
+
+	questions := []DNSQuestion{
+		DNSQuestion{Name: "whatever.yo", Type: dns.TypeA},
+		// ensure differing types result in the same padded length
+		DNSQuestion{Name: "sure.yep", Type: dns.TypeA},
+		DNSQuestion{Name: "sure.yep", Type: dns.TypeMX},
+		DNSQuestion{Name: "sure.yep", Type: dns.TypeTA},
+		// ensure very long domains are fine
+		DNSQuestion{Name: strings.Repeat("a", 253), Type: dns.TypeA},
+		DNSQuestion{Name: strings.Repeat("a", 253), Type: dns.TypeTA},
+	}
+
+	for _, q := range questions {
+		if _, err := g.Query(q); err != nil {
+			t.Errorf(err.Error())
+		}
+
+	}
+
+	// a name longer that 253 bytes should be an error
+	if _, err := g.Query(DNSQuestion{Name: strings.Repeat("a", 254)}); err == nil {
+		t.Errorf("expected an error for a too-long DNS name")
+	}
+}
+
+func TestNameTooLong(t *testing.T) {
+	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		t.Errorf("no request should be made")
+	}))
+	defer ts.Close()
+
+	g := GDNSProvider{
+		Endpoint: ts.URL,
+		Pad:      true,
+	}
+
+	// a name longer that 253 bytes should be an error
+	if _, err := g.Query(DNSQuestion{Name: strings.Repeat("a", 254)}); err == nil {
+		t.Errorf("expected an error for a too-long DNS name")
+	}
+}