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

WIP Permission prompt #1018

Closed
wants to merge 3 commits into from
Closed

WIP Permission prompt #1018

wants to merge 3 commits into from

Conversation

hayd
Copy link
Contributor

@hayd hayd commented Oct 19, 2018

#1008

  • work out how to add tests
  • better messages for listen/accept
  • move Permissions struct off of DenoFlags to IsolateState (maybe?)

@ry
Copy link
Member

ry commented Oct 19, 2018

I've added atty to third_party. Please update your third_party to point to this branch: https://github.com/denoland/deno_third_party/tree/20181019_atty

@ry
Copy link
Member

ry commented Oct 19, 2018

Once we have granted access, you shouldn't prompt again for it. You can update the isolate.state.flags struct.

When I tried this, it asked twice for listen and accept

> ./out/debug/deno tests/http_bench.ts
Deno requests network access to "listen". Grant? yNy
Listening on 127.0.0.1:4500
Deno requests network access to "accept". Grant? yN

src/flags.rs Outdated
fn permission_prompt(message: String) -> DenoResult<()> {
if !atty::is(atty::Stream::Stdin) { return Err(permission_denied()) };

eprint!("{} Grant? yN", message);
Copy link
Member

@ry ry Oct 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is stderr the right thing to use here? I think you should use stdout.

Modify the line to have a space and question mark.

print!("{} Grant? y N ? ", message);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reasoning for stderr is if you're doing something like:

$ deno file.ts > out.log

Then this will still work. What's the argument against?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, makes sense. Use stderr.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a comment about why you're using stderr - since that wasn't obvious to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. That said, maybe we should also be checking if stderr is tty above too (if not asking permission is not going to print!)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I guess you need stderr and stdin to be tty for this to work.

src/flags.rs Outdated

eprint!("{} Grant? yN", message);
let mut buf = vec![0u8; 1];
// FIXME: This is line buffered, hence waits for the newline.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the FIXME. I think line buffered is the behavior we want here.

src/ops.rs Outdated
@@ -1111,7 +1117,8 @@ fn op_listen(
data: &'static mut [u8],
) -> Box<Op> {
assert_eq!(data.len(), 0);
if !state.flags.allow_net {
// FIXME
if state.flags.permissions_check_net("listen").is_err() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"listen" isn't a domain.

I think it would be good if permissions_check_net() took the Op's name and displayed it along with an optional domain.

You can get the op name with this: msg::enum_name_any(base.inner_type())

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True! (That was the "FIXME".)

I think there is a larger question of if you allow net access for "listen" does that really mean you want net access to www.facebook.com, the message would be a little misleading if that's the case. Will check this out.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For listen there is the address you're listening on. Which would make sense to display.

However for "accept" ... there's no argument that makes sense to display. So - I guess the address should be optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't addressed this yet.

src/flags.rs Outdated
pub fn permissions_check_write(&self, filename: &str) -> DenoResult<()> {
if self.allow_write { return Ok(()) };
// TODO get location (where access occurred)
permission_prompt(format!("Deno requests write access to \"{}\".", filename))
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think permissions_check_write and friends should not be methods of the Flags object, but rather standalone functions. Probably best to put them in src/errors.rs or even create a new src/permissions.rs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The benefit of them being here is the allow_write field and friends are private, so permissions must be checked via permissions_check_write.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should probably have a standalone permissions struct - rather than using flags. But that should be done separately

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that. I think moving to src/permissions.rs and adding Permissions struct is the same job - will try.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move to a new struct. Though in future this shouldn't be tacked on the DenoFlags, but be independent/new argument to Isolate/IsolateState. As a mutex.

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! This is going to be great.

Have you given any thought to how we going to test this? We might have to do something where we create a fake TTY and send data to it.... It might be worth looking at how we tested the REPL in Node...

@hayd
Copy link
Contributor Author

hayd commented Oct 19, 2018

A fake tty sounds worth investigating. (I had nothing.)

There is an (empty) issue on atty: softprops/atty#8

@hayd
Copy link
Contributor Author

hayd commented Oct 19, 2018

I don't understand the Appveyor build failure is (am I missing a lib in third_party for win)?

IMO Permissions should be mutable only once we have finer permissions, specific domains etc.

I'm still not sure on a test strategy... thinking about it.

@ry
Copy link
Member

ry commented Oct 19, 2018

@hayd try this

diff --git a/build_extra/rust/BUILD.gn b/build_extra/rust/BUILD.gn
index 93c370c..1beab94 100644
--- a/build_extra/rust/BUILD.gn
+++ b/build_extra/rust/BUILD.gn
@@ -114,13 +114,14 @@ rust_crate("winapi") {
     "cfg",
     "cfgmgr32",
     "combaseapi",
+    "consoleapi",
     "errhandlingapi",
     "excpt",
     "fileapi",
     "guiddef",
     "handleapi",
-    "inaddr",
     "in6addr",
+    "inaddr",
     "knownfolders",
     "ktmtypes",
     "libloaderapi",
@@ -134,6 +135,7 @@ rust_crate("winapi") {
     "objbase",
     "objidl",
     "objidlbase",
+    "processenv",
     "processthreadsapi",
     "profileapi",
     "propidl",

@hayd
Copy link
Contributor Author

hayd commented Oct 20, 2018

No joy. Am I supposed to be updating third_party too? It would be really useful to have some instructions/README for that - when I run tools/sync_third_party.py I get a completely different (much larger) third_party diff than your commit.

@ry
Copy link
Member

ry commented Oct 20, 2018

@hayd also add "wincon" to the winapi features .. no need to update third party

@hayd hayd force-pushed the permission-prompt branch 2 times, most recently from c5b5fcd to e16e317 Compare October 22, 2018 07:19
@ry
Copy link
Member

ry commented Oct 22, 2018

@hayd How's this one going? Do you need help with windows support?

@hayd
Copy link
Contributor Author

hayd commented Oct 22, 2018

@ry I think for windows it's good, it just needs you to push my third_party commit (after I rebased yours it X'd) to denoland/deno_third_party. Same with #998 actually.

I'm not sure how to test this actually (maybe it's best to somehow disable atty)? Or remove the atty test altogether until we have a testing strategy for it...

@hayd
Copy link
Contributor Author

hayd commented Oct 22, 2018

For tests I'll see if I can get something working with https://stackoverflow.com/a/1402389/1240268 (I'm not sure if that's unix only). I feel like there must be a python subprocess solution to do this... perhaps with pty? Not sure.

If we work out a solution we could potentially do the same for the repl (add an atty check).

@ry
Copy link
Member

ry commented Oct 23, 2018

@hayd thanks for looking into it. pty python module seems most promising - if that doesn't work we can perhaps hack together a native code test running in rust?

@hayd
Copy link
Contributor Author

hayd commented Oct 23, 2018

Sigh, I thought I had it... but I think I was doing the wrong test (part of the way there anyhow). https://gist.github.com/hayd/32accb02fd2515fb0814050a79e4aec9

@omeid
Copy link

omeid commented Oct 23, 2018

I may be late to the party but I think interactive prompts are the wrong way to go about this. I think a Selinux like Policy or better yet iOS App's like Capiblities approach is much more reasonable.

The essence of my argument is that permissions for any given library should be declared and known upfront, it would make a whole lot of controls much simpler.

@hayd
Copy link
Contributor Author

hayd commented Oct 23, 2018

@omeid I agree that an finely-grained app-based config/policy/permissions toml file is preferable.
IIUC this is merely an alternative for other use cases.

Once we have more finely-grained permissions, I think it will be clearer what those policy files could look like. Indeed it could be that unless a policy is passed at the cli, it could require prompting too: this app wants this list of permissions. Grant? Yn.

@hayd
Copy link
Contributor Author

hayd commented Oct 23, 2018

Okay, so I have a strategy for testing (atty) here. https://gist.github.com/hayd/4f46a68fc697ba8888a7b517a414583e https://stackoverflow.com/a/52954716/1240268

I'll add that machinery/tests in this evening.

@omeid
Copy link

omeid commented Oct 24, 2018

I think the policy should be part of a module and lack of any permission should result in an exception. I think making this interactive will have a lot of negative security implications, people will start automating yes to their processes because libraries start asking for permission at runtime.

@ry
Copy link
Member

ry commented Oct 24, 2018

@omeid Let's talk about it in v2 - this is sufficient for now

hayd added 3 commits October 23, 2018 21:21
It would be nice if ops.rs could use the shortcut syntax...
This is only available if stdin is tty.
Move permissions to its own module.
Update third_party - add atty.
@hayd hayd force-pushed the permission-prompt branch from e16e317 to a296f49 Compare October 24, 2018 06:08
@hayd
Copy link
Contributor Author

hayd commented Oct 24, 2018

I've rebased and added tests (it needs a push to third_party), but I haven't tested them on Windows.

It still asks every time, which I argue is okay until we have finer permissions. That is:

Deno requests network access to "listen". Grant? yNy
Listening on 127.0.0.1:4500
Deno requests network access to "accept". Grant? yN

It would be strange if accepting "listen" access implied "accept" access (or fetch access to localhost implied fetch access to facebook etc.).

A follow up to this PR would be Mutex for a more complicated Permission struct (on isolate) - i.e. not just 3 booleans... I think we ought to discuss what that struct should look like in a new issue.

@hayd
Copy link
Contributor Author

hayd commented Oct 24, 2018

I could change the message to not be so specific for now and be mutable.

Deno requests network access. Grant? yNy

And add the finer permissions later.

@ry
Copy link
Member

ry commented Oct 24, 2018

@hayd Looking good. I did some work on it here:

https://github.com/ry/deno/commits/atty

@ry
Copy link
Member

ry commented Oct 24, 2018

i've opened #1081 with my WIP

let's continue there.

@ry ry closed this Oct 24, 2018
@hayd hayd deleted the permission-prompt branch November 6, 2018 20:59
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

Successfully merging this pull request may close these issues.

3 participants