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

Subresource integrity for modules #200

Closed
nknapp opened this issue Jun 7, 2018 · 14 comments · Fixed by #3231
Milestone

Comments

@nknapp
Copy link

@nknapp nknapp commented Jun 7, 2018

I don't know if this already came up, but if security is a major concern and referencing modules via http(s) should be possible, then maybe there should be a way to use subresource integrity to ensure that the downloaded module isn't tampered with.

It could be part of the hash, like

import { test } from "https://unpkg.com/deno_testing@0.0.5/testing.ts#sha384-oqVuAfXRKap7fdgcCY5uykM6+R9GqQ8K/uxy9rx7HNQlGYl1kPzQho1wx4JwY8wC"

That would be very explicit, although not compatible to semver ranges.

@ianp

This comment has been minimized.

Copy link

@ianp ianp commented Jun 8, 2018

I think that when a resource if first fetched (and cached) it should be checksummed and this should be written to a file (similar to yarn.lock or package-lock.json). Maybe having a way to specify required/expected checksums ahead of time as well. For example, in deno.lock

checksums:
  - required:
    - https://unpkg.com/deno_testing@0.0.5: ["sha384", "oqVuAfXRKa..."]
  - computed:
    - https://unpkg.com/foo@1.2.3: ["sha384", "32e66f3809..."]
@ry

This comment has been minimized.

Copy link
Collaborator

@ry ry commented Jun 9, 2018

Yes, this is definitely something to be dealt with eventually. I'll leave this issue open, but I don't plan to address it for a while. (There are bigger fish to fry.)
@ianp Yea I agree. It would also be nice to specify the checksum in import too tho.
Are there any sub-resource integrity proposals for import?
Another idea that's been thrown around is to support IPFS for import - that also addresses the integrity problem.

@Hedzer

This comment has been minimized.

Copy link

@Hedzer Hedzer commented Jun 19, 2018

I was originally worried about compromised domains and subresource integrity. To expand on @nknapp 's idea. What if instead of a hash of the file it was the hash of a public key, and the ts file would begin with a comment that contained the public key, and a hash of the file (sans comment) encrypted with the private key.
e.g.

In deno code

import { test } from "https://unpkg.com/deno_testing@0.0.5/testing.ts#sha384-X3QnUIQNoQbIr3Nk2dmIFkkIdUwCl2WrX5RAPKa1TLYmHod/p9t/1c1fEnNJWUbI"

In testing.ts, first few lines

/* SRI-V0.0.1
{
    "key": "the public key",
    "type": "RSA-1024",
    "hash": "the hash of the file, encrypted by the private key"
}
*/

obviously the build process would have to take care of adding the subresource metadata.

also, on a bit of a tangent, this could be combined with some server side magic for semver compatible packages.

import { test } from "https://unpkg.com/deno_testing/^0.0.5/testing.ts#sha384-X3QnUIQNoQbIr3Nk2dmIFkkIdUwCl2WrX5RAPKa1TLYmHod/p9t/1c1fEnNJWUbI"

The server would then see this /^0.0.5/ portion and deliver the appropriate, still signed semver.

@DanielRuf

This comment has been minimized.

Copy link

@DanielRuf DanielRuf commented Jul 23, 2018

Support IPFS and SRI hashes would be very useful and are two things that make it superior in regards of security and decentralization.

@DanielRuf

This comment has been minimized.

Copy link

@DanielRuf DanielRuf commented Jul 23, 2018

I guess https://github.com/ry/deno/issues/170 overlaps with this.

@Hedzer

This comment has been minimized.

Copy link

@Hedzer Hedzer commented Jul 23, 2018

It's outside of the scope of this project, but I guess I'm suggesting publisher integrity with subresource integrity. Basically we trust a publisher and probably don't want to constantly update hashes in our project. What I'm suggesting above is a way to have subresource integrity with a hash that changes, but that can be verified to have come from the same publisher. Updates can be handled without massive changes to a project.

@DanielRuf

This comment has been minimized.

Copy link

@DanielRuf DanielRuf commented Jul 23, 2018

It's outside of the scope of this project, but I guess I'm suggesting publisher integrity with subresource integrity. Basically we trust a publisher and probably don't want to constantly update hashes in our project. What I'm suggesting above is a way to have subresource integrity with a hash that changes, but that can be verified to have come from the same publisher. Updates can be handled without massive changes to a project.

Definitely.
Also jsDelivr, unpkg and others already provide the needed information afaik.

@crabmusket

This comment has been minimized.

Copy link
Contributor

@crabmusket crabmusket commented Oct 21, 2019

The way Go handles this with go.sum files seems relevant. FAQs in their wiki

@ry ry referenced this issue Oct 21, 2019
23 of 37 tasks complete
@ry

This comment has been minimized.

Copy link
Collaborator

@ry ry commented Oct 21, 2019

Added to 1.0 blockers.

@andyfleming

This comment has been minimized.

Copy link
Contributor

@andyfleming andyfleming commented Oct 21, 2019

Some thoughts about lock file concepts (from gitter)

discussion permalink (gitter.im)


Ryan Dahl @ry
yes we need lock files......

Jed Fox @j-f1
Lockfiles only work if you can get a permanent link to a given file. Deno’s method of pulling data from arbitrary URLs means that there is no way to get a permanent link, so the only ways to implement a lockfile would be to store hashes and prevent the user from running the code if the contents of the URL have changed (like SRI) or to make the lockfile store a copy of every dependency (maybe in a compressed form so it doesn’t waste space?). Unless there’s something I’m not thinking of.

Ryan Dahl @ry
@j-f1 storing a hash of every dependency isn't a problem - and, yes, if people link to a master branch they will not have stable code.
a permanent link is only permanent until it isn't :P
that is, just because "npm.org" or "crates.io" says something is permanent doesn't mean it actually is. Similarly for https://deno.land/std@v0.21.0/examples/cat.ts ...

Jed Fox @j-f1
That’s true, but those registries have a stronger commitment to immutability than we do, since it’s possible to modify a Git tag.

Ryan Dahl @ry
deno.land isn't special - unpkg.org or pika.dev can maybe provide those sorts of promises - so lockfiles would be useful.
The lockfile just says - "last time i ran this, i had this exact code, and next time i run it i want to error out unless it's this exact code"
seems very reasonable feature. the fact that git supports force pushing has nothing to do with that.

Bartek Iwańczuk @bartlomieju
seems relatively easy to implement with current infrastructure
deno --reload would overwrite lock file
I'm not sure how it should interact with dynamic imports tho

Nayeem Rahman @nayeemrmn
Where exactly would the lock file go? DENO_DIR?

Ryan Dahl @ry
@nayeemrmn hmm - yea I think that would be reasonable.
probably we would just ignore the lockfile unless --locked was present ?
actually the lockfile needs to be checked into the project - so it shouldn't be in the DENO_DIR.

Nayeem Rahman @nayeemrmn
Exactly

Bartek Iwańczuk @bartlomieju
deno --lock, deno --lock=.my.deno.lock ?

Ryan Dahl @ry
deno generate-lockfile https://deno.land/std/examples/gist.ts ?
kinda verbose
https://doc.rust-lang.org/cargo/commands/cargo-generate-lockfile.html <--- that's what cargo does tho
deno fetch --generate-lockfile https://deno.land/std/examples/gist.ts
idk something like that

Nayeem Rahman @nayeemrmn
Maybe DENO_DIR could instead locate itself where the lock file is... like how npm uses package.json to locate the project root and puts node_modules there? 😅😅
Then you could implicitly declare a project with touch deno-lock or something

Ryan Dahl @ry
I think just explicitly specifying where the lockfile is is fine
it's not something you do interactively usually - so it doesn't need to be ergonomic
you just have it in CI

Nayeem Rahman @nayeemrmn
It needs to be done for any commit with a new dependency., that's if you only care about CI.

Andy Fleming @andyfleming
What's the behavior on mismatch with the lock?
It seems like the lock file should automatically be used and a flag --ignore-lock and/or --update-lock should be offered.

Andy Fleming @andyfleming
Also, @ry, I disagree about the lockfile only being needed for CI. Even locally when developing, dependencies should be deterministic by default. I should be aware if I’m pulling a different version/content than expected. Dev teams commonly use npm ci for this reason now.
I could be won over on it being a warning by default. That's reasonable.
Then we could have another flag like --strict or --strict-lock that would cause the script to exit immediately on mismatch
(which would be used for CI/production)

Andy Hayden @hayd
strict with a descriptive error message seems a better default.

Nayeem Rahman @nayeemrmn

It seems like the lock file should automatically be used and a flag --ignore-lock and/or --update-lock should be offered.

The problem with this was that there isn't an obvious answer to where the lock file should go by default and whose dependencies populate it, that's what spurred the discussion. I agree that using lock files shouldn't require a flag every time... this should be thought through more.

Andy Fleming @andyfleming
The SRI approach (#200) would address that. Locks would effectively be inline.
Then it could fail with a descriptive error message (like you are saying @hayd) by default unless --ignore-sri is on
The only thing that would get a little uglier is generating/updating "locks". Do we modify the import lines for users with a command?

Nayeem Rahman @nayeemrmn
To be clear, I still like lock files being optional. My suggestion was to have deno scan your directory tree for a lock file and use it if and only if it exists... somehow build a solution around that.

Yeah I haven't looked much into it but SRI seems like it would work too. Lock files are probably more convenient.

Andy Fleming @andyfleming
A challenge of the SRI approach is if you wanted to lock someone else's files. You might be able to lock the contents of the first file that you are importing from a URL, but if it imports another URL without SRI, we don't really have a guarantee about that 2nd file's contents.

Nayeem Rahman @nayeemrmn
Oh... yeah that's definitely a deal breaker. SRI doesn't substitute a lock file.

Andy Hayden @hayd
yeah, I guess my point was IF you have a lock file it THEN you should error if there's a mismatch.

other questions seem very much open...

@nayeemrmn

This comment has been minimized.

Copy link
Contributor

@nayeemrmn nayeemrmn commented Oct 21, 2019

My current suggestion is to walk up the directory tree looking for a deno.lock and use it as a lock file if one is found.

By 'use' I mean it should be checked for all fetches made from within its scope by default, but not written to unless explicitly specified with --lock or something.

Writing to it by default would mean it gets populated when you're experimenting or using some remote utility that's not a dependency of the project. I would want the lock file to only be updated when I run deno fetch --lock src/main.ts or something - but I still want it to be checked for everything by default.

The result of this is that lock files are opt in, but as a project author you can still force the checking part onto collaborators/CI.

@ry

This comment has been minimized.

Copy link
Collaborator

@ry ry commented Oct 21, 2019

My current suggestion is to walk up the directory tree looking for a deno.lock and use it as a lock file if one is found.

I do not want to get into the business of trying to guess the location of things. This seems innocent but can be very complex for other tooling to interact with. I don't mind if we check the current directory for a fixed filename - but I think it would be better if we always explicitly specified the lockfile filename.

@kitsonk

This comment has been minimized.

Copy link
Contributor

@kitsonk kitsonk commented Oct 21, 2019

I proposed in #3179 that we have a comprehensive meta data file, which would be opt in, work in a very similar way to a main module, and be the explicit location for the locks for a given workload.

I agree, guessing and walking isn't good. We should have the ability to utilise a remote set of lock information just as we do with other code (and if we do it for lock information, why no co-locate all meta data into a single file?).

@nayeemrmn

This comment has been minimized.

Copy link
Contributor

@nayeemrmn nayeemrmn commented Oct 21, 2019

I do not want to get into the business of trying to guess the location of things. This seems innocent but can be very complex for other tooling to interact with. I don't mind if we check the current directory for a fixed filename - but I think it would be better if we always explicitly specified the lockfile filename.

Makes sense. The intention behind that was to somehow make the checking part of it implicit. I think that's important. I hope there'll be a way to achieve this either way.

@ry ry referenced this issue Oct 29, 2019
@ry ry closed this in #3231 Nov 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
9 participants
You can’t perform that action at this time.