Fix 889fc47 for SSL bumping with an authentication type other than the Basic

[verdel]

Commit 889fc47 was made to fix issue with Basic authentication and SSL bumping.

But after this commit we can no longer properly use http_access with
proxy_auth/proxy_auth_regex ACL because that type of ACL always return 1(match)
regardless of the conditions in the rules.

I add an authentication scheme check. If 889fc47 patch was made to fix the bug with
Basic authentication, then we should check whether this type of authentication is really used.

[squid-prbot]

Can one of the admins verify this patch?

[rousskov]

OK to test

[rousskov]

I understand 889fc47 logic as follows: Authentication of sslBumped requests is done by AuthenticateAcl(). matchProxyAuth() is only called when AuthenticateAcl() returned ACCESS_ALLOWED. Thus, matchProxyAuth() must return 1 for all sslBumped requests.

Is the above logic flawed?

Please note that even if the above logic is correct, there may still be bugs that need to be fixed. However, the fix may be different than the one you have proposed.

To avoid misunderstanding, I assume that 889fc47 broke something in your environment and that the proposed fix works well in your environment. I am not trying to dispute those assertions. The next step is to understand what exactly was broken (and either accept your fix or come up with a better one).

proxy_auth/proxy_auth_regex ACL [...] always return 1(match) regardless of the conditions in the rules.

AuthenticateAcl(ACLChecklist *ch)
{
    ...
    if (request->flags.sslBumped) {
        debugs(28, 5, "SslBumped request: It is an encapsulated request do not authenticate");
        checklist->auth_user_request = checklist->conn() != NULL ? checklist->conn()->getAuth() : request->auth_user_request;
        if (checklist->auth_user_request != NULL)
            return ACCESS_ALLOWED;
        else
            return ACCESS_DENIED;

Do you know why AuthenticateAcl() always returns ACCESS_ALLOWED for sslBumped requests in your environment?

If 889fc47 patch was made to fix the bug with Basic authentication, then ...

AFAICT, 889fc47 was agnostic to the authentication scheme used. It is possible that some authentication schemes are immune from the bug fixed by 889fc47 because of how Squid stores their authentication info, but I doubt we should treat basic authentication specially in matchProxyAuth() context.

[rousskov]

Please avoid out-of-scope whitespace changes.

[verdel]

Forgive me for inattention with out-of-scope whitespace.

[verdel]

I understand 889fc47 logic as follows

Yes, 889fc47 logic is:

1. AclProxyAuth method match() call method AuthenticateAcl()
2. When request sslBumped AuthenticateAcl() set checklist->auth_user_request as result of getAuth() method of connection object or request->auth_user_request
   After that AuthenticateAcl() return ACCESS_ALLOWED if checklist->auth_user_request is not NULL or ACCESS_DENIED otherwise
3. If AuthenticateAcl() return ACCESS_ALLOWED - executed matchProxyAuth()
4. When request sslBumped matchProxyAuth() always return 1.
   The result is a violation of the logic of ACL with type proxy_auth and proxy_auth_regex on requests inside sslBump bump context. In fact, we can not use proxy_auth acl with all requests inside sslBumped connection. This type of ACL always return match regardless of the match string specified in the ACL.
   If request not sslBumped matchProxyAuth() check auth_user_request with authenticateUserAuthenticated() method and return 0 if it false.
   And at the end matchProxyAuth() return result of matching username with match string specified in the ACL.

Do you know why AuthenticateAcl() always returns ACCESS_ALLOWED for sslBumped requests in your environment?

AuthenticateAcl() not always returns ACCESS_ALLOWED only if checklist->auth_user_request is not NULL and with this part all is ok. You do not need to re-authenticate if the procedure was passed for the connection that initialized sslBump.

I will now try to quickly understand the process of adding data to the user auth cache and answer your question about the possible immunity of the authentication schemes to the bug fixed by 889fc47.

I will try to write as quickly as possible.

[rousskov]

I think I understand your use case and the bug better now, thank you. You are using proxy_auth foo on a bumped request after (successfully) authenticating its connection with proxy_auth bar.

Commit 889fc47 logic was probably flawed because it only considered the authentication stage. It should have considered post-authentication stage as well, when a different proxy_auth ACL may be used. matchProxyAuth() checks must not affect authentication state (that was the bug 889fc47 fixed and we must keep it fixed), but they still may report a mismatch (that is the bug this pr is fixing).

AFAICT, what's left is making sure that matchProxyAuth() (including the code it calls) does not affect authentication state when called for bumped requests. It may return a mismatch, but it must not modify anything related to authentication.

I will try to write as quickly as possible.

No rush. Quality (including clarity) is far more important than speed here, especially if you already have a workaround for your deployment. And thank you for working on this fix!

[verdel]

I once again carefully examined the entire mechanism of work regex_auth ACL and understood the reason why the commit 889fc47 was made.

1. Without commit 889fc47 matchProxyAuth() method checks condition

if (!authenticateUserAuthenticated(Filled(checklist)->auth_user_request)) {
        return 0;
}

and returns mismatch for acl if authenticateUserAuthenticated(Filled(checklist)->auth_user_request) returns 0
Filled(checklist)->auth_user_request includes cached auth user data

2. authenticateUserAuthenticated() from src/auth/UserRequest.cc returns

int
authenticateUserAuthenticated(Auth::UserRequest::Pointer auth_user_request)
{
    if (auth_user_request == NULL || !auth_user_request->valid())
        return 0;

    return auth_user_request->authenticated();
}

• 0 for Null auth_user_request or invalid formed auth_user_request
• return value of auth_user_request->authenticated()

3. auth_user_request->authenticated() checks user()->credentials(). It can be equal to:
   • Unchecked
   • Authenticated
   • Pending helper result
   • Handshake happening in stateful auth.
   • Failed auth

This method for Basic authentication scheme in src/auth/basic/UserRequest.cc:

int
Auth::Basic::UserRequest::authenticated() const
{
    Auth::Basic::User const *basic_auth = dynamic_cast<Auth::Basic::User const *>(user().getRaw());

    if (basic_auth && basic_auth->authenticated())
        return 1;

    return 0;
}

For Digest authentication scheme in src/auth/digest/UserRequest.cc:

int
Auth::Digest::UserRequest::authenticated() const
{
    if (user() != NULL && user()->credentials() == Auth::Ok)
        return 1;

    return 0;
}

etc...

user() object is taken from the auth user cache. The data in the cache can only be changed for the authentication scheme Basic.
For this we can use the method updateCached() from src/auth/basic/User.cc.

void
Auth::Basic::User::updateCached(Auth::Basic::User *from)
{
    debugs(29, 9, HERE << "Found user '" << from->username() << "' already in the user cache as '" << this << "'");

    assert(strcmp(from->username(), username()) == 0);

    if (strcmp(from->passwd, passwd)) {
        debugs(29, 4, HERE << "new password found. Updating in user master record and resetting auth state to unchecked");
        credentials(Auth::Unchecked);
        xfree(passwd);
        passwd = from->passwd;
        from->passwd = NULL;
    }

    if (credentials() == Auth::Failed) {
        debugs(29, 4, HERE << "last attempt to authenticate this user failed, resetting auth state to unchecked");
        credentials(Auth::Unchecked);
    }
}

If the same user sends a CONNECT request with invalid credentials this method set credentials(Auth::Unchecked)

Because of this, for all subsequent requests in sslBump session expression:

if (!authenticateUserAuthenticated(Filled(checklist)->auth_user_request)) {
        return 0;
}

will return 0 and in the end ACL will always(because we do not re-authenticate in sslBump session) return mismatch.

After the commit 889fc47, the behavior changed to the opposite. For all requests in sslBump connection ACL always returns match.

Because of the possibility of poisoning the auth user cache in Basic authentication scheme аnd at the same time, the lack of re-authentication in sslBump connection, we need to make a decision. What the proxy_auth ACL should always return (match or mismatch)?

In any case, we must exclude from this check the Digest, NTLM and Negotiate authentication scheme , because of this scheme are immune from auth user cache poisoning:

    if (checklist->request->flags.sslBumped) {
        if (checklist->auth_user_request->user()->auth_type == Auth::AUTH_BASIC)
            return 1; // AuthenticateAcl() already handled this bumped request
    }

In this case ACL will return 1 for all regex_auth match string for sslBump request with Basic authentication scheme only.

I would really like to know @chtsanti opinion about this situation.

[verdel]

I pondered a little more and came to the following conclusion:
Since we do not perform re-authentication for requests inside the sslBumped connection already at the level of the method AuthenticateAcl() inside AclProxyAuth.cc so we must skip this check for sslBumped requests inside matchProxyAuth() too

if (!authenticateUserAuthenticated(Filled(checklist)->auth_user_request)) {
            return 0;
}

For all requests inside sslBumped connection we must immediately perform the actions of comparing the username (we take a value from the cache where it falls at the time of the initialization of the sslBump connection by method CONNECT) with the match string, which is specified in the proxy_auth ACL. We make this decision only on the basis of the use of the sslBump and must do this regardless of the authentication scheme used.

Therefore, in my opinion, the method should still look like this:

/* aclMatchProxyAuth can return two exit codes:
 * 0 : Authorisation for this ACL failed. (Did not match)
 * 1 : Authorisation OK. (Matched)
 */
int
ACLProxyAuth::matchProxyAuth(ACLChecklist *cl)
{
    ACLFilledChecklist *checklist = Filled(cl);
    if (!checklist->request->flags.sslBumped) {
        if (!authenticateUserAuthenticated(Filled(checklist)->auth_user_request)) {
            return 0;
        }
    }
    /* check to see if we have matched the user-acl before */
    int result = cacheMatchAcl(&checklist->auth_user_request->user()->proxy_match_cache, checklist);
    checklist->auth_user_request = NULL;
    return result;
}

[yadij]

FYI the overall auth system behaviour should be:

1. fetch both connection credentials and HTTP request credentials,

2. IF both exist and the request credentials non-match the connection credentials, deliver error and close the client connection.

Otherwise connection auth credentials take precedence over request credentials for step 3-4:

3. validity test using the auth schemes process (unless bumped traffic).

4. test validated credentials against ACL criteria.

Bumping should take the credentials used to authenticate the original CONNECT request and store them as connection auth credentials for the future bumped requests.

In all cases the error status for bumped requests should always a 403, for non-bumping may be 401 or 407, or 511.

[verdel]

@yadij, thanks for explaining the principle of the auth system behaviour.

All steps related to the authentication process are performed before the method matchProxyAuth() is called.

If I understand the code correctly, then the decision to authenticate the user or use credentials used to authenticate the original CONNECT request(if current request is sslBumped) is already taken at the time this method is called. Correct me if I am wrong.

Therefore, as I wrote above:

For all requests inside sslBumped connection we must immediately perform the actions of comparing the username (we take a value from the cache where it falls at the time of the initialization of the sslBump connection by method CONNECT) with the match string, which is specified in the proxy_auth ACL. We make this decision only on the basis of the use of the sslBump and must do this regardless of the authentication scheme used.

As a result of this discussion, I would like to change the commit code in the PR for the above if the reviewers agree on this:

/* aclMatchProxyAuth can return two exit codes:
 * 0 : Authorisation for this ACL failed. (Did not match)
 * 1 : Authorisation OK. (Matched)
 */
int
ACLProxyAuth::matchProxyAuth(ACLChecklist *cl)
{
    ACLFilledChecklist *checklist = Filled(cl);
    if (!checklist->request->flags.sslBumped) {
        if (!authenticateUserAuthenticated(Filled(checklist)->auth_user_request)) {
            return 0;
        }
    }
    /* check to see if we have matched the user-acl before */
    int result = cacheMatchAcl(&checklist->auth_user_request->user()->proxy_match_cache, checklist);
    checklist->auth_user_request = NULL;
    return result;
}

[verdel]

Maybe I should rebase fix-sslbump-proxy_auth-acl branch onto master of upstream and amend commit from this PR with this code

/* aclMatchProxyAuth can return two exit codes:
 * 0 : Authorisation for this ACL failed. (Did not match)
 * 1 : Authorisation OK. (Matched)
 */
int
ACLProxyAuth::matchProxyAuth(ACLChecklist *cl)
{
    ACLFilledChecklist *checklist = Filled(cl);
    if (!checklist->request->flags.sslBumped) {
        if (!authenticateUserAuthenticated(Filled(checklist)->auth_user_request)) {
            return 0;
        }
    }
    /* check to see if we have matched the user-acl before */
    int result = cacheMatchAcl(&checklist->auth_user_request->user()->proxy_match_cache, checklist);
    checklist->auth_user_request = NULL;
    return result;
}

[rousskov]

Maybe I should rebase fix-sslbump-proxy_auth-acl branch onto master of upstream

By default, I would not worry about rebasing until your changes are approved in principle. Otherwise, you will end up rebasing every few days as master changes... (Eventually, rebasing will happen automatically. Now, it should happen manually at the end or whenever it becomes important for some reason.)

and amend commit from this PR with this code

Yes, please do commit as many changes as you need to address reviewer concerns. It is easier to review committed code than copy-pasted snippets.

BTW, you do not need to call Filled() for an ACLFilledChecklist pointer. We only need to "fill" the checklist once.

[verdel]

BTW, you do not need to call Filled() for an ACLFilledChecklist pointer. We only need to "fill" the checklist once.

This code:
if (!authenticateUserAuthenticated(Filled(checklist)->auth_user_request))
is in the master branch of the repository. I did not want to change the code much, only as part of fixing a specific problem. But I agree that this part can also be changed.

[rousskov]

My wrong assumption that you have called Filled() twice is an excellent illustration of why reviewing committed code is better than reviewing copy-pasted code snippets :-). Yes, please continue to avoid changing the code that does not contribute or affect the bug you are fixing.

[chtsanti]

Sorry I missed this PR.

The patch is correct.
The cacheMatchAcl function is responisble to check if the username (which is already authenticated) is allowed or denied.

I am dismissing my stale negative vote. Based on @chtsanti review, I assume that @verdel claim that calling cacheMatchAcl() does not change authentication state and, hence, we are not recreating the bug fixed by 889fc47

[rousskov]

Edit of the dismissal comment above: I assume that @verdel claim that ... is correct.