compose: Add --check-passwd/group options, to fail if uids/gids change #56
Conversation
|
Just looked at changing to fmemopen+fgetpwent ... the main problem is that we can't sort the entries if we do it that way, and it only gives us an entry for one file. So we have to read all the entries for each file, duplicate them, sort them, and then compare resulting lists. Then we end up with more code than just parsing the files by hand (because split into lines, and sort are two lines atm). If you still want to do it, let me tomorrow, otherwise I'll look at merging the code for both files for monday. |
|
Agree the lack of comments in JSON is extremely painful. If we end up with a lot of code, split into a separate file? |
|
@james-antill Can you rebase against master and squash the two together? |
|
Ok, done. |
The two main reasons I did it that way are:
...1 could eaisly be sovled by cherry-pick if that ever gets in ;) ... and I guess you are happier with the policy at rpm-ostree level? I can certainly fix that if you want, although I did think about the file overrides checksum thing but it seemed like something where a normal user couldn't attack the builder user easily (you'd need access to the dir. where the builder is building, which would be restricted). |
9f55fb5 to
f3595c3
Compare
- rename internal functions like _string_in_ptr_array0 to the more common OBJECT_VERB_NOUN style, like: "ptrarray_contains_str" - Fix GError handling - we need a distinct output boolean, it's not allowed by convention to check *error - Also verify uid/gid on directories and symlinks; I can see security sensitivity here - Use gs_file_enumerator_iterate() as it's more convenient - Some control flow tweaks to clarify things
|
I made a branch which squashes your commits to one, then adds two on top: https://github.com/cgwalters/rpm-ostree/tree/wip/uid-gid-checking Can we use this as a basis for further iteration? |
|
With respect to the policy discussion (maybe let's call it "reference source"?), I can see some value if we defaulted to inspecting the previous commit. But yeah, anything more than that would probably be -toolbox territory. |
|
ok, push --force ping. Seems to work for me, and have a new -toolbox push as well that doesn't try to pass refs. |
|
If we go the treefile path for the last comment, I wonder if it'd make sense then to do it fully instead of commandline options? Something like: ? |
|
You seem really desperate to have people convert their passwd files into JSON dict data ... is there something I should know ;) But more seriously, having a simple previous/none type entry should be trivial to add. |
|
I'm also OK with reading the passwd data from an external file, but it seems like it would go along more with the rest of the tooling if we took input from the treefile instead of commandline arguments. This would require changing the -toolbox code to generate it in the json. The way I think of this is that given a treefile, I should be able to reproduce use it on another system to regenerate the tree. So anything that affects the output in a functional way should be there. On the other hand, things like --workdir-tmps are operational concerns. Whether or not I use a tmpfs shouldn't change the output, and that should be driven by build-side tools. WRT the format, I'm certainly not saying JSON is awesome, but it does the job. I guess we could support YAML too. I originally thought YAML was crazy, but experience with Ansible, cloud-init, and Kubernetes pods has warmed me to it a bit. |
|
JSON code push ping, note that this doesn't need any changes to -toolbox now as everything is only specified in the JSON. |
|
👍 to squash and push from me. After we have #75 let's see how hard doing unit testing for some of this is. |
|
Ok, cool. Squashed rebased and merged. |
The checking code from #56 landed, and started triggering for me on the `dockerroot` user. It's nice to know it works. Then the issue is... "what now"? It turns out in the case of `dockerroot` it's actually unused, so we could fix this by deleting it. But in general we need to support dynamic uids/gids/. And we can't yet take a hard dep on #49. So this patch changes things so we take a copy of the passwd/group data from the previous commit. Any users subsequently added in the *new* commit will be additive. Closes: coreos#78
rpm-ostree had code to check for this, which didn't actually work. See coreos/rpm-ostree#56 I don't see a no backwards compatibility concern in changing this, as it's unlikely a caller would try to sensibly disambiguate FAILED.
|
The check for |
|
Not sure that I explicitly tested this, but this is basically the code that ostree log uses, and I'm 100% sure that used to work. |
|
Nevermind, somewhere along the way it changed from ostree_repo_load_variant to ostree_repo_read_commit but only the former returns NOT_FOUND. |
No description provided.