Re: [Tails-dev] Please review and merge whisperback:bugfix/…

このメッセージを削除

このメッセージに返信
著者: Alan
日付:  
To: tails-dev
題目: Re: [Tails-dev] Please review and merge whisperback:bugfix/filter_more_serial_nos
Hi,

> I had a quick look to help a bit anonym.
>

Thanks.

> Alan wrote (01 Mar 2014 23:28:20 GMT) :
> > - Sanitize IPv6 addresses in WhisperBack
> > (https://labs.riseup.net/code/issues/6571)
>
> I'm not following you on the "I don't see why to use such a complex
> solution while addresses in the exemple are already expanded"
> position. The fact the provided example contains expanded IPv6
> addresses does not guarantee that "compressed" (or whatever it's
> called) IPv6 addresses never appear in logs.
>

Sure. But:

- I can't remove something that I don't have examples how it looks like;
- the proposed fix looks too complicated to me and I have no easy way
to test it.

> To clarify: I'm (almost, see below) fine with seeing your proposed
> changes merged, because it improves the current situation, but IMO
> what it does is resolve a (non-existing yet) "Sanitize expanded IPv6
> addresses in WhisperBack reports" subtask, but clearly *not* #6571
> entirely.
>

OK. Created :

- Sanitize expanded IPv6 addresses in WhisperBack
(https://labs.riseup.net/code/issues/6804) which is ready for QA
- Sanitize "compressed" IPv6 addresses in WhisperBack ;
(https://labs.riseup.net/code/issues/6805) which is not.

> > +    log_string = re.sub(r'[0-9a-fA-F]{4}(:[0-9a-fA-F]{4}){7}',
> > +                        r'[IP6 REMOVED]',
> > +                        log_string)

>
> Better make the regexp case insensitive, I think.
>

Why? The above regexp will match uppercase and lowercase.

> Also, there's "IP6" written in a few places. Is this intentional?
>

Yes. What would you prefer?

> > - Whisperback should sanitize DMI
> > (https://labs.riseup.net/code/issues/6797)
>
> I'm unsure about that one. The DMI string often contain useful info
> (e.g. the version of the BIOS), so killing it entirely because it may
> contain serial numbers does not look optimal to me.
>

I think we already have most of the information we want from dmidecode,
which gives it more selectively. I fail to see how keep what could be
useful to us while removing the serial number of the machine in this
very vendor-specific string, so I preferred to be on the safe side.

> > - Whisperback should sanitize more serial numbers
> > (https://labs.riseup.net/code/issues/6798)
>
> The regexp could certainly use some refactoring at some point,
> but that's no blocker yet.
>

Please see #6800 "Whisperback should use a generic regexp to
match serial numbers" for the next step (for 1.1 I think). I just
wanted to have a chance to include the improvements that was already in
my git to land in 0.23 and 1.0.
>
> Also, I'm glad there is a test suite, but the way it works seems
> strange to me: if I understand it correctly, it's testing that
> "[$something REMOVED]" can be found after sanitizing, but it does not
> test that the info that should be removed cannot be found anymore,
> which is what we should really test, no?


It would be better, yes. Want to create a ticket?

Thanks again for this review,

Cheers