1214079 - Doom all the cache of the signed packaged web app if it's not verified.

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Henry Chang [:hchang]]

When a signed package doesn't pass the signature verification or resource integrity check, we need to invalidate all the cached subresource of the package. 

This might be a duplicate of Bug 1190757. However, since we still don't have a good solution to it, I tend to solve this special case at first.

Comment 1

[Honza Bambas (:mayhemer)]

Henry, not sure why we need another bug when we have the cache transaction model (bug 1203113).  I thought that until we verify all the resources, the cache transaction is simply not committed.  When the verification fails, resources are simply rolled back (aka threw away) and never touches the existing http cache.

Comment 3

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Honza Bambas (:mayhemer) from comment #1)
> Henry, not sure why we need another bug when we have the cache transaction
> model (bug 1203113).  I thought that until we verify all the resources, the
> cache transaction is simply not committed.  When the verification fails,
> resources are simply rolled back (aka threw away) and never touches the
> existing http cache.

We only take this approach for pinned packages. For regular signed packages, we want each file to be available as fast as possible, i.e. possibly before the entire package is downloaded, as soon as the resources are available and we have verified that they have passed their integrity check. 

The problem we have at the moment if we get half way through the package, and an integrity check fails, we don't evict the things we have already cached. My understanding of this bug is that this is to invalidate all cache entries on encountering any type of failure (invalid manifest signature, invalid file hash or end of package reached without seeing all the files in the manifest). The first case is the most pressing but we need to deal with all of these.

Comment 4

[Honza Bambas (:mayhemer)]

What I do in bug 1203113 is providing an API that lets you delay caching a set of entries.  Until you call commit() or rollback() those entries are not available to load by a normal web browsing.  The old version (if previously cached) is still used.  That's all.

I expect bug 1190290 to use that cache API to build a functional mechanism for atomic transaction-lie updates of pinned packages.


I think a F2F meet-up will be better than this endless bugzilla ping pong.

Comment 5

[Henry Chang [:hchang]]

Attachment: 0001-Bug-1214079-Doom-the-package-cache-if-the-signature-.patch

Comment 6

[Henry Chang [:hchang]]

Hi Honza,

I just realized we don't need to doom all the cached files but only need to doom the package cache only since every request for the packaged content will use the "availability of the package cache" as the subresource's availability in the very beginning (even though there might be false positive)

I also add a new test case to verify the patch. The test case makes two identical requests in a row. However, the second request will get a "304 Not Modified" so that it will load from the cache. Without the patch, the test will fail because the cache files are not doomed.

Could you please have a review for the patch? Thanks!

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Honza: Bug 1203113 and bug 1190290 are still highly relevant. Nothing has changed there. However we need to also support consuming packages without them being pinned. I.e. we want to support simply navigating to a packaged URL and have that open in the browser. If the user like the page in the package they can choose to pin it, but that's not a requirement.

I'd definitely support having a F2F meeting about packages, signed packages and pinning though.

Comment 8

[Honza Bambas (:mayhemer)]

(In reply to Jonas Sicking (:sicking) from comment #7)
> Honza: Bug 1203113 and bug 1190290 are still highly relevant. Nothing has
> changed there. However we need to also support consuming packages without
> them being pinned. I.e. we want to support simply navigating to a packaged
> URL and have that open in the browser. If the user like the page in the
> package they can choose to pin it, but that's not a requirement.
> 
> I'd definitely support having a F2F meeting about packages, signed packages
> and pinning though.

OK.  I was at the Tuesday NSec meeting and discussed with Paul, Henry and others on this topic.  We also established few open question directed to you regarding exactly pinning.  If you want to meet F2F, please catch me on IRC during afternoon/evening CET times (GMT+1) or suggest something at that hours via email.  I'll be happy to meet you!

Comment 9

[Honza Bambas (:mayhemer)]

Comment on attachment 8679406 [details] [diff] [review]
0001-Bug-1214079-Doom-the-package-cache-if-the-signature-.patch

assigning the review to me.

Comment 10

[Honza Bambas (:mayhemer)]

Comment on attachment 8679406 [details] [diff] [review]
0001-Bug-1214079-Doom-the-package-cache-if-the-signature-.patch

Review of attachment 8679406 [details] [diff] [review]:
-----------------------------------------------------------------

I think this should go through Valentin.  The condition for existence of this entry to allow load of resources from the cache is in his code.

::: netwerk/test/unit/test_bug1214079.js
@@ +1,3 @@
> +// This test is to verify Bug 1214079: when we failed to load a signed packaged
> +// content due to the bad signature, we are able to load in the next time since
> +// the cache is not cleaned up after signature verification.

please rephrase this comment.  it's not clear what you mean.

Comment 11

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8679406 [details] [diff] [review]
0001-Bug-1214079-Doom-the-package-cache-if-the-signature-.patch

Review of attachment 8679406 [details] [diff] [review]:
-----------------------------------------------------------------

This is ok for now, until the transactional storage feature lands.
We'll at least have the unit test to make sure we don't regress.
Also, could you rename the test to something like test_packaged_app_bug1214079.js ? It's useful to be able to run the packaged app unit tests with just a one-liner.

Thanks!

Comment 12

[Henry Chang [:hchang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d80d80a42b5b

Comment 13

[Henry Chang [:hchang]]

Attachment: patch (r+'ed)

Comment 14

[Henry Chang [:hchang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=650f2724a864

Comment 15

[Henry Chang [:hchang]]

Attachment: patch for checkin (r+'ed)

Comment 16

[Henry Chang [:hchang]]

Thanks Valentin! I changed the test file name :) Thanks.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a93a69859594

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a93a69859594