Passthrough OAuth bearer token supplied to Query service through to ES storage #1599

rubenvp8510 · 2019-06-12T20:23:52Z

Which problem is this PR solving?

To provide fine grained visibility of tracing information stored in Elasticsearch, protected using tools like SearchGuard, it would be useful if the OAuth bearer token for the authenticated user could be passed through the Query service to the ES storage plugin and then propagated to the ES cluster.
To enable users to disable the propagation, this PR includes the possibility of enable/disable this behavior using a flag.

Short description of the changes

I injected the token in the context, and used it on ES plugin, also included a flag, but not sure if the flag should be at the query level, I put it the query level because this behavior only makes sense there. (IMHO). Also I did some changes to disable health check on plugin because the token is provided on each request, so I cannot check health of ES cluster when query service starts.

yurishkuro · 2019-06-12T20:46:48Z

plugin/storage/es/spanstore/service_operation.go

@@ -38,21 +38,18 @@ const (

 // ServiceOperationStorage stores service to operation pairs.
 type ServiceOperationStorage struct {
-	ctx          context.Context


yurishkuro · 2019-06-12T20:48:07Z

cmd/query/app/server.go

+func BearTokenPropagationHandler(logger *zap.Logger, h http.Handler) http.Handler {
+	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		ctx := r.Context()
+		authHeaderValue := r.Header.Get("Authorization")


What is the format of Authorization header?

According to https://tools.ietf.org/html/rfc6750#section-2.1
It should be in the form : Authorization: "Bearer <token>"

yurishkuro · 2019-06-12T20:51:26Z

cmd/query/app/server.go

+		if authHeaderValue != "" {
+			bearerToken := strings.Split(authHeaderValue, " ")
+			if len(bearerToken) == 2 {
+				ctx = context.WithValue(ctx, "Storage-Token", bearerToken)


The recommended way of storing values in Context is by using private keys:

type storageContextKeyType string var storageContextKey = storageContextKeyType("Storage-Token")

Thanks for the recomendation, I'll change it

yurishkuro · 2019-06-12T20:53:12Z

cmd/query/app/server.go

+	if queryOpts.BearerTokenPropagation {
+		compressHandler = handlers.CompressHandler(BearTokenPropagationHandler(logger, r))
+	} else {
+		compressHandler = handlers.CompressHandler(r)


avoid duplicating wrappers. There's no business meaning of calling CompressHandler twice in different context.

I suggest var h http.Hander = r and then keep overwriting h with every new decorator (including in L90)

yurishkuro · 2019-06-12T20:55:17Z

cmd/query/app/server.go

 	return &http.Server{
 		Handler: recoveryHandler(compressHandler),
 	}
 }

+func BearTokenPropagationHandler(logger *zap.Logger, h http.Handler) http.Handler {


doesn't need to be public

yurishkuro · 2019-06-12T20:56:25Z

pkg/es/config/config.go

 			}
 		} else {
 			httpClient.Transport = httpTransport
 			options = append(options, elastic.SetBasicAuth(c.Username, c.Password))
 		}
+
+		if c.TokenFromContext {
+			httpClient.Transport = &tokenAuthTransport{


this will overwrite L281

yurishkuro · 2019-06-12T20:57:54Z

pkg/es/config/config.go

+		token := r.Context().Value("Storage-Token")
+		if token != nil && token != "" {
+			r.Header.Set("Authorization", "Bearer "+token.(string))
+			return tr.wrapped.RoundTrip(r)


repeats L356-357, please DRY

yurishkuro · 2019-06-12T20:58:24Z

plugin/storage/es/spanstore/reader.go

@@ -231,7 +231,7 @@ func (s *SpanReader) GetServices(ctx context.Context) ([]string, error) {
 	defer span.Finish()
 	currentTime := time.Now()
 	jaegerIndices := s.timeRangeIndices(s.serviceIndexPrefix, currentTime.Add(-s.maxSpanAge), currentTime)
-	return s.serviceOperationStorage.getServices(jaegerIndices)
+	return s.serviceOperationStorage.getServices(jaegerIndices, ctx)


ctx should be in the first position

codecov · 2019-06-19T17:21:22Z

Codecov Report

Merging #1599 into master will increase coverage by <.01%.
The diff coverage is 95%.

@@            Coverage Diff             @@
##           master    #1599      +/-   ##
==========================================
+ Coverage   98.72%   98.72%   +<.01%     
==========================================
  Files         191      193       +2     
  Lines        9182     9211      +29     
==========================================
+ Hits         9065     9094      +29     
+ Misses         91       90       -1     
- Partials       26       27       +1

Impacted Files	Coverage Δ
cmd/query/app/flags.go	`100% <100%> (ø)`	⬆️
cmd/query/app/token_propagation_handler.go	`100% <100%> (ø)`
cmd/query/app/server.go	`90.76% <100%> (+0.44%)`	⬆️
plugin/storage/es/options.go	`100% <100%> (ø)`	⬆️
plugin/storage/es/spanstore/reader.go	`100% <100%> (ø)`	⬆️
plugin/storage/es/spanstore/writer.go	`97.59% <100%> (ø)`	⬆️
plugin/storage/es/spanstore/service_operation.go	`100% <100%> (ø)`	⬆️
storage/spanstore/token_propagation.go	`71.42% <71.42%> (ø)`
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b52726...53aafca. Read the comment docs.

jpkrohling

Looks very good! I have only a couple of comments, especially about sanitizing the bearer value and about making sure that only the appropriate authorization types are propagated down.

cmd/query/app/token_propagation_handler.go

cmd/query/app/token_propagation_hander_test.go

jpkrohling · 2019-06-24T09:14:17Z

cmd/query/app/token_propagation_handler.go

+			headerValue := strings.Split(authHeaderValue, " ")
+			token := ""
+			if len(headerValue) == 2 {
+				token = headerValue[1]


The way it is, you are probably propagating non-bearer authorization as well, such as Basic. If this is what you want, you should rename the functions from bearerTokenPropagation to authorizationHeaderPropagation.

If you didn't intend to propagate the basic auth as well, check explicitly for the authorization type ("Bearer" == headerValue[0]).

pkg/es/config/config.go

objectiser · 2019-06-26T09:09:33Z

cmd/query/app/token_propagation_handler.go

+func bearTokenPropagationHandler(logger *zap.Logger, h http.Handler) http.Handler {
+	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		ctx := r.Context()
+		authHeaderValue := r.Header.Get("Authorization")


I think we also need to be checking for X-Forwarded-Access-Token - as (for example) if using the OpenShift OAuthProxy - it will receive the bearer token from the UI, and if configured to do so, will pass it in the upstream request in the X-Forwarded-Access-Token header instead of Authorization.

@jpkrohling does this sound reasonable to you?

Yes, it does. That's a common header set by reverse proxies.

I put support for X-Forwarded-Access-Token, I tried to get token from there if Authorization header is not defined.

objectiser · 2019-06-26T13:02:00Z

storage/spanstore/token_propagation.go

+
+// ContextWithBearerToken set bearer token in context
+func ContextWithBearerToken(ctx context.Context, token string) context.Context {
+	return context.WithValue(ctx, bearerToken, token)


Should this check whether the token has a value? Currently, if Authorization is not Bearer the supplied string will be empty? If empty token, then could just return the ctx.

objectiser · 2019-06-26T13:11:53Z

pkg/es/config/config.go

-	r.Header.Set("Authorization", "Bearer "+tr.token)
+	token := tr.token
+	if tr.allowOverrideFromCtx {
+		token, _ = spanstore.GetBearerToken(r.Context())


Would be better to return the token into a local var, check a value has been found, before overwriting the token. That way - if a request is sent in without a bearer token, it will still revert to the supplied tr.token.

well, I wasn't sure if we wanted to do that, but ok I'll do the change

objectiser · 2019-06-26T13:38:25Z

@rubenvp8510 Once the changes have been applied, could you do a quick test using the operator with Oauth proxy cli parameter --pass-user-bearer-token and check that a token is passed to ES?

rubenvp8510 · 2019-06-26T13:40:39Z

@objectiser sure!

…S storage Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

jpkrohling · 2019-06-27T07:44:28Z

cmd/query/app/token_propagation_hander_test.go

+)
+
+
+func Test_bearTokenPropagationHandler(t *testing.T)  {


Look, I found another bear!

rubenvp8510 requested review from black-adder, jpkrohling, objectiser, pavolloffay, tiffon, vprithvi and yurishkuro as code owners June 12, 2019 20:23

yurishkuro reviewed Jun 12, 2019

View reviewed changes

rubenvp8510 force-pushed the es-bearer-token branch 9 times, most recently from e48b633 to 94aa253 Compare June 19, 2019 17:04

rubenvp8510 force-pushed the es-bearer-token branch 4 times, most recently from 22e1a92 to e04a824 Compare June 19, 2019 23:54

rubenvp8510 changed the title ~~WIP: Passthrough OAuth bearer token supplied to Query service through to ES storage~~ Passthrough OAuth bearer token supplied to Query service through to ES storage Jun 20, 2019

pavolloffay added the storage/elasticsearch label Jun 21, 2019

jpkrohling reviewed Jun 24, 2019

View reviewed changes

objectiser reviewed Jun 26, 2019

View reviewed changes

rubenvp8510 force-pushed the es-bearer-token branch 3 times, most recently from fbc712d to ecf5ed6 Compare June 26, 2019 12:06

objectiser reviewed Jun 26, 2019

View reviewed changes

rubenvp8510 force-pushed the es-bearer-token branch from ecf5ed6 to 1649c83 Compare June 26, 2019 22:34

Passthrough OAuth bearer token supplied to Query service through to E…

53aafca

…S storage Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

rubenvp8510 force-pushed the es-bearer-token branch from 1649c83 to 53aafca Compare June 26, 2019 22:35

objectiser approved these changes Jun 27, 2019

View reviewed changes

jpkrohling reviewed Jun 27, 2019

View reviewed changes

pavolloffay merged commit cf03be7 into jaegertracing:master Jun 27, 2019

albertteoh mentioned this pull request Feb 26, 2023

Allow SPM to pass bearer token to Prometheus #4247

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Passthrough OAuth bearer token supplied to Query service through to ES storage #1599

Passthrough OAuth bearer token supplied to Query service through to ES storage #1599

rubenvp8510 commented Jun 12, 2019

yurishkuro Jun 12, 2019

yurishkuro Jun 12, 2019

rubenvp8510 Jun 12, 2019 •

edited

Loading

yurishkuro Jun 12, 2019

rubenvp8510 Jun 12, 2019 •

edited

Loading

yurishkuro Jun 12, 2019

yurishkuro Jun 12, 2019

yurishkuro Jun 12, 2019

yurishkuro Jun 12, 2019

yurishkuro Jun 12, 2019

codecov bot commented Jun 19, 2019 •

edited

Loading

jpkrohling left a comment

jpkrohling Jun 24, 2019

objectiser Jun 26, 2019

jpkrohling Jun 26, 2019

rubenvp8510 Jun 26, 2019

objectiser Jun 26, 2019 •

edited

Loading

objectiser Jun 26, 2019

rubenvp8510 Jun 26, 2019

objectiser commented Jun 26, 2019

rubenvp8510 commented Jun 26, 2019

jpkrohling Jun 27, 2019

Passthrough OAuth bearer token supplied to Query service through to ES storage #1599

Passthrough OAuth bearer token supplied to Query service through to ES storage #1599

Conversation

rubenvp8510 commented Jun 12, 2019

Which problem is this PR solving?

Short description of the changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubenvp8510 Jun 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubenvp8510 Jun 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 19, 2019 • edited Loading

Codecov Report

jpkrohling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

objectiser Jun 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

objectiser commented Jun 26, 2019

rubenvp8510 commented Jun 26, 2019

Choose a reason for hiding this comment

rubenvp8510 Jun 12, 2019 •

edited

Loading

rubenvp8510 Jun 12, 2019 •

edited

Loading

codecov bot commented Jun 19, 2019 •

edited

Loading

objectiser Jun 26, 2019 •

edited

Loading