Xpra-org / xpra

Persistent remote applications for X11; screen sharing for X11, MacOS and MSWindows.
https://xpra.org/
GNU General Public License v2.0
1.98k stars 169 forks source link

Xpra Client not reconnecting after Update to Version 12 #4240

Closed zvooveDev closed 3 months ago

zvooveDev commented 5 months ago

Describe the bug After upgrading Xpra from version 4.4.6-r29-1 to 6.0-r0-1 I have issues reconnecting. When I close the XPra Client Browser Tab, wait a minute and reopen it I get the message "connection failed, invalid address?". Clicking on Connect results in the same message. The only fix in this situation is deleting Website data in Crome.
The problem does not occur if I activate "Disable cache" in the Network Tab of the Chrome development Console. I can reliably reproduce and fix the problem by upgrading to 6.0-r0-1 (HTML Client Version 12) or downgrading to Version 4.4.6-r29-1 (HTML Client Version 8.1).

To Reproduce Steps to reproduce the behavior: Close the XPra Client Browser Tab, wait a minute and reopen it I get the message "connection failed, invalid address?". Clicking on Connect results in the same message. The only fix in this situation is deleting Website data in Crome.

System Information (please complete the following information): Server OS: ubuntu:22.04 Client OS: Windows 11 Browser: Chrome 124.0.6347.93 Xpra Server Version 6.0-r0-1 (also tested 6.0.1) Xpra Client Version 12 Application: Delphi Application running in Wine version 8.0.2 We are using an OAuth Proxy, maybe something changed in the local cache which interferes with our tokens?

Any idea what could cause this problem? What exactly is being cached and used accordingly when connecting?

totaam commented 5 months ago

The message connection failed, invalid address? comes from: https://github.com/Xpra-org/xpra-html5/blob/aad8e6c116089180eee60f200b11e8301a5cd915/html5/js/Client.js#L2431-L2435 And is called from only two places:

In both cases, there should be a message in your javascript console preceding this - you may need to enable main debug logging to see the one from the disconnect handler. If the authentication went as far as the server, there should also be a message in your server log (add -d auth there to see more details).

It is possible that the problem comes from the authentication and only shows up as a more general connection failure.

Issues that may be (distantly?) related: https://github.com/Xpra-org/xpra-html5/issues/278 https://github.com/Xpra-org/xpra-html5/issues/262 https://github.com/Xpra-org/xpra-html5/issues/260 And this one has caused us pain a number of times: https://github.com/Xpra-org/xpra-html5/pull/292

totaam commented 5 months ago

Can you reproduce this with the 5.x LTS branch? (this should narrow things down somewhat) When I tested with a simple:

xpra start --bind-tcp=0.0.0.0:10000,auth=sys --no-daemon  --start=xterm :6

Closed the tab in Chrome, waited a few seconds and then restored it. The automatic re-connection failed to prompt me for a password and I was sent back to the connect dialog. But clicking connect from there worked as expected. I am moving this issue to: https://github.com/Xpra-org/xpra-html5/issues/308

Is your issue the same? Does the bug occur if you just open the URL again, without restoring the tab?

zvooveDev commented 5 months ago

Some findings from today:

totaam commented 5 months ago

I don't think that it will fix things, but worth a try: https://github.com/Xpra-org/xpra-html5/releases/tag/v13

zvooveDev commented 5 months ago

Updating to Xpra 6.1-r35816-1 and Client Version 13 did not have any effect on the issue. But it's interesting that 5.0.8-r0-1 works fine.

totaam commented 5 months ago

I'm stuck on this one. Can you run your server with -d auth and post the log output of both a successful connection and a failed reconnect?

I'm not using Xpra for Authentication but an OAuth2 Proxy

Can you expand on this? Are you using the keycloak authentication module? What command line arguments? Can you share an example proxy config?

Is there anything in the Javascript console? (you may need to enable the "keep logs" option so that it doesn't start clean when it navigates - as even reloading the page may trigger it)


Looking at the changes to the keycloak auth module and its superclass, there were a lot of cosmetic changes and refactoring between v5 and v6 - any issues in these types of changes are normally caught by running the type checks or the unit tests, unfortunately the keycloak module is the only one that does not have proper unit tests that can run automatically.. I don't have an environment to test this at my end.

The only significant change in v6 is "add keycloak validation of authentication groups support": b4ba4ca258e613405fb7837bec4c2498ff48af49 by @iDmple - but really can't see how it would make any difference if you don't set the claim_field authentication module option or the XPRA_KEYCLOAK_CLAIM_FIELD environment variable


This "groups support" can be undone easily enough by reverting this patch:

--- a/xpra/server/auth/keycloak.py  2024-06-18 22:06:25.184036424 +0700
+++ b/xpra/server/auth/keycloak.py  2024-06-18 22:06:11.348127306 +0700
@@ -16,6 +16,9 @@
 KEYCLOAK_CLIENT_ID = os.environ.get("XPRA_KEYCLOAK_CLIENT_ID", "example_client")
 KEYCLOAK_CLIENT_SECRET_KEY = os.environ.get("XPRA_KEYCLOAK_CLIENT_SECRET_KEY", "secret")
 KEYCLOAK_REDIRECT_URI = os.environ.get("XPRA_KEYCLOAK_REDIRECT_URI", "http://localhost/login/")
+KEYCLOAK_CLAIM_FIELD = os.environ.get("XPRA_KEYCLOAK_CLAIM_FIELD", "")  # field containing groups claim
+KEYCLOAK_AUTH_GROUPS = os.environ.get("XPRA_KEYCLOAK_AUTH_GROUPS", "")  # authorized groups
+KEYCLOAK_AUTH_CONDITION = os.environ.get("XPRA_KEYCLOAK_AUTH_CONDITION", "and")  # authentication condition
 KEYCLOAK_SCOPE = os.environ.get("XPRA_KEYCLOAK_SCOPE", "openid")
 KEYCLOAK_GRANT_TYPE = os.environ.get("XPRA_KEYCLOAK_GRANT_TYPE", "authorization_code")

@@ -28,6 +31,9 @@
         self.client_id = kwargs.pop("client_id", KEYCLOAK_CLIENT_ID)
         self.client_secret_key = kwargs.pop("client_secret_key", KEYCLOAK_CLIENT_SECRET_KEY)
         self.redirect_uri = kwargs.pop("redirect_uri", KEYCLOAK_REDIRECT_URI)
+        self.claim_field = kwargs.pop("claim_field", KEYCLOAK_CLAIM_FIELD)
+        self.auth_groups = set(kwargs.pop("auth_groups", KEYCLOAK_AUTH_GROUPS).split())
+        self.auth_condition = kwargs.pop("auth_condition", KEYCLOAK_AUTH_CONDITION)
         self.scope = kwargs.pop("scope", KEYCLOAK_SCOPE)
         self.grant_type = kwargs.pop("grant_type", KEYCLOAK_GRANT_TYPE)

@@ -162,8 +168,67 @@
             log("keycloak: token is active")

             user_info = keycloak_openid.userinfo(access_token)
-            log("userinfo_info: %r", user_info)
-            log("keycloak authentication succeeded: token is active")
+            log("user_info: %r", user_info)
+
+            log("claim_field: %r", self.claim_field)
+            log("auth_groups: %r", self.auth_groups)
+
+            # if claim_field is defined, check that the auth_groups match the groups_claim condition
+            if self.claim_field:
+                if not self.auth_groups:
+                    log.error("Error: keycloak authentication failed as auth_groups is invalid")
+                    return False
+
+                def extract_groups_claim(data_dict, keys):
+
+                    """
+                    Extracts a nested value from a dictionary using a list of keys.
+
+                    This function is specifically designed to handle nested dictionaries,
+                    such as the ones provided by Keycloak userinfo.
+
+                    For example, userinfo could contain the following structure:
+
+                    "resource_access": {
+                        "roles": {
+                            "group": [
+                                "client-group-1",
+                                "client-group-2"
+                            ]
+                        }
+                    }
+
+                    In this scenario, the function traverses the dictionary using the keys
+                    ['resource_access', 'roles', 'group'] to extract the list of groups
+                    ['client-group-1', 'client-group-2'].
+
+                    Args:
+                        data_dict (dict): The dictionary from which to extract the nested value.
+                        keys (list): A list of keys that represents the path to the desired value
+                                     in the nested dictionary.
+
+                    Returns:
+                        The nested value extracted from the dictionary. If the sequence of keys is
+                        exhausted, it returns the portion of the dictionary that it has navigated to.
+                    """
+                    return extract_groups_claim(data_dict[keys.pop(0)], keys) if len(keys) else data_dict
+
+                groups_claim = set(extract_groups_claim(user_info, self.claim_field.split('.')))
+                log("groups_claim: %r", groups_claim)
+
+                if self.auth_condition == "or":
+                    if not self.auth_groups.intersection(groups_claim):
+                        log.error("Error: keycloak authentication failed as groups claim is not satisfied")
+                        return False
+                elif self.auth_condition == "and":
+                    if not self.auth_groups.issubset(groups_claim):
+                        log.error("Error: keycloak authentication failed as groups claim is not satisfied")
+                        return False
+                else:
+                    log.error("Error: keycloak authentication failed as auth_condition is invalid")
+                    return False
+
+            log("keycloak authentication succeeded")
             return True
         except KeycloakError as e:
             log.error("Error: keycloak authentication failed")
iDmple commented 5 months ago

For what's it's worth we don't have this issue on our end with the added code. I'd also like to know more about OAuth2 Proxy.

zvooveDev commented 3 months ago

I just updated to Version 6.1 and Client Version 14 (including other dependencys) and the problem is solved. Thank you for your help.