=== chihchun_afk is now known as chihchun [05:15] morning [06:50] Good morning [06:50] mborzecki: did you see my second pass review [06:50] zyga: hey [06:51] zyga: yes, i pushed an update with comments [06:51] Thanks, I’ll look soon [06:56] Lovely day === pstolowski|afk is now known as pstolowski [07:08] morning [07:09] hey zyga, i'm planning to review your implicit plugs/slots stuff today [07:09] pstolowski: hey [07:09] pstolowski: thank you [07:10] pedronis: changed it as well, it looks very good imo [07:10] Look at patch history please [07:11] ack [07:44] https://github.com/snapcore/snapd/pull/6740 is short and could help me move snap-update-ns forward [07:47] pedronis: I noticed that defer is blocked, is there something specific you want me to focus on? I would love to be able to move it forward next week while you are away [07:50] zyga: he is off from today [07:50] eh [07:50] not great [07:50] ok [08:41] pstolowski: can you take a look at https://github.com/snapcore/snapd/pull/6748 ? [08:43] yep [08:49] pstolowski: hey, can you please look at https://github.com/snapcore/snapd/pull/6740 next [08:52] pstolowski: and please tell us what to review [08:52] I think that's all of us today [08:55] pstolowski: in https://github.com/snapcore/snapd/pull/6755 could you add a Fixes: ... line that references the bug number? [08:55] zyga: it's a private bug [08:55] that's fine [08:55] it's only private for now [08:55] ok [08:55] it will become public once the security aspect expires [08:55] zyga: shall i add it in the code, or PR description only? [08:56] pstolowski: in the PR commit message please [08:56] k [08:56] thanks! === chihchun is now known as chihchun_afk [12:47] jdstrand: hey, is https://github.com/snapcore/snapd/pull/6681 ready for review? === chihchun_afk is now known as chihchun [13:49] zyga (cc pedronis and mvo): yes, I made some adjustments to the PR for the investigation yesterday. what remains is a) trying to figure out why spread is failing in: [13:49] 2019-04-19 13:02:27 Error executing google:ubuntu-16.04-32:tests/main/static : [13:49] ----- [13:49] + CGO_ENABLED=0 [13:49] + go build -o snapd.static github.com/snapcore/snapd/cmd/snapd [13:49] # github.com/snapcore/snapd/overlord/snapstate [13:49] ../../../overlord/snapstate/check_snap.go:433:16: undefined: osutil.FindUid [13:49] ../../../overlord/snapstate/check_snap.go:434:16: undefined: osutil.FindGid [13:49] b) comment updates from pedronis [13:49] jdstrand: perhaps those are failing because FindUid requires cgo? [13:49] jdstrand: FYI, I'm off on Monday [13:50] c) more spread tests to verify functionality of the bpf [13:50] jdstrand: if you have a moment please review 6714 [13:50] jdstrand: did you get to the bottom of the getuid issue? [13:50] is it a bug in libseccomp or a glibc wrapper confusing the actual syscall? [13:50] jdstrand: perdonis is off today and next week [13:51] zyga: please see the comment in the PR (I put it all in there) [13:51] zy [13:51] jdstrand: percfect, I surely will :) [13:51] zyga: so, FindUid is not new and this only started failing within the last couple days [13:51] hmm [13:52] odd, perhaps it was not build with CGO_ENABLED=0 [13:52] I don't know... [13:52] but I didn't look, my comment was just a hunch [13:52] this is the function that snap-seccomp uses to resolve u:root [13:52] jdstrand: snap-seccomp is built with cgo [13:52] jdstrand: snapd is not [13:52] jdstrand: you cannot use this function from snapd [13:53] zyga: I literally have to [13:53] we cannot :/ [13:53] we need a helper to resolve it then [13:53] a process you can call to give you the value [13:53] * jdstrand sighs [13:53] * zyga is happy at least that part is not mysterious anymore [13:53] * zyga hugs jdstrand [13:53] sorry, I know this is painful [13:54] I can call out to getent I guess [13:54] probably yeah [13:54] or parse /etc/passwd and /etc/group myself [13:54] jdstrand: that doesn't work [13:55] jdstrand: you can use go functions for that anyway [13:55] jdstrand: the real problem is nsswitch.conf [13:55] and ldap like systems [13:55] the go functions are what are doing this [13:55] jdstrand: that's why you need to use cgo to use that [13:55] jdstrand: there's a line where pure go can do simple things [13:55] like read passwd files [13:55] and cgo can call the glibc functions that do everything [13:55] zyga: I know that. though for this particular thing, it should really be on the system [13:55] jdstrand: we chose not to cross that line in snapd [13:56] jdstrand: then perhaps you just need to use the simple functions and not the osutil ones? [13:57] well, jeez [13:58] the check is to make sure the user/group is available and bail early (an implied 'assumes') [13:58] but it should really behave exactly like what snap-seccomp is doing [13:59] snap-seccomp could move to parsing /etc/passwd and /etc/group [13:59] jdstrand: (silly Friday idea): call snap-seccomp with setgid g:daemon and parse the word out of the filter ;) [13:59] but that is a bigger change than I want to make [13:59] * jdstrand goes with getent [13:59] heh [14:00] jdstrand: I think there are builtin go functions that give you that answer [14:00] I think getent will be much cheaper :) [14:00] *think* [14:00] not sure [14:00] you don't need to shell out to getent [14:00] I just googling-- people seem to have packages for it but not a part of the language [14:00] * zyga looks [14:01] hmm, actually, this starting failing when I added the implied assumes most likely. that makes more sense [14:01] jdstrand: os/user.LookupGroup [14:01] zyga: yes, that is what we are doing [14:01] well [14:01] then get group.GID if it didn't fail [14:01] we are using LookupUser [14:01] hold on? are we? [14:01] yes [14:02] wait, and that doesn't compile? [14:02] * zyga is sceptical now [14:02] os.FindUid uses LookupUser [14:02] can you do a tiny program that calls LookupGroup? [14:02] yes [14:02] LookupUser is old [14:02] LookupUser has always existed in golang [14:02] LookupGroup is new [14:02] *new enough for us though [14:02] the test is failing on FindUid [14:02] yes, we stole LookupGroup from newer golang [14:03] and could remove that code [14:03] FindUid is different [14:03] I was actually planning on doing that in another PR since we now require 1.10 [14:03] I mean [14:03] hey folks, what happens if I have a snap named "foo" and then I install a "bar" snap that has an autoalias bar.foo -> foo ? [14:03] but why is FindUid failing? [14:03] jdstrand: we can probably drop lookupGroup [14:03] jdstrand: and just use the plain versions [14:03] zyga: yes, I just said that :) [14:03] jdstrand: and that will get you out of the issue [14:04] jdstrand: that is probably the only thing that requires C [14:04] jdstrand: cool, ping me and mborzecki for review please [14:04] it will get me out of the issue for FindGid, but not FindUid, no? [14:04] jdstrand: no, why? [14:04] jdstrand: it's just the "import C" that breaks you [14:04] drop that from osutil and you are good [14:05] because of what I jus wrote? [14:05] findUid does user.Lookup [14:05] hm had a branch somewhere that drops all C dependant bits from osutil [14:05] jdstrand: I don't get it then, sorry, can you reprhase? [14:05] jdstrand: calling user.Lookup is not the problem [14:05] jdstrand: osutil *does not build* [14:05] the spread test fail because of FindGid *and* FindUid [14:05] jdstrand: when you disable C [14:05] at least not that part :) [14:05] ok, not that makes more sense [14:06] now* [14:06] nothing else used group.gp presumably [14:06] jdstrand: https://github.com/bboozzoo/snapd/commit/66e761e11bdb4f29ba279113b20c55d000035fda ? [14:06] let me do a quick PR for that [14:07] mborzecki: so are you saying that FindUid also needs cgo even though it is using user.Lookup? [14:08] jdstrand: it's one file [14:09] the whole file gets removed from the build process [14:09] that's all [14:09] zyga: right, I understand your point [14:09] that makes sense [14:09] but then mborzecki pointed me at https://github.com/bboozzoo/snapd/commit/66e761e11bdb4f29ba279113b20c55d000035fda which stubs FindUid [14:09] when it seems it may only need to stub FindGid [14:10] so I wanted a clarification on that [14:10] since if FindUid also needs cgo, then the LookupGroup fix is valid on its own, but doesn't help me [14:11] * zyga has a new flower pot :) [14:11] I mean, I can just try this out I guess [14:11] :D [14:12] zyga: anyway, so there a few things to tie up with my PR and I'll be adding more tests, but it is ready for review. you could wait til Tuesday I guess (all my bits should be done then) [14:12] and yes, I plan to look at PRs today [14:12] jdstrand: cool, [14:13] jdstrand: I have a few piles of changes for snap-confine pending but I don't want to throw this all at you [14:13] jdstrand: one thing that was bugging me a while ago was stdlib's IO functions that we cannot use because they apparently suck on error reporting [14:13] jdstrand: so I wrote my own proof of concept with tests to show what happens in all error cases [14:14] jdstrand: if you want to have cgo enabled, but use non-cgo implementation of os/user bits you'd have to pass -tags osuser (and wit that patch i linked -tags osutil too) [14:14] * zyga is starving [14:14] time to eat [14:14] mborzecki: that is interesting, but I think a different answer to my question [14:14] mborzecki: I am going to do a PR to use user.LookupGroup [14:15] mborzecki: and remove all the cgo stuff in osutil [14:15] mborzecki: I thought that would fix stuff, but then your commit stubbed osutil.FindUid, that already used user.LookupUser, so I didn't know why you did that one since it was already using the bultin [14:16] jdstrand: that's tricky i think, we'll loose /etc/nss* and probably extrausers too [14:16] I see [14:16] so the builtins don't use nss at all [14:17] we need extra users [14:17] yup, let me point you to the source code [14:17] ok, I think instead of using builtins, I can convert FindUid and FindGid to use getent [14:18] jdstrand: https://github.com/golang/go/tree/master/src/os/user look at cgo_lookup_unix.go and lookup_unix.go [14:18] actually, for this PR, I will probably use LookupGroup. then add a code comment that when we need extrausers, we will need to use getent [14:19] since we don't need extrausers for this now [14:19] but we will later [14:20] passwd: compat extrausers [14:20] from nsswitch.conf [14:20] ok, this is all clear now [14:20] zyga, mborzecki: thanks [14:22] jdstrand: btw. that also makes calling the os/user bits from snap-exec snap-update-ns bit problematic, as those are built statically via -extldflags=-ldflags=-static, but will still attempt to dlopen the nss bits [14:23] mborzecki: I'm sorry, if I'm using only builtins, what is the problem? [14:25] mborzecki: nothing except snap-seccomp and my PR use FindUid and FindGid [14:25] jdstrand: because builtins only know about /etc/groups & /etc/passwd [14:25] mborzecki: I know. but snap-exec and snap-update-ns don't do lookups... ? [14:26] mborzecki: at least not via these functions. I feel like I am missing your point [14:26] jdstrand: they don't, i stumbled upon this problem when trying to do the mapping for $HOME/snap_ to $HOME/snap with parallel installs [14:27] jdstrand: which we eventually dropped anyway [14:32] * zyga breaks for lunch [14:49] zyga: fyi https://github.com/snapcore/snapd/pull/6759 [14:54] Back in 10 [15:07] back now [15:08] jdstrand: the error on https://travis-ci.org/snapcore/snapd/jobs/522179183 is puzzling! [15:09] jdstrand: as a nitpick, could you please expand the commit message, it's nicer to know _why_ we are doing that not what is being done [15:11] jdstrand: the error talks about build ID, I would suspect it is caused by lack of cgo now [15:11] jdstrand: and default build mode that don't inject build-id [15:17] zyga, any idea from the snap side for this? https://github.com/livecd-tools/livecd-tools/issues/125 [15:18] looking [15:18] hmm, no idea if we are using livecd-tools anymore [15:18] it's a distro question [15:18] for snapd side I can say this [15:18] you can put seed data on a filesystem [15:19] then on 1st boot snapd spots the seed and installs the snaps listed there [15:19] I don't know anything about those things [15:19] this is done for ubuntu live CD for instance [15:19] so it'd be handy if you could detail that for this [15:19] but I don't know anything about livecd-tools [15:19] I don't know much about this honestly, this is really mvo/chipaca area [15:20] I can try to ask them after easter [15:20] livecd-tools just basically uses a kickstart file as input to create live media [15:20] it's how Fedora and other derivatives of Fedora make media [15:20] *basically* is what most people would not use :) [15:20] I don't know how distros do that today [15:20] * zyga (I mean the word basically assumes you know this) [15:21] I use basically as "simply speaking" [15:21] sorry [15:21] no worries :) [15:22] I'm looking for seeding docs [15:23] Son_Goku: I answered in the github issue now [15:25] thanks [15:35] jdstrand: I commented on https://github.com/snapcore/snapd/pull/6759 [15:36] I'm back to hacking on groups [15:36] I solved my conundrum === pstolowski is now known as pstolowski|afk [16:54] * zyga begs for review on https://github.com/snapcore/snapd/pull/6714 [17:45] jdstrand: ping [17:48] jdstrand: my last patch of the day [17:49] jdstrand: in case you are around I wanted to explain some of the things since you will be probably the only one that _has to_ review it [17:52] jdstrand: https://github.com/snapcore/snapd/pull/6760 [17:53] jdstrand: I'm EODing and EOWing now, please if you have some time left today have a look at https://github.com/snapcore/snapd/pull/6714 which is the follow-up from the /tmp security issue [17:53] * zyga waves [19:29] zyga: hey, if you are around, mind giving some insight on https://github.com/ChristianPauly/fluffychat/issues/117#issuecomment-484959454 ... I suspect this is a host lacking many services, but have not dug into it and you might know more about it === chihchun is now known as chihchun_afk === chihchun_afk is now known as chihchun [21:00] sergiusens: someone has staged real xdg-open and it took priority [21:00] sergiusens: our xdg-open doesn't use dbus-launch and does not print that output [21:15] jdstrand: thank you for both reviews [21:16] * zyga started another print and goes to sleep