| jamesh | amurray: hi. I was wondering if you had some time to look at some of the polkit related PRs I have up. This is still following the basic plan I outlined on the forum a while back, that you looked over. | 02:07 |
|---|---|---|
| amurray | hey jamesh - I'm on leave next week but will put it at the top of my list to look at when I get back on the 11th - I see 4 different PRs mentioning polkit https://github.com/snapcore/snapd/pulls?q=is%3Apr+is%3Aopen+polkit - but none are marked as needs-security-review - which ones in particular, or in which order should I do them? | 02:11 |
| jamesh | amurray: three of them (9986, 10173 and 10219) are conceptually all part of the one feature, with nothing exposed until the last would land. I'm not sure how you'd find it easiest to review that. | 02:16 |
| jamesh | amurray: i.e. I think we'd need security sign-off for the last one to land, but that sign-off would be contingent on the code in the other two being sane. | 02:17 |
| jamesh | The last PR (allowing a snap to act as a polkit agent) is really only really useful for Ubuntu Core Desktop at the moment: I'd like some feedback on the AppArmor rules and how best to confine the setuid helper, but it probably counts as a lower priority. | 02:19 |
| amurray | ok thanks, I'll prob go in the order you list them above then | 02:38 |
| jamesh | amurray: would you be okay with some of the earlier PRs getting landed before you do the full review? | 02:40 |
| jamesh | in particular I'd like to get 10173 landed to simplify review of 10219. | 02:40 |
| jamesh | (it just adds infrastructure for use by interfaces, rather than exposing anything of its own). | 02:41 |
| jamesh | i.e. it will install a polkit policy file if an interface tells it to, but there are currently no interfaces that tell it to do so. | 02:42 |
| amurray | hmm I was thinking that might be the more crucial one to get reviewed as it is the one that interfaces with the host system but I don't want to block you getting things done so if you need it more urgently, I don't mind coming back to it later | 02:43 |
| jamesh | the logic is essentially all contained in a new ./interfaces/polkit directory, so should be easy to come back to | 02:44 |
| amurray | actually I've just had a quick look over that one and I don | 02:44 |
| amurray | don't think it looks too scary - does it do validation of the policy anywhere though? | 02:44 |
| amurray | I see it doesn't yet - I would want to see how 10173 gets integrated with 9986 then as that will be the security critical part | 02:46 |
| jamesh | we've got validation in 9986 (to the extent that Go's XML library can do validation). | 02:46 |
| jamesh | I was planning to tie the validation code into 10219 when that's merged: the plan is to have an interface attribute giving an action ID prefix, and check that the actions referenced in the XML all start with that. | 02:47 |
| jamesh | This way we can use store assertions to control what actions a snap can install | 02:48 |
| amurray | sounds great - I've just approved 10173 :) | 02:49 |
| jamesh | thanks! | 02:49 |
| amurray | you're welcome :) - will take a look at 9986 etc in a week or so when I get back | 02:50 |
| jamesh | the action ID checks in the validator check both defined actions and "imply" assertions | 02:51 |
| jamesh | We don't want a snap to declare one action with a "default yes" policy that is set to imply a grant of an unrelated service's actions | 02:52 |
| mborzecki | morning | 05:50 |
| mardy_2nd | mborzecki: hi! | 05:59 |
| mborzecki | mardy_2nd: hey | 06:00 |
| === mardy_2nd is now known as mardy | ||
| zyga | happy Friday :) | 06:24 |
| zyga | I won't be on IRC much today, heading to the office to set up some boards | 06:25 |
| zyga | o/ | 06:25 |
| === jamesh_ is now known as jamesh | ||
| pstolowski | morning | 07:05 |
| mborzecki | pstolowski: hey | 07:27 |
| mvo | good morning! | 07:27 |
| mardy | hi zyga-mbp, pstolowski, mvo | 07:38 |
| mborzecki | mvo: hey | 07:42 |
| mborzecki | mvo: no new fires since yesterday? 🙂 | 07:46 |
| mborzecki | mvo: can you take a look at https://github.com/snapcore/snapd/pull/10865 ? | 07:46 |
| mup | PR #10865: cmd/snap: improve snap run help message <Simple 😃> <Created by bboozzoo> <https://github.com/snapcore/snapd/pull/10865> | 07:46 |
| mup | PR snapd#10867 opened: desktop, usersession: observe notifications <Created by stolowski> <https://github.com/snapcore/snapd/pull/10867> | 07:50 |
| mvo | mborzecki: haha - no fires **yet** | 07:51 |
| mvo | mborzecki: sure, let me check that PR | 07:51 |
| pstolowski | jamesh: hi, i've opened the next notifications PR ^ | 07:52 |
| jamesh | pstolowski: cool. I'll have a look | 07:52 |
| pstolowski | jamesh: also, I've tracked down the occasional panic I had with the existing code | 07:52 |
| pstolowski | so it's fixed there | 07:52 |
| jamesh | oh? what was the problem? | 07:52 |
| pstolowski | jamesh: afaiu the ObserveNotifications loop could break either on ctx.Done() or channel closing (in which case we would receive nil signal), we need the usual "case sig, ok := <-ch:" guard there | 07:54 |
| jamesh | ah. | 07:54 |
| pstolowski | jamesh: is the problem was in the pre-existing code, I think we simply never used context there until now | 07:55 |
| pstolowski | s/is// | 07:56 |
| mborzecki | mvo: while at it, any chance you could take a look at https://github.com/snapcore/snapd/pull/10847 too? | 08:02 |
| mup | PR #10847: cmd/snap-confine: die when snap process is outside of snap specific cgroup <cgroupv2> <Created by bboozzoo> <https://github.com/snapcore/snapd/pull/10847> | 08:02 |
| pstolowski | jamesh: btw shall I drop TestCloseNotificationError test for gtk? you said it's not a realistic case right? | 08:18 |
| mardy | do we have any utility function to copy a map? (better if deep copy) | 08:19 |
| pstolowski | mardy: I don't think so, we have CopyAttributes for interface attributes, and we might have something else for a specific kind of map, but nothing generic | 08:21 |
| mborzecki | mardy: hmm maybe miguelpires added something, i think he had some code for deep comparison or sth | 08:24 |
| pstolowski | mvo: could you land https://github.com/snapcore/snapd/pull/10760 ? unrelated arch failure | 08:25 |
| mup | PR #10760: o/snapstate: support ignore validation flag on install/update <Needs Samuele review> <validation-sets :white_check_mark:> <Created by stolowski> <https://github.com/snapcore/snapd/pull/10760> | 08:25 |
| miguelpires | mardy mborzecki: sorry, no luck there, that was just an unsorted match checker for tests | 08:28 |
| mvo | pstolowski: sure | 08:28 |
| pstolowski | thanks! | 08:30 |
| mup | PR snapd#10760 closed: o/snapstate: support ignore validation flag on install/update <Needs Samuele review> <validation-sets :white_check_mark:> <Created by stolowski> <Merged by mvo5> <https://github.com/snapcore/snapd/pull/10760> | 08:31 |
| === pstolowski_ is now known as pstolowski | ||
| === alan_g_ is now known as alan_g | ||
| mup | PR snapd#10868 opened: o/snapstate: support ignore-validation flag when updating to a specific snap revision <validation-sets :white_check_mark:> <Created by stolowski> <https://github.com/snapcore/snapd/pull/10868> | 09:31 |
| mardy | can someone explain me what these two lines do? https://github.com/snapcore/snapd/blob/master/overlord/ifacestate/helpers.go#L922-L923 | 09:33 |
| mardy | at a first sight that cref variable is exiting the scope, so it would seem that these lines might not have an effect | 09:34 |
| pstolowski | mardy: they affect cref.ID() below? | 09:37 |
| pstolowski | see ID() implementation | 09:37 |
| mardy | pstolowski: makes sense, thanks! | 09:42 |
| pstolowski | https://github.com/snapcore/snapd/pull/10818 is ready for reviews now | 10:47 |
| mup | PR #10818: tests: test for enforcing with prerequisites <validation-sets :white_check_mark:> <Created by stolowski> <https://github.com/snapcore/snapd/pull/10818> | 10:47 |
| mup | PR snapd#10869 opened: o/snapstate: use device ctx in prerequisite install/update <Created by MiguelPires> <https://github.com/snapcore/snapd/pull/10869> | 11:11 |
| mup | PR snapd#10865 closed: cmd/snap: improve snap run help message <Simple 😃> <Created by bboozzoo> <Merged by mvo5> <https://github.com/snapcore/snapd/pull/10865> | 11:21 |
| mup | PR core#126 closed: Makefile: refactor to have a common $TARGET_BASENAME <Created by mvo5> <Merged by mvo5> <https://github.com/snapcore/core/pull/126> | 11:37 |
| === waveform_ is now known as waveform | ||
| mborzecki | mvo: can you land https://github.com/snapcore/snapd/pull/10534 ? | 12:16 |
| mup | PR #10534: overlord/devicestate, tests: enable UC20 remodel, add spread tests <Remodel 🚋> <Run nested> <Created by bboozzoo> <https://github.com/snapcore/snapd/pull/10534> | 12:16 |
| mvo | mborzecki: with pleasure | 12:50 |
| mup | PR snapd#10534 closed: overlord/devicestate, tests: enable UC20 remodel, add spread tests <Remodel 🚋> <Run nested> <Created by bboozzoo> <Merged by mvo5> <https://github.com/snapcore/snapd/pull/10534> | 12:51 |
| mborzecki | thanks! | 12:52 |
| ijohnson[m] | Nice | 12:55 |
| * mvo hugs mborzecki ijohnson[m] | 13:29 | |
| pstolowski | mborzecki: which of your PRs need 2nd review? | 13:29 |
| mborzecki | pstolowski: this one https://github.com/snapcore/snapd/pull/10847 | 13:30 |
| mup | PR #10847: cmd/snap-confine: die when snap process is outside of snap specific cgroup <cgroupv2> <Created by bboozzoo> <https://github.com/snapcore/snapd/pull/10847> | 13:30 |
| pstolowski | mborzecki: +1 | 13:49 |
| ijohnson[m] | mardy: in your reivew of my disks PR you say `"this function returns the root mount points for the given partition" which is not what the function returns` | 13:51 |
| ijohnson[m] | but that's 100% what the function does | 13:52 |
| ijohnson[m] | so I'm confused about if I am confused about your comment or you are confused about what the function does ? 😀 | 13:52 |
| mborzecki | pstolowski: thanks! | 13:54 |
| mardy | ijohnson[m]: mmm... my understanding is that the function returns the mount points for the given partition's root directory | 13:57 |
| mardy | ijohnson[m]: right, I think I see where the confusion is coming from: when I read "root mount point" I think of the filesystem root | 13:59 |
| mardy | and there's only one of them | 13:59 |
| ijohnson[m] | it returns the set of directory's where the partition has it's root directory mounted | 13:59 |
| ijohnson[m] | oh you mean on the partition? wouldn't that just always be "/" ? | 14:00 |
| mardy | no no, I mean on the whole FS | 14:00 |
| mardy | I mean, to me the expression "root mount point" is very confusing | 14:00 |
| ijohnson[m] | mardy: well what I mean is that you could have mount points like the directory /foo that physically exists on the filesystem of /dev/sda1, and that /foo directory is mounted at /usr/foo | 14:01 |
| ijohnson[m] | this function doesn't want want any subdirectories of the partition, it only wants the root directory of the partition | 14:01 |
| mardy | ijohnson[m]: yes, and this function would skip that | 14:01 |
| mardy | indeed | 14:01 |
| ijohnson[m] | what would be a more clear name / phrase for you ? | 14:01 |
| ijohnson[m] | mardy: exactly | 14:01 |
| mardy | ijohnson[m]: the one I suggested :-) | 14:02 |
| ijohnson[m] | ah right sorry I forgot you suggested one in your comment, yes that's fair thanks | 14:02 |
| mardy | ijohnson[m]: I think that the confusion arises from the fact that "root" can be read in two ways: substantive ("the root of the partition") and appositive ("the root partition"). You were referring to the former, but when I read that function name I think of the latter. :-) I think that the name I suggested helps reducing the ambiguity | 14:07 |
| ijohnson[m] | yeah I think you're right it is less ambiguous | 14:08 |
| mup | PR snapd#10870 opened: overlord/devicestate: record recovery capable system on a successful remodel <Run nested> <Created by bboozzoo> <https://github.com/snapcore/snapd/pull/10870> | 14:12 |
| mup | PR snapd#10871 opened: many: support an API flag system-restart-immediate to make snap ops proceed immediately with system restarts <Created by pedronis> <https://github.com/snapcore/snapd/pull/10871> | 14:17 |
| mup | PR core20#115 opened: Start bootchart earlier <Created by alfonsosanchezbeato> <https://github.com/snapcore/core20/pull/115> | 15:35 |
| ijohnson[m] | mvo: could you force land https://github.com/snapcore/snapd/pull/10862 for me? | 15:39 |
| mup | PR #10862: osutil/disks: add RootMountPointsForPartition <Simple 😃> <Created by anonymouse64> <https://github.com/snapcore/snapd/pull/10862> | 15:39 |
| ijohnson[m] | the failures there are unrelated | 15:39 |
| mvo | ijohnson[m]: sure, done | 15:45 |
| mup | PR snapd#10862 closed: osutil/disks: add RootMountPointsForPartition <Simple 😃> <Created by anonymouse64> <Merged by mvo5> <https://github.com/snapcore/snapd/pull/10862> | 15:47 |
| === jschwart_ is now known as jschwart | ||
| === sarnold_ is now known as sarnold | ||
| mup | PR snapd#10872 opened: osutil/disks: support filtering by mount opts in MountPointsForPartitionRoot <Simple 😃> <Skip spread> <Created by anonymouse64> <https://github.com/snapcore/snapd/pull/10872> | 22:38 |
| === rZZZr is now known as RzR | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!