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

Chromium recording #9174

Merged
merged 2 commits into from
Jul 10, 2023
Merged

Conversation

mishamosher
Copy link
Contributor

I've been having issues with Firefox in my network setup, as described in this issue:

Chromium-based web browsers do not have this issue.

This PR provides support for Chromium.

@Antreesy Antreesy requested a review from danxuliu March 27, 2023 09:46
@nickvergessen nickvergessen added this to the 🖤 Next Major (27) milestone Mar 27, 2023
@mishamosher mishamosher force-pushed the chromium-recording branch 2 times, most recently from 4c5a4ba to c97cb2f Compare March 28, 2023 10:44
Copy link
Member

@danxuliu danxuliu left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your contributions and sorry for the delay.

Besides the comments, could you rebase into latest master? In Nextcloud Talk we prefer rebasing the branch and force pushing it (--force-with-lease :-) ) rather than merging the master branch into the pull request branch.

Could you also squash the commits that update the copyright as well as the commits with the fixes into the commits that modified the files in first place, and the commits that remove the hardcoded browser setting and add the recording setting to the default configuration into the commit that adds the setting?

Thanks!

recording/server.conf.in Outdated Show resolved Hide resolved
recording/src/nextcloud/talk/recording/Participant.py Outdated Show resolved Hide resolved
recording/src/nextcloud/talk/recording/Participant.py Outdated Show resolved Hide resolved
recording/src/nextcloud/talk/recording/Participant.py Outdated Show resolved Hide resolved
recording/src/nextcloud/talk/recording/Participant.py Outdated Show resolved Hide resolved
@mishamosher
Copy link
Contributor Author

@danxuliu done with most (if not all) of the requested changes.

Copy link
Member

@danxuliu danxuliu left a comment

Choose a reason for hiding this comment

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

Thanks!

I have left some minor nitpicks (sorry!), but I can do the changes myself if you want, just let me know :-)

recording/src/nextcloud/talk/recording/Participant.py Outdated Show resolved Hide resolved
recording/src/nextcloud/talk/recording/Participant.py Outdated Show resolved Hide resolved
recording/src/nextcloud/talk/recording/Participant.py Outdated Show resolved Hide resolved
options.add_argument('--kiosk')
options.add_argument(f'--window-size={width},{height}')
options.add_argument('--disable-infobars')
options.add_argument('--no-sandbox')
Copy link
Member

Choose a reason for hiding this comment

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

While I trust Talk :-) I would prefer not to disable the sandbox. Why is that needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was unable to run Chromium in my test environment without it. ARM64 + Rocky Linux 9 + Docker.

Refs:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've been going down the rabbit hole for more than an hour and still can't make it work without disabling the sandbox. Is disabling the sandbox a no-go?

Copy link
Member

Choose a reason for hiding this comment

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

Mmmm, given that it implies disabling an important security feature and that it looks like something specific to your setup (at least in my case it worked without issues also in a Docker container, but on x86_64 rather than ARM64, maybe that is the problem), I would prefer not to disable it by default for everyone.

Have you tried to directly pass --no-sandbox to the chromium-browser executable inside your container instead? For example, setting CHROMIUM_FLAGS (I have not tested it, and from a quick look to /usr/bin/chromium-browser it seems that /etc/chromium-browser/default is not sourced, but it might be possible to do it with /etc/chromium-browser.d/default instead or something like that).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the question, but how did you run it in x86_64, under Docker? I'm having the same issue in my Fedora x86_64 install.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Finished installing Ubuntu 22.04 and Fedora 38. Both x86_64, and up-to-date.

Tried many, many workarounds (as long as they don't disable the isolation already provider by Docker), but I am still unable to start Chromium inside Docker with sandboxing enabled.

  File "/usr/local/lib/python3.8/dist-packages/nextcloud/talk/recording/Participant.py", line 474, in __init__
    self.seleniumHelper.startChrome(width, height, env)
  File "/usr/local/lib/python3.8/dist-packages/nextcloud/talk/recording/Participant.py", line 250, in startChrome
    self.driver = ChromeDriver(
  File "/usr/local/lib/python3.8/dist-packages/selenium/webdriver/chrome/webdriver.py", line 84, in __init__
    super().__init__(
  File "/usr/local/lib/python3.8/dist-packages/selenium/webdriver/chromium/webdriver.py", line 104, in __init__
    super().__init__(
  File "/usr/local/lib/python3.8/dist-packages/selenium/webdriver/remote/webdriver.py", line 286, in __init__
    self.start_session(capabilities, browser_profile)
  File "/usr/local/lib/python3.8/dist-packages/selenium/webdriver/remote/webdriver.py", line 378, in start_session
    response = self.execute(Command.NEW_SESSION, parameters)
  File "/usr/local/lib/python3.8/dist-packages/selenium/webdriver/remote/webdriver.py", line 440, in execute
    self.error_handler.check_response(response)
  File "/usr/local/lib/python3.8/dist-packages/selenium/webdriver/remote/errorhandler.py", line 245, in check_response
    raise exception_class(message, screen, stacktrace)
selenium.common.exceptions.WebDriverException: Message: unknown error: Chrome failed to start: crashed.
  (unknown error: DevToolsActivePort file doesn't exist)
  (The process started from chrome location /usr/bin/chromium-browser is no longer running, so ChromeDriver is assuming that Chrome has crashed.)
Stacktrace:
#0 0x55ddc2bd6b23 <unknown>
#1 0x55ddc28ebb21 <unknown>
#2 0x55ddc2915e9a <unknown>
#3 0x55ddc2910461 <unknown>
#4 0x55ddc2957b73 <unknown>
#5 0x55ddc29570af <unknown>
#6 0x55ddc294da23 <unknown>
#7 0x55ddc291c662 <unknown>
#8 0x55ddc291dd7e <unknown>
#9 0x55ddc2c03c73 <unknown>
#10 0x55ddc2bb9dde <unknown>
#11 0x55ddc2bb97b0 <unknown>
#12 0x55ddc2bba5f5 <unknown>
#13 0x55ddc2bffabb <unknown>
#14 0x55ddc2bba9ae <unknown>
#15 0x55ddc2b9a9a4 <unknown>
#16 0x55ddc2bc5728 <unknown>
#17 0x55ddc2bc58d5 <unknown>
#18 0x55ddc2bd08ff <unknown>
#19 0x7f4303900609 start_thread

Copy link
Member

Choose a reason for hiding this comment

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

I am not aware of having modifed anything in my Docker configuration to disable the isolation... but I will investigate. This is really weird 🤔

But it is unlikely that I will be able to do that this week, sorry :-(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to be clear, I've used these ISOs:

...then went ahead for a minimal install in a fresh VM, updated the system (apt/dnf), and followed the Install Docker Engine instructions. Afterwards, practically the same exact steps that you've described.

Copy link
Member

Choose a reason for hiding this comment

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

After testing with a Ubuntu 22.04 LiveUSB I have been able to reproduce the issue (not being able to run Chromium without --no-sandbox in a Docker container due to the Failed to move to new namespace error). I guess that at some point I modified some defaults for Docker in my development machine and forgot to revert the changes. Sorry for the noise.

However, I am still against launching Chromium with --no-sandbox from the recording server. The recording server can be run in a Docker container, but other container technologies might have different defaults that would allow a sandboxed Chromium to run, and in any case it can also be run directly on the host, without any container. The sandbox is a security feature of Chromium, and disabling it is needed to run it in a Docker container, but it is not a limitation imposed by the recording server itself (like other arguments like starting in kiosk mode). Therefore, the recording server should not disable the sandbox; working around that should be done in Docker instead.

It seems that the default seccomp of the Docker container could be overriden to allow some extra system calls needed by Chromium in sandboxed mode. The linked profile seems to be a bit old, so maybe some of the additional syscalls are no longer needed.

Another alternative seems to be enabling user namespace cloning in the kernel. Unlike the previous option I guess that this one affects all the containers rather than just the specific container where Chromium will be run, so it might be less recommended (but unfortunately I am not familiar enough with these security features to weigh that).

Finally, the alternative used by the Docker images for Selenium is --no-sandbox. However, this is done at the container level, by adding a wrapper script that runs Chromium with that option by default, so the Selenium code does not need to worry about that.

The docker-compose for the recording server could include a note about having to use one of these alternatives to run Chromium. I guess it would be even fine to apply one of the alternatives (either the seccomp profile or the wrapper to add --no-sandbox, although unfortunately I am not sure which one would be better from a security point of view), given that by default it would not be possible to run Chromium in Docker otherwise.

But independently of that --no-sandbox should be removed from Participant.py; for me that (and fixing the typo I have just seen :-) ) is the only remaining thing needed to merge this pull request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it.

@mishamosher mishamosher force-pushed the chromium-recording branch 2 times, most recently from 6a71d3a to 9cb4042 Compare April 27, 2023 10:47
Signed-off-by: Elmer Miroslav Mosher Golovin <miroslav@mishamosher.com>
Signed-off-by: Elmer Miroslav Mosher Golovin <miroslav@mishamosher.com>
@mishamosher mishamosher force-pushed the chromium-recording branch 2 times, most recently from 4662a21 to d0ae757 Compare May 16, 2023 14:41
@mishamosher
Copy link
Contributor Author

@danxuliu done with the changes. Hope this time the PR can be merged (:

Now that you mention the docker PR (#9177) and suggestions about the chromium sandbox, should I add a workaround to it? If positive, does a --no-sandbox wrapper sound OK?

Copy link
Member

@danxuliu danxuliu 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 👍

Thanks a lot again for your contributions and sorry for the long delay, I have been out of the office for several weeks and I forgot to merge before leaving 🤦

Now that you mention the docker PR (#9177) and suggestions about the chromium sandbox, should I add a workaround to it? If positive, does a --no-sandbox wrapper sound OK?

Yes, I think that the workaround should be added to the Docker compose file (a little comment explaining why would be great too :-) ), and a --no-sandbox wrapper sounds good :-) Thanks!

@danxuliu danxuliu merged commit 5d37045 into nextcloud:master Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants