hockeypuck team mailing list archive

[Alexandre Gomes <mp+413895@xxxxxxxxxxxxxxxxxx>]

Alexandre Gomes has proposed merging ~alejdg/hockeypuck:124887 into hockeypuck:master.

Requested reviews:
  Hockeypuck (hockeypuck)

For more details, see:
https://code.launchpad.net/~alejdg/hockeypuck/+git/hockeypuck/+merge/413895
-- 
Your team Hockeypuck is requested to review the proposed merge of ~alejdg/hockeypuck:124887 into hockeypuck:master.

diff --git a/src/hockeypuck/hkp/handler.go b/src/hockeypuck/hkp/handler.go
index 00ce4b3..3adecf6 100644
--- a/src/hockeypuck/hkp/handler.go
+++ b/src/hockeypuck/hkp/handler.go
@@ -45,6 +45,7 @@ const (
 	shortKeyIDLen       = 8
 	longKeyIDLen        = 16
 	fingerprintKeyIDLen = 40
+	maxRespLen          = 116997644
 )
 
 var errKeywordSearchNotAvailable = errors.New("keyword search is not available")
@@ -70,6 +71,7 @@ type Handler struct {
 
 	keyReaderOptions []openpgp.KeyReaderOption
 	keyWriterOptions []openpgp.KeyWriterOption
+	maxRespLen       int
 }
 
 type HandlerOption func(h *Handler) error
@@ -160,7 +162,8 @@ func KeyWriterOptions(opts []openpgp.KeyWriterOption) HandlerOption {
 
 func NewHandler(storage storage.Storage, options ...HandlerOption) (*Handler, error) {
 	h := &Handler{
-		storage: storage,
+		storage:    storage,
+		maxRespLen: maxRespLen,
 	}
 	for _, option := range options {
 		err := option(h)
@@ -171,6 +174,15 @@ func NewHandler(storage storage.Storage, options ...HandlerOption) (*Handler, er
 	return h, nil
 }
 
+func NewHandlerWithLen(storage storage.Storage, maxRespLen int, options ...HandlerOption) (*Handler, error) {
+	h, err := NewHandler(storage, options...)
+	if err != nil {
+		return nil, errors.WithStack(err)
+	}
+	h.maxRespLen = maxRespLen
+	return h, nil
+}
+
 func (h *Handler) Register(r *httprouter.Router) {
 	r.GET("/pks/lookup", h.Lookup)
 	r.POST("/pks/add", h.Add)
@@ -207,6 +219,7 @@ func (h *Handler) HashQuery(w http.ResponseWriter, r *http.Request, _ httprouter
 		return
 	}
 	var result []*openpgp.PrimaryKey
+	rl := 0
 	for _, digest := range hq.Digests {
 		rfps, err := h.storage.MatchMD5([]string{digest})
 		if err != nil {
@@ -219,12 +232,17 @@ func (h *Handler) HashQuery(w http.ResponseWriter, r *http.Request, _ httprouter
 			continue
 		}
 		result = append(result, keys...)
+		rl = rl + keys[0].Length
+		if rl >= h.maxRespLen {
+			log.Infof("Response too lengthy, limiting it to %dKB", h.maxRespLen)
+			break
+		}
 	}
 
 	w.Header().Set("Content-Type", "pgp/keys")
 
 	// Write the number of keys
-	err = recon.WriteInt(w, len(result))
+	_ = recon.WriteInt(w, len(result))
 	for _, key := range result {
 		// Write each key in binary packet format, prefixed with length
 		err = writeHashqueryKey(w, key)
@@ -571,7 +589,6 @@ func (h *Handler) Delete(w http.ResponseWriter, r *http.Request, _ httprouter.Pa
 		"deleted": []string{signingFp},
 	}).Info("delete")
 
-	return
 }
 
 func (h *Handler) checkSignature(keytext, keysig string) (string, error) {
diff --git a/src/hockeypuck/hkp/handler_test.go b/src/hockeypuck/hkp/handler_test.go
index 85ca3b6..6121995 100644
--- a/src/hockeypuck/hkp/handler_test.go
+++ b/src/hockeypuck/hkp/handler_test.go
@@ -19,6 +19,7 @@ package hkp
 
 import (
 	"bytes"
+	"crypto/md5"
 	"encoding/json"
 	"fmt"
 	"io/ioutil"
@@ -30,6 +31,7 @@ import (
 	"github.com/julienschmidt/httprouter"
 	gc "gopkg.in/check.v1"
 
+	"hockeypuck/conflux/recon"
 	"hockeypuck/openpgp"
 	"hockeypuck/testing"
 
@@ -72,6 +74,8 @@ func Test(t *stdtesting.T) { gc.TestingT(t) }
 type HandlerSuite struct {
 	storage *mock.Storage
 	srv     *httptest.Server
+	handler *Handler
+	digests int
 }
 
 var _ = gc.Suite(&HandlerSuite{})
@@ -97,8 +101,10 @@ func (s *HandlerSuite) SetUpTest(c *gc.C) {
 	r := httprouter.New()
 	handler, err := NewHandler(s.storage)
 	c.Assert(err, gc.IsNil)
-	handler.Register(r)
+	s.handler = handler
+	s.handler.Register(r)
 	s.srv = httptest.NewServer(r)
+	s.digests = 50
 }
 
 func (s *HandlerSuite) TearDownTest(c *gc.C) {
@@ -246,3 +252,55 @@ func (s *HandlerSuite) TestFetchWithBadSigs(c *gc.C) {
 	c.Assert(keys[0].ShortID(), gc.Equals, tk.sid)
 	c.Assert(len(keys[0].Others), gc.Equals, 0)
 }
+
+func (s *HandlerSuite) SetupHashQueryTest(c *gc.C) (*httptest.ResponseRecorder, *http.Request) {
+	// Determine reference digest to compare with
+	h := md5.New()
+	refDigest := h.Sum(nil)
+	url, err := url.Parse("/pks/hashquery")
+	c.Assert(err, gc.IsNil)
+	var buf bytes.Buffer
+	c.Assert(err, gc.IsNil)
+	err = recon.WriteInt(&buf, s.digests)
+	c.Assert(err, gc.IsNil)
+	for i := 0; i < s.digests; i++ {
+		err = recon.WriteInt(&buf, len(refDigest))
+		c.Assert(err, gc.IsNil)
+		_, err = buf.Write(refDigest)
+		c.Assert(err, gc.IsNil)
+	}
+	// Create an HTTP request
+	req := &http.Request{
+		Method: "POST",
+		URL:    url,
+		Body:   ioutil.NopCloser(bytes.NewBuffer(buf.Bytes())),
+	}
+	w := httptest.NewRecorder()
+
+	return w, req
+}
+
+func (s *HandlerSuite) TestHashQuery(c *gc.C) {
+	w, req := s.SetupHashQueryTest(c)
+	s.handler.HashQuery(w, req, nil)
+	// Number of keys in response based on the length estimation of one key
+	nk := w.Body.Len() / 1446
+	// The number of keys should be the same as the number of digests
+	c.Assert(nk, gc.Equals, s.digests)
+}
+
+func (s *HandlerSuite) TestHashQueryResponseTooLong(c *gc.C) {
+	var err error
+	w, req := s.SetupHashQueryTest(c)
+
+	// Test HashQuery when the response is too long
+	// Reduce the response max length for testing purposes
+	s.handler, err = NewHandlerWithLen(s.storage, 14460)
+	c.Assert(err, gc.IsNil)
+	s.handler.HashQuery(w, req, nil)
+	// Number of keys in response based on the length estimation of one key
+	nk := w.Body.Len() / 1446
+	// The number of keys cannot be the same as the number of digests as the response
+	// is being limited
+	c.Assert(nk, gc.Not(gc.Equals), s.digests)
+}