feat: HTTP middleware #214

gkats · 2024-02-05T15:03:45Z

HTTP middleware for enhancing http.Handler with Clerk authentication. The http package provides two middleware:

WithHeaderAuthentication checks the Authorization header for a valid JWT and sets the active session claims in the request context.
RequireHeaderAuthentication responds with 403 Forbidden and halts the handler execution chain if it's unable to detect valid session claims.

Example usage:

ctx := context.Background()
rootHandler := func(w http.ResponseWriter, r *http.Request) {
  claims, _ := clerk.SessionClaimsFromContext(r.Context())
  w.Write([]byte(fmt.Sprintf(`{"user_id":"%s"}`, claims.UserID)))
}
// Adds active session claims to the request context
http.HandleFunc("/", clerkhttp.WithHeaderAuthentication(ctx)(rootHandler)

Please note that cookie based authentication is not supported. The SDK v1 middleware supported cookie authentication as well.

Added a new jwt package which implements the core function to validate a JWT. The function name is Verify and is used by the middleware to validate the Bearer JWT. The middleware above uses the Verify() method internally.

Added a new jwk package which provides operations for the JSON Web Keys API. We need to be able to fetch the instance JWK in order to validate and parse the Bearer token.

Added a dependency to go-jose/v3 package for performing operations on JWTs.

georgepsarakis

👏 Just a few questions, overall looks fine to me.

georgepsarakis · 2024-02-05T15:43:42Z

jwk.go

+
+	rsaPublicKey, err := x509.ParsePKIXPublicKey(block.Bytes)
+	if err != nil {
+		return nil, fmt.Errorf("failed to parse public key: %v", err)


🔧 I think %v will print the error struct in Go format?

Suggested change

return nil, fmt.Errorf("failed to parse public key: %v", err)

return nil, fmt.Errorf("failed to parse public key: %s", err.Error())

👍 You are right, copy-pasted it from v1. Changed it to wrap the error with %w instead.

georgepsarakis · 2024-02-05T15:45:49Z

jwk.go

+	Use       string `json:"use"`
+}
+
+func (k *JSONWebKey) UnmarshalJSON(data []byte) error {


❓ There's probably good reason, I just don't see it yet, would you mind explaining quickly why the custom deserializer is required?

I wanted to avoid using a go-jose type and instead make sure we use only our own package's (clerk) types everywhere.

There's no reason to expose types that are meant for internal use anywhere. We unfortunately do this in v1 of our SDK.

The go-jose library is able to decode JSON web keys and we can leverage that to get a response from our Backend API GET /v1/jwks endpoint and translate it to our own clerk.JSONWebKeySet and effectively clerk.JSONWebKey.

Unfortunately, it looks like extracting the key ID and key from a JWK response is not that straightforward. Depending on the algorithm that's used the JWK response might have different fields set, each with different meaning.

The go-jose library handles unmarshaling nicely, but its raw unmarshaled type is unexported.

So, what I thought we could do is use the jose.JSONWebKey type to unmarshal the raw JSON and then copy over the resulting fields to our type.

I initially tried type aliasing but it didn't work as expected.

Hope the above makes the decision clearer. Let me know if I should offer more details! Perhaps I can add a comment explaining what's going on here?

I wanted to avoid using a go-jose type and instead make sure we use only our own package's (clerk) types everywhere. There's no reason to expose types that are meant for internal use anywhere. We unfortunately do this in v1 of our SDK.

Very good call. This provides us with flexibility to change the underlying library (go-jose) without it being a breaking change.

I see, yes good call indeed. Now that the field is unexported the purpose is also much clearer!

georgepsarakis · 2024-02-05T15:46:44Z

jwk/jwk.go

@@ -0,0 +1,25 @@
+// Package jwk provides the JSON Web Tokens API.
+package jwk


❓ Would jwks be a more easily recognizable name? I'd assume there's more internal/external familiarity but not sure.

Indeed. I've opted for a singular term for all packages so far and thought I'd continue the same pattern. Happy to change it though if you think it's a problem.

Since "jwks" is not a plural term but an acronym, I think the singular/plural rationale (which I'm all for!) does not apply here, right? If so, I tend to agree with George's suggestion.

Yes, initially I thought I'd treat this as a "JSON Web Key" API (hence jwk).

I guess everybody else expects the "JSON Web Key Set" API, so I've changed it.

georgepsarakis · 2024-02-06T09:17:45Z

http/middleware.go

+// the authorization JWT will be verified.
+func JSONWebKey(key string) AuthorizationOption {
+	return func(params *AuthorizationParams) error {
+		// From the Clerk docs: "Note that the JWT Verification key is not in


🔧 Might be worth adding a unit test here, even for illustrating the use a bit better.

My intention was to add an integration test for the options, but setting the tests up took more time than expected. I'm planning to get back to this once I find some time.

georgepsarakis · 2024-02-06T09:20:06Z

http/middleware_test.go

+	"github.com/stretchr/testify/require"
+)
+
+func TestWithHeaderAuthentication_InvalidAuthorization(t *testing.T) {


❓ Don't you need a successful test case as well?

Yes, setting up tests for token verification is cumbersome, but I'll get back to this. I'll open a ticket to track this.

georgepsarakis · 2024-02-06T09:22:02Z

jwt.go

+
+type key string
+
+const activeSessionClaims = key("activeSessionClaims")


💡 We can prefix/namespace the context key with clerk so the scope is even clearer if someone inspects its contents.

Great idea! Updated.

jwt.go

jwt/jwt.go

kostaspt · 2024-02-07T09:31:15Z

http/middleware_test.go

+	"github.com/stretchr/testify/require"
+)
+
+func TestWithHeaderAuthentication_InvalidAuthorization(t *testing.T) {


🙃 Looks like there's a mix-up between Authentication and Authorization, even in the PR description. But Authorization seems to be the right term for this case.

Suggested change

func TestWithHeaderAuthentication_InvalidAuthorization(t *testing.T) {

func TestWithHeaderAuthorization_InvalidAuthorization(t *testing.T) {

Yes, the problem is that the header is called Authorization but it's often used for authentication. I'll try to reconcile.

agis

Amazing work! Really like the new direction where this is headed 💯 👏🏽

agis · 2024-02-07T10:45:08Z

http/middleware.go

+	jwt "github.com/clerk/clerk-sdk-go/v2/jwt"
+)
+
+// RequireAuthorization attempts to get Clerk authorization claims


🔧 s/RequireAuthorization/RequireHeaderAuthorization

Also, how about the following?

RequireHeaderAuthorization will respond with HTTP 403 if the Authorization header does not contain a valid session token.

👍 Nice, updated!

agis · 2024-02-07T10:48:12Z

http/middleware.go

+func RequireHeaderAuthorization(ctx context.Context, opts ...AuthorizationOption) func(http.Handler) http.Handler {
+	return func(next http.Handler) http.Handler {
+		return WithHeaderAuthorization(ctx, opts...)(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			claims, ok := clerk.SessionClaimsFromContext(r.Context())


❓ Conceptually speaking, what's the difference between r.Context() and ctx here?

Actually, why does WithHeaderAuthorization accepts a context in the first place? Shouldn't it work off of the incoming request's context?

Good question, I had the same doubts.

I left the ctx context for possible future usage.

Although it's not actually needed, if we don't add it now and we need it in a future version, we'll break the API.

Do you think it's better to remove it?

I'm not sure I can think of a good usage pattern here too, since the middleware setup doesn't happen within an operation context. Perhaps we should remove it?

Removed the context from the arguments.

http/middleware.go

agis · 2024-02-07T10:56:40Z

http/middleware.go

+			}
+
+			// Token was verified. Add the session claims to the request context
+			sessionCtx := clerk.ContextWithSessionClaims(r.Context(), claims)


🙃 Perhaps newCtx is a more accurate name? Perhaps not introducing a variable at all would be OK here as well.

Updated to newCtx.

agis · 2024-02-07T11:03:43Z

http/middleware.go

+}
+
+// JSONWebKey allows to provide a custom JSON Web Key based on which
+// the authorization JWT will be verified.


🔧 I believe it's worth noting here the runtime implications this option has. For example, a few questions raised when I read this:

if I set this, does it mean my SDK will never query the FAPI/BAPI JWKs endpoints?
a. if so, what happens if the keys are rolled and hence the JWKs need to be refreshed?
b. if not, how often the JWKs values are queried via FAPI/BAPI?

Enhanced the comment with more info.

jwt/jwt.go

agis · 2024-02-07T11:25:21Z

jwt/jwt.go

+
+// Decode decodes a JWT without verifying it. This is a quick way
+// to validate the format of a JSON web token, but the resulting Claims
+// should be considered untrusted.


🔧 It's worth putting this under a WARNING: The token is not validated, therefore the returned Claims should NOT be trusted.

agis · 2024-02-07T11:27:33Z

jwk.go

+	Use       string `json:"use"`
+}
+
+func (k *JSONWebKey) UnmarshalJSON(data []byte) error {


I wanted to avoid using a go-jose type and instead make sure we use only our own package's (clerk) types everywhere. There's no reason to expose types that are meant for internal use anywhere. We unfortunately do this in v1 of our SDK.

Very good call. This provides us with flexibility to change the underlying library (go-jose) without it being a breaking change.

agis · 2024-02-07T11:28:03Z

jwk.go

+
+type JSONWebKey struct {
+	APIResource
+	jose.JSONWebKey


🔧 Since we don't want to expose jose, shouldn't we avoid embedding this and instead wrap it with our own implementation?

Well, yes! Great catch, I missed this. Used an unexported field instead.

agis · 2024-02-07T11:33:47Z

jwk/jwk.go

@@ -0,0 +1,25 @@
+// Package jwk provides the JSON Web Tokens API.


🔧 How about something like:

Package jwks provides access to the JWKs endpoint.

See https://clerk.com/docs/reference/backend-api/tag/JWKS

👍 Updated.

georgepsarakis · 2024-02-07T16:23:00Z

http/middleware.go

+func RequireHeaderAuthorization(ctx context.Context, opts ...AuthorizationOption) func(http.Handler) http.Handler {
+	return func(next http.Handler) http.Handler {
+		return WithHeaderAuthorization(ctx, opts...)(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			claims, ok := clerk.SessionClaimsFromContext(r.Context())


I'm not sure I can think of a good usage pattern here too, since the middleware setup doesn't happen within an operation context. Perhaps we should remove it?

georgepsarakis · 2024-02-07T16:23:49Z

http/middleware.go

+			}
+
+			token := strings.TrimPrefix(authorization, "Bearer ")
+			_, err := jwt.Decode(ctx, &jwt.DecodeParams{Token: token})


🔧 I think you need the Request context here:

Suggested change

_, err := jwt.Decode(ctx, &jwt.DecodeParams{Token: token})

_, err := jwt.Decode(r.Context(), &jwt.DecodeParams{Token: token})

georgepsarakis · 2024-02-07T16:27:22Z

http/middleware.go

+
+// CustomClaims allows to pass a type (e.g. struct), which will be populated with the token claims based on json tags.
+// You must pass a pointer for this option to work.
+func CustomClaims(claims any) AuthorizationOption {


ℹ️ I think you're already aware but just adding a reference for #202 before we solidify the option design!

georgepsarakis · 2024-02-07T16:31:14Z

http/middleware.go

+				}
+			}
+			params.Token = token
+			claims, err := jwt.Verify(ctx, &params.VerifyParams)


🔧 Suggestion for a later PR to perhaps consider allowing client injection dependency here for testing.

georgepsarakis · 2024-02-07T16:32:13Z

jwk.go

+	Use       string `json:"use"`
+}
+
+func (k *JSONWebKey) UnmarshalJSON(data []byte) error {


I see, yes good call indeed. Now that the field is unexported the purpose is also much clearer!

georgepsarakis · 2024-02-07T16:33:53Z

jwt.go

+
+// ContextWithSessionClaims returns a new context which includes the
+// active session claims.
+func ContextWithSessionClaims(ctx context.Context, value any) context.Context {


🔧 Shouldn't the value type here be *SessionClaims 🤔 ?

🤔 I'm not sure about that. TBD after we settle on custom type for the claims.

georgepsarakis · 2024-02-07T16:34:39Z

jwt/jwt.go

+	// JWK is a custom JSON Web Key that can be provided to skip
+	// fetching one.
+	JWK          *clerk.JSONWebKey
+	CustomClaims any


Oh just saw your comment here, please ignore my comment above referencing the same issue 😬

georgepsarakis · 2024-02-07T16:37:36Z

jwt/jwt.go

+		return nil, err
+	}
+
+	err = claims.Claims.ValidateWithLeeway(jwt.Expected{Time: time.Now()}, params.Leeway)


🔧

Suggested change

err = claims.Claims.ValidateWithLeeway(jwt.Expected{Time: time.Now()}, params.Leeway)

err = claims.Claims.ValidateWithLeeway(jwt.Expected{Time: time.Now().UTC()}, params.Leeway)

HTTP middleware for enhancing http.Handler with Clerk authentication. The http package provides two middleware: - WithHeaderAuthentication checks the Authorization header for a valid JWT and sets the active session claims in the request context. - RequireHeaderAuthentication responds with 403 Forbidden and halts the handler execution chain if it's unable to detect valid session claims. Added a jwt package which implements the core function to validate a JWT. The function name is Verify and is used by the middleware to validate the Bearer JWT. Added a jwk package which provides operations for the JSON Web Keys API. We need to be able to fetch the instance JWK in order to validate and parse the Bearer token. Added a dependency to go-jose/v3 package for performing operations on JWTs.

agis

Approving for expediency, I don't see any blockers here since we're not releasing yet. Awesome work! 💯

agis · 2024-02-08T09:19:02Z

http/middleware.go

+// ProxyURL can be used to set the URL that proxies the Clerk Frontend
+// API. Useful for proxy based setups.
+// See https://clerk.com/docs/advanced-usage/using-proxies
+func ProxyURL(proxyURL string) AuthorizationOption {


💡 Shall we validate that proxyURL is indeed a valid URL?

agis · 2024-02-08T09:20:18Z

http/middleware.go

+// Satellite can be used to signify that the authorization happens
+// on a satellite domain.
+// See https://clerk.com/docs/advanced-usage/satellite-domains
+func Satellite(isSatellite bool) AuthorizationOption {


❓ Is there any reason for someone to call this as Satellite(false)? If not, then perhaps this should be simplified to Satellite() (and always set params.IsSatellite = true).

I guess there's not. I mainly copied whatever was there before. The only thing I can think of is the usage difference

opts := []Opt{} if isSatellite { opts = append(opts, Satellite()) } WithHeaderAuthorization(opts...) // vs WithHeaderAuthorization(Satellite(isSatellite))

I believe the above is a valid case for leaving the API as is.

agis · 2024-02-08T09:21:22Z

http/middleware.go

+		// From the Clerk docs: "Note that the JWT Verification key is not in
+		// PEM format, the header and footer are missing, in order to be shorter
+		// and single-line for easier setup."
+		if !strings.HasPrefix(key, "-----BEGIN") {


It seems to me that it would be more fool-proof than what v1 does, but then again I understand if you decide it's not even worth it, since it's an edge case. Up to you! :)

agis · 2024-02-08T09:25:27Z

jwt/jwt.go

+}
+
+type DecodeParams struct {
+	Token string


❓ What's the reason for wrapping Token in a struct? Is it for providing us a way to add more options without breaking compatibility (i.e. keeping the function signature intact)?

If so, I'm having a hard time imagining something that Decode would need to be extended with, that a functional option wouldn't be a fit for. In other words, perhaps it's perfectly fine to go with Decode(ctx, string) and in the future extend with functional options if needed? The reason I'm suggesting this is because the latter seems a simpler/easier to use API to me.

Yes, we're trying to use custom types for method arguments so that we can easily extend them in the future without introducing breaking changes in the API.

agis · 2024-02-08T09:29:02Z

jwt/jwt.go

+		return nil, fmt.Errorf("invalid issuer %s", iss)
+	}
+
+	if claims.AuthorizedParty != "" && len(params.authorizedParties) > 0 {


Interesting. That's maybe worthy of more investigation, but it's out of scope of this PR. I'll create an internal ticket to investigate further.

gkats requested a review from a team as a code owner February 5, 2024 15:03

gkats force-pushed the core-1510-middleware branch from 4f2ec26 to 0f8fb45 Compare February 5, 2024 15:07

georgepsarakis reviewed Feb 6, 2024

View reviewed changes

gkats force-pushed the core-1510-middleware branch from 0f8fb45 to e9f0b8c Compare February 6, 2024 13:31

kostaspt reviewed Feb 7, 2024

View reviewed changes

gkats force-pushed the core-1510-middleware branch from e9f0b8c to a1ac263 Compare February 7, 2024 10:24

agis reviewed Feb 7, 2024

View reviewed changes

gkats force-pushed the core-1510-middleware branch 3 times, most recently from 6b18595 to ca6040b Compare February 7, 2024 14:22

georgepsarakis approved these changes Feb 7, 2024

View reviewed changes

gkats force-pushed the core-1510-middleware branch from ca6040b to 4419472 Compare February 7, 2024 17:12

agis approved these changes Feb 8, 2024

View reviewed changes

gkats merged commit d7d5a50 into v2 Feb 8, 2024
4 checks passed

gkats deleted the core-1510-middleware branch February 8, 2024 09:48

NicolasLopes7 mentioned this pull request May 13, 2024

update v2 04 changelog #294

Merged

NicolasLopes7 mentioned this pull request Sep 5, 2024

nicolas/dash 342 dapi delete web3 wallet #321

Merged

	return nil, fmt.Errorf("failed to parse public key: %v", err)
	return nil, fmt.Errorf("failed to parse public key: %s", err.Error())

		@@ -0,0 +1,25 @@
		// Package jwk provides the JSON Web Tokens API.
		package jwk


		type key string

		const activeSessionClaims = key("activeSessionClaims")

	func TestWithHeaderAuthentication_InvalidAuthorization(t *testing.T) {
	func TestWithHeaderAuthorization_InvalidAuthorization(t *testing.T) {

	_, err := jwt.Decode(ctx, &jwt.DecodeParams{Token: token})
	_, err := jwt.Decode(r.Context(), &jwt.DecodeParams{Token: token})

	err = claims.Claims.ValidateWithLeeway(jwt.Expected{Time: time.Now()}, params.Leeway)
	err = claims.Claims.ValidateWithLeeway(jwt.Expected{Time: time.Now().UTC()}, params.Leeway)

feat: HTTP middleware #214

feat: HTTP middleware #214

Conversation

gkats commented Feb 5, 2024

georgepsarakis 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

gkats Feb 6, 2024 • 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

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

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

agis 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

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

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

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

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

agis 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkats Feb 6, 2024 •

edited

Loading