oci: preserve dir mode when extracting tar layers

[frazew]

OCI push/pull/diff is currently not "round-trip" safe, because the tar extraction function hardcodes 0o750 for directories

This:

flux push artifact oci://$OCI_REGISTRY/test:0.0.1 --path=oci/testdata/artifact/ --source=test --revision=test

TEMP_DIR=$(mktemp -d)
flux pull artifact oci://$OCI_REGISTRY/test:0.0.1 --output=$TEMP_DIR
echo diff with local directory
flux diff artifact oci://$OCI_REGISTRY/test:0.0.1 --path=oci/testdata/artifact/
echo diff with pulled directory
flux diff artifact oci://$OCI_REGISTRY/test:0.0.1 --path=$TEMP_DIR

Outputs the following, which feels very weird:

► pushing artifact to <registry>/namespace-busy-buck/test:0.0.1
✔ artifact successfully pushed to <registry>/namespace-busy-buck/test@sha256:9ef46286aa2a7377a88ba47ec010d87828798df636fe97d91709589f8f612c52
► pulling artifact from <registry>/namespace-busy-buck/test:0.0.1
✔ source test
✔ revision test
✔ digest <registry>/namespace-busy-buck/test@sha256:9ef46286aa2a7377a88ba47ec010d87828798df636fe97d91709589f8f612c52
✔ artifact content extracted to /tmp/tmp.fvo5yauoUa
diff with local directory
✔ no changes detected
diff with pulled directory
✗ the remote artifact contents differs from the local one

This was noticed while debugging why flux diff always returned a diff, even for seemingly identical file trees. Maybe there's a specific reason for doing that, but I couldn't find one while looking around the git / Github PR history!

There's (in my opinion) another issue with file modes not being fixed when creating the tar layers, but that would need to be in another PR and might need discussion because it might be a breaking change? (i.e. I think the code here should also rewrite header.Mode to 0o750/0o640 so that umask differences between machines don't impact the final artifact)

[stefanprodan]

With this change, a maliciously crafted artifact could now create world-writable directories (0o777) that source-controller would process. Not sure if this would not result in a CVE. cc @hiddeco

[hiddeco]

Blindly accepting mode.Perm() would create an attack vector. Someone could create a malicious archive, and e.g. set the permission bytes to 0o000 or 0o555. This could then cause issues during cleanup or future writes, creating a potential DoS issue by filling up disk space.

However, I do think that you would get around the diff issue by setting a "floor", e.g.

Suggested change

	if err := os.MkdirAll(abs, mode.Perm()); err != nil {
	// Ensure the owner can always traverse, read, and write
	// into extracted directories, regardless of what the tar
	// header claims. This prevents crafted archives from
	// creating directories that block cleanup or future writes.
	dirPerm := mode.Perm() | 0o700
	if err := os.MkdirAll(abs, dirPerm); err != nil {

[frazew]

oh I hadn't thought of that, thanks! I've applied your suggestion