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

Secrets: Prevent Accidental Exposure #3709

Closed
1 task
StachuDotNet opened this issue Apr 16, 2022 · 4 comments
Closed
1 task

Secrets: Prevent Accidental Exposure #3709

StachuDotNet opened this issue Apr 16, 2022 · 4 comments

Comments

@StachuDotNet
Copy link
Member

StachuDotNet commented Apr 16, 2022

Currently, Dark "Secrets" aren't shown within the editor UI, but it's trivial to "leak" them accidentally (e.g. create the Secret, then create a route that returns the Secret). This is sort of status quo for a secret manager - it's just a dictionary. Dark has a somewhat unique opportunity to build in mechanisms preventing users from accidentally exposing/leaking their secrets.

Here's one idea. At each function and top-level, you're warned about Secrets used by itself and its dependencies. Some warning icon presents itself within or adjacent to the TopLevel/function. When you click that warning icon, you're presented with a choice of marking the function:

  • as "potentially unsafe" - warn in the dependencies
    e.g. a fn to map/convert a Secret to another Type
  • as "function totally safe - don't warn in the dependencies
    e.g. a function that makes an API call, and needs the secret as part of auth.
  • (maybe some other options related to scope. e.g. "OK in canvas, but not elsewhere")

You should be able to see all of the warnings centrally somewhere.
(centralized error reporting+logs is worth a separate larger chat!)

  • draw and include sketches

Tangential thoughts to extract:

  • should Dark allow Secrets of other (non-String) types? (I think so)
    I think Dark only support strings? I've wanted some non-Strings, and had to convert)
  • should Dark enable users to create/update/delete Secrets dynamically? (I'd like to)
    there' opportunity here to do this in a unique way.
    maybe this is some variant of a Datastore (a new Secrets type there? Maybe it's already supported - I haven't checked)
  • is there any danger of setting this precedent? (I'm probably overthinking this)
    what if the feature "breaks" somehow, and we don't warn them somewhere? might look bad on Dark
    (ideally tests prevent this, but bugs happen)
@pbiggar
Copy link
Member

pbiggar commented Apr 16, 2022

So a static analysis that tells you where the secret could reach? I like the idea, I think it would be useful.

There are limits to how far the analysis can go though. Pass it through String.split or save it in the DB and all bets are off.

An extension to this could be to have secrets as their own type. You can convert them to strings via Secret::toString if absolutely necessary, and we can refuse to return secrets. SDKs like Stripe::pay which take a secret would have a secret-typed argument, and we could validate that it is only used to create a header that's sent to stripe.com.

@StachuDotNet
Copy link
Member Author

StachuDotNet commented Apr 16, 2022

There are limits to how far the analysis can go though. Pass it through String.split or save it in the DB and all bets are off.

How so?
If you have a fn/TL that uses a String.split on a Secret, it will be flagged with a warning. Seems safe.
Same thing with a fn/TL that inserts the secret into a DB - a warning will be raised - if you choose to say it's OK, then all usages of the DB get warnings.

I'm sure the limit is somewhere, though.

An extension to this could be to have secrets as their own type. You can convert them to strings via Secret::toString if absolutely necessary and we can refuse to return secrets.

Yes! Maybe it would turn out like Secret<T>, where T is any normal type?

SDKs like Stripe::pay which take a secret would have a secret-typed argument, and we could validate that it is only used to create a header that's sent to stripe.com.

Totally agreed. I imagine packages requiring you to configure settings within that specific context.
Visually, I'm imagining something like:

  • use a fn in a package for the first time
  • UI pops up with a record {} of secrets you'll need to use the package, probably with something Option-like for each value
  • can revisit it later
    (might apply to things other than secrets?)

@pbiggar
Copy link
Member

pbiggar commented Apr 16, 2022

How so?
If you have a fn/TL that uses a String.split on a Secret, it will be flagged with a warning. Seems safe.

It's just a common problem with static analysis: you get false positives, and once there's enough false positives users stop looking at them.

Which isn't to say there isn't value here, not at all. Just that we have a type system too and if we can use it alongside the analysis to either reduce or eliminate false positives, we'll get better outcomes for users.

@StachuDotNet
Copy link
Member Author

merged this issue into #5260

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

No branches or pull requests

2 participants