Opened 4 years ago

Closed 4 years ago

#17115 closed defect (fixed)

Can't search by fingerprint with spaces

Reported by: strugee Owned by: phw
Priority: Medium Milestone:
Component: Metrics/Onionoo Version:
Severity: Normal Keywords:
Cc: iwakeh Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Some places, like Tor Weather, show relay fingerprints with spaces in them, like so:

710E 9E3A 0A44 3E3F D33D 2801 2980 4278 3CAD 2EAE

Unfortunately, Atlas doesn't recognize this format of fingerprint in search. However, if you remove all the spaces, everything is just dandy.

We really should make it so that this will Just Work™.

Child Tickets

Change History (11)

comment:1 Changed 4 years ago by phw

Keywords: easy added

I spent 30 minutes trying to fix this, without success. Leaving it open and tagging it as "easy," for somebody who actually knows JavaScript.

comment:2 Changed 4 years ago by karsten

Component: AtlasOnionoo

Oh, this is probably not something you can fix in Atlas, but something that we'd have to fix in Onionoo. The issue is that Onionoo interprets spaces as separators between search terms, and if you put in ten space-separated hex blocks, it looks for fingerprints/IP addresses/nicknames starting with all ten of them. I'm not yet clear how to fix this in Onionoo. I'm moving it to the Onionoo component. Please move it back to Atlas if I understood the issue completely wrong though.

comment:3 in reply to:  2 Changed 4 years ago by phw

Replying to karsten:

Oh, this is probably not something you can fix in Atlas, but something that we'd have to fix in Onionoo.

Couldn't Atlas just sanitise fingerprints before it sends them to Onionoo? After all, it should probably be conservative in what it sends anyway.

Last edited 4 years ago by phw (previous) (diff)

comment:4 Changed 4 years ago by karsten

Agreed, Atlas should sanitize fingerprints before it sends them to Onionoo. In fact, Onionoo already suggests that when it says "Complete hex-encoded fingerprints should always be hashed using SHA-1, regardless of searching for a relay or a bridge, in order to not accidentally leak non-hashed bridge fingerprints in the URL."

This case is a bit different, because it's not always possible to distinguish a 4-character hex block from other valid input like (part of) a nickname. For example, should "DEFA CB7E 7D73" be considered the beginning of fingerprint DEFACB7E7D73, or is it supposed to be a search for the (existing) relay with nickname default and fingerprint CB7E7D734E28312337DE322C1A0E4DE53578D2AE?

But I'm inclined to improve usability by allowing fingerprints with spaces even at the risk of returning false negatives for mixed searches with 4-character hex nicknames or nickname parts. After all, users shouldn't rely on nicknames anymore.

Suggestion (not implemented yet, might be implemented differently):

  • Onionoo clients, including Atlas, are advised to remove spaces between any two search terms consisting of exactly 4 hex characters. (If the result is a search term consisting of 40 hex characters, clients are advised to hash that using SHA-1.)
  • The Onionoo server performs the same operation, which would cover any clients that don't follow this advice. This is a major protocol change that needs to be announced at least 1 month before becoming effective.

What do you think? What did I overlook?

comment:5 Changed 4 years ago by karsten

Cc: iwakeh added
Keywords: easy removed
Status: newneeds_review

Wait, maybe there's a simpler way to implement this: we can extend the search parameter to support exact matching of any 4-character hex block in a space-separated fingerprint. That is, in addition to checking if a search term is the beginning of a relay's fingerprint we can check if it matches one of the ten 4 hex characters in the relay's space-separated fingerprint. This wouldn't require any input processing on client or server side.

In the example above, this would work as follows:

  • "DEFA" matches part of the relay's nickname "default",
  • "CB7E" is the beginning of the relay's fingerprint "CB7E7D734E28312337DE322C1A0E4DE53578D2AE" and exactly matches the first hex block, and
  • "7D73" exactly matches the second hex block of the relay's fingerprint.

I implemented this idea in branch task-17115 in my public repository. I have some concerns that this will put more load on the Onionoo server, but I think I'm willing to risk that. It's still a major protocol change where we should give client developers a 1-month warning. But I'd want to start running it on https://onionoo.thecthulhu.com/ even before that to gather some feedback. I think I'll deploy it there in a few days, unless there are objections. Deployment on https://onionoo.torproject.org/ would happen around November 15.

Feedback welcome!

comment:6 Changed 4 years ago by iwakeh

I cannot say if the above use-case is satisfied.

Concerning the implementation:
In case of space issues it might be useful to only use an array String[] of the
ten hex-strings. The hashset will have a size multiple the size of the string array.
For the look-up use Arrays.binarySearch, which should have a similar performance in the
size of the search set we consider here. The sorting before storing the array should be
about similar or less of the book-keeping done by the hashset.

comment:7 Changed 4 years ago by karsten

iwakeh, good suggestion. I think I implemented that. Mind taking a look at the updated branch?

comment:8 Changed 4 years ago by iwakeh

Yeah, that looks fine.

Maybe, also add a test in order to verify that fingerprintFourHexBlocks is indeed only available as sorted array.

Maybe, even adjust its name to fingerprintSortedHexBlocks or similar as reminder.

comment:9 Changed 4 years ago by karsten

Thanks for the suggestions, I added them as another squash commit.

I'll wait another day or two and then deploy this branch on the Onionoo mirror and also announce the upcoming major protocol change on tor-dev@. And I'll set "next_major_version_scheduled" to "2015-11-15" (writing this here mostly as reminder to myself).

comment:10 in reply to:  9 Changed 4 years ago by karsten

Severity: Normal
Status: needs_reviewnew

Replying to karsten:

Thanks for the suggestions, I added them as another squash commit.

I'll wait another day or two and then deploy this branch on the Onionoo mirror and also announce the upcoming major protocol change on tor-dev@. And I'll set "next_major_version_scheduled" to "2015-11-15" (writing this here mostly as reminder to myself).

Did that. I'll leave this ticket open as a reminder to squash and merge in a month from now.

comment:11 Changed 4 years ago by karsten

Resolution: fixed
Status: newclosed

Merged and deployed. Closing.

Note: See TracTickets for help on using tickets.