fix: check apikeys for otlp requests too (+tests) (#672)

## Which problem is this PR solving?

- #671 -- Refinery's APIKeys configuration value was not being used for
OTel requests

## Short description of the changes

- Refactor middleware to pull out the key-validation logic
- Use it in the http middleware
- Call it in the OTLP handler
- Add a normal (mocked) test for OTLP
- Add an integration test for HTTP

app/app_test.go

@@ -261,7 +261,8 @@ func TestAppIntegration(t *testing.T) {
 }
 
 func TestAppIntegrationWithNonLegacyKey(t *testing.T) {
-	t.Parallel()
+	// This is failing in Parallel, so disable it for now.
+	// t.Parallel()
 
 	var out bytes.Buffer
 	a, graph := newStartedApp(t, &transmission.WriterSender{W: &out}, 10500, nil, false)
@@ -301,6 +302,35 @@ func TestAppIntegrationWithNonLegacyKey(t *testing.T) {
 	assert.Equal(t, `{"data":{"foo":"bar","meta.refinery.original_sample_rate":1,"trace.trace_id":"1"},"dataset":"dataset"}`+"\n", out.String())
 }
 
+func TestAppIntegrationWithUnauthorizedKey(t *testing.T) {
+	t.Parallel()
+
+	var out bytes.Buffer
+	a, graph := newStartedApp(t, &transmission.WriterSender{W: &out}, 10500, nil, false)
+	a.IncomingRouter.SetEnvironmentCache(time.Second, func(s string) (string, error) { return "test", nil })
+	a.PeerRouter.SetEnvironmentCache(time.Second, func(s string) (string, error) { return "test", nil })
+
+	// Send a root span, it should be sent in short order.
+	req := httptest.NewRequest(
+		"POST",
+		"http://localhost:10500/v1/traces",
+		strings.NewReader(`[{"data":{"trace.trace_id":"1","foo":"bar"}}]`),
+	)
+	req.Header.Set("X-Honeycomb-Team", "badkey")
+	req.Header.Set("Content-Type", "application/json")
+
+	resp, err := http.DefaultTransport.RoundTrip(req)
+	assert.NoError(t, err)
+	assert.Equal(t, 400, resp.StatusCode)
+	data, err := io.ReadAll(resp.Body)
+	resp.Body.Close()
+	assert.NoError(t, err)
+	assert.Contains(t, string(data), "not found in list of authed keys")
+
+	err = startstop.Stop(graph.Objects(), nil)
+	assert.NoError(t, err)
+}
+
 func TestPeerRouting(t *testing.T) {
 	t.Parallel()
 

route/middleware.go

@@ -37,6 +37,25 @@ func (r *Router) queryTokenChecker(next http.Handler) http.Handler {
 	})
 }
 
+func (r *Router) isKeyAllowed(key string) bool {
+	allowedKeys, err := r.Config.GetAPIKeys()
+	if len(allowedKeys) == 0 {
+		return true
+	}
+	if err != nil {
+		return false
+	}
+	for _, allowedKey := range allowedKeys {
+		if allowedKey == "*" {
+			return true
+		}
+		if allowedKey == key {
+			return true
+		}
+	}
+	return false
+}
+
 func (r *Router) apiKeyChecker(next http.Handler) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
 		apiKey := req.Header.Get(types.APIKeyHeader)
@@ -48,24 +67,11 @@ func (r *Router) apiKeyChecker(next http.Handler) http.Handler {
 			r.handlerReturnWithError(w, ErrAuthNeeded, err)
 			return
 		}
-		allowedKeys, err := r.Config.GetAPIKeys()
-		if err != nil {
-			r.handlerReturnWithError(w, ErrConfigReadFailed, err)
+		if r.isKeyAllowed(apiKey) {
+			next.ServeHTTP(w, req)
 			return
 		}
-		for _, key := range allowedKeys {
-			if key == "*" {
-				// all keys are allowed, it's all good
-				next.ServeHTTP(w, req)
-				return
-			}
-			if apiKey == key {
-				// we're in the allowlist, it's all good
-				next.ServeHTTP(w, req)
-				return
-			}
-		}
-		err = fmt.Errorf("api key %s not found in list of authed keys", apiKey)
+		err := fmt.Errorf("api key %s not found in list of authed keys", apiKey)
 		r.handlerReturnWithError(w, ErrAuthNeeded, err)
 	})
 }

route/otlp_trace.go

@@ -3,6 +3,7 @@ package route
 import (
 	"context"
 	"errors"
+	"fmt"
 	"net/http"
 
 	huskyotlp "github.com/honeycombio/husky/otlp"
@@ -13,6 +14,13 @@ import (
 
 func (r *Router) postOTLP(w http.ResponseWriter, req *http.Request) {
 	ri := huskyotlp.GetRequestInfoFromHttpHeaders(req.Header)
+
+	if !r.isKeyAllowed(ri.ApiKey) {
+		err := fmt.Errorf("api key %s not found in list of authed keys", ri.ApiKey)
+		r.handlerReturnWithError(w, ErrAuthNeeded, err)
+		return
+	}
+
 	if err := ri.ValidateTracesHeaders(); err != nil {
 		if errors.Is(err, huskyotlp.ErrInvalidContentType) {
 			r.handlerReturnWithError(w, ErrInvalidContentType, err)

route/otlp_trace_test.go

@@ -386,6 +386,36 @@ func TestOTLPHandler(t *testing.T) {
 		assert.Equal(t, "local", event.Environment)
 		mockTransmission.Flush()
 	})
+
+	t.Run("rejects bad API keys", func(t *testing.T) {
+		router.Config.(*config.MockConfig).GetAPIKeysVal = []string{"bad-key"}
+		req := &collectortrace.ExportTraceServiceRequest{
+			ResourceSpans: []*trace.ResourceSpans{{
+				ScopeSpans: []*trace.ScopeSpans{{
+					Spans: helperOTLPRequestSpansWithStatus(),
+				}},
+			}},
+		}
+		body, err := protojson.Marshal(req)
+		if err != nil {
+			t.Error(err)
+		}
+
+		request, _ := http.NewRequest("POST", "/v1/traces", bytes.NewReader(body))
+		request.Header = http.Header{}
+		request.Header.Set("content-type", "application/json")
+		request.Header.Set("x-honeycomb-team", legacyAPIKey)
+		request.Header.Set("x-honeycomb-dataset", "dataset")
+
+		w := httptest.NewRecorder()
+		router.postOTLP(w, request)
+		assert.Equal(t, http.StatusBadRequest, w.Code)
+		assert.Contains(t, w.Body.String(), "not found in list of authed keys")
+
+		assert.Equal(t, 0, len(mockTransmission.Events))
+		mockTransmission.Flush()
+	})
+
 }
 
 func helperOTLPRequestSpansWithoutStatus() []*trace.Span {