#‘dagger run’ & session security

1 messages · Page 1 of 1 (latest)

junior totem
#

In the low level API it would just be a matter of running a service that enables dagger-in-dagger and also forwards a container unix sock <-> host localhost. They can choose to include localdirs and env vars (using the same mechanisms we have for exposing those today).

We could then have a higher level "porcelain" around it in the dagger CLI. Something like:
dagger sandbox localhost:8080 --local-dir ~/src --env MY_TOKEN

which would result in a service being started that has dagger-in-dagger enabled, forwards the dagger api to the host's localhost:8080 and has the provided local dirs and env from the host exposed

#

Honestly, if we want an even shorter route to this we can skip the low-level API parts and the dagger-in-dagger aspect. It would be relatively easy today to add a dagger listen localhost:8080 --local-dir ~/src --env MY_TOKEN. The only difference between the previous version of dagger listen being that now host resources are exposed on an opt-in basis.

junior totem
#

@thorny badger What do you think about doing that ^^ instead of relying on a random id in the URL?

Basically, we change dagger run such that host resources are exposed on an opt-in basis w/ --local-dir and --env. Honestly, I'd probably be okay with dagger listen if that's the case too.

Env is easy to selectively expose; local dirs actually are too thanks to our filesync session attachable @balmy oyster added.

#

I think it's doable in the next day if we want to go this route

thorny badger
#

@junior totem under the above scenario, what happens if you run dagger run without any local-dirs or env? The engine will be pretty much useless?

junior totem
#

But then when you want those host resources, you just add a flag

thorny badger
#

got it. In that case that SGTM. I'm all in for least privilege principle. Are those features already in main so I can implement them in the CLI?

#

if we decide to go this route, we need to update the docs with these considerations cc @polar palm

junior totem
# thorny badger got it. In that case that SGTM. I'm all in for least privilege principle. Are th...

Yeah the local dir part will just require updating this code to optionally accept a list/map of allowed directories: https://github.com/sipsma/dagger/blob/3642f7fb8f03df3b403652f8454adfebad57bf5a/engine/engine.go#L278-L278

That's pretty much just plumbing though from cli arg -> engine.Config field -> that AnyDirSource.

Env will be pretty much the same, but just plumb the allowed vars to here instead: https://github.com/sipsma/dagger/blob/3642f7fb8f03df3b403652f8454adfebad57bf5a/core/schema/host.go#L59-L59

#

We'll want the SDKs to have the same behavior as before (everything is allowed) but that shouldn't change much

thorny badger
#

I'm a bit lost in the dagger-in-dagger part, how would dagger run work in that mode?

#

since it's currently execing the cmd sent as an argument

junior totem
#

And just to keep it minimal, we can still drop dagger listen for now. We could just make dagger run work this way and then consider adding dagger listen back with the same mechanism.

I like this approach of selectively exposing the host more than the random URL one because I think there's a higher chance it is something we'd actually want to stick with in the long run

thorny badger
junior totem
thorny badger
junior totem
#

And actually, we totally can also do the random url too, it would work perfectly well with this. I forgot that it's really easy to implement the random url because of the fact that we're wrapping the go sdk

thorny badger
#

it, perfect. Yeah, the random URL is easy to implement. In that case I'm "mostly" sold with the caveat that we might want to make the CWD of the dagger run command allowed by default. Mostly for DX purposes since having to specifcy --local-dir every time even for CWD, might be annoying. WDYT?

junior totem
#

Of course we can always change it to be exposed by default in subsequent releases, the reverse direction is just much more painful

thorny badger
#

I see your point. I guess it's ok to avoid it initially.

So, @random ice TL;DR

  • Add a --local-dir ([]string) flag to dagger run which will serve as an allow list of local directories which can be used from the engine.
  • Add a --env ([]string) flag that sets the env variables that you want to pass to the dagger run wrapped command. By default only $DAGGER_SESSION_URL will be present.
#

@junior totem am I missing something?

junior totem
#

I'd also add that SDKs will continue to work the same; this "opt-in" exposure of host resources will be exclusive to when the api is being served over tcp.

thorny badger
random ice
#

guys let’s talk about this live… this is a big last minute change

thorny badger
#

👍

random ice
#

I don’t see how we can change dagger run to have no host access by default without at least thinking through the second order implications . For example tools using an official SDK with auto-install will not be sandboxed; but if wrapped with dagger run they will be? How do I figure out which directories to allow access to, trial and error? Pretty big change for end users

#

I like the idea of that feature (possibly with interactive prompt ios-style too?) but requires careful UX design

#

I think somewhere along the way we stepped over from “how to avoid security flaw in exposing the session” to “let’s improve the trust & permission system like we discussed in the past, regardless of how session is exposed”.

#

both are good things to do, but IMO we should either decouple them, or if that’s not possible then push back the launch and get it right

#

One protects from malicious local users, the other from malicious code

thorny badger
thorny badger
junior totem
#

Summary so I don't forget:

  1. Leave dagger run as is (no flags) but document the security considerations (treat url as secret, especially if you have a long-running process)
  2. Can re-add dagger listen in very near future (not needed by launch though) but with the localdir+env allowlist feature.
    • I started working on the localdir part of this, will send out as draft in a bit
thorny badger
random ice
#

@thorny badger quick DX question: what is the most practical for client developers:

  1. A single $DAGGER_SESSION_URL variable?
  2. Two variables $DAGGER_SESSION_PORT and $DAGGER_SESSION_PATH?

Strictly from the perspective of DX convenience: which one is the least work to pass to vanilla graphql clients in various languages?

thorny badger
random ice
#

Ok 👍

green mist
#

Hey, I’m sick today, been in bed until now. Catching up

#

@random ice @junior totem @thorny badger To me this feel rushed.

I think treating the URL as a secret is unconventional — no other API does this

I think in a non-Unix world we should have taken the time to design the security model — auth header, basic auth; anything

#

Also: the “DSI” conversation started with the goal of making connection simpler / standardized / documented (e.g. let’s not have different schemes, methods).

With this we’re going to have two different methods (unix and tcp)?

random ice
#

No unix method is the idea

#

tcp only

#

I agree it's unconventional but that doesn't make it the wrong option. We are deep in "unconventional" territory no matter what, between session helper, local API endpoint etc. So need to find a pragmatic solution that matches the constraints we have

#

The constraints basically boil down to:

  1. Reduce DX friction as much as possible. Especially important since we're starting with an unfamiliar model, so adding friction only makes it worse

  2. Don't create security flaws that we'll regret later.

Balancing those two things is what makes this design discussion tricky.

#

Things that add DX friction:

  • Requiring client developers to use a unix socket
  • Making it hard for client developers to use curl
#

Possible security flaws:

  • User accidentally exposes a privileged API on the internet
  • Local session can be used for local privilege escalation, via tcp port scanning
junior totem
#

Yeah if we are starting w/ dagger run over tcp then I agree we may as well just leave the SDKs mostly as is (except w/ an update to read the random url from the session bin), the docs we write about security implications of dagger run will just be applicable to use of an SDK too.

#

And if someone complains we can choose between A) switching everything to unix/npipe or B) supporting both protocols

#

Among other options

green mist
#

Path as a secret feels wrong. It can leak everywhere in logs, and is generally not considered to be secret

“Making it hard for client developers to use curl” —> Stripe has the same constraint yet they pass the token as a curl flag rather than URL

junior totem
#

From my perspective, it's a pretty small lift from secret URL->auth token. The big thing was just tcp vs unix for me.

Only tiny downside I can think of is we'll have two env vars from dagger run: DAGGER_SESSION_URL and DAGGER_SESSION_TOKEN or similar. But I'm personally fine with that in the tcp side of the decision tree.

random ice
#

I’m fine with making it an auth header instead if it’s not too much extra friction (I guess auth basic support is ubiquitous)

#

I think leaking the path in the url is a non-issue in the context of an ephemeral session that will last a few seconds. Different story with listen

#

But yeah if we can remove that weirdness without adding DX friction, fine with me

random ice
junior totem
junior totem
#

@thorny badger ^^

#

Any thoughts?

thorny badger
#

👋 missed this. I'll reply in a sec. Talking to Gerhard and Vikram

junior totem
thorny badger
# junior totem Any thoughts?

np! just jumping into this. I don't think we'll release today given the time and the fact that we're still going back and forth with this decisions

random ice
#

I commented there

thorny badger
#

now, regarding the basic auth, I personally don't think it hurts the UX since all graphql clients have a way to specify auth headers. From the docs and examples perspective, the change is not disruptive also since it's basically s/random path/auth header in the messaging

random ice
#

‘dagger run’ & session security

thorny badger
#

@random ice from the UX perspective this is quite common amongst amongst graphql clients. Pretty much all "getting started" docs highlight how to send additional headers specifically for auth purposes. So I don't see any red flags there

#

I'd merge Erik's PR and wrap listen and run as they are. listen being hidden

random ice
#

Can you reply in the PR for the record? 😁

thorny badger
#

sure

junior totem
random ice
#

And yes I agree with all this

#

Just want to make sure the many people involved can see the context later

thorny badger
#

@junior totem mind updating the dagger run help example as well to add the header please 🙏

random ice
#

does this conflict with the docs main PR?

junior totem
thorny badger
#

yes, docs need to be updated to highlight this header thing

junior totem
thorny badger
random ice
#

would the clients support embedding the token in the url?

thorny badger
random ice
#

ie http://<TOKEN>:@localhost:<port>

thorny badger
#

usually header is better since URL parsing depends what URL parsing logic the client is doing

#

it "generally" works.. but it could break in some cases.

random ice
#

ok then separate env var is fine

#

I feel obligated to bring this up since we’re still making changes @junior totem @thorny badger : at this point should we consider biting the bullet and just going to DAGGER_SESSION_PORT now?

#

to avoid a future change for devs, wdyt

#

would remove the confusion about whether it’s local or not; is this ehere I plug a remote engine etc

#

just observing that we actually resolved the DSI bikeshedding as a byproduct of this discussion (I think)

#

so if we wanted, we could just do it now and be done with it

thorny badger
#

I mean.. we know we want to support TCP, but it's not clear to me how that and UDS will remain playing together

junior totem
#

I was under the impression that DAGGER_SESSION_PORT resulted in slightly more work relative to DAGGER_SESSION_URL

random ice
#

I think if we have addressed @junior totem ‘s security concerns then we should kill the unix socket transport, simpler to just have one thing

random ice
junior totem
#

if that's what you mean by interpolation

random ice
#

yes 👍 I think that’s fine and is slightly better actually because it reduces opportunities for footgun and confusion

#

“oh this is always local”

#

“ah this is not where I plug a remote engine”

#

I’m still ok to make this change later but observing that we could do it now

junior totem
random ice
#

Right. Also when I say “we could do it now” I’m of course referring to the dagger run part, SDKs we should change soon but can be post-launch

junior totem
thorny badger
#

🤔 I'm not sure how DAGGER_SESSION_PORT bring us closer to DSI, but I'd prefer to have $DAGGER_SESSION_URL since it's easier to make less mistakes when setting up clients

junior totem
#

Yeah I think URL works for now and decent chance it will in the long run too

random ice
#

ok let’s keep it then. @thorny badger the tradeoff is that some users will want to point that URL to something remote and be confused that they can’t - or worse find a way to do it

#

just preparing you for those support conversations 😉

thorny badger
random ice
#

someone will run dagger listen on a public port and manually set the url to that, and complain if it doesn’t work (or will get hacked)

#

actually this reminds me of “docker is insecure” memes

thorny badger
random ice
#

it will exist soon though

#

I think this actually may be a “daggerbleed” scenario

junior totem
thorny badger
random ice
thorny badger
junior totem