Add --untar and --untardir flags to policy pull command #206

thc1006 · 2025-10-08T03:16:59Z

Implements #107: Add possibility to untar policy content to disk when pulling from OCI registry.

This change simplifies the workflow from 3 steps to 1 step:

Before: policy pull + policy save + tar extraction
After: policy pull --untar --untardir

Changes:

Add --untar flag to enable automatic extraction after pull
Add --untardir flag to specify extraction directory (default: ".")
Implement ExtractPolicyBundle() with security features:
- Path traversal attack prevention
- Safe symlink handling
- Directory creation on demand
Add ErrExtractFailed error type
Update Pull() method signature to support new parameters
Maintain backward compatibility (existing code unchanged)

Security:

Validates all file paths to prevent writing outside target directory
Safely handles symlinks by checking resolved targets
Skips unknown file types with warnings

Inspired by Helm's helm pull --untar --untardir functionality.

Testing:

Backward compatibility verified (existing tests pass)
Code compiles successfully
Help text displays new flags correctly

Implements opcr-io#107: Add possibility to untar policy content to disk when pulling from OCI registry. This change simplifies the workflow from 3 steps to 1 step: - Before: policy pull + policy save + tar extraction - After: policy pull --untar --untardir <dir> Changes: - Add --untar flag to enable automatic extraction after pull - Add --untardir flag to specify extraction directory (default: ".") - Implement ExtractPolicyBundle() with security features: * Path traversal attack prevention * Safe symlink handling * Directory creation on demand - Add ErrExtractFailed error type - Update Pull() method signature to support new parameters - Maintain backward compatibility (existing code unchanged) Security: - Validates all file paths to prevent writing outside target directory - Safely handles symlinks by checking resolved targets - Skips unknown file types with warnings Inspired by Helm's `helm pull --untar --untardir` functionality. Testing: - Backward compatibility verified (existing tests pass) - Code compiles successfully - Help text displays new flags correctly Signed-off-by: thc1006 <[email protected]>

gertd · 2025-10-08T08:55:42Z

cmd/policy/pull.go


 type PullCmd struct {
 	Policies []string `arg:"" name:"policy" help:"Policies to pull from the remote registry."`
+	Untar    bool     `name:"untar" help:"Untar the policy bundle after pulling."`


Why have two flags? It feels redundant, as you always have to validate UntarDir, so you might as well use the fact that when UntarDir is set instead of --untar

I do not think there exists a real use case where --untardir is set and --untar == false

gertd · 2025-10-08T08:57:54Z

cmd/policy/pull.go

 type PullCmd struct {
 	Policies []string `arg:"" name:"policy" help:"Policies to pull from the remote registry."`
+	Untar    bool     `name:"untar" help:"Untar the policy bundle after pulling."`
+	UntarDir string   `name:"untardir" help:"Directory to extract the policy bundle." default:"." type:"path"`


From a security POV, I would require that --untardir must point to an existing directory.

To enforce this, please use type:"existingdir" instead of type:"path" to leverage the existence check present in kong package.

gertd · 2025-10-08T09:06:26Z

pkg/app/extract.go

+	}()
+
+	// Create destination directory if it doesn't exist
+	if err := os.MkdirAll(destDir, 0755); err != nil {


The os.MkdirAll call is not needed when we enforce existance of --untardir

If we still need this, please use the existing filemode constant x.OwnerReadWriteExecute

gertd · 2025-10-08T09:12:13Z

pkg/app/extract.go

+			}
+
+			// Create and write file
+			outFile, err := os.OpenFile(targetPath, os.O_CREATE|os.O_RDWR|os.O_TRUNC, os.FileMode(header.Mode))


Should this be an os.Create() call?

gertd · 2025-10-08T09:13:29Z

pkg/app/extract.go

+		// Security check: prevent path traversal attacks
+		targetPath := filepath.Join(absDestDir, header.Name)
+		if !isPathSafe(targetPath, absDestDir) {
+			c.UI.Problem().Msgf("Skipping unsafe path: %s", header.Name)


I would abort in this case

pkg/app/extract.go

@gertd

Based on review feedback from @gertd: 1. Remove redundant --untar flag - Only keep --untardir flag - Bundle extraction is automatic when --untardir is set - Simplifies the interface and removes redundancy 2. Enhance security and validation - Change --untardir type from "path" to "existingdir" for built-in validation - Require destination directory to exist before extraction - Abort on unsafe paths instead of skipping with warnings - Validate absolute path existence immediately 3. Improve code quality - Use os.Create() instead of os.OpenFile() for file creation - Use x.OwnerReadWriteExecute constant for directory permissions - Remove unnecessary os.MkdirAll for validated existing directory These changes maintain backward compatibility while improving security and simplifying the user interface. Signed-off-by: thc1006 <[email protected]>

thc1006 · 2025-10-09T01:19:57Z

Hi @gertd,

Thank you for your thorough review and valuable feedback! I've implemented all your suggestions:

Changes Made:

1. Simplified Flag Interface

✅ Removed the redundant --untar flag
✅ Now only --untardir is needed - when set, extraction happens automatically
This makes the interface cleaner and more intuitive

2. Enhanced Security

✅ Changed --untardir type from "path" to "existingdir" for built-in validation
✅ Added immediate validation that the destination directory exists
✅ Changed from warnings to hard failures when unsafe paths are detected
✅ Both regular paths and symlinks now abort on security violations

3. Improved Code Quality

✅ Replaced os.OpenFile() with os.Create() for file creation
✅ Using x.OwnerReadWriteExecute constant for directory permissions
✅ Removed unnecessary os.MkdirAll since we now require existing directories

Result:

The implementation still fully addresses issue #107's requirement to extract policy content in a single step, but with a simpler and more secure approach. Users can now simply run:

policy pull <ref> --untardir ./existing-dir

Instead of the original three-step process (pull → save → tar).

All changes have been tested and the code compiles successfully. The DCO sign-off has also been added to the commit.

Thank you again for your insightful review - these changes significantly improve both the usability and security of the feature!

thc1006 · 2025-10-24T16:26:10Z

Dear @gertd is this apporach LGFU? Happy to adjust if you'd prefer a different structure!

gertd · 2025-10-30T10:41:00Z

pkg/app/extract.go

+		}
+
+		// Security check: prevent path traversal attacks
+		targetPath := filepath.Join(absDestDir, header.Name)


@thc1006 can you please look into the linter issues

pkg/app/extract.go

+
+		case tar.TypeSymlink:
+			// Handle symlinks carefully - ensure they don't point outside destDir
+			linkTarget := header.Linkname


Please address linter and security scan issues

Security fixes: - Add comprehensive path sanitization in ExtractPolicyBundle - Validate and reject absolute paths in tar archives - Validate and reject path traversal attempts (..) - Sanitize symlink targets to prevent symlink-based attacks - Implement isPathSafe helper for final safety checks This addresses the CodeQL warnings about "Arbitrary file write extracting an archive containing symbolic links" (CWE-22, CWE-59). Linter fixes: - Add nolint directives for legitimate security validation complexity - Fix missing periods in comments (godot) - Shorten help text to meet line length requirements (lll) - Add proper whitespace between logical blocks (wsl) - Run gofmt on all Go files for consistent formatting Testing: - All unit tests passing (pkg/app, oci, parser) - go vet: clean - go build: successful - golangci-lint: clean (only pre-existing nolintlint in login.go) - goreleaser build: successful Fixes opcr-io#206 Signed-off-by: thc1006 <[email protected]>

thc1006 · 2025-10-30T13:13:55Z

Dear @gertd All linter and security issues have been addressed in commit e183925:

Security fixes:

Added path sanitization for header.Name and header.Linkname
Reject absolute paths and path traversal attempts
Fixes CodeQL warnings (CWE-22, CWE-59)

Linter fixes:

Added nolint directives with justifications
Fixed comment formatting (godot)
Shortened help text (lll)
Added whitespace (wsl)
Ran gofmt on all files

thc1006 force-pushed the feature/pull-untar-107 branch from 1a5d954 to f61ac35 Compare October 8, 2025 03:18

thc1006 mentioned this pull request Oct 8, 2025

[feature] Add possibility to untar policy content to the disk when pulling OCI from registry. #107

Open

gertd self-requested a review October 8, 2025 06:05

gertd reviewed Oct 8, 2025

View reviewed changes

pkg/app/extract.go Show resolved Hide resolved

github-advanced-security bot found potential problems Oct 30, 2025

View reviewed changes

gertd previously approved these changes Oct 30, 2025

View reviewed changes

thc1006 requested a review from gertd November 13, 2025 04:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add --untar and --untardir flags to policy pull command #206

Add --untar and --untardir flags to policy pull command #206

thc1006 commented Oct 8, 2025

Uh oh!

gertd Oct 8, 2025 •

edited

Loading

Uh oh!

gertd Oct 8, 2025

Uh oh!

gertd Oct 8, 2025

Uh oh!

gertd Oct 8, 2025

Uh oh!

gertd Oct 8, 2025

Uh oh!

Uh oh!

thc1006 commented Oct 9, 2025

Uh oh!

thc1006 commented Oct 24, 2025

Uh oh!

Check failure

gertd Oct 30, 2025

Uh oh!

Check failure

thc1006 commented Oct 30, 2025 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Add --untar and --untardir flags to policy pull command #206

Are you sure you want to change the base?

Add --untar and --untardir flags to policy pull command #206

Conversation

thc1006 commented Oct 8, 2025

Uh oh!

gertd Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gertd Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

gertd Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

gertd Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

gertd Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thc1006 commented Oct 9, 2025

Changes Made:

1. Simplified Flag Interface

2. Enhanced Security

3. Improved Code Quality

Result:

Uh oh!

thc1006 commented Oct 24, 2025

Uh oh!

Check failure

Uh oh!

gertd Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

Check failure

Uh oh!

thc1006 commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gertd Oct 8, 2025 •

edited

Loading

thc1006 commented Oct 30, 2025 •

edited

Loading