[virt-tools-list] [virt-bootstrap] [PATCH v3 05/12] bootstrap: Implement UID/GID mapping

[Cedric Bosdonnat]

On Thu, 2017-07-20 at 12:29 +0100, Radostin Stoyanov wrote:
> When Libvirt creates LXC container with enabled user namespace the
> ownership of files in the container should be mapped to the specified
> target UID/GID.
> 
> Files and directories in the root file system are mapped as foll:
> 
> New file's UID/GID = Target UID/GID + File's UID/GID - User's UID/GID

The formula doesn't sound completely right. Imagine the (ideal) case
where the user creates the container and adds mapping corresponding to
his uid. The new file's UID would be 1000 + 0 - 1000, i.e: no change,
while it should be 1000.

I would drop the - User's UID/GID part of the formula, but may be there
is a case I'm overlooking where it is perfectly valid.

> 
> Example:
> - User which creates container:        uid/gid = 0
> - File extracted from container image: uid/gid = 0
> - Target:                              uid/gid = 1000
> 
> New file's uid/gid: 1000 = 1000 + 0 - 0
> 
> The new file's uid/gid will be 1000 on the host and 0 inside container
> with:
> 
> ```xml
>     <idmap>
>       <uid start='0' target='1000' count='10'/>
>       <gid start='0' target='1000' count='10'/>
>     </idmap>
> ```

Again, using the 4 spaces code block indentation would be more readable
with git log / git show.

> ---
>  src/virtBootstrap/virt_bootstrap.py | 53 +++++++++++++++++++++++++++++++++----
>  1 file changed, 48 insertions(+), 5 deletions(-)
> 
> diff --git a/src/virtBootstrap/virt_bootstrap.py b/src/virtBootstrap/virt_bootstrap.py
> index 98c629a..588d1f7 100755
> --- a/src/virtBootstrap/virt_bootstrap.py
> +++ b/src/virtBootstrap/virt_bootstrap.py
> @@ -69,12 +69,36 @@ def get_source(source_type):
>          raise Exception("Invalid image URL scheme: '%s'" % source_type)
>  
>  
> +def map_id(path, map_uid, map_gid):
> +    """
> +    Map the ownership of files in the root file system
> +
> +    New file's UID/GID = Mapped UID/GID + File's UID/GID - User's UID/GID
> +    """
> +    user_uid = os.geteuid()
> +    user_gid = os.getegid()
> +    path = os.path.realpath(path)
> +
> +    for root, _ignore, files in os.walk(path):
> +        for name in [root] + files:
> +
> +            filepath = os.path.join(root, name)
> +            stat_info = os.lstat(filepath)
> +            file_uid = stat_info.st_uid
> +            file_gid = stat_info.st_gid
> +
> +            new_uid = map_uid + (file_uid - user_uid)
> +            new_gid = map_gid + (file_gid - user_gid)
> +            os.lchown(filepath, new_uid, new_gid)
> +
> +
>  # pylint: disable=too-many-arguments
>  def bootstrap(uri, dest,
>                fmt='dir',
>                username=None,
>                password=None,
>                root_password=None,
> +              idmap=None,
>                not_secure=False,
>                no_cache=False,
>                progress_cb=None):
> @@ -104,9 +128,14 @@ def bootstrap(uri, dest,
>             no_cache=no_cache,
>             progress=prog).unpack(dest)
>  
> -    if fmt == "dir" and root_password is not None:
> -        logger.info("Setting password of the root account")
> -        utils.set_root_password(dest, root_password)
> +    if fmt == "dir":
> +        if root_password is not None:
> +            logger.info("Setting password of the root account")
> +            utils.set_root_password(dest, root_password)
> +
> +        if idmap is not None:
> +            logger.info("Mapping UID/GID")
> +            map_id(dest, *idmap)

Hum... that makes me wonder if we need the file mapping too if the filesystem
is in a qcow2 file, but I think so.

>  
>  
>  def set_logging_conf(loglevel=None):
> @@ -157,6 +186,8 @@ def main():
>                          help=_("Password for accessing the source registry"))
>      parser.add_argument("--root-password", default=None,
>                          help=_("Root password to set in the created rootfs"))
> +    parser.add_argument("-i", "--idmap", default=None,
> +                        help=_("UID / GID mapping of the created rootfs"))

-i usually doesn't mean idmap, but rather input. I'ld drop the short option
for this one.

Oh and I'm suddenly thinking that we have no man page at all for virt-bootstrap...
that is bad ;)

>      parser.add_argument("--no-cache", action="store_true",
>                          help=_("Do not store downloaded Docker images"))
>      parser.add_argument("-f", "--format", default='dir',
> @@ -173,8 +204,6 @@ def main():
>                          help=_("Show only the current status and progress"
>                                 "of virt-bootstrap"))
>  
> -    # TODO add UID / GID mapping parameters
> -
>      try:
>          args = parser.parse_args()
>  
> @@ -182,6 +211,19 @@ def main():
>              # Configure logging lovel/format
>              set_logging_conf(args.loglevel)
>  
> +        idmap = None
> +        if args.idmap:
> +            try:
> +                uid, gid = args.idmap.split(':')
> +                idmap = (int(uid), int(gid))
> +            except Exception:
> +                msg = ("Invalid UID / GID mapping value.\n"
> +                       "Supported format:\n\t"
> +                       "<uid>:<gid>\n"
> +                       "Example:\n\t"
> +                       "virt-bootstrap docker://fedora /tmp/foo -i 1000:1000")
> +                raise ValueError(msg)
> +

The format of the mapping should be documented in the help (and man later).
Libvirt has the concept of a maximum number of mapped UID/GID... even if docker
has not, we surely need to have that somehow in our options.

How about such options?

     --uidmap start[:len] and --gidmap start[:len]

--
Cedric

>          # do the job here!
>          bootstrap(uri=args.uri,
>                    dest=args.dest,
> @@ -189,6 +231,7 @@ def main():
>                    username=args.username,
>                    password=args.password,
>                    root_password=args.root_password,
> +                  idmap=idmap,
>                    not_secure=args.not_secure,
>                    no_cache=args.no_cache,
>                    progress_cb=args.status_only)