Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

talk-recording - set allow_all and skip_verify via env #2880

Merged
merged 3 commits into from
Jul 20, 2023

Conversation

szaimen
Copy link
Collaborator

@szaimen szaimen commented Jun 27, 2023

No description provided.

@szaimen szaimen added the 2. developing Work in progress label Jun 27, 2023
@szaimen szaimen added this to the next milestone Jun 27, 2023
@szaimen szaimen requested a review from SystemKeeper June 27, 2023 09:00
@szaimen szaimen added 3. to review Waiting for reviews enhancement New feature or request and removed 2. developing Work in progress labels Jun 27, 2023
@szaimen szaimen marked this pull request as ready for review June 27, 2023 11:15
@Zoey2936
Copy link
Collaborator

can I ask for a reason?

@szaimen
Copy link
Collaborator Author

szaimen commented Jun 27, 2023

can I ask for a reason?

@SystemKeeper asked me if he could reuse the container in @juliushaertl docker-dev. For that he needs to adjust these values in order to run it locally.

@szaimen szaimen modified the milestones: v6.3.0, next Jul 14, 2023
@szaimen szaimen requested a review from juliusknorr July 16, 2023 22:53
Copy link

@SystemKeeper SystemKeeper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No sorry this is not working like this, I’ll add some more details later

@SystemKeeper
Copy link

So, back again. Again, very sorry for not finding the time earlier and now rushing into this :-(.

I noticed 2 things here I wanted to discuss:

  1. Currently we have the secret set under backend with a TODO to remove it, after a issue in the recording server is fixed
    This is now a problem, because when we use ALLOW_ALL we need to have this secret.

  2. Right now we expose the signaling server directly, that is without a directory. We introduced the protocol and the domain separately, but that still wouldn't work, because for the dev environment we don't have the /standalone-signaling/ path.

So what I thought was to do something like this:

diff --git a/Containers/talk-recording/start.sh b/Containers/talk-recording/start.sh
index 991ea69..f38a7a9 100644
--- a/Containers/talk-recording/start.sh
+++ b/Containers/talk-recording/start.sh
@@ -12,6 +12,15 @@ elif [ -z "$INTERNAL_SECRET" ]; then
     exit 1
 fi
 
+if [ -z "$SIGNALING_URL" ]; then
+    SIGNALING_URL="https://${NC_DOMAIN}/standalone-signaling/"
+fi
+
+# TODO: Enable if-clause below when https://github.com/nextcloud/spreed/issues/9580 is fixed
+#if [ "$ALLOW_ALL" = true ]; then
+    ALLOW_ALL_SECRET="secret = ${RECORDING_SECRET}"
+#fi
+
 cat << RECORDING_CONF > "/etc/recording.conf"
 [logs]
 # 30 means Warning
@@ -22,8 +31,7 @@ listen = 0.0.0.0:1234
 
 [backend]
 allowall = ${ALLOW_ALL}
-# TODO: remove secret below when https://github.com/nextcloud/spreed/issues/9580 is fixed
-secret = ${RECORDING_SECRET}
+${ALLOW_ALL_SECRET}
 backends = backend-1
 skipverify = ${SKIP_VERIFY}
 maxmessagesize = 1024
@@ -32,7 +40,7 @@ videoheight = 1080
 directory = /tmp
 
 [backend-1]
-url = ${HPB_PROTOCOL}://${NC_DOMAIN}
+url = https://${NC_DOMAIN}
 secret = ${RECORDING_SECRET}
 skipverify = ${SKIP_VERIFY}
 
@@ -40,7 +48,7 @@ skipverify = ${SKIP_VERIFY}
 signalings = signaling-1
 
 [signaling-1]
-url = ${HPB_PROTOCOL}://${NC_DOMAIN}/standalone-signaling/
+url = ${SIGNALING_URL}
 internalsecret = ${INTERNAL_SECRET}
 
 [ffmpeg]

For the first issue we just check if ALLOW_ALL is set and then additionally add the secret under backend. Right now the if-clause is commented, because of the issue mentioned above.

For the second issue we check if a SIGNALING_URL is provided and if not, we construct it as it was before. This way we could remove the HPB_PROTOCOL again. Alternatively we could of course just add a HPB_PATH as a variable.

@szaimen
Copy link
Collaborator Author

szaimen commented Jul 18, 2023

Alternatively we could of course just add a HPB_PATH as a variable.

I guess this would be the simplest approach then.

@szaimen szaimen requested a review from SystemKeeper July 18, 2023 08:01
Copy link

@SystemKeeper SystemKeeper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and works! Thank you very much!

We need to keep in mind that the TODO about the global secret is not true anymore when we set ALLOW_ALL to true.

Also, it does not work with latest master of talk, because of nextcloud/spreed#9940 (but no backport yet).

szaimen added 3 commits July 20, 2023 15:52
Signed-off-by: Simon L <szaimen@e.mail.de>
Signed-off-by: Simon L <szaimen@e.mail.de>
Signed-off-by: Simon L <szaimen@e.mail.de>
@szaimen szaimen force-pushed the enh/noid/talk-recording-allow-all branch from badabe0 to a262d1f Compare July 20, 2023 13:52
@szaimen szaimen merged commit a51ad39 into main Jul 20, 2023
@delete-merged-branch delete-merged-branch bot deleted the enh/noid/talk-recording-allow-all branch July 20, 2023 13:53
@szaimen
Copy link
Collaborator Author

szaimen commented Jul 21, 2023

This is now released with v6.4.0 Beta. Testing and feedback is welcome! See https://github.com/nextcloud/all-in-one#how-to-switch-the-channel

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants