spreed icon indicating copy to clipboard operation
spreed copied to clipboard
verified publisher icon nextcloud

Implement JWT auth for signaling connections (hello v2)

Open fancycode opened this issue 3 years ago • 14 comments

See https://github.com/nextcloud/spreed/issues/7336

  • Needs https://github.com/nextcloud/spreed/pull/7372
  • Needs https://github.com/strukturag/nextcloud-spreed-signaling/pull/251
  • [x] Autodetect if signaling server support hello-v2
  • [x] Add tests

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

fancycode avatar Jun 10 '22 07:06 fancycode

@nickvergessen this is now ready for review

fancycode avatar Jul 07 '22 13:07 fancycode

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 avatar Aug 02 '22 15:08 nickvergessen

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 https://github.com/nextcloud/spreed/pull/7472/commits/e6c05edc1e1d1b850dc0280fb6b979f1517c1c0f

I'll try to reproduce this myself locally.

fancycode avatar Aug 03 '22 07:08 fancycode

Could that have been a caching issue of the JavaScript?

Could be as well.

nickvergessen avatar Aug 03 '22 09:08 nickvergessen

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 avatar Aug 03 '22 09:08 fancycode

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

fancycode avatar Aug 03 '22 09:08 fancycode

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

Not found in my log file.

nickvergessen avatar Aug 03 '22 09:08 nickvergessen

I'll try to reproduce this myself locally.

I tried with a new setup (new DB):

  • installed Talk before this PR (076db311466bd60f4d4c8f58632aa7de0a3dd601)
  • Built JS (make buid-js).
  • Switched to https://github.com/nextcloud/spreed/pull/7472/commits/39715df9b0fd0128a71f7b1fd21dfc186fe40857 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 https://github.com/nextcloud/spreed/pull/7472/commits/fe58d16629faf43cbc2c6521d10caeb5e76ccb0c
  • 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 avatar Aug 03 '22 14:08 fancycode

(*) 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: https://github.com/strukturag/nextcloud-spreed-signaling/pull/251/commits/7496b09f3ead0700079311035b8b2d77df95ebf4

fancycode avatar Aug 03 '22 15:08 fancycode

If you still do, please provide the necessary steps.

No, all good with your changes from yesterday evening

nickvergessen avatar Aug 03 '22 15:08 nickvergessen

https://github.com/strukturag/nextcloud-spreed-signaling/pull/251 is merged. I'm planing a new release the next days or early next week.

fancycode avatar Aug 04 '22 07:08 fancycode

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.

fancycode avatar Aug 04 '22 09:08 fancycode

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.

nickvergessen avatar Aug 04 '22 09:08 nickvergessen

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 avatar Aug 04 '22 09:08 fancycode