jwt validation #171

abel296 · 2024-01-05T08:00:19Z

Features:

Provide the system with the possibility of validating jwt
Jwt config: validation included paths, allowed scopes and jwks url config
Jwt config can be set per domain or in common config

abel296 · 2024-01-08T14:57:55Z

Hi @fabiocicerchia , I don't have access to see why the snyk test is not passing, can you give me access permit to check it out please?

fabiocicerchia · 2024-01-12T20:02:04Z

Hi @abel296, thank you for the PR.
I'll try to sort out the accesses to the project, possibly make it available publicly, in the meantime here are the errors raised by snyk:

abel296 · 2024-01-15T09:33:49Z

Hi @fabiocicerchia , could you review the PR please?

config/config.go

config/model.go

server/jwt/jwt.go

server/jwt/jwt_test.go

server/jwt/jwt_utils.go

config/model.go

server/jwt/jwt_functional_test.go

server/jwt/jwt.go

abel296 · 2024-01-29T10:37:29Z

Hi @fabiocicerchia , we've added a couple of changes:

we've adjusted the jwt initialization (https://github.com/fabiocicerchia/go-proxy-cache/compare/35c881e815019187be5c26317470d4609f7b11bf..06f0b0f106c46bdb981f0dfba25f7a8d62f7fce0 and https://github.com/fabiocicerchia/go-proxy-cache/compare/7df31b69d7c94c4779ea41ef425f8826df005ff4..ccc6bd3f46cad138169a3aa2c737734306fe9f0e)
we've made the JWKS URL configurable by domain through an environment variable as well, this being, if it exists, the predominant configuration (https://github.com/fabiocicerchia/go-proxy-cache/compare/06f0b0f106c46bdb981f0dfba25f7a8d62f7fce0..ff0bc17f29bdd8e1a7a27e59d2a947a1f49c6e73) . We also added some documentation about it (https://github.com/fabiocicerchia/go-proxy-cache/compare/ff0bc17f29bdd8e1a7a27e59d2a947a1f49c6e73..0b3995e57de9854c9e51d9e8c0c8250571a43ede)

abel296 · 2024-03-12T14:04:50Z

Hi @fabiocicerchia , these are the last updates:

excluded paths instead of included paths in JWT config
regex compatibility with the excluded paths config for excluding dynamic paths
improved JWT init and JWT handler
improved testing nginx dockerfile to avoid compatibility issues with otel_ngx_module.so
migration from github.com/lestrrat-go/jwx to github.com/lestrrat-go/jwx/v2
update dependencies

Signed-off-by: Abel Andrés <[email protected]>

fabiocicerchia

Hi @abel296, apologies for not getting back to you sooner, recently I couldn't focus on the project properly.
I'll try spending some more time to test properly this MR in the next following days.

It would be really good not to push force and have a reviewable commit history, because as of now, it's really impossible to figure out what each change contributed to.

I made some more suggestions, but let me test it first before starting fixing, as I might add some more comments, and don't want to let you do too much back and forth.

Thank you for the patience and the effort for putting together this PR!

fabiocicerchia · 2024-10-21T18:11:49Z

.env.dist

+#  - JWT_JWKS_URL enviroment variable
+
+# Time in minutes that takes for JWKS to refresh automatically
+# JWT_REFRESH_INTERVAL=


Let's use the same example values from config.yml.dist:

Suggested change

# JWT_REFRESH_INTERVAL=

# JWT_REFRESH_INTERVAL=15

fabiocicerchia · 2024-10-21T18:12:00Z

.env.dist

+
+# --- JWT
+# A list of space-separated paths.
+# JWT_EXCLUDED_PATHS=


Let's use the same example values from config.yml.dist:

Suggested change

# JWT_EXCLUDED_PATHS=

# JWT_EXCLUDED_PATHS=/

fabiocicerchia · 2024-10-21T18:12:46Z

.env.dist

+# JWT_EXCLUDED_PATHS=
+
+# A list of space-separated scopes to be allowed.
+# JWT_ALLOWED_SCOPES=


Let's use the same example values from config.yml.dist:

Suggested change

# JWT_ALLOWED_SCOPES=

# JWT_ALLOWED_SCOPES=scope1, scope2

fabiocicerchia · 2024-10-21T18:12:59Z

config.yml.dist

+#   # A list of space-separated paths.
+#   excluded_paths:
+#     - /
+#   # A list of space-separated scopes to be allowed.


space or comma separated?

fabiocicerchia · 2024-10-21T18:17:03Z

config/model.go

+// Jwt - Defines the config for the jwt validation.
+type Jwt struct {
+	ExcludedPaths       []string   `yaml:"excluded_paths" envconfig:"JWT_EXCLUDED_PATHS" split_words:"true"`
+	AllowedScopes       []string   `yaml:"allowed_scopes" envconfig:"JWT_ALLOWED_SCOPES" split_words:"true"`


scope1, scope2 will end up in having item 0 scope1, and item 1 scope2

fabiocicerchia · 2024-10-21T18:30:32Z

server/jwt/jwt_functional_test.go

+
+	h.ServeHTTP(rr, req)
+
+	assert.Equal(t, http.StatusMovedPermanently, rr.Code)


it would be useful to assert the Location header of this 301 respose.

fabiocicerchia · 2024-10-21T18:30:43Z

server/jwt/jwt_functional_test.go

+
+	h.ServeHTTP(rr, req)
+
+	assert.Equal(t, http.StatusMovedPermanently, rr.Code)


it would be useful to assert the Location header of this 301 respose.

fabiocicerchia · 2024-10-21T18:32:18Z

server/jwt/jwt_functional_test.go

+
+	h.ServeHTTP(rr, req)
+
+	assert.Equal(t, http.StatusMovedPermanently, rr.Code)


is it correct that all the tests with a JWT are going to 301?

fabiocicerchia · 2024-10-21T18:33:18Z

server/jwt/jwt_test.go

+
+	res := getScopes(token)
+
+	assert.ElementsMatch(t, res, []string{"scope1", "scope2", "scope3"}, "Scopes provided doesn't match")


Suggested change

assert.ElementsMatch(t, res, []string{"scope1", "scope2", "scope3"}, "Scopes provided doesn't match")

assert.ElementsMatch(t, res, []string{"scope1", "scope2", "scope3"}, "The provided scopes don't match")

fabiocicerchia · 2024-10-21T18:33:36Z

server/jwt/jwt_test.go

+
+	res := getScopes(token)
+
+	assert.ElementsMatch(t, res, []string{"scope1", "scope2", "scope3"}, "Scopes provided doesn't match")


Suggested change

assert.ElementsMatch(t, res, []string{"scope1", "scope2", "scope3"}, "Scopes provided doesn't match")

assert.ElementsMatch(t, res, []string{"scope1", "scope2", "scope3"}, "The provided scopes don't match")

fabiocicerchia · 2024-10-26T20:02:48Z

server/jwt/jwt.go

+		return logJWTErrorAndAbort(w, err)
+	}
+	if err := jwt.Validate(token); err != nil {
+		return logJWTErrorAndAbort(w, err)


it would be good to increase the test coverage by adding a test case to cover this scenario as well (ie testing the failure of validation).

fabiocicerchia · 2024-10-26T20:45:26Z

server/jwt/jwt_test.go

+}
+
+func TestRefreshKeySet(t *testing.T) {
+	t.Skip("To run this test, you should set refreshIntervalDuration (from config.go) to time.Second")


Can this test be executed automatically, without any manual changes?

fabiocicerchia · 2024-10-27T13:38:51Z

@abel296 please have a look at some suggestions made in #208

fabiocicerchia · 2024-11-04T21:39:13Z

merged all changes + improvements in #208

abel296 force-pushed the feature/jwt-validation branch from 6dd2956 to f205092 Compare January 5, 2024 13:21

monwolf mentioned this pull request Jan 8, 2024

JWT Validation #154

Open

abel296 force-pushed the feature/jwt-validation branch 6 times, most recently from 31b1471 to 502345e Compare January 8, 2024 14:41

abel296 force-pushed the feature/jwt-validation branch from 502345e to 6bafe05 Compare January 9, 2024 07:35

juansm90 force-pushed the feature/jwt-validation branch from 6bafe05 to 291df62 Compare January 9, 2024 09:56

abel296 force-pushed the feature/jwt-validation branch 4 times, most recently from a8bd311 to 8acec37 Compare January 11, 2024 10:57

abel296 force-pushed the feature/jwt-validation branch 3 times, most recently from f326f4a to 7b4c1a4 Compare January 15, 2024 09:05

fabiocicerchia requested changes Jan 17, 2024

View reviewed changes

abel296 force-pushed the feature/jwt-validation branch from 7b4c1a4 to c398bcb Compare January 18, 2024 12:02

abel296 requested a review from fabiocicerchia January 18, 2024 12:11

fabiocicerchia requested changes Jan 24, 2024

View reviewed changes

config/model.go Outdated Show resolved Hide resolved

server/jwt/jwt_functional_test.go Outdated Show resolved Hide resolved

server/jwt/jwt.go Outdated Show resolved Hide resolved

abel296 force-pushed the feature/jwt-validation branch 5 times, most recently from ff0bc17 to 0b3995e Compare January 29, 2024 10:27

abel296 force-pushed the feature/jwt-validation branch from dc4cc31 to dcf1a34 Compare February 21, 2024 09:26

abel296 force-pushed the feature/jwt-validation branch 19 times, most recently from 23c7218 to 4e87b96 Compare March 12, 2024 13:45

added jwt validation

c12a5ce

Signed-off-by: Abel Andrés <[email protected]>

abel296 force-pushed the feature/jwt-validation branch from 4e87b96 to c12a5ce Compare March 15, 2024 11:55

fabiocicerchia requested changes Oct 21, 2024

View reviewed changes

fabiocicerchia reviewed Oct 26, 2024

View reviewed changes

fabiocicerchia mentioned this pull request Oct 27, 2024

Improvements on JWT Validation PR#171 #208

Merged

fabiocicerchia closed this Nov 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

jwt validation #171

jwt validation #171

abel296 commented Jan 5, 2024

abel296 commented Jan 8, 2024

fabiocicerchia commented Jan 12, 2024

abel296 commented Jan 15, 2024

abel296 commented Jan 29, 2024 •

edited

Loading

abel296 commented Mar 12, 2024

fabiocicerchia left a comment •

edited

Loading

fabiocicerchia Oct 21, 2024

fabiocicerchia Oct 21, 2024

fabiocicerchia Oct 21, 2024

fabiocicerchia Oct 21, 2024

fabiocicerchia Oct 21, 2024

fabiocicerchia Oct 21, 2024

fabiocicerchia Oct 21, 2024

fabiocicerchia Oct 21, 2024

fabiocicerchia Oct 21, 2024

fabiocicerchia Oct 21, 2024

fabiocicerchia Oct 26, 2024

fabiocicerchia Oct 26, 2024

fabiocicerchia commented Oct 27, 2024

fabiocicerchia commented Nov 4, 2024


		h.ServeHTTP(rr, req)

		assert.Equal(t, http.StatusMovedPermanently, rr.Code)


		res := getScopes(token)

		assert.ElementsMatch(t, res, []string{"scope1", "scope2", "scope3"}, "Scopes provided doesn't match")

jwt validation #171

jwt validation #171

Conversation

abel296 commented Jan 5, 2024

abel296 commented Jan 8, 2024

fabiocicerchia commented Jan 12, 2024

abel296 commented Jan 15, 2024

abel296 commented Jan 29, 2024 • edited Loading

abel296 commented Mar 12, 2024

fabiocicerchia left a comment • 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

fabiocicerchia commented Oct 27, 2024

fabiocicerchia commented Nov 4, 2024

abel296 commented Jan 29, 2024 •

edited

Loading

fabiocicerchia left a comment •

edited

Loading