From: Stefan Sperling Subject: Re: FreeBSD - Progress on applying Capsicum to got To: Yang Zhong Cc: gameoftrees@openbsd.org Date: Tue, 24 Nov 2020 22:16:20 +0100 On Tue, Nov 24, 2020 at 12:13:05PM -0800, Yang Zhong wrote: > Hello, > > I'm the intern working on applying Capsicum to got. I have adapted the > init and checkout commands to use Capsicum, as a proof of concept. The > changes need polish for things like the names of variables and the order > of include directives, and there are some very minor unresolved issues, > but it's enough to get a good idea of what is necessary. Almost all of the > changes are for checkout, so I'll be discussing that. > > The first of the following diffs contains all the relevant Capsicum changes. > The second contains the FreeBSD compatibility changes on top of which I > worked. > https://people.freebsd.org/~emaste/got/got_cap_poc.diff > https://people.freebsd.org/~emaste/got/freebsd-0.43.diff > > Most changes involve turning open() style functions to openat() style > ones, and many functions now take in fds to support this. The got_repo > and got_worktree structs also now store fds corresponding to their paths. > In general, the program now passes around fds for the root worktree and > repo directories, and operates on paths relative to those fds. > > As a consequence to this, got_worktree_open now takes in the fd of its > associated repository, as capsicum's capability mode needs the directory > to be pre-opened. For checkout this works since we call got_repo_open > beforehand anyway, which requires the same fd. However, future commands > will need a function that grabs the repository path from a worktree > directory, or similar, to run before entering capability mode. > > Another consequence: got_repo_open no longer loops through progressively > higher directories to find the repo's root, as this must be done before > entering capability mode. The new function got_repo_find_git_path does > the looping, and we call it early on. > > The rest of the changes are self-contained: got_privsep_wait_for_child > replaces wait4 with kqueue, as the former is forbidden in capability > mode. Similarly, got_privsep_exec_child uses fexecve instead of execl. > Since some of these aren't suitable for upstream, I'd appreciate your > thoughts on how these areas could be abstracted going forward. > > A note: these changes affect the signatures of many common functions > (got_repo_open, got_worktree_open, got_ref_open, etc). I've filled > them with dummy parameters in places where they aren't being > called, and have added printed warnings before them for my reference. > This means that commands other than init and checkout will not work. > > I've lightly tested my work, and checkout works with both got- and > git-style repositories. All the flags work in simple cases as well. > > Is the direction of these changes agreeable? I believe that changes in > this style will suffice to Capsicumize almost all of got. I'd greatly > appreciate any feedback on how suitable these changes are. If these > changes are agreeable, we'll apply them to the rest of got and submit > them as logically-separated patches. > > Yang Zhong > Hi, Thank you very much for sharing this work. The changes are larger than I expected, in my ignorance of capsicum. I did not expect capsicum to require this many changes to existing code. There are calls to functions such as mkostempsat() which do not exist on OpenBSD. I'd rather not add such changes to OpenBSD-specific upstream code. Generally, I welcome patches which make use of interfaces that are readily available on both systems. But I don't have enough spare time and energy to co-maintain code that supports features specific to other operating systems. It would be easier for me if OS-specific changes were maintained in a separate -portable version of Got (which I could provide hosting for if desired), or a FreeBSD-specific version if that's what you prefer. That said, I do want to help you with keeping differences between the versions at a minimum. I do like the changes which fix races inherent in using plain paths to refer to already open files and directories, such as replacing use of rename() with renameat(), or passing a directory fd to read_meta_file() in order to replace open() with openat(). If you could break out such changes as separate patches against latest upstream code without any capsicum-specific bits, this would be very welcome and should already help with reducing differences somewhat. There are many places where upstream code will benefit from passing around file descriptors, such as the status walk. But for functions where we would just end up passing -1 everywhere in upstream code it adds too much noise. I'd like to find some middle ground with you. Wherever you need an open file descriptor stored in a struct, I am not opposed to adding those to upstream code, even if the upstream code itself doesn't make use of such fds beyond opening and closing them. We could also look at changing some functions to expect pointers to structs instead of individual arguments, so that you can more easily access an fd variable without having to change the function's parameter list, where possible. Cheers, Stefan