hockeypuck team mailing list archive

[Joel Sing <mp+413895@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing



Diff comments:

> 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

I presume 58498822KB should be 58498822 bytes (i.e. 55MB)?

Either way, this should some more sensible value like 128MB or 256MB. And it would be even better if we could configure this in the config file so that we can adjust it without changing the code.

I'd probably also stick with "Response" rather than "Resp" throughout - it is only slightly longer, but more readable.

>  )
>  
>  var errKeywordSearchNotAvailable = errors.New("keyword search is not available")
> @@ -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) {

This appears to only exist so we can change maxRespLen for testing - if that's the case, we should either set the value directly or provide a setter:

h := NewHandler(...)
h.maxResponseLen = 134

Or:

h := NewHandler(...)
h.SetMaxResponseLen(134)

> +	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

responseLen would be more readable and matches maxResponseLen.

>  	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)

I would suggest something like:

"Limiting response to %d bytes (maximum %d bytes)", responseLen, h.maxResponseLen)

This also means that you need to do the length check before the addition:

if responseLen + keys[0].Length > h.maxResponseLen {
   ...
}
responseLen = responseLen + keys[0].Length
result = append(result, keys...)

There is also another issue here - this is only checking the length of the first key, while the append is appending multiple keys.

> +			break
> +		}
>  	}
>  
>  	w.Header().Set("Content-Type", "pgp/keys")
>  
>  	// Write the number of keys
> -	err = recon.WriteInt(w, len(result))
> +	_ = recon.WriteInt(w, len(result))

This should either check the error, or remove the assignment.

>  	for _, key := range result {
>  		// Write each key in binary packet format, prefixed with length
>  		err = writeHashqueryKey(w, key)


-- 
https://code.launchpad.net/~alejdg/hockeypuck/+git/hockeypuck/+merge/413895
Your team Hockeypuck is subscribed to branch hockeypuck:master.