Changes the format of the signature to allow for later extensions. #18

steamraven · 2018-12-20T22:32:33Z

Should be backwards compatible
Uped version to 0.5.0
Moves from comment to a meta tag inside head
Allows multiple signatures
Allows for a non-minimized signature for firefox users
Adds an allowedMethods field to signatures to limit when the
signature can be used.
Adds a version field to signatures to allow updating minimizer in the
the future
uses npm-install-version to allow multiple versions of the minimizer to
co-exist in the same codebase to allow updates
Updated README

Solves #15 and #6. Lays the foundation for solving #1, #2, #13, and #16

Discussion and comments are welcome

- Should be backwards compatible - Uped version to 0.5.0 - Moves from comment to a meta tag inside head - Allows multiple signatures - Allows for a non-minimized signature for firefox users - Adds an allowedMethods field to signatures to limit when the signature can be used. - Adds a version field to signatures to allow updating minimizer in the the future - uses npm-install-version to allow multiple versions of the minimizer to co-exist in the same codebase to allow updates - Updated README

rugk · 2018-12-20T23:22:46Z

src/background.js

+  const head = content.split(/<\/head\s*>/i)[0];
+
+  // Parse signature metadata
+  const signatureRegex = RegExp('<meta\\s+name="signature"\\s+content="([^"]*)"\\s*>', 'gi');


Hmm, so why is that RegEx safe to use? One could also just place this meta string anywhere (also multiple times), possibly even user-generated content or so… (though unlikely for the types of websites we talk about…)

Especially if it then replaces the strings matched by this here for verification… 🤔

I first split on and take only the first part. This ensures I just get the head. I then regex for what I need. Any user content will not be in the head.

I could pass it to DOM parser, but I though this was easier to handle and still safe

Users can affect the head, especially with the prevalence of "og:" tags. Additionally, this parsing is quite strict so it'll break if someone puts the content before the name of any other variation.

rugk · 2018-12-20T23:24:22Z

src/background.js

+    // Chain promises returning when the first one is true
+    if (promise) {
+      last = last.then(verified => {
+        if (verified)


Not that sure what you do here, but possible Promise.race works here, too?

promise.race only returns one value, the first. Promise.all only returns when all the values are completed. This code goes through the signatures one at a time, from first to last and returns true when one of them returns true. Since verification is CPU and not IO, I don't know how much parallelism we actually get.

Updated the code to hopefully be cleaner

Code is cleaner and now returns on first valid signature

tasn · 2018-12-24T09:31:00Z

Thanks a lot for this! It's very much needed, and I can't wait to review and merge it. However, I have a few comments and a few things I need to think about before reviewing.

Comments:

Could you please split the commits to be small and have small contained logical changes? It's very hard to review such big commits that touch many areas. Not only for me, but also for future reviewers/independent auditors.
I think that using data attributes rather embedding a format in the "content" attribute is cleaner, so:

<meta name="signature" content="
    type=pgp,
    version=1.0.0,
    allowedMethods=filteredResponseData,
    signature=%%%SIGNED_PAGES_PGP_SIGNATURE%%%">

Would become:

<meta name="signed-pages" content="%%%SIGNED_PAGES_PGP_SIGNATURE%%%"
     data-type=pgp
     data-version=1.0.0
     data-allowed-methods=filteredResponseData>

I don't think we need to use semver in the data-version field. I think a number should suffice. As the version should rarely be updated, won't change in in non major versions, and as a number, much easier to handle.
Allowed methods: not sure what you're trying to achieve with that, but the same signature can't cover more than one method anyway, so not sure how it helps or what it does. If it's to indicate if it's minimized, just data-minimized=true should suffice.
Minimize version - shouldn't be embedded there, because it should be part of the version stated in data-version. If we update the pinned minimize version, we should update the the main version. No need to split them, as it's part of the protocol.
4.1. I also don't think we need npm-install-version at this stage. We currently only support one version of minimized. This will only be needed at a future date when we want to sign a page using multiple versions. We can do this change then.
4.2. It should be pinned in the package.json though, rather than the minimum version constraint that's there at the moment.
While I can see where you're coming from splitting the handling to browsers that support filteredResponseData and those who don't, I'd rather not do it. The reason is that less code paths mean less bugs. Since we are already minimizing for Chrome et al, we may as well, do the same for firefox and be consistent.
The addition of the keyid parameter to page-signer: I agree with the feature, being able to support people with multiple keys, but disagree with the implementation. I think it should be either an env var, or an optional parameter. Not everyone has multiple keys (most don't?), so it's much simpler to choose the default.
The signature should also sign the placeholder (%%%SIGNED_PAGES_PGP_SIGNATURE%%%) like it does now, rather than have it redacted. Less room for error.

Any thoughts on the above? Looking to hearing from you.

P.S: obviously this drops support for the current mechanism, but that's absolutely fine. Usage is not yet that widespread, so it's better to just force users to update, than having the legacy code, that may reduce security, remain there.

tasn

See my comment

rugk · 2018-12-24T10:13:17Z

src/background.js

+      reject(error);
+    }))).then(values =>  {
+      if (values.every(x => x == false))
+        resolve(false)


Okay, this resolves promises with false. So it is only supported for promises that return boolean values…

tasn · 2019-01-05T19:11:21Z

Hey, just chasing this one, as I'm really excited to have it already. :)
Looking forward to hearing your thoughts.

tasn · 2019-01-27T10:49:56Z

Hey @steamraven, how are you?
Do you plan on working on this, or should I take over and make the aforementioned changes myself?

rugk reviewed Dec 20, 2018

View reviewed changes

Updated promise code in signature validation.

d8ce253

Code is cleaner and now returns on first valid signature

tasn requested changes Dec 24, 2018

View reviewed changes

rugk reviewed Dec 24, 2018

View reviewed changes

tasn mentioned this pull request Apr 12, 2019

Thoughts on "HTML Signing Profile"? #20

Closed

tasn mentioned this pull request Jun 13, 2019

Support for DNS TXT record with master signing key for domain #22

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes the format of the signature to allow for later extensions. #18

Changes the format of the signature to allow for later extensions. #18

steamraven commented Dec 20, 2018

rugk Dec 20, 2018

steamraven Dec 21, 2018

tasn Dec 24, 2018

rugk Dec 20, 2018

steamraven Dec 21, 2018

steamraven Dec 24, 2018

tasn commented Dec 24, 2018

tasn left a comment

rugk Dec 24, 2018

tasn commented Jan 5, 2019

tasn commented Jan 27, 2019

Changes the format of the signature to allow for later extensions. #18

Are you sure you want to change the base?

Changes the format of the signature to allow for later extensions. #18

Conversation

steamraven commented Dec 20, 2018

rugk Dec 20, 2018

Choose a reason for hiding this comment

steamraven Dec 21, 2018

Choose a reason for hiding this comment

tasn Dec 24, 2018

Choose a reason for hiding this comment

rugk Dec 20, 2018

Choose a reason for hiding this comment

steamraven Dec 21, 2018

Choose a reason for hiding this comment

steamraven Dec 24, 2018

Choose a reason for hiding this comment

tasn commented Dec 24, 2018

tasn left a comment

Choose a reason for hiding this comment

rugk Dec 24, 2018

Choose a reason for hiding this comment

tasn commented Jan 5, 2019

tasn commented Jan 27, 2019