Tails

#40 Updated by Anonymous 2018-11-18 13:49:25

@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 2018-11-18 14:08:09

> 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 2018-11-18 14:10:44

I’ve no clue what’s going on but my sample YAML data (Bug #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 2018-11-18 14:16:58

Actually the one on Bug #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 Anonymous 2018-11-18 14:25:27

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

#45 Updated by Anonymous 2018-11-19 07:36:17

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.

#47 Updated by sajolida 2018-11-19 17:19:14

> 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 Anonymous 2018-11-20 09:49:40

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.

#49 Updated by Anonymous 2018-11-20 09:52:43

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 2018-11-29 16:28:20

u wrote:
> - have the extension download JSON instead of YAML

FYI my branch for Bug #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 Anonymous 2018-11-30 14:20:32

intrigeri wrote:
> u wrote:
> > - have the extension download JSON instead of YAML
>
> FYI my branch for Bug #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.

#53 Updated by Anonymous 2018-12-05 10:31:49

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

#54 Updated by Anonymous 2018-12-05 10:39:26

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 Anonymous 2018-12-05 11:49:43

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.

#58 Updated by Anonymous 2018-12-05 13:45:34

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 Feature #16128.

#59 Updated by Anonymous 2018-12-05 13:48:38

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 Anonymous 2018-12-05 13:51:36

I merged the master branch, fixed conflicts and pushed.

#63 Updated by intrigeri 2018-12-05 13:56:47

>> * 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!

#66 Updated by intrigeri 2018-12-06 11:48:53

> 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 2018-12-06 11:49:44

u wrote:
> Later this week we’ll try to find out more about the security policy.

Just curious: what security policy?

#68 Updated by Anonymous 2018-12-06 14:33:34

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

#69 Updated by intrigeri 2018-12-07 09:15:33

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

OK, so I understand this is about Bug #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 Bug #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 Anonymous 2018-12-07 10:13:44

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 2018-12-07 10:20:59

> 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!

#75 Updated by intrigeri 2018-12-07 15:16:25

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 Anonymous 2018-12-07 15:19:59

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 Anonymous 2018-12-07 15:20:51

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.

#79 Updated by intrigeri 2018-12-07 15:47:55

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 2018-12-07 16:04:38

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 :)

#82 Updated by Anonymous 2018-12-11 14:27:09

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 Anonymous 2018-12-11 17:12:58

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.

#86 Updated by intrigeri 2018-12-17 14:09:29

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

#88 Updated by Anonymous 2018-12-17 15:30:02

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.

#90 Updated by intrigeri 2018-12-17 16:24:53

>> 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 Anonymous 2018-12-17 17:42:38

i added that info to the readme.