Skip to content

Add a feature to pass user provided files during qcow2ova image conversion #851

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.

Sign up for GitHub

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

Open
Amulyam24 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add a feature to pass user provided files during qcow2ova image conversion #851

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 88 additions & 3 deletions cmd/image/qcow2ova/prep/prepare.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package prep

import (
"fmt"
"io"
"os"
"path/filepath"
"strings"
Expand All @@ -34,7 +35,7 @@ var (
// - Install and configure multipath for rootfs
// - Install all the required modules for PowerVM
// - Sets the root password
func prepare(mnt, volume, dist, rhnuser, rhnpasswd, rootpasswd string) error {
func prepare(mnt, volume, dist, rhnuser, rhnpasswd, rootpasswd, writeToDirPath string, writeFilesList []string) error {
lo, err := setupLoop(volume)
if err != nil {
return err
Expand Down Expand Up @@ -144,6 +145,19 @@ func prepare(mnt, volume, dist, rhnuser, rhnpasswd, rootpasswd string) error {
return err
}

@mkumatag mkumatag Aug 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[Correctness] Unconditional MkdirAll when no files are specified

When neither flag is provided, writeToDirPath is "" and writeFilesList is empty. filepath.Join(mnt, "") resolves to mnt (the image root), so MkdirAll runs on the root — harmless but unnecessary. More importantly, if a user passes --write-to-dir-path /some/path without --write-files-list, a spurious empty directory is silently created inside the image.

Wrap the entire block in a guard:

if len(writeFilesList) > 0 {
    imageWritePath := filepath.Join(mnt, writeToDirPath)
    if err := os.MkdirAll(imageWritePath, 0755); err != nil {
        return fmt.Errorf("failed to create directory %s: %w", imageWritePath, err)
    }
    for _, filePath := range writeFilesList {
        destinationPath := filepath.Join(imageWritePath, filepath.Base(filePath))
        if err := copyFiles(filePath, destinationPath); err != nil {
            return err
        }
    }
}

// Write user provided files to given path in the image

@mkumatag mkumatag Aug 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[Security] No path traversal guard on writeToDirPath

filepath.Join does not prevent ../ escape sequences. A user passing --write-to-dir-path ../../etc could resolve a path outside the intended mnt subtree.

Add a check after constructing imageWritePath:

if !strings.HasPrefix(filepath.Clean(imageWritePath), filepath.Clean(mnt)+"/") {
    return fmt.Errorf("write-to-dir-path %q escapes the image mount root", writeToDirPath)
}

imageWritePath := filepath.Join(mnt, writeToDirPath)
if err := os.MkdirAll(imageWritePath, 0755); err != nil {
return fmt.Errorf("failed to create directory %s: %w", imageWritePath, err)
}

for _, filePath := range writeFilesList {
destinationPath := filepath.Join(imageWritePath, filepath.Base(filePath))
if err := copyFiles(filePath, destinationPath); err != nil {
return err
}
}

err = Chroot(mnt)
if err != nil {
return err
Expand All @@ -169,19 +183,90 @@ func UmountHostPartitions(mnt string) {
}
}

func Prepare4capture(mnt, volume, dist, rhnuser, rhnpasswd, rootpasswd string) error {
func Prepare4capture(mnt, volume, dist, rhnuser, rhnpasswd, rootpasswd, writeToDirPath string, writeFilesList []string) error {
//cwd, err := os.Getwd()
//if err != nil {
// return err
//}
//defer os.Chdir(cwd)
switch dist := strings.ToLower(dist); dist {
case "rhel", "centos":
return prepare(mnt, volume, dist, rhnuser, rhnpasswd, rootpasswd)
return prepare(mnt, volume, dist, rhnuser, rhnpasswd, rootpasswd, writeToDirPath, writeFilesList)
case "coreos":

@mkumatag mkumatag Aug 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[Correctness] coreos silently drops user-provided files

The file-copy block only runs inside prepare(), which is only called for rhel/centos. For coreos, any --write-files-list entries are silently ignored. A user would get no error and no warning.

Either propagate the copy step for coreos too, or return an explicit error when writeFilesList is non-empty for that distro:

case "coreos":
    if len(writeFilesList) > 0 {
        return fmt.Errorf("--write-files-list is not supported for coreos distro")
    }
    klog.Info("No image preparation required for the coreos.")
    return nil

klog.Info("No image preparation required for the coreos.")
return nil
default:
return fmt.Errorf("not a supported distro: %s", dist)
}
}

func copyFiles(src, dest string) error {
fileInfo, err := os.Stat(src)
if err != nil {
return err

@mkumatag mkumatag Aug 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[Security] os.Stat follows symlinks transparently

os.Stat resolves symlinks, so if any entry in --write-files-list is a symlink (e.g. pointing to /etc/shadow), it will be silently read and copied into the image as a regular file. Consider using os.Lstat here and explicitly deciding whether to follow, skip, or error on symlinks.

}

@mkumatag mkumatag Aug 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[Readability] Trailing blank line before closing brace

    return copyFile(src, dest)
                                // ← remove this blank line
}

gofmt does not produce a blank line before a closing brace. Minor, but inconsistent with the rest of the file.

if fileInfo.IsDir() {
return copyDir(src, dest)
}

return copyFile(src, dest)

}

func copyFile(src, dest string) error {
in, err := os.Open(src)
if err != nil {
return err
}

@mkumatag mkumatag Aug 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[Performance] Double stat call in copyFile

os.Stat(src) was already called in copyFiles to determine if the path is a file or directory — then in.Stat() is called again here on the open file handle to retrieve permissions. You could pass fileInfo down from copyFiles to copyFile to avoid the redundant syscall:

func copyFile(src, dest string, srcInfo os.FileInfo) error {
    in, err := os.Open(src)
    ...
    out, err := os.OpenFile(dest, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, srcInfo.Mode())
    ...
}

defer in.Close()

srcInfo, err := in.Stat()
if err != nil {
return err
}

out, err := os.OpenFile(dest, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, srcInfo.Mode())
if err != nil {
return err
}

if _, err := io.Copy(out, in); err != nil {
out.Close()
return err
}

return out.Close()
}

func copyDir(src, dest string) error {
srcInfo, err := os.Stat(src)
if err != nil {
return err
}

if err := os.MkdirAll(dest, srcInfo.Mode()); err != nil {
return fmt.Errorf("failed to create directory %s: %w", dest, err)
}

entries, err := os.ReadDir(src)
if err != nil {
return err
}

for _, entry := range entries {
srcPath := filepath.Join(src, entry.Name())
destPath := filepath.Join(dest, entry.Name())

if entry.IsDir() {
if err := copyDir(srcPath, destPath); err != nil {
return err
}
} else {
if err := copyFile(srcPath, destPath); err != nil {
return err
}
}
}
return nil
}
7 changes: 6 additions & 1 deletion cmd/image/qcow2ova/qcow2ova.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,9 @@ Examples:
# Step 2 - Make the necessary changes to the above generated template file(bash shell script) - image-prep.template
# Step 3 - Run the qcow2ova with the modified image preparation template
pvsadm image qcow2ova --image-name centos-82 --image-dist centos --image-url /root/CentOS-8-GenericCloud-8.2.2004-20200611.2.ppc64le.qcow2 --prep-template image-prep.template

# For adding custom files to the image, run the qcow2ova with flags --write-files-list and --write-to-dir-path
pvsadm image qcow2ova --image-name --image-dist centos --image-url /root/CentOS-8-GenericCloud-8.2.2004-20200611.2.ppc64le.qcow2 --prep-template image-prep.template --write-files-list a.txt,b.log --write-to-dir-path /home/user

@mkumatag mkumatag Aug 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[Correctness — High] Missing flag co-validation: --write-files-list requires --write-to-dir-path

When --write-files-list is provided without --write-to-dir-path, writeToDirPath defaults to "". filepath.Join(mnt, "") resolves to mnt (the image root), so all user files get dumped at / inside the image — almost certainly not intended.

Add a validation check in PreRunE, consistent with how other flag dependencies are validated in this file:

if len(opt.WriteFilesList) > 0 && opt.WriteToDirPath == "" {
    return fmt.Errorf("--write-to-dir-path is required when --write-files-list is specified")
}

# Customize the cloud config and Convert image with user defined cloud config template.
# Step 1 - Dump the default cloud config template
Expand Down Expand Up @@ -263,7 +266,7 @@ Qcow2 images location:
klog.Info("Resize completed")

klog.Info("Preparing the image")
err = prep.Prepare4capture(mnt, rawImg, opt.ImageDist, opt.RHNUser, opt.RHNPassword, opt.OSPassword)
err = prep.Prepare4capture(mnt, rawImg, opt.ImageDist, opt.RHNUser, opt.RHNPassword, opt.OSPassword, opt.WriteToDirPath, opt.WriteFilesList)
if err != nil {
return fmt.Errorf("failed while preparing the image for %s distro, err: %v", opt.ImageDist, err)
}
Expand Down Expand Up @@ -300,6 +303,8 @@ func init() {
Cmd.Flags().StringVar(&pkg.ImageCMDOptions.OSPassword, "os-password", "", "Root user password, will auto-generate the 12 bits password(applicable only for redhat and cento distro)")
Cmd.Flags().StringVarP(&pkg.ImageCMDOptions.TempDir, "temp-dir", "t", os.TempDir(), "Scratch space to use for OVA generation")
Cmd.Flags().StringVar(&pkg.ImageCMDOptions.PrepTemplate, "prep-template", "", "Image preparation script template, use --prep-template-default to print the default template(supported distros: rhel and centos)")
Cmd.Flags().StringSliceVar(&pkg.ImageCMDOptions.WriteFilesList, "write-files-list", []string{}, "List of files to be copied to the image")
Cmd.Flags().StringVar(&pkg.ImageCMDOptions.WriteToDirPath, "write-to-dir-path", "", "User provided directory path where the provided files will be copied to")
Cmd.Flags().BoolVar(&pkg.ImageCMDOptions.PrepTemplateDefault, "prep-template-default", false, "Prints the default image preparation script template, use --prep-template to set the custom template script(supported distros: rhel and centos)")
Cmd.Flags().StringSliceVar(&pkg.ImageCMDOptions.PreflightSkip, "skip-preflight-checks", []string{}, "Skip the preflight checks(e.g: diskspace, platform, tools) - dev-only option")
Cmd.Flags().BoolVar(&pkg.ImageCMDOptions.OSPasswordSkip, "skip-os-password", false, "Skip the root user password")
Expand Down
2 changes: 2 additions & 0 deletions pkg/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ type imageCMDOptions struct {
TempDir string
PrepTemplate string
PrepTemplateDefault bool
WriteFilesList []string
WriteToDirPath string
CloudConfig string
CloudConfigDefault bool
OSPasswordSkip bool
Expand Down