-
Notifications
You must be signed in to change notification settings - Fork 32
Allow configuring sandbox option casd-socket #1945
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
base: master
Are you sure you want to change the base?
Conversation
This declares path in sandbox where casd socket will be mounted to
Exposing the unfiltered buildbox-casd socket would result in exposing access to the host filesystem via the LocalCAS protocol. An option that may be safe (to be reviewed) would be to change the instance name to a random token, generated in each BuildStream session, and then use Ideally, REAPI upstream would define a standard way how REAPI access can be exposed to a the action command. This shouldn't be buildbox-casd-specific and should also be supported with remote execution. Maybe configured by platform property. |
I'm not sure how this works: does it create a "sandboxed" instance somehow or is it just to have the LocalCas methods apply on the sandbox instead of the host? But this indeed needs to put some thoughts on what exactly we want to provide to the sandboxes: just CAS? CAS and action cache? do we need remote asset too? (we need to at least restrict what can be requested from remote asset). How about remote execution? maybe not by default, but it's probably what would give the most performance benefit. How about LocalCAS? do we need to restrict that? |
That's kind of the same thing, isn't it? Is there a particular difference between the two options that needs to be clarified? As mentioned, we should review the code to make sure it's sufficiently sandboxed, though.
Yes, we certainly need to give this some more thought. |
I meant is access to the CAS somehow sandboxed? (from your answer I guess it's not). Not that I think it should be, I just wanted clarification. |
@@ -133,6 +133,15 @@ def _execute_action(self, action, flags): | |||
else: | |||
stdin = subprocess.DEVNULL | |||
|
|||
if "casd-socket" in self.__config: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably a mistake: self.__config
doesn't seem to exist here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, is it the name mangling? self.__config belongs to grandparent of this class, Sandbox.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, anything that starts with two underscores is private to a class (and python enforces it)
Is it fair to say that #2014, which appears more complete, although in draft state, replaces this ? |
This declares path in sandbox where casd socket will be mounted to. Continuation of #1772