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

Added Header Authentication - Server Part #312

Merged
merged 26 commits into from
May 4, 2024

Conversation

joewashear007
Copy link
Contributor

@joewashear007 joewashear007 commented Feb 15, 2024

This is my attempt at an implementation of header authentication as referenced in issue actualbudget/actual#1092

Looking at how actual passwords work, the front end simply sends a password, so my take was to have my SSO proxy set a X-ACTUAL-PASSWORD header that has the value of the my Actual password. To enable this, I add a new env ACTUAL_LOGIN_METHOD which should have the value of "header".

I updated the the /account/login endpoint to check this env and pass along the header value to existing login logic.

I also added another property to the /account/needs-bootstrap that returns the property of the loginMethod. I am working on a second PR (actualbudget/actual#2362) that will adjust the login to automatically the call the /account/login endpoint.

Please let me know your thought and recommendations how I can improve and test this.

Joseph

Copy link
Contributor

@twk3 twk3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joewashear007 can you also look at adding a trusted proxy header auth config option where we can specify cidrs that are allowed to perform header auth? That should help make this a bit more palatable for folks who are security conscious while we still haven't added multi-token support (which would be a first step in allowing us to support better mechanisms like jwts for this).

You should be able to verify the request against the trusted ips using https://github.com/jshttp/proxy-addr#readme which is already available in the project.

@joewashear007
Copy link
Contributor Author

I think that should be do able. I hope to get to this this week,

@joewashear007
Copy link
Contributor Author

I am still wanting to work on this, I just haven't gotten the free time to make it happen.

@joewashear007
Copy link
Contributor Author

@twk3 I was looking at this some more tonight and I want to make I am thinking about this correctly.

I am assumeing that we want to make a config option that will be for the cidr for auth server. If this is set, when we do an authenticate request, we should check if the sender to see if it is in the range of the cidr we expected, then pass or fail based on that.

I am not sure how to best use proxy-addr for this. If we pass in the config option to the proxy-addr then it will spit out the first record of an ip that is not in that list. If that is the case, I am not sure what we do it with it.

I see that ipaddr is also included, and the subnetMatch function looks more helpful. But I want to make sure I am not missing something

@twk3
Copy link
Contributor

twk3 commented Mar 25, 2024

@joewashear007 sorry, you're right. proxy-addr isn't all that useful here.

Taking another look, I think you are on the right path, but we can probably just use req.ip and the ipaddr parsing.

For reference this is what is done for authetik and this home assistance addon (python so the code doesn't apply here, but I mean in terms of what we are trying to enable) https://github.com/BeryJu/hass-auth-header

@joewashear007
Copy link
Contributor Author

@twk3 I think have gotten this part working. I was able to test multiple cases on my local machine and I think it working nicely:

  • Password login work
  • header login works
  • header with invalid password -> redirect to password login
  • invalid header or bad proxy trust -> redirect to password login

Copy link
Contributor

@twk3 twk3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! I still need to try it out with the client side and review it holistically. In the meantime I've left some minor feedback here.

src/load-config.js Outdated Show resolved Hide resolved
upcoming-release-notes/312.md Outdated Show resolved Hide resolved
@joewashear007 joewashear007 requested a review from twk3 March 28, 2024 16:18
@kyrias
Copy link
Contributor

kyrias commented Apr 2, 2024

can you also look at adding a trusted proxy header auth config option where we can specify cidrs that are allowed to perform header auth? That should help make this a bit more palatable for folks who are security conscious while we still haven't added multi-token support (which would be a first step in allowing us to support better mechanisms like jwts for this).

It seems rather strange to me to do this for this use-case. Normally you would have whitelisting for trusted proxies when you're actually relying on the proxy to provide correct information, such as if the proxy gives you the name of a pre-authenticated user or to pass in the original client IP as otherwise it might be possible for a malicious client to lie about either of those things, but in this case we're just shifting the exact same information to be sent in a custom header rather than in the Authorization header in the payload. There is no proxy-provided information that is assumed to be correct or valid, and any client that can provide one can provide the other.

But for the same reason I'm sort of skeptical over implementing it like this at all, because if we're going to be passing in the same password either way then I don't see any advantage in implementing this instead of having your proxy set the correct Authorization header?

@kyrias
Copy link
Contributor

kyrias commented Apr 2, 2024

Oh sorry, I just realized that I completely forgot how the authentication already works, d'oh.

But well, if we're going to do it this way then I still think that both 1) it would make more sense to use the Authorization header than a custom one, and 2) it makes no sense to have this gated by a trusted proxy configuration option, since the value is still authenticated the same way no matter how it's passed in.

@joewashear007
Copy link
Contributor Author

joewashear007 commented Apr 2, 2024

@kyrias thanks for the feedback. I totally agree that would be nice to use the Authorization header but when it came to designing a solution I wasn't sure how to make it work without a lot of design/code change. I thought that using the existing login with supplying the password through a different channel was a simple low change solution. Do you have any good designs ideal that would make use of the Authorization header?

@kyrias
Copy link
Contributor

kyrias commented Apr 2, 2024

@joewashear007 I'm confused as to what you mean. What would be the problem with supplying the same password through the Authorization header?

@joewashear007
Copy link
Contributor Author

My understanding is that the Authorization header was usually Basic Auth (user/password), Bearer token, Oauth token, etc. None of those seemed to quite fix properly and I don't want to overload a standard header that could be used in the future. It standard practice for many API to do authentication with a custom header so I thought it first the best while leaving the path clear for more robust authentication methods in the future

@kyrias
Copy link
Contributor

kyrias commented Apr 2, 2024

Using the Basic scheme with an empty user-id would be the standard way to do this, and if a full user system is implemented in the future it's easy enough to distinguish the authentication type by the presence or absence of a user-id.

@joewashear007
Copy link
Contributor Author

I guess that is valid. I know Azure DevOps does that with their API, I just thought it was a weird way to do it. My implementation would allow that to be a third option pretty easily.

@twk3
Copy link
Contributor

twk3 commented Apr 3, 2024

There is no proxy-provided information that is assumed to be correct or valid, and any client that can provide one can provide the other.

That is a good point. I asked for it to be implemented more as a gating mechanism for controlling how widely header auth is accepted. Not realising/checking that login wasn't already enforcing csrf tokens (so it's not really widening exposure atm). I still think having them it's ideal. But it shouldn't have been required at this stage.

src/util/validate-user.js Outdated Show resolved Hide resolved
@joewashear007
Copy link
Contributor Author

Sounds good, I think I might have mixed that up trying to get the TS compile to work.

@joewashear007 joewashear007 requested a review from twk3 April 8, 2024 13:49
dependabot bot and others added 9 commits April 22, 2024 15:14
Bumps [tar](https://github.com/isaacs/node-tar) from 6.1.11 to 6.2.1.
- [Release notes](https://github.com/isaacs/node-tar/releases)
- [Changelog](https://github.com/isaacs/node-tar/blob/main/CHANGELOG.md)
- [Commits](isaacs/node-tar@v6.1.11...v6.2.1)

---
updated-dependencies:
- dependency-name: tar
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…albudget#334)

* Allow setting accessValidForDays per bank

This also stops taking `accessValidForDays` from the client since it's
hardcoded there anyway and it's simpler to just have these per-bank
values in one place.

Signed-off-by: Johannes Löthberg <[email protected]>

* Get the max allowed maxHistoricalDays value from the GoCardless API

Signed-off-by: Johannes Löthberg <[email protected]>

* Upgrade nordigen-node to 1.4.0

Contrary to the claims in the nordigen-node changelog 1.3.0 did *not*
fix the missing support for passing in accessValidForDays, so we have to
upgrade to 1.4.0 to actually get the fix.

Signed-off-by: Johannes Löthberg <[email protected]>

---------

Signed-off-by: Johannes Löthberg <[email protected]>
…BABEBB (actualbudget#345)

* Add backup date field for GoCardless transactions with bank BNP_BE_GEBABEBB

* release note
Copy link
Contributor

@twk3 twk3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @joewashear007

Merging the server side now

Would really appreciate it if you could start preparing a documentation PR regarding this for our June release.

@twk3 twk3 merged commit 3a486ed into actualbudget:master May 4, 2024
6 checks passed
MMichotte pushed a commit to MMichotte/actual-server that referenced this pull request Sep 9, 2024
* Added loginMethod config option

Added a new config option loginMethod that can be password (default) or header for header authentication. This value is returned to the client to be used on the front end


---------

Signed-off-by: dependabot[bot] <[email protected]>
Signed-off-by: Johannes Löthberg <[email protected]>
Co-authored-by: DJ Mountney <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Johannes Löthberg <[email protected]>
Co-authored-by: Matt Fiddaman <[email protected]>
Co-authored-by: DJ Mountney <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants