=== chihchun_afk is now known as chihchun
zygaGood morning06:50
zygamborzecki: did you see my second pass review06:50
mborzeckizyga: hey06:50
mborzeckizyga: yes, i pushed an update with comments06:51
zygaThanks, I’ll look soon06:51
zygaLovely day06:56
=== pstolowski|afk is now known as pstolowski
pstolowskihey zyga, i'm planning to review your implicit plugs/slots stuff today07:09
mborzeckipstolowski: hey07:09
zygapstolowski: thank you07:09
zygapedronis: changed it as well, it looks very good imo07:10
zygaLook at patch history please07:10
zygahttps://github.com/snapcore/snapd/pull/6740 is short and could help me move snap-update-ns forward07:44
zygapedronis: 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 away07:47
pstolowskizyga: he is off from today07:50
zyganot great07:50
mborzeckipstolowski: can you take a look at https://github.com/snapcore/snapd/pull/6748 ?08:41
zygapstolowski: hey, can you please look at https://github.com/snapcore/snapd/pull/6740 next08:49
zygapstolowski: and please tell us what to review08:52
zygaI think that's all of us today08:52
zygapstolowski: in https://github.com/snapcore/snapd/pull/6755 could you add a Fixes: ... line that references the bug number?08:55
pstolowskizyga: it's a private bug08:55
zygathat's fine08:55
zygait's only private for now08:55
zygait will become public once the security aspect expires08:55
pstolowskizyga: shall i add it in the code, or PR description only?08:55
zygapstolowski: in the PR commit message please08:56
zyga thanks!08:56
=== chihchun is now known as chihchun_afk
zygajdstrand: hey, is https://github.com/snapcore/snapd/pull/6681 ready for review?12:47
=== chihchun_afk is now known as chihchun
jdstrandzyga (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
jdstrand2019-04-19 13:02:27 Error executing google:ubuntu-16.04-32:tests/main/static :13:49
jdstrand+ CGO_ENABLED=013:49
jdstrand+ go build -o snapd.static github.com/snapcore/snapd/cmd/snapd13:49
jdstrand# github.com/snapcore/snapd/overlord/snapstate13:49
jdstrand../../../overlord/snapstate/check_snap.go:433:16: undefined: osutil.FindUid13:49
jdstrand../../../overlord/snapstate/check_snap.go:434:16: undefined: osutil.FindGid13:49
jdstrandb) comment updates from pedronis13:49
zygajdstrand: perhaps those are failing because FindUid requires cgo?13:49
zygajdstrand: FYI, I'm off on Monday13:49
jdstrandc) more spread tests to verify functionality of the bpf13:50
zygajdstrand: if you have a moment please review 671413:50
zygajdstrand: did you get to the bottom of the getuid issue?13:50
zygais it a bug in libseccomp or a glibc wrapper confusing the actual syscall?13:50
zygajdstrand: perdonis is off today and next week13:50
jdstrandzyga: please see the comment in the PR (I put it all in there)13:51
zygajdstrand: percfect, I surely will :)13:51
jdstrandzyga: so, FindUid is not new and this only started failing within the last couple days13:51
zygaodd, perhaps it was not build with CGO_ENABLED=013:52
jdstrandI don't know...13:52
zygabut I didn't look, my comment was just a hunch13:52
jdstrandthis is the function that snap-seccomp uses to resolve u:root13:52
zygajdstrand: snap-seccomp is built with cgo13:52
zygajdstrand: snapd is not13:52
zygajdstrand: you cannot use this function from snapd13:52
jdstrandzyga: I literally have to13:53
zygawe cannot :/13:53
zygawe need a helper to resolve it then13:53
zygaa process you can call to give you the value13:53
* jdstrand sighs13:53
* zyga is happy at least that part is not mysterious anymore13:53
* zyga hugs jdstrand 13:53
zygasorry, I know this is painful13:53
jdstrandI can call out to getent I guess13:54
zygaprobably yeah13:54
jdstrandor parse /etc/passwd and /etc/group myself13:54
zygajdstrand: that doesn't work13:54
zygajdstrand: you can use go functions for that anyway13:55
zygajdstrand: the real problem is nsswitch.conf13:55
zygaand ldap like systems13:55
jdstrandthe go functions are what are doing this13:55
zygajdstrand: that's why you need to use cgo to use that13:55
zygajdstrand: there's a line where pure go can do simple things13:55
zygalike read passwd files13:55
zygaand cgo can call the glibc functions that do everything13:55
jdstrandzyga: I know that. though for this particular thing, it should really be on the system13:55
zygajdstrand: we chose not to cross that line in snapd13:55
zygajdstrand: then perhaps you just need to use the simple functions and not the osutil ones?13:56
jdstrandwell, jeez13:57
jdstrandthe check is to make sure the user/group is available and bail early (an implied 'assumes')13:58
jdstrandbut it should really behave exactly like what snap-seccomp is doing13:58
jdstrandsnap-seccomp could move to parsing /etc/passwd and /etc/group13:59
zygajdstrand: (silly Friday idea): call snap-seccomp with setgid g:daemon and parse the word out of the filter ;)13:59
jdstrandbut that is a bigger change than I want to make13:59
* jdstrand goes with getent13:59
zygajdstrand: I think there are builtin go functions that give you that answer14:00
jdstrandI think getent will be much cheaper :)14:00
zyganot sure14:00
zygayou don't need to shell out to getent14:00
jdstrandI just googling-- people seem to have packages for it but not a part of the language14:00
* zyga looks14:00
jdstrandhmm, actually, this starting failing when I added the implied assumes most likely. that makes more sense14:01
zygajdstrand: os/user.LookupGroup14:01
jdstrandzyga: yes, that is what we are doing14:01
zygathen get group.GID if it didn't fail14:01
jdstrandwe are using LookupUser14:01
zygahold on? are we?14:01
zygawait, and that doesn't compile?14:02
* zyga is sceptical now14:02
jdstrandos.FindUid uses LookupUser14:02
zygacan you do a tiny program that calls LookupGroup?14:02
zygaLookupUser is old14:02
jdstrandLookupUser has always existed in golang14:02
zygaLookupGroup is new14:02
zyga*new enough for us though14:02
jdstrandthe test is failing on FindUid14:02
jdstrandyes, we stole LookupGroup from newer golang14:02
jdstrandand could remove that code14:03
zygaFindUid is different14:03
jdstrandI was actually planning on doing that in another PR since we now require 1.1014:03
zygaI mean14:03
roadmrhey 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
jdstrandbut why is FindUid failing?14:03
zygajdstrand: we can probably drop lookupGroup14:03
zygajdstrand: and just use the plain versions14:03
jdstrandzyga: yes, I just said that :)14:03
zygajdstrand: and that will get you out of the issue14:03
zygajdstrand: that is probably the only thing that requires C14:04
zygajdstrand: cool, ping me and mborzecki for review please14:04
jdstrandit will get me out of the issue for FindGid, but not FindUid, no?14:04
zygajdstrand: no, why?14:04
zygajdstrand: it's just the "import C" that breaks you14:04
zygadrop that from osutil and you are good14:04
jdstrandbecause of what I jus wrote?14:05
jdstrandfindUid does user.Lookup14:05
mborzeckihm had a branch somewhere that drops all C dependant bits from osutil14:05
zygajdstrand: I don't get it then, sorry, can you reprhase?14:05
zygajdstrand: calling user.Lookup is not the problem14:05
zygajdstrand: osutil *does not build*14:05
jdstrandthe spread test fail because of FindGid *and* FindUid14:05
zygajdstrand: when you disable C14:05
zygaat least not that part :)14:05
jdstrandok, not that makes more sense14:05
jdstrandnothing else used group.gp presumably14:06
mborzeckijdstrand: https://github.com/bboozzoo/snapd/commit/66e761e11bdb4f29ba279113b20c55d000035fda ?14:06
jdstrandlet me do a quick PR for that14:06
jdstrandmborzecki: so are you saying that FindUid also needs cgo even though it is using user.Lookup?14:07
zygajdstrand: it's one file14:08
zygathe whole file gets removed from the build process14:09
zygathat's all14:09
jdstrandzyga: right, I understand your point14:09
jdstrandthat makes sense14:09
jdstrandbut then mborzecki pointed me at https://github.com/bboozzoo/snapd/commit/66e761e11bdb4f29ba279113b20c55d000035fda which stubs FindUid14:09
jdstrandwhen it seems it may only need to stub FindGid14:09
jdstrandso I wanted a clarification on that14:10
jdstrandsince if FindUid also needs cgo, then the LookupGroup fix is valid on its own, but doesn't help me14:10
* zyga has a new flower pot :)14:11
jdstrandI mean, I can just try this out I guess14:11
jdstrandzyga: 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
jdstrandand yes, I plan to look at PRs today14:12
zygajdstrand: cool,14:12
zygajdstrand: I have a few piles of changes for snap-confine pending but I don't want to throw this all at you14:13
zygajdstrand: one thing that was bugging me a while ago was stdlib's IO functions that we cannot use because they apparently suck on error reporting14:13
zygajdstrand: so I wrote my own proof of concept with tests to show what happens in all error cases14:13
mborzeckijdstrand: 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 starving14:14
zygatime to eat14:14
jdstrandmborzecki: that is interesting, but I think a different answer to my question14:14
jdstrandmborzecki: I am going to do a PR to use user.LookupGroup14:14
jdstrandmborzecki: and remove all the cgo stuff in osutil14:15
jdstrandmborzecki: 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 bultin14:15
mborzeckijdstrand: that's tricky i think, we'll loose /etc/nss* and probably extrausers too14:16
jdstrandI see14:16
jdstrandso the builtins don't use nss at all14:16
jdstrandwe need extra users14:17
mborzeckiyup, let me point you to the source code14:17
jdstrandok, I think instead of using builtins, I can convert FindUid and FindGid to use getent14:17
mborzeckijdstrand: https://github.com/golang/go/tree/master/src/os/user look at cgo_lookup_unix.go and lookup_unix.go14:18
jdstrandactually, for this PR, I will probably use LookupGroup. then add a code comment that when we need extrausers, we will need to use getent14:18
jdstrandsince we don't need extrausers for this now14:19
jdstrandbut we will later14:19
jdstrandpasswd:         compat extrausers14:20
jdstrandfrom nsswitch.conf14:20
jdstrandok, this is all clear now14:20
jdstrandzyga, mborzecki: thanks14:20
mborzeckijdstrand: 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 bits14:22
jdstrandmborzecki: I'm sorry, if I'm using only builtins, what is the problem?14:23
jdstrandmborzecki: nothing except snap-seccomp and my PR use FindUid and FindGid14:25
mborzeckijdstrand: because builtins only know about /etc/groups & /etc/passwd14:25
jdstrandmborzecki: I know. but snap-exec and snap-update-ns don't do lookups... ?14:25
jdstrandmborzecki: at least not via these functions. I feel like I am missing your point14:26
mborzeckijdstrand: they don't, i stumbled upon this problem when trying to do the mapping for $HOME/snap_<instance> to $HOME/snap with parallel installs14:26
mborzeckijdstrand: which we eventually dropped anyway14:27
* zyga breaks for lunch 14:32
jdstrandzyga: fyi https://github.com/snapcore/snapd/pull/675914:49
zygaBack in 1014:54
zygaback now15:07
zygajdstrand: the error on https://travis-ci.org/snapcore/snapd/jobs/522179183 is puzzling!15:08
zygajdstrand: as a nitpick, could you please expand the commit message, it's nicer to know _why_ we are doing that not what is being done15:09
zygajdstrand: the error talks about build ID, I would suspect it is caused by lack of cgo now15:11
zygajdstrand: and default build mode that don't inject build-id15:11
Son_Gokuzyga, any idea from the snap side for this? https://github.com/livecd-tools/livecd-tools/issues/12515:17
zygahmm, no idea if we are using livecd-tools anymore15:18
zygait's a distro question15:18
zygafor snapd side I can say this15:18
zygayou can put seed data on a filesystem15:18
zygathen on 1st boot snapd spots the seed and installs the snaps listed there15:19
Son_GokuI don't know anything about those things15:19
zygathis is done for ubuntu live CD for instance15:19
Son_Gokuso it'd be handy if you could detail that for this15:19
zygabut I don't know anything about livecd-tools15:19
zygaI don't know much about this honestly, this is really mvo/chipaca area15:19
zygaI can try to ask them after easter15:20
Son_Gokulivecd-tools just basically uses a kickstart file as input to create live media15:20
Son_Gokuit's how Fedora and other derivatives of Fedora make media15:20
zyga*basically* is what most people would not use :)15:20
zygaI don't know how distros do that today15:20
* zyga (I mean the word basically assumes you know this)15:20
Son_GokuI use basically as "simply speaking"15:21
zygano worries :)15:21
zygaI'm looking for seeding docs15:22
zygaSon_Goku: I answered in the github issue now15:23
zygajdstrand: I commented on https://github.com/snapcore/snapd/pull/675915:35
zygaI'm back to hacking on groups15:36
zygaI solved my conundrum15:36
=== pstolowski is now known as pstolowski|afk
* zyga begs for review on https://github.com/snapcore/snapd/pull/671416:54
zygajdstrand: ping17:45
zygajdstrand: my last patch of the day17:48
zygajdstrand: 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 it17:49
zygajdstrand: https://github.com/snapcore/snapd/pull/676017:52
zygajdstrand: 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 issue17:53
* zyga waves17:53
sergiusenszyga: 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 it19:29
=== chihchun is now known as chihchun_afk
=== chihchun_afk is now known as chihchun
zygasergiusens: someone has staged real xdg-open and it took priority21:00
zygasergiusens: our xdg-open doesn't use dbus-launch and does not print that output21:00
zygajdstrand: thank you for both reviews21:15
* zyga started another print and goes to sleep21:16

Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!