New features added over the past 6 months have created some really bloated files and complicated architecture. The plugin needs a major refactor to break up major functionality and improve code documentation. Way too much stuff is in the ServeHTTP handler that could be separated out into smaller functions for independent testing.
The list of items below are some ideas for a major refactoring. If you're looking for some way to contribute to this plugin, feel free to tackle any of these.
[x] Get rid of the global backend cache - Currently, there is a global cache for key material that is read from the filesystem. It contains some state in backend.current that is the source of subtle bugs, particularly during testing. The backend should be broken out into its own file and become the main interface for dealing with secrets, whether in files, env vars, or elsewhere (kubernetes secrets?). Ideally, the current state should be removed and passed explicitly in calls to locate key material. The backend should move into the Auth struct because there's no good reason to keep it as a global var and would make it easier to mock in tests.
[ ] Deprecate individual claims in headers - Claims from a valid token are passed as individual HTTP headers. This works fine for a fairly flat and small claim structure. Some third-party tokens (like Auth0) bundle a lot of deeply nested and FQDN-namespaced claims that cause problems with HTTP headers. They can contain non-ASCII characters. Currently, the claim names are URL-encoded but the values are not. This can cause subtle bugs and errors when non-ASCII characters are encoded either as header names or values. Over time, I would like to deprecate the individual headers in favor of sending all the claims in one base64-encoded JSON structure that can be read/manipulated downstream. This removes the requirement to flatten deeply-nested claim values, allows UTF-8 values, and eliminates problems with type-coercion to strings. This is a major breaking change.
Token-Claims: <base64 encoded JSON stucture>
[ ] Refactor tests - The test file test_jwt.go now clocks in at near 1000 lines due to scope-creep. Many of the tests are end-to-end integration tests because the individual features are not broken out into smaller functions. At the same time these are broken out, I'd like to refactor the tests to improve the ability to test smaller pieces of functionality. This would allow more advanced security testing techniques, like fuzzing inputs. It might be better to move away from ginkgo/gomega for tests, relying more on test tables and standard library facilities.
[x] Consider vendoring dependencies - At a minimum, the core dependency on jwt-go should be vendored to prevent a major upgrade of that library from breaking new Caddy builds. Dep is now stable enough to be used. The vendor directory would have to be committed to the repository because the Caddy build server cannot deal with dep lock files. This is not supported by the current build server.
[ ] Add optional logging - When this plugin was created, the best practice for Caddy plugins was not to log anything. I think logging should be optional, but it needs to integrate well with Caddy main logging functionality. It would be an immense help in debugging scenarios. Currently, the only information that is available is the 401 response, which doesn't provide enough information to determine the cause of the failure, which could be a result of missing secrets, misconfiguration, etc.
New features added over the past 6 months have created some really bloated files and complicated architecture. The plugin needs a major refactor to break up major functionality and improve code documentation. Way too much stuff is in the
ServeHTTPhandler that could be separated out into smaller functions for independent testing.The list of items below are some ideas for a major refactoring. If you're looking for some way to contribute to this plugin, feel free to tackle any of these.
[x] Get rid of the global backend cache - Currently, there is a global cache for key material that is read from the filesystem. It contains some state in
backend.currentthat is the source of subtle bugs, particularly during testing. The backend should be broken out into its own file and become the main interface for dealing with secrets, whether in files, env vars, or elsewhere (kubernetes secrets?). Ideally, thecurrentstate should be removed and passed explicitly in calls to locate key material. The backend should move into the Auth struct because there's no good reason to keep it as a global var and would make it easier to mock in tests.[ ] Deprecate individual claims in headers - Claims from a valid token are passed as individual HTTP headers. This works fine for a fairly flat and small claim structure. Some third-party tokens (like Auth0) bundle a lot of deeply nested and FQDN-namespaced claims that cause problems with HTTP headers. They can contain non-ASCII characters. Currently, the claim names are URL-encoded but the values are not. This can cause subtle bugs and errors when non-ASCII characters are encoded either as header names or values. Over time, I would like to deprecate the individual headers in favor of sending all the claims in one base64-encoded JSON structure that can be read/manipulated downstream. This removes the requirement to flatten deeply-nested claim values, allows UTF-8 values, and eliminates problems with type-coercion to strings. This is a major breaking change.
[ ] Refactor tests - The test file
test_jwt.gonow clocks in at near 1000 lines due to scope-creep. Many of the tests are end-to-end integration tests because the individual features are not broken out into smaller functions. At the same time these are broken out, I'd like to refactor the tests to improve the ability to test smaller pieces of functionality. This would allow more advanced security testing techniques, like fuzzing inputs. It might be better to move away from ginkgo/gomega for tests, relying more on test tables and standard library facilities.[x] Consider vendoring dependencies - At a minimum, the core dependency on jwt-go should be vendored to prevent a major upgrade of that library from breaking new Caddy builds. Dep is now stable enough to be used. The vendor directory would have to be committed to the repository because the Caddy build server cannot deal with dep lock files.This is not supported by the current build server.[ ] Add optional logging - When this plugin was created, the best practice for Caddy plugins was not to log anything. I think logging should be optional, but it needs to integrate well with Caddy main logging functionality. It would be an immense help in debugging scenarios. Currently, the only information that is available is the 401 response, which doesn't provide enough information to determine the cause of the failure, which could be a result of missing secrets, misconfiguration, etc.