Deno: discussion: Permissions for Deno Worker

[bartlomieju]

With #4784 landed, we can now create new workers with Deno namespace available. Permissions for newly created worker are inherited from the process. It's very desirable to be able to configure permissions more granularly (eg. to run a sandboxed worker with no permissions).

Proposed change to WorkerOptions

export interface WorkerOptions {
  type?: "classic" | "module";
  name?: string;
  deno?: {
    permissions: {
      read?: boolean;
      readWhitelist?: string[];
      write?: boolean;
      writeWhitelist?: string[];
      net?: boolean;
      netWhitelist?: string[];
      env?: boolean;
      hrtime?: boolean;
      run?: boolean;
    }
  };
}

However I'm not sure how this API should behave if one of the fields in permissions interface is omitted - should it inherit from parent or maybe it should be set to false/empty list?

Example:

$ deno -A mod.ts

// mod.ts
new Worker("./worker.ts", {
  type: "module",
  deno: {
    permissions: {
      read: true,
      write: true,
    }
  }
});

My feeling is that all omitted permissions should be set to false - otherwise it'd be surprising that worker has access to net/run/hrtime because it's inherited from parent.

Of course, trying to specify permissions that escalate permissions of process will result in error:

$ deno --allow-read mod.ts
Uncaught error: PermissionDenied, trying to create worker with permissions escalating process' permissions.

// mod.ts
new Worker("./worker.ts", {
  type: "module",
  deno: {
    permissions: {
      read: true,
      write: true,
    }
  }
});

[lucacasonato]

What about also adding a "inherit" option? So something like this:

export interface WorkerOptions {
  type?: "classic" | "module";
  name?: string;
  deno?: {
    permissions: {
      read?: boolean | "inherit";
      readWhitelist?: string[] | "inherit";
      write?: boolean | "inherit";
      writeWhitelist?: string[] | "inherit";
      net?: boolean | "inherit";
      netWhitelist?: string[] | "inherit";
      env?: boolean | "inherit";
      hrtime?: boolean | "inherit";
      run?: boolean | "inherit";
    } | "inherit";
  };
}

And then default to permissions = "inherit". That way it is clear that specifying nothing means inherited everything, specifying an empty object means no permissions, and only specifying some means _allow this but nothing else_. This would also work a lot better if the base permission and the whitelist would be combined:

export interface WorkerOptions {
  type?: "classic" | "module";
  name?: string;
  deno?: {
    permissions: {
      read?: boolean | string[] | "inherit";
      write?:  boolean | string[] | "inherit";
      net?: boolean | string[] | "inherit";
      env?: boolean | "inherit";
      hrtime?: boolean | "inherit";
      run?: boolean | "inherit";
    } | "inherit";
  };
}

[bartlomieju]

Actually one more case; WorkerOptions.deno.permissions should be optional as well - ie. one can do:

new Worker("./worker.ts", {
  type: "module",
  deno: {}
});

Again, should permissions be inherited? Maybe something similar to process.stdout should be used, where it can be either inherit/piped/null or a number.

EDIT: @lucacasonato was faster

[ry]

Tangental comment: we really need documentation of how workers work. Is the deno option explained anywhere in the manual?

[bartlomieju]

Tangental comment: we really need documentation of how workers work. Is the deno option explained anywhere in the manual?

Opened #4868

[nayeemrmn]

Slightly tangential, I think when adding Deno-specific extensions to web APIs, we should make it a point to use symbols so that the Deno namespace has to be referenced to use it:

export interface WorkerOptions {
  type?: "classic" | "module";
  name?: string;
  [Deno.symbols.extension]?: { // Deno.symbols.extension = Symbol("Deno.symbols.extension")
    permissions: {
      read?: boolean | "inherit";
      readWhitelist?: string[] | "inherit";
      write?: boolean | "inherit";
      writeWhitelist?: string[] | "inherit";
      net?: boolean | "inherit";
      netWhitelist?: string[] | "inherit";
      env?: boolean | "inherit";
      hrtime?: boolean | "inherit";
      run?: boolean | "inherit";
    } | "inherit";
  };
}

[ColinHarrington]

should permissions be inherited?

Inheriting permissions from the parent is an acceptable default when no permissions are supplied (as @bartlomieju described in his example):

new Worker("./worker.ts", {
  type: "module",
  deno: {}
});

As soon as you start defining worker permissions, then I'd say that you have to fill out the permissions that the worker requires and deny any that are not specified.

new Worker("./worker.ts", {
  type: "module",
  deno: { 
    permissions: { 
      read: true,
      write: true,
      net: "inherit"
    } 
  }
});

[ColinHarrington]

I think that the permissions & whitelist relationship should be treated as a unit. Instead of breaking the read permissions into read and readWhitelist I'd rather see it defined as a complex type as @lucacasonato described:

permissions: {
  read?: boolean | string[] | "inherit";
  write?:  boolean | string[] | "inherit";
  net?: boolean | string[] | "inherit";
  env?: boolean | "inherit";
  hrtime?: boolean | "inherit";
  run?: boolean | "inherit";
}

That definition is more in line with the parsing on command line flags like --allow-read

[brandonkal]

I'd like to see an option to allow static imports inside a worker script without specifying allow-net or at least not giving that worker that permission.

[GideonMax]

Maybe to set the worker to have no permissions you could set

deno: {
    permissions: null
}

or

deno: {
    permissions: "none"
}

it's a little more intuitive then

deno: {
    permissions: {}
}

p.s.
there should be a way of saying "inherit all options apart from the options I'm specifying in permissions."

maybe do

deno: {
    permissions?:{.....},
    inheritPermissions?: boolean=false
}

[brandonkal]

Adding an inheritPermissions doesn't seem necessary and adds confusion. Those properties would be exclusionary. I'd rather query permissions and destructure it into the Worker's permissions object.

[GideonMax]

to me having to specify "inherit" for every permission except the one I want to change seems harder then just setting inheritPermissions.

[brandonkal]

I'd like to send in a PR for this. Based on the above, I would say the JS API should be:

export interface WorkerOptions {
  type?: "classic" | "module";
  name?: string;
  deno?: {
    permissions: {
      read?: boolean | string[] | "inherit";
      write?:  boolean | string[] | "inherit";
      net?: boolean | string[] | "inherit";
      env?: boolean | "inherit";
      hrtime?: boolean | "inherit";
      run?: boolean | "inherit";
    } | "inherit";
  };
}

To inherit permissions, you would have to declare it:

deno: { permissions: 'inherit' }

Having inherit be the default seems dangerous as you could easily unintentionally give a worker more permissions than desired. An escalation attempt would result in an error as well.

[nayeemrmn]

We still need the namespace inclusion flag to be a separate option:

export interface WorkerOptions {
  type?: "classic" | "module";
  name?: string;
  deno?: {
    includeNamespace?: boolean
    permissions?: {
      read?: boolean | string[] | "inherit";
      write?:  boolean | string[] | "inherit";
      net?: boolean | string[] | "inherit";
      env?: boolean | "inherit";
      hrtime?: boolean | "inherit";
      run?: boolean | "inherit";
    } | "inherit";
  };
}

(unless we remove the option in favour of always including the Deno namespace).

[bartlomieju]

@brandonkal proposed API looks good. I agree that inheriting permissions should be explicit. @nayeemrmn also agree that Deno namespace inclusion should be configurable.

[brandonkal]

Yes Deno namespace will remain configurable.

Minor note: it could just be deno.namespace: true as it is easier to read and write and just as clear.

[bartlomieju]

@brandonkal yes, that sounds better. Let me know if you need any help with the PR - keep in mind there's already a PR changing this interface (#6704)

[nayeemrmn]

To be clear, I was pointing out that namespace would need to be a separate option because it seemed like the intention was to infer it from deno: { ... } being specified, which would not have been good.

As discussed in IRC I'm actually against Deno namespace inclusion being configurable in favour of always including it. @brandonkal Feel free to preserve the functionality for now, but the issue should be settled before stabilisation.