Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
daemon: add polkit support to /v2/login #3581
Conversation
codecov-io
commented
Jul 12, 2017
•
Edited 1 time
-
codecov-io
Jul 21, 2017
-
Jul 21, 2017
Codecov Report
@@ Coverage Diff @@
## master #3581 +/- ##
=========================================
- Coverage 75.86% 75.8% -0.06%
=========================================
Files 400 402 +2
Lines 34663 34744 +81
=========================================
+ Hits 26296 26338 +42
- Misses 6495 6532 +37
- Partials 1872 1874 +2
Continue to review full report at Codecov.
|
chipaca
reviewed
Jul 19, 2017
Code looks overall sane, but I wonder about the interface of the polkit package.
| if err == nil { | ||
| if uid == 0 { | ||
| // Superuser does anything. | ||
| return true | ||
| } | ||
| + if c.ActionID != "" { | ||
| + subject := polkit.ProcessSubject{Pid: int(pid)} | ||
| + if result, err := polkitCheckAuthorization(subject, c.ActionID, nil, polkit.CheckAuthorizationAllowUserInteraction); err == nil { |
chipaca
Jul 19, 2017
•
Edited 1 time
-
chipaca
Jul 19, 2017
Member
-
Jul 19, 2017
TBH I think I'd rather move make the polkit package handle all of this. That is, have something like a polkit.IsPidAuthorizedFor(int, string) (bool, error). Is there a reason you didn't go for that approach?
jhenstridge
Jul 19, 2017
Contributor
I think we'll probably want to retain the flags argument, and let the client decide whether interaction should be allowed (e.g. if you run a snap command from a cron job, you probably don't want interactive auth).
The details map is not particularly useful with the version of polkit we currently ship in Ubuntu, but does offer some value with more recent releases. If we protected the package install action with polkit and provided appropriate details, an administrator might be able to write a policy that prevented the user from installing snaps with classic confinement, for instance.
| + "http://www.freedesktop.org/standards/PolicyKit/1.0/policyconfig.dtd"> | ||
| +<policyconfig> | ||
| + | ||
| + <vendor>Snapcraft</vendor> |
jhenstridge
Jul 19, 2017
Contributor
I just adapted the policy file from snapd-glib, which had the same vendor value. I'm not even sure if this ever gets displayed to the user.
| +) | ||
| + | ||
| +// CheckAuthorization queries polkit to determine whether a subject is | ||
| +// authorized to perform an action. |
chipaca
Jul 19, 2017
Member
is there a reason for this not to return (bool, error) instead of something you need to talk to in a particular way to get the boolean answer you're looking for and is implied by the docstring?
jhenstridge
Jul 19, 2017
Contributor
I used the D-Bus API and existing C binding as a guide for the interface. I guess it depends on whether we want something that only covers our immediate use case, or something that can be reused.
The result struct is described here:
The main pieces of additional information are:
- if the user is authorised successfully, and policy allows that authorisation to be retained for a time, it provides an opaque ID that can be used to cancel the retained authorisation.
- if you ask for non-interactive auth, it can tell you that authorisation could succeed with interaction.
- differentiate between "access denied" and "user cancelled the auth dialog".
(2) and (3) look like they could be handled via custom errors. I'm not sure how important (1) is.
niemeyer
Aug 14, 2017
Contributor
Yes, I think 2 and 3 are better handled as errors, and for 1 we can introduce another entry point which allows handling the more complex case nicely (it will probably involve context.Context).
| +func CheckAuthorization(subject Subject, actionId string, details map[string]string, flags CheckAuthorizationFlags) (result AuthorizationResult, err error) { | ||
| + s, err := subject.serialize() | ||
| + if err != nil { | ||
| + return |
chipaca
Jul 19, 2017
Member
please don't use this style of returns, it makes the code harder to read as it grows larger. Return the explicit thing.
jhenstridge
added some commits
Jul 11, 2017
All checks have passed
|
I'm not sure how my changes could have caused the single spread test failure (linode:ubuntu-14.04-64:tests/main/econnreset) -- it doesn't even look like it uses snap login. Could it be a transient failure? |
|
@jhenstridge Yeah, the spread test failure is a nown unstable test, I restarted it. Things should be more robust if you merge in master. |
| @@ -77,25 +78,41 @@ type Command struct { | ||
| // is this path accessible on the snapd-snap socket? | ||
| SnapOK bool | ||
| + // can polkit grant access? | ||
| + ActionID string |
niemeyer
Aug 14, 2017
Contributor
Given the pattern established above, I suggest this for more clarity
// can polkit grant admin access? set to polkit action ID if so
PolkitOK string
| + | ||
| + <action id="io.snapcraft.snapd.login"> | ||
| + <description>Add a Snap store account</description> | ||
| + <message>Authentication is required to add a Snap store account to the system</message> |
niemeyer
Aug 14, 2017
Contributor
Perhaps:
<description>Authenticate on snap daemon</description>
<message>Authorization is required to authenticate on the snap daemon</message>
This seems more inline with the idea of "login" there.
| + "github.com/godbus/dbus" | ||
| +) | ||
| + | ||
| +type CheckAuthorizationFlags uint32 |
niemeyer
Aug 14, 2017
Contributor
Would be nice to s/Authorization/Auth/ on all strings here, to save some reading and typing.
| + | ||
| +const ( | ||
| + CheckAuthorizationNone CheckAuthorizationFlags = 0x00 | ||
| + CheckAuthorizationAllowUserInteraction CheckAuthorizationFlags = 0x01 |
niemeyer
Aug 14, 2017
Contributor
This one feels over the top. Perhaps we can use CheckFlags, CheckNone, and CheckAllowInteraction here.
| + | ||
| +var ( | ||
| + ErrDismissed = errors.New("Authorization request dismissed") | ||
| + ErrInteractionRequired = errors.New("Authorization requires interaction") |
niemeyer
Aug 14, 2017
Contributor
This might also be just ErrInteraction. polkit.ErrInteraction sounds nice.
| + if result.IsChallenge { | ||
| + err = ErrInteractionRequired | ||
| + } else if result.Details["polkit.dismissed"] != "" { | ||
| + err = ErrDismissed |
| +// CheckAuthorizationForPid queries polkit to determine whether a process is | ||
| +// authorized to perform an action. | ||
| +func CheckAuthorizationForPid(pid uint32, actionId string, details map[string]string, flags CheckAuthorizationFlags) (bool, error) { | ||
| + subject := subject{ |
niemeyer
Aug 14, 2017
Contributor
Can we please not do type and variable with same name? Perhaps authSubject and authResult for the types?
niemeyer
changed the title from
Add polkit authorization support to /v2/login
to
daemon: add polkit support to /v2/login
Aug 14, 2017
Some checks were not successful
|
Thanks for the review. Most of the type and constant naming in the polkit module came directly from the D-Bus interface. I agree that the shorter versions you suggested make things clearer without adding ambiguity. |
Some checks were not successful
niemeyer
approved these changes
Aug 16, 2017
LGTM. Let's please just get the error messages fixed to conform to the usual conventions.
| + | ||
| +// CheckAuthorizationForPid queries polkit to determine whether a process is | ||
| +// authorized to perform an action. | ||
| +func CheckAuthorizationForPid(pid uint32, actionId string, details map[string]string, flags CheckFlags) (bool, error) { |
niemeyer
Aug 16, 2017
Contributor
I'd still prefer to rename "Authorization" to "Auth" here, but that's mostly an irrelevant nitpick considering this is most likely going to be used once over the whole code base.
jhenstridge
Aug 16, 2017
Contributor
The only nominal concern I can see with shortening it is confusion between authorisation and authentication. Maybe it doesn't matter here, but it seemed easier to just maintain the polkit's terminology here.
| + // processes trying to fool us | ||
| + idx := strings.IndexByte(contents, ')') | ||
| + if idx < 0 { | ||
| + return 0, fmt.Errorf("Error parsing file %s", filename) |
niemeyer
Aug 16, 2017
Contributor
"cannot parse %s"
Lowercase, and we try to standardize on "cannot" as the usual prefix for anything resembling that sort of message (instead of, say, "can not", or "unable to", or "could not", "error while", etc). All errors are also prefixed by "error: " on the way out into the terminal.
Same for the messages below.
Some checks were not successful
|
I've got no idea about this CI error. It isn't clear to me it has anything to do with the contents of my branch: Is this possibly a broken builder, or unreliable test? |
Some checks were not successful
|
This looks very nice - I'm keen to land it (once tests are green, I merged master which will probably help). One quick question - will it fail gracefully if no polkit is available on the given system? |
|
Looks like the main CI tests are passing now. This branch just adds a new way for the user to assert superuser privileges. If polkit is not available (or if snapd can't connect to the system bus), this is treated the same way as polkit not authorising the access: the user will need to pass one of the other checks in order to use the API (provide a valid macaroon, or have uid 0). |
|
@jhenstridge Yeah, the mix of auth in the branch looks nice and correct. The question is more whether the implementation behaves properly if polkit is not around, as in will it introduce improper latency if a bus is not available or similar. |
mvo5
reviewed
Aug 23, 2017
This looks great, I have some nitpicks and suggestions for your consideration. Happy to help with any of them, I'm keen to land this for 2.28 :)
| + Details: make(map[string]dbus.Variant), | ||
| + } | ||
| + subject.Details["pid"] = dbus.MakeVariant(pid) | ||
| + if startTime, err := getStartTimeForPid(pid); err == nil { |
mvo5
Aug 23, 2017
Collaborator
(nitpick) I think its slightly more idiomatic go to write this with err != nil, i.e.:
startTime, err := getStartTimeForPid(pid)
if err != nil {
return false, err
}
subject.Details["start-time"] = dbus.MakeVariant(startTime)
| + file, err := os.Open(filename) | ||
| + if err != nil { | ||
| + return 0, err | ||
| + } |
mvo5
Aug 23, 2017
Collaborator
Is there a defer file.Close() missing here? Looking at the code it might be simple to just use:
data, err := ioutil.ReadFile(fmt.Sprintf("/proc/%d/stat", pid)
...
in lines 36-41 (which is slightly shorter and deals with the close for you).
| + contents := string(data) | ||
| + | ||
| + // start time is the token at index 19 after the '(process | ||
| + // name)' entry - since only this field can contain the ')' |
mvo5
Aug 23, 2017
Collaborator
Maybe a small reference like: "see man proc and see for starttime for details about what the number means".
| + | ||
| +var _ = check.Suite(&polkitSuite{}) | ||
| + | ||
| +func (s *polkitSuite) TestGetStartTime(c *check.C) { |
mvo5
Aug 23, 2017
Collaborator
I wonder if it would make sense to add a test that actually uses mock data for /proc/%d/stat via e.g. a export_test.go that allows to point to a mock-on-disk /proc. The reason is that if we ever refactor getStartTimeForPid() the test for (startTime, ot(Equals), 0) is true for most of the fields in the stat file. So a off-by-one error in the index might escape.
mvo5
added this to the 2.28 milestone
Aug 23, 2017
|
So there are a few places where a problem could occur:
If polkit is available and we can make the method call, we're not currently imposing any timeout on the response. With that said, the response may be blocked on user interaction, so it isn't clear how long it is sensible to wait. |
jhenstridge commentedJul 12, 2017
This pull request comes out of discussions on the forum, and draws some code and ideas from @robert-ancell's previous pull request #409.
As a first step, this branch only attempts to add polkit support to a single API endpoint:
/v2/login. This effectively lets snapd satisfy the use cases applications are usingsnapd-login-servicefor.This branch can be tested as follows:
ensure that
data/polkit/io.snapcraft.snapd.policyis copied to/usr/share/polkit-1/actions/. This should happen automatically if you build a Debian package, but will need to be done manually if running snapd from the build directory.Move
~/.snap/auth.jsonout of the way temporarily.Run
snap login(without sudo)Enter store account details.
A graphical password prompt should be presented by the desktop session's polkit agent. If the user correctly authenticates, then the login API call will succeed. If they cancel the prompt, the login will fail.