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

Run info in notebook metadata #106747

Closed
jrieken opened this issue Sep 15, 2020 · 14 comments
Closed

Run info in notebook metadata #106747

jrieken opened this issue Sep 15, 2020 · 14 comments
Assignees
Labels
api insiders-released Patch has been released in VS Code Insiders notebook under-discussion Issue is under discussion for relevance, priority, approach
Milestone

Comments

@jrieken
Copy link
Member

jrieken commented Sep 15, 2020

We have NotebookCellMetadata#runnable and NotebookDocumentMetadata#runnable and it's unclear what this means given that we have support to execute cells/notebooks with different kernels. Should a kernel update all cells when becoming active? Would it be better if kernels somehow express what cells they can execute?

@jrieken jrieken added api notebook under-discussion Issue is under discussion for relevance, priority, approach labels Sep 15, 2020
@rebornix rebornix added this to the October 2020 milestone Oct 5, 2020
@rebornix rebornix modified the milestones: October 2020, On Deck Oct 26, 2020
@jrieken jrieken modified the milestones: On Deck, February 2021 Feb 12, 2021
@jrieken
Copy link
Member Author

jrieken commented Feb 12, 2021

Let's add this to the Feb discussions backlog. IMO the "runnability" is determined by:

  • having a kernel or not
  • the kernel supporting the language of the code cell
  • the cell being a code cell

So, in my head there is no metadata that can say "runnable". I also believe that this cut helps in decoupling the notebook content provider APIs from the kernel/execution API

@rebornix rebornix removed their assignment Feb 22, 2021
@roblourens roblourens reopened this Feb 23, 2021
@roblourens roblourens modified the milestones: February 2021, March 2021 Feb 23, 2021
@roblourens
Copy link
Member

I pushed the change to remove the runnable metadata and only infer it, but then reverted it, because we need to think about how this works with untrusted notebooks.

What we really need is to get Jupyter onto real vscode trusted workspaces, I pointed @DonJayamanne towards Steven for that, but in the meantime we need to make sure that they still have a way to accomplish this. It looks like Jupyter still attaches a kernel when the notebook is untrusted.

@DonJayamanne would it be ok to not provide a kernel when the notebook/workspace is untrusted, to cause cells to not be runnable? Or do you need kernels to still be available?

@jrieken
Copy link
Member Author

jrieken commented Feb 23, 2021

Can't we just control this? We know whether a workspace or a notebook is trusted and in that case we simply don't ask for kernels (or prevent execution)

@DonJayamanne
Copy link
Contributor

would it be ok to not provide a kernel when the notebook/workspace is untrusted

Yes that's ok, we don't need a kernel when a notebook is not trusted.

@roblourens
Copy link
Member

roblourens commented Feb 23, 2021

Yeah @jrieken, I think we can discuss whether this would be enforced by vscode or by an extension, since many extensions probably don't need to care about workspace trust. And I guess we already added some notebook metadata related to trust? We can discuss how trust should work, but it sounds like I can re-merge the change next week.

I'll ping again when I do it @DonJayamanne, but runnable metadata will go away, and you will want to not return a kernel when a notebook is not trusted.

@DonJayamanne
Copy link
Contributor

DonJayamanne commented Feb 23, 2021

I'll ping again when I do it @DonJayamanne, but runnable metadata will go away, and you will want to not return a kernel when a notebook is not trusted.

Hmm, interesting. Who should own the concept of trust here.
If the content provider says that a notebook is not trusted, then shouldn't it be able to control whether any kernel can run any of its code.

It seems like with the the proposed model we're passing responsibility of trust to a kernel.
I.e. if a different kernel exists that can run a notebook then VSC will allow the ability for the user to run that notebook, even when the contents are not trusted.

How do we protect the user here? Personally i think the content provider should protect the user here.

An excellent example is .NET Interactive extension today, they provide kernels as well.
But Jupyter owns ipynb files, hence if an ipynb is not trusted and jupyter will not return a kernel (based on suggestion). However .NET extension could still return a kernel & they could end running code in an untrusted notebook.
Sure they too can then check if the notebook is trusted or not, etc.., but feels it should be done higher up, so that kernels don't have to check if they are allowed to do something or now.

I.e. execution should be blocked, as opposed to making it an opt out.

@DonJayamanne
Copy link
Contributor

DonJayamanne commented Feb 23, 2021

Can't we just control this? We know whether a workspace or a notebook is trusted and in that case we simply don't ask fo

Will workspace trust make trusting notebooks unnecessary? Or do we still need both.
If both, then extension owns the notion of trust, hence we need to make changes at our end, but what about other kernel running code against untrusted notebooks.

@jrieken
Copy link
Member Author

jrieken commented Feb 24, 2021

If the content provider says that a notebook is not trusted, then shouldn't it be able to control whether any kernel can run any of its code.

That's a great point and I totally agree that you need to trust the kernel. About the document I am unsure. For me there is two kinds of trust

  1. trust the kernel executing arbitrary code
  2. trust existing output that "renders" by running code, e.g some html/js output, no kernel attached

So, I am actually unsure what we are trying to achieve? Trust the kernel, trust the output, have two separated trusts for each kind? @DonJayamanne maybe you can help to clarify what we are aiming for or what other notebook UI do

@roblourens
Copy link
Member

I think the only thing in scope here is trusting the content of a workspace. Jupyter's model is only about trusting the notebook document's content, right? That's compatible with what vscode is implementing in workspace trust.

Trusting a kernel/extension is up to the user.

@DonJayamanne
Copy link
Contributor

DonJayamanne commented Feb 24, 2021

Jupyter's model is only about trusting the notebook document's content, right

I'd say yes, but we also prevent users form running a kernel against an untrusted notebook, basically the notebook is readly - so they cannot execute the code if they dont trust it.
Sounds like VS Code will still allow users to run code in a document that is not trusted.

aybe you can help to clarify what we are aiming for or what other notebook UI do

Time to bring in the big guns..Hopefully our PM and manager can provide the requirements clearly (or change how we work)

Pinging @claudiaregio @greazer

@roblourens
Copy link
Member

roblourens commented Feb 24, 2021

Sounds like VS Code will still allow users to run code in a document that is not trusted.

I don't think we are saying that necessarily. I think we're trying to decide who enforces the rule or what the rule is.

And a key difference here is that vscode's model trusts entire workspaces, not individual documents like Jupyter. There's no problem with that, right?

@DonJayamanne
Copy link
Contributor

And a key difference here is that vscode's model trusts entire workspaces, not individual documents like Jupyter. There's no problem with that, right?

@claudiaregio @greazer /cc

@greazer
Copy link
Member

greazer commented Feb 25, 2021

Big guns... haha...

I'm not clear as to the question that's being asked to state any opinion about. What I do know is that cell renderers (i.e. cell outputs) that can execute code should not be rendered on opening a file, folder or workspace until the user explicitly trusts every cell in an opened file. I don't think it matters what kernel is going to be used to run the notebook. If the file is part of a trusted workspace, then the file should automatically be considered trusted.

It's also true that other notebook extensions may not have a need to support the notion of trust as their outputs can never be some script that can run arbitrary code.

Finally, the way Jupyter Classic and Jupyter Lab models work is to tie trust to each cell output. So that means that when a notebook is loaded no output is shown. At this point the user can explicitly state that the notebook is trusted so as to cause each output to be made visible. OR they can run a cell or set of cells. Running a cell implies that a cell is trusted. If the user runs all the cells, then the entire notebook is considered trusted automatically.

So based on all of that, have we considered adding some sort of "can-run-arbitrary-code" type capability setting for renderers? If we supported this model, then it seems like VS Code should own the notion of trust for files. Maybe we already have something like this that I'm not aware of (sorry if that's the case :)

Closer to what this issue was opened about, I think a question is whether we could or should duplicate the read-only nature of an untrusted notebook as we supported in our jupyter extension. I don't really think that it's necessary, but if any extension owner would like to enforce read-only because their source language can be used in a way that obfuscates what code is run, or their afraid there users can be easily tricked into running cells without realizing what they're doing, then having a way to disable running cells altogether until a notebook is trusted would be needed.

Hopefully this answered some questions?

@roblourens
Copy link
Member

I will do some more thinking about notebook trust on our side. But for now I intend to merge the original change today, FYI @DonJayamanne

#106747 (comment)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api insiders-released Patch has been released in VS Code Insiders notebook under-discussion Issue is under discussion for relevance, priority, approach
Projects
None yet
Development

No branches or pull requests

6 participants
@roblourens @rebornix @jrieken @DonJayamanne @greazer and others