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

HTTP 401 responses causing issues with Basic Authentication #2983

Closed
makmic opened this issue Mar 24, 2020 · 12 comments
Closed

HTTP 401 responses causing issues with Basic Authentication #2983

makmic opened this issue Mar 24, 2020 · 12 comments
Assignees
Milestone

Comments

@makmic
Copy link

makmic commented Mar 24, 2020

Infos:

  • Used Zammad version: 3.2.0
  • Installation method (source, package, ..): From source using Ruby 2.5.5, behind nginx rproxy.
  • Operating system: Ubuntu 18.04.4 LTS (bionic)
  • Database + version: postgres 9.5.21
  • Elasticsearch version: 5.6.16
  • Browser + version: Google Chrome 80.0.3987.132

Expected behavior:

Zammad integrates smoothly with a layer of basic authentication. Therefore, the status code "HTTP 401 Unauthorized" is never used. As an alternative, status code 403 would be a proper replacement.

Actual behavior:

Overall, Zammad has not many issues when combined with basic auth. But there a few cases, where a request is answered with a status code 401 and the current user is forced to re-enter his basic auth credentials.

The codebase can be searched easily for status 401 (or unauthorized):
https://github.com/zammad/zammad/search?l=Ruby&q=%3Aunauthorized

Steps to reproduce the behavior:

  • Set up a Zammad instance and put it behind basic authentication

THEN

  • Try to log in with wrong credentials
  • The application renders a page with status code 401 and forces you to re-enter the basic authentication credentials

OR

  • Create a ticket assigned to a group where person 1 has access
  • Person 1 interacts with that ticket
  • Move that ticket to another group which is not accessible for person 1
  • Person 1 will still see a 401 request to the ticket above on his Zammad overview, which will log him out of basic auth

Yes I'm sure this is a bug and no feature request or a general question.

@MrGeneration
Copy link
Member

Please update your first article to keep your exact installation information - "any" is not really suitable at this point - sorry. :)

Also, please do provide your complete webserver configuration (and let us know which one you're using). Right now it smells a bit like a technical question, but I'd like to make sure completely. However, for that I need everything. ;)

Thanks.

@makmic
Copy link
Author

makmic commented Mar 24, 2020

Hi again,
I updated the issue description - sorry for the initial lack of it!
Best

@MrGeneration
Copy link
Member

Thanks for the update. The config of your webserver is our default configuration extended by basic auth? Would you mind providing that vhost config as well? Just to be sure I'm not missing something.

Thanks!

@makmic
Copy link
Author

makmic commented Mar 24, 2020

Yes, it's basically just Zammad Default config + Basic Auth. Here is the vhost config:

auth_basic 'Restricted: general basic auth';
auth_basic_user_file /etc/.htpasswd.d/zammad;
location /ws {
    proxy_pass  http://zammad_ws;
    proxy_redirect  off;
    proxy_hide_header X-Powered-By;
    proxy_set_header  Host              $host;
    proxy_set_header  X-Forwarded-For   $proxy_add_x_forwarded_for;
    proxy_set_header  X-Forwarded-Proto $scheme;
    proxy_set_header  X-Real-IP         $remote_addr;
    proxy_set_header  X-Forwarded-Host  $host;
    proxy_set_header  X-Forwarded-Port  443;
    proxy_set_header CLIENT_IP $remote_addr;
    proxy_read_timeout 86400;
    proxy_http_version 1.1;
    proxy_set_header  Upgrade           $http_upgrade;
    proxy_set_header  Connection        "upgrade";
    proxy_max_temp_file_size  0;
    error_page 502 503 504 =503 @fallback;
}
location / { 
    try_files $uri @proxy;
}
location @proxy { 
    proxy_pass  http://zammad;
    proxy_redirect  off;
    proxy_hide_header X-Powered-By;
    proxy_set_header  Host              $host;
    proxy_set_header  X-Forwarded-For   $proxy_add_x_forwarded_for;
    proxy_set_header  X-Forwarded-Proto https;
    proxy_set_header  X-Real-IP         $remote_addr;
    proxy_set_header  X-Forwarded-Host  $host;
    proxy_set_header  X-Forwarded-Port  $server_port;
    proxy_http_version 1.1;
    proxy_set_header  Upgrade           $http_upgrade;
    proxy_set_header  Connection        "upgrade";
    proxy_max_temp_file_size  0;
    proxy_set_header CLIENT_IP $remote_addr;
    error_page 502 503 504 =503 @fallback;
}

@thomase
Copy link

thomase commented Mar 27, 2020

We looked into that again.
Our guess (as Michael wrote) would be to not return a HTTP 401 (which makes the browser believe that the given basic auth credentials are incorrect) but to return HTTP 403 forbidden.

If I saw it correctly, this would mean

app/controllers/application_controller/handles_errors.rb#L39
respond_to_exception(e, :unauthorized)

should be replaced with
respond_to_exception(e, :forbidden)

RFC says browsers should behave differently (https://tools.ietf.org/html/rfc7231#section-6.5.3): "If authentication credentials were provided in the request, the server considers them insufficient to grant access. The client SHOULD NOT automatically repeat the request with the same credentials. "

Though, we had the same issue in another project last week and 403 works.
If you do not see other problems returning a 403 we can issue a PR.

Best

@MrGeneration
Copy link
Member

Sorry for taking so long.
Please note that this is not a Zammad related issue (application based), but a "backend issue" on your nginx.

The authentication requests for the basic authentication does never reach Zammad as application but already ends (and is checked by) your nginx or any other webserver you'd use.

Thus, changing source code is in my opinion not solving your issue at all.
Technically a 401 unauthorized is correct from what I can tell (even though I understand you'd want a 403).

See also:
https://serverfault.com/questions/616770/nginx-auth-basic-401-htpasswd

By the way:
Just to double check I completely commented out the Zammad proxy parts to ensure the backend of Zammad can't reply. The result with 401 is the same and thus fault of nginx. :)

Closing as this is not a bug but a technical question.

@makmic
Copy link
Author

makmic commented Apr 21, 2020

Hi @MrGeneration,
thanks for looking into this.

While I understand that this issue seems to be out-of-scope for the Zammad application, I still think that it is caused by it (and not the Ngnix). I'll try to explain why:

Let's assume our ngnix is not configured to use basic authentication. In this case, both of us agree that the Zammad application ("behind" the ngnix) just works fine.

Now, we enable basic auth by adding the configuration stated above to our ngnix. Please note that even now, almost everything still works as expected - including both authentications (basic and Zammad login) and the Zammad application itself.

The issue I tried to describe earlier on is caused sometimes later (by Zammad!) when the application renders a page with status code 401. In this case, any webserver with enabled basic authentication is forced to log you out.

I agree that semantically speaking 401 sounds "just right" in this case. Technically speaking, it causes inevitable issues with basic authentication and should be replaced with 403.
Please re-open this issue as it might also affect the UX of the Zammad application for many users.

@MrGeneration
Copy link
Member

@thorsteneckel what's your opinion on this?

@thorsteneckel
Copy link
Contributor

thorsteneckel commented Apr 21, 2020

Hey guys! Thanks for the valuable information and descriptions. I read the RFC and was still a bit confused about the differences between 401 and 403. However, I found this great explanation over at StackOverflow. Quote:

There's a problem with 401 Unauthorized, the HTTP status code for authentication errors. And that’s just it: it’s for authentication, not authorization.

That brings it to the point. Zammad uses 401 for authorization errors. This is technically wrong and therefore a bug. I'll reopen the issue.

However, we need to check the impact. I assume this is a breaking change because of all the implementations and API consumers out there.
My current plan is to soft deprecate 401 authorization with Zammad 3.4, hard deprecate it with 3.5 (or 3.6) and switch to 403.
We need to discuss this further internally.

Further thoughts on this anyone?

@makmic
Copy link
Author

makmic commented Apr 21, 2020

Those are great news! Sounds good to me.

@thorsteneckel thorsteneckel self-assigned this Jul 13, 2020
@thorsteneckel thorsteneckel added this to the 3.5.0 milestone Jul 13, 2020
@thorsteneckel thorsteneckel added this to To do in OLD Workflow Jul 30, 2020
@thorsteneckel thorsteneckel modified the milestones: 3.5.0, 3.6.0 Sep 7, 2020
@thorsteneckel thorsteneckel modified the milestones: 3.6.0, 3.7.0 Nov 6, 2020
@thorsteneckel
Copy link
Contributor

thorsteneckel commented Jan 22, 2021

To keep you in the loop: We will implement this with the upcoming 4.0 release.

For internal implementation purposes: https://thoughtbot.com/blog/forbidden-kisses-http-fluency-in-clearance

@MrGeneration MrGeneration moved this from To do to In progress in OLD Workflow Jan 29, 2021
@thorsteneckel thorsteneckel moved this from In progress to Done in OLD Workflow Feb 4, 2021
@thorsteneckel
Copy link
Contributor

Fixed with the commit above. We took the chance and improved some of the messages of 401 errors but basically the only thing we changed were the non-authentication errors to 403 Forbidden.

You can test this with the latest develop package in about 30 minutes from now. Please be aware this is a working branch and not stable. Therefore ensure to have a backup and expect the worst :) Looking forward to hear your feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
OLD Workflow
  
Done
Development

No branches or pull requests

4 participants