Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Signature verification #16

Closed
wants to merge 33 commits into from
Closed

Signature verification #16

wants to merge 33 commits into from

Conversation

somecookie
Copy link
Collaborator

This PR add the following new features:

  • The signature verification of a status-vote consensus in both JavaScript and python
  • A demo of the the signature verification in python
  • Tools to update the signing keys

somecookie and others added 30 commits October 27, 2018 17:19
The script is at examples/signature_verification.py. You can either run it
with the key at verif/my_public2048.pem and the signature at verif/my_signature.txt
(Lines 26-27 and 34-36 must be commented out since those are the lines that check the
key in the consensus corresponds to the one at /var/lib/tor/cached-certs)
or with the first key of the consensus (verif/public.pem) and the first signature
(verif/signature.txt).
The script is at examples/signature_verification.py. You can either run it
with the key at verif/my_public2048.pem and the signature at verif/my_signature.txt
(Lines 26-27 and 34-36 must be commented out since those are the lines that check the
key in the consensus corresponds to the one at /var/lib/tor/cached-certs)
or with the first key of the consensus (verif/public.pem) and the first signature
(verif/signature.txt).
We can now download the info of the authority signing keys and save the pem, modulus and exponent to a json file
We can now save the following information about the signing keys of the
authority to a JSON file:
-key in pem format
-key modulus
-key exponent
@somecookie somecookie requested a review from wouterl December 15, 2018 18:32
/**
* This function verifies the given raw consensus
*
* Note 1: TOR does not perform a full PKCS#1 v1.5 signature (RFC 2313) as mentioned in the TOR's reference.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one 👍

* RFC 2313 for more details), however the digest is not wrapped into the data structure described in the
* subsection 10.1.2. This is the reason why RSA is performed manually.
*
* Note 2: <script src="http://peterolson.github.com/BigInteger.js/BigInteger.min.js"></script> must be included in the HTML file using this function.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I understand here, implementing our own little RSA to get around Tor quirks is a good thing, however the "rooling your own" vibe together with Javascript and a third-party library may not be the best we can do.

For the descriptors, we can improve it a bit by using a "stronger" implementation (like HACL* ported in WebAssembly that we can reuse for the ntor handshakes) to validate router-sig-ed25519 whenever possible.

For the consensus, I don't see such feature, thus I recommend to directly use the native BigInt without poly-fill (just as we need to migrate away from userland to browserland cryptographic implementation).

I'm stating this comment as a reminder, for later integration in a more formalized roadmap.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I say no on verifying the router-sig-ed25519 version for now. I agree it could be a nice feature that we can add to a "roadmap" or "feature request", but I'd rather focus on getting this code in for now. We can always change it later.

@Native BigInt: this does not work, and will not work for some time to come. Currently only Chrome supports native BigInt. Since this is only a few lines of code, I would again prefer just to merge this, and then once the native code lands, we can see about dealing with it then.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for both points

About router-sig-ed25519, I've opened #18 to document this request.

I'm sorry, I misread documentation about BigInt – and you're right, we do need a polyfill. I've opened #17 to document this request and I'm favorable to merging while relying on BigInteger.js.

* modulus: modulus //the modulus of the key as a string
* exponent: exponent //the exponent of the key as a string
* }
* @param {Number} minimal the minimal percentage of the signatures that must be verified in order to verify the consensus
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default (safe) value needs to be documented, such as a warning (enclosed within <pre> tags for example) about the potential misuse of this parameter (making explicit recommendations on the API usage).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point @plcp! @somecookie could you explicitly add the default value to the documentation? I think stem sets it to 0.5 as well right?

}

return padded_hash.substring(sep_index + 2)
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing newline

@plcp
Copy link
Collaborator

plcp commented Dec 15, 2018

Nice work again! This need serious review, such as you other two PRs… hopefully I may be able to get into it seriously as I got some vacations for the end of the year.

@plcp
Copy link
Collaborator

plcp commented Dec 15, 2018

Note: instead of including dumped .json in js-client/demo or tools, you may prefer including documentation to generate these where they are used.

@plcp
Copy link
Collaborator

plcp commented Dec 17, 2018

It could be nice to integrate BigInteger.js in the bundled distribution at some point as it'll become a core functionality of lightnion – however, this may not need be a requirement today in order to focus on merging this.

@somecookie
Copy link
Collaborator Author

@wouterl this PR has also the same issue (a is undefined) as the one I showed you for the path selection

@laurent-girod
Copy link
Collaborator

The branch was merged into master when an other branch was merged.

@laurent-girod laurent-girod deleted the signatureVerification branch January 20, 2020 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants