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

Dont run browser as root #5058

Closed

Conversation

halfline
Copy link
Contributor

@halfline halfline commented Aug 20, 2023

Right now firefox gets run as root which it really doesn't like doing. We have a workaround in place (unset XAUTHORITY) to prevent it from even erroring on start up.

There's no good reason to run it as root though. We can just run it unprivileged since all the heavy lifting is in privileged backend code anyway.

@@ -66,21 +66,23 @@ esac

# prepare empty firefox profile dir with theme based on the passed profile id
FIREFOX_THEME_DIR="/usr/share/anaconda/firefox-theme"
FIREFOX_PROFILE_PATH="/tmp/anaconda-firefox-profile"
LIVEUSER=$(id -n -u $PKEXEC_UID)
Copy link
Contributor

Choose a reason for hiding this comment

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

So this same script is also used on the boot.iso to run the Web UI there & I don't think there is a live user account available there. I'll try to build a boot.iso with this PR & also a Live image when I'm at it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should be okay, if $PKEXEC_UID is unset LIVEUSER should just get set to the current running user.

Maybe the variable LIVEUSER could have a better name (INSTALLER_USER?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

of course that means firefox continues to run as root on boot.iso, which is less than ideal, but it shouldn't complain in that case, because it won't think it's a user elevated to root, but root all the way through

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think renaming the variable would be best, plus maybe a comment we expect it to be liveuser on live?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think renaming the variable would be best, plus maybe a comment we expect it to be liveuser on live?

Agreed with both.

@M4rtinK
Copy link
Contributor

M4rtinK commented Aug 21, 2023

/boot-iso --webui

ui/webui/webui-desktop Fixed Show resolved Hide resolved
ui/webui/webui-desktop Fixed Show resolved Hide resolved
@github-actions
Copy link

boot.iso built successfully based on commit b3cd860. Download it from the bottom of the job status page.

@VladimirSlavik
Copy link
Contributor

/kickstart-test --testtype smoke

@VladimirSlavik
Copy link
Contributor

So, I triggered the tests and it looks like something did not work for webui...

machine_core.exceptions.Failure: SSH master process exited with code: 255

@halfline
Copy link
Contributor Author

So, I triggered the tests and it looks like something did not work for webui...

machine_core.exceptions.Failure: SSH master process exited with code: 255

hmm i just tried it locally again, and it seems to work fine here. do you have more logs ?

@halfline
Copy link
Contributor Author

note this pull request will need some changes if #5057 lands first

(we'll need to stop unsetting XAUTHORITY and XDG_RUNTIME_DIR in webui-desktop)

@halfline halfline force-pushed the dont-run-browser-as-root branch from d77473c to 75fedfc Compare August 31, 2023 15:40
@halfline
Copy link
Contributor Author

i've now rebased this on top of #5057 so let's not land this until it lands.

@halfline halfline mentioned this pull request Sep 5, 2023
readarray -t user_environment < <(pkexec --user "${INSTALLER_USER}" env XDG_RUNTIME_DIR="/run/user/${PKEXEC_UID}" systemctl --user show-environment)

for variable in "${user_environment[@]}"; do
export "$variable"
Copy link
Contributor

@VladimirSlavik VladimirSlavik Sep 6, 2023

Choose a reason for hiding this comment

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

Shellcheck says:

data/liveinst/liveinst:34:16: warning: This does not export 'variable'. Remove $/${} for that, or use ${var?} to quiet. [SC2163]

If you know what you're doing (you do), alternatively, # shellcheck disable=SC2163

@M4rtinK
Copy link
Contributor

M4rtinK commented Sep 6, 2023

I strongly suggest we only merge this after we are done with F39 Beta, as I don't think we will realistically have the time to properly test this and fix any possible breakage.

@M4rtinK M4rtinK force-pushed the dont-run-browser-as-root branch from 75fedfc to cb62a85 Compare September 18, 2023 16:50
readarray -t user_environment < <(pkexec --user "${INSTALLER_USER}" env XDG_RUNTIME_DIR="/run/user/${PKEXEC_UID}" systemctl --user show-environment)

for variable in "${user_environment[@]}"; do
export "$variable"

Check warning

Code scanning / shellcheck

This does not export 'variable'. Remove $/${} for that, or use ${var?} to quiet. Warning

This does not export 'variable'. Remove $/${} for that, or use ${var?} to quiet.
@M4rtinK
Copy link
Contributor

M4rtinK commented Sep 18, 2023

Rebased on top of latest master & pushed the updated version.

@M4rtinK
Copy link
Contributor

M4rtinK commented Sep 18, 2023

/kickstart-test --testtype smoke

@M4rtinK
Copy link
Contributor

M4rtinK commented Sep 18, 2023

/build-image --live

@M4rtinK
Copy link
Contributor

M4rtinK commented Sep 18, 2023

/boot-iso --webui

@M4rtinK
Copy link
Contributor

M4rtinK commented Sep 18, 2023

@halfline Just a heads up that I had to rewrite the first commit message a bit as some of the changes I've previously cherry-picked to the Firefox styling PR that has been merged a while ago. :)

@github-actions
Copy link

Images built based on commit cb62a85:

  • Live: success

Download the images from the bottom of the job status page.

@M4rtinK
Copy link
Contributor

M4rtinK commented Sep 19, 2023

/boot-iso --webui

@M4rtinK
Copy link
Contributor

M4rtinK commented Sep 19, 2023

/build-image --boot.iso --webui

@M4rtinK
Copy link
Contributor

M4rtinK commented Oct 23, 2023

/build-image --boot.iso --live --webui

@github-actions
Copy link

Images built based on commit 28ea45b:

  • boot.iso: success

  • Live: failure

Download the images from the bottom of the job status page.

@M4rtinK
Copy link
Contributor

M4rtinK commented Oct 23, 2023

/kickstart-test --testtype smoke

@M4rtinK
Copy link
Contributor

M4rtinK commented Oct 23, 2023

/build-image --live

@github-actions
Copy link

Images built based on commit 28ea45b:

  • Live: success

Download the images from the bottom of the job status page.

@jkonecny12
Copy link
Member

Hi all, I was just pinged by Red Hatters who tested accessibility of web UI. They found that our solution doesn't work because root applications don't have access to accessibility DBus. So, I think this information is raising a priority to merge this.

@KKoukiou
Copy link
Contributor

KKoukiou commented Nov 7, 2023

Hi all, I was just pinged by Red Hatters who tested accessibility of web UI. They found that our solution doesn't work because root applications don't have access to accessibility DBus. So, I think this information is raising a priority to merge this.

I don't see how org.a11y.Bus is involved here. Can you ask for some more insights? Is the testing of the accessibility prohibited or the accessibility of the new UI itself is hindered? I am assuming it's the former?

@tyrylu
Copy link

tyrylu commented Nov 7, 2023

Before this MR, unfortunately the latter. I have no idea how a root process can even find the correct a11y bus. And I can't test with a test image without building a new one of this PR, as they're all expired now.

@VladimirSlavik
Copy link
Contributor

/build-image --live

Copy link

github-actions bot commented Nov 7, 2023

Images built based on commit 28ea45b:

  • Live: failure

Download the images from the bottom of the job status page.

Copy link

github-actions bot commented Jan 7, 2024

This PR is stale because it has been open 60 days with no activity.
Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the stale label Jan 7, 2024
@tyrylu
Copy link

tyrylu commented Jan 8, 2024

Could we push this forward, please? From the perspective of a11y, we'd have to do a huge and ugly environment variables hack to tell the browser about the unprivileged a11y bus, or we do it cleanly.

@KKoukiou
Copy link
Contributor

KKoukiou commented Jan 8, 2024

@tyrylu thanks for ping. I added this in our current quarter planning. I see the current pr is terribly outdated, it needs a nontrivial amount of work.

@tyrylu
Copy link

tyrylu commented Jan 8, 2024

Thanks for that. But, if I could set the criteria, when the web UI is the default (no idea when that's planned), and this is not resolved, I'd call it a critical regression, of course, only for the users needing Orca, I know...

Copy link

This PR is stale because it has been open 60 days with no activity.
Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the stale label Apr 30, 2024
@tyrylu
Copy link

tyrylu commented Apr 30, 2024

Alright, now, F41 has the web-ui installer, and this situation is not resolved, so, we have a return to the dark ages where a visually impaired person could not install an OS...

@github-actions github-actions bot removed the stale label May 1, 2024
@jkonecny12
Copy link
Member

We are currently working on even keep it there. It's still not accepted by FESCO. We definitely want to add this, however first we need to resolve how to keep Web UI in Fedora 😔

Copy link

github-actions bot commented Jul 7, 2024

This PR is stale because it has been open 60 days with no activity.
Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the stale label Jul 7, 2024
Copy link

github-actions bot commented Aug 7, 2024

This PR was closed because it has been stalled for 30 days with no activity.

@github-actions github-actions bot closed this Aug 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

6 participants