Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 49 additions & 0 deletions gateway/log_helpers.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,15 @@
package gateway

import (
"encoding/base64"
"encoding/json"
"errors"
"io"
"net"
"net/http"
"net/url"
"strings"
"syscall"

"github.com/sirupsen/logrus"

Expand Down Expand Up @@ -65,3 +73,44 @@
}
return logger.WithFields(fields)
}

func (gw *Gateway) logJWKError(logger *logrus.Entry, jwkURL string, err error) {
if err == nil {
return
}

// typed check for content/JSON errors
var syntaxErr *json.SyntaxError
var unmarshalErr *json.UnmarshalTypeError
var b64Err base64.CorruptInputError

if errors.As(err, &syntaxErr) ||
errors.As(err, &unmarshalErr) ||
errors.As(err, &b64Err) ||
errors.Is(err, io.EOF) {

logger.WithError(err).Errorf("Invalid JWKS retrieved from endpoint: %s", jwkURL)
return
}

// string fallback check for content/JSON errors
errStr := err.Error()
if strings.Contains(errStr, "invalid JWK") || strings.Contains(errStr, "illegal base64") {
logger.WithError(err).Errorf("Invalid JWKS retrieved from endpoint: %s", jwkURL)
return
}

// network errors
var urlErr *url.Error
var netErr net.Error

// errors.As(err, &netErr) catches any error that implements the net.Error interface.
// This covers DNS errors, timeouts, connection refused, dial errors, etc.
// errors.Is(err, syscall.ECONNREFUSED) catches underlying system call errors specifically.
if errors.As(err, &urlErr) || errors.As(err, &netErr) || errors.Is(err, syscall.ECONNREFUSED) {
logger.WithError(err).Errorf("JWKS endpoint resolution failed: invalid or unreachable host %s", jwkURL)
return
}

logger.WithError(err).Errorf("Failed to fetch or decode JWKs from %s", jwkURL)
}

Check warning on line 116 in gateway/log_helpers.go

View check run for this annotation

probelabs / Visor: security

security Issue

The `jwkURL` is logged directly in multiple error messages within the `logJWKError` function. URLs, especially for authentication-related endpoints like JWKS, can sometimes contain sensitive information such as access tokens in query parameters. Logging the full URL could expose this information.
Raw output
Sanitize the URL before logging it. Parse the URL and log only the scheme, host, and path, omitting the query string. Alternatively, implement a filtering mechanism to remove known sensitive query parameters.

Example:
```go
	parsedURL, parseErr := url.Parse(jwkURL)
	sanitizedURL := jwkURL
	if parseErr == nil {
		parsedURL.RawQuery = ""
		parsedURL.Fragment = ""
		sanitizedURL = parsedURL.String()
	}
	logger.WithError(err).Errorf("JWKS endpoint resolution failed: invalid or unreachable host %s", sanitizedURL)
```
113 changes: 113 additions & 0 deletions gateway/log_helpers_test.go
Original file line number Diff line number Diff line change
@@ -1,10 +1,20 @@
package gateway

import (
"encoding/base64"
"encoding/json"
"errors"
"io"
"net"
"net/http/httptest"
"net/url"
"reflect"
"syscall"
"testing"

"github.com/sirupsen/logrus"
"github.com/sirupsen/logrus/hooks/test"
"github.com/stretchr/testify/assert"
)

func TestGetLogEntryForRequest(t *testing.T) {
Expand Down Expand Up @@ -135,3 +145,106 @@ func TestGetLogEntryForRequest(t *testing.T) {
}
}
}

func TestGatewayLogJWKError(t *testing.T) {
logger, hook := test.NewNullLogger()
entry := logrus.NewEntry(logger)

gw := &Gateway{}
testURL := "https://idp.example.com/jwks"

tests := []struct {
name string
err error
expectedLog string
shouldLog bool
}{
{
name: "Nil Error (Should not log)",
err: nil,
shouldLog: false,
},
{
name: "String-based 'invalid JWK' (go-jose mismatch)",
err: errors.New("go-jose: invalid JWK, public keys mismatch"),
expectedLog: "Invalid JWKS retrieved from endpoint: " + testURL,
shouldLog: true,
},
{
name: "JSON Syntax Error",
err: &json.SyntaxError{},
expectedLog: "Invalid JWKS retrieved from endpoint: " + testURL,
},
{
name: "JSON Unmarshal Type Error",
err: &json.UnmarshalTypeError{
Value: "number",
Type: reflect.TypeOf(""),
},
expectedLog: "Invalid JWKS retrieved from endpoint: " + testURL,
},
{
name: "Empty Body (io.EOF)",
err: io.EOF,
expectedLog: "Invalid JWKS retrieved from endpoint: " + testURL,
},
{
name: "Typed Base64 Error",
err: base64.CorruptInputError(10),
expectedLog: "Invalid JWKS retrieved from endpoint: " + testURL,
},
{
name: "String-based 'illegal base64' (go-jose fallback)",
err: errors.New("illegal base64 data at input byte 0"),
expectedLog: "Invalid JWKS retrieved from endpoint: " + testURL,
},

{
name: "URL Error",
err: &url.Error{Op: "Get", URL: testURL, Err: errors.New("timeout")},
expectedLog: "JWKS endpoint resolution failed: invalid or unreachable host " + testURL,
},
{
name: "Net DNS Error (implements net.Error)",
err: &net.DNSError{Err: "no such host", Name: "example.com", IsNotFound: true},
expectedLog: "JWKS endpoint resolution failed: invalid or unreachable host " + testURL,
},
{
name: "Net OpError (implements net.Error)",
err: &net.OpError{Op: "dial", Net: "tcp", Err: errors.New("timeout")},
expectedLog: "JWKS endpoint resolution failed: invalid or unreachable host " + testURL,
},
{
name: "Syscall Connection Refused",
err: syscall.ECONNREFUSED,
expectedLog: "JWKS endpoint resolution failed: invalid or unreachable host " + testURL,
},
{
name: "Wrapped Connection Refused",
err: &net.OpError{Op: "dial", Err: syscall.ECONNREFUSED},
expectedLog: "JWKS endpoint resolution failed: invalid or unreachable host " + testURL,
},
{
name: "Generic Error",
err: errors.New("something random"),
expectedLog: "Failed to fetch or decode JWKs from " + testURL,
},
}

for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
hook.Reset()
gw.logJWKError(entry, testURL, tc.err)

if tc.expectedLog == "" {
assert.Empty(t, hook.Entries)
return
}

if assert.Len(t, hook.Entries, 1) {
assert.Equal(t, logrus.ErrorLevel, hook.LastEntry().Level)
assert.Equal(t, tc.expectedLog, hook.LastEntry().Message)
}
})
}
}
3 changes: 3 additions & 0 deletions gateway/mw_external_oauth.go
Original file line number Diff line number Diff line change
Expand Up @@ -182,11 +182,13 @@
// Fallback to original method if client factory fails
k.Logger().Debug("[ExternalServices] Falling back to legacy JWK client due to factory error")
if jwkSet, err = getJWK(url, k.Gw.GetConfig().JWTSSLInsecureSkipVerify); err != nil {
k.Gw.logJWKError(k.Logger(), url, err)
return nil, err
}
} else {
k.Logger().Debugf("[ExternalServices] Using external services JWK client to fetch: %s", url)
if jwkSet, err = getJWKWithClient(url, client); err != nil {
k.Gw.logJWKError(k.Logger(), url, err)
return nil, err
}
}
Expand Down Expand Up @@ -215,6 +217,7 @@

decodedSource, err := base64.StdEncoding.DecodeString(jwtValidation.Source)
if err != nil {
k.Logger().WithError(err).Errorf("JWKS source decode failed: %s is not a base64 string", jwtValidation.Source)

Check failure on line 220 in gateway/mw_external_oauth.go

View check run for this annotation

probelabs / Visor: security

security Issue

The `jwtValidation.Source` field, which can contain a base64-encoded secret for JWT signing (e.g., for HMAC algorithms), is logged in cleartext if base64 decoding fails. This can expose sensitive credentials in the logs.
Raw output
Avoid logging the raw `JWTSource`. Instead, log a generic error message or a sanitized version of the source (e.g., its type or length).

```go
k.Logger().WithError(err).Error("JWKS source decode failed: input is not a valid base64 string")
```
return nil, err
}

Expand Down
51 changes: 51 additions & 0 deletions gateway/mw_external_oauth_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import (
"time"

"github.com/golang-jwt/jwt/v4"
"github.com/sirupsen/logrus"
logrustest "github.com/sirupsen/logrus/hooks/test"
"github.com/stretchr/testify/assert"

"github.com/TykTechnologies/tyk/apidef"
Expand Down Expand Up @@ -418,3 +420,52 @@ func Test_isExpired(t *testing.T) {
assert.False(t, isExpired(claimsBuilder(10*time.Minute)))
assert.True(t, isExpired(claimsBuilder(-10*time.Minute)))
}

func TestGetSecretFromJWKURL_FetchError_LogsError(t *testing.T) {
ts := StartTest(nil)
defer ts.Close()

if externalOAuthJWKCache != nil {
externalOAuthJWKCache.Flush()
}

logger, hook := logrustest.NewNullLogger()

tsServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
w.WriteHeader(http.StatusOK)
_, err := w.Write([]byte("invalid-json-content"))
assert.NoError(t, err)
}))
defer tsServer.Close()

spec := BuildAPI(func(spec *APISpec) {
spec.ExternalOAuth = apidef.ExternalOAuth{
Enabled: true,
Providers: []apidef.Provider{
{
JWT: apidef.JWTValidation{
Enabled: true,
IdentityBaseField: "user_id",
SigningMethod: RSASign,
Source: tsServer.URL,
},
},
},
}
})[0]

baseMw := &BaseMiddleware{Gw: ts.Gw, Spec: spec}
baseMw.logger = logger.WithField("mw", "ExternalOAuthMiddleware")
k := ExternalOAuthMiddleware{BaseMiddleware: baseMw}

t.Run("Standard fetch failure triggers logJWKError", func(t *testing.T) {
_, err := k.getSecretFromJWKOrConfig("any-kid", spec.ExternalOAuth.Providers[0].JWT)
assert.Error(t, err)

assert.NotEmpty(t, hook.Entries, "Expected a log entry but found none")
lastLog := hook.LastEntry()
assert.Equal(t, logrus.ErrorLevel, lastLog.Level)
assert.Contains(t, lastLog.Message, "Invalid JWKS retrieved from endpoint")
assert.Contains(t, lastLog.Message, tsServer.URL)
})
}
13 changes: 8 additions & 5 deletions gateway/mw_jwt.go
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,6 @@
}

if err != nil {
k.Logger().WithError(err).Infof("Failed to fetch or decode JWKs from %s", jwk.URL)
continue
}

Expand Down Expand Up @@ -180,14 +179,14 @@
if !found {
resp, err := client.Get(url)
if err != nil {
k.Logger().WithError(err).Error("Failed to get resource URL")
k.Gw.logJWKError(k.Logger(), url, err)
return nil, err
}
defer resp.Body.Close()

// Decode it
if err := json.NewDecoder(resp.Body).Decode(&jwkSet); err != nil {
k.Logger().WithError(err).Error("Failed to decode body JWK")
k.Gw.logJWKError(k.Logger(), url, err)
return nil, err
}

Expand Down Expand Up @@ -242,7 +241,8 @@

decodedURL, err := base64.StdEncoding.DecodeString(cachedAPIDef.JWTSource)
if err != nil {
k.Logger().WithError(err).Errorf("JWKS source decode failed: %s is not a base64 string", cachedAPIDef.JWTSource)
return nil, err

Check failure on line 245 in gateway/mw_jwt.go

View check run for this annotation

probelabs / Visor: security

security Issue

The `cachedAPIDef.JWTSource` field, which can contain a base64-encoded secret for JWT signing (e.g., for HMAC algorithms), is logged in cleartext if base64 decoding fails. This can expose sensitive credentials in the logs.
Raw output
Avoid logging the raw `JWTSource`. Instead, log a generic error message or a sanitized version of the source (e.g., its type or length).

```go
k.Logger().WithError(err).Error("JWKS source decode failed: input is not a valid base64 string")
```
}

if string(decodedURL) != url {
Expand All @@ -269,14 +269,16 @@
client, clientErr := clientFactory.CreateJWKClient()
if clientErr == nil {
if jwkSet, err = getJWKWithClient(url, client); err != nil {
k.Gw.logJWKError(k.Logger(), url, err)
k.Logger().WithError(err).Info("Failed to decode JWKs body with factory client. Trying x5c PEM fallback.")

Check warning on line 273 in gateway/mw_jwt.go

View check run for this annotation

probelabs / Visor: architecture

architecture Issue

The error object `err` is logged twice in this block. The new helper function `logJWKError` is called on line 272, which logs the error with its details at the ERROR level. Immediately after, on line 273, `k.Logger().WithError(err).Info(...)` is called, which logs the same error object again with an informational message. This creates redundant log entries and is inconsistent with a similar change on line 281 where `WithError(err)` was correctly removed from the informational log.
Raw output
Remove `WithError(err)` from the `.Info()` log call to avoid duplication, as the error has already been logged by `logJWKError`. This will make the logging pattern consistent throughout the function.
}
}

// Fallback to original method if factory fails or JWK fetch fails
if clientErr != nil || err != nil {
if jwkSet, err = GetJWK(url, k.Gw.GetConfig().JWTSSLInsecureSkipVerify); err != nil {
k.Logger().WithError(err).Info("Failed to decode JWKs body. Trying x5c PEM fallback.")
k.Gw.logJWKError(k.Logger(), url, err)
k.Logger().Info("Failed to decode JWKs body. Trying x5c PEM fallback.")

key, legacyError := k.legacyGetSecretFromURL(url, kid, keyType)
if legacyError == nil {
Expand Down Expand Up @@ -339,8 +341,9 @@
}

// If not, return the actual value
decodedCert, err := base64.StdEncoding.DecodeString(config.JWTSource)

Check failure on line 344 in gateway/mw_jwt.go

View check run for this annotation

probelabs / Visor: security

security Issue

The `config.JWTSource` field, which can contain a base64-encoded secret for JWT signing (e.g., for HMAC algorithms), is logged in cleartext if base64 decoding fails. This can expose sensitive credentials in the logs.
Raw output
Avoid logging the raw `JWTSource`. Instead, log a generic error message or a sanitized version of the source (e.g., its type or length).

```go
k.Logger().WithError(err).Error("JWKS source decode failed: input is not a valid base64 string")
```
if err != nil {
k.Logger().WithError(err).Errorf("JWKS source decode failed: %s is not a base64 string", config.JWTSource)
return nil, err
}

Expand Down Expand Up @@ -452,7 +455,7 @@
}

if err != nil {
k.Logger().WithError(err).Infof("Failed to fetch or decode JWKs from %s", jwk.URL)
k.Gw.logJWKError(k.Logger(), jwk.URL, err)
fallbackJWKURIs = append(fallbackJWKURIs, jwk)
continue
}
Expand Down
Loading
Loading