[auth] Decode token once #1266

MariemBaccari · 2025-09-09T13:10:46Z

This PR is a follow-up to #1263 and removes the duplicate decoding of the authentication token.
It decodes the token once in the authentication middleware and passes the result in the context.

Remove duplicate decoding

barroco · 2025-09-15T13:27:10Z

cmds/core-service/main.go

+		}
+
+		ctx = context.WithValue(r.Context(), auth.CtxAuthKey{}, auth.CtxAuthValue{
+			Claims: claims,


Instead of using the context to pass the subject to the logger, we may want to zap it in the logger directly like the httpmiddleware is doing it. Could you please evaluate this approach and propose a new version ?

In 9cbfe67 I added the logger as an argument to the auth middleware to write the values there and pass the new (enriched) logger to the httpMiddleware. What do you think ?

barroco · 2025-10-13T12:24:39Z

@mickmis can you please have a second look since it is security related. Thank you.

mickmis

I see this PR intends to optimize the runtime of the DSS in avoiding decoding the token twice by passing it through the context.
The main challenge here is dealing with the different approaches of how the logging and the authorization are performed. The logging is made through an http middleware, while the authorization is by invoking the authorizer close to the endpoint handler.

This PR deals with that by moving part of the authorization in a middleware. This is fine as long as:

the split makes sense, here on one side we validate the token validity w.r.t. the instance (signature, audience) and on the other side w.r.t. the request (scopes)
those related logics are not scattered away in the codebase

As such I suggest here to:

move authMiddleware to being a method of Authorizer in auth package and rename it to reflect better its function, e.g. TokenMiddleware:

func (a *Authorizer) TokenMiddleware(handler http.Handler) http.Handler

// in main.go:
handler = authorizer.TokenMiddleware(handler)

clarify that Authorize authorizes the request knowing that the token was already validated before by the middleware
unexpose unecessary exposed functions and structs (this is BTW a hint towards related logic being scattered) such as ExtractClaims or Claims or AcceptedAudiences

mickmis · 2025-10-13T15:36:05Z

pkg/auth/auth.go

+type CtxAuthKey struct{}
+type CtxAuthValue struct {


nit: you are in package auth, so you may drop the Auth part from the variable names.

mickmis · 2025-10-14T13:40:45Z

cmds/core-service/main.go


+		ctx = context.WithValue(ctx, logging.CtxAuthKey{}, logging.CtxAuthValue{
+			Subject: claims.Subject,
+			ErrMsg:  errMsg,


Why is propagating the error necessary? Remove if not.
Then the subject may be propagated with a single string

Also, nit, but for context key you may do:

type CtxKey string ctx = context.WithValue(ctx, CtxKey("sub"), parsedSub)

Propagating the error allows us to log it when the sub can't be found:

When provided with a valid token, the log outputs the following:
2025-09-03T06:21:28.402Z info logging/http.go:95 GET / HTTP/1.1 {"address": ":8082", "req_dump": "", "req_sub": "uss1", "resp_dump": "404 page not found\n", "req_headers": {"Accept":["application/json"],"Authorization":["Bearer ...

When the token decoding fails, the log outputs the following:
info logging/http.go:95 GET / HTTP/1.1 {"address": ":8082", "req_dump": "", "resp_sub_err": "Access token validation failed: token is expired by 29h16m20s", "resp_dump": "404 page not found\n", "req_headers": {"Accept":["application/json"],"Authorization":["Bearer

Is this aligned with the expected behavior ? If not, I can remove it.

MariemBaccari · 2025-10-21T16:14:00Z

@mickmis Since we moved the decoding into auth, I ended up adding a decodedClaims field to the Authorizer instead of passing the token in the context as it seemed cleaner. What do you think ?

Remove duplicate decoding

…dup-token

mickmis

Some comments, most importantly the one about decodedClaims. But we are getting there :)

mickmis · 2025-11-19T09:06:46Z

pkg/auth/auth.go

+
 	acceptedAudiences map[string]bool
+
+	decodedClaims *middlewareResult


Not OK to pass this through the Authorizer struct, we will have race conditions: decodedClaims is scoped to the request, while the authorizer is scoped to the runtime. Pass this through the context instead, like for the sub. The context is made for stuff scoped to the request.

mickmis · 2025-11-19T09:15:09Z

pkg/logging/http.go

-type CtxAuthError struct{}
-type CtxAuthSubject struct{}
+type CtxKey string
+type CtxAuthValue struct {


nit: rather than a custom struct, I'd suggest to flatten this and just use two different keys with a string value type CtxKey("sub") and CtxKey("sub_err_msg").

mickmis · 2025-11-19T09:22:24Z

pkg/logging/http.go

-		if !ok {
-			authErrorMsg := r.Context().Value(CtxAuthError{}).(string)
-			logger = logger.With(zap.String("resp_sub_err", authErrorMsg))
+		authResults := r.Context().Value(CtxKey("sub")).(CtxAuthValue)


This removes the type assertion (with the value, ok := var.(Type) pattern), meaning that a wrong type will cause this line to panic (which may also happen when there is no value at all).
This is acceptable when it is obvious that a wrong type is a programming error. Which in this current version of the PR is the case (sub should always be added by the token middleware), however if you do implement my previous nit about flattening the CtxAuthValue, this might not be the case anymore (where you would have either sub or sub_err_msg).

mickmis · 2025-11-19T09:22:50Z

pkg/auth/auth_test.go

+				claims: claims,
+				err:    err,
+			}
+			res := a.Authorize(nil, test.req.WithContext(context.Background()), []api.AuthorizationOption{})


nit: use t.Context() instead

mickmis · 2025-11-19T09:25:41Z

pkg/auth/auth.go

-	keyClaims, err := a.ExtractClaims(r)
-	if err != nil {
-		return api.AuthorizationResult{Error: stacktrace.PropagateWithCode(err, dsserr.Unauthenticated, "Failed to extract claims from access token")}
+	if a.decodedClaims == nil {


This should come from the context, and not form the Authorizer, c.f. other comment.

mickmis · 2025-11-19T09:32:58Z

pkg/auth/auth.go

+		return api.AuthorizationResult{Error: stacktrace.NewErrorWithCode(dsserr.Unauthenticated, "Access token not found")}
 	}

-	if !a.acceptedAudiences[keyClaims.Audience] {


Actually, why not keeping the audience check here if we have the claims?

mickmis · 2025-11-19T09:33:42Z

pkg/auth/auth.go

 	return keyClaims, nil
 }

-func HasScope(scopes []string, requiredScope api.RequiredScope) bool {


Any reason why this function has moved? This makes the diff harder to read.

MariemBaccari and others added 6 commits September 9, 2025 15:00

decode only once in middleware

7fd1d7e

fix formatting error

03a06f3

[logging] Add subject field to log output (interuss#1263)

b20dc4c

Remove duplicate decoding

remove merge messages

fc27a16

fix unit test

625f684

edit error message

1ef90ba

MariemBaccari marked this pull request as ready for review September 9, 2025 14:18

barroco requested changes Sep 15, 2025

View reviewed changes

MariemBaccari requested a review from barroco September 17, 2025 08:41

MariemBaccari marked this pull request as draft September 18, 2025 07:20

Revert changes and improve middleware declarations

5860a2e

MariemBaccari force-pushed the 1051-dup-token branch from 9cbfe6f to 5860a2e Compare September 18, 2025 09:22

MariemBaccari marked this pull request as ready for review September 18, 2025 09:37

mickmis reviewed Oct 14, 2025

View reviewed changes

MariemBaccari added 2 commits October 21, 2025 18:05

address pr comments

30dd60f

fix context key nit

06b3ae2

MariemBaccari force-pushed the 1051-dup-token branch from b630258 to 06b3ae2 Compare October 21, 2025 16:15

MariemBaccari and others added 11 commits October 21, 2025 18:17

Merge branch 'master' into 1051-dup-token

86bb4ca

add missing context

363c8da

clarify middleware doc

9fd7ae0

decode only once in middleware

36df8df

fix formatting error

929fbe6

[logging] Add subject field to log output (interuss#1263)

0621a00

Remove duplicate decoding

remove merge messages

7316590

fix unit test

1a74ef7

edit error message

1325293

Revert changes and improve middleware declarations

c11145a

address pr comments

d78dc68

MariemBaccari added 5 commits October 21, 2025 18:30

fix context key nit

33cca92

add missing context

09f7926

clarify middleware doc

26eae64

Merge branch '1051-dup-token' of github.com:Orbitalize/dss into 1051-…

ac80360

…dup-token

fix auth_test.go

6e443ca

MariemBaccari requested a review from mickmis October 21, 2025 16:41

mickmis requested changes Nov 19, 2025

View reviewed changes


		acceptedAudiences map[string]bool

		decodedClaims *middlewareResult

[auth] Decode token once #1266

Are you sure you want to change the base?

[auth] Decode token once #1266

Uh oh!

Conversation

MariemBaccari commented Sep 9, 2025

Uh oh!

barroco Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MariemBaccari Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

barroco commented Oct 13, 2025

Uh oh!

mickmis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MariemBaccari Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MariemBaccari commented Oct 21, 2025

Uh oh!

mickmis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

barroco Sep 15, 2025 •

edited

Loading

MariemBaccari Sep 17, 2025 •

edited

Loading

MariemBaccari Oct 21, 2025 •

edited

Loading