Opened 7 years ago

Closed 5 years ago

#6320 closed enhancement (fixed)

Implement fingerprint hash based search (bridge support)

Reported by: hellais Owned by: phw
Priority: High Milestone:
Component: Metrics/Relay Search Version:
Severity: Keywords:
Cc: me@…, me@…, phw, karsten Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

This applies both to relays and bridges. It is obviously very important for bridges since we don't want to disclose the bridge FP.

We need to be able to search bridges also by fingerprint hashes. This feature has been implemented in onionoo in #5368.

We basically need to take the plaintext fingerprint that is input inside of the search box and client side do a SHA-1 hash of the input text.

If the user inputs a fingerprint hash we should hash that again.

To understand if you are searching for a fingerprint or a hash we must check if the input search text is more than 19 chars (maximum nick size), exactly 40 and is only hex digits.

Child Tickets

Change History (37)

comment:1 Changed 7 years ago by karsten

Note that hashing a fingerprint here means convert hex to binary, apply SHA1, convert back to hex. For example, here's how you'd hash gabelmoo's fingerprint in Python:

>>> from binascii import *
>>> from hashlib import sha1
>>> sha1(a2b_hex("f2044413dac2e02e3d6bcf4735a19bca1de97281")).hexdigest()
'16ef359c2fbf50fc08cf9a95717be3060575b67e'

comment:2 Changed 7 years ago by karsten

Priority: normalmajor

arma created #7877 today which asks for what this ticket is about. Closed that ticket as a duplicate. Raising priority of this ticket to major.

comment:3 Changed 6 years ago by f3ndot

Cc: me@… added
Status: newneeds_review

Using crypto-js I implemented a solution. Not sure how elegant it is nor if the code organization is up to snuff, but I've written a helper that'll obtain a hash of anything that matches 40 hex chars (so an existing hash or a fingerprint). The helper is used right before any remote call to Onionoo is made.

You can see the code here:
https://github.com/f3ndot/torproject-atlas/compare/issue-6320-hash-based-search

The design is abstracted so the user doesn't see the hashed fingerprint anywhere locally/Atlas side, only when the call is made out to Onionoo.

Last edited 6 years ago by f3ndot (previous) (diff)

comment:4 Changed 6 years ago by karsten

Cc: me@… added

Neat, thanks for working on this!

rndm, would you be able to review this branch? (rndm is the author of Globe which already has a similar feature.)

comment:5 Changed 6 years ago by rndm

The code looks fine, but there is a small bug that produces an error on some page loads on a local atlas version.

f3ndot said he is going to fix that soon.

If you wan't I could review it again once that is fixed.

comment:6 Changed 6 years ago by f3ndot

For clarification, there's a race condition for the declaration of the helper that uses CryptoJS and the code responsible for including it in the source. I'll have to mess about with how the app requires JS the crypto lib as I've clearly stuck 'em in the wrong place.

comment:7 Changed 6 years ago by f3ndot

Alright, fixed the bug. Commit 06d340d8331b0cb130ead5459948a7360efb009e moves the library loading before the helper declaration.

Again you can view the feature branch here:
https://github.com/f3ndot/torproject-atlas/compare/issue-6320-hash-based-search

comment:8 Changed 6 years ago by phw

Thanks for working on this; it looks good so far! There's a second part, though. So far, Atlas only cares about 'relay' fields in onionoo data. It needs to be extended to also understand 'bridge' fields and display them correctly. Among other things, that might include breaking up templates/details/main.html into a separate bridge.html and relay.html. Do you want to tackle this as well?

Ultimately, it would be great if this feature would be similar to what Globe does. Here are two examples with a relay and a bridge:
http://globe.rndm.de/#/search/query=C96977F9573247CAF33444E916089341AF2DC500
http://globe.rndm.de/#/search/query=4F46476B9B2253FBD3E805D6AA6FDFE0A51FA5DF

comment:9 Changed 6 years ago by phw

Cc: phw added
Owner: changed from hellais to phw
Status: needs_reviewassigned

comment:10 Changed 6 years ago by f3ndot

I'll tackle it as well, but delivery may take some time-- I'm presently quite full-up with obligations.

People are welcome to contribute or take over if I lag too much.

Last edited 6 years ago by f3ndot (previous) (diff)

comment:11 Changed 5 years ago by lumag

I have modified Atlas to display information about bridges and to search them using a hashed fingerprint or a nickname. The patch is being cleaned up right now. I have the following questions:

  1. Is it ok to just insert several if (bridge) conditions into the templates/details/main.html or it is better to split that into several templates?
  2. Is it neccessary to add searching by unhashed fingerprint? It seems that Globe does that, but I just want to be sure.

comment:12 Changed 5 years ago by lumag

ok, I'll use jsSHA as a SHA-1 engine to enabled searching by non-hashed fingerprints. The first question still holds.

comment:13 in reply to:  11 ; Changed 5 years ago by phw

Replying to lumag:

I have modified Atlas to display information about bridges and to search them using a hashed fingerprint or a nickname. The patch is being cleaned up right now.

Thanks for working on this!

  1. Is it ok to just insert several if (bridge) conditions into the templates/details/main.html or it is better to split that into several templates?

I think it depends on how many if statements we are talking about. I think if it's more than just a small handful, IMHO it would be cleaner to divide main.html into relay.html and bridge.html.

  1. Is it neccessary to add searching by unhashed fingerprint? It seems that Globe does that, but I just want to be sure.

I think it would be a neat usability feature to not require bridge operators to hash their fingerprint before searching for it.

comment:14 in reply to:  13 ; Changed 5 years ago by lumag

Replying to phw:

I think it depends on how many if statements we are talking about. I think if it's more than just a small handful, IMHO it would be cleaner to divide main.html into relay.html and bridge.html.

Hmm. I'm not a JS/Underscore expert. How should I include a template from another template?

comment:15 in reply to:  14 Changed 5 years ago by phw

Replying to lumag:

Replying to phw:

I think it depends on how many if statements we are talking about. I think if it's more than just a small handful, IMHO it would be cleaner to divide main.html into relay.html and bridge.html.

Hmm. I'm not a JS/Underscore expert. How should I include a template from another template?

Including? I was just referencing your idea of splitting main.html into several templates. My JavaScript knowledge is quite limited too which is the reason why I just take care of minor bug fixes for Atlas.

comment:16 Changed 5 years ago by lumag

ok, done division. pending jsSHA and few more testing.

Changed 5 years ago by lumag

comment:17 Changed 5 years ago by lumag

phw, I have attached four patches necessary to make Atlas work with Tor bridges. If you prefer, I can push a tree to GitHub.

comment:18 in reply to:  17 ; Changed 5 years ago by phw

Status: assignedneeds_review

phw, I have attached four patches necessary to make Atlas work with Tor bridges. If you prefer, I can push a tree to GitHub.

Great work, thanks a lot! Here's some feedback:

  • In the file "templates/details/bridge.html", there are several occurrences of the string "relay" which should be replaced with "bridge".
  • In the file "templates/details/error.html", we could now say "No Tor relays or bridges matched your query :(".
  • It might be useful to have a tooltip over the "OR Addresses" field on the "details" page which says that this is just a fake address and the bridge's real address is not revealed. Otherwise, we might get some confused users.
  • On the search result page, it would be great if we could distinguish relays from bridges. Perhaps by having another columns which says either "relay" or "bridge"?
  • if (fp.match(/[a-f0-9]{40}/i) != null) -- maybe we should match for upper case characters too?
  • FYI, https://globe.torproject.org also has bridge search and you might find its UI/implementation useful.

Apart from the above, I'm not very familiar with Atlas' code base but maybe we can get hellais or karsten to have a look at the patch? I will use your patch over the next couple of days and get back to you if I stumble across something else.

comment:19 in reply to:  18 Changed 5 years ago by lumag

Replying to phw:

Great work, thanks a lot! Here's some feedback:

Thank you, I will try to update patches in a few days.

  • It might be useful to have a tooltip over the "OR Addresses" field on the "details" page which says that this is just a fake address and the bridge's real address is not revealed. Otherwise, we might get some confused users.

Globe just outputs 'IPv4' or 'IPv6' instead of fake addresses. Probably I can also make Atlas to do so.

  • On the search result page, it would be great if we could distinguish relays from bridges. Perhaps by having another columns which says either "relay" or "bridge"?

What about just 'bridge' flag? I find switching between Relays and Bridges on Globe search page a bit misleading, thus I wanted to behave in a different way.

  • if (fp.match(/[a-f0-9]{40}/i) != null) -- maybe we should match for upper case characters too?

I believe /i makes it ignore case.

Yes, I know :)

Changed 5 years ago by lumag

Changed 5 years ago by lumag

comment:20 Changed 5 years ago by lumag

Updated the patchset.
Changes from previous version:

  1. Fixed bridge.html & error.html templates
  2. Replaced bogus IPv4/IPv6 addresses with "IPv4"/"IPv6" strings (as Globe does)
  3. Added a "Bridge" flag to all bridges (to enhance search view)
  4. Added clients graph.

comment:21 Changed 5 years ago by lumag

Any progress reviewing patches?

comment:22 Changed 5 years ago by phw

Cc: karsten added

Sorry for the delay. I hope to be able to have a look at it in a couple of hours. I'm also adding karsten to the CC list and wonder what he thinks.

comment:23 in reply to:  22 ; Changed 5 years ago by karsten

Replying to phw:

Sorry for the delay. I hope to be able to have a look at it in a couple of hours. I'm also adding karsten to the CC list and wonder what he thinks.

Happy to look at and comment on a modified Atlas, but I cannot code-review patches. Can you make the modified Atlas available somewhere? Or do you have a Git branch with the most recent patch series?

comment:24 in reply to:  23 ; Changed 5 years ago by lumag

Replying to karsten:

Happy to look at and comment on a modified Atlas, but I cannot code-review patches. Can you make the modified Atlas available somewhere? Or do you have a Git branch with the most recent patch series?

I have pushed my patches to GitHub. Also you can access my modified version of Atlas.

comment:25 in reply to:  24 ; Changed 5 years ago by karsten

Replying to lumag:

I have pushed my patches to GitHub. Also you can access my modified version of Atlas.

I looked at your modified version (but not at the code). Looks great! Two remarks:

  • The "Bridge" flag is probably misleading. Flags are assigned by the directory authorities or the bridge authorities, and adding a custom flag only to distinguish relays from bridges may confuse users. Can you instead add a new column for "Type" and put in "relay" or "bridge"?
  • The y axis of the clients graph uses a percent scale, which is not correct.

Thanks for working on this ticket!

comment:26 in reply to:  25 ; Changed 5 years ago by lumag

Replying to karsten:

Replying to lumag:

I have pushed my patches to GitHub. Also you can access my modified version of Atlas.

I looked at your modified version (but not at the code). Looks great! Two remarks:

Fixed both remarks (both in the repo and in the gitio site pages).

comment:27 in reply to:  26 Changed 5 years ago by phw

Replying to lumag:

Replying to karsten:

Replying to lumag:

I have pushed my patches to GitHub. Also you can access my modified version of Atlas.

I looked at your modified version (but not at the code). Looks great! Two remarks:

Fixed both remarks (both in the repo and in the gitio site pages).

Thanks! Looks good to me. I successfully merged the changes to the following branch in my Atlas user repository: https://gitweb.torproject.org/user/phw/atlas.git/shortlog/refs/heads/bug_6320-0

If karsten is also OK with the changes, we can merge it to master and deploy.

comment:28 Changed 5 years ago by karsten

Looks good to me, too! Philipp, want to merge to master and deploy, or shall I do that?

comment:29 Changed 5 years ago by phw

Merged and deployed. Thanks, everyone!

comment:30 Changed 5 years ago by phw

Resolution: fixed
Status: needs_reviewclosed
Note: See TracTickets for help on using tickets.