Proposal: Remove the auth_jwt_redirect directive

[max-lt]

The auth_jwt_redirect directive makes the code overly complicated and can be easily replaced by the nginx's error_page directive:

from:

location ~ ^/secure/ {
    auth_jwt_enabled on;
    auth_jwt_validation_type COOKIE=rampartjwt;
    auth_jwt_redirect on;
    auth_jwt_loginurl "https://teslagov.com";
}

to:

location @login_err_redirect {
    return 302 https://teslagov.com?redirect=$request_uri&$args;
}

location ~ ^/secure/ {
    auth_jwt_enabled on;
    auth_jwt_validation_type COOKIE=rampartjwt;
    error_page 401 = @login_err_redirect;
}

I'm maintaining a branch that had this simplification (around 130 less lines (~25%) for the module.c file, and no more gotos (#13), diff here) and can make a PR if you are interested.

[kevinmichaelchen]

looks good.
since error_page is a directive that can be used in http, server, or location, let's plan on doing

# for api
error_page 401

# for ui
error_page 302

[fitzyjoe]

@Maxx-T I looked at your branch and I like the elimination of all of that code. Can you make a pull request?

[max-lt]

Yes, it sould not take long.

[max-lt]

Done #42 🙂

[fitzyjoe]

Thanks @max-lt It only took me 2 years to merge your commit - sorry. This was an awesome commit because it did away with so much of the C code.

[fitzyjoe]

Ok... after merging the commit I had to revert it, @max-lt . I ran into trouble with the redirect:

location @login_err_redirect {
return 302 https://teslagov.com?redirect=$request_uri;
}

request_uri is not urlencoded. So, if the original request was for /secure/index.htm?foo=bar&hello=world it would return a 302 for this location:

https://teslagov.com?redirect=/secure/index.htm?foo=bar&hello=world

This won't work because the ? and & need to be URL encoded. I couldn't find any way to urlencode in the nginx.conf.

[max-lt]

Can this help ? https://stackoverflow.com/a/31269010/4111143

[JoshMcCullough]

I believe that example is simply stripping the path off the URL and passing it through to the proxy. The issue we are seeing is, for example, in the context of a login redirect.

In short, we need to encode the entire original URL, and append it to the login URL so we can send the user back to that URL after they login.

Example

1. user tries to go to some protected URL without having logged in:

https://example.com/admin/users?status=active&username=test%20user

2. NGINX Auth JWT module notices missing JWT
3. login redirect URL is made up of the URL to the login page, and then the originally requested URL as a URL parameter so we can send the user back to the URL they were trying to access after login -- NGINX responds with 302 and the URL ...

https://example.com/login?return_url=https://example.com/admin/users?status=active&username=test user

4. when the login page pulls the return_url parameter out of the querystring, it is only going to get the portion up to the first ampersand, so it will redirect to https://example.com/admin/users?status=active, because the ampersand was not encoded as a URI component -- the expected URL that NGINX responds with would look like ...

https://example.com/login?return_url=https%3A%2F%2Fexample.com%2Fadmin%2Fusers%3Fstatus%3Dactive%26username%3Dtest%20user

@fitzyjoe found a way to do this by using Javascript within the NGINX config (calling out to a script and using encodeURIComponent to encode the original URL. But we don't like this solution as it seemed questionable to mix JS with NGINX config -- we want it to be fast and not prone to breakage.

---

It would be great if we could accept your changes but also perhaps expose a function in the module which would allow us to encode some value from NGINX. We could call it like this ...

set $original_url = $scheme://$host:$port$request_uri; # build full redirect URL
auth_jwt encode_uri_component $encoded_original_url; # don't know the syntax

return 302 https://example.com/login?return_url=$encoded_original_url;

We somehow feel better about doing this in C than JS (speed?).

[JoshMcCullough]

@max-lt looks like we can use ngx_escape_uri to do this. And we already do use it, so we should have realized that.

[JoshMcCullough]

BTW @max-lt, reading over this issue again and IMO, your proposed solution is a bit more wordy than what we have today. Since you would generally only set auth_jwt_redirect and auth_jwt_loginurl once, e.g. at the http or server level, and then within your locations you would rarely set these directives. So a more realistic example would be:

http {
    # ...standard stuff
    
    server {
        listen 80;
        server_name example.com;

        # auth JWT config
        auth_jwt_enabled on;
        auth_jwt_key 'abcdef123456...';
        auth_jwt_loginurl 'https://example.com/login';
        auth_jwt_redirect on;

        # no auth JWT for login
        location /login {
            auth_jwt_enabled off;
            try_files index.html =404;
        }

        # secured locations...
        location /secure/a {
            # ... standard stuff, no auth JWT directives needed for this to be secure
        }

        location /secure/b {
            # ... standard stuff, no auth JWT directives needed for this to be secure
        }

        # ... more secured locations

        location /secure/z {
            # ... standard stuff, no auth JWT directives needed for this to be secure
        }
    }
}

You could also use nested locations to consolidate your auth JWT directives, if necessary.

---

So, while I like the idea of getting rid of all of that C code, I believe allowing this module to handle the redirect on its own makes the API easier and less verbose, and also keeps it all in one place.

[JoshMcCullough]

Closing this for now but we can revisit if needed.

[lukepalmer]

I came across this and it was really helpful.
It's particularly useful because after auth I can continue with the original request using $request_uri instead of decoding the urlencoded parameter.

@JoshMcCullough if you're ok with it, I could submit a PR to add to docs. Let me know.

[JoshMcCullough]

@lukepalmer please feel free to submit a PR to update the docs. Thanks!