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?

[bugy]

I'm not sure, what you mean :)
As for me, USERNAME should be one of:

1. Authenticated username
2. OR username from the trusted proxy header

In any other case, I would prefer keeping USERNAME empty and make audit name fallback to other values

[oktorok]

Okay thanks for the explication :p.

Yes.. if USERNAME is mapped with user_id, then, in the cases where user_id is equal to an ip or a token effectively will have none sense as username.

Maybe sending server_conf.user_header_name to audit_utils.py as argument:

def get_all_audit_names(request_handler, user_header_name):
    result = {}

    auth = request_handler.application.auth
    if auth.is_enable():
        auth_username = auth.get_username(request_handler)
    else if (user_header_name):
        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

What do you think? Although maybe is a bit ugly send user_header_name like that to the function.

[bugy]

I'm coming from OOP world, so for me more natural would be to make identification to expose one more method: get_audit_username
This will be just username for AuthIdentification and IpBasedIdentification will provide user_header if enabled, or None for other cases

[oktorok]

I am a completely amateur in OOP, i just need to force me to create objects because in my mind everything are functions ^_^''

Then the thing will be create a method in Identification class:

class Identification(metaclass=abc.ABCMeta):
    @abc.abstractmethod
    def identify(self, request_handler):
        pass
    def get_audit_username(self, request_handler):
        if self._user_header_name:
            return self._user_header_name
        return None

And in audit_utils.py:

def get_all_audit_names(request_handler):
    result = {}

    auth = request_handler.application.auth
    audit_username = request_handler.application.identification.get_audit_username(request_handler)
    if auth.is_enable():
        auth_username = auth.get_username(request_handler)
    else if audit_username:
        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]

:) Not really
In OOP the main idea, that you have different implementations of the same function, so

(only showing new lines, and I though that identify_for_audit would be a better name here)

class Identification(metaclass=abc.ABCMeta):
    ...
    @abc.abstractmethod
    def identify_for_audit(self, request_handler):
        pass

class AuthBasedIdentification(Identification):
    ....
    def identify_for_audit(self, request_handler):
        return self.identify(request_handler);

class IpBasedIdentification(Identification):
    ....
    def identify_for_audit(self, request_handler):
        remote_ip = request_handler.request.remote_ip
        if (remote_ip in self._trusted_ips) and (self._user_header_name):
            return request_handler.request.headers.get(self._user_header_name, None)
        return None

And then the code in audit_utils remains pretty simple (instead of auth):

    auth_username = request_handler.application.identification.identify_for_audit(request_handler)
    if auth_username:
        result[AUTH_USERNAME] = auth_username

So, because of OOP polymorphism concept, audit_utils doesn't care how identifications work internally.

[oktorok]

Oh, I see.

Thank you so much for everything. I have implemented it on my server and works perfectly.
If you need anything from me i would be pleased to help you.

Than you so much again 😄

[bugy]

Do you want to make a pull request? So we can add it as a feature Jaume <[email protected]> schrieb am Fr., 17. Apr. 2020, 00:31:

…

Oh, I see. Thank you so much for everything. I have implemented it on my server and works perfectly. If you need anything from me i would be pleased to help you. Than you so much again 😄 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#292 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJXPJN7KBKGAZZOGI63VY3RM6BMXANCNFSM4MIVSWLA> .

[oktorok]

I think i did it, i'm not sure because is my first time doing a pull request :p