Skip to content
This repository has been archived by the owner. It is now read-only.

Add base64_digest for subresource integrity #645

Merged
merged 3 commits into from Oct 7, 2014
Merged

Conversation

@mastahyeti
Copy link
Contributor

mastahyeti commented Oct 4, 2014

This PR adds Asset#integrity to simplify the implementation of subresource integrity.

/cc @josh

mastahyeti added 2 commits Oct 4, 2014
@josh

This comment has been minimized.

Copy link
Contributor

josh commented Oct 6, 2014

Haha, a had a local branch to support something similar.

Though, I'm still not seeing integrity support Canary 40.0.2180.0. So I was waiting to actually confirm it before making any code changes (in github.com).

But yeah, I do want to see something like this in sprockets core.

What I had stubbed out was Asset#integrity which returned the full ni URI. That would include the content type too, which sprockets knows. "ni:///sha-256;-MO_YqmqPm_BYZwlDkir51GTc9Pt9BvmLrXcRRma8u8?ct=text/plain". Then you can just do <script integrity=<%= asset.integrity %>.

I do think we probably need to extend the manifest to include this so you don't need sprockets at deployment. I'm just trying to think through the migration issues of adding a new field. Its not something that has been done before.

@mastahyeti

This comment has been minimized.

Copy link
Contributor Author

mastahyeti commented Oct 6, 2014

Yeah, I debated adding the whole ni URL, but opted for just the SHA because it seems more general. For example, some folks might not want to specify ?ct= because it gets overwritten by CDN or something. For example our CDN seems to be serving JS with application/x-javascript, which I thought was a legacy value.

On the other hand it does make sense for sprockets to generate the whole URL since it knows about the digest algorithm. We'd definitely want to whitelist algorithms though since the spec specifically tells browsers not to implement weak algos like MD5.

As for the manifest, it seems like it should be safe since its already a hash, but I'm far from a sprockets expert. This is definitely important for deployment though...

@josh

This comment has been minimized.

Copy link
Contributor

josh commented Oct 6, 2014

For example our CDN seems to be serving JS with application/x-javascript, which I thought was a legacy value.

Yeah, we actually really need to fix that.

We'd definitely want to whitelist algorithms though since the spec specifically tells browsers not to implement weak algos like MD5.

Yeah, I think we should just force sha256.

@mastahyeti

This comment has been minimized.

Copy link
Contributor Author

mastahyeti commented Oct 7, 2014

I made a few changes. It is now providing the full integrity URI rather than just the digest. I also re-read the spec and properly implemented the digest. I also took your suggestion and forced the use of SHA256 for the integrity digest.

@josh

This comment has been minimized.

Copy link
Contributor

josh commented Oct 7, 2014

Rad.

I'll merge for now and deal with the manifest changes later. 3.0 is still "beta" after all.

Long term I'm excited to push for integrity by default for rails assets. I'm glad that sprockets makes it automatic.

josh added a commit that referenced this pull request Oct 7, 2014
Add base64_digest for subresource integrity
@josh josh merged commit d0c01a2 into sstephenson:master Oct 7, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
@mastahyeti

This comment has been minimized.

Copy link
Contributor Author

mastahyeti commented Oct 7, 2014

Cool! Is more than this required to get the integrity URL in the manifest?

@josh

This comment has been minimized.

Copy link
Contributor

josh commented Oct 7, 2014

We'll probably need an API to query it maybe. I'm still trying to think through a more extensionable metadata section for the manifest. As is they'll be no integrity on already compiled assets. And thats not something we can guarantee. I think APIs will just have to be designed to gracefully degrade in that case.

@mastahyeti

This comment has been minimized.

Copy link
Contributor Author

mastahyeti commented Oct 7, 2014

Before I ventured to mess with sprockets, I had been thinking to modify our code to parse the SHA1 our of the filename, parse it as hex, and then basey64 encode it. That's obviously nasty, but assuming that people are using a strong algorithm already it would be a passable workaround.

@josh

This comment has been minimized.

Copy link
Contributor

josh commented Oct 7, 2014

I don't think thats something that's safe to assume in sprockets itself. For anyone running 2.x, the file fingerprint 1) was md5 by default (that changed in 3.x) and 2) was a hash of more than the file contents itself. So theres a bit of a migration issue there.

For github itself, we've always used sha256 and we've pretty much did a clear jump to 3.x and didn't have to worry about backwards compat.

What I do like about this new manifest field is we know its always been generated correctly if it exists.

@@ -192,6 +192,7 @@ def build_asset_by_uri(uri)
encoding: Encoding::BINARY,
length: self.stat(asset[:filename]).size,
digest: digest_class.file(asset[:filename]).hexdigest,
integrity: Utils.integrity_uri(File.read(asset[:filename])),

This comment has been minimized.

Copy link
@josh

josh Oct 7, 2014

Contributor

Minor perf thing, I would like to avoid doing a full memory read of the file contents twice. We do it once for the regular digest but that also have some smart chunked reading stuff. Ideally we could generate one Digest::* class and just access the hexdigest and base64 parts.

Maybe for 3.x we could just kill digest_class configuration and force everyone into the SHA-256 future. Or allow non-sha256 right up until December 31, 2015 (kidding)

I'll look into this later.

This comment has been minimized.

Copy link
@mastahyeti

mastahyeti Oct 7, 2014

Author Contributor

Yeah. I felt bad about this too. I didn't feel like digging in far enough to make the change though...

@mastahyeti

This comment has been minimized.

Copy link
Contributor Author

mastahyeti commented Oct 7, 2014

It seems like if people bump major versions of sprockets they would't be surprised at needing to regenerate their manifest....

@josh

This comment has been minimized.

Copy link
Contributor

josh commented Oct 7, 2014

It seems like if people bump major versions of sprockets they would't be surprised at needing to regenerate their manifest...

The goal for 3.x is to be a transparent an upgrade as possible from 2.x. #643

@mastahyeti

This comment has been minimized.

Copy link
Contributor Author

mastahyeti commented Oct 7, 2014

Gotcha.

@josh

This comment has been minimized.

Copy link
Contributor

josh commented Oct 7, 2014

But I think what we have here now is pretty good. Its just understanding that fallback limitation, which I think is acceptable.

@josh

This comment has been minimized.

Copy link
Contributor

josh commented Oct 7, 2014

4.x can assert integrity always exist while 3.x can support a soft fail.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
2 participants
You can’t perform that action at this time.