Opened 3 years ago

Closed 3 years ago

#21195 closed defect (fixed)

wrong exit-list annotation

Reported by: iwakeh Owned by: iwakeh
Priority: High Milestone: CollecTor 1.1.2
Component: Metrics/CollecTor Version:
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Reported and analyzed by Wojtek Porczyk, see metrics-team ml.

Next steps:

  • (if possible) create test to prevent similar in future
  • apply patch
  • prepare data correction (script and documentation)
  • test script on mirror
  • hotfix release 1.1.2

Child Tickets

Change History (14)

comment:1 Changed 3 years ago by iwakeh

Should a patch be applied immediately on mirror or main to have one source of recent correct data?

comment:2 Changed 3 years ago by iwakeh

Milestone: CollecTor 1.1.2

comment:3 Changed 3 years ago by karsten

We're not losing any data right now, just adding the wrong @type descriptor, right? If so, no need to apply the patch immediately, IMO.

comment:4 Changed 3 years ago by iwakeh

You're right. The mail by Wojtek mentions that stem fails to parse the exit-lists.
But, knowing the bug the descriptors could easily be pre-processed by the downloading party.
Thus, no immediate changes to any CollecTor mirror necessary.

comment:5 Changed 3 years ago by atagar

For what it's worth the trouble here for stem is simply that it uses the @type to infer the descriptor type. If they specify a 'descriptor_type' argument when they parse the descriptors stem will ignore the @type field and should be happy with these...

https://stem.torproject.org/api/descriptor/descriptor.html#stem.descriptor.__init__.parse_file

Cheers! -Damian

comment:6 Changed 3 years ago by iwakeh

Owner: changed from metrics-team to iwakeh
Status: newassigned

comment:7 Changed 3 years ago by iwakeh

Status: assignedneeds_review

My branch is ready for review.
There are three commits: one for the fix, one for a possible unclosed reader (which could be omitted), and one for the shell script.

The fix is running on mirror collector.sky-ip.org and the files there were fixed with the provided script.
Documentation is added as comment in the script.

Please test carefully.

comment:8 Changed 3 years ago by iwakeh

Regarding the test: I added a note to tickets #20543. The restructuring is a prerequisite for creating meaningful tests here.

comment:9 Changed 3 years ago by karsten

Status: needs_reviewmerge_ready

dcccaaa looks good.

7752b6d looks good.

5bc248e looks good and works, though I have two tweaks:

@@ -2,7 +2,7 @@
 #
 #  Only for upgrading from 1.1.0 or 1.1.1!!!
 #
-#  Script for correcting ext-list annotations.
+#  Script for correcting exit-list annotations.
 #  See task-21195 for details.
 #  Replaces 'torperf' with 'tordnsel' in files
 #  and archives.
@@ -36,7 +36,8 @@
     tar xf $ARC;
     fix $TEMP exit-list-$ym
     mv $ARC $ARC-old;
-    tar --remove-files -cJf exit-list-$ym.tar.xz exit-list-$ym
+    tar --remove-files -cf exit-list-$ym.tar exit-list-$ym
+    xz -9e exit-list-$ym.tar
     mv $TEMP/exit-list-$ym.tar.xz $ARC
     echo "--> $ARC is done."
 done;

The idea is to use xz -9e just like we used that option when creating original tarballs. Not that it matters much, but why not be consistent.

Okay, how do we proceed? Should I merge your commits (with the tweak) and put out 1.1.2? And would you want to announce it when it's out?

comment:10 in reply to:  9 Changed 3 years ago by iwakeh

Replying to karsten:

...

The idea is to use xz -9e just like we used that option when creating original tarballs. Not that it matters much, but why not be consistent.

All fine.

Okay, how do we proceed? Should I merge your commits (with the tweak) and put out 1.1.2? And would you want to announce it when it's out?

Sounds like a good plan!

comment:11 Changed 3 years ago by karsten

Here's a pre-release. Please take a look and let me know if that's good to go. Thanks!

comment:12 Changed 3 years ago by iwakeh

Looks fine:

  • offline building from the supplied sources resulted in identical bytecode
  • ant checks and test tasks passed
  • signed jars passed verification

I want to make the announcement after the exit-lists on collector.tpo are fixed. Could you add a comment here when that is done?

Thanks!

comment:13 Changed 3 years ago by karsten

There, you'll find the tarball and signature in the usual place, and all exit lists provided by https://collector.torproject.org/ contains the correct @type annotation. Please announce when you're ready, and please close this ticket afterwards. Thanks!

comment:14 Changed 3 years ago by iwakeh

Resolution: fixed
Status: merge_readyclosed

All finished and announced.

Closing.

Thanks!

Note: See TracTickets for help on using tickets.