Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NJS Access Filter #474

Open
lancedockins opened this issue Feb 20, 2022 · 22 comments
Open

NJS Access Filter #474

lancedockins opened this issue Feb 20, 2022 · 22 comments

Comments

@lancedockins
Copy link

Are there any plans to add a js_access_filter? We currently use Lua to control some aspects of access to different locations. While you can sort of achieve this with either js_set or other existing NJS directives, the only way to do this for access filters that also require access to the request body is with js_content. So if you need to filter access based on the request body, you basically have to use js_content. While that does seem to work, right now it appears that the only way to do this for a location that is also passed to FastCGI is to use an internal redirect to a named location. That's OK as a workaround but it does complicate the use of location specific filters for things like js_header_filter or js_body_filter because it redirects the entire request internally to a global named location. A js_access_filter directive that has access to the request body would solve this (just like it currently does with Lua and OpenResty's integration with Nginx)

@crasyangel
Copy link

What is access_filter? access phase or header_filter?

@lancedockins
Copy link
Author

I'm referring to the access phase and filtering that (to change the allow/deny/status type of response). There's already a header filter in NJS so obviously there is no need for another one for that.

@crasyangel
Copy link

I'm referring to the access phase and filtering that (to change the allow/deny/status type of response). There's already a header filter in NJS so obviously there is no need for another one for that.

How access phase filter? filter is not the place where make response

@lancedockins
Copy link
Author

I don't think that you are following what I'm talking about at all. Nginx has an access phase. That phase happens before the content phase where the response is set. That phase is specifically related to request authorization. What I'm talking about is tapping into that phase so that the determination of whether the user can proceed or not is done programmatically with JS instead of a simple allow/deny in the location block. If you need more information about that phase, see the Nginx docs.
https://nginx.org/en/docs/dev/development_guide.html

The concept that I'm describing is not remotely new. It's been in Lua's Nginx extension logic for years.
https://github.com/openresty/lua-nginx-module#access_by_lua_blockgg

It wouldn't be exactly the same with NJS. But it's the same concept. There are legitimate reasons to do this. They're not about setting the content of the response. They are about allowing/denying the entire request.

@crasyangel
Copy link

I don't think that you are following what I'm talking about at all. Nginx has an access phase. That phase happens before the content phase where the response is set. That phase is specifically related to request authorization. What I'm talking about is tapping into that phase so that the determination of whether the user can proceed or not is done programmatically with JS instead of a simple allow/deny in the location block. If you need more information about that phase, see the Nginx docs. https://nginx.org/en/docs/dev/development_guide.html

The concept that I'm describing is not remotely new. It's been in Lua's Nginx extension logic for years. https://github.com/openresty/lua-nginx-module#access_by_lua_blockgg

It wouldn't be exactly the same with NJS. But it's the same concept. There are legitimate reasons to do this. They're not about setting the content of the response. They are about allowing/denying the entire request.

Yes, I understood. But the nginx access phase is totally different from header_filter or body_filter. The 'access_filter' is weird that doesn't make any sense at all

@drsm
Copy link
Contributor

drsm commented Feb 21, 2022

@crasyangel

the access_by_lua_block runs after the nginx access phase, so unaffected by satisfy any|all

@lancedockins

why not to use auth_request to js_content ?

@xeioex
Copy link
Contributor

xeioex commented Feb 21, 2022

@lancedockins

The standard way to implement the logic of access filter is to use /auth_request with js_content.

Take a look at following examples.

@lancedockins
Copy link
Author

lancedockins commented Feb 21, 2022

@drsm and @xeioex Thank you for that context. I had a suspicion that that filter might run technically after the access phase (e.g. post access, pre content). It said that it was part of the "tail" of the access phase.

How would that work in the context of a FastCGI backend? All of the examples demonstrate it in the context of a proxy backend. Would you just create an internal location, set the js_content filter of the internal location to use the sort of logic that I'm describing, set an auth_request to the internal location from the primary location, and then configure the rest of the primary location to just fastcgi_pass directly?

e.g.

`/primary-location {
auth_request /access-filter;

fastcgi_pass unix:/path/to/sock;

}

/access-filter {
internal;

js_content http/access.js;

}

access.js

// Logic to parse request body and approve/deny the request?`

Also, in terms of implementation, isn't that a lot more cumbersome than just having an access filter? And if the intended response was a 444 rather than 401 or 403, wouldn't that either be impossible or require passing back content to the auth_request_var that you would use to deny the request with 444 in the primary location?

@xeioex
Copy link
Contributor

xeioex commented Feb 21, 2022

@lancedockins

A js_access_filter directive that has access to the request body would solve this

there is also an issue with having request body in access phase. By nginx design, access phase is a relatively early phase where body is not read yet (because the body can be quite large, this way nginx avoids DOS attacks).

@lancedockins
Copy link
Author

That makes sense. But I think that the presence of the auth_request directive sort of illustrates the value here.

That may also explain why the OpenResty version requires you to specifically retrieve the request body in any extension code - perhaps some under the hood internal magic when you do that.

Regardless, though, I don't want to get hung up on the phase or name of this. The concept is obviously valuable or there wouldn't be an auth_request directive. Sure, it runs later in the processing phase than the "access" phase specifically due to the DOS side of things but if it's still running before the content phase then you're actually getting a happy middle ground where you have the DOS protection from not retrieving the request body in earlier phases and the benefit of having access to the request body for additional auth/filtration before the content phase (but after the access phase).

It could be something that's just named differently like js_auth_filter instead and would serve as a simpler way to do the same sort of thing that we're talking about with auth_request.

@lcrilly
Copy link

lcrilly commented Feb 21, 2022

@lancedockins
Copy link
Author

@lcrilly Yep. That is very helpful. That's another example of the exact type of functionality that I'm looking for.

Question about that one:
Is the r.variables.upstream variable just a made up variable name or named upstream group? Or is that a first party Nginx variable name? I didn't see that one specifically in the Nginx docs. I'm assuming that (either way), the implication here is that there is an upstream group defining the upstreams, that that would just be referenced like fastcgi_pass backend; in the location block, and that the NJS example here would either pass to that backend or not based off of the returned value for r.variables.upstream (in the part of the example that uses that).

That said, though, the fact that the workaround is this cumbersome sort of illustrates the point of my issue/request entirely. There is obviously a need for the sort of functionality that I'm talking about. Otherwise things like auth_request and your article wouldn't even be a thing.

I think a lot of people would much prefer to use something like NJS over Lua strictly because it's first party and JavaScript instead of Lua. I know I fit that category. But on this particular functionality need, OpenResty's Lua integration API is hands down, dominating over NJS's ways of achieving the same thing. With NJS, all available solutions are workarounds (more or less). With the OpenResty Lua API, that's a first party capability that goes right into the location block of interest without having to create a variety of placeholder/stub location blocks just to do request body handling. That's what I'm suggesting - that NJS adopt some sort of first party way to do this sort of thing as well. I'm not proposing that NJS adopt every one of their conventions or anything. Things like js_set are actually pretty creative. But hooking into an auth/pre-content phase

@lcrilly
Copy link

lcrilly commented Feb 21, 2022

$upstream is a user-defined variable to indicate the success target for a given location
https://gist.github.com/nginx-gists/6a8e7c65fdc41bc955f7e67a1d475469#file-warehouse_api_jsonbody-conf-L15

With FastCGI you'll need a fastcgi_pass target to handle the requests that fail your auth check. An error-handler to send a 401 response (or similar).

@lancedockins
Copy link
Author

lancedockins commented Feb 28, 2022

Thanks, @lcrilly.

For what it's worth, this method doesn't seem to work either. I attempted to use it and keep things very simple to test and the request body is still undefined at the variable's runtime. The only change that I made is that I used the variable like this:

if ($variablename = value) { // do something }

In your example, you used it as a variable that would set the upstream name. Essentially, it appears that the request body is only ever defined at js_content. I even forcibly set the mirror request body setting to require the request body and it still wouldn't work.

@xeioex auth_request also doesn't appear to work for this. The auth_request directive does pass the request to the internal route where you have js_content defined but it seems to be doing so as a GET request only. So the request body is not defined.

So far, the ONLY way that I've been able to get a request body in an NJS script is in the js_content directive. That would normally be fine but NJS also doesn't seem to allow subrequests to FastCGI upstreams. It only supports doing that for proxy upstreams. If you use the fastcgi_pass directive in the same location as js_content, js_content is skipped. So when mirror, auth_request, and js_set all provide no access to the request body, the only option left is js_content and r.internalRedirect to a single named location to handle the FastCGI component. That does work. But since it's forcibly changing the internal location, any other filters that you might want to apply (e.g. js_header_filter, js_body_filter) can only be applied on the internally redirected location block. Since that internal location block is really just a workaround version of an upstream, though, it sort of globalizes all filters even if you have different things that you want to do with the header/body filters on different input request paths that all go to the same upstream.

I still maintain that there needs to be an easier way to do something this simple (access the request body in NJS or pass a subrequest to a fastcgi backend). For better or worse, the OpenResty team figured out how to make it happen. I'd rather not use LuaJIT for this sort of thing if it's avoidable. But the overall sentiment here "seems" to be that NJS doesn't have a real taste for implementing this sort of support for easier access to the request body (e.g. like a js_ directive to require the request body or support for subrequests to a fastcgi location). Considering that there are first party blog articles on Nginx.com about doing this sort of request body access, I can't entirely wrap my head around why there wouldn't be a pretty strong case for adding this sort of support - particularly given the substantial hoops that have to be jumped to even document that in such articles.

@lcrilly
Copy link

lcrilly commented Mar 1, 2022

The only change that I made is that I used the variable like this:
if ($variablename = value) { // do something }

This won't work because if operates at the rewrite phase, which is quite early, and before the request body is read. The mirror module operates at the pre-content phase, just before fastcgi_pass (content phase).

So you cannot use if to make your auth decision. You must put the decision variable in the fastcgi_pass directive so that the evaluation of that variable happens after the mirror module has read the request body.

Here's a sample config based on my understanding of your needs.

# this njs function performs authentication by $request_body, returning:
# 1. "unix:/path/to/sock" (for auth success)
# 2. "127.0.0.1:11111" (for auth failure)
js_import conf.d/auth.js;
js_set $auth_result auth.inspectBody;

server {
    listen …;
    location / {
        mirror /_get_request_body; # force population of $request_body
        fastcgi_pass $auth_result; # this will trigger inspectBody()
    }

    location /_get_request_body {
        return 204;
    }
}

server {
    listen 11111;
    return 401;
}

@lancedockins
Copy link
Author

@lcrilly Thank you. I genuinely appreciate that you took the time to outline this option. I'll give it a try. Again... thank you.

This does seem to suggest that either some sort of control over which phase that js_set runs in or some sort of simpler way to get access to the request body may be in order.

OpenResty Solved it this way:
https://github.com/openresty/lua-nginx-module#lua_need_request_body

Technically it could probably be done like I describe in this issue too:
#475

And I'm sure that there are other solutions.

But I think some sort of way to get access to the request body reliably without bizarre workarounds is definitely in order.

@stefanwerfling
Copy link

stefanwerfling commented Dec 30, 2022

Hello @lancedockins , I was just reading through the discussion. Your implementation for the Auth is not yet entirely clear to me. But basically I don't think there is a problem. I generate the nginx config in my project, so each route (http/stream) has its ID with js_set. After that the normal module follows and creates an "auth_basic_user_file" which has an empty htpasswd. After that I can normally fall back to an auth_request. Here you can then call up your function for checking. At this point you could also give back another proxy pass or upstream. Here is my example which Allow or Deny returns, maybe it will help you to be able to create a functionality of your wish.

http {
	server {
		listen 10443 ssl http2;
.....

		location /auth3 {
			internal ;
			set $ff_auth_basic_url https://127.0.0.1:3000/njs/auth_basic;
			set $ff_location_id 3;
			set $ff_logging_level silly;
			set $ff_authheader $http_authorization;
			js_content mainhttp.authorizeHttp;
		}

		location / {
			satisfy any;
			auth_basic "Flyingfish Test";
			auth_basic_user_file /opt/app/nginx/htpasswd;
			auth_request /auth3;
			proxy_set_header Host $host;
			proxy_set_header X-Forwarded-Scheme $scheme;
			proxy_set_header X-Forwarded-Proto $scheme;
			proxy_set_header X-Forwarded-For $remote_addr;
			proxy_set_header X-Real-IP $remote_addr;
			proxy_pass http://192.168.2.1/;
		}
	}
}
async function authorizeHttp(s: NginxHTTPRequest) {
    const v = s.variables;

    if (!v.ff_authheader) {
        s.warn('authorize -> no authheader, send 401');
        s.return(401);
    } else if (v.ff_auth_basic_url) {
        let location_id = '0';

        if (v.ff_location_id) {
            location_id = v.ff_location_id;
        }

        const resulte = await ngx.fetch(v.ff_auth_basic_url, {
            body: '',
            headers: {
                'authheader': v.ff_authheader,
                'location_id': location_id
            },
            verify: false
        });

        s.warn(`authorize(fetch->status) -> ${resulte.status}`);

        if (resulte.status == 200) {
            s.return(200);
        } else {
            s.return(403);
        }
    } else {
        s.warn('authorize -> Auth Url not found!');
        s.return(500);
    }
}

export default {authorizeHttp};

@lancedockins
Copy link
Author

lancedockins commented Dec 30, 2022

Hi @stefanwerfling,

Thanks for sharing that. That's a bit closer to what I was trying to achieve than what I've been able to pull off so far. Unfortunately, though, the key detail for my situation is access to the POST body in a POST request. I'm not strictly authorizing the request based on variables that are otherwise present in a GET. The method that @lcrilly provided seemed promising to me but I wasn't able to get it to work for our particular need.

In the end, it wasn't ideal but I found a way to work around this issue by just using js_content directives that have access to the POST body and then selectively rewriting the request to the desired backend location block or denying the request depending on the situation.

Had to write a number of my own libraries to entirely pull it off since NJS doesn't seem to include logic to parse the body of the request but the logic is something like this:

// Post object is a parsed POST body in key/value format
if(typeof post.namedvariable == 'badvalue'){
    r.return(403);
    return;
}
else{
    r.internalRedirect('@namedlocation');
    return;		    
}

I'd use ngx.fetch but it doesn't seem to work for FastCGI backends and there doesn't appear to be any support (or at least no documentation) on how to connect to a defined FastCGI backend in Nginx from NJS. So I had to get creative to get things to work.

For those might stumble across this, though, I wanted to share an example that solved all of that. Admittedly, though, it would be better in my mind if there was at least native key/value parsing of POST body data in NJS natively and native support for Nginx's own FastCGI backend support in NJS.

Thank you again for taking the time to share, @stefanwerfling

@stefanwerfling
Copy link

Hi @lancedockins ,
you could put your FastCGI backends on a listen server with 127.0.0.1 and then use the fetch internally on the listen. Just crossed my mind.

What does your post actually look like? As a standard, the auth is made with the header as far as I can see. (just asking out of interest)

:)

@lancedockins
Copy link
Author

Thanks @stefanwerfling. We actually have to use a unix socket. That's the bit that NJS doesn't seem to support yet. I reported this in another bug report. It supports proxy and TCP but not sockets (even though Nginx itself does) - at least as far as I can find.

See - #476

And for this particular use case, we're not so much authorizing requests that hold to a proper format but hard failing requests that are known types of malicious activity. So you're right about the way that proper auth is done. But we're not really auth'ing the request so much as detecting particularly bad types of request and hard failing them. That forces us to use different methods and account for a wider variety of contexts.

@stefanwerfling
Copy link

Hi @lancedockins , I would be interested to know how you build your fortress. I have now implemented a black and white list in my project. Maybe I could do more with it. Is that also "Open Source" watch from you?

:)

@lancedockins
Copy link
Author

That particular project is actually an internal project. I'm not at liberty to share it without authorization from the company and given what it's for, I don't expect that it will be eligible for open source.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants