Opened 2 years ago

Closed 2 years ago

#20714 closed defect (fixed)

Retain hidserv- lines in sanitized bridge extra-info descriptors

Reported by: karsten Owned by: metrics-team
Priority: High Milestone: CollecTor 1.1.1
Component: Metrics/CollecTor Version:
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

CollecTor logs recently started containing lines like this:

2016-11-18 09:09:01,897 WARN o.t.c.b.SanitizedBridgesWriter:1214 Unrecognized line 'hidserv-stats-end 2016-11-17 22:50:24 (86400 s)'. Skipping.

The reason is that we don't have code in SanitizedBridgesWriter#sanitizeAndStoreExtraInfoDescriptor() to handle hidserv- lines, and the default is to skip descriptors with unknown lines.

There's nothing particular sensitive in these lines that would reveal the bridge IP address or location. We should simply copy them over like the other statistics lines.

Patch:

diff --git a/src/main/java/org/torproject/collector/bridgedescs/SanitizedBridgesWriter.java b/src/main/java/org/torproject/collector/bridgedescs/SanitizedBridgesWriter.java
index 5e101eb..c54d83e 100644
--- a/src/main/java/org/torproject/collector/bridgedescs/SanitizedBridgesWriter.java
+++ b/src/main/java/org/torproject/collector/bridgedescs/SanitizedBridgesWriter.java
@@ -1192,7 +1192,8 @@ public class SanitizedBridgesWriter extends CollecTorMain {
             || line.startsWith("dirreq-")
             || line.startsWith("cell-")
             || line.startsWith("entry-")
-            || line.startsWith("exit-")) {
+            || line.startsWith("exit-")
+            || line.startsWith("hidserv-")) {
           scrubbed.append(line + "\n");
 
         /* When we reach the signature, we're done. Write the sanitized

Child Tickets

Change History (14)

comment:1 Changed 2 years ago by karsten

Status: newneeds_review

comment:2 Changed 2 years ago by iwakeh

That change is ok.

Currently, there are three possible lines in extra-info descriptors:

  • hidserv-stats-end: measurement ending
  • hidserv-rend-relayed-cells: number of cells seen
  • hidserv-dir-onions-seen: number of unique hidden-services

The latter two are obfuscated already and the first doesn't pose a problem.

Good, to get rid of the unnecessary warnings.

comment:3 Changed 2 years ago by karsten

Thanks for looking! Pushed to master.

comment:4 Changed 2 years ago by karsten

Resolution: fixed
Status: needs_reviewclosed

comment:5 Changed 2 years ago by karsten

Resolution: fixed
Status: closedreopened

This just came to mind: any reason not to put a patch release 1.1.1 for this one? That way I'd run a released version on the main instance, not just some branch.

If so, what else would need to go into that patch release? Properly signed .jar files. Anything else that really has to be in there?

comment:6 Changed 2 years ago by iwakeh

Milestone: CollecTor 1.1.1

A new minor release is fine and I don't see more than what you listed.
Defined mini milestone 1.1.1.

comment:7 Changed 2 years ago by karsten

Status: reopenedneeds_review

Alright, please review my task-20714 branch with release preparations including previously missing change log entries and this pre-release tarball.

comment:8 Changed 2 years ago by iwakeh

Looks good, just a few minor things:

LICENSE:

- Copyright 2010--2012 The Tor Project
+ Copyright 2010--2016 The Tor Project

Currently there is only the license for apache commons items added. Maybe, add the other contents of Onionoo's license file here too?

CHANGELOG.md

 * Minor changes
   - Add instructions and sample configuration for using nginx as HTTP
-     server rather than Apache.
+     server as an alternative to Apache.

Apache is still an option and surely the more popular one, but nginx can be used, too.

Would it be very cumbersome to make the hidserv-lines be part of a test?

comment:9 Changed 2 years ago by karsten

Thanks for looking! I made the two changes as suggested and added a test for hidserv- lines. Thanks for catching those things!

I did not, however, add contents from Onionoo's license file, which I'd rather want to do with a little more time (and a larger screen than I have currently available). Let's do that for the next (patch) release. Can you open a new ticket for that and maybe work on a patch together with hiro?

Please review my task-20714-2 branch and this second pre-release tarball.

comment:10 Changed 2 years ago by iwakeh

The license check is for ticket #20759 and all Metrics projects.

Test and other changes look fine.

I just noticed that this release also contains #20695. So, an additional line for the changelog:

   Handle json reading exception gracefully instead of stopping the module run.

Or similar.

Ready for release!

comment:11 Changed 2 years ago by karsten

Thanks for creating that ticket.

Regarding #20695, I put the following line into the change log:

   - Handle corrupt internal file used for checking references between
     descriptors by deleting and regenerating instead of escalating.

Should we add more details there? If so, what should we write?

comment:12 in reply to:  11 Changed 2 years ago by iwakeh

Replying to karsten:

...

   - Handle corrupt internal file used for checking references between
     descriptors by deleting and regenerating instead of escalating.

This is explains the change. All fine.

comment:13 Changed 2 years ago by karsten

Great! Pushed to master and uploaded here: https://dist.torproject.org/collector/1.1.1/ . Feel free to re-close when the announcement is out. Thanks!

comment:14 Changed 2 years ago by iwakeh

Resolution: fixed
Status: needs_reviewclosed

Announced here.

Thanks!

Closing.

Note: See TracTickets for help on using tickets.