pkg/tar: rename extractTar to ExtractTarInsecure #1616
Conversation
|
@dgonyeo Can we add a parameter to |
|
Doesn't matter to me which way as long as we expose a way to avoid chrooting. @jonboulle originally suggested I do it this way, want me to change it? |
|
I don't have strong feelings on this, but if we move to a single function I would want the docstring to have a warning/caveat lector about chroot=false |
|
I like having two functions better. |
|
I don't mind if it is one or two functions. But I wrote other concerns if |
| // ExtractTarInsecure extracts a tarball (from a tar.Reader) into the root | ||
| // directory if pwl is not nil, only the paths in the map are extracted. If | ||
| // overwrite is true, existing files will be overwritten. | ||
| func ExtractTarInsecure(tr *tar.Reader, overwrite bool, pwl PathWhitelistMap, uidRange *uid.UidRange) error { |
There was a problem hiding this comment.
uidRange *uid.UidRange
changes to
cb FilePermissionsEditor
where we have a generic interface that extractors can fulfill
type FilePermissionsEditor func(path string, fi os.FileInfo) error
(stupid name, hopefully you can come up with a better one)
line 169 becomes
if err := cb(p, fi); err != nil {
return err
}
what is currently in line 169-184 becomes
func makeUidShiftingFilePermissionsEditor(uidRange *uid.UidRange) FilePermissionsEditor
...
}
then you define a
func MaybeExtractThingiesAsTheEncodedUserIfWeAreRootOtherwiseJustExtractAsOurselves(path string, fi os.FileInfo) error {
....
}
There was a problem hiding this comment.
example ref for the latter one:
http://git.savannah.gnu.org/cgit/tar.git/tree/src/extract.c#n363
There was a problem hiding this comment.
Sorry, thickening the plot once again.
pkg/tar/chroot.go moves somewhere like common/tar/chroot.go
Then in acbuild you implement a version of that file, but it's simpler - no need for uid range or for the overwrite flag (I don't think?)
func ExtractTar(rs io.Reader, dir string, pwl PathWhitelistMap) error
Sorry for the yak shave but this is cleanup that I think has been needed for a long time. Packages under pkg/ should not have anything rkt specific and this one feels a bit leaky.
There was a problem hiding this comment.
pkg/aci/render.go calls ExtractTar in the pkg/tar/chroot.go file, so I don't think it can easily be moved out of pkg/tar.
There was a problem hiding this comment.
I kinda did what you wanted. You should check it over and tell me what else to tweak.
| type FilePermissionsEditor func(string, int, int, byte, os.FileInfo) error | ||
|
|
||
| func NewUidShiftingFilePermEditor(uidRange *uid.UidRange) (FilePermissionsEditor, error) { | ||
| user, err := user.Current() |
There was a problem hiding this comment.
This seems to fail on Semaphore:
run: error setting up stage0: error preparing stage1:
error rendering tree image: treestore: cannot render aci:
error extracting ACI: extracttar error: exit status 1, output:
error determining current user:
user: lookup userid 0: no such file or directory
However, I cannot reproduce it while ssh'ing on Semaphore...
There was a problem hiding this comment.
Because /etc/passwd doesn't exist in the chroot..?
There was a problem hiding this comment.
Perhaps on err it could just assume it's running as root? I feel like I might be missing some edge case with that though.
There was a problem hiding this comment.
Unless you actually need other user fields just use os.Geteuid()
On Mon, Oct 19, 2015 at 10:26 AM, Derek Gonyeo notifications@github.com
wrote:
In pkg/tar/tar.go
#1616 (comment):@@ -38,10 +39,47 @@ var ErrNotSupportedPlatform = errors.New("platform and architecture is not suppo
// root of the tar file and should be cleaned (for example using filepath.Clean)
type PathWhitelistMap map[string]struct{}-// extractTar extracts a tarball (from a tar.Reader) into the root directory
-// if pwl is not nil, only the paths in the map are extracted.
-// If overwrite is true, existing files will be overwritten.
-func extractTar(tr *tar.Reader, overwrite bool, pwl PathWhitelistMap, uidRange *uid.UidRange) error {
+type FilePermissionsEditor func(string, int, int, byte, os.FileInfo) error
+
+func NewUidShiftingFilePermEditor(uidRange *uid.UidRange) (FilePermissionsEditor, error) {
- user, err := user.Current()
Perhaps on err it could just assume it's running as root? I feel like I
might be missing some edge case with that though.—
Reply to this email directly or view it on GitHub
https://github.com/coreos/rkt/pull/1616/files#r42399857.
There was a problem hiding this comment.
Don't know how I missed that function, should be good now.
It would be useful in acbuild to be able to untar ACIs without chrooting, so this commit makes the function that does the extracting of the tar file exposed. With this change the tar package exposes an ExtractTar function for extracting an ACI inside of a chroot, and an ExtractTarInsecure function for extracting an ACI without any chroot happening. Additionally the ExtractTarInsecure function's logic related to the uidshift has been factored out into a function that gets passed in, so that clients using it can have better control over it.
|
I want to bump the version of rkt that acbuild uses to bring in #1689, and it'd be cool if I could get this at the same time. Anything additional tweaking this needs before it can get merged? |
|
LGTM |
pkg/tar: rename extractTar to ExtractTarInsecure
It would be useful in acbuild to be able to untar ACIs without
chrooting, so this commit makes the function that does the extracting of
the tar file exposed. With this change the tar package exposes an
ExtractTar function for extracting an ACI inside of a chroot, and an
ExtractTarInsecure function for extracting an ACI without any chroot
happening.