Support for Placeholder Integration on JWKURL by Cafeine42 · Pull Request #96 · ggicci/caddy-jwt

Cafeine42 · 2025-04-30T17:59:13Z

Exemple of use case in Caddyfile :

        map {host} {app.jwk_url} {
            demo.localhost {$KEYCLOAK_URL}/realms/demo/protocol/openid-connect/certs
            default {$KEYCLOAK_URL}/realms/test/protocol/openid-connect/certs
        }
        log_append debug {app.jwk_url}
        jwtauth {
            jwk_url {app.jwk_url}
        }

Cafeine42 · 2025-04-30T17:59:36Z

go.mod

@@ -1,4 +1,4 @@
-module github.com/ggicci/caddy-jwt
+module github.com/Cafeine42/caddy-jwt


Got it. You don't have to change the module path if you do this in Git:

# clone the repo git clone git@github.com:ggicci/httpin.git # rename origin to upstream git remote rename origin upstream # add your forked repo as the origin remote git remote add origin git@github.com:Cafeine42/httpin.git # make some changes to the source code and commit to origin (your own repo) and open PR on GitHub git add ... git commit ... git push origin ...

I have this error when i try to install my version, it's why i changed this line.

RUN apk add --no-cache git &&
xcaddy build
--output /usr/local/bin/frankenphp
...
# Auth module
--with github.com/Cafeine42/caddy-jwt@v0.0.10

11.73 2025/05/27 09:45:51 [INFO] exec (timeout=0s): /usr/local/go/bin/go get -v -ldflags -w -s -extldflags '-Wl,-z,stack-size=0x80000' github.com/Cafeine42/caddy-jwt@v0.0.9 github.com/caddyserver/caddy/v2
15.89 go: downloading github.com/Cafeine42/caddy-jwt v0.0.9
21.22 go: github.com/Cafeine42/caddy-jwt@v0.0.9 found: parsing go.mod:
21.22 module declares its path as: github.com/ggicci/caddy-jwt
21.22 but was required as: github.com/Cafeine42/caddy-jwt
21.22 go: github.com/Cafeine42/caddy-jwt@v0.0.9 requires github.com/Cafeine42/caddy-jwt@v0.0.9: parsing go.mod:
21.22 module declares its path as: github.com/ggicci/caddy-jwt
21.22 but was required as: github.com/Cafeine42/caddy-jwt
21.22 2025/05/27 09:46:00 [FATAL] exit status 1

codecov-commenter · 2025-04-30T18:00:53Z

Codecov Report

Attention: Patch coverage is 70.51282% with 23 lines in your changes missing coverage. Please review.

Project coverage is 90.09%. Comparing base (a248851) to head (3ce36e8).

Files with missing lines	Patch %	Lines
jwt.go	70.51%	16 Missing and 7 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #96      +/-   ##
==========================================
- Coverage   95.10%   90.09%   -5.01%     
==========================================
  Files           2        2              
  Lines         347      404      +57     
==========================================
+ Hits          330      364      +34     
- Misses         12       28      +16     
- Partials        5       12       +7

Flag	Coverage Δ
unittests	`90.09% <70.51%> (-5.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ggicci · 2025-05-08T04:28:31Z

Thank you @Cafeine42 very much for the contribution! I will take a look this weekend and aim to merge and release as well.

ggicci · 2025-05-26T00:37:21Z

jwt.go

-	jwkCache     *jwk.Cache
-	jwkCachedSet jwk.Set
+	// JWK cache by resolved URL to support placeholders in JWKURL
+	jwkCaches map[string]*struct {


Since the keys are populated during requests handling, I believe it should be guarded with a sync.RWMutex to allow concurrent access safely. No?

ggicci · 2025-05-26T00:39:09Z

jwt.go

 }

 func (ja *JWTAuth) setupJWKLoader() {
+	// Initialiser le cache pour toutes les URL possibles


Cool, French is beautiful, but let's use English here 😆

ggicci · 2025-05-26T01:29:08Z

jwt.go

+		cache     *jwk.Cache
+		cachedSet jwk.Set
+	})
+	ja.logger.Info("JWK cache initialized for placeholder URL", zap.String("placeholder_url", ja.JWKURL))


Does it have to be a placeholder URL here? Looks like the wording is a little misleading as both a static JWK URL and a placeholder URL will go this code path.

ggicci · 2025-05-26T01:35:15Z

jwt.go

+			// Resolve JWKURL with placeholders
+			replacer := request.Context().Value(caddy.ReplacerCtxKey).(*caddy.Replacer)
+			resolvedURL := replacer.ReplaceAll(ja.JWKURL, "")
+


Even there're only two lines of code here, I would prefer creating a separate function to get the JWK URL which is bound to the given HTTP request.

ggicci · 2025-05-26T01:35:34Z

jwt.go

+			ja.logger.Info("JWK unresolved", zap.String("placeholder_url", ja.JWKURL))
+			ja.logger.Info("JWK resolved", zap.String("placeholder_url", resolvedURL))


Maybe it's better to merge them as one.

ggicci · 2025-05-26T01:38:53Z

jwt.go

 	// Replace placeholders in the signKey such as {file./path/to/sign_key.txt}
-	signKey = repl.ReplaceAll(signKey, "")
-	if len(signKey) == 0 {
+	resolvedSignKey := repl.ReplaceAll(signKey, "")


Nice renaming, thx!

ggicci · 2025-05-26T01:53:42Z

jwt.go

+		cache     *jwk.Cache
+		cachedSet jwk.Set


Also, shall we create a struct type for the caches? I saw it has been used multiple times in the code later.

type jwkCacheEntry struct { URL String Cache *jwk.Cache CachedSet jwk.Set }

ggicci · 2025-05-26T01:59:48Z

jwt.go

+	cache     *jwk.Cache
+	cachedSet jwk.Set


Please simplify this by using a new type, which was requested in my comments above.

ggicci · 2025-05-26T02:14:56Z

jwt.go

+	// Try to refresh the cache immediately
+	_, err = cache.Refresh(context.Background(), resolvedURL)
+	if err != nil {
+		ja.logger.Warn("failed to refresh JWK cache during initialization", zap.Error(err), zap.String("url", resolvedURL))
+		// Continue even in case of error, the important thing is that the URL is registered
+	}


This block is a repeat of refreshJWKCache(resolvedURL). I can imagine why you didn't call refreshJWKCache here was that it creates recusive calls.

Creating fine grained functions can avoid this. For example:

refreshJWKCache(entry, resolvedURL) // or even better when we have the new type for the entry entry.refresh() // entry: of jwkCacheEntry

ggicci · 2025-05-26T02:20:04Z

jwt.go

+	entry, err := ja.getOrCreateJWKCache(resolvedURL)
+	if err != nil {
+		return err
+	}


Since we want to reuse the refresh logic in getOrCreateJWKCache(), let's don't call getOrCreateJWKCache() here. The function should have less dependencies. I would suggest encapsulating it in a method of the proposed jwkCacheEntry struct above.

ggicci

hey @Cafeine42, thanks for the contribution. The code logic looks good to me. I left a bunch of comments there, basically all for code simplifying. Please let me know if you want my help on the update in case you don't have much time.

Cafeine42 · 2025-05-27T11:25:44Z

@ggicci Thanks for the review ! I’ve implemented the suggested changes, they help simplify the code. I wasn’t able to test everything thoroughly on my project though, so let me know if you run into any issues or if there’s anything else to tweak. I might test it more extensively next week.

ggicci

LGTM, thx :)

ggicci · 2025-05-30T06:33:01Z

Too late now, I will merge and add docs and then do the release soon. Again, thanks very much for the feature implementation :)

change module

1c34bf9

Cafeine42 commented Apr 30, 2025

View reviewed changes

Cafeine42 force-pushed the main branch from ac34e6e to d7c7229 Compare May 1, 2025 06:34

Support placeholder on JWKURL

2a121c1

Cafeine42 force-pushed the main branch from d7c7229 to 2a121c1 Compare May 1, 2025 06:40

ggicci reviewed May 26, 2025

View reviewed changes

ggicci requested changes May 26, 2025

View reviewed changes

Cafeine42 force-pushed the main branch 2 times, most recently from de67265 to 03b05a6 Compare May 27, 2025 09:46

Change for PR

3ce36e8

Cafeine42 force-pushed the main branch from 03b05a6 to 3ce36e8 Compare May 27, 2025 13:06

ggicci approved these changes May 30, 2025

View reviewed changes

ggicci merged commit a0761d8 into ggicci:main Jun 3, 2025
1 check passed

		@@ -1,4 +1,4 @@
		module github.com/ggicci/caddy-jwt
		module github.com/Cafeine42/caddy-jwt

		ja.logger.Info("JWK unresolved", zap.String("placeholder_url", ja.JWKURL))
		ja.logger.Info("JWK resolved", zap.String("placeholder_url", resolvedURL))

Uh oh!

Conversation

Cafeine42 commented Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Cafeine42 Apr 30, 2025

Choose a reason for hiding this comment

Uh oh!

ggicci May 26, 2025

Choose a reason for hiding this comment

Uh oh!

Cafeine42 May 27, 2025

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ggicci commented May 8, 2025

Uh oh!

ggicci May 26, 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!

ggicci May 26, 2025

Choose a reason for hiding this comment

Uh oh!

ggicci May 26, 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!

ggicci May 26, 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!

ggicci May 26, 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!

ggicci May 26, 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!

ggicci May 26, 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!

ggicci May 26, 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!

ggicci May 26, 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!

ggicci May 26, 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!

ggicci left a comment

Choose a reason for hiding this comment

Uh oh!

Cafeine42 commented May 27, 2025

Uh oh!

ggicci left a comment

Choose a reason for hiding this comment

Uh oh!

ggicci commented May 30, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Cafeine42 commented Apr 30, 2025 •

edited

Loading

codecov-commenter commented Apr 30, 2025 •

edited

Loading

ggicci May 26, 2025 •

edited

Loading

ggicci May 26, 2025 •

edited

Loading

ggicci May 26, 2025 •

edited

Loading

ggicci May 26, 2025 •

edited

Loading

ggicci May 26, 2025 •

edited

Loading

ggicci May 26, 2025 •

edited

Loading

ggicci May 26, 2025 •

edited

Loading

ggicci May 26, 2025 •

edited

Loading

ggicci May 26, 2025 •

edited

Loading