Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rootless: don't create a namespace unless for containers-storage #653

Merged
merged 2 commits into from May 18, 2019

Conversation

TristanCacqueray
Copy link
Contributor

@TristanCacqueray TristanCacqueray commented May 16, 2019

This change fixes skopeo usage in restricted environment such as
bubblewrap where it doesn't need extra capabilities or user namespace
to perform its action.

Close #649

Signed-off-by: Tristan Cacqueray tdecacqu@redhat.com

@vrothberg
Copy link
Member

@vrothberg vrothberg commented May 16, 2019

This looks like a clever solution to me. @giuseppe PTAL

@giuseppe
Copy link
Member

@giuseppe giuseppe commented May 16, 2019

thanks for the patch! Yes, I think it is a good fix, and should also address #652

@TristanCacqueray
Copy link
Contributor Author

@TristanCacqueray TristanCacqueray commented May 16, 2019

You're welcome, thank you for the prompt review. FWIW I tried using a cleaner method (such as alltransports.ParseImageName) but I couldn't find the right predicate to test for containers-storage type. The proposed solution assumes all the containers-storage image reference start with the containers-storage: prefix.

@giuseppe
Copy link
Member

@giuseppe giuseppe commented May 16, 2019

@mtrmac is there any cleaner way to test that? Are you fine with this approach?

@vrothberg
Copy link
Member

@vrothberg vrothberg commented May 16, 2019

One way could be to use ref, err := ParseImageName(...) and if err == nil to compare ref.Transport().Name() == storage.Transport.Name().

@TristanCacqueray
Copy link
Contributor Author

@TristanCacqueray TristanCacqueray commented May 16, 2019

The issue with ParseImageName is that it may fail early with chown /home/fedora/.local/share/containers/storage/vfs: operation not permitted

@vrothberg
Copy link
Member

@vrothberg vrothberg commented May 16, 2019

Ouch, sorry for missing that. In this case, we can mimic what ParseImageName(...) effectively does:

parts := strings.SplitN(imgName, ":", 2)
if len(parts) != 2 {
   continue
}
if parts[0] == storage.Transport.Name() { // we have storage reference (but can't validate it yet) }

@TristanCacqueray
Copy link
Contributor Author

@TristanCacqueray TristanCacqueray commented May 16, 2019

Done, thank you for the suggestion.

Copy link
Member

@vrothberg vrothberg left a comment

LGTM, thanks.

@mtrmac @rhatdan PTAL

@rhatdan
Copy link
Member

@rhatdan rhatdan commented May 16, 2019

LGTM
Although I still want @mtrmac to chime in.

@mtrmac
Copy link
Collaborator

@mtrmac mtrmac commented May 16, 2019

The issue with ParseImageName is that it may fail early with chown /home/fedora/.local/share/containers/storage/vfs: operation not permitted

Oh, wow. That’s… a mess.

@nalind AFAICS this is essentially because storageTransport.ParseStoreReference needs a working store so that it can support image ID prefixes:

if img, err := store.Image(possibleID); err == nil && img != nil && len(possibleID) >= minimumTruncatedIDLength && strings.HasPrefix(img.ID, possibleID)

and the like. Is there any chance we could drop that functionality? (I guess not, but it would make dealing with this in callers so much simpler.)

Alternatively, would it be reasonably possible for c/storage.GetStore to return a typed error indicating that “this would work in a user namespace”?

Copy link
Collaborator

@mtrmac mtrmac left a comment

(Assuming that we can’t wiggle out of having to do this by changing c/storage or c/image/storage.)

cmd/skopeo/utils.go Outdated Show resolved Hide resolved
func needsRexec(c *cli.Context) error {
return maybeReexec()
for _, arg := range c.Args() {
Copy link
Collaborator

@mtrmac mtrmac May 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is way too imprecise. Instead:

  • In the various commands, drop Before: needsReexec
  • In each of the command handlers, run something like maybeReexec(inputParameter) only for the relevant parameters, something like:
        inputImageName := image.args[0]
        if err := maybeReexec(inputImageName); err != nil { 
           return err
        }
        … := parseImageSource(ctx, opts.image, inputImageName) // or parseImage or alltransports.ParseImageName
    possibly rearranging this so that the maybeReexecs happen at the start of the command handlers, before any other real work gets done.

cmd/skopeo/utils.go Outdated Show resolved Hide resolved
@fungi
Copy link

@fungi fungi commented May 16, 2019

Just for another data point, we're temporarily running with a16489e applied in production and it seems to have solved #649 for us. Thanks @TristanCacqueray!

@TristanCacqueray
Copy link
Contributor Author

@TristanCacqueray TristanCacqueray commented May 17, 2019

@mtrmac Thank you for the suggestions, I think the last change implements them along with containers/image#631 for the TransportFromImageName procedure. Not sure what is the policy around vendoring thus I inlined the c/image change in that PR.

As @fungi mentioned, the opendev.org infrastructure is using a continuously built version of skopeo which triggered the issue of using reexec in unprivileged environment like bubblewrap.

Copy link
Collaborator

@mtrmac mtrmac left a comment

ACK to the overall approach.

Not sure what is the policy around vendoring thus I inlined the c/image change in that PR.

Sure. Ideally, update vendor.conf, run make vendor, and commit that as a separate commit from the other changes, in the same PR.

cmd/skopeo/copy.go Outdated Show resolved Hide resolved
cmd/skopeo/layers.go Outdated Show resolved Hide resolved
cmd/skopeo/unshare_linux.go Outdated Show resolved Hide resolved
@TristanCacqueray TristanCacqueray force-pushed the master branch 2 times, most recently from 81aeea4 to a6ade2c Compare May 18, 2019
cmd/skopeo/copy.go Show resolved Hide resolved
vendor.conf Outdated
@@ -2,7 +2,7 @@
github.com/urfave/cli v1.20.0
github.com/kr/pretty v0.1.0
github.com/kr/text v0.1.0
github.com/containers/image ff926d3c79684793a2135666a2cb738f44ba33dc
github.com/containers/image 2d92e2533d517b6db4ae7d085a332915fec57867
Copy link
Collaborator

@mtrmac mtrmac May 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That commit does not actually exist yet, which would make make vendor not reproducible. After containers/image#631 is merged, please update this with the actual merge commit and re-vendor.

Copy link
Contributor Author

@TristanCacqueray TristanCacqueray May 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Depends-On: containers/image#631
Signed-off-by: Tristan Cacqueray <tdecacqu@redhat.com>
This change fixes skopeo usage in restricted environment such as
bubblewrap where it doesn't need extra capabilities or user namespace
to perform its action.

Close containers#649
Signed-off-by: Tristan Cacqueray <tdecacqu@redhat.com>
@mtrmac
Copy link
Collaborator

@mtrmac mtrmac commented May 18, 2019

LGTM. Thanks!

@mtrmac mtrmac merged commit 2b50861 into containers:master May 18, 2019
1 check passed
TristanCacqueray added a commit to TristanCacqueray/skopeo that referenced this issue May 19, 2019
This change adds a couple of tests to prevent further regression
introduced by containers#653

Signed-off-by: Tristan Cacqueray <tdecacqu@redhat.com>
openstack-gerrit pushed a commit to openstack/openstack that referenced this issue May 21, 2019
* Update system-config from branch 'master'
  - Merge "Revert "Pin skopeo to unbreak skopeo+bubblewrap""
  - Revert "Pin skopeo to unbreak skopeo+bubblewrap"
    
    This reverts commit 0d370a285b09bd28c5b1cdfc6b89d2997f67da5d.
    
    Fixed by containers/skopeo#653 so safe to
    merge this once a new build appears in the PPA.
    
    Change-Id: I858eee79d084016b6b71eec46a6118d78f68cafa
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants