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

Use minimum system permissions necessary #261

Closed
cmoog opened this issue Sep 20, 2022 · 5 comments
Closed

Use minimum system permissions necessary #261

cmoog opened this issue Sep 20, 2022 · 5 comments

Comments

@cmoog
Copy link
Contributor

cmoog commented Sep 20, 2022

I noticed that the default dev script invokes itself with full permissions over the entire system. Ideally, Lume should function fine if invoked with the following minimum permissions.

$ deno run --allow-net=localhost:3000 --allow-read=./ --allow-write=./_site lume/task.ts
@oscarotero
Copy link
Member

It's not so easy, for several reasons:

  • This depends on the site configuration. For example, if the site is configured to use a different dest folder or if the server port is different (like 8080 instead of 3000), the command should change also the permissions configuration.
  • deno run lume/task.ts doesn't work, because the lume/task.ts file is not resolved with the import map, so you have to include the full file url: deno run https://deno.land/x/lume@1.11.4/task.ts. But this introduce another url to maintain and keep up to date on update Lume, in addition to the Lume url in the import_map.json file. This is why the lume task is deno eval "import 'lume/task.ts'" because deno eval takes the import_map.json file into account to resolve the imports, making this file a single source of truth. More info about deno run and deno eval here: deno tasks combined with import maps (feat request) denoland/deno#14760
  • deno eval has implicit all permissions (it's like --allow-all).
  • lume/task.ts file just run https://github.com/lumeland/lume/blob/master/ci.ts and this file run the real deno command to run Lume with all required parameters. More info: https://github.com/lumeland/lume/blob/master/ci.ts#L107-L115

I'd like to simplify this in the near future, when this proposal is implemented: denoland/deno#12763 so you can configure the permissions in the Deno config file.

@cmoog
Copy link
Contributor Author

cmoog commented Sep 20, 2022

This depends on the site configuration.

Point taken. Then the minimum permissions for most projects would be --allow-net=localhost --allow-read=./ --allow-write=./. If some plugin needs to make other network requests, the user should need to explicitly allow that.

deno eval has implicit all permissions (it's like --allow-all).

After reading this issue, I can't help but feel the current behavior is a big mistake. If my understanding is correct, the lack of support for import maps in deno run means the only ergonomic way to invoke "framework" CLIs is without any sandboxing. This erases a major benefit of Deno's security model.

And to preempt, I don't think this is bikeshedding. Given that Lume is plugin-based, it's plausible that there will be a long tail of unaudited, untrusted plugins. Why should plugins have exec permissions?

Potential (temporary) solution

I haven't thought about this too much... but this came across my mind as a potential solution. This way, only one line of code runs with global permissions, then the task.ts process itself is sandboxed.

{
  "importMap": "import_map.json",
  "tasks": {
    "lume": "deno run --allow-read=./ --allow-write=./ --allow-net=localhost $(deno eval \"console.log(JSON.parse(Deno.readTextFileSync('import_map.json')).imports['lume/'])\")task.ts",
    "build": "deno task lume",
    "serve": "deno task lume -s"
  }
}

Of course this would also require changes to ci.ts and task.ts to remove the unnecessary self-invocation.

@oscarotero
Copy link
Member

task.ts and ci.ts works as wrappers to execute the real CLI script (cli.ts). This wrapper fixes different issues. So, restricting permissions to these files has no effect to cli.ts(AFAIK, permissions are not propagated between childprocesses: a file with only --allow-run could run a script passing the --allow-all argument).

The use cases are so variable that this is the reason I've decided to run cli.ts with --allow-all, for example:

  • Sites that fetch data from an external API, needs allow-net or allow-net=api-url.
  • Sites that have different configurations depending on the environment variables (like ENV=dev) needs --allow-env.
  • Sites that need to run some scripts during the build needs --allow-run
  • Sites that use some binary libraries connected with ffi needs --allow-ffi.
  • Sites that use WebAssembly libraries needs --allow-read to read the WebAssembly file.

Restricting the permissions right now can break many sites. I think it's better to let the users configure the permisisions once Deno team implements permission configuration in the deno.json file. In Lume 2.0 I'm planning to remove these wrapper files and depend only on deno.json that will be mandatory (unlike now, that both deno.json and import_map.json files are optional).

If you're concerned about that, the correct way to apply permissions is by running the cli.ts file directly. You can create a task like this:

{
  "importMap": "import_map.json",
  "tasks": {
    "lume": "deno run --unstable --allow-read=./ --allow-write=./ --allow-net=localhost https://lume.land/x/lume@1.11.4/cli.ts",
    "build": "deno task lume",
    "serve": "deno task lume -s"
  }
}

This conversation makes me think that a Permissions section in the documentation website would be really useful for users that want to restrict these permissions.

@bryophyta
Copy link
Contributor

This conversation makes me think that a Permissions section in the documentation website would be really useful for users that want to restrict these permissions.

This sounds like a great idea! It will also be useful for raising awareness of permissions considerations among users who wouldn't otherwise be thinking about it.

I don't think I know enough about Deno's permissions API or Lume's requirements to write this myself, but I'd be very happy to help with writing this documentation if anyone feels like collaborating on it.

@oscarotero
Copy link
Member

Permissions are explained here: https://lume.land/docs/advanced/permissions/
Closing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants