From 471fd63d977c96ad3b4d2d7dff15cb4b094f4b44 Mon Sep 17 00:00:00 2001 From: Derek Trider Date: Mon, 18 Jul 2022 13:28:25 -0400 Subject: [PATCH] fix: GNAP header inspection bug The GNAP middleware's Accept method checks each value in the Authorization header slice and if any of them contain a GNAP token, then the middleware handles it. However, the middleware Handle method then uses the Header.Get method, which always gets the first value from the Header slice. Thus, if a request comes in with a GNAP token in the Authorization header but in any value but the first, the handler will accept it but then fail to handle it since it will miss the token. Also updated code that access headers to use the built-in Header methods to retrieve the values (instead of directly accessing the map) since they ensure that values are canonicalized. Signed-off-by: Derek Trider --- .../mw/authmw/gnapmw/gnap_middleware.go | 28 ++++++---- .../mw/authmw/gnapmw/gnap_middleware_test.go | 52 +++++++++++++++++++ .../mw/authmw/zcapmw/zcap_middleware.go | 4 +- 3 files changed, 73 insertions(+), 11 deletions(-) diff --git a/pkg/controller/mw/authmw/gnapmw/gnap_middleware.go b/pkg/controller/mw/authmw/gnapmw/gnap_middleware.go index 1b0b623d..b48cb264 100644 --- a/pkg/controller/mw/authmw/gnapmw/gnap_middleware.go +++ b/pkg/controller/mw/authmw/gnapmw/gnap_middleware.go @@ -69,13 +69,12 @@ type HTTPHandler interface { // Accept checks if the request can be handled by the GNAP middleware. func (mw *Middleware) Accept(req *http.Request) bool { - if v, ok := req.Header["Authorization"]; ok { - for _, h := range v { - if strings.Contains(h, gnapToken) { - logger.Debugf("Accept: %v is true", v) + v := req.Header.Values("Authorization") + for _, h := range v { + if strings.Contains(h, gnapToken) { + logger.Debugf("Accept: %v is true", v) - return true - } + return true } } @@ -108,9 +107,20 @@ type gnapHandler struct { // ServeHTTP authorizes an incoming HTTP request using GNAP. func (h *gnapHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) { - tokenHeader := strings.Split(strings.Trim(req.Header.Get("Authorization"), " "), " ") + var tokenHeader string - if len(tokenHeader) < 2 || tokenHeader[0] != gnapToken { + v := req.Header.Values("Authorization") + for _, h := range v { + if strings.Contains(h, gnapToken) { + tokenHeader = h + + break + } + } + + tokenHeaderSplit := strings.Split(strings.Trim(tokenHeader, " "), " ") + + if len(tokenHeaderSplit) < 2 || tokenHeaderSplit[0] != gnapToken { http.Error(w, "unauthorized", http.StatusUnauthorized) return @@ -121,7 +131,7 @@ func (h *gnapHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) { Key: h.clientKey, }, Proof: proofType, - AccessToken: tokenHeader[1], + AccessToken: tokenHeaderSplit[1], } resp, err := h.client.Introspect(introspectReq) diff --git a/pkg/controller/mw/authmw/gnapmw/gnap_middleware_test.go b/pkg/controller/mw/authmw/gnapmw/gnap_middleware_test.go index 473b0984..a1edf704 100644 --- a/pkg/controller/mw/authmw/gnapmw/gnap_middleware_test.go +++ b/pkg/controller/mw/authmw/gnapmw/gnap_middleware_test.go @@ -167,6 +167,58 @@ func TestMiddleware(t *testing.T) { require.Equal(t, http.StatusOK, rr.Code) }) + t.Run("should call next handler if request is authorized "+ + "(GNAP token is second value in Authorization header)", func(t *testing.T) { + ctrl := gomock.NewController(t) + + privKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + + privJWK := &jwk.JWK{ + JSONWebKey: jose.JSONWebKey{ + Key: privKey, + KeyID: "key1", + Algorithm: "ES256", + }, + Kty: "EC", + Crv: "P-256", + } + + pubJWK := jwk.JWK{ + JSONWebKey: privJWK.Public(), + Kty: "EC", + Crv: "P-256", + } + + require.NoError(t, err) + client := NewMockGNAPRSClient(ctrl) + client.EXPECT().Introspect(gomock.Any()).Return(&gnap.IntrospectResponse{Active: true, Key: &gnap.ClientKey{ + JWK: pubJWK, + }}, nil) + + mVerifier := NewMockGNAPVerifier(ctrl) + mVerifier.EXPECT().Verify(gomock.Any()).Times(1) + + mw, err := gnapmw.NewMiddleware(client, &pubJWK, func(req *http.Request) gnapmw.GNAPVerifier { + return mVerifier + }, false) + require.NoError(t, err) + + next := NewMockHTTPHandler(ctrl) + next.EXPECT().ServeHTTP(gomock.Any(), gomock.Any()).Times(1) + + req, err := http.NewRequestWithContext(context.Background(), "", "", nil) + require.NoError(t, err) + + req.Header.Add("Authorization", "Some other token") + req.Header.Add("Authorization", "GNAP token") + + rr := httptest.NewRecorder() + + mw.Middleware()(next).ServeHTTP(rr, req) + + require.Equal(t, http.StatusOK, rr.Code) + }) + t.Run("should return 401 Unauthorized if request is unauthorized due to signature error", func(t *testing.T) { ctrl := gomock.NewController(t) diff --git a/pkg/controller/mw/authmw/zcapmw/zcap_middleware.go b/pkg/controller/mw/authmw/zcapmw/zcap_middleware.go index 61acc828..259a4138 100644 --- a/pkg/controller/mw/authmw/zcapmw/zcap_middleware.go +++ b/pkg/controller/mw/authmw/zcapmw/zcap_middleware.go @@ -62,9 +62,9 @@ type Middleware struct { // Accept checks if middleware can handle auth for the given request. func (mw *Middleware) Accept(req *http.Request) bool { - _, ok := req.Header["Capability-Invocation"] + headerValues := req.Header.Values("Capability-Invocation") - return ok + return len(headerValues) > 0 } // Middleware returns middleware func.