Project

General

Profile

Bug #15995

Feature #15292: Distribute a USB image

Have Tails Verification handle IDFs with several Tails installation paths (ISO/IMG)

Added by u 12 months ago. Updated 9 months ago.

Status:
Resolved
Priority:
Normal
Assignee:
Category:
-
Target version:
Start date:
09/28/2018
Due date:
% Done:

100%

Estimated time:
12.00 h
Spent time:
Feature Branch:
verification-extension.git:usbimage
Type of work:
Code
Blueprint:
Starter:
Affected tool:
Verification Extension

Description

(or one IDF that references both the ISO and .img) including:
- make it easier for the next developer to get started working on this code base

Reviewer and rubber-duck: intrigeri
Release :sajolida:


Related issues

Related to Tails - Bug #16087: The download extension doesn't work on Chrome > 69 Resolved 10/31/2018
Related to Tails - Bug #16119: Drop jQuery in Tails verification extension Resolved 11/11/2018
Related to Tails - Feature #16128: Move the verification JavaScript from the verification extension to the page itself Confirmed 11/15/2018
Related to Tails - Feature #16143: Clean up mirror-dispatcher.js Confirmed 11/19/2018
Related to Tails - Bug #16200: Allow verifying different versions of Tails (older versions, testing versions, etc.) Confirmed
Blocks Tails - Bug #15997: Test and release new Tails Verification Resolved 09/28/2018

History

#1 Updated by u 12 months ago

  • Related to Bug #15996: Code review, rubber-duck & review design doc added

#2 Updated by u 12 months ago

  • Related to deleted (Bug #15996: Code review, rubber-duck & review design doc)

#3 Updated by u 12 months ago

  • Blocks Bug #15996: Code review, rubber-duck & review design doc added

#4 Updated by u 12 months ago

  • Assignee set to u
  • Target version changed from Sponsor_I_2018 to Tails_3.11

#5 Updated by u 12 months ago

  • Blocks Bug #15997: Test and release new Tails Verification added

#6 Updated by intrigeri 11 months ago

  • Description updated (diff)

#7 Updated by intrigeri 11 months ago

  • Blocks deleted (Bug #15996: Code review, rubber-duck & review design doc)

#8 Updated by u 11 months ago

  • Status changed from Confirmed to In Progress
  • Assignee changed from u to intrigeri
  • QA Check set to Info Needed

I've read the code, commented it a bit better, because otherwise it's very tiresome for the anybody who has to work on that code base imo.

I've now come to the conclustion that it would be easiest to reference two files in the same IDF. Because otherwise we would need to download two files and I would still need to check if the file to be verified is and ISO or not, so having all information in one file seems more efficient.

Currently the IDF looks like that:


---
build-target: amd64
channel: stable
product-name: Tails
version: '3.10.1'
target-files:
- sha256: 6c6849b2cbaba7f88779ae74c4481d0149cfcf847e2bc6082862440ec6873733
  size: 1225568256
  url: http://dl.amnesia.boum.org/tails/stable/tails-amd64-3.10.1/tails-amd64-3.10.1.iso

To me it would be useful to modify the file to look like this:


---
build-target: amd64
channel: stable
product-name: Tails
version: '3.10.1'
target-files:
- sha256: 6c6849b2cbaba7f88779ae74c4481d0149cfcf847e2bc6082862440ec6873733
  size: 1225568256
  url: http://dl.amnesia.boum.org/tails/stable/tails-amd64-3.10.1/tails-amd64-3.10.1.iso
- usbimgsha256: e74c4481d0149cfcf847e2bc6082862440ec68737336c6849b2cbaba7f88779a
  usbimgsize: 1225533356
  usbimgurl: http://dl.amnesia.boum.org/tails/stable/tails-amd64-3.10.1/tails-amd64-3.10.1.img

Would that work for you, dear rubberduck?
Please reassign to me with your answer :)

#9 Updated by u 11 months ago

Actually, the second "-" (dash) might not be needed, I let you decide how to handle this.

#10 Updated by u 11 months ago

  • % Done changed from 0 to 10
  • Feature Branch set to verification-extension.git:usbimage

I have a prototype for this now. This prototype downloads the IDF with more lines and add these lines to the internal conf variable. Then I check the filetype of the downloaded file and if it is a .img file, I simply modify the size/hash/url variables that are tested with those of the .img. This avoids having to if/else twice in the existing code.

I've tested it locally, it seems to work. I updated quite a bit of README and patches to run the extension locally.

Things that are not yet clear to me:

- what is the MIME type of our .img file? I assume it is "application/x-diskimage" while the .iso should be "application/x-cd-image".

Left to be done:

- actually test this with a yaml that contains the lines in question (for now my tests tested it inversely)

- actually test this with a real .img to be downloaded.

- write a patch for download.js's counter variable that does prevent testing locally (low prio, it is documented in the readme)

#11 Updated by u 11 months ago

- what is the MIME type of our .img file? I assume it is "application/x-diskimage" while the .iso should be "application/x-cd-image".

Seems to be "application/x-raw-disk-image" instead.

#12 Updated by u 11 months ago

  • Assignee changed from intrigeri to u
  • QA Check deleted (Info Needed)

I#ll take this back until that thing works as expected. I'm testing it with more data now.

#13 Updated by u 11 months ago

  • % Done changed from 10 to 20

I've now tested:

- verifying a .img (i converted this myself using `dd` and a Tails iso image) → this works
- changing the expected filesize in checkFileTypeAndRewriteDf() → works

Untested:

- using a real IDF downloaded from our website (we could make that happen I guess?)
- trying to verify an .img and if it fails trying to verify an .iso without reloading the download page.
- did not test the extension in Chrome, because it is currently broken there (#16087)

Accidentally, the function that checks the filetype now also prevents users from actually trying to verify whatever file. This would not work, but the current extension does not check what users verify. It simply would not work as they would not find the correct SHA and filesize in our IDF, but there was no other check then that.

I did not yet run the code through JSLint.

#14 Updated by u 11 months ago

  • Related to Bug #16087: The download extension doesn't work on Chrome > 69 added

#15 Updated by u 10 months ago

There has been an expert review, and a bunch of code has been simpli- and beautified.

I also dropped the complicated procedure for local testing by implementing a method that would check if the access to "file:///" is allowed by looking at the manifest.

The code could already be reviewed as is but I still want to look into making it run faster by computing the sha256sum asynchronously instead of waiting for it before computing the next chunk.

#16 Updated by u 10 months ago

  • Assignee changed from u to intrigeri
  • QA Check set to Ready for QA

@intrigeri:

I'd be interested in a first code review of branch `usbimage`. latest commit: a77f8e0c61745c9bc7c529715ad9b3b4226a283e

We (mostly Enrico) have created another branch `async_shasum`: this branch computes the shasum asynchronously in a worker thread. It works in FF, but not in Chromium (needs some more work to comply with security issues of using web workers in extensions). However, the speed increase is only ~15%. So I'm not sure we should merge this branch, because the code is less maintainable. But it's there for us to look at if we want. And it was fun to try and make it work :)

Another thing that could be done in the extension would be to drop jquery. Everything that is done with jquery can be done in vanillaJS. I did not do that yet, but ideally, we'd do it at some point on some kind of budget line that we have outside of this grant. What do you think?

#17 Updated by u 10 months ago

Oh and the extension works both in Chrome and FF; although in Chrome it does not seem to update tabs that are already open, see #16087. This can be fixed.

#18 Updated by u 10 months ago

  • Related to Bug #16119: Drop jQuery in Tails verification extension added

#19 Updated by intrigeri 10 months ago

  • Assignee changed from intrigeri to u
  • QA Check changed from Ready for QA to Info Needed

Regarding the code changes:

  • I did not test the new code yet: sajolida will have to do it before releasing anyway.
  • I see that upgrade/tails-download.mdwn includes install/inc/js/download so I'm not sure about commit 688fbc731b960cbf39fc4981123e9062ffc80d64
  • I'm super happy about all the cleanups and modernization! (I'm not qualified to review them but I'll happily transitively trust E.'s expertise :) Now, I won't be able to review commits like 636bce77d482e0de0520bc1c2e1f66df1af58ddf that change lots of things with no explanation. So I'll explicitly rely on E.'s review that prompted these changes. If you want me to review this kind of things in the future (not sure it would be sensible given my poor JS skills), I'll need more atomic and better documented commits.
  • I know that sajolida is supposed to update the design doc but given the large changes (e.g. dropping the background script and revamping some permission-related code), I think he'll need a summary from you, at least to update the "Security inside the browser" section: we can't expect he'll grap this by reading the code and Git history.
  • Regarding the file type check (and assuming it does not come from the web server the file was downloaded from): if the MIME type check is implemented by browsers using platform-specific mechanisms, I need to tell you that we've had a few bad experiences with shared-mime-info, such as regressions in detected MIME types. In the history of Tails, we've had to contribute various fixes upstream about such problems; it's unlikely that these fixes made it into already released LTS distros. Given this extension is supposed to work as-is on any platform, I'm slightly concerned: e.g. if a new shared-mime-info recognizes our .img as application/x-something-else on recent distros, we'll need to support both application/x-raw-disk-image (for compatibility with older distros) and application/x-something-else; and once I think about what various versions of Windows and macOS can do, I'm a bit sceptical. Checking the file extension would arguably be more ugly, but perhaps would not have such long-term maintenance issues? Your call!
  • Whenever we'll feel this code is ready to be released, please make it explicit which versions of Firefox you've tested: ideally this would be tested on the current ESR, current release channel, and stable Tor Browser.

Regarding the IDF format, the original format was a simplified version of our UDF format. Now, the project we're currently working makes our needs closer to the original UDF format. So we can either diverge further (as you did with totally different semantics for the target-files field), or converge back. Things would be nicer to people who work both on our upgrade system and on this extension (i.e. myself and nobody else at the moment) if we converged back. But converging back would make the file format a bit more complicated and might require less hackish YAML parsing code. If you want to hear more about this, I'm happy to give an example of how converging back could look like :)

Finally, if we change the IDF format in an incompatible way, given the file format itself is not versioned, we need to bump the API version in the URLs: install/v1 → install/v2. It seems that your proposed new format is compatible with the old one (old clients would simply ignore the usb.* fields) so I guess that it's a concern only if you pick my above suggestion, but please double check.

#20 Updated by u 10 months ago

intrigeri wrote:

  • I see that upgrade/tails-download.mdwn includes install/inc/js/download so I'm not sure about commit 688fbc731b960cbf39fc4981123e9062ffc80d64

Agreed, I'm also unsure about this one.

However, if we don't do this, all the scripts of the extension will be injected in all tabs that start with https://tails.boum.org. While this might be ignored, I find that it's not a clean way to do it.

  • I'm super happy about all the cleanups and modernization! (I'm not qualified to review them but I'll happily transitively trust E.'s expertise :) Now, I won't be able to review commits like 636bce77d482e0de0520bc1c2e1f66df1af58ddf that change lots of things with no explanation. So I'll explicitly rely on E.'s review that prompted these changes. If you want me to review this kind of things in the future (not sure it would be sensible given my poor JS skills), I'll need more atomic and better documented commits.

I've briefly documented what prompted these changes in the README file under "Coding" because I knew that this commit was certainly not atomic enough.

  • I know that sajolida is supposed to update the design doc but given the large changes (e.g. dropping the background script and revamping some permission-related code), I think he'll need a summary from you, at least to update the "Security inside the browser" section: we can't expect he'll grap this by reading the code and Git history.

Ack. I could update the design doc myself eventually, which might be much faster than summarizing this.

  • Regarding the file type check (and assuming it does not come from the web server the file was downloaded from): if the MIME type check is implemented by browsers using platform-specific mechanisms, I need to tell you that we've had a few bad experiences with shared-mime-info, such as regressions in detected MIME types. In the history of Tails, we've had to contribute various fixes upstream about such problems; it's unlikely that these fixes made it into already released LTS distros. Given this extension is supposed to work as-is on any platform, I'm slightly concerned: e.g. if a new shared-mime-info recognizes our .img as application/x-something-else on recent distros, we'll need to support both application/x-raw-disk-image (for compatibility with older distros) and application/x-something-else; and once I think about what various versions of Windows and macOS can do, I'm a bit sceptical. Checking the file extension would arguably be more ugly, but perhaps would not have such long-term maintenance issues? Your call!

This is something we need to test first. For that to happen: could you provide such an image to me?

  • Whenever we'll feel this code is ready to be released, please make it explicit which versions of Firefox you've tested: ideally this would be tested on the current ESR, current release channel, and stable Tor Browser.

I'm unsure what "current release channel" means. May you please explain this?

Regarding the IDF format, the original format was a simplified version of our UDF format. Now, the project we're currently working makes our needs closer to the original UDF format. So we can either diverge further (as you did with totally different semantics for the target-files field), or converge back. Things would be nicer to people who work both on our upgrade system and on this extension (i.e. myself and nobody else at the moment) if we converged back. But converging back would make the file format a bit more complicated and might require less hackish YAML parsing code. If you want to hear more about this, I'm happy to give an example of how converging back could look like :)

As there was no such suggestion of a format available to me, I prepared something that would work based on what the IDF currently looks like. But I really don't care much. If you want to propose a format, I can adapt the extension to read this format.

In that case I'd be glad if you could upload a test file using this format somewhere which I can use to test the code of the extension.

Finally, if we change the IDF format in an incompatible way, given the file format itself is not versioned, we need to bump the API version in the URLs: install/v1 → install/v2. It seems that your proposed new format is compatible with the old one (old clients would simply ignore the usb.* fields) so I guess that it's a concern only if you pick my above suggestion, but please double check.

As said, I don't care much. If you had an idea for such a format in mind already, please make it clear and prepare a file for me to test the extension code on. I considered what I did with the extension to be some kind of prototype, and if you want to change the format, we can adapt the code accordingly. The current solution was picked because it does not require big code changes, but that does not mean it's the best solution.

#21 Updated by u 10 months ago

  • Assignee changed from u to intrigeri

#22 Updated by u 10 months ago

u wrote:

intrigeri wrote:

  • I see that upgrade/tails-download.mdwn includes install/inc/js/download so I'm not sure about commit 688fbc731b960cbf39fc4981123e9062ffc80d64

Agreed, I'm also unsure about this one.

However, if we don't do this, all the scripts of the extension will be injected in all tabs that start with https://tails.boum.org. While this might be ignored, I find that it's not a clean way to do it.

Reading you better: I can add "upgrade/" to the allowed injection URLs :)

#23 Updated by sajolida 10 months ago

  • Affected tool set to Verification Extension

#24 Updated by sajolida 10 months ago

I'm joining this discussion as the maintainer of the verification extension (outside of this work on distributing USB images) as we've been discussing with Ulrike to use some of the core budget for part of the cleaning up and other improvements that she already started to work on.

So far I don't understand your discussion around the changes to the IDF format. Is this strictly needed for the work on the USB image project? Why?

#26 Updated by intrigeri 10 months ago

I'm joining this discussion as the maintainer of the verification extension (outside of this work on distributing USB images) as we've been discussing with Ulrike to use some of the core budget for part of the cleaning up and other improvements that she already started to work on.

Just to avoid misunderstandings: the IDF format change we're discussing here (whichever option u ends up picking) is definitely neither maintenance nor random improvements: it's about extending existing code & file formats to satisfy the new needs brought by the USB image project.

So far I don't understand your discussion around the changes to the IDF format. Is this strictly needed for the work on the USB image project? Why?

On the short term, both options work just fine, so my answer would be: no, we don't strictly need to use terminology consistently.
On the long term, I would argue that consistent terminology is useful: it makes it easier to maintain and contribute to our code.
But since we have vague long-term plans to ditch our Upgrader, maybe it's not a big deal here: if/once that happens, the only remaining inconsistency will be between the meaning of target-files in the IDF file format vs. where this concept came from (the TUF framework or Mozilla's update description format, I don't remember).

#27 Updated by intrigeri 10 months ago

  • Assignee changed from intrigeri to u
  • QA Check changed from Info Needed to Dev Needed
  • Regarding the file type check (and assuming it does not come from the web server the file was downloaded from): if the MIME type check is implemented by browsers using platform-specific mechanisms, I need to tell you that we've had a few bad experiences with shared-mime-info, such as regressions in detected MIME types. In the history of Tails, we've had to contribute various fixes upstream about such problems; it's unlikely that these fixes made it into already released LTS distros. Given this extension is supposed to work as-is on any platform, I'm slightly concerned: e.g. if a new shared-mime-info recognizes our .img as application/x-something-else on recent distros, we'll need to support both application/x-raw-disk-image (for compatibility with older distros) and application/x-something-else; and once I think about what various versions of Windows and macOS can do, I'm a bit sceptical. Checking the file extension would arguably be more ugly, but perhaps would not have such long-term maintenance issues? Your call!

This is something we need to test first.

Good idea, but FTR I doubt we'll gain enough confidence only by exploratory testing: this code needs to work on too many platforms we cannot realistically test it on, so even once you've tested it on the few systems you have handy, I'll still be left with the same doubt. So I'm tempted to check how this is specified in the JS specs and implemented in Firefox.

For that to happen: could you provide such an image to me?

Sure! For now, I think your best bet is to grab a .img file there: https://nightly.tails.boum.org/build_Tails_ISO_feature-15292-usb-image/lastSuccessful/archive/build-artifacts/

  • Whenever we'll feel this code is ready to be released, please make it explicit which versions of Firefox you've tested: ideally this would be tested on the current ESR, current release channel, and stable Tor Browser.

I'm unsure what "current release channel" means. May you please explain this?

tl;dr: on the latest non-ESR stable Firefox release i.e. FF 63 currently.

(Ignoring ESR, Mozilla maintains 3 different release channels for Firefox: "central" (= Git master in many other workflows), "beta" and "release". The "release" one is the latest non-ESR stable Firefox release. So the "current release channel" is Firefox 63 today and in December it'll be Firefox 64: https://wiki.mozilla.org/Release_Management/Calendar.)

As said, I don't care much. If you had an idea for such a format in mind already, please make it clear and prepare a file for me to test the extension code on.

OK, here we go, here's the data structure I have in mind. For now I'll write it in YAML to ease comparison with the original UDF and IDF formats and your new one:

---
build-target: amd64
channel: stable
product-name: Tails
installations:
- version: 3.10.1
  installation-paths:
  - type: iso
    target-files:
    - url: http://dl.amnesia.boum.org/tails/stable/tails-amd64-3.10.1/tails-amd64-3.10.1.iso
      sha256: 6c6849b2cbaba7f88779ae74c4481d0149cfcf847e2bc6082862440ec6873733
      size: 1225568256
  - type: img
    target-files:
    - url: http://dl.amnesia.boum.org/tails/stable/tails-amd64-3.10.1/tails-amd64-3.10.1.img
      sha256: 0b27fd69b886f34bf30263d366a4ef3d0f5cb674f4af9c160a44d9bb87cb63a4
      size: 1225568256

… i.e. exactly the same format as our IUKs (with room for all kinds of future extension), modulo:

  • s/upgrade/installation/
  • instead of "full" and "incremental" options, we have "iso" and "img"
  • I'm skipping some additional fields that we don't need here.

But I have strong doubts about whether it's feasible to stick to YAML here. I care about the data structure, not about the format it's encoded with. Initially we asked "can we have YAML?" because we used YAML for UDFs already. The original author of DAVE answered "yeah, sure" and wrote the regexp-based pseudo-parser we have, which does the job but cannot reasonably support a more complex data structure. Assuming there's no readily usable proper YAML parsing library, switching to JSON is probably our best bet here: as you know it's natively supported by JavaScript, we can write a schema to document the format and validate files before we publish them, and we would get rid of the ugly pseudo-YAML-parser.

Now, if you tell me "oh my, that's terribly over-engineered", I'll understand. But then I'd like target-files to be renamed (e.g. to installation-paths) to avoid confusion.

I considered what I did with the extension to be some kind of prototype, and if you want to change the format, we can adapt the code accordingly. The current solution was picked because it does not require big code changes, but that does not mean it's the best solution.

We're on the same page :)

#28 Updated by sajolida 10 months ago

  • Description updated (diff)

#29 Updated by u 10 months ago

@intrigeri: This YAML data structure requires reading lines in a precise order. It worked when there was only one file but with two files that looks more error prone to me. I think this is absolutely terrible design. Hence using JSON seems like a much better bet. And indeed would also allow to write a scheme and Git hook validating the format. Should I or do you want to come up with a JSON format for this?

#30 Updated by u 10 months ago

  • Related to Feature #16128: Move the verification JavaScript from the verification extension to the page itself added

#31 Updated by sajolida 10 months ago

:

Just to avoid misunderstandings: the IDF format change we're discussing here (whichever option u ends up picking) is definitely neither maintenance nor random improvements: it's about extending existing code & file formats to satisfy the new needs brought by the USB image project.

In the budget for this ticket we wrote:

* Have Tails Verification handle two IDFs
(or one IDF that references both the ISO and .img)

The 1st strategy ("handle two IDFs") was to have 2 different IDFs with
the same good old format.

Right now we have:

  • /install/v1/Tails/amd64/stable/latest.yml

I think we could use the same format and have 2 files instead:

  • /install/v1/Tails/amd64/stable/latest-iso.yml
  • /install/v1/Tails/amd64/stable/latest-img.yml

The extension would verify again latest-iso.yml or latest-img.yml
depending on the version of the download page; each version of the
download page downloads only 1 of the 2 files depending on the
installation scenario.

We put this strategy first because we thought it would be cheaper to
implement. The code for the different versions of the download is
already written and reviewed and I can explain more if needed.

The 2nd strategy ("one IDF that references both the ISO and .img")
clearly implied a new file format. Because of this, it was put second
(and in parenthesis) because we thought it would be more complicated to
implement.

I understand from the discussion here (and Ulrike's code that I briefly
scanned) that you went for the 2nd strategy.

Seeing that Ulrike is overclocked already, I'd like to know if the 1st
strategy was considered and discarded for some reason that we overlooked
when building the budget.

Otherwise, I'd like to know, at this point in the development, which of
the 1st and 2nd strategy will be less expensive to implement (possibly
discarding some of the work that was done already).

#32 Updated by intrigeri 10 months ago

@intrigeri: This YAML data structure requires reading lines in a precise order.

I don't understand what you mean. But wait, my example was mangled when copy'n'pasting, I've updated it and here's the correct version: #15995#note-27.

Should I or do you want to come up with a JSON format for this?

Please go ahead and convert this data structure to JSON. It should be 100% mechanical (I would actually do it programmatically by loading this YAML into a Perl/Python/Ruby data structure that I would then export to JSON). But if you need help, I'm here :)

#33 Updated by intrigeri 10 months ago

I'd like to know if the 1st strategy was considered and discarded for some reason that we overlooked when building the budget.

Me too: I did not understand the reasoning in #15995#note-8.

If we switch back to that 1st strategy, then ignore everything I wrote about the IDF data format :)

#34 Updated by u 10 months ago

sajolida wrote:

:

Just to avoid misunderstandings: the IDF format change we're discussing here (whichever option u ends up picking) is definitely neither maintenance nor random improvements: it's about extending existing code & file formats to satisfy the new needs brought by the USB image project.

In the budget for this ticket we wrote:

Thanks for checking!

I understand from the discussion here (and Ulrike's code that I briefly
scanned) that you went for the 2nd strategy.

Seeing that Ulrike is overclocked already, I'd like to know if the 1st
strategy was considered and discarded for some reason that we overlooked
when building the budget.

Otherwise, I'd like to know, at this point in the development, which of
the 1st and 2nd strategy will be less expensive to implement (possibly
discarding some of the work that was done already).

To me these strategies were not hierarchized. I've considered the first strategy but discarded it for the following reasons:

First of all the extension's code is fixed (runs on all pages the same code I mean), so I would definitely not want to bother checking from which page I download something. That would be unmaintainable if we change the name of one of these pages, or forget some div somewhere. Also, I want the code to do IMG and ISO verification at the same time, because one could also have downloaded or copied the ISO/IMG beforehand and then expect the verification to work.

→ So we need to download one file with all info or both files all the time.

Downloading both files will add more load on our server: there is one more HTTP request to execute. Also note, that this happens all the time currently when people with the extension installed visit any page on t.b.o. It's bad design and I tried to limit this to visiting /install only. (Needs to be fixed to also include /upgrade).

Then, using two files would have complexified the code quite a bit and I wanted to have clean solution. (Now that we've cleaned the code up a lot, this option could be reconsidered, especially when I'll get rid of using jQuery Ajax instead of Fetch(). But this still leaves us with a multiplication of HTTP requests).

Now, the cleanest way to do it IMO would be to use a clear data structure that does not require reading lines in a particular order and would possibly simplify the life of the RM on the long term (Generating only one file instead of two.) Also, if we drop the Upgrader at some point, using JSON with JS seems much more futureproof to me than to work around reading a YAML file.

#35 Updated by u 10 months ago

intrigeri wrote:

I'd like to know if the 1st strategy was considered and discarded for some reason that we overlooked when building the budget.

Me too: I did not understand the reasoning in #15995#note-8.

I should have explained how I came to that conclusion. I hope that's a bit clearer now.

#37 Updated by u 10 months ago

But in summary, if you think we should do it, let's generate two yaml files, and I'll download both of them all the time, I have no strong opinion on it, and once I'm done with #16119, it will be easier to do that.

#38 Updated by sajolida 10 months ago

In the same way as I already have a bunch of <div> with metadata at the beginning of the download page (eg. "activate-tails-verification" and "extension-version" in /install/inc/steps/download.inline.html), it would be easy for me to add another one that indicates whether to verify an ISO image or an USB image depending on the version of the download page. If this helps of course.

#39 Updated by u 10 months ago

sajolida wrote:

In the same way as I already have a bunch of <div> with metadata at the beginning of the download page (eg. "activate-tails-verification" and "extension-version" in /install/inc/steps/download.inline.html), it would be easy for me to add another one that indicates whether to verify an ISO image or an USB image depending on the version of the download page. If this helps of course.

Thank you but I think this is not helpful. Imagine the scenario in which a user had previously downloaded an ISO and then gone to do other things in their life, switched off the computer and comes back some days later to follow the installation instructions. Now they accidentally end up on the other page. Then this would not work anymore. So I don't want to rely on a div, I want to rely on what the user wants to validate.

#40 Updated by u 10 months ago

@intrigeri: Thank you for re-pasting the YAML.
Did you know that JSON is considered to be valid YAML? Maybe this information can give us the best of both worlds?

So YAML like that:

---

product-name: Tails
installations:
  version: 3.10.1
  installation-paths:
    - type: iso
      target-files:
      - url:          http://dl.amnesia.boum.org/tails/stable/tails-amd64-3.10.1/tails-amd64-3.10.1.iso
      - sha256: 6c6849b2cbaba7f88779ae74c4481d0149cfcf847e2bc6082862440ec6873733
      - size: 1225568256
    - type: img
      target-files:
      - url: http://dl.amnesia.boum.org/tails/stable/tails-amd64-3.10.1/tails-amd64-3.10.1.img
      - sha256: 0b27fd69b886f34bf30263d366a4ef3d0f5cb674f4af9c160a44d9bb87cb63a4
      - size: 1225568256

(But I think your YAML does not contain that many dashs.

would result in JSON :

{
  "product-name": "Tails",
  "installations": {
    "version": "3.10.1",
    "installation-paths": [
      {
        "type": "iso",
        "target-files": [
          {
            "url": "http://dl.amnesia.boum.org/tails/stable/tails-amd64-3.10.1/tails-amd64-3.10.1.iso" 
          },
          {
            "sha256": "6c6849b2cbaba7f88779ae74c4481d0149cfcf847e2bc6082862440ec6873733" 
          },
          {
            "size": 1225568256
          }
        ]
      },
      {
        "type": "img",
        "target-files": [
          {
            "url": "http://dl.amnesia.boum.org/tails/stable/tails-amd64-3.10.1/tails-amd64-3.10.1.img" 
          },
          {
            "sha256": "0b27fd69b886f34bf30263d366a4ef3d0f5cb674f4af9c160a44d9bb87cb63a4" 
          },
          {
            "size": 1225568256
          }
        ]
      }
    ]
  }
}

#41 Updated by intrigeri 10 months ago

Did you know that JSON is considered to be valid YAML? Maybe this information can give us the best of both worlds?

Wow, no, I had no clue!

> […]
>       target-files:
>       - url: http://dl.amnesia.boum.org/tails/stable/tails-amd64-3.10.1/tails-amd64-3.10.1.img
>       - sha256: 0b27fd69b886f34bf30263d366a4ef3d0f5cb674f4af9c160a44d9bb87cb63a4
>       - size: 1225568256
> 

(But I think your YAML does not contain that many dashs.

Indeed, that's a different thing (and I really don't understand what it means nor how we could use it): I meant target-files to be a list of data structures that each represent a file to download/verify (URL, hash, size).

would result in JSON :

And I think mine would result in something like:

"target-files": [
{
"url": "http://dl.amnesia.boum.org/tails/stable/tails-amd64-3.10.1/tails-amd64-3.10.1.iso" 
"sha256": "6c6849b2cbaba7f88779ae74c4481d0149cfcf847e2bc6082862440ec6873733",
"size": 1225568256
},
]

#42 Updated by intrigeri 10 months ago

I've no clue what's going on but my sample YAML data (#15995#note-32) got mangled again and is still buggy. I'm pretty sure I had fixed it the other day :/ Sorry about that. I'll email you what I really mean ASAP.

#43 Updated by intrigeri 10 months ago

Actually the one on #15995#note-27 is correct. So I did correctly fix that one, but then pasted the wrong one in my next comment, hahaha!

Once sent through https://yamlvalidator.com/ this gives me this JSON:

{
  "build-target": "amd64",
  "channel": "stable",
  "product-name": "Tails",
  "installations": [
    {
      "version": "3.10.1",
      "installation-paths": [
        {
          "type": "iso",
          "target-files": [
            {
              "url": "http://dl.amnesia.boum.org/tails/stable/tails-amd64-3.10.1/tails-amd64-3.10.1.iso",
              "sha256": "6c6849b2cbaba7f88779ae74c4481d0149cfcf847e2bc6082862440ec6873733",
              "size": 1225568256
            }
          ]
        },
        {
          "type": "img",
          "target-files": [
            {
              "url": "http://dl.amnesia.boum.org/tails/stable/tails-amd64-3.10.1/tails-amd64-3.10.1.img",
              "sha256": "0b27fd69b886f34bf30263d366a4ef3d0f5cb674f4af9c160a44d9bb87cb63a4",
              "size": 1225568256
            }
          ]
        }
      ]
    }
  ]
}

… which seems to be exactly what I meant :)

#44 Updated by u 10 months ago

Cool, the latter one looks pretty much what we want then :)

#45 Updated by u 10 months ago

intrigeri wrote:

Did you know that JSON is considered to be valid YAML? Maybe this information can give us the best of both worlds?

Wow, no, I had no clue!

I should credit Enrico for that, who, for another reason, has read the entire YAML documentation and remembered that little detail.

#46 Updated by u 10 months ago

#47 Updated by sajolida 10 months ago

Imagine the scenario in which a user had previously downloaded an ISO and then gone to do other things in their life, switched off the computer and comes back some days later to follow the installation instructions.

If someone downloads an ISO and then tries to install it from Windows,
they will end up with what we used to call an intermediary Tails and
will get into serious troubles down the line (no upgrade, no
persistence). People downloading an USB image and then burning it on a
DVD will get into trouble as well.

We thought about this while writing the proposal and included in our
work on the download page: "Add watchdogs in all installation scenarios
to make sure that people have the right file".

So I still think that the best would be to validate only USB images on
the Windows scenario (and prevent people from installing an ISO). But I
don't think it's worth adding this complexity to our code to handle this
(and I also won't have time to work more on the download page).

Right now I added a note to the installation steps for people to check
the extension of their file (d3b54bb5fd). Note that the minority of
people using an ISO image (DVD and VM) won't see such a note because
they don't have specific installation steps.

I also don't think that this will happen a lot since all scenarios will
use the USB image and the ISO image will only be accessible to people
click on the DVD or VM link (which are already a bit hidden by default).
So I'm not so worried about such scenarios...

So yeah, I think you should do whatever is the easiest for you :)

#48 Updated by u 10 months ago

During the meeting this morning intrigeri and me agreed to implement the structure as described in https://redmine.tails.boum.org/code/issues/15995#note-43. It's slightly overengineered, but forwards compatible.

This will intersects sajolida's work (releasing the add-on) and intrigeri's work (release process) and both should come up with a plan on when changes will be merged into master and the addon should be put out.

To prepare when publishing v2: have the installation assistant require the new version of the add-on (it has a version check). Besided this, documentation should not be impacted by this change, only the design doc.

#15999 could be done in a way that at least for now, we keep generating + publishing v1 for ISO, so there's some degree of backwards compat and maybe less of a flag day.

#49 Updated by u 10 months ago

TODO:

- have the extension download JSON instead of YAML
- have a check on file extensions on top of the MIME type check
- update forge library
- see if there is a signed git tag of forge library releases
- update readme (see sajolida's changes on master)
- make it reload itself in Chromium
- test extensively

#50 Updated by intrigeri 10 months ago

u wrote:

- have the extension download JSON instead of YAML

FYI my branch for #16171 adds an IDF v2 (JSON) for the current Tails release, and updates the release process to that we publish both IDF v1 and v2 for now. It should be merged into master soonish so you'll be able to test your updated code against our live website :)

- have a check on file extensions on top of the MIME type check

I'm not sure about "on top of": if this is about the MIME type check concern I had, my concern is that it may reject valid files in some cases, so adding another check on top won't help; now, if you mean something like "alternatively", i.e. if the MIME type check fails, fall back to checking the extension, yeah, cool!

#51 Updated by u 10 months ago

intrigeri wrote:

u wrote:

- have the extension download JSON instead of YAML

FYI my branch for #16171 adds an IDF v2 (JSON) for the current Tails release, and updates the release process to that we publish both IDF v1 and v2 for now. It should be merged into master soonish so you'll be able to test your updated code against our live website :)

Cool.

- have a check on file extensions on top of the MIME type check

I'm not sure about "on top of": if this is about the MIME type check concern I had, my concern is that it may reject valid files in some cases, so adding another check on top won't help; now, if you mean something like "alternatively", i.e. if the MIME type check fails, fall back to checking the extension, yeah, cool!

That was exactly my plan, i.e. if the MIME type check fails.

#52 Updated by u 10 months ago

  • QA Check changed from Dev Needed to Ready for QA

This is now done.

- I'm verifying MIME type and the extension.
- I'm using the new JSON format.
- We could theoretically use the code of the extension to verify anything we want, even Debian ISOs now :)
I think there could be an improvement made on the version checks: this was currently not necessary, as we distributed only one version. But if we start putting several versions in the JSON, the code should change so that if one would verify an old ISO/IMG, one would get told: the hash is correct, but your version is outdated.
- I got rid of jQuery (yay!)
- I updated the forge library.
- I updated the README.
- I fixed the issue raised concerning /upgrade.
- I tested extensively only in Chromium for now and will do more testing.

I'm asking Enrico to do a last review from a security POV.
intrigeri can already review the branch from his side if he wants to.

#53 Updated by u 10 months ago

Now also tested in FF and Chrome Beta. I did not test it on Firefox ESR.

#54 Updated by u 10 months ago

FYI In master, we modified the content security policy in 3e9c4f8ecd42a17fabfeeab939d4b3dbb16dbb26. This was necessary because we injected scripts into the browser in some sort of workaround. In the usbimage branch we got rid of this workaround and it seems to work as is, without loosening the CSR.

#55 Updated by u 10 months ago

In Firefox, when testing, simply activating and deactivating the extension works as intendend and the scripts are injected into the open tabs automatically.

It's supposed to run automatically in Chrome too, but I seem to not be able to test this locally by activating and deactivating the extension: https://developer.chrome.com/extensions/content_scripts#declaratively

I'm vaguely considering to write a script for Chrome only that reloads the affected pages. And then we would have this script ready for deployment if needed, but we would only include it in that particular case.

#56 Updated by u 10 months ago

  • Assignee changed from u to intrigeri

I let you review this now. Note that I had to force push on the last modification of the branch. It should not affect anyone else than the reviewer.

#57 Updated by intrigeri 10 months ago

  • Assignee changed from intrigeri to u
  • QA Check changed from Ready for QA to Dev Needed

I let you review this now.

Thanks! There we go :)

As discussed earlier I've only skimmed over most of the code changes because Enrico, who's waaaaay better than me at JS, already reviewed them.

All this looks good except two things:

  • My — rather sensitive — detector for suspicious regexp got triggered. I don't understand the intent behind (file.name).match(/^(.*\.iso$)?[^.]*$/i) and its .img counterpart. The commit message (233a1c2b6980ccccd1abaeb4cfeeeef6b443c0a5) says "let's also check for the file extension" but this would presumably be achieved by a mere (file.name).match(/\.iso$/i) so I wonder why the code does something else. Why do we also consider as ISO files any file whose name satisfies the "does not contain a dot" condition? Besides, even if we really want to do that, then we can drop the similar bit in the regexp for the following else if for .img: we'll already have matched in the first if for ISO images, so it can't possibly match there too and it just makes the code more complex and somewhat confusing. Note that the current code will treat as an ISO any file whose name contains no dot, even if it has the application/x-raw-disk-image MIME type.
  • Could you please merge the current master branch into yours, just to be sure it won't have conflicts? I see that you've cherry-picked one of its commits already but let's do the whole thing :)

#58 Updated by u 10 months ago

u wrote:

In Firefox, when testing, simply activating and deactivating the extension works as intendend and the scripts are injected into the open tabs automatically.

It's supposed to run automatically in Chrome too, but I seem to not be able to test this locally by activating and deactivating the extension: https://developer.chrome.com/extensions/content_scripts#declaratively

I'm vaguely considering to write a script for Chrome only that reloads the affected pages. And then we would have this script ready for deployment if needed, but we would only include it in that particular case.

This could also be solved by #16128.

#59 Updated by u 10 months ago

intrigeri wrote:

I let you review this now.

Thanks! There we go :)

As discussed earlier I've only skimmed over most of the code changes because Enrico, who's waaaaay better than me at JS, already reviewed them.

All this looks good except two things:

  • My — rather sensitive — detector for suspicious regexp got triggered. I don't understand the intent behind (file.name).match(/^(.*\.iso$)?[^.]*$/i) and its .img counterpart. The commit message (233a1c2b6980ccccd1abaeb4cfeeeef6b443c0a5) says "let's also check for the file extension" but this would presumably be achieved by a mere (file.name).match(/\.iso$/i) so I wonder why the code does something else. Why do we also consider as ISO files any file whose name satisfies the "does not contain a dot" condition? Besides, even if we really want to do that, then we can drop the similar bit in the regexp for the following else if for .img: we'll already have matched in the first if for ISO images, so it can't possibly match there too and it just makes the code more complex and somewhat confusing. Note that the current code will treat as an ISO any file whose name contains no dot, even if it has the application/x-raw-disk-image MIME type.

Right, I made it more complicated to achieve less apparently :) Fixed that.

  • Could you please merge the current master branch into yours, just to be sure it won't have conflicts? I see that you've cherry-picked one of its commits already but let's do the whole thing :)

I'll have some fun with this.

#60 Updated by u 10 months ago

I merged the master branch, fixed conflicts and pushed.

#61 Updated by u 10 months ago

  • QA Check changed from Dev Needed to Ready for QA

#62 Updated by u 10 months ago

  • Assignee changed from u to intrigeri

Could you please check the regexp again? I know I initially said I check for iso/img and only if the MIME type does not work check for the file extension, but in the end I did it differently. let me know if this still addresses your concerns or if you prefer that I stick to the original idea.

#63 Updated by intrigeri 10 months ago

  • My — rather sensitive — detector for suspicious regexp got triggered. I don't understand the intent behind (file.name).match(/^(.*\.iso$)?[^.]*$/i) and its .img counterpart. The commit message (233a1c2b6980ccccd1abaeb4cfeeeef6b443c0a5) says "let's also check for the file extension" but this would presumably be achieved by a mere (file.name).match(/\.iso$/i) so I wonder why the code does something else. Why do we also consider as ISO files any file whose name satisfies the "does not contain a dot" condition? Besides, even if we really want to do that, then we can drop the similar bit in the regexp for the following else if for .img: we'll already have matched in the first if for ISO images, so it can't possibly match there too and it just makes the code more complex and somewhat confusing. Note that the current code will treat as an ISO any file whose name contains no dot, even if it has the application/x-raw-disk-image MIME type.

Right, I made it more complicated to achieve less apparently :) Fixed that.

LGTM!

#64 Updated by u 10 months ago

  • Assignee changed from intrigeri to u

I also got the other review of code. Later this week we'll try to find out more about the security policy.

#65 Updated by u 10 months ago

  • % Done changed from 20 to 80
  • QA Check deleted (Ready for QA)

#66 Updated by intrigeri 10 months ago

Could you please check the regexp again? I know I initially said I check for iso/img and only if the MIME type does not work check for the file extension, but in the end I did it differently. let me know if this still addresses your concerns or if you prefer that I stick to the original idea.

It seems to me that you implemented what was planned (we did not really specify if the fallback to checking file extensions would be done after checking the 2 MIME types or after each MIME type check). LGTM.

#67 Updated by intrigeri 10 months ago

u wrote:

Later this week we'll try to find out more about the security policy.

Just curious: what security policy?

#68 Updated by u 10 months ago

that one: https://developer.mozilla.org/en-US/docs/Web/HTTP/CSP (specified in the extension)

#69 Updated by intrigeri 10 months ago

that one: https://developer.mozilla.org/en-US/docs/Web/HTTP/CSP (specified in the extension)

OK, so I understand this is about #15995#note-54, i.e. trying to revert 3e9c4f8ecd42a17fabfeeab939d4b3dbb16dbb26 as a security improvement made possible by your changes on this branch. I understand it's tempting to add as many improvements as we can to this branch because it'll minimize context switching, reviewing & QA overhead, which is good. But I'd like to have one less "almost done" thing on my plate and I'm not convinced this should block completing my work on this ticket, i.e. me doing a final quick review, testing, merging into master and sending over to sajolida for #15997.

Assuming there are no known blockers (please correct me if I'm wrong), what do you thing about I finish my part of the work tomorrow (if the CSP has been made stricter by then, great! and if not, no big deal, it can be handled elsewhere)?

If it's more complicated than that or if I missed something, we can discuss it at the team meeting this afternoon :)

#70 Updated by u 10 months ago

Currently I'm using the same CSP as master. And fine with me to leave it like that for now.

IMO a merge into master is possible, but there is currently one blocker: it seems that the page does not update in Chrome when the extension is freshly installed. This part of the work is still missing and I won't be able to do that before next week. Let's talk about it at the meeting.

#71 Updated by intrigeri 10 months ago

IMO a merge into master is possible, but there is currently one blocker: it seems that the page does not update in Chrome when the extension is freshly installed.

Assuming that's a regression, indeed it's a blocker.

This part of the work is still missing and I won't be able to do that before next week. Let's talk about it at the meeting.

OK!

#72 Updated by u 10 months ago

  • Assignee changed from u to intrigeri
  • QA Check set to Ready for QA

There you go, please review at commit: 17bd30bd326ba4bb148f43dee08e716ee1e0a7c0

#73 Updated by u 10 months ago

  • Related to Bug #16200: Allow verifying different versions of Tails (older versions, testing versions, etc.) added

#74 Updated by u 10 months ago

  • Subject changed from Have Tails Verification handle two IDFs to Have Tails Verification handle IDFs with several Tails installation paths (ISO/IMG)

#75 Updated by intrigeri 10 months ago

Thanks! I'll review at commit 17bd30bd326ba4bb148f43dee08e716ee1e0a7c0 (git diff --ignore-space-change tells me it's the same code modulo space changes).

#76 Updated by u 10 months ago

BTW i made a slight mistakes in commit message 2aa7f71d20f87dc54dee856f4eb86bb73c57443f. I also modified the code from jQuery to Vanilla JS and forgot to mention it in the message. But I had already pushed and did not want to rebase and force push.

#77 Updated by u 10 months ago

intrigeri wrote:

Thanks! I'll review at commit 17bd30bd326ba4bb148f43dee08e716ee1e0a7c0 (git diff --ignore-space-change tells me it's the same code modulo space changes).

I think you need to review from a3e4162706268d32e6bd44de2aa88835736f79e5.

#78 Updated by intrigeri 10 months ago

  • Assignee changed from intrigeri to u
  • QA Check changed from Ready for QA to Dev Needed

All right, we're almost there, I only have a few polishing requests! Congrats \o/

Regarding the jQuery removal:

  • Woohoo, less (vendorized) dependencies :)))
  • scripts/vendor/forge/forge.min.js seems to use jQuery in a couple places; I guess the part of the Forge library that we use ourselves doesn't, and if that ever changes, then we'll notice obvious breakage when we test things after we update this vendorized library, right? I'd like to know why I should be confident about the "obvious breakage" part. But if that's too hard, forget it.
  • There are still bits about jquery in README.

In the changelog snippet added for 2.0:

  • It would be nice to make it explicit that support for IDF v1 is being replaced by support for IDF v2 (this API endpoint and file format has formal versioning, let's use it :)
  • "CSP: specify script-src and object-src" seems to match no change brought by this branch; I guess that's about the additional CSP work you want to do soon. If I'm correct, then this bullet point should be removed for now.
  • In "Restrict loading of the extension's scripts", I suggest appending /* to the URLs, to avoid any misunderstanding.

Other than that, I see you've added a "FIXME" to README. Is this something you plan to fix before the release, as part of this ticket, or a mere note for later?

Now I'll test the new code a little bit. From now on, please push to this topic branch only changes that won't invalidate my testing (the ones I'm requesting above should be fine).

#79 Updated by intrigeri 10 months ago

intrigeri wrote:

Other than that, I see you've added a "FIXME" to README. Is this something you plan to fix before the release, as part of this ticket, or a mere note for later?

Actually it seems to be a blocker: I don't manage to follow these instructions because on our current master branch, line 182 is something else. I'll ignore this problem and will try to test the code anyway.

#80 Updated by intrigeri 10 months ago

On a local version of our website, in Chromium, install/download.en.html wants me to install the extension, even though I've done the "Load unpacked extension…" trick first. So I click through it and install 1.2. But it still wants me to install the extension.

Similarly, in Firefox 63, I did "Load Temporary Add-on" but then I'm asked to install the extension anyway.

I guess that's because the temporary ID the browsers give the extension when loaded this way differs from what our website expects. That would make sense. But then, I don't understand how the instructions in the "Testing the extension locally" section can work as-is. I think that's out of scope of this ticket but this prevents me from testing the verification process. Any hint? Should I follow the "Release process" section and build a zip?

Regarding commit 428641b670ed5a2364459df23e6940733d509fef and "no more need of patching this brutally": FWIW personally I prefer copy'n'pasting a command line that will apply a patch, to manually editing a file based on a patch-like-but-not-really-a-patch plaintext snippet. But I'm not the one who'll have to follow this regularly so: whatever works for you :)

#81 Updated by u 9 months ago

  • Assignee changed from u to intrigeri
  • QA Check changed from Dev Needed to Ready for QA

intrigeri wrote:

On a local version of our website, in Chromium, install/download.en.html wants me to install the extension, even though I've done the "Load unpacked extension…" trick first. So I click through it and install 1.2. But it still wants me to install the extension.

Similarly, in Firefox 63, I did "Load Temporary Add-on" but then I'm asked to install the extension anyway.

For testing with a local version of the website you need to add to manifest.json under "permissions" the permission to access "file:///".

Regarding commit 428641b670ed5a2364459df23e6940733d509fef and "no more need of patching this brutally": FWIW personally I prefer copy'n'pasting a command line that will apply a patch, to manually editing a file based on a patch-like-but-not-really-a-patch plaintext snippet. But I'm not the one who'll have to follow this regularly so: whatever works for you :)

It is very easy IMO to manually add the abovementioned line, no need to write and maintain a patch for this.

#82 Updated by u 9 months ago

I did not yet implement the proposed changes yet because it seems you were not really able to test the extension, right?

#83 Updated by u 9 months ago

intrigeri wrote:

All right, we're almost there, I only have a few polishing requests! Congrats \o/

Regarding the jQuery removal:

  • Woohoo, less (vendorized) dependencies :)))
  • scripts/vendor/forge/forge.min.js seems to use jQuery in a couple places; I guess the part of the Forge library that we use ourselves doesn't, and if that ever changes, then we'll notice obvious breakage when we test things after we update this vendorized library, right? I'd like to know why I should be confident about the "obvious breakage" part. But if that's too hard, forget it.

Right. jQuery is an optional dependency of forge that is used to collect entropy via mouse movements for example.
There is a ticket in the bugtracker of forge that wants to get rid of jQuery: https://github.com/digitalbazaar/forge/issues/522, I'm pretty confident this might actually happen.

  • There are still bits about jquery in README.

fixed, thanks for spotting this.

In the changelog snippet added for 2.0:

  • It would be nice to make it explicit that support for IDF v1 is being replaced by support for IDF v2 (this API endpoint and file format has formal versioning, let's use it :)

Added that info to the changelog.

  • "CSP: specify script-src and object-src" seems to match no change brought by this branch; I guess that's about the additional CSP work you want to do soon. If I'm correct, then this bullet point should be removed for now.

ack.

  • In "Restrict loading of the extension's scripts", I suggest appending /* to the URLs, to avoid any misunderstanding.

Great idea, done.

Other than that, I see you've added a "FIXME" to README. Is this something you plan to fix before the release, as part of this ticket, or a mere note for later?

This needs to be added to the patch for the local testing. I did that in 5b01e16d8fec36d84bc093a55a44cd1cf4c7364a.

Now I'll test the new code a little bit. From now on, please push to this topic branch only changes that won't invalidate my testing (the ones I'm requesting above should be fine).

Ack, pushed all of them now.

#84 Updated by u 9 months ago

  • Parent task changed from #15994 to #15292

#85 Updated by intrigeri 9 months ago

  • Target version changed from Tails_3.11 to Tails_3.12

#86 Updated by intrigeri 9 months ago

Reviewed all the fixes (up to cbf138864c58d5433b2f93c9c99ca52e6f431121) I've requested, LGTM! I'm going to test this again thanks to your updated instructions.

#87 Updated by intrigeri 9 months ago

  • Assignee changed from intrigeri to u
  • QA Check changed from Ready for QA to Pass

I've tried again. In all cases I've used a local build of the website on file:/// (i.e. with no local webserver).

I finally managed to have the temporary add-on fire up. I had to patch manifest.json a bit more than what's documented. Pushed an update to README with my findings.

FWIW, here the JS errors land on Firefox' stdout (so in the Journal) and not in the Debugger. Took me some time to figure it out. Might be a custom local config, I should retry with a fresh profile. Some day :]

I've successfully verified, with Firefox 64.0-1 from sid, the 3.11 ISO and a locally built USB image (for the later, I've tweaked latest.json and pointed conf.js to my local one; I was surprised the add-on allowed using an IDF downloaded without HTTPS by the way).

Then with Chromium 72.0.3626.7-3 from sid, I've tried to verify the 3.11 ISO. After clicking "Verify" a progress bar appears but nothing else seems to be happening. But that's because URL scheme must be "http" or "https" for CORS request. and I still had my local tweak to point to my local latest.json. Once I've reverted this local tweak I could flawlessly verify the 3.11 ISO :) But I could not try verifying a USB image because we haven't published an IDF that references one yet and I'm too lazy to push one somewhere else. As long as you've tested this somehow, happy to call this good enough.

With both browsers I've also tested verifying the wrong file and seen the verification fail as expected. With Firefox I've also tested verifying a file that has the correct hash but not the correct size and seen the verification fail as expected.

I've not tested on Firefox ESR nor Tor Browser. I think that's OK but I suspect sajolida will appreciate if at least you tell him what browsers you've already tested yourself :)

So from my PoV this works pretty well, congrats! I'll let you handle the next steps, whichever they are.

#88 Updated by u 9 months ago

intrigeri wrote:

I've tried again. In all cases I've used a local build of the website on file:/// (i.e. with no local webserver).

I finally managed to have the temporary add-on fire up. I had to patch manifest.json a bit more than what's documented. Pushed an update to README with my findings.

Great! It looks like something I omitted unintentionally.

FWIW, here the JS errors land on Firefox' stdout (so in the Journal) and not in the Debugger. Took me some time to figure it out. Might be a custom local config, I should retry with a fresh profile. Some day :]

Yes, they do: we could change this if we want, but I find it more convenient like that. We could also disable logging in the pulic release of the extension. I let sajolida decide what he fins more convenient.

I've successfully verified, with Firefox 64.0-1 from sid, the 3.11 ISO and a locally built USB image (for the later, I've tweaked latest.json and pointed conf.js to my local one; I was surprised the add-on allowed using an IDF downloaded without HTTPS by the way).

That should probably be fixed, however it's not a regression.

Then with Chromium 72.0.3626.7-3 from sid, I've tried to verify the 3.11 ISO. After clicking "Verify" a progress bar appears but nothing else seems to be happening. But that's because URL scheme must be "http" or "https" for CORS request. and I still had my local tweak to point to my local latest.json. Once I've reverted this local tweak I could flawlessly verify the 3.11 ISO :) But I could not try verifying a USB image because we haven't published an IDF that references one yet and I'm too lazy to push one somewhere else. As long as you've tested this somehow, happy to call this good enough.

I've tested verifying .img files. What I did to do that: uploaded a tweaked version of the IDF to a webserver of mine and changed the URL of the IDF to be downloaed in scripts/conf.js and manifest.json.

I also tested faulty checksums using the same technique.

With both browsers I've also tested verifying the wrong file and seen the verification fail as expected. With Firefox I've also tested verifying a file that has the correct hash but not the correct size and seen the verification fail as expected.

\o/

I've not tested on Firefox ESR nor Tor Browser. I think that's OK but I suspect sajolida will appreciate if at least you tell him what browsers you've already tested yourself :)

I tested on Firefox ESR 60.4.0esr-1~deb9u1, Chromium 71.0.3578.80-1~deb9 and Chrome Beta.

So from my PoV this works pretty well, congrats! I'll let you handle the next steps, whichever they are.

Yay. Thanks!

I'll close this ticket and open one for improving the IDF download over HTTPS.

#89 Updated by u 9 months ago

  • Status changed from In Progress to Resolved
  • % Done changed from 80 to 100

#90 Updated by intrigeri 9 months ago

FWIW, here the JS errors land on Firefox' stdout (so in the Journal) and not in the Debugger. Took me some time to figure it out. Might be a custom local config, I should retry with a fresh profile. Some day :]

Yes, they do: we could change this if we want, but I find it more convenient like that. We could also disable logging in the pulic release of the extension. I let sajolida decide what he fins more convenient.

Personally I don't really care where they end up as long as README points to the right place :)

#91 Updated by u 9 months ago

i added that info to the readme.

Also available in: Atom PDF