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

Docs: Describe surprising side effects of auth_param basic #1612

Conversation

rousskov
Copy link
Contributor

@rousskov rousskov commented Dec 5, 2023

acl badGuys proxy_auth Bob
http_access deny badGuys

Admins may be surprised that their proxy_auth ACLs do not match users
with logins identical to those listed as proxy_auth ACL values. For
example, a user logged in as "Bob" will no match the above ACL if Basic
authentication is used without an explicit "casesensitive on" setting.
In fact, the above ACL cannot match any user in that environment!

    acl badGuys proxy_auth Bob
    http_access deny badGuys

Admins may be surprised that their proxy_auth ACLs do not match users
with logins identical to those listed as proxy_auth ACL values. For
example, a user logged in as "Bob" will no match the above ACL if Basic
authentication is used without an explicit "casesensitive on" setting.
In fact, the above ACL cannot match any user in that environment!
Copy link
Contributor Author

@rousskov rousskov left a comment

Choose a reason for hiding this comment

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

This PR was triggered by surprising test results obtained while working on #1611: I was not expecting a case sensitivity setting, especially the default one, to result in altered usernames.

FWIW, I tried to verify statements in the updated text, but authentication caveats is not my expertise area, and I could have missed something important. I would prefer not to merge this PR without @yadij blessing (at least).

We should either document or remove this caveat, but I do not insist on any specific documentation text. If you can think of a better wording, please edit the proposed text or post specific replacement suggestions.

@rousskov rousskov requested a review from yadij December 5, 2023 14:34
@yadij
Copy link
Contributor

yadij commented Dec 5, 2023

FYI, there is likely a bug involved here.

When Squid sends charset="utf-8" it is telling the client it will follow RFC 7617 section 2.1 requirement:

   For the user-id, recipients MUST support all characters defined in
   the "UsernameCasePreserved" profile defined in Section 3.3 of
   [RFC7613], with the exception of the colon (":") character.

The absence of charset means deprecated ASCII handling. Which does leaves case sensitivity undefined in the spec.

So, to fix this properly we need to:

  • make casesenitive a tri-state on/off/unset
  • when ON or unset Squid should send charset="utf-8" and always[1] handle the credentials case sensitively.
  • when OFF Squid should omit the charset from sent header and retain the legacy behaviour of default-insensitive

[^1] by "always" I do mean everywhere. The auth_param logics, as well as ACLs etc

@rousskov
Copy link
Contributor Author

rousskov commented Dec 6, 2023

FYI, there is likely a bug involved here.

I agree that the current behavior can be considered a bug (or a combination of bugs). We probably disagree on the specifics of that bug.

I recommend merging this documentation (if we agree that it represents the current behavior that is unlikely to change in a few days) and then wait for improvements and fixes. When those improvements/fixes come, they will be reflected in the already merged documentation, assisting us with their impact comprehension.

Regardless of whether these documentation improvements are merged, I recommend discussing any authentication configuration/implementation changes elsewhere. I see several significant red flags in the proposed changes, but it feels wrong to start reviewing that proposal here, especially if we agree that those changes are not going to be trivial and should be done in dedicated PR(s). If you are going to work on improving authentication along the lines of that proposal, please make an RFC on squid-dev, open a Draft PR (without any code changes at first), or something like that.

@rousskov rousskov added the S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box label Dec 6, 2023
@kinkie
Copy link
Contributor

kinkie commented Dec 16, 2023

@yadij any comments? I'll clear for merge on Dec 17th unless you have any, and then we can add looking into the bug to our backlog

Copy link
Contributor

@yadij yadij left a comment

Choose a reason for hiding this comment

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

.

@yadij yadij added M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels and removed S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box labels Dec 17, 2023
squid-anubis pushed a commit that referenced this pull request Dec 17, 2023
    acl badGuys proxy_auth Bob
    http_access deny badGuys

Admins may be surprised that their proxy_auth ACLs do not match users
with logins identical to those listed as proxy_auth ACL values. For
example, a user logged in as "Bob" will no match the above ACL if Basic
authentication is used without an explicit "casesensitive on" setting.
In fact, the above ACL cannot match any user in that environment!
@squid-anubis squid-anubis added the M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels label Dec 17, 2023
@squid-anubis squid-anubis added M-merged https://github.com/measurement-factory/anubis#pull-request-labels and removed M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels labels Dec 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
M-merged https://github.com/measurement-factory/anubis#pull-request-labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants