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

Add possibility for 'user-auth' authorization #812

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Nanowires
Copy link

Another PR for #601
Mostly copied from #602

Same as the original author: I'm not particularly familiar with go, or the ntfy code base, I haven't tried to run this code.

If I have time I will try to add tests in the next few days.

I'm also not sure about the way to trace here (e.g. in case that the user-header is set and the client sent one, but the user is not found)

@binwiederhier
Copy link
Owner

This looks great, thank you. Given that it is an auth change, I will test this thoroughly myself, but I would highly appreciate some unit tests around this. Check out server_test.go and look for TestServer_Auth_* tests. You can probably model them after that.

@Nanowires
Copy link
Author

Given that it is an auth change, I will test this thoroughly myself

I would expect nothing less ;-)

I would highly appreciate some unit tests around this. Check out server_test.go and look for TestServer_Auth_* tests. You can probably model them after that.

I will definitely give this a try, but as I said, this will at least take some days.

Maybe one question about performance:
Does it make sense, to do this new authentication first? As this is (in my opinion) only a rare corner case (< 1% I would think), but we would still check this path first every time.
On the other hand, if the config value isn't set, this is only one additional check against a value cached in memory...

server/server.go Show resolved Hide resolved
# Otherwise the authentication can be bypassed by a crafted header.
#
# user-header: x-forwarded-user

Copy link
Owner

Choose a reason for hiding this comment

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

I think this header should only be allowed if behind-proxy: true is set.

server/server.go Outdated

// extractUserHader pulls the username of an already authenticated user from the configured header
func extractUserHeader(r *http.Request, h string) (username string) {
return readParam(r, h)
Copy link
Owner

Choose a reason for hiding this comment

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

Took me a while, but this is a giant security flaw, becausereadParam also accepts query params and not just headers. So this must be just r.Header.Get()

@binwiederhier
Copy link
Owner

I refined your code in #816. Note that your commit are not in there, because the ntfy binary (30MB) would be in the history. If you'd like your commit to be in there, feel free to force-push an updated branch, or open a new PR. You and the original author will get credit in the changelog either way though.

Please review #816 to see if this would work for you. I think it's a nice addition.

Question: What auth system are you using?

@binwiederhier
Copy link
Owner

Does it make sense, to do this new authentication first? As this is (in my opinion) only a rare corner case (< 1% I would think), but we would still check this path first every time. On the other hand, if the config value isn't set, this is only one additional check against a value cached in memory...

My code will only check the value if the config value is set. It's a single if statement. It'll be fine.

@Nanowires
Copy link
Author

Question: What auth system are you using?

I would go for a client certificate, as the number of clients is very small (just me ;-P)

@binwiederhier
Copy link
Owner

Fair warning: The ntfy Android client does not work with custom certs though.

@binwiederhier
Copy link
Owner

@Nanowires I am a little confused now. I thought that this would enable the use of Authelia or Keycloak. Am I wrong?

@Nanowires
Copy link
Author

As far as I understand, this allows any kind of authentication, as long as the proxy sends the user defined header with the associated user.

@wunter8
Copy link
Contributor

wunter8 commented Nov 3, 2023

I tested this with Authelia and Caddy tonight. Here's the config:

Caddy

ntfy-test.domain.com {
    forward_auth localhost:8010 {
        uri /api/verify?rd=https://auth.domain.com
        copy_headers Remote-User
    }

    reverse_proxy 127.0.0.1:8093
}

ntfy

base-url: https://ntfy-test.domain.com
listen-http: :8093
behind-proxy: true
auth-user-header: Remote-User
auth-file: ./user.db
auth-default-access: deny

Here are some things I found:

  1. if a person is logged into Authelia and doesn't have a corresponding ntfy account with the same username, the ntfy web app will fail immediately. it will return a 401 unauthorized for every request (including static assets). is this expected?
  2. subscribing and publishing using the web app to restricted topics worked after logging in with Authelia
  3. i tried it with enable-login: true, and I wasn't automatically signed in, meaning the "Sign In" button was still visible in the top right corner, clicking on it took me to the login form page, none of my account topics automatically loaded
  4. filling out the login form with my ntfy credentials and logging in that way did load/sync my topics
  5. websocket connections seemed to work fine in the background of the webapp, even going through Authelia
  6. the user I'm logged in as doesn't show up under Settings > Manage Users

There might be more issues, but for at least these reasons, I don't think this is ready to merge right away.

I did not test anything from the Android app. I'm not even sure how I could sign into Authelia from the ntfy app...

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.

None yet

3 participants