-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add auth token propagation for metrics reader #3341
Changes from 2 commits
a5133cc
fa128b3
d1c96ad
f0774b2
8e5f1b9
73db361
787b07b
9af8681
bac98cc
56ac31b
f5e3801
65208c2
696eb45
4d7e1c5
696cb5f
bf6a818
2742a11
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,18 +12,44 @@ | |
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
||
package app | ||
package bearertoken | ||
|
||
import ( | ||
"context" | ||
"net/http" | ||
"strings" | ||
|
||
"go.uber.org/zap" | ||
|
||
"github.com/jaegertracing/jaeger/storage/spanstore" | ||
) | ||
|
||
func bearerTokenPropagationHandler(logger *zap.Logger, h http.Handler) http.Handler { | ||
type contextKey string | ||
|
||
// Key is the string literal used internally in the implementation of this context. | ||
const Key = "bearer.token" | ||
const bearerToken = contextKey(Key) | ||
|
||
// StoragePropagationKey is a key for viper configuration to pass this option to storage plugins. | ||
const StoragePropagationKey = "storage.propagate.token" | ||
|
||
// ContextWithBearerToken set bearer token in context. | ||
func ContextWithBearerToken(ctx context.Context, token string) context.Context { | ||
if token == "" { | ||
return ctx | ||
} | ||
return context.WithValue(ctx, bearerToken, token) | ||
} | ||
|
||
// GetBearerToken from context, or empty string if there is no token. | ||
func GetBearerToken(ctx context.Context) (string, bool) { | ||
val, ok := ctx.Value(bearerToken).(string) | ||
return val, ok | ||
} | ||
|
||
// PropagationHandler returns a http.Handler containing the logic to extract | ||
// the Authorization token from the http.Request and inserts it into the http.Request | ||
// context for easier access to the request token via GetBearerToken for bearer token | ||
// propagation use cases. | ||
func PropagationHandler(logger *zap.Logger, h http.Handler) http.Handler { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Renamed I considered renaming |
||
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
ctx := r.Context() | ||
authHeaderValue := r.Header.Get("Authorization") | ||
|
@@ -40,15 +66,14 @@ func bearerTokenPropagationHandler(logger *zap.Logger, h http.Handler) http.Hand | |
token = headerValue[1] | ||
} | ||
} else if len(headerValue) == 1 { | ||
// Tread all value as a token | ||
// Treat the entire value as a token. | ||
token = authHeaderValue | ||
} else { | ||
logger.Warn("Invalid authorization header value, skipping token propagation") | ||
} | ||
h.ServeHTTP(w, r.WithContext(spanstore.ContextWithBearerToken(ctx, token))) | ||
h.ServeHTTP(w, r.WithContext(ContextWithBearerToken(ctx, token))) | ||
} else { | ||
h.ServeHTTP(w, r.WithContext(ctx)) | ||
} | ||
}) | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,41 +12,54 @@ | |
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
||
package app | ||
package bearertoken | ||
|
||
import ( | ||
"context" | ||
"net/http" | ||
"net/http/httptest" | ||
"sync" | ||
"testing" | ||
"time" | ||
|
||
"github.com/stretchr/testify/assert" | ||
"go.uber.org/zap" | ||
|
||
"github.com/jaegertracing/jaeger/storage/spanstore" | ||
) | ||
|
||
func Test_bearTokenPropagationHandler(t *testing.T) { | ||
func Test_GetBearerToken(t *testing.T) { | ||
token := "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJhZG1pbiIsIm5hbWUiOiJKb2huIERvZSIsImlhdCI" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Couldn't this string just be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in 8e5f1b9. |
||
ctx := context.Background() | ||
ctx = ContextWithBearerToken(ctx, token) | ||
contextToken, ok := GetBearerToken(ctx) | ||
assert.True(t, ok) | ||
assert.Equal(t, contextToken, token) | ||
} | ||
|
||
func Test_bearerTokenPropagationHandler(t *testing.T) { | ||
httpClient := &http.Client{ | ||
Timeout: 2 * time.Second, | ||
} | ||
|
||
logger := zap.NewNop() | ||
bearerToken := "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJhZG1pbiIsIm5hbWUiOiJKb2huIERvZSIsImlhdCI" | ||
|
||
validTokenHandler := func(stop *sync.WaitGroup) http.HandlerFunc { | ||
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
return func(w http.ResponseWriter, r *http.Request) { | ||
ctx := r.Context() | ||
token, ok := spanstore.GetBearerToken(ctx) | ||
token, ok := GetBearerToken(ctx) | ||
assert.Equal(t, token, bearerToken) | ||
assert.True(t, ok) | ||
stop.Done() | ||
}) | ||
} | ||
} | ||
|
||
emptyHandler := func(stop *sync.WaitGroup) http.HandlerFunc { | ||
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
return func(w http.ResponseWriter, r *http.Request) { | ||
ctx := r.Context() | ||
token, _ := spanstore.GetBearerToken(ctx) | ||
token, _ := GetBearerToken(ctx) | ||
assert.Empty(t, token, bearerToken) | ||
stop.Done() | ||
}) | ||
} | ||
} | ||
|
||
testCases := []struct { | ||
|
@@ -68,7 +81,7 @@ func Test_bearTokenPropagationHandler(t *testing.T) { | |
t.Run(testCase.name, func(t *testing.T) { | ||
stop := sync.WaitGroup{} | ||
stop.Add(1) | ||
r := bearerTokenPropagationHandler(logger, testCase.handler(&stop)) | ||
r := PropagationHandler(logger, testCase.handler(&stop)) | ||
server := httptest.NewServer(r) | ||
defer server.Close() | ||
req, err := http.NewRequest("GET", server.URL, nil) | ||
|
@@ -81,5 +94,4 @@ func Test_bearTokenPropagationHandler(t *testing.T) { | |
stop.Wait() | ||
}) | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,6 +31,7 @@ import ( | |
promapi "github.com/prometheus/client_golang/api/prometheus/v1" | ||
"go.uber.org/zap" | ||
|
||
"github.com/jaegertracing/jaeger/pkg/bearertoken" | ||
"github.com/jaegertracing/jaeger/pkg/prometheus/config" | ||
"github.com/jaegertracing/jaeger/plugin/metrics/prometheus/metricsstore/dbmodel" | ||
"github.com/jaegertracing/jaeger/proto-gen/api_v2/metrics" | ||
|
@@ -253,13 +254,30 @@ func getHTTPRoundTripper(c *config.Configuration, logger *zap.Logger) (rt http.R | |
|
||
// KeepAlive and TLSHandshake timeouts are kept to existing Prometheus client's | ||
// DefaultRoundTripper to simplify user configuration and may be made configurable when required. | ||
return &http.Transport{ | ||
httpTransport := &http.Transport{ | ||
Proxy: http.ProxyFromEnvironment, | ||
DialContext: (&net.Dialer{ | ||
Timeout: c.ConnectTimeout, | ||
KeepAlive: 30 * time.Second, | ||
}).DialContext, | ||
TLSHandshakeTimeout: 10 * time.Second, | ||
TLSClientConfig: ctlsConfig, | ||
} | ||
return &tokenAuthTransport{ | ||
wrapped: httpTransport, | ||
}, nil | ||
} | ||
|
||
// tokenAuthTransport wraps an instance of http.Transport for the purpose of | ||
// propagating an Authorization token from inbound to outbound HTTP requests. | ||
type tokenAuthTransport struct { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: could we take this opportunity and refactor/cleanup bearer token functionality into a standalone package? We have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in d1c96ad. |
||
wrapped *http.Transport | ||
} | ||
|
||
// RoundTrip implements the http.RoundTripper interface, injecting the outbound | ||
// Authorization header with the token provided in the inbound request. | ||
func (tr *tokenAuthTransport) RoundTrip(r *http.Request) (*http.Response, error) { | ||
headerToken, _ := bearertoken.GetBearerToken(r.Context()) | ||
r.Header.Set("Authorization", "Bearer "+headerToken) | ||
return tr.wrapped.RoundTrip(r) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,6 +31,7 @@ import ( | |
"github.com/stretchr/testify/require" | ||
"go.uber.org/zap" | ||
|
||
"github.com/jaegertracing/jaeger/pkg/bearertoken" | ||
"github.com/jaegertracing/jaeger/pkg/config/tlscfg" | ||
"github.com/jaegertracing/jaeger/pkg/prometheus/config" | ||
"github.com/jaegertracing/jaeger/proto-gen/api_v2/metrics" | ||
|
@@ -331,12 +332,33 @@ func TestGetRoundTripper(t *testing.T) { | |
}, | ||
}, logger) | ||
require.NoError(t, err) | ||
assert.IsType(t, &http.Transport{}, rt) | ||
assert.IsType(t, &tokenAuthTransport{}, rt) | ||
if tc.tlsEnabled { | ||
assert.NotNil(t, rt.(*http.Transport).TLSClientConfig) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moving the common transport into the Suggestions also welcome. |
||
assert.NotNil(t, rt.(*tokenAuthTransport).wrapped.TLSClientConfig) | ||
} else { | ||
assert.Nil(t, rt.(*http.Transport).TLSClientConfig) | ||
assert.Nil(t, rt.(*tokenAuthTransport).wrapped.TLSClientConfig) | ||
} | ||
|
||
server := httptest.NewServer( | ||
http.HandlerFunc( | ||
func(w http.ResponseWriter, r *http.Request) { | ||
assert.Equal(t, "Bearer foo", r.Header.Get("Authorization")) | ||
}, | ||
), | ||
) | ||
defer server.Close() | ||
|
||
req, err := http.NewRequestWithContext( | ||
bearertoken.ContextWithBearerToken(context.Background(), "foo"), | ||
http.MethodGet, | ||
server.URL, | ||
nil, | ||
) | ||
require.NoError(t, err) | ||
|
||
resp, err := rt.RoundTrip(req) | ||
require.NoError(t, err) | ||
assert.Equal(t, http.StatusOK, resp.StatusCode) | ||
}) | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would split this file:
context.go
http.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in f0774b2.