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

Investigate and improve permission prompting #31

Open
dsherret opened this issue Dec 3, 2022 · 9 comments
Open

Investigate and improve permission prompting #31

dsherret opened this issue Dec 3, 2022 · 9 comments
Labels
enhancement New feature or request

Comments

@dsherret
Copy link
Owner

dsherret commented Dec 3, 2022

I think the permission prompting could be a little better and explanatory.

Edit: investigated all permission prompts...

  1. Once deno supports Wasm modules then we can get rid of needing to save and read the cache directory.
  2. Calling Deno.cwd() is necessary when the shell is created and cannot be lazily evaluated since it could change while the shell is executing, which would lead to very unexpected behaviour. This can be bypassed by providing an explicit cwd option.
  3. Getting env access to all when the shell is initialized is necessary in order to get the environment state when the shell is spawned (can't be lazily evaluated, which is the same issue as the last point). This could be mitigated by supporting clearEnv() in the future.
@dsherret dsherret added the enhancement New feature or request label Dec 3, 2022
@andrewbrey
Copy link
Contributor

andrewbrey commented Dec 15, 2022

Related to both this issue and #27, I have taken to using a shebang on executable files which lets each file declare its own permissions:

#!/usr/bin/env -S deno run --allow-env=HOME,PATH --allow-net=deno.land --allow-read=.

I think this could be another useful thing to include in docs (it's already in the Deno docs, which is where I got it from, e.g. https://deno.land/manual@v1.29.0/examples/hashbang).

Going this route lets you invoke child scripts that can have different permissions from the calling Deno process:

script-a.ts

#!/usr/bin/env -S deno run --allow-env=PWD --allow-read=.

import { $ } from 'https://deno.land/x/dax/mod.ts'

const scriptB =  $.path.join(Deno.env.get('PWD'), 'script-b.ts') // file with executable bit set

await $`${scriptB}`

script-b.ts

#!/usr/bin/env -S deno run --allow-env=HOME --allow-run=/bin/ls

import { $ } from 'https://deno.land/x/dax/mod.ts'

await $`ls -lah ${Deno.env.get('HOME')}`

Could be worth a mention 👍

@sigmaSd
Copy link
Contributor

sigmaSd commented Dec 19, 2022

You scripts seems to have wrong permissions ?

  • dax will always ask for -allow-env=all
  • even if you pass --allow-run=bin, dax will stat many paths to search for that bin, so you have to give --allow-read=1,2,3, all the paths that dax search

I wonder if can any of the above be improved ?

The other permissions are:

  • downloading and writing the wasm file if it doesn't exist so --allow-net=deno.land and --allow-write=.local/share../wasm
  • and after the first usage, it becomes just --allow-read=wasmfile

@andrewbrey
Copy link
Contributor

You scripts seems to have wrong permissions ?

I was just quickly typing this as an illustrative example, not as a copy-pasteable script. You are right, more perms are typically needed for any script being run by dax but I figured pointing out the possibility of defining the perms in this way could be helpful as a documentation item which may help a subset of needs. 👍

@NfNitLoop
Copy link
Contributor

NfNitLoop commented Aug 1, 2023

Once deno supports Wasm modules then we can get rid of needing to save and read the cache directory.

I think you can do this before Deno supports native WASM modules. I haven't used WASM much myself, but as I understand it you just feed bytes into WebAddembly.Module() (example)

So if you transform your .wasm file to something like base64_encoded_wasm.ts (or .js or .json), you can just

import wasmBytes from "./base64_encoded.wasm.ts"

This lets deno install or deno compile cache the file for you. And lets deno run fetch (and cache!) the file for you without needing --allow-net or --allow-write access.

Shameless plug: If you don't want to base64-encode the wasm file yourself, you could use deno-embedder, which I wrote to do just this kind of thing.

@dsherret
Copy link
Owner Author

dsherret commented Aug 1, 2023

There is an option in wasmbuild (which dax uses to build the wasm file) to do that: https://github.com/denoland/wasmbuild#cli-flags (--sync). I believe it is slower to load, but I should measure to see how much slower as maybe it's not a big deal.

@dsherret
Copy link
Owner Author

dsherret commented Aug 1, 2023

@NfNitLoop deno-embedder looks nice!

@NfNitLoop
Copy link
Contributor

NfNitLoop commented Aug 1, 2023

As for cwd/HOME -- I'm not sure why dax needs to read those explicitly. For example, I can just run a Deno command without the --allow-env:

#!/usr/bin/env -S deno run --allow-run

const result = await new Deno.Command("env").output()
const outText = new TextDecoder().decode(result.stdout)
console.log(outText)

(Update): Aha, I see. Some of the built-in commands like pwd will need to know CWD to be able to operate correctly w/o calling a command. (At least, with the current implementation.)


Context: I'd avoided using Dax for a while because my very first experience with it was it asking for permissions to environment variables and reading/writing local directories, and network access, which seemed strange. (yes, even in the face of giving it --allow-run. Or maybe in especially for that reason.)

Months later, I was quickly making a script and forgot about my issues. This time I just threw on an -A because I was in a hurry, and I very much like the simplicity of Dax's interface. ❤️ And I am a fan of replacing Bash with Deno

I'd love to use Dax while granting it fewer permissions!

@NfNitLoop
Copy link
Contributor

@dsherret I'm not sure if my PR link 2 days ago sent a notification to this issue, so just in case it didn't, see the above. 😊

I created a simple benchmark to test the "before" performance. Then I converted to use the --sync option, and found out that it's actually slightly faster. So if you were sticking to the other loading method for speed, there's no need AFAICT.

@dsherret
Copy link
Owner Author

dsherret commented Aug 5, 2023

Thanks, @NfNitLoop! That was a big improvement.

And I am a fan of replacing Bash with Deno

I agree. I'm actually probably doing a lightning talk on this subject next month at a conference.

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

No branches or pull requests

4 participants