jkroepke/openvpn-auth-azure-ad

Multiple issue about remember users

pppwaw opened this issue · 13 comments

pppwaw commented

At openvpn_auth_azure_ad/authenticator.py, 323 lines, we define the result as an empty Dict.
At 343 lines, we set result as the self.handle_response_challenge(client) 's return value . In some time ( not unusual ). If the result isn't None, it will run the code between 345 to 381 lines.
At 391 lines, if remember-user is enabled, we will use util.is_authenticated(result) to determine whether the user is authenticated. And it uses return "access_token" in result to decide.
But It causes a problem . The result that can be None, and it will causes TypeError: argument of type 'NoneType' is not iterable .

So, I changed the util.is_authenticated 's code to return result is not None and "access_token" in result, and I will test whether it runs correctly in the next few hours.

pppwaw commented

Second problem:
openvpn_auth_azure_ad/authenticator.py, 386-389 lines

            if "common_name" not in client["env"] and accounts:
                result = self._app.acquire_token_silent(
                    self.token_scopes, account=client["env"]["common_name"]
                )

if there isn't a "common name" in client["env"], why keep call it at acquire_token_silent ?

pppwaw commented

I think this code can be changed to this:

            accounts = self._app.get_accounts(client["env"]["username"])
            if "common_name" not in client["env"] and accounts:
                result = self._app.acquire_token_silent(
                    self.token_scopes, account=accounts[0]
                )
            elif "common_name" in client["env"]:
                result = self._app.acquire_token_silent(
                    self.token_scopes, account=client["env"]["common_name"]
                )

Users with a logged-in account and do not have a common_name use the account to log in. Otherwise, use common_name to log in.
But I am not sure if this is correct, and I hope you can guide me. :)

jkroepke commented

I guess your openvpn setup does not require client certs. In history, I tested the scenario, but I also set the OpenVPN setting --username-as-common-name. I never thought that common_name can be undefined.

pppwaw commented

I guess your openvpn setup does not require client certs. In history, I tested the scenario, but I also set the OpenVPN setting --username-as-common-name. I never thought that common_name can be undefined.

Yes. I used to use the LDAP authentication plugin. To make a ovpn file available to everyone, I removed the client certificate.

jkroepke commented

I will take a look on this scenario and PR on weekend/next week.

jkroepke commented

Hi, if you enable username-as-common-name on your openvpn server, would this resolve your issue?`

pppwaw commented 
Aug 07 16:57:36 debian openvpn-auth-azure-ad[443608]: 2022-08-07 16:57:36,323 ERROR Exception in thread
Aug 07 16:57:36 debian openvpn-auth-azure-ad[443608]: Traceback (most recent call last):
Aug 07 16:57:36 debian openvpn-auth-azure-ad[443608]:   File "/usr/local/lib/python3.9/dist-packages/openvpn_auth_azure_ad/util/thread_pool.py", line 24, in _function_wrapper
Aug 07 16:57:36 debian openvpn-auth-azure-ad[443608]:     return fn(*args, **kwargs)
Aug 07 16:57:36 debian openvpn-auth-azure-ad[443608]:   File "/usr/local/lib/python3.9/dist-packages/openvpn_auth_azure_ad/authenticator.py", line 308, in client_connect
Aug 07 16:57:36 debian openvpn-auth-azure-ad[443608]:     self.authenticate_client(client)
Aug 07 16:57:36 debian openvpn-auth-azure-ad[443608]:   File "/usr/local/lib/python3.9/dist-packages/openvpn_auth_azure_ad/authenticator.py", line 391, in authenticate_client
Aug 07 16:57:36 debian openvpn-auth-azure-ad[443608]:     if util.is_authenticated(result):
Aug 07 16:57:36 debian openvpn-auth-azure-ad[443608]:   File "/usr/local/lib/python3.9/dist-packages/openvpn_auth_azure_ad/util/__init__.py", line 12, in is_authenticated

root@debian:~# cat /etc/openvpn/server.conf 
port 1194
proto tcp-server
server 10.77.2.0 255.255.255.0
dh dh2048.pem
dev tun
ca /etc/openvpn/rsa/pki/ca.crt
key /etc/openvpn/rsa/pki/private/server.key 
cert /etc/openvpn/rsa/pki/issued/server.crt
ifconfig-pool-persist ipp.txt
push "route 10.0.0.0 255.0.0.0"
push "route 59.192.0.0 255.192.0.0"
duplicate-cn
keepalive 10 120
persist-key
persist-tun
verb 3
username-as-common-name
management 127.0.0.1 1145 
management-client-auth

seems no.

jkroepke commented

I see. Changing the lint to

def is_authenticated(result: dict) -> bool:
    return result is not None and "access_token" in result

as you describe it in your PR should resolve it?

pppwaw commented

error:

Aug 09 19:40:36 debian openvpn-auth-azure-ad[23640]: 2022-08-09 19:40:36,888 INFO [cid: 5]: Received client connect
Aug 09 19:40:36 debian ovpn-server[25190]: 120.229.78.58:20911 peer info: IV_LZO=1
Aug 09 19:40:36 debian openvpn-auth-azure-ad[23640]: 2022-08-09 19:40:36,888 INFO [cid: 5 | 36f6e4dc-5b5e-45c7-bdb7-a8eb6a3d7c86]: {'env': {'n_clients': '0', 'password': 'xxxxxx', 'untrusted_port': '20911', 'untrusted_ip': '120.229.78.58', 'username': 'pppwaw@skywolftech.net', 'IV_SSO': 'openurl', 'IV_GUI_VER': 'OpenVPN_GUI_11', 'IV_TCPNL': '1', 'IV_COMP_STUBv2': '1', 'IV_COMP_STUB': '1', 'IV_LZO': '1', 'IV_LZ4v2': '1', 'IV_LZ4': '1', 'IV_CIPHERS': 'AES-256-GCM:AES-128-GCM', 'IV_NCP': '2', 'IV_PROTO': '6', 'IV_PLAT': 'win', 'IV_VER': '2.5.7', 'remote_port_1': '1194', 'local_port_1': '1194', 'proto_1': 'tcp-server', 'daemon_pid': '25190', 'daemon_start_time': '1660045061', 'daemon_log_redirect': '0', 'daemon': '1', 'verb': '3', 'config': '/etc/openvpn/server.conf', 'ifconfig_local': '10.77.2.1', 'ifconfig_remote': '10.77.2.2', 'route_net_gateway': '120.241.236.1', 'route_vpn_gateway': '10.77.2.2', 'route_network_1': '10.77.2.0', 'route_netmask_1': '255.255.255.0', 'route_gateway_1': '10.77.2.2', 'script_context': 'init', 'tun_mtu': '1500', 'link_mtu': '1623', 'dev': 'tun0', 'dev_type': 'tun', 'redirect_gateway': '0', 'END': ''}, 'reason': 'connect', 'cid': '5', 'kid': '0', 'state_id': '36f6e4dc-5b5e-45c7-bdb7-a8eb6a3d7c86'} [{'home_account_id': 'xxxxxx2', 'environment': 'login.microsoftonline.com', 'username': 'pppwaw@skywolftech.net', 'authority_type': 'MSSTS', 'local_account_id': 'xxxxxx', 'realm': 'xxxxxx'}]
Aug 09 19:40:36 debian ovpn-server[25190]: 120.229.78.58:20911 peer info: IV_COMP_STUB=1
Aug 09 19:40:36 debian ovpn-server[25190]: 120.229.78.58:20911 peer info: IV_COMP_STUBv2=1
Aug 09 19:40:36 debian openvpn-auth-azure-ad[23640]: 2022-08-09 19:40:36,889 ERROR Exception in thread
Aug 09 19:40:36 debian openvpn-auth-azure-ad[23640]: Traceback (most recent call last):
Aug 09 19:40:36 debian openvpn-auth-azure-ad[23640]:   File "/usr/local/lib/python3.9/dist-packages/openvpn_auth_azure_ad/util/thread_pool.py", line 24, in _function_wrapper
Aug 09 19:40:36 debian openvpn-auth-azure-ad[23640]:     return fn(*args, **kwargs)
Aug 09 19:40:36 debian openvpn-auth-azure-ad[23640]:   File "/usr/local/lib/python3.9/dist-packages/openvpn_auth_azure_ad/authenticator.py", line 308, in client_connect
Aug 09 19:40:36 debian openvpn-auth-azure-ad[23640]:     self.authenticate_client(client)
Aug 09 19:40:36 debian openvpn-auth-azure-ad[23640]:   File "/usr/local/lib/python3.9/dist-packages/openvpn_auth_azure_ad/authenticator.py", line 389, in authenticate_client
Aug 09 19:40:36 debian openvpn-auth-azure-ad[23640]:     self.token_scopes, account=client["env"]["common_name"]
Aug 09 19:40:36 debian openvpn-auth-azure-ad[23640]: KeyError: 'common_name'

conf:

port 1194
proto tcp-server
server 10.77.2.0 255.255.255.0
dh dh2048.pem
dev tun
ca /etc/openvpn/rsa/pki/ca.crt
key /etc/openvpn/rsa/pki/private/server.key 
cert /etc/openvpn/rsa/pki/issued/server.crt
tls-crypt static.key
ifconfig-pool-persist ipp.txt
push "route 10.0.0.0 255.0.0.0"
push "route 59.192.0.0 255.192.0.0"
duplicate-cn
keepalive 10 120
persist-key
persist-tun
verb 3
username-as-common-name
verify-client-cert none
management 127.0.0.1 1145 
management-client-auth

Seems no too.

From this discussion, seems the username-as-common-name is only useful for auth-user-pass-verify? If so, should we try to read the username when the common_name doesn't exist?

jkroepke commented

Okay thanks for figure this out here.

If so, should we try to read the username when the common_name doesn't exist?

I would not fully agree with this. Instead, using a fallback mechanism, I will introduce a new command line option to exlicit define if common_name or username should be used. This should avoid any new unknown security issues.

jkroepke commented

I guess #14 should able to fix your issue. I'm currently not able to verify this patch.

pppwaw commented

It has an error:

Aug 17 19:56:00 debian openvpn-auth-azure-ad[204445]: 2022-08-17 19:56:00,545 ERROR Exception in thread
Aug 17 19:56:00 debian openvpn-auth-azure-ad[204445]: Traceback (most recent call last):
Aug 17 19:56:00 debian openvpn-auth-azure-ad[204445]:   File "/usr/local/lib/python3.9/dist-packages/openvpn_auth_azure_ad/util/thread_pool.py", line 24, in _function_wrapper
Aug 17 19:56:00 debian openvpn-auth-azure-ad[204445]:     return fn(*args, **kwargs)
Aug 17 19:56:00 debian openvpn-auth-azure-ad[204445]:   File "/usr/local/lib/python3.9/dist-packages/openvpn_auth_azure_ad/authenticator.py", line 284, in client_connect
Aug 17 19:56:00 debian openvpn-auth-azure-ad[204445]:     self.authenticate_client(client)
Aug 17 19:56:00 debian openvpn-auth-azure-ad[204445]:   File "/usr/local/lib/python3.9/dist-packages/openvpn_auth_azure_ad/authenticator.py", line 361, in authenticate_client
Aug 17 19:56:00 debian openvpn-auth-azure-ad[204445]:     result = self._app.acquire_token_silent(
Aug 17 19:56:00 debian openvpn-auth-azure-ad[204445]:   File "/usr/local/lib/python3.9/dist-packages/msal/application.py", line 1117, in acquire_token_silent
Aug 17 19:56:00 debian openvpn-auth-azure-ad[204445]:     result = self.acquire_token_silent_with_error(
Aug 17 19:56:00 debian openvpn-auth-azure-ad[204445]:   File "/usr/local/lib/python3.9/dist-packages/msal/application.py", line 1170, in acquire_token_silent_with_error
Aug 17 19:56:00 debian openvpn-auth-azure-ad[204445]:     result = self._acquire_token_silent_from_cache_and_possibly_refresh_it(
Aug 17 19:56:00 debian openvpn-auth-azure-ad[204445]:   File "/usr/local/lib/python3.9/dist-packages/msal/application.py", line 1225, in _acquire_token_silent_from_cache_and_possibly_refresh_it
Aug 17 19:56:00 debian openvpn-auth-azure-ad[204445]:     "home_account_id": (account or {}).get("home_account_id"),
Aug 17 19:56:00 debian openvpn-auth-azure-ad[204445]: AttributeError: 'str' object has no attribute 'get

seems that the account can't pass as a string?
I tried to change the authenticator.py to this

...
        if self._remember_user_enabled:
            if self._openvpn_identity_key in client["env"]:
                accounts = self._app.get_accounts(client["env"][self._openvpn_identity_key])
                result = self._app.acquire_token_silent(
                    self.token_scopes, account=accounts[0] if accounts else None
                )

...

and seems it works good.

github-actions commented

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.