weixin_39661129
weixin_39661129
2020-12-27 03:53

lib/commit: Fix hardlink checkout commit with bare-user + mod xattrs

This is more subtle fallout from: https://github.com/ostreedev/ostree/pull/1170 AKA commit: 8fe45362578a43260876134d6547ebd0bb2485c3

Before, if we found a devino cache hit, we'd use it unconditionally.

Recall that bare-user repositories are very special in that they're the only mode where the on disk state ("physical state") is not the "real" state. The latter is stored in the user.ostreemeta xattr. (bare-user repos are also highly special in that symlinks are regular files physically, but that's not immediately relevant here).

Since we now have bare-user-only for the "pure unprivileged container" case, bare-user should just be used for "OS builds" which have nonzero uids (and possibly SELinux labels etc.)

In an experimental tool I'm writing "skopeo2ostree" which imports OCI images into refs, then squashes them together into a single final commit, we lost the the 81 group ID for /usr/libexec/dbus-1/dbus-daemon-launch-helper.

This happened because the commit code was loading the "physical" disk state, where the uid/gid are zero because that's the uid I happened to be using. We didn't just directly do the link speedup because I was using --selinux-policy which caused the xattrs to change, which caused us to re-commit objects from the physical state.

The unit test I added actually doesn't quite trigger this, but I left it because "why not". Really testing this requires the installed test which uses SELinux policy from /.

The behavior without this fix looks like:


-00755 0 0     12 { [(b'user.ostreemeta', [byte 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x51, 0x00, 0x00, 0x81, 0xed]), (b'security.selinux', b'system_u:object_r:lib_t:s0')] } /usr/lib/dbus-daemon-helper

which was obviously totally broken - we shouldn't be picking up the user.ostreemeta xattr and actually committing it of course.

该提问来源于开源项目:ostreedev/ostree

  • 点赞
  • 写回答
  • 关注问题
  • 收藏
  • 复制链接分享
  • 邀请回答

6条回答

  • weixin_39661129 weixin_39661129 4月前

    I think down the line we'll need to better distinguish between new content and existing hardlinks to make this pattern work well. For example doing unprivileged OS builds, we want to specify ostree commit --owner-uid 0 but only have that apply to new files. Which actually is what gnome-continuous has been doing for a long time, and that PR broke it too.

    Maybe something like a flag in the callbacks that say whether or not the object existed, then callbacks can decide what to do?

    点赞 评论 复制链接分享
  • weixin_39608509 weixin_39608509 4月前

    Ouch, yeah that makes sense. Just a question about the commin message:

    The unit test I added actually doesn't quite trigger this, but I left it because "why not". Really testing this requires the installed test which uses SELinux policy from /.

    It looks like this was done now, right?

    点赞 评论 复制链接分享
  • weixin_39661129 weixin_39661129 4月前

    Yep, that's done, just noting why I kept the unit test.

    点赞 评论 复制链接分享
  • weixin_39608509 weixin_39608509 4月前

    Oh I see. I misunderstood the "Really testing this requires..." to mean it wasn't part of this patch when it clearly was. But makes sense now!

    -atomic-bot r+ fc23e29

    点赞 评论 复制链接分享
  • weixin_39630126 weixin_39630126 4月前

    :hourglass: Testing commit fc23e29 with merge ed15723...

    点赞 评论 复制链接分享
  • weixin_39630126 weixin_39630126 4月前

    :sunny: Test successful - status-atomicjenkins Approved by: jlebon Pushing ed15723cd1688a7f2c003c7cbc95be202166c33d to master...

    点赞 评论 复制链接分享