ooni / probe

OONI Probe network measurement tool for detecting internet censorship
https://ooni.org/install
BSD 3-Clause "New" or "Revised" License
754 stars 142 forks source link

HTTP HHFM test report doesn't match spec #1586

Open joelanders opened 7 years ago

joelanders commented 7 years ago
    self.report['tampering'] = { 
        'total': False,
        'request_line_capitalization': False, // checks equality, not just capitalization
        'header_name_capitalization': False, // never set away from False
        'header_field_value': False, // ditto ^
        'header_field_number': False // ditto ^
    }

[...]

    request_headers = TrueHeaders(self.request_headers)
    diff = request_headers.getDiff(TrueHeaders(response_headers_dict),
                                   ignore=['Connection'])
    if diff:
        self.report['tampering']['header_field_name'] = True
    else:
        self.report['tampering']['header_field_name'] = False
    self.report['tampering']['header_name_diff'] = list(diff)

header_field_name isn't explained in the spec (it's in the example, though). Since it's True iff header_name_diff is nonempty, so I'd call it redundant.

Not sure if the fix is to add these checks to the client implementations (already submitted reports would still need to be fixed?). Seems easier to do this analysis on the backend.*

[*] says the person writing client code :)

hellais commented 7 years ago

Yes I can confirm this is in fact a bug with ooniprobe and possibly also a bug in ooni-spec.

I would suggest we do the following:

  1. Implement in MK a test that matches up with the keys that are properly set and does not set the keys that are not actually used (header_name_capitalization, header_field_value, header_field_number)

  2. Remove the keys that have no purpose from ooniprobe and update the spec to mention this and add descriptions of the other keys.

While I agree that it's best to have this analysis done in the backend (and for long term analysis that is indeed what we should be doing), one of the strong things of ooniprobe is that it gives immediate feedback to the user of what is happening so some useful analysis should be in the client too.

I do agree that in the future maybe this should be done while communicating to the collector, that change would be a major architectural overhaul that we don't want to embark on at the moment.

Does this make sense?

hellais commented 4 years ago

@bassosimone moved this into probe-engine. Probably also related to: https://github.com/ooni/probe/issues/477.