[00:18] bandali: no, it just needs to be reviewed by the reviewers - I'll take a look this morning and cast my vote at least to keep it ticking along [00:40] PR snapd#10202 closed: tests: new buckets for snapd-spread project on gce [02:01] PR snapd#10204 opened: o/servicestate/quota_control.go: introduce basic group manipulation methods [02:48] amurray, gotcha, and thank you :-) [03:34] amurray: when you've got some time, could you give some feedback on https://forum.snapcraft.io/t/proposal-add-polkit-and-polkit-agent-interfaces-to-snapd/23876?u=jamesh ? [03:35] amurray: this is a proposal for a pair of interfaces to allow snaps to use polkit [04:09] jamesh: I'm just looking now :) [04:10] thanks! [04:28] re the unix process subject type - as you said the base policy already allows `@{PROC}/@{pid}/stat r,` so this grants any snap the ability to read the start time of any other process on the system (assuming traditional DAC checks allow it) - so I think that this would be good to try and cover this use-case as well [04:30] That allows a process to read its own information (i.e. through the /proc/self symlink) [04:30] For servers using the unix process subject type, they would need to read that data for arbitrary processes [04:32] yes it does - there is no owner restriction on that rule and @{pid} is just a clumsy regex to match against any pid - so it should allow to read any processes start time [04:32] hmm. I thought it was a special token to match the current pid [04:34] unfortunately not - that would be nice if it was though - much more principle-of-least-privilege if it were [04:34] okay, that makes things a bit simpler [04:35] so a lot of the AppArmor rules in system_observe duplicate the base template [04:36] hmm I wonder if perhaps the base template has grown over time... [04:37] but you're right: I can e.g. read /proc/1/stat from "snap run --shell chromium" [04:38] how do polkit rules files differ from policy? where/how is one used over the other? [04:40] the XML .policy files can specify one of 6 default canned policies. Anything more complicated than those policies needs to be specified via pkla files (Ubuntu/Debian) or Javascript rules files (most other distros) [04:42] The implication of using the "yes" default policy (always allow) is that polkitd will always tell the server application that the client is authorised. [04:43] If the actions are provided by the same package as the server, then this doesn't meaningfully reduce security: the server could just as easily have not used polkit [04:47] ok, so I am wondering whether either pkla files or rules allow to express anything more (in terms of authority) than the default policy? I am guessing this is "no" which means we are more worried about the js rules as an attack surface for the js parser itself etc rather than them allowing to specify outcomes that grant more authority than would otherwise be authorised - is that a fair characterisation? [04:50] yes. I can create a pkla file saying that only users who are members of a particular group are allowed to invoke a particular set of actions [04:51] you've got a limited version of that in .policy files via "auth_admin" (where polkit is configured to consider some users to be administrators). But if you wanted to use different group membership to gate different actions, you'd need to use pkla files or JS [04:52] In general, it wasn't common for packages to provide pkla policy files. [04:53] There's been more use of shipping default JavaScript policy files (look in /usr/share/polkit-1/rules.d on your system) [04:54] But I don't even want to consider allowing snaps to be able to provide JS to be run by an unconfined system daemon [04:54] (even if it wouldn't ever run on current Ubuntu releases) [04:55] yeah I noticed that - but I assume this is ignored as we don't ship the js backend in policykit [04:56] given we don't allow snaps to install man pages due to fears over untrusted input (personally I think the usability benefits outweigh any perceived security risk here) then it would be crazy to let them ship js policy [04:57] i'd be all for man pages in snaps being symlinked out to /usr/share/man or similar... lots of snaps ship them but they are not readily available... [04:59] right. That's why I thought it'd be better to ignore these more advanced policy configuration options: they won't work consistently across all distros, and it's harder to convince ourselves that they are safe [05:05] yep, that can be a problem for another day :) [05:20] amurray: reading over /etc/apparmor.d/tunables/kernelvars, it sounds like @{pid} is intended to represent the current process but covers all processes now due to kernel limitations [05:21] that kind of explains why we might have the /@{pid}/ vs. /*/ distinction in the base template and system-observe [05:22] jamesh: hmm I wonder if/when that were to change how many things would break due to the semantic difference... ok I'll keep that in mind.. we may want to add a rule then just for this to the polkit plug to allow this (/proc/*/stat r,) [05:23] yep. It's quite possible that if they were to implement kernel support, it would need to have a different name in the policy language [05:24] or have some way to declare a version of the policy language [07:01] good morning! [07:04] morning [07:04] good morning pstolowski [07:07] pstolowski: mvo: morning [07:11] hey mborzecki ! [07:22] PR snapd#10199 closed: o/servicestate/quotas: add functions for getting and setting quotas in state [07:57] PR snapd#10205 opened: cmd/snap, client: snap remove-quota command [07:57] degville: hi, could you look at the help text in https://github.com/snapcore/snapd/pull/10196 when you have a moment [07:57] PR #10196: o/devicestate,o/hookstate/ctlcmd: introduce SystemModeInfo methods and snapctl system-mode [08:20] pedronis: hello! Yes, of course. [09:08] mvo: seems there's an order issue in the servicestate/quotas tests, that's why I got unit tests failures in my PR [09:08] mvo: they are on master [09:08] mvo: https://github.com/snapcore/snapd/pull/10196/checks?check_run_id=2455491426 [09:08] PR #10196: o/devicestate,o/hookstate/ctlcmd: introduce SystemModeInfo methods and snapctl system-mode [09:20] pedronis: oh no! sorry for that [09:21] pedronis: are you on it or shall I? [09:21] I'm on it [09:22] mvo ^ [09:25] ok [09:51] mvo: https://github.com/snapcore/snapd/pull/10206 [09:51] PR #10206: o/servicestate: test has internal ordering issues, consider both cases <⚠ Critical> [09:52] PR snapd#10206 opened: o/servicestate: test has internal ordering issues, consider both cases <⚠ Critical> [09:59] amurray: also I went to lots of effort to add apparmor and seccomp to man-db to mitigate all these (IMO overblown in this case anyway) fears about untrusted input. Just hasn't been integrated on the snapd side yet [10:02] PR snapd#10207 opened: vendor: bump github.com/canonical/go-tpm2 revision [10:27] PR snapd#10208 opened: daemon: implement REST API for quota groups (create / list / get) [10:47] mmh. I should have set that PR to skip spread [10:47] PR snapd#10206 closed: o/servicestate: test has internal ordering issues, consider both cases <⚠ Critical> [10:52] PR snapd#10206 opened: o/servicestate: test has internal ordering issues, consider both cases <⚠ Critical> [11:02] ijohnson: pstolowski: what's the next PRs in order I should review or re-review? there are quite a few [11:05] pedronis: #10197, then #10208 [11:05] Bug #10197: folder widget text entry loves to crash evo [11:05] PR #10197: cmd/snap: introduce 'snap quota' command [11:05] Bug #10208: Needs rebuild with new libneon [11:05] PR #10208: daemon: implement REST API for quota groups (create / list / get) [11:12] pstolowski: thx [11:18] i see quotas_test.go:43: servicestateQuotasSuite.TestQuotas test failure due to ordering, but i think it's fixed in on of Ian's PRs [11:22] PR snapd#10206 closed: o/servicestate: test has internal ordering issues, consider both cases <⚠ Critical> [11:24] pstolowski: I fixed independently given that it broke master and Ian's PR don't have enough +1 yet [11:24] a possible fix is now merged [11:27] thanks [11:28] pedronis: i think i pulled the trigger too fast on API PR, i now see ServiceManager methods that I should probably be using instead of servicestate [11:28] i'm marking it blocked and need to rework some bits [11:32] PR snapd#10088 closed: o/configstate/configcore/picfg.go: use ubuntu-seed config.txt in uc20 run mode [11:53] pedronis: what output do we want from 'snap quotas'? [12:02] PR snapd#10209 opened: add dm-crypt interface to support external storage encryption [12:03] pstolowski: we don't have anything like it so far I suppose [12:05] pedronis: not really... closest is probably debug timings (as far as nesting is depicted in tabular output) [12:05] yea [12:40] cjwatson: nice - ok I'll try follow-up with the snapd folks then :) [13:17] hmm ... having a daemon with "install-mode: disable" and a "timer:" entry still seems to start the timer on install it seems ... is that a bug or expected behaviour (the doc doesnt say anything about combining the two) [13:36] Bug #1926442 opened: cannot execute 'netplan generate' from within a snap [14:26] pedronis: for 'snap quotas', what do you think about tabular output with just "Name Parent Memory Limit" columns (and otherwise the details can be inspected with snap quota )? [14:27] "Quota Parent Max-Memory" ? [14:27] pedronis: ok, sure [14:27] let's start there [14:27] ok [14:27] to be clear the api has all the details so we can extend [14:28] it's maybe obvious but we should sort them by hierarchy [14:28] and otherwise by name [14:31] makes sense [15:25] mvo: will you find time to do 2nd review of https://github.com/snapcore/snapd/pull/10197 ? [15:25] PR #10197: cmd/snap: introduce 'snap quota' command [15:25] pstolowski: yeah, happy to do right after my current meeting [15:25] mvo: thanks! [15:35] pstolowski: pedronis I updated https://github.com/snapcore/snapd/pull/10198 [15:35] PR #10198: snap/quotas: followups from previous PR [15:35] ijohnson: thx [15:37] ijohnson: ack, thanks, i'll review [15:37] ogra: that sounds like a bug re install-mode and timer, can you file a LP bug please? [15:38] yep [15:38] thanks [15:43] ijohnson, https://bugs.launchpad.net/snapd/+bug/1926474 [15:43] Bug #1926474: combining timer and "install-mode: disabled" for a daemon seems not respected [15:43] great thank you [15:55] ijohnson: a few small comments [15:58] pstolowski: thanks I asked a question there [16:08] ijohnson: I actually put that TODO there, so I clarified what I meant in a couple of comments [16:08] ok, sorry I pushed right as you left your review [16:08] PR snapd#10197 closed: cmd/snap: introduce 'snap quota' command [16:11] ijohnson: my main point was: https://github.com/snapcore/snapd/pull/10198#discussion_r622326464, which is not really about this PR but higher levels [16:11] PR #10198: snap/quotas: followups from previous PR [16:11] pstolowski: will you merge master into 10205 or shall I (I realize it's late for you) [16:11] pedronis: I guess my main question is should I make further changes to that PR or are we happy with that PR [16:13] mvo: i'll merge (i didn't realize you just landed the previous PR, thanks!) [16:13] pstolowski: \o/ thank you [16:14] ijohnson: +1 from me, but do consider that question and also my last small remark that probably can be covered in one of the follow-ups [16:15] I mean do consider the question in the higher levels [16:15] mvo: done [16:18] ok, I left some TODOs I think it is ready to land after the spread tests run [16:18] * ijohnson moves on to the other changes [16:19] ijohnson: +1 [16:26] ijohnson: thx [16:39] * cachio lunch [18:04] PR snapd#10198 closed: snap/quotas: followups from previous PR [19:21] PR snapcraft#3515 opened: Use snapd-spread gce project instead of computeengine project === not_phunyguy is now known as phunyguy [20:45] jamesh and amurray: you're right that @{pid} is supposed to mean your own pid, hence the distinction between base template and system-observe of @{pid} vs * (ie, trying to have a reasonable experience if we get the kernelvar support). so yeah there is overlap. it's also possible we have duplicated rules (as opposed to overlapping) which imho we could remove from the interfaces if they are in the default [20:45] template (where @{pid} and * are overlapping, not duplicate of course) [20:47] jamesh and amurray: but like you said, I imagine whenever kernel support landed, there would be stuff that would break since they all of a sudden might need system-observe (or changes to the base template). that would have to be a careful rollout [20:50] amurray: cjwatson is right. the man page thing is fixable now. someone needs to do the work. though, with man pages, the idea was that we'd allow any snap to install a man page. that might be different than with policykit files (I certainly like the idea of restricing js as mentioned; it is also possible to make the interface restricted with some sort of vetting procedure like with other stuff (I didn't [20:50] read all backscroll or through the PR carefully, so forgive me if that was discussed. I'm just tossing out a couple of ideas)) [20:52] I'd love to see the man pages work picked up (on the snapd side) [20:59] PR snapd#10153 closed: wrappers, quota: implement quota groups slice generation [21:11] PR snapcraft#3516 opened: github: update workflows that to use pull_request_target [23:10] jdstrand: hey :) - yeah I would also love to see man pages surfaced by snapd so hopefully we can make that happen