From a51c7140eaf0700fe6b522b623725f501e4e0551 Mon Sep 17 00:00:00 2001 From: Caleb Doxsey Date: Wed, 7 Apr 2021 14:57:24 -0600 Subject: [PATCH] cryptutil: use bytes for hmac (#2067) --- authenticate/handlers.go | 6 +++--- authenticate/handlers_test.go | 2 +- authenticate/middleware.go | 4 ++-- authorize/check_response.go | 2 +- internal/httputil/reproxy/reproxy.go | 4 ++-- internal/middleware/middleware.go | 4 ++-- internal/middleware/middleware_test.go | 8 ++++---- internal/urlutil/signed.go | 6 +++--- internal/urlutil/signed_test.go | 6 +++--- pkg/cryptutil/hmac.go | 6 +++--- pkg/cryptutil/hmac_test.go | 4 ++-- proxy/state.go | 4 ++-- 12 files changed, 28 insertions(+), 28 deletions(-) diff --git a/authenticate/handlers.go b/authenticate/handlers.go index c55d22a7b..c70472535 100644 --- a/authenticate/handlers.go +++ b/authenticate/handlers.go @@ -79,7 +79,7 @@ func (a *Authenticate) mountDashboard(r *mux.Router) { c := cors.New(cors.Options{ AllowOriginRequestFunc: func(r *http.Request, _ string) bool { state := a.state.Load() - err := middleware.ValidateRequestURL(r, string(state.sharedSecret)) + err := middleware.ValidateRequestURL(r, state.sharedSecret) if err != nil { log.FromRequest(r).Info().Err(err).Msg("authenticate: origin blocked") } @@ -243,7 +243,7 @@ func (a *Authenticate) SignIn(w http.ResponseWriter, r *http.Request) error { // build our hmac-d redirect URL with our session, pointing back to the // proxy's callback URL which is responsible for setting our new route-session - uri := urlutil.NewSignedURL(options.SharedKey, callbackURL) + uri := urlutil.NewSignedURL([]byte(options.SharedKey), callbackURL) httputil.Redirect(w, r, uri.String(), http.StatusFound) return nil } @@ -606,5 +606,5 @@ func (a *Authenticate) getSignOutURL(r *http.Request) (*url.URL, error) { urlutil.QueryRedirectURI: {redirectURI}, }).Encode() } - return urlutil.NewSignedURL(a.options.Load().SharedKey, uri).Sign(), nil + return urlutil.NewSignedURL([]byte(a.options.Load().SharedKey), uri).Sign(), nil } diff --git a/authenticate/handlers_test.go b/authenticate/handlers_test.go index 00cbc39d8..bbb9760e0 100644 --- a/authenticate/handlers_test.go +++ b/authenticate/handlers_test.go @@ -621,7 +621,7 @@ func TestAuthenticate_userInfo(t *testing.T) { }, { "bad signature", - urlutil.NewSignedURL("BAD KEY", mustParseURL("/?pomerium_redirect_uri=http://example.com")).Sign(), + urlutil.NewSignedURL([]byte("BAD KEY"), mustParseURL("/?pomerium_redirect_uri=http://example.com")).Sign(), http.MethodGet, &mstore.Store{Encrypted: true, Session: &sessions.State{ID: "SESSION_ID", IssuedAt: jwt.NewNumericDate(now)}}, http.StatusBadRequest, diff --git a/authenticate/middleware.go b/authenticate/middleware.go index 37f4e196f..b32437fec 100644 --- a/authenticate/middleware.go +++ b/authenticate/middleware.go @@ -13,7 +13,7 @@ import ( func (a *Authenticate) requireValidSignatureOnRedirect(next httputil.HandlerFunc) http.Handler { return httputil.HandlerFunc(func(w http.ResponseWriter, r *http.Request) error { if r.FormValue(urlutil.QueryRedirectURI) != "" || r.FormValue(urlutil.QueryHmacSignature) != "" { - err := middleware.ValidateRequestURL(r, a.options.Load().SharedKey) + err := middleware.ValidateRequestURL(r, []byte(a.options.Load().SharedKey)) if err != nil { return httputil.NewError(http.StatusBadRequest, err) } @@ -25,7 +25,7 @@ func (a *Authenticate) requireValidSignatureOnRedirect(next httputil.HandlerFunc // requireValidSignature validates the pomerium_signature. func (a *Authenticate) requireValidSignature(next httputil.HandlerFunc) http.Handler { return httputil.HandlerFunc(func(w http.ResponseWriter, r *http.Request) error { - err := middleware.ValidateRequestURL(r, a.options.Load().SharedKey) + err := middleware.ValidateRequestURL(r, []byte(a.options.Load().SharedKey)) if err != nil { return err } diff --git a/authorize/check_response.go b/authorize/check_response.go index 1a3faa65a..bfa5878fd 100644 --- a/authorize/check_response.go +++ b/authorize/check_response.go @@ -167,7 +167,7 @@ func (a *Authorize) redirectResponse(in *envoy_service_auth_v3.CheckRequest) (*e q.Set(urlutil.QueryRedirectURI, url.String()) signinURL.RawQuery = q.Encode() - redirectTo := urlutil.NewSignedURL(opts.SharedKey, signinURL).String() + redirectTo := urlutil.NewSignedURL([]byte(opts.SharedKey), signinURL).String() return a.deniedResponse(in, http.StatusFound, "Login", map[string]string{ "Location": redirectTo, diff --git a/internal/httputil/reproxy/reproxy.go b/internal/httputil/reproxy/reproxy.go index f78de1d70..8d9af0c90 100644 --- a/internal/httputil/reproxy/reproxy.go +++ b/internal/httputil/reproxy/reproxy.go @@ -55,7 +55,7 @@ func (h *Handler) GetPolicyIDFromHeaders(headers http.Header) (uint64, bool) { h.mu.RLock() defer h.mu.RUnlock() - return policyID, cryptutil.CheckHMAC([]byte(policyStr), hmac, string(h.key)) + return policyID, cryptutil.CheckHMAC([]byte(policyStr), hmac, h.key) } // GetPolicyIDHeaders returns http headers for the given policy id. @@ -64,7 +64,7 @@ func (h *Handler) GetPolicyIDHeaders(policyID uint64) [][2]string { defer h.mu.RUnlock() s := strconv.FormatUint(policyID, 10) - hmac := base64.StdEncoding.EncodeToString(cryptutil.GenerateHMAC([]byte(s), string(h.key))) + hmac := base64.StdEncoding.EncodeToString(cryptutil.GenerateHMAC([]byte(s), h.key)) return [][2]string{ {httputil.HeaderPomeriumReproxyPolicy, s}, {httputil.HeaderPomeriumReproxyPolicyHMAC, hmac}, diff --git a/internal/middleware/middleware.go b/internal/middleware/middleware.go index 4222c0c7a..dd81faa3b 100644 --- a/internal/middleware/middleware.go +++ b/internal/middleware/middleware.go @@ -28,7 +28,7 @@ func SetHeaders(headers map[string]string) func(next http.Handler) http.Handler // ValidateSignature ensures the request is valid and has been signed with // the correspdoning client secret key -func ValidateSignature(sharedSecret string) func(next http.Handler) http.Handler { +func ValidateSignature(sharedSecret []byte) func(next http.Handler) http.Handler { return func(next http.Handler) http.Handler { return httputil.HandlerFunc(func(w http.ResponseWriter, r *http.Request) error { ctx, span := trace.StartSpan(r.Context(), "middleware.ValidateSignature") @@ -44,7 +44,7 @@ func ValidateSignature(sharedSecret string) func(next http.Handler) http.Handler // ValidateRequestURL validates the current absolute request URL was signed // by a given shared key. -func ValidateRequestURL(r *http.Request, key string) error { +func ValidateRequestURL(r *http.Request, key []byte) error { return urlutil.NewSignedURL(key, urlutil.GetAbsoluteURL(r)).Validate() } diff --git a/internal/middleware/middleware_test.go b/internal/middleware/middleware_test.go index 71d7fad7e..fab373b90 100644 --- a/internal/middleware/middleware_test.go +++ b/internal/middleware/middleware_test.go @@ -125,13 +125,13 @@ func TestValidateSignature(t *testing.T) { tests := []struct { name string - secretA string - secretB string + secretA []byte + secretB []byte wantStatus int wantBody string }{ - {"good", "secret", "secret", http.StatusOK, http.StatusText(http.StatusOK)}, - {"secret mistmatch", "secret", "hunter42", http.StatusBadRequest, "{\"Status\":400,\"Error\":\"Bad Request: internal/urlutil: hmac failed\"}\n"}, + {"good", []byte("secret"), []byte("secret"), http.StatusOK, http.StatusText(http.StatusOK)}, + {"secret mistmatch", []byte("secret"), []byte("hunter42"), http.StatusBadRequest, "{\"Status\":400,\"Error\":\"Bad Request: internal/urlutil: hmac failed\"}\n"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/internal/urlutil/signed.go b/internal/urlutil/signed.go index b8022ee47..b42498e4a 100644 --- a/internal/urlutil/signed.go +++ b/internal/urlutil/signed.go @@ -13,7 +13,7 @@ import ( // SignedURL is a shared-key HMAC wrapped URL. type SignedURL struct { uri url.URL - key string + key []byte signed bool // mockable time for testing @@ -24,7 +24,7 @@ type SignedURL struct { // // N.B. It is the user's responsibility to make sure the key is 256 bits and // the url is not nil. -func NewSignedURL(key string, uri *url.URL) *SignedURL { +func NewSignedURL(key []byte, uri *url.URL) *SignedURL { return &SignedURL{uri: *uri, key: key, timeNow: time.Now} // uri is copied } @@ -93,7 +93,7 @@ func (su *SignedURL) Validate() error { // hmacURL takes a redirect url string and timestamp and returns the base64 // encoded HMAC result. -func hmacURL(key string, data ...interface{}) string { +func hmacURL(key []byte, data ...interface{}) string { h := cryptutil.GenerateHMAC([]byte(fmt.Sprint(data...)), key) return base64.URLEncoding.EncodeToString(h) } diff --git a/internal/urlutil/signed_test.go b/internal/urlutil/signed_test.go index 4370eeb71..eb8d781c1 100644 --- a/internal/urlutil/signed_test.go +++ b/internal/urlutil/signed_test.go @@ -31,14 +31,14 @@ func TestSignedURL(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - signedURL := NewSignedURL(tt.key, &tt.uri) + signedURL := NewSignedURL([]byte(tt.key), &tt.uri) signedURL.timeNow = tt.origTime if diff := cmp.Diff(signedURL.String(), tt.wantStr); diff != "" { t.Errorf("signedURL() = %v", diff) } - signedURL = NewSignedURL(tt.key, &tt.uri) + signedURL = NewSignedURL([]byte(tt.key), &tt.uri) signedURL.timeNow = tt.origTime got := signedURL.Sign() @@ -89,7 +89,7 @@ func TestSignedURL_Validate(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - out := NewSignedURL(tt.key, &tt.uri) + out := NewSignedURL([]byte(tt.key), &tt.uri) out.timeNow = tt.timeNow if err := out.Validate(); (err != nil) != tt.wantErr { diff --git a/pkg/cryptutil/hmac.go b/pkg/cryptutil/hmac.go index efdb20056..9cf6499f8 100644 --- a/pkg/cryptutil/hmac.go +++ b/pkg/cryptutil/hmac.go @@ -20,15 +20,15 @@ var ( ) // GenerateHMAC produces a symmetric signature using a shared secret key. -func GenerateHMAC(data []byte, key string) []byte { - h := hmac.New(sha512.New512_256, []byte(key)) +func GenerateHMAC(data, key []byte) []byte { + h := hmac.New(sha512.New512_256, key) h.Write(data) return h.Sum(nil) } // CheckHMAC securely checks the supplied MAC against a message using the // shared secret key. -func CheckHMAC(data, suppliedMAC []byte, key string) bool { +func CheckHMAC(data, suppliedMAC, key []byte) bool { expectedMAC := GenerateHMAC(data, key) return hmac.Equal(expectedMAC, suppliedMAC) } diff --git a/pkg/cryptutil/hmac_test.go b/pkg/cryptutil/hmac_test.go index 83e2539ee..041945e30 100644 --- a/pkg/cryptutil/hmac_test.go +++ b/pkg/cryptutil/hmac_test.go @@ -34,11 +34,11 @@ func TestHMAC(t *testing.T) { keyBytes := &[32]byte{} copy(keyBytes[:], keySlice) - macDigest := GenerateHMAC(dataBytes, string(keyBytes[:])) + macDigest := GenerateHMAC(dataBytes, keyBytes[:]) if !bytes.Equal(macDigest, expectedDigest) { t.Errorf("test %d generated unexpected mac", idx) } - if !CheckHMAC(dataBytes, macDigest, string(keyBytes[:])) { + if !CheckHMAC(dataBytes, macDigest, keyBytes[:]) { t.Errorf("test %d generated unexpected mac", idx) } } diff --git a/proxy/state.go b/proxy/state.go index 9e46c37d9..5d53bb2cb 100644 --- a/proxy/state.go +++ b/proxy/state.go @@ -19,7 +19,7 @@ import ( ) type proxyState struct { - sharedKey string + sharedKey []byte sharedCipher cipher.AEAD authenticateURL *url.URL @@ -44,7 +44,7 @@ func newProxyStateFromConfig(cfg *config.Config) (*proxyState, error) { } state := new(proxyState) - state.sharedKey = cfg.Options.SharedKey + state.sharedKey = []byte(cfg.Options.SharedKey) state.sharedCipher, _ = cryptutil.NewAEADCipherFromBase64(cfg.Options.SharedKey) state.cookieSecret, _ = base64.StdEncoding.DecodeString(cfg.Options.CookieSecret)