Skip to content
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.

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

cue/interpreter/embed: embed should not follow symbolic links #3299

Open
myitcv opened this issue Jul 17, 2024 · 4 comments
Open

cue/interpreter/embed: embed should not follow symbolic links #3299

myitcv opened this issue Jul 17, 2024 · 4 comments

Comments

@myitcv
Copy link
Member

myitcv commented Jul 17, 2024

What version of CUE are you using (cue version)?

$ cue version
cue version v0.0.0-20240712164527-719893f23850

go version go1.22.3
      -buildmode exe
       -compiler gc
  DefaultGODEBUG httplaxcontentlength=1,httpmuxgo121=1,tls10server=1,tlsrsakex=1,tlsunsafeekm=1
     CGO_ENABLED 1
          GOARCH arm64
            GOOS linux
             vcs git
    vcs.revision 719893f23850172d224720e6d1257586179ac895
        vcs.time 2024-07-12T16:45:27Z
    vcs.modified false
cue.lang.version v0.10.0

Does this issue reproduce with the latest release?

Yes

What did you do?

# We need symbolic links
[!unix] skip

# Setup
env CUE_EXPERIMENT=embed
cd $WORK/x
exec ln -s ../top_secret

# Check we cannot reference a symbolic link
! exec cue export x.cue
stderr 'top_secret is a symbolic link'

-- x/cue.mod/module.cue --
module: "mod.example"
language: {
	version: "v0.10.0"
}
-- x/x.cue --
@extern(embed)

package x

x: _ @embed(file=top_secret, type=text)
-- top_secret --
This is a secret

What did you expect to see?

Passing test (for some variation of the error message)

What did you see instead?

# We need symbolic links (0.000s)
# Setup (0.001s)
# Check we cannot reference a symbolic link (0.013s)
> ! exec cue export x.cue
[stdout]
{
    "x": "This is a secret\n"
}
FAIL: /tmp/testscript613483604/repro.txtar/script.txtar:10: unexpected command success
@myitcv
Copy link
Member Author

myitcv commented Jul 23, 2024

Noting the "conclusion" I proposed in #3300 (comment), I still remain of the opinion that @embed should not read from symlinks, even in the main module. Such behaviour would be consistent with Go, which also does not follow symlinks when it comes to embedded files. This would, however, create an exception to the "rule" proposed in that conclusion.

@mvdan mvdan self-assigned this Jul 31, 2024
@rogpeppe
Copy link
Member

See the comment here: #3300 (comment)

Also, it's not currently trivial to add symlink checks in embed as it uses fs.FS which doesn't
make it possible to check for symbolic links in general (see golang/go#49580).

cueckoo pushed a commit that referenced this issue Aug 7, 2024
Valid symbolic links are currently allowed, but that will change
in the next commit. This test allows us to see the change in behavior.

While here, add more error test cases for embedding as well.
In particular, the "cannot embed directories" and invalid filetype
error code paths were not covered in the tests.

For #3299.

Signed-off-by: Daniel Martí <mvdan@mvdan.cc>
Change-Id: I5f80b1dc81e2a2bd13bd084be74cabec6f205ba4
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1199032
Unity-Result: CUE porcuepine <cue.porcuepine@gmail.com>
TryBot-Result: CUEcueckoo <cueckoo@cuelang.org>
Reviewed-by: Roger Peppe <rogpeppe@gmail.com>
@mvdan
Copy link
Member

mvdan commented Aug 7, 2024

We have gone back and forth on this in offline discussions, so I'll try to summarize the current state:

  1. Per all: to what extent should CUE support symlinks? #3300, it turns out that Bazel users also need symlink support for embedding. This is a minor but present problem with Go: cmd/go/internal/load: (Optionally) follow symlinks referenced by //go:embed golang/go#59924
  2. As embedding uses io/fs.FS, and io/fs.SymlinkFS is not merged yet (proposal: io/fs: add ReadLinkFS interface golang/go#49580), forbidding symlinks is possible only by going around the io/fs abstraction and doing os.Lstat. It works, but it's not ideal.
  3. Forbidding embedding via os.Lstat or io/fs.SymlinkFS.StatLink is non-trivial. It works when embedding a symlink file itself, but when the symlink is a directory along a path like glob="symlink-dir/*/*.json", this becomes harder. Neither fs.Open nor fs.Glob treat symlinks any different, so we would have to manually check every directory level leading up to a file too.

We have added test cases for embedding symlinks in https://review.gerrithub.io/c/cue-lang/cue/+/1199032 and https://review.gerrithub.io/c/cue-lang/cue/+/1199041, which capture the current behavior. Those are merged for rc.1.

For the reasons above, https://review.gerrithub.io/c/cue-lang/cue/+/1199041 will not be merged for rc.1. Particularly, it fails on point 3 - it does not forbid glob="symlink-dir/*/*.json". We will leave this issue open to discuss on Monday to decide what to do for the final release.

Assuming that #3300 converges on "we must support symlinks when loading packages and when embedding files for good Bazel compatibility" in the coming days, I'd be fine with shipping v0.10.0 with that implementation.

cueckoo pushed a commit that referenced this issue Aug 7, 2024
This is different than embedding a symlink itself, as we follow
a symlink as part of traversing a directory to open a regular file
or match a glob resulting in regular files.

For #3299.

Signed-off-by: Daniel Martí <mvdan@mvdan.cc>
Change-Id: I1c02a64a442b2a1c29192a29171b0ad6a903b43e
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1199041
Reviewed-by: Roger Peppe <rogpeppe@gmail.com>
TryBot-Result: CUEcueckoo <cueckoo@cuelang.org>
Unity-Result: CUE porcuepine <cue.porcuepine@gmail.com>
@mvdan
Copy link
Member

mvdan commented Aug 13, 2024

I am moving this to v0.11 per the above - we are releasing v0.10.0 this week and it seems like @rogpeppe is confident with leaving symlink support in place in @embed. @rogpeppe please follow up on #3300 and update this issue with your latest conclusion as soon as you're able to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: v0.11.0-rc.1
Development

No branches or pull requests

3 participants