Skip to content
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

jwt isValidIssuer check not strict enough? #309

Open
bgentry opened this issue Aug 6, 2024 · 1 comment
Open

jwt isValidIssuer check not strict enough? #309

bgentry opened this issue Aug 6, 2024 · 1 comment

Comments

@bgentry
Copy link
Contributor

bgentry commented Aug 6, 2024

Hi, while perusing the code I noticed this function:

clerk-sdk-go/jwt/jwt.go

Lines 127 to 133 in 685118c

func isValidIssuer(iss string, proxyURL *string) bool {
if proxyURL != nil {
return iss == *proxyURL
}
return strings.HasPrefix(iss, "https://clerk.") ||
strings.Contains(iss, ".clerk.accounts")
}

It appears to accept any issuer whose first domain segment is clerk.. Is this intentional? While it may not lead to a vulnerability on its own, it seems like it would have to be undesirable to accept an issuer like https://clerk.example.com (or substitute any attacker domain suffix).

Again I'm not saying this is itself a vulnerability, but if this check has any purpose at all then it would seem to be not doing a good enough job of serving it.

@gkats
Copy link
Member

gkats commented Sep 9, 2024

Hello @bgentry,

You are right, the issuer validity check is incomplete. I would say that it's a compromise though.

A more robust and complete check would be to verify that the JWT's issuer is the same as the instance's Frontend API.

Here's an example. If your domain is github.com then most likely your FAPI lives in clerk.github.com, and that's what the JWT's iss claim should be set to.

Unfortunately, the Go SDK currently has no way of knowing your domain without contacting Clerk's servers.

The problem could be easily solved if users provided their FAPI (or domain) when setting up the SDK, but we wanted to avoid having to pass too many configuration options.

The check (and its rationale) was carried over from v1. It provides a simple check leveraging just enough information, but it's by no means complete.

Hope the above explains the rationale behind this function.

Now, where do we go from here? I guess at some point we'd want to provide more configuration options to the SDK, especially if we want to support cookie based authentication too.

We should definitely revisit the iss claim validation. Is this currently a blocker for you?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants