fix: build image layer for dir symlink #350
Conversation
Summary of ChangesHello @FengyunPan2, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a bug related to the incorrect handling of directory symlinks during the image layer build and extraction phases. Previously, directory symlinks were not properly recognized, leading to potential issues in layer integrity and content. The changes introduce robust mechanisms to detect directory symlinks, store their target paths in layer metadata, and accurately recreate them during extraction, thereby ensuring that the built image layers correctly reflect the intended filesystem structure, including symbolic links. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for handling directory symlinks when building image layers. The changes are in the right direction, but I've found a critical security vulnerability, a bug, and several areas for improvement in terms of correctness, efficiency, and code clarity. Please see my detailed comments.
| dstFullPath := filepath.Join(outputDir, fileMetadata.DstLinkPath) | ||
| if err := os.MkdirAll(dstFullPath, 0755); err != nil && !os.IsExist(err) { | ||
| return fmt.Errorf("failed to create directory %s: %w", dstFullPath, err) |
There was a problem hiding this comment.
This block of code introduces a critical path traversal vulnerability. fileMetadata.DstLinkPath is not sanitized, and if it contains .. or is an absolute path, os.MkdirAll could write outside the intended outputDir.
Additionally, this function's responsibility should be to extract the artifact, which means creating the symlink. It should not be responsible for creating the symlink's target.
I strongly recommend removing lines 149-151. The os.Symlink call on line 157 is sufficient to create the symlink, and it will correctly be a dangling symlink if the target does not exist.
| switch { | ||
| case info.Mode().IsRegular(): | ||
| metadata.Typeflag = 0 // Regular file | ||
| metadata.Typeflag = tar.TypeReg // Regular file | ||
| case info.Mode().IsDir(): | ||
| metadata.Typeflag = 5 // Directory | ||
| metadata.Typeflag = tar.TypeDir // Directory | ||
| lInfo, err := os.Lstat(path) | ||
| if err != nil { | ||
| return metadata, fmt.Errorf("failed to get file info: %w", err) | ||
| } | ||
| isSymlink := lInfo.Mode()&os.ModeSymlink != 0 | ||
| if isSymlink { | ||
| // 对于符号链接，获取其指向的目标信息 | ||
| dstLinkPath, err := os.Readlink(path) | ||
| if err != nil { | ||
| return metadata, fmt.Errorf("failed to read symlink: %w", err) | ||
| } | ||
| logrus.Debugf("builder: symlink detected for file %s -> %s", path, dstLinkPath) | ||
| metadata.Typeflag = tar.TypeSymlink // Symlink | ||
| metadata.DstLinkPath = dstLinkPath | ||
| } | ||
|
|
||
| case info.Mode()&os.ModeSymlink != 0: | ||
| metadata.Typeflag = 2 // Symlink | ||
| metadata.Typeflag = tar.TypeSymlink // Symlink | ||
| default: | ||
| return metadata, errors.New("unknown file typeflag") | ||
| } |
There was a problem hiding this comment.
There are a few issues with the logic in getFileMetadata:
- Unreachable code: The case
case info.Mode()&os.ModeSymlink != 0:on line 500 is unreachable.infocomes fromos.Stat, which follows symlinks, soinfo.Mode()will never have theos.ModeSymlinkbit. This is a bug. - Inefficiency: The
os.Lstatcall on line 484 is redundant.BuildLayeralready callsos.Lstaton the same path. This information could be passed down to avoid a second system call. - Incomplete symlink handling: The new logic correctly handles symlinks to directories. However, symlinks to files will fall into the
case info.Mode().IsRegular()and be treated as regular files, not preserved as symlinks.
To fix this, getFileMetadata should be refactored to use os.Lstat at the beginning to determine the file type (including all symlinks) and then proceed accordingly. This would fix all three issues.
| } | ||
| isSymlink := lInfo.Mode()&os.ModeSymlink != 0 | ||
| if isSymlink { | ||
| // 对于符号链接，获取其指向的目标信息 |
There was a problem hiding this comment.
This comment is in Chinese. For consistency and maintainability of the codebase, please translate it to English.
| // 对于符号链接，获取其指向的目标信息 | |
| // For a symbolic link, get its target information. |
| return ocispec.Descriptor{}, fmt.Errorf("failed to copy content to hash: %w", err) | ||
| } | ||
| digest := fmt.Sprintf("sha256:%x", hash.Sum(nil)) | ||
| desc, err := ab.strategy.OutputLayer(ctx, mediaType, relPath, digest, size, reader, hooks) |
There was a problem hiding this comment.
The reader is consumed by io.Copy on line 163 but is not reset before being passed to ab.strategy.OutputLayer. While this might work for an empty reader, it's fragile and inconsistent with computeDigestAndSize, which does reset the reader. Please reset the reader to its beginning before this line. Since a code suggestion cannot add lines, I'll describe the change. Before this line, please add:
if _, err := reader.Seek(0, io.SeekStart); err != nil {
return ocispec.Descriptor{}, fmt.Errorf("failed to reset reader for symlink: %w", err)
}| return fmt.Errorf("failed to remove file %s: %w", fullPath, err) | ||
| } | ||
| if err := os.Symlink(fileMetadata.DstLinkPath, fullPath); err != nil { | ||
| return fmt.Errorf("failed to create symlink from %s to %s: %w", fullPath, dstFullPath, err) |
There was a problem hiding this comment.
The error message is slightly confusing. dstFullPath is the resolved path of the symlink target, but the symlink itself contains the unresolved path fileMetadata.DstLinkPath. To make debugging easier, consider using fileMetadata.DstLinkPath in the error message to show what the symlink is pointing to.
| return fmt.Errorf("failed to create symlink from %s to %s: %w", fullPath, dstFullPath, err) | |
| return fmt.Errorf("failed to create symlink %s pointing to %s: %w", fullPath, fileMetadata.DstLinkPath, err) |
| } | ||
| isSymlink := lInfo.Mode()&os.ModeSymlink != 0 | ||
| if isSymlink { | ||
| // 对于符号链接，获取其指向的目标信息 |
There was a problem hiding this comment.
It would be great if comment in English
|
|
||
| # Dependency directories (remove the comment below to include it) | ||
| # vendor/ | ||
| /vendor |
There was a problem hiding this comment.
Do we need to ignore vendor in this PR?
2 participants
Fix: #345