Conversation
In packaging more things and attempting to run with minimal perms, there's... it's possible for there to be a lot of friction around basics. This is starting work to address that, while also making it easier to configure permissions dropping out of box (and hopefully, make that a reasonable default). Signed-off-by: Eric Myhre <hash@exultant.us>
Signed-off-by: Eric Myhre <hash@exultant.us>
mknod is not at all safe by default; perhaps it's possible with some amount of apparmor, seccomp, or other LSMs, but with capabilities alone, no. See also: https://groups.google.com/forum/#!topic/fa.linux.kernel/ZluZ_clwvuM Signed-off-by: Eric Myhre <hash@exultant.us>
If you have a security model that depends on port numbering, you've got bigger problems. Meanwhile, I absolutely do not want to see people starting their in-container processess as root just so they can get webservers on port 80. And as lovely as the power tools of some other networking layers are, they're not an out-of-box feature target for repeatr right now. tl;dr I see only friction from restricting this cap and absolutely zero problems allowing it, even if there's arguably lots of complex things you could also do to make it less relevant. Signed-off-by: Eric Myhre <hash@exultant.us>
Signed-off-by: Eric Myhre <hash@exultant.us>
Signed-off-by: Eric Myhre <hash@exultant.us>
The latter is to be disablable since it causes fs mutations, and we sometimes might want to explicitly avoid such inheritable things, etc. Defaulting env vars might sometimes be unwanted, but I don't really anticipate a lot of impetus to do so, and it's already easily overriden within existing config. Signed-off-by: Eric Myhre <hash@exultant.us>
Now cover the fields we're considering treating as mutable for the first time with this cradle work. Signed-off-by: Eric Myhre <hash@exultant.us>
The new names for the Policy constants are specifically chosen to avoid mentioning "root", because that's been *immediately* a seed of confusion in every conversation about the subject over the last few days (including in my own earlier docs about fakeroot: it's clearly inaccurate to say that needing uid=0 to alter files owned by uid=0 is an incorrect request for "root", natch, right?). Signed-off-by: Eric Myhre <hash@exultant.us>
Signed-off-by: Eric Myhre <hash@exultant.us>
I have no idea why I originally wrote it to just to the type part of the mode. There's no earthly reason to intentionally tolerate untested behavior on the other bits. Signed-off-by: Eric Myhre <hash@exultant.us>
Signed-off-by: Eric Myhre <hash@exultant.us>
Signed-off-by: Eric Myhre <hash@exultant.us>
…ily. At this point, we need to map from the Policy info to a UID to define some of those fields. I'm hesitant to go too far on that right away (YAGNI senses are tingling hard), but otoh, I'm pretty sure we're going to end up having a parser and writer for passwd files at some point shortly (even if that gets moved behind the cuttoff for this first PR, sadly I'm pretty sure it's coming -- I recall, for one random but certainly not rare or unique example, real nasty behavior in python when it finds your uid doesn't easily map to a name, which I don't think is a pain we should allow to be associated with low-priv operation)... and that means we might wanna just have a struct right now for representing all the smutchz necessary for that. Going to take a middle road and have a struct right off the bat, at least. Perhaps populate more fields later on-demand. Signed-off-by: Eric Myhre <hash@exultant.us>
Leave todo in user account name handling. That requires significant code quantity that nothing else does right now (handling /etc/passwd and its ilk, not just parsing but sanity checking and re-emitting), so I'm kicking that down the road to at least one PR after this. Signed-off-by: Eric Myhre <hash@exultant.us>
This will be a fairly significant and possibly breaking change to defaults! We're in alpha, wheeee! - The default CWD is now `/task`. If you previously relied on the default value of "/", that'll break. - If you have a formula which explicitly should *not* modify its filesystem, at all, you'll now need to configure 'action.cradle' to be false. The $HOME env var is now also set by default. On the plus side, getting your CWD created by default is going to remove a teeny tiny irritant from a *lot* of formulas, going by my own sample set. Updated executor tests to match these new behaviors. The new security component of Policy and the userid dropping is still not yet invoked. Signed-off-by: Eric Myhre <hash@exultant.us>
Does indeed do the right thing, happily. Signed-off-by: Eric Myhre <hash@exultant.us>
Note: since yes, priv dropping *is* the default, this is fairly huge. In particular, `repeatr twerk` will now *not* give you uid=0! (Guess we'll probably want to add flags to that subcommand to crank it back up...) Signed-off-by: Eric Myhre <hash@exultant.us>
Not... tested. I'm not entirely sure how to tackle this, tbh. I think I'll have to slowly accumulate a corpus of "interesting" commands to run inside the full container to probe the boundaries of the security. This will be a big and ongoing project. (Possibly an entirely separate binary -- capsprobe? ...Oh, I guess I could use capsh for now. But I kinda want to get something that actually *tries* to do the_things and then asserts that they error exactly as much as expected, because that's more topically on point to the actual security of it all.) Signed-off-by: Eric Myhre <hash@exultant.us>
Runc is erroring out and calling CAP_AUDIT_READ unknown. I'm somewhat confused by this, because it *is* documented. Maybe it's doing detection of my system's available caps (I don't have a sufficiently recent kernel on this host to have this particular cap), but I haven't seen such code yet... Commenting it out and moving on for the moment because I don't anticipate dropping CAP_AUDIT_READ even on systems that support it to be problematic. Signed-off-by: Eric Myhre <hash@exultant.us>
c6cfc56 to
de5bb6a
Compare
This involves, alarmingly, upping the Policy for the repeat-thyself formula to have slightly more than the routine. Why? It's covered by our acceptance tests... and it's using the git transmat. The git transmat is currently operating with more permissions than it should, and also leaving ownership of the checkout set to uid=0:gid=0, which lands this script in hot water unless it can write there. We'll need to address this asap. Signed-off-by: Eric Myhre <hash@exultant.us>
The default perms for a transmat system with no inherent concept of perms and no filters configured should be 1000/1000. Also, yeah, we should just plain be running git with far few privs. This always should have been the case -- enabling priv-dropping by default is now shining a light directly on it (acceptance tests started failing!). Yay safe defaults: calling BS where BS should indeed be called. Aside: I continue to wish that it's reasonable to run the transmats themselves in sandboxes (anything outside of simple tar usage, at least; that one we're fairly willing to be confident of the code quality on)... and yet I continue to not do that, because it seems valuable to design transmats to work alone, moving things on and off the host filesystem in save/load subcommands, which should not themselves necessarily require priviledges or even container support, and that means continuing to refrain from using containers for the transmats. Maybe someday there'll be a happier middle road in the future that gives us more isolation to the transmats without onerous requirements. Or we'll design a way to make containers optional such that they provide failsafes when present while avoiding behavioral changes so that the transmats still work the same uncontained (just without the failsafes)... which come to think of it, actually sounds possible. Future work. Signed-off-by: Eric Myhre <hash@exultant.us>
Unfortunately... This is backing out on getting git content permissions right by actually running git as a less privileged user in the first place. That *would* be desirable, but see the comments in the commit body for an explanation of the current state of defeat in detail. This subject will need to be revisited with significantly more firepower in the future. On the plus side, with this change, the ownership and bits exposed by the git transmat to the inside of containers is not just correct, but correct in such a way that the less excellent internal parts are hidden... so when we *do* revisit this to make better engineered and lower-priviledged git transmat guts, it should be a completely transparent, not-at-all-breaking change. Signed-off-by: Eric Myhre <hash@exultant.us>
8342481 to
4d5ce53
Compare
And tests to confirm errors are raised when appropriate. Signed-off-by: Eric Myhre <hash@exultant.us>
Member
Author
|
Updating things to work with this is a little friction, but starting to evaluate formulas with lower privileged policies is already forcing quality improvements in pretty much every formula that's been affected by the change 👍 It's also quite nice to be able to flip between |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Working with containers should not represent a major regression towards high-privileged operations. There, I said it.
And as we all know by now -- this is a siren song that requires active resistance.
So, here's the first major steps in making Repeatr's default modes of operation switch immediately and conveniently to lower privilege -- non-root UIDs at bare minimum, and more advanced mechanisms like dropping capabilities as much as possible if an executor engine supports it.
Key concerns driving the design are as follows:
Policychoices. Policies should be as uncomplicated as possible: they are not multiple choice, they are not swiss cheese, and they are roughly ranked in asafety <-> powercontinuum, so that it's always easy to say "you should move to the left if you can".Policydescribes some amount of Minimum Viable Preparations to match the privilege level. If you are operating within a safe, minimally-privileged Policy, then Repeatr will default to making sure your process has some valid space to work (a writable dir, a$HOME, etc).Emphasis remains on "minimal" preparations -- their existence at all is a concession. The point here is to address needs that are pretty much a stretch mark leftover from ancient history when there was a grizzled sysadmin granting you the hallowed keys to the monstrously babied university mainframe, when simply don't have such a role in the modern world (certainly not in this context, anyway). These preparations could be shrugged off to users of repeatr by using a prior stage in a pipeline to set them up, but doing so would raise two other problems: it would create an implicit contract between formulas for something very very basic (explicit is better than implicit, and no requirement at all is better than explicit), and also that setup stage would tend to require
PolicyGoverneritself, which seems rather like losing ground again. Thus, the concessions. But on principle, Repeatr should never be presumptive about your flow, and every little piece of magic we do lets people get away with making crappier software, so making the fewest exceptions here is remains important, and also, they are to be easily disabled.This is quite likely a breaking change for many formulas. Anything that assumed it has uid=0... doesn't, anymore. Standard alpha disclaimer applies here! 😈
Policies are not a do-all end-all solution for arbitrarily complex security needs, and they're not intended to be. If you have really specific capability desires, that's... cool, but Policies are not going to help, and a more correct behavior is to reach down into the per-executor configuration to really tweak. More knobs is not a happier thing in security.
In the future, the hope is to produce an increasingly large body of software which works normally at the safest Policy levels. In order to encourage this, one could imagine a build farm which accepts jobs... but only runs them if the Policy is the lowest level, "Routine", for example. The high-level approach of policies instead of exposing all conceivable fine-grained details makes rules like that possible, simple to express, and immediately comprehensible.
All that's the hope, anyway. But one thing's for sure: after this merges, we can run things as uid=$somebody instead of uid=root. And that can't possibly be bad.