Create unit tests

[lgrosz]

Unit tests should be runnable as a separate application and added with CMakes add_test. See src/t in the lighttpd repo.

Unfortunately, what makes this hard is that this repo lacks the lighttpd source to compile tests, my lack of understanding of lighttpd's request_t, and my lack of experience with testing 😳.

[lgrosz]

The first point, of the lighttpd source not being available, also disallows compilation on some platforms (like MacOS), as they do not (or I have not found the flags to allow for) compiling as a loadable module.

I could just inject lighttpd as a git submodule, build it, and create some cmake find/config that exposes the shared source to be linked compiled into an external modules tests.

Or.. I could build off my current CMake variables and just access the source directly through LIGHTTPD_SOURCE_DIR, but that seems a bit unreliable as I've already experienced issues with LIGHTTPD_BUILD_DIR when building with different build tools (cmake vs autoconf).

[gstrauss]

Hi! I am a lighttpd developer and happened across this project. How is it working for you? Do you have any plans to try to contribute this to upstream lighttpd? Your use of BSD 3-Clause license is compatible with lighttpd, also BSD 3-Clause. I mention this since writing unit tests might be easier if integrated with the lighttpd source.

In the meantime, I'd recommend a prep step for your unit tests where you unpack the lighttpd source, patch this module into the source tree, and then build and run the unit tests in lighttpd.

---

Some quick notes looking at the code: For performance and simpler configuration, I'd recommend aggregating all the separate config options into a single config option of a list of key,value pairs. Then, at lighttpd startup, parse the list into structured data, including such things as reading the keyfile into memory (instead of repeatedly reading the keyfile every single request). I could help with this, or point you to some code examples in other lighttpd modules.

[lgrosz]

Hi, @gstrauss. Thanks for checking out my module!

How is it working for you? Do you have any plans to try to contribute this to upstream lighttpd?

It's working great for the use-case I have. I'd be excited to contribute upstream, assuming its range of use-cases isn't too narrow. I'm not sure how common JWT authentication/authorization is without the rest of OAuth 2.0.

I am kind of hacking around the basic/digest handlers at the moment. I'm not sure how desirable this is. Ideally, there'd be a bearer/token authentication handler, but I can understand how the addition of that may blow things up.

In the meantime, I'd recommend a prep step for your unit tests where you unpack the lighttpd source, patch this module into the source tree, and then build and run the unit tests in lighttpd.

That sounds good. I'll use your mod_authn_tkt as a guide.

I'd recommend aggregating all the separate config options into a single config option of a list of key,value pairs. Then, at lighttpd startup, parse the list into structured data, including such things as reading the keyfile into memory (instead of repeatedly reading the keyfile every single request).

I was toying with this idea when I was writing this module. I'll definitely make those changes.

[gstrauss]

I have performed some code transformations in a fork at https://github.com/gstrauss/mod_authn_jwt Note: My develop branch is completely untested (besides simple compilation) and I might force-push changes.

Please review the patches in the order committed (git log --reverse -p 418d6f68..HEAD)

I have a question about something which looks like a bug. You have issuer, subject, and audience all use jwt_valid_add_grant() with "iss", rather than using "iss", "sub", and "aud", respectively. It looks like you committed some tests which use "iss" for all of these, too, though I have not looked closely. Why are you using "iss" for all of these instead of only for issuer?

FYI: Fedora 40 (released last week) still ships libjwt version 1.12.1 from Oct 2020, which does not have jwt_exception_str(), jwt_valid_set_exp_leeway(), or jwt_valid_set_nbf_leeway().

[gstrauss]

Why are auth.backend.jwt.issuer, auth.backend.jwt.subject, auth.backend.jwt.audience not simply included as key => value pairs in auth.backend.jwt.claims, for, respectively, keys "iss", "sub", and "aud"?

[lgrosz]

Please review the patches in the order committed (git log --reverse -p 418d6f68..HEAD)

I'll get on this

I have a question about something which looks like a bug. You have issuer, subject, and audience all use jwt_valid_add_grant() with "iss", rather than using "iss", "sub", and "aud", respectively. It looks like you committed some tests which use "iss" for all of these, too, though I have not looked closely. Why are you using "iss" for all of these instead of only for issuer?

You are absolutely correct. Subject and audience should be "sub" and "aud" respectively.

FYI: Fedora 40 (released last week) still ships libjwt version 1.12.1 from Oct 2020, which does not have jwt_exception_str(), jwt_valid_set_exp_leeway(), or jwt_valid_set_nbf_leeway().

I noticed this as well when installing the package on an Github hosted ubuntu-latest runner. I opened an issue, benmcollins/libjwt#196, to get clarification on the out-of-date packages. For now I am just building the dependency from source.

[lgrosz]

Why are auth.backend.jwt.issuer, auth.backend.jwt.subject, auth.backend.jwt.audience not simply included as key => value pairs in auth.backend.jwt.claims, for, respectively, keys "iss", "sub", and "aud"?

Those claims are specifically called out in RFC 7519 Section 4.1, so I thought helpers were reasonable.

[lgrosz]

commit 4c055196b2f1595a80169cd21428f71f8c32101e
Author: Glenn Strauss <gstrauss@gluelogic.com>
Date:   Tue Apr 30 18:04:14 2024 -0400

    some jwt_valid_* funcs do not fail

    some jwt_valid_* funcs do not fail unless (jwt_valid_t *) passed is NULL

diff --git a/mod_authn_jwt.c b/mod_authn_jwt.c
index c411019..e564ab2 100644
--- a/mod_authn_jwt.c
+++ b/mod_authn_jwt.c
@@ -362,17 +362,8 @@ handler_t mod_authn_jwt_bearer(request_st *r, void *p_d, const http_auth_require
     // TODO These fields should be propogated as error data to the client
     unsigned int errno;

-    errno = jwt_valid_set_exp_leeway(jwt_valid, p->conf.exp_leeway);
-    if (0 != errno) {
-        log_error(r->conf.errh, __FILE__, __LINE__, "Failed to set exp_leeway to %d: %s", p->conf.exp_leeway, jwt_exception_str(errno));
-        goto jwt_valid_finish;
-    }
-
-    errno = jwt_valid_set_nbf_leeway(jwt_valid, p->conf.nbf_leeway);
-    if (0 != errno) {
-        log_error(r->conf.errh, __FILE__, __LINE__, "Failed to set nbf_leeway to %d: %s", p->conf.nbf_leeway, jwt_exception_str(errno));
-        goto jwt_valid_finish;
-    }
+    jwt_valid_set_exp_leeway(jwt_valid, p->conf.exp_leeway);
+    jwt_valid_set_nbf_leeway(jwt_valid, p->conf.nbf_leeway);

     if (NULL != p->conf.issuer) {
         errno = jwt_valid_add_grant(jwt_valid, "iss", p->conf.issuer->ptr);
@@ -438,11 +429,7 @@ handler_t mod_authn_jwt_bearer(request_st *r, void *p_d, const http_auth_require
         }
     }

-    errno = jwt_valid_set_now(jwt_valid, (time_t)log_epoch_secs);
-    if (0 != errno) {
-        log_error(r->conf.errh, __FILE__, __LINE__, "Failed to set now: %s", jwt_exception_str(errno));
-        goto jwt_valid_finish;
-    }
+    jwt_valid_set_now(jwt_valid, (time_t)log_epoch_secs);
     errno = jwt_validate(jwt, jwt_valid);
     if (0 != errno) {

jwt_valid_set_now does return a valid errno, so let's revert just that hunk.

[gstrauss]

jwt_valid_set_now does return a valid errno, so maybe revert this hunt?

Use the source, Luke. As my commit message stated, those routines return an error only if the passed in jwt_valid param is NULL.

int jwt_valid_set_now(jwt_valid_t *jwt_valid, const time_t now)
{
        if (!jwt_valid)
                return EINVAL;

        jwt_valid->now = now;

        return 0;
}

[gstrauss]

Why are auth.backend.jwt.issuer, auth.backend.jwt.subject, auth.backend.jwt.audience not simply included as key => value pairs in auth.backend.jwt.claims, for, respectively, keys "iss", "sub", and "aud"?

Those claims are specifically called out in RFC 7519 Section 4.1, so I thought helpers were reasonable.

Those are "4.1. Registered Claim Names" and there is no reason to have separate accessors. They can be listed in the claims list.

[lgrosz]

jwt_valid_set_now does return a valid errno, so maybe revert this hunt?

Use the source, Luke. As my commit message stated, those routines return an error only if the passed in jwt_valid param is NULL.

int jwt_valid_set_now(jwt_valid_t *jwt_valid, const time_t now)
{
        if (!jwt_valid)
                return EINVAL;

        jwt_valid->now = now;

        return 0;
}

That's fair. I didn't look at the source, just went by the documentation - my mistake.

[lgrosz]

Why are auth.backend.jwt.issuer, auth.backend.jwt.subject, auth.backend.jwt.audience not simply included as key => value pairs in auth.backend.jwt.claims, for, respectively, keys "iss", "sub", and "aud"?

Those claims are specifically called out in RFC 7519 Section 4.1, so I thought helpers were reasonable.

Those are "4.1. Registered Claim Names" and there is no reason to have separate accessors. They can be listed in the claims list.

Could the same be said for exp and nbf? I suppose the RFC details how to handle those, while aud, sub, and iss claims are generally processed application specific... My only reason to having keys for aud, sub, and iss, is that they were detailed in the RFC. If that reason alone is not good enough, then removing those keys is good with me.

[gstrauss]

The RFC lists other Registered Claim Names, too. I do not see your logic of explicitly listing some of them, when they all fit the definition of Claims.

Regarding exp and nbf, the functions jwt_valid_set_exp_leeway() and jwt_valid_set_nbf_leeway() take integer arguments and the libjwt code does not parse them from claims submitted with jwt_valid_add_grant(). That is a shortcoming of the current libjwt code, and I wish it were different, but that limitation means that the same can not be said for exp and nbf in the current libjwt code. Time allowing, it might be worth submitting a patch to libjwt to fix that.

[lgrosz]

The RFC lists other Registered Claim Names, too. I do not see your logic of explicitly listing some of them, when they all fit the definition of Claims.

I agree, let's remove .audience, .subject, and .issuer in favor of just using .claims.

[lgrosz]

commit ea543616e6c7b7cf7255e48fb072a4aa812eb1ae
Author: Glenn Strauss <gstrauss@gluelogic.com>
Date:   Tue Apr 30 04:12:51 2024 -0400

    startup parse auth.backend.jwt.opts

diff --git a/mod_authn_jwt.c b/mod_authn_jwt.c
index 5327368..70b3888 100644
--- a/mod_authn_jwt.c
+++ b/mod_authn_jwt.c
<-- lines omitted -->
@@ -378,97 +405,30 @@ handler_t mod_authn_jwt_bearer(request_st *r, void *p_d, const http_auth_require
< -- lines omitted -->
+        // TODO These fields should be propagated as error data to the client
+        // (??? revisit comment; be careful about info exposed to client)
< -- lines omitted -->

These are the type of responses I was referencing. They may be out of scope of the module. I hadn't implemented anything for the reason you stated.

[gstrauss]

I'll be pushing some additional commits in a little while which add some of this extra info when lighttpd debugging is enabled with lighttpd.conf debug.log-request-header = "enable" and debug.log-response-header = "enable". Due to the potential for replay attacks in some configurations, the token should not be logged to the error log by default.

[gstrauss]

I force-pushed to my fork with some additional commits. Even if you do not agree with all the changes, I hope that I broke things up into multiple commits in such a way that you could follow along with the stepwise code transformations.

[lgrosz]

commit ea543616e6c7b7cf7255e48fb072a4aa812eb1ae
Author: Glenn Strauss <gstrauss@gluelogic.com>
Date:   Tue Apr 30 04:12:51 2024 -0400

    startup parse auth.backend.jwt.opts

diff --git a/mod_authn_jwt.c b/mod_authn_jwt.c
index 5327368..70b3888 100644
--- a/mod_authn_jwt.c
+++ b/mod_authn_jwt.c
<-- lines omitted -->
@@ -83,34 +79,14 @@ FREE_FUNC(mod_authn_jwt_cleanup) {

 static void mod_authn_jwt_merge_config_cpv(plugin_config * const pconf, const config_plugin_value_t * const cpv) {
     switch (cpv->k_id) { /* index into static config_plugin_keys_t cpk[] */
-      case 0: /* auth.backend.jwt.keyfile */
+      case 0: /* auth.backend.jwt.opts */
+        if (cpv->vtype == T_CONFIG_LOCAL) break;
+        pconf->jwt_valid = cpv->v.v;
+        break;
+      case 1: /* auth.backend.jwt.keyfile */
<-- lines omitted -->

Should this be cpv->vtype != T_CONFIG_LOCAL?

[lgrosz]

Two scenarios have changed response behavior

• When a token is supplied but it is a malformed token.. e.g. Bearer acbdefg
• When a token is supplied but fails to be decoded (bad signature)

If I'm reading RFC 6750 Section 3.1 correctly, these cases should have a 401 response.

The access token provided is expired, revoked, malformed, or invalid for other reasons. The resource SHOULD respond with the HTTP 401 (Unauthorized) status code.

[lgrosz]

The 400 case also specifies malformed.. so I'm not sure what the response should be.

... or is otherwise malformed. The resource server SHOULD respond with the HTTP 400 (Bad Request) status code.

[lgrosz]

Two scenarios have changed response behavior

• When a token is supplied but it is a malformed token.. e.g. Bearer acbdefg
• When a token is supplied but fails to be decoded (bad signature)

If I'm reading RFC 6750 Section 3.1 correctly, these cases should have a 401 response.

The access token provided is expired, revoked, malformed, or invalid for other reasons. The resource SHOULD respond with the HTTP 401 (Unauthorized) status code.

Scratch this. With your latest patches the behavior is the same.

[lgrosz]

I force-pushed to my fork with some additional commits. Even if you do not agree with all the changes, I hope that I broke things up into multiple commits in such a way that you could follow the along with the stepwise code transformations.

Thank you! I've checked them out and I agree with the changes. I appreciate the well-formed commits; that made it easy to review. I'll pull the patches into my develop branch.

[gstrauss]

Should this be cpv->vtype != T_CONFIG_LOCAL?

Yes. I'll fix that. Thanks.

[gstrauss]

BTW, my suggestions combined the config into to options: auth.backend.jwt.opts and auth.backend.jwt.keyfile. Is it common or desirable to have both in auth.backend.jwt.opts? Might opts be shared with different keyfile for different parts of a site or different sites on the same server? If not, I'll probably combine into .opts. I will probably need to extend .opts with an additional parameter for which label to use to set REMOTE_USER. Alternatively, I could make this not configurable and simply check for 'email' and if not found, check for 'user'. https://www.iana.org/assignments/jwt/jwt.xhtml Are there common conventions already in use?

[gstrauss]

https://github.com/OpenIDC/mod_auth_openidc README suggests that it creates REMOTE_USER as [sub] claim, concatenated with the OP's Issuer identifier ([sub]@[iss]). That sounds like a good initial start.

[gstrauss]

I pushed a commit to my develop branch to set REMOTE_USER. Compiles but not otherwise tested.

On your gstrauss-develop branch, you can git fetch and then git rebase gstrauss/develop (if git remote -v to my repo is named gstrauss)

[lgrosz]

BTW, my suggestions combined the config into to options: auth.backend.jwt.opts and auth.backend.jwt.keyfile. Is it common or desirable to have both in auth.backend.jwt.opts? Might opts be shared with different keyfile for different parts of a site or different sites on the same server? If not, I'll probably combine into .opts.

I think it is fine to combine it into .opts. It's probably preferable since the key would be tied to .opts.algorithm.

[lgrosz]

https://github.com/OpenIDC/mod_auth_openidc README suggests that it creates REMOTE_USER as [sub] claim, concatenated with the OP's Issuer identifier ([sub]@[iss]). That sounds like a good initial start.

This sounds good to me. REMOTE_USER is something I hadn't really considered up to this point.