Implement JWT auth for signaling connections (hello v2) #7472

fancycode · 2022-06-10T07:36:59Z

See #7336

Needs Require firebase JWT lib #7372
Needs Support hello auth version "2.0" with JWT strukturag/nextcloud-spreed-signaling#251
Autodetect if signaling server support hello-v2
Add tests

~~For now can be enabled by setting the app config use-hello-v2 to 1.~~

Signed-off-by: Joachim Bauch <[email protected]>

fancycode · 2022-07-07T13:24:54Z

@nickvergessen this is now ready for review

nickvergessen · 2022-08-02T15:34:42Z

Before I visited the admin page my log was spamed with this error and I couldn't join any room:

{
  "reqId": "YulBGSaLbRnLRs8M99e14wAAAA0",
  "level": 3,
  "time": "2022-08-02T17:22:01+02:00",
  "remoteAddr": "172.17.0.2",
  "user": "--",
  "app": "no app in context",
  "method": "POST",
  "url": "/ocs/v2.php/apps/spreed/api/v3/signaling/backend",
  "message": "OCA\\Talk\\Config::validateSignalingTicket(): Argument #2 ($ticket) must be of type string, null given, called in /home/nickv/Nextcloud/25/server/appsbabies/spreed/lib/Controller/SignalingController.php on line 523 in file /home/nickv/Nextcloud/25/server/appsbabies/spreed/lib/Config.php line 499",
  "userAgent": "nextcloud-spreed-signaling/5d818c725765d03b47f0c436ebb2cd482958c947",
  "version": "25.0.0.4",
  "exception": {
    "Exception": "Exception",
    "Message": "OCA\\Talk\\Config::validateSignalingTicket(): Argument #2 ($ticket) must be of type string, null given, called in /home/nickv/Nextcloud/25/server/appsbabies/spreed/lib/Controller/SignalingController.php on line 523 in file /home/nickv/Nextcloud/25/server/appsbabies/spreed/lib/Config.php line 499",
    "Code": 0,
    "Trace": [
      {
        "file": "/home/nickv/Nextcloud/25/server/lib/private/AppFramework/App.php",
        "line": 172,
        "function": "dispatch",
        "class": "OC\\AppFramework\\Http\\Dispatcher",
        "type": "->"
      },
      {
        "file": "/home/nickv/Nextcloud/25/server/lib/private/Route/Router.php",
        "line": 298,
        "function": "main",
        "class": "OC\\AppFramework\\App",
        "type": "::"
      },
      {
        "file": "/home/nickv/Nextcloud/25/server/ocs/v1.php",
        "line": 62,
        "function": "match",
        "class": "OC\\Route\\Router",
        "type": "->"
      },
      {
        "file": "/home/nickv/Nextcloud/25/server/ocs/v2.php",
        "line": 23,
        "args": [
          "/home/nickv/Nextcloud/25/server/ocs/v1.php"
        ],
        "function": "require_once"
      }
    ],
    "File": "/home/nickv/Nextcloud/25/server/lib/private/AppFramework/Http/Dispatcher.php",
    "Line": 165,
    "Previous": {
      "Exception": "TypeError",
      "Message": "OCA\\Talk\\Config::validateSignalingTicket(): Argument #2 ($ticket) must be of type string, null given, called in /home/nickv/Nextcloud/25/server/appsbabies/spreed/lib/Controller/SignalingController.php on line 523",
      "Code": 0,
      "Trace": [
        {
          "file": "/home/nickv/Nextcloud/25/server/appsbabies/spreed/lib/Controller/SignalingController.php",
          "line": 523,
          "function": "validateSignalingTicket",
          "class": "OCA\\Talk\\Config",
          "type": "->"
        },
        {
          "file": "/home/nickv/Nextcloud/25/server/appsbabies/spreed/lib/Controller/SignalingController.php",
          "line": 502,
          "function": "backendAuth",
          "class": "OCA\\Talk\\Controller\\SignalingController",
          "type": "->"
        },
        {
          "file": "/home/nickv/Nextcloud/25/server/lib/private/AppFramework/Http/Dispatcher.php",
          "line": 225,
          "function": "backend",
          "class": "OCA\\Talk\\Controller\\SignalingController",
          "type": "->"
        },
        {
          "file": "/home/nickv/Nextcloud/25/server/lib/private/AppFramework/Http/Dispatcher.php",
          "line": 133,
          "function": "executeController",
          "class": "OC\\AppFramework\\Http\\Dispatcher",
          "type": "->"
        },
        {
          "file": "/home/nickv/Nextcloud/25/server/lib/private/AppFramework/App.php",
          "line": 172,
          "function": "dispatch",
          "class": "OC\\AppFramework\\Http\\Dispatcher",
          "type": "->"
        },
        {
          "file": "/home/nickv/Nextcloud/25/server/lib/private/Route/Router.php",
          "line": 298,
          "function": "main",
          "class": "OC\\AppFramework\\App",
          "type": "::"
        },
        {
          "file": "/home/nickv/Nextcloud/25/server/ocs/v1.php",
          "line": 62,
          "function": "match",
          "class": "OC\\Route\\Router",
          "type": "->"
        },
        {
          "file": "/home/nickv/Nextcloud/25/server/ocs/v2.php",
          "line": 23,
          "args": [
            "/home/nickv/Nextcloud/25/server/ocs/v1.php"
          ],
          "function": "require_once"
        }
      ],
      "File": "/home/nickv/Nextcloud/25/server/appsbabies/spreed/lib/Config.php",
      "Line": 499
    },
    "CustomMessage": "--"
  }
}

But after visiting the admin settings it worked fine (I guess due to welcome page or something). I guess we need to either run a setup step somewhere or make sure the necessary keys are generated on all paths

nickvergessen

Looks good apart from the error in the upgrade scenario

Signed-off-by: Joachim Bauch <[email protected]>

fancycode · 2022-08-03T07:16:44Z

Could that have been a caching issue of the JavaScript? I noticed the old-style ticket parameter was missing in the signaling settings which could result in old clients sending a null ticket. This is now fixed with e6c05ed

I'll try to reproduce this myself locally.

nickvergessen · 2022-08-03T09:09:56Z

Could that have been a caching issue of the JavaScript?

Could be as well.

fancycode · 2022-08-03T09:16:11Z

Did you also see an error like Undefined index: ticket at /path/to/apps/spreed/lib/Controller/SignalingController.php#517? With the old JS and the PHP from this PR, I get the Undefined index and the error you were getting.

fancycode · 2022-08-03T09:17:18Z

But with the latest commits here it's also working with the old JS.

nickvergessen · 2022-08-03T09:27:02Z

Undefined index: ticket at /path/to/apps/spreed/lib/Controller/SignalingController.php#517

Not found in my log file.

fancycode · 2022-08-03T14:57:06Z

I'll try to reproduce this myself locally.

I tried with a new setup (new DB):

installed Talk before this PR (076db31)
Built JS (make buid-js).
Switched to 39715df from this PR (before my two last changes).
Still using old JS.
Got the error Argument 2 passed to OCA\\Talk\\Config::validateSignalingTicket() must be of the type string, null given you saw, but also Undefined index: ticket.
Switched to fe58d16
Still using old JS.
Restart signaling server to make sure new capabilites with the public key for the JWT token are fetched on next client connection (*).
No longer got errors, connected without JWT.
Built JS (make buid-js) and reload (cache disabled).
Connected with JWT.

So, I can't reproduce the problem you had with the latest changes to this PR. If you still do, please provide the necessary steps.

(*) Thinking about it, I should detect this case in the signaling server and expire the capabilites in this case so no restart is required. But this should not block this PR.

fancycode · 2022-08-03T15:17:09Z

(*) Thinking about it, I should detect this case in the signaling server and expire the capabilites in this case so no restart is required. But this should not block this PR.

Done: strukturag/nextcloud-spreed-signaling@7496b09

nickvergessen · 2022-08-03T15:48:40Z

If you still do, please provide the necessary steps.

No, all good with your changes from yesterday evening

fancycode · 2022-08-04T07:10:48Z

strukturag/nextcloud-spreed-signaling#251 is merged. I'm planing a new release the next days or early next week.

nickvergessen

LGTM

To not break our test server (with incompatible Signaling server) we have to wait with the merge until you released

lib/Capabilities.php

Signed-off-by: Joachim Bauch <[email protected]>

fancycode · 2022-08-04T09:11:58Z

To not break our test server (with incompatible Signaling server) we have to wait with the merge until you released

This should be fully backwards compatible. The new hello v2 auth is only used if the signaling server supports it.

nickvergessen · 2022-08-04T09:16:12Z

To not break our test server (with incompatible Signaling server) we have to wait with the merge until you released

This should be fully backwards compatible. The new hello v2 auth is only used if the signaling server supports it.

But you added it to isCompatibleSignalingServer which enforces it?
But now after checking it will just create a log entry and flag a missing update in the admin section. So should be fine. I thought that would make it not use the signaling server anymore.

fancycode · 2022-08-04T09:18:14Z

But you added it to isCompatibleSignalingServer which enforces it?

Yeah, I figured you might want to push users to update to reduce load on the Nextcloud server.

fancycode requested a review from nickvergessen June 10, 2022 07:36

fancycode mentioned this pull request Jun 10, 2022

Use JWT instead of requiring the HPB to check authentication of the user #7336

Closed

fancycode added 2. developing feature: signaling 📶 Internal and external signaling backends performance 🚀 labels Jun 10, 2022

fancycode force-pushed the jwt-auth branch from efba36a to f38205a Compare June 14, 2022 14:42

fancycode force-pushed the jwt-auth branch 2 times, most recently from 2361f30 to 798071c Compare July 6, 2022 14:57

fancycode added 2 commits July 7, 2022 12:00

Implement JWT auth for signaling connection.

79c1920

Signed-off-by: Joachim Bauch <[email protected]>

Autodetect if signaling server supports hello-v2.

fdd9a0d

Signed-off-by: Joachim Bauch <[email protected]>

fancycode force-pushed the jwt-auth branch from fc12208 to fdd9a0d Compare July 7, 2022 10:01

Add support for algorithm "EdDSA" and add tests.

39715df

Signed-off-by: Joachim Bauch <[email protected]>

fancycode added 3. to review and removed 2. developing labels Jul 7, 2022

fancycode marked this pull request as ready for review July 7, 2022 13:24

nickvergessen requested changes Aug 2, 2022

View reviewed changes

fancycode added 2 commits August 3, 2022 09:01

Return hello v1 ticket to maintain backwards compatibility.

e6c05ed

Signed-off-by: Joachim Bauch <[email protected]>

Document "helloAuthParams" in signaling settings.

fe58d16

Signed-off-by: Joachim Bauch <[email protected]>

nickvergessen reviewed Aug 4, 2022

View reviewed changes

lib/Capabilities.php Show resolved Hide resolved

nickvergessen requested a review from danxuliu August 4, 2022 09:06

nickvergessen added this to the 💚 Next Major (25) milestone Aug 4, 2022

Document "hello-v2-token-key" capability.

ad81347

Signed-off-by: Joachim Bauch <[email protected]>

nickvergessen approved these changes Aug 4, 2022

View reviewed changes

nickvergessen merged commit 91d3bd8 into master Aug 12, 2022

nickvergessen deleted the jwt-auth branch August 12, 2022 14:07

danxuliu mentioned this pull request Dec 2, 2022

Fix using signaling settings while being refetched #8427

Merged

fancycode mentioned this pull request Apr 6, 2023

'Ignore unknown event' error in browser console #9080

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement JWT auth for signaling connections (hello v2) #7472

Implement JWT auth for signaling connections (hello v2) #7472

fancycode commented Jun 10, 2022 •

edited

Loading

fancycode commented Jul 7, 2022

nickvergessen commented Aug 2, 2022

nickvergessen left a comment

fancycode commented Aug 3, 2022

nickvergessen commented Aug 3, 2022

fancycode commented Aug 3, 2022

fancycode commented Aug 3, 2022

nickvergessen commented Aug 3, 2022

fancycode commented Aug 3, 2022

fancycode commented Aug 3, 2022

nickvergessen commented Aug 3, 2022

fancycode commented Aug 4, 2022

nickvergessen left a comment

fancycode commented Aug 4, 2022

nickvergessen commented Aug 4, 2022

fancycode commented Aug 4, 2022

Implement JWT auth for signaling connections (hello v2) #7472

Implement JWT auth for signaling connections (hello v2) #7472

Conversation

fancycode commented Jun 10, 2022 • edited Loading

fancycode commented Jul 7, 2022

nickvergessen commented Aug 2, 2022

nickvergessen left a comment

Choose a reason for hiding this comment

fancycode commented Aug 3, 2022

nickvergessen commented Aug 3, 2022

fancycode commented Aug 3, 2022

fancycode commented Aug 3, 2022

nickvergessen commented Aug 3, 2022

fancycode commented Aug 3, 2022

fancycode commented Aug 3, 2022

nickvergessen commented Aug 3, 2022

fancycode commented Aug 4, 2022

nickvergessen left a comment

Choose a reason for hiding this comment

fancycode commented Aug 4, 2022

nickvergessen commented Aug 4, 2022

fancycode commented Aug 4, 2022

fancycode commented Jun 10, 2022 •

edited

Loading