sudo: Rework it for the local machine #13482
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mvollmer
force-pushed
the
sudo-rework-local
branch
from
1c282be
to
7db1974
Compare
mvollmer
added
the
no-test
mvollmer
force-pushed
the
sudo-rework-local
branch
5 times, most recently
from
4debcff
to
174fd31
Compare
mvollmer
force-pushed
the
sudo-rework-local
branch
8 times, most recently
from
f55166d
to
bd58d94
Compare
|
@garrett I know we discussed some changes to these designs, but I just didn't have time to look into that yet. |
mvollmer
force-pushed
the
sudo-rework-local
branch
13 times, most recently
from
70e8e2d
to
120070c
Compare
This will be used to track startup of superuser bridges.
Bridges without "match" entries are now never started implicitly. Superuser bridges will be started explicitly in the next commit.
The bridge now exposes a new D-Bus API for explicitly starting and stopping superuser bridges. Arbitrary prompts from sudo or pkexec are routed back to the caller of that API and can be answered interactively. Only the "org.cockpit-project.cockpit.root-bridge" PolKit action is now supported. Cockpit doesn't have UI for general PolKit prompts, and we control everything by either being "root" or not.
This is done with an addition to the "init" message to the bridge for new enough bridges. We keep using the good old "authorize" message for the password instead of including it in the "init" message. We additionally send a "init-done" message to let cockpit-ws know when it can poison the creds. The point of this is to avoid introducing new places where a password might travel or be stored, but at the same time making it clear that the password is only needed once during initialization.
mvollmer
force-pushed
the
sudo-rework-local
branch
2 times, most recently
from
6f8a1a9
to
cf00d51
Compare
These use the new D-Bus API to start a superuser bridge while holding a conversation with the user. The button is either in the Shell (for the local machine), or in the Overview (for remote machines on the dashboard).
A page that uses cockpit.permission will now be forcefully reloaded when the superuser level changes.
Hide buttons for privileged tasks when appropriate. This also introduces lib/superuser.jsx for monitoring, and automatic page reloads. The plan is to migrate all pages away from cockpit.permission to lib/superuser.jsx eventually.
Configuration links now look like normal text when disabled. We don't need to watch the hostname as superuser, and then we don't need to reload the page when the superuser level changes.
This way people don't have to explicitly choose which one to use. We prefer sudo since that is what people use on the command line.
mvollmer
force-pushed
the
sudo-rework-local
branch
from
cf00d51
to
a6f27d9
Compare
mvollmer
force-pushed
the
sudo-rework-local
branch
from
a6f27d9
to
2dc0738
Compare
|
Just some common machines flakes left, nice! |
martinpitt
approved these changes
May 6, 2020
There was a problem hiding this comment.
I tested this again, and didn't find any obvious shortcomings any more. The PCP enablement button looks enabled (but isn't) in unprivileged mode, but that's unrelated. I also went over the commits again; while there's still a lot of small nitpicks, none of them are important enough to hold this up any longer. Go from my side! 🎉
The multi-machine failures on fedora-31/firefox look a bit suspicious... let's retry. |
|
I guess this could wrapper better? :) |
Separate issue, please! |
Yes.
Agreed. I think this is a PF4-related issue, actually. We can easily fix it, but it should be shoved upstream too. (But we'll work on it in another PR.) |
1 task
martinpitt
pushed a commit
to allisonkarlitskaya/cockpit
that referenced
this pull request
Feb 25, 2021
This field was introduced in cockpit-project#4964 as "purely informational for now", and isn't even parsed by cockpit. Meanwhile, it's one of the few things preventing us from untangling our webpack build from autotools. Even within cockpit, usage is inconsistent: most packages set "version" to the string that @Version@ expands to, but machines had it set to the integer 0, and pcp and static had no "version" field at all. There are only two places where this was being used: - Login pages before commit 4511d83 check the remote base1 manifest version to decide if it supports remote URLs. This version is still supported for a few years in RHEL/CentOS 7, so for now we can't remove base1's version field. But this only functions as an "API compatibility level" field: it does not need to accurately follow the release numbers, it just needs to be bumped on incompatible changes. The last one was the sudo authentication rework in PR cockpit-project#13482 in version 219, so set version to that and add a `version-note:` field (in lieu of a comment) that explains the number. - The base1/test-http.js unit test: this directly reads the manifest.json.in file from the playground and does a direct comparison on it. That's been adjusted accordingly. Thus, drop all version fields except base1's.
allisonkarlitskaya
added a commit
that referenced
this pull request
Mar 1, 2021
This field was introduced in #4964 as "purely informational for now", and isn't even parsed by cockpit. Meanwhile, it's one of the few things preventing us from untangling our webpack build from autotools. Even within cockpit, usage is inconsistent: most packages set "version" to the string that @Version@ expands to, but machines had it set to the integer 0, and pcp and static had no "version" field at all. There are only two places where this was being used: - Login pages before commit 4511d83 check the remote base1 manifest version to decide if it supports remote URLs. This version is still supported for a few years in RHEL/CentOS 7, so for now we can't remove base1's version field. But this only functions as an "API compatibility level" field: it does not need to accurately follow the release numbers, it just needs to be bumped on incompatible changes. The last one was the sudo authentication rework in PR #13482 in version 219, so set version to that and add a `version-note:` field (in lieu of a comment) that explains the number. - The base1/test-http.js unit test: this directly reads the manifest.json.in file from the playground and does a direct comparison on it. That's been adjusted accordingly. Thus, drop all version fields except base1's.
thomasvandenbosch13
pushed a commit
to thomasvandenbosch13/cockpit
that referenced
this pull request
Apr 12, 2021
This field was introduced in cockpit-project#4964 as "purely informational for now", and isn't even parsed by cockpit. Meanwhile, it's one of the few things preventing us from untangling our webpack build from autotools. Even within cockpit, usage is inconsistent: most packages set "version" to the string that @Version@ expands to, but machines had it set to the integer 0, and pcp and static had no "version" field at all. There are only two places where this was being used: - Login pages before commit 4511d83 check the remote base1 manifest version to decide if it supports remote URLs. This version is still supported for a few years in RHEL/CentOS 7, so for now we can't remove base1's version field. But this only functions as an "API compatibility level" field: it does not need to accurately follow the release numbers, it just needs to be bumped on incompatible changes. The last one was the sudo authentication rework in PR cockpit-project#13482 in version 219, so set version to that and add a `version-note:` field (in lieu of a comment) that explains the number. - The base1/test-http.js unit test: this directly reads the manifest.json.in file from the playground and does a direct comparison on it. That's been adjusted accordingly. Thus, drop all version fields except base1's.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Release notes
Cockpit now allows more control over your access level. You can gain
administrative access in a running session, and Cockpit will remember
that for the next session.
https://www.youtube.com/watch?v=-JDMe015m-k
Cockpit respects the "sudo" configuration for your account. For
example, when
NOPASSWDis in your configuration, you don't have toprovide a password in Cockpit to gain administrative access, and if
"sudo" is configured for two factor authentication, Cockpit will ask
twice.
Cockpit only supports "sudo" to elevate the access level. If your
custom Cockpit page relies on Polkit to grant access, you now need to
use the
superuseroption for the relevant channels. For example,using the UDisks2 D-Bus API now has to look like this:
We plan to bring Polkit support back shortly, in enhanced form.
FOLLOWUP
cockpit.permission. Uselib/superuser. Controls should be hidden instead of disabled. No tooltips re permissions.