Using auth-proxy username not detected

[oktorok]

I have auth-proxy configuration, everything works fine but in the logs the variable USERNAME is None instead of the value in my user_header_name

conf.json

{
    "port": 5000,
    "address": "0.0.0.0",
    "title": "My Script Server",
    "max_request_size": 100,
    "access": {
	"allowed_users": "*",
	"admin_users": [ "USERA", "USERB", "USERC" ],
	"user_header_name": "X-Forwarded-User",
	"trusted_ips": "*"
    },
    "logging": {
	"execution_file": "${USERNAME}.log",
	"execution_date_format": "%y-%m-%d-%H:%M"
    }
}

[bugy]

Hi @oktorok, thanks for reporting. It looks like a bug indeed.

[oktorok]

Hi @bugy i have been looking around this bug, and although i didn't find the exact problem, because the problem has to do with request_handler and im not sure where is generated.

Anyway i can tell you that the problem appers when there are upper cases in the user_header_name. If in my example i put instead x_forwarded_user it works as expected.

[bugy]

Wow, thanks for checking! I thought I knew the root cause, until you said, that it works sometimes...
Could it be, that you enabled sending basic auth credentials as well? Because in this case Script server can get username from basic auth

[oktorok]

My configuration is this one:

{
    "port": 5000,
    "address": "0.0.0.0",
    "title": "My Script Server",
    "max_request_size": 100,
    "access": {
	"allowed_users": "*",
	"admin_users": [ "USERA", "USERB", "USERC" ],
	"user_header_name": "X-Forwarded-User",
	"trusted_ips": "*"
    },
    "logging": {
	"execution_file": "${USERNAME}.log",
	"execution_date_format": "%y-%m-%d-%H:%M"
    }
}

The problem i saw is that in the file src/auth/identification.py

       if new_trusted:
            if request_handler.get_cookie(self.COOKIE_KEY):
                request_handler.clear_cookie(self.COOKIE_KEY)
            if self._user_header_name:
                user_header = request_handler.request.headers.get(self._user_header_name, None
)

self._user_header_name instead of be "X_Forwarded_User", as i specified in conf file, it was "x_forwarded_user" which obviously was not in request headers...

Now that i think it... that actually make sense because the headers are not case sensitive meaning that you receive all headers in lower case meanwhile in conf file i wrote with capital letters,

Maybe i would suggest a .tolower in server_conf file when reading user_header_name or specify in the wiki files that the value in user_header_name should be lowercase.

[oktorok]

Sorry, i was wrong, its not working yet, but now i can see that the value is taken properly in identification.py.

I will still looking on it.

Thank you so much for this project ^^

[oktorok]

in logging.py file USERNAME is mapped with username whose value comes from:

username = get_first_existing(all_audit_names, audit_utils.AUTH_USERNAME, audit_utils.PROXIED_USERNAME)

As i saw the user_header_name header is mapped with user_id (wich one is correctly set if i open a log file) then i did't find any place where one of those values (all_audit_names, PROXIED_NAME. AUTH_NAME) could be pointing to user_header_name.

Then the problem could be that one of these names should have been mapped to user_header_name or that should be added a fourth value pointing to user_header_name or another possible solution could be add to the logging parameters user_id which one is mapped to user_name_header.

What are your thoughts?
Sorry for bother you so much

[bugy]

Hi @oktorok,

I guess this guy is gulty
https://github.com/bugy/script-server/blob/master/src/utils/audit_utils.py:22

It takes auth username, but in case of header identification, we don't use auth
Header identification can be obtained with:
user_id = request_handler.application.identification.identify(request_handler)
But user_id can also be a randomly generated id, if no header is specified
So using it as username might be incorrect

PS you cannot bother by helping me :)

[oktorok]

Then i understand that this value we are looking for should be at AUTH_USERNAME.
Following AUTH_USERNAME i finish in src/utils.tornado_utils.py in function get_secure_cookie, i suppose this is for another type of auth, doesnt it?

before tornado_utils.py it goes through src/auth/tornado_auth.py:

    def get_username(self, request_handler):
        if not self.is_enabled():
            return None

        username = self._get_current_user(request_handler)
        return username

Maybe the solution is modify it like this:

    def get_username(self, request_handler):
        if not self.is_enabled():
            if server_config.user_header_name:
                username = request_handler.request.headers.get(user_header_name, None)
                if username:
                    return username
            return None

        username = self._get_current_user(request_handler)
        return username

If i remember correctly for use user_header_name auth needs to be disable then we will be sure to user_id is the value we want and not an random one, in case user_header_name is not present it will return None as he does right now

[bugy]

@oktorok yes, it would work.
But to be honest, I don't want to mix authentication and identification. I would prefer auth to have only one responsibility: to be responsible only for authentication
May be audit utils should use request_handler.application.identification instead of request_handler.application.auth

[oktorok]

I see your point, and im agree with you, is more clean if auth only auts and identification only identify. Then this could be done in audit_utils.py:

    auth = request_handler.application.auth
    if auth.is_enable():
        auth_username = auth.get_username(request_handler)
    else:
        auth_username = request_handler.application.identification.identify(request_handler)
    if auth_username:
        result[AUTH_USERNAME] = auth_username

    basic_auth_username = find_basic_auth_username(request_handler)
    if basic_auth_username:
        result[PROXIED_USERNAME] = basic_auth_username

[bugy]

Yep, looks better and more isolated to me

My only concern now, that you will get randomly generated user id, if neither auth nor header_name is enabled. It will be like 123.456.789-abc-def. I'm not sure, if it will be an expected behaviour. To show this ID everywhere in logging. Usually showing simple IP or hostname would be easier to identify the user
But I'm not sure about it. May be showing this unique id will be good

[oktorok]

But it auth.is_enable() == True it will take auth_username from auth as it is doing right now and if auth.is_enable() == False identification will be IpBasedIdentification as specified in server.py.

There, in case user_header_name exist, it will return the value we want.

If it does not exist, means that

• Or the client is identified by ip, then the headers X-Forwarded-From or X-Real-Ip should be be set .
• Or the client is identified by a random token.

In both cases ip/random-token work as identifier of the client then its logic they become client's username.
For me everything looks correct, if someone doesn't like logging users with random token, can add ip authentication, or log them with another variable.

What do you think? maybe im misunderstanding something.

[bugy]

In both cases ip/random-token work as identifier of the client then its logic they become client's username.

Previously, the audit name would be resolved as a proxy basic auth or user's PC as a fallback to auth.
So when you check the execution history, you will see, that the script was executed by johns-macbook, for example. It's usually more readable than 192.168.0.1-abc-def
And readability/backward compatibility is what I'm concerned about here.

[oktorok]

You mean change the value that USERNAME receive when no auth + ip and no auth + random token?