[WIP] feat: Implement Forward Authentication

[mdbell]

I've seen there's been a couple different attempts wrt implementing forward auth (#2189 and #1109 ) and this is my attempt. I've yet to test it out on mobile, and I imagine it's broken there. I have done some basic testing with Authentik + nginx locally and it seems to be working though.

I'd like to have it where the header is user-specified instead of hardcoding X-Forwarded-User. And maybe also require a magic password header as an alternative to IP based validation,similar to how actual budget implements it.

Other things to consider:

• Do we want to allow the root user to login using this method
• We probably want some way to indicate to the user that misconfiguring this can potentially expose your server to the entire internet. Perhaps require an explicit ENV var to be set to be able to use it

Screenshot of the settings right now (An IP address can be specficed with or without a CIDR. If omitted the program will treat it as a /0, or exact match):

[advplyr]

The problem is the mobile apps. @Sapd is the contributor with the most auth experience and broke it down in detail here #2189 (comment)

I haven't gone through your implementation yet I just want to highlight the previous issue if you have thoughts

[Sapd]

Yeah mobile apps can and will never work with forward auth - for technical reasons.
Only exception would be when we would allow to supply an additional header as requested here: advplyr/audiobookshelf-app#254
The user could specify a secret as a header, which would (when the reverse proxy is configured correctly) skip forward auth and allow at least proceeding to normal local auth or OIDC. (or if the reverse proxy supports it, a special secret per user, skipping auth altogether).

I think however that the feature makes sense to add anyway - if the admin wants higher security and does not require the apps. But maybe it should include a warning above IP pattern, like "Forward Authentication will not work with mobile Apps".

[Sapd]

I did not check the Regex, but in any case that must be /32, which will be exactly one address, instead of /0.

[mdbell]

Ah you're right, I'll fix this tonight.

[Sapd]

Also I noticed that your CIDR checks only supports IPv6 localhost address ::1

I think its better to directly use a package for the IP handling. Especially as your code would need thorough testing for correct parsing for security reasons. A package would also make the code smaller. https://github.com/indutny/node-ip - https://www.npmjs.com/package/ip seems to be the most popular. It also fully supports IPv6 and you could / should even use the ip.isPrivate function for additional security.

[mdbell]

@Sapd

Also I noticed that your CIDR checks only supports IPv6 localhost address ::1
Yeah that was me just hardcoding a 'fix' in, I was moreso doing a quick-n-dirty implementation to get something working in the first place and then handle ipv6.

I agree it makes more sense to use a library then rolling my own (especially since I had the CIDR mask wrong 😅). I'll switch to that package tonight, as well as adding a warning about forward auth being incompatible with the mobile app. Plus it appears the ip library is already in the project, as a transitive dependency for another package.

As a side note: I don't have much experience in the JS/TS world, aside from some personal projects and a little exposure at work; I also noticed that there appear to be a number of libraries in the repo, as well as some defined within the package.json. Would you or @advplyr be able to explain the rational for them being in the repo? Wouldn't that be a security risk itself, as it'll prevent tools like npm audit from functioning?

Edit:

In this screenshot I provided the input "fc00::" and it auto appended the /128 before saving

Same thing happens with ipv4, if you omit the mask it'll append /32

[mdbell]

And here's the warning and info labels:

[advplyr]

I went through the dependencies 2 years ago and saw how many of them are unnecessary or outdated or were being included multiple times by other packages requiring different versions. I was working on removing as many dependencies as I could by pulling them out and modifying them (removing lodash for example).

The project was a lot smaller then and some of those libs I will probably put back in the package.json after review.

Dependencies are a real issue with the node/js ecosystem in general. I found many cases where a dependency was using another dependency but only about 1% of it. So we are pulling in a large package with potentially many other dependencies just to use a single feature of it. Many of the nested dependencies weren't being used at all for our use-case.

Sometimes a popular npm package will get hijacked or the author decides to release a version with malicious code. When we update dependencies I try to look through it first but when we have hundreds or thousands of dependencies this becomes an impossible task. It's even worse when we are bundling the nested dependency, not even using it, and still on the hook for malicious code.

I wouldn't do it the same way again but it did remove a lot of useless code we were bundling. Now I'm just extra picky on adding new dependencies and still hope to clean out some of those in the libs folder.

[advplyr]

I came across this article that reminded me of the comment I made above about dependencies. The issues I was pointing out with the node package ecosystem is better described in this article.
https://bvisness.me/microlibraries/

[mdbell]

@advplyr thanks for the link! I've also been dealing with the mess that is node packages at work the last few days... I get it now.

Back on topic of this PR, any more feedback or suggestions?