Opened 23 months ago

Closed 7 weeks ago

Last modified 6 weeks ago

#16861 closed enhancement (implemented)

Pad Tor connections to collapse netflow records

Reported by: mikeperry Owned by: mikeperry
Priority: High Milestone: Tor: 0.3.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 028-triage, 028-triaged, pre028-patch, 201511-deferred, 201512-deferred, tor-guard, TorCoreTeam-postponed-201604, nickm-deferred-20160905, review-group-9, nickm-deferred-20161005
Cc: andrea, karsten, nickm, yawning, arma, mcs, kernelcorn, isis Actual Points:
Parent ID: Points: 6
Reviewer: nickm Sponsor:

Description

The collection of traffic statistics from routers is quite common. Recently, there was a minor scandal when a University network administrator upstream of UtahStateExits (and UtahStateMeekBridge) posted that they had collected over 360G of netflow records to boingboing:
https://lists.torproject.org/pipermail/tor-relays/2015-August/007575.html

Unfortunately, the comment has since disappeared, but the tor-relays archives preserve it.

This interested me, so I asked some questions about the defaults and record resolution, and did some additional searching. It turns out that Cisco IOS routers have an "inactive flow timeout" that by default is 15 seconds, and it can't be set lower than 10 seconds. What this timeout does is cause the router to emit a new netflow "record" for a connection that is idle for that long, even if it stays open. Several other routers have similar settings. The Fortinet default is also 15 seconds for this. For Juniper, it is also 30 seconds (but Juniper routers can set it as low as 4 seconds).

With this information, I decided to write a patch that sends padding on a client's Tor connection bidirectionally at a random interval that we can control from the consensus, with a default of 4s-14s. It only sends padding if the connection is idle. It does not pad connections that are used only for tunneled directory traffic.

It also gives us the ability to control how long we keep said connections open. Since the default netflow settings for Cisco also generate a record for active flows after 30 minutes, it doesn't make a whole lot of sense to pad beyond that point.

This should mean that the total overhead for this defense is very low, especially since we have recently moved to only one guard. Well under 50 bytes/second for at most 30 minutes.

I still have a few questions, though, which is why I put so many people in Cc to this ticket. I will put my questions in the first comment.

Child Tickets

TicketTypeStatusOwnerSummary
#17199enhancementclosedmikeperryImplement non-adaptive padding negotiation primitives
#17592defectclosedmikeperryClean up connection timeout logic
#17604defectclosedmikeperryTry to use only one canonical connection

Change History (85)

comment:1 Changed 23 months ago by mikeperry

  • Status changed from new to needs_review

Ok, the squashed version of my branch lives here:
https://gitweb.torproject.org/mikeperry/tor.git/commit/?h=netflow_padding-squashed

Current head is 0c29e83ede329728d4b606a9a2af1a858b517880. My plan is add fixup commits there and then squash again before merge.

I've tested it on Chutney testing networks, and appears to be behaving fine, at least based on loglines.

My questions are highlighted in the patch with "XXX:" comments, but here is also a brief summary.

Questions for Karsten/Roger:

  1. Are the rephist.c stats enough information to graph the overhead for this and other padding defenses? Note that I only output exactly 24 hours worth of data, and no history.
  2. Do you want other stats, such as the average lifespan of client OR connections, in case we want to tune that aspect of the defense, too?

Questions for Athena/Nickm:

  1. I had to create a high-res version of channel_t::timestamp_active (channel_t::timestamp_active_highres). Am I setting it in the right places to correspond to when packets are actually being sent/received on the channel, or should I set it elsewhere?
  2. Should I completely replace timestamp_active with my version? I had to remove a timestamp in channel_timestamp_drained() though, because that definitely could get called when no packets were actually sent.
  3. In Chutney testing networks, I sometimes saw circuits manage to open without a current channel. This is marked with XXX in circuit_send_next_onion_skin(). Is this a bug?
  4. I also had to create a channel_t::used_for_nondir_circs member to indicate that a channel was being used for something other than tunneled dirconns. Did I set this one in the right place, or is there a better place?

Yawning, you're on the Cc to tell me how much my C sucks (and also answer any other questions above :).

comment:2 Changed 23 months ago by nickm

What's the target release on this? We're 11 days from freeze, so I'm inclined to say "0.2.8" there. (Will have a closer look rsn)

comment:3 Changed 23 months ago by mikeperry

The patch is not terribly complicated, and can be tuned/disabled via consensus params. I was aiming for 0.2.7, unless we suspect there will be need for major changes/overhaul.

comment:4 Changed 23 months ago by mikeperry

  • Cc asn added

Oh, we may also want to tweak the Wgx weights in the consensus for this, depending on the amount of padding overhead that we notice, since it will mean more traffic to Guard-flagged nodes but not other nodes. I'm not sure where asn is at with respect to updating those weights for the switch to 1 guard node. Are those changes also on track for 0.2.7, or will they slip to 0.2.8?

FWIW, I think we'll want to update these Wgx weights once more, because I think we should be switching from 3 directory guards to 2-hop tunneled dir cons through the main guard. So that is also something to consider with respect to timelines for these weight updates and this patch.

Last edited 23 months ago by mikeperry (previous) (diff)

comment:5 follow-ups: Changed 23 months ago by nickm

  • Cc asn removed

Preliminary review, attempts to answer questions, etc. (Can't promise anything with an 0.2.7 merge yet, got to see what other folks think of the code and design. The timeframe is very tight. :)

Preliminary review, high-level requirements stuff. This doesn't all need to be on Mike, but it *does* all need to get done.

  • Needs-proposal. There should be one in enough detail (and there may *already* be one in enough detail!) that somebody else could make code that works the same. I bet that this would be a nice short proposal that wouldn't take too long. But it needs a spec-level proposal all the same. The proposal also needs to get review and attention.
  • All the functions and structures need documentation.
  • All new stuff in options needs documentation in doc/tor.1.txt.in
  • Needs a changes file.
  • Needs tests.

Nitty-gritty code stuff:

  • In C, functions that take no arguments are declared int foo(void), not int foo().
  • Why does get_rep_hist-padding_count_lines return NULL when read or write count is 0? Should that be an && ?
  • You can't format uint64_t as %ld. Use the magic U64_FORMAT and U64_PRINTF_ARG macros instead.
  • I'd suggest "has_been_used_for_nondir_circs" instead of "used_for_nondir_circs". The latter could be confused to mean "should only be used for...".
  • connection_run_housekeeping is already huge; it would be great if we could extract as much as possible of the new code into a function.
  • Looks like there's a memory leak on pad_event and conn_id_ptr in launch_netflow_pending_callback.
  • "Scheduled a netflow padding cell, but connection already closed." -- this probably shouldn't be notice; I bet it will trigger often.
  • Are you casting away the const on chan in send_netflow_padding? That's a bit scary to me.
  • DFLT_NETFLOW_INACTIVE_KEEPALIVE_MAX should probably be a function so it's clear that something is happening inside it.

Medium-level design stuff:

  • How badly does this fail when we don't have monotonic time?
  • gettimeofday() is basically free on Linux, but it's more expensive elsewhere. We need to figure out how expensive it is and actually figure out whether (say) cached_
  • Libevent doesn't like it so much when we have tens of thousands of timers added in random order. It's O(lg n) to add or remove one, and IIRC the algorithm starts to get sad around that point. We'd better make sure this doesn't matter.
  • I think connection_get_by_global_id() does a linear search. I bet that won't be affordable.

Overall comments, first impressions:

  • I dig the idea of tricking netflow hardware. A pox upon them!
  • I wonder if we can abstract this code so that the logic of when to generate packets is separated a bit more from the logic that sends them, so we can drop in other algorithms more modularly in the future.

comment:6 follow-ups: Changed 23 months ago by arma

Nick asked me to opine on the urgency of this patch. I haven't looked at the design or patch in detail yet. Here's a slightly-cleaned-up paste of my answer to him.

Big picture answer: yes, I think we should experiment with padding approaches, with the goal of stymying some of the potential traffic analysis attacks out there -- website fingerprinting, end-to-end correlation, and the things in between. Padding between the guard and the client is especially appealing because a) it looks like it can provide pretty good mileage, and also b) I expect that we'd have an easier time raising more capacity at guards (compared to exits) if we publicize the reason why we need it.

I think this is a huge research area where we need to get the whole PETS community thinking about it. We partly contributed to some potential misunderstandings about the efficacy of end-to-end correlation attacks at scale, by saying "Assume the correlation attack works perfectly and instantaneously, I don't know if it does, but it might" and having that turn into "Everybody knows the correlation attack works perfectly ad instantaneously".

I've been envisioning even like a grand challenge: "Hey everybody, here are five attacks, they sure seem hard to resolve, especially all at once, but let's think about ways to increase the false positives at scale." For some of them even a small bump in false positive rate would be huge in practice. It would be neat to get two different designs and then have people analyze the heck out of them. Ideally even more than two.

I think picking the first one Mike ran across is a fine thing to deploy in the mean time, but we shouldn't rush to deploy it, or put too much stock in its being right.

For a little while I was thinking "man, this is just going to cause some research group to write a paper about how we're morons because look, this padding thing doesn't help here and here." But then I realized, that's great! Whatever it takes to get them to write the paper.

comment:7 Changed 23 months ago by mikeperry

I am in the process of addressing Nick's comments. I am also relocating all of this code to channelpadding.c, and refactoring it to try to use the channel abstraction layer instead of connections (where possible).

comment:8 in reply to: ↑ 6 Changed 23 months ago by mikeperry

Roger - I'm in complete agreement with your statements, save for hesitation on moving quickly. This is a narrow case where it's really easy to do what we want from a technical POV. So long as we ensure that this patch is doing what we intend (which is just to send at least one cell on a connection every 15s), then I think getting this patch out there faster will move everything you said forward quicker - mobilizing the research community, making people excited to run more fast guard nodes, etc. And if we find out it isn't doing what we intend, or causing too much load, we turn it off from the consensus. Release early, release often! Move fast and break stuff (yeah I just said that). Etc etc.

Some third-rate researchers will be sure to deliberately misinterpret this defense so they can get a cheap publication, but I also suspect that some good researchers will tell us what else we could do against the more complicated, higher-resolution cases than default-configuration netflow records.

I also believe that future defenses will be completely orthogonal to the netflow defense code and can be completely ignorant of it in their implementation and still remain optimal, since if they decide to send padding for any reason, then the netflow defense won't (since the netflow defense only sends padding if the connection is idle).

comment:9 in reply to: ↑ 6 Changed 23 months ago by arma

Replying to arma:

I think picking the first one Mike ran across is a fine thing to deploy in the mean time, but we shouldn't rush to deploy it, or put too much stock in its being right.

It is for these reasons that if Nick wants to delay this feature until 0.2.8 (and it looks like he does), I will support him on it. This isn't an "oh, we just do this simple thing and then everybody is clearly safer" situation, and these sorts of "emergent behavior" designs often have surprising side effects (or heck, effects) as they get rolled out more widely.

This is not to say that I don't think we should do the feature. We should. We just shouldn't screw up everything else in 0.2.7 that's been getting good testing for months now.

With all due haste!

comment:10 Changed 23 months ago by mikeperry

Ok, well I am going to withhold arguing about risking a delay of 0.2.7 or otherwise impacting it in an unrecoverable way, and instead just try to get this done. We can make the call on Sept 1st if I've accomplished that. It seems premature to make that call now.

I'd also prefer it if the people who I've asked questions of not assume "Well, I guess I can ignore Mike's questions now", because that *will* impact the quality of what I manage to get done by Sept 1st.

comment:11 Changed 23 months ago by arma

Sounds great! (Everybody please avoid the trap where you decide you can ignore the topic because it might not be an 0.2.7 thing.)

comment:12 in reply to: ↑ 6 Changed 23 months ago by yawning

Note: Still trying to boot my brain up.

Replying to arma:

I've been envisioning even like a grand challenge: "Hey everybody, here are five attacks, they sure seem hard to resolve, especially all at once, but let's think about ways to increase the false positives at scale." For some of them even a small bump in false positive rate would be huge in practice. It would be neat to get two different designs and then have people analyze the heck out of them. Ideally even more than two.

Another question we should pose to the research community is "we know this netflow thing is done, we know that a certain amount of metrics tracking is required as part of the administering a large scale network, is there a way to get an acceptable amount of information in a privacy preserving manner?".

comment:13 in reply to: ↑ 5 ; follow-up: Changed 23 months ago by mikeperry

Ok, I just pushed fixes for Nick's review to the neflow_padding-squashed branch, and have a new squashed patch at https://gitweb.torproject.org/mikeperry/tor.git/commit/?h=netflow_padding-squashed2.

Everything has been refactored and reorganized into channelpadding.{c,h}, and the code is generally a lot more organized and properly abstracted to use channel_t. Channel lookup during timer invocation is now O(1), but this will only work for TLS-based channels (because connection_t has an index into the connection array that allows O(1) lookups, but channel_t does not provide O(1) lookups based on its global_identifier).

The following issues remain, which I will fix later:

  • Still needs a proposal
  • Still needs tests
  • Still needs a changes file
  • Still has lots of questions for folks listed in comment:1 and in the XXX comments in the code.

The following were non-issues as far as I can tell:

  • rep_hist_get_padding_count_lines() deliberately omits the extra-info lines if either the read *or* write cells were empty, because in either case we don't have enough info to safely publish stats.
  • I don't think I actually leaked conn_id_ptr. In any case, I refactored that code and now pass an allocated struct. I don't think I leak that one either.
  • I don't think I was casting away a const in send_netflow_padding(). That function is now called channelpadding_send_padding_cell_callback(). The cast has been replaced by proper macro usage, but there was no const there anyway, unless I missed something?
  • If the clock jumps, at worst we would have emitted a warn about the padding time being in the past. Now we only emit a notice. Should we double-check here anyway somehow?

I'm still now sure what to do about the following:

  • Am I still leaking pad_event from tor_evtimer_new in channelpadding_schedule_padding()? I cargo-culted that from dns_launch_correctness_checks() in dns.c, so if I'm leaking, that function probably is too..
  • Is there a way to test for a slow gettimeofday()? I noticed some TIME_IS_FAST ifdefs in util.c, but nothing sets that define. We *can* use time() if we need to, and it will still work fine, but we'll end up sending more padding due to truncation error in that case (which is why I added timestamp_active_highres in the first place).
  • How should we check if there are too many libevent timers scheduled? Note that the code didn't (and still doesn't) schedule a callback unless we're within 1 second of the padding timeout. It just waits for the next invocation in those cases. That should mean that even if all connections are always idle, only 1/10 of them are scheduling timer callbacks (because the function is called once per second, and the timeout range is 10 seconds wide). We can still check for to many timer callbacks anyway, and call directly in that case, but how do I do that? I've added an XXX in the code where we'd need to do this.

There are still lots of XXX's in the code for my other questions.

comment:14 in reply to: ↑ 13 Changed 23 months ago by yawning

Replying to mikeperry:

Ok, I just pushed fixes for Nick's review to the neflow_padding-squashed branch, and have a new squashed patch at https://gitweb.torproject.org/mikeperry/tor.git/commit/?h=netflow_padding-squashed2.

I'll look at this when I have a moment, which realistically will probably be sometime next week. But chiming in a bit on your comments...

I'd like to see how this interacts with the circuit scheduler since we can avoid sending padding if we have user payload to transmit instead. If you do that already, great. If not, interactions here need to be carefully considered (and the relevant optimizations made).

[snip]

I'm still now sure what to do about the following:

  • Is there a way to test for a slow gettimeofday()? I noticed some TIME_IS_FAST ifdefs in util.c, but nothing sets that define. We *can* use time() if we need to, and it will still work fine, but we'll end up sending more padding due to truncation error in that case (which is why I added timestamp_active_highres in the first place).

Systems with a slow gettimeofday() will likely have a slow time() as well, so I don't see much of a point here. The only systems I can think of that fall under this that are relays are old Linux (who cares) and some of the BSD variants (FreeBSD in certain virtualization envs in particular, may be fixed).

What matters here is "does the OS vDSO certain libc calls".

  • How should we check if there are too many libevent timers scheduled? Note that the code didn't (and still doesn't) schedule a callback unless we're within 1 second of the padding timeout. It just waits for the next invocation in those cases. That should mean that even if all connections are always idle, only 1/10 of them are scheduling timer callbacks (because the function is called once per second, and the timeout range is 10 seconds wide). We can still check for to many timer callbacks anyway, and call directly in that case, but how do I do that? I've added an XXX in the code where we'd need to do this.

If too many libevent timers are a problem, then why not use 1 libevent timer, and a doubly linked list of padding events? (Or a more sophisticated data structure if insertion is too expensive)...

comment:15 Changed 23 months ago by mcs

  • Cc mcs added

comment:16 Changed 23 months ago by mikeperry

Proposal posted to tor-dev (and also mentioned on tor-relays in the Utah thread): https://lists.torproject.org/pipermail/tor-dev/2015-August/009326.html

I also pushed some changes based on Karsten's rephist.c review to both netflow_padding-squashed and netflow_padding-squashed2. I force-pushed netflow_padding-squashed2 so that it remains a single diff, but kept the history in netflow_padding-squashed. Both branches should be identical in delta.

comment:17 Changed 23 months ago by mikeperry

Yawning: I took your comment about gettimeofday() into account, and pushed a new commit (to both squashed and squashed2) to change all the places where I need high resolution to simply use the tv_sec timeval field instead of also calling time(). That way, the new code will be no slower than the old code, at least.

However, as I said on IRC yesterday, I believe this code is independent of the circuit scheduler. Not only are we padding at the connection level (and only when there are no other packets to send), but we also need to pad on a connection long after all circuits are gone from it. I think involving circuit information is not right (in addition to being a complicated layer coordination issue) for these reasons.

Your suggestion about making a data structure to compensate for libevent's timer paucity is interesting and maybe ultimately the right plan, but right now I can't think of anything that won't still max out at 1000 timers (since I still want to preserve millisecond resolution on actual packet delivery). I also worry that this complexity may be a bit error prone and fragile. I'd be much happier just giving up, emitting a notice, and sending the packet directly from channelpadding_schedule_padding() without a timer in the case that we have more than say 50k timers (or whatever we think libevent will start breaking at), and optimizing later if that actually happens in the wild (which I still think is unlikely at our current client load and timeout values).

It may also be possible that the common case will be even slower as a result of this data structure, unless we do something really simple like make an array of 1000 smartlists that we can index into based on the current millisecond. But even that may end up slower in the common case..

comment:18 Changed 23 months ago by mikeperry

Ok, I have pushed an updated version of the tor-dev proposal (with some change history) to:
https://gitweb.torproject.org/user/mikeperry/torspec.git/tree/proposals/xxx-netflow-padding.txt?h=netflow_padding

I also pushed new code branches that add tests and fix several issues. As one commit:
https://gitweb.torproject.org/mikeperry/tor.git/commit/?h=netflow_padding-squashed2

The old netflow_padding-squashed branch still preserves history, in case Nick wants to see the delta since last review (though it is probably actually larger and harder to read than the single-commit version, due to refactoring into channelpadding.c). I will continue to preserve history in netflow_padding-squashed, to ease future reviews.

Here is a summary of the issues in the code that I'd like some input on, if possible. Each of these has one or more matching XXX comments in the patch:

  1. Still not sure if I'm setting timestamp_active_highres in the places that accurately reflect network activity
  2. Not sure how to best handle libevent timers. Can we check how many are outstanding? How? Should we remove timers in the case where traffic arrives before the timer expires (which eliminates the need for it), or will that be slower? Do we really need an auxiliary data structure?
  3. Am I actually leaking pad_event still? Do I need to free it myself in the callback?
  4. Am I setting has_been_used_for_nondir_circs in the right places? (And how/why does Chutney complete a circuit without a valid channel for it???)
  5. I still need to test this on PT bridges, to see if the is_canonical test fails for them.
  6. I added a ReducedConnectionPadding torrc option to reduce padding (for mobile users). Unfortunately, since padding is bidirectional, I don't currently have a way to fully disable padding that the server sends other than closing the OR connection earlier. With the current values, this should reduce the worst-case per-connection padding overhead from ~180KB to ~18KB (while still preventing multiple netflow records from being created for the duration of the shorter connection if the relay sends padding). Is this a problem, or a feature? The only alternative seems to be to create sub-fields of CELL_PADDING to communicate padding preferences to the relay..
  7. Do we want any more statistics (like average per-orconn padding stats) exported in extra-info?
  8. Karsten is still working with me to tune the rephist.c stat parameters for optimal information reduction.

I think all of the other comments from nickm, Yawning, and Karsten have been addressed.

comment:19 Changed 22 months ago by mikeperry

  • Status changed from needs_review to needs_revision

FYI: My questions above still stand, but the following additional issues make me think that 0.2.8.x is a better target for this:

  1. Predictive circuit building is causing otherwise unused connections to live far longer than I expected in many cases (1.25-1.5 hours). This means that the overhead is more like 500KB per connection in the worst case, and even with ReducedConnectionPadding, 330KB in the worst case. I think that much overhead for mobile is not acceptable without allowing users to opt-out (or opt to reduce their connection lifespan).
  2. This probably also means we want the ability for clients to tell relays not to pad, or pad less, in case we discover that mobile connections should still live a long time, but pad less overall.
  3. I think all of this does mean we want statistics on average ORconn lifespan (issue 7 in comment:18), as well as stats on avg per-orconn padding, as this will help us tune the defense.
Last edited 22 months ago by mikeperry (previous) (diff)

comment:20 in reply to: ↑ 5 ; follow-up: Changed 22 months ago by isis

Replying to nickm:

  • Needs-proposal. There should be one in enough detail (and there may *already* be one in enough detail!) that somebody else could make code that works the same. I bet that this would be a nice short proposal that wouldn't take too long. But it needs a spec-level proposal all the same. The proposal also needs to get review and attention.


I've merged mike's netflow_padding2 torspec branch because it includes statistical changes documenting developments in the corresponding code, and I believe that reading a short specification which actually matches its corresponding code will make life easier for reviewers. Please let me know if I erred in these assumptions.

comment:21 in reply to: ↑ 20 Changed 22 months ago by nickm

Replying to isis:

Replying to nickm:

  • Needs-proposal. There should be one in enough detail (and there may *already* be one in enough detail!) that somebody else could make code that works the same. I bet that this would be a nice short proposal that wouldn't take too long. But it needs a spec-level proposal all the same. The proposal also needs to get review and attention.


I've merged mike's netflow_padding2 torspec branch because it includes statistical changes documenting developments in the corresponding code, and I believe that reading a short specification which actually matches its corresponding code will make life easier for reviewers. Please let me know if I erred in these assumptions.

Generally we just let all proposal authors merge as many changes to their proposals as they want, as long as they haven't gone crazy or started trolling. So no, no problem with merging any changes to Mike's proposal that Mike thinks are good.

comment:22 Changed 22 months ago by kernelcorn

  • Cc kernelcorn added

I look forward to seeing this defense in Tor and will certainly be updating to whichever release we merge this into. It will probably have implications beyond ISP-level netflows anyway.

comment:23 Changed 21 months ago by nickm

  • Milestone set to Tor: 0.2.8.x-final

comment:24 Changed 21 months ago by mikeperry

  • Keywords 0.2.8.x-triage added

comment:25 Changed 21 months ago by mikeperry

  • Keywords 028-triage added; 0.2.8.x-triage removed

comment:26 Changed 21 months ago by nickm

  • Keywords 028-triaged added

comment:27 Changed 21 months ago by mikeperry

  • Points set to medium

The remaining work on this is probably medium, not counting #17199.

comment:28 Changed 20 months ago by isis

  • Cc isis added
  • Severity set to Normal

comment:29 Changed 20 months ago by nickm

  • Priority changed from Medium to High

comment:30 Changed 20 months ago by nickm

  • Keywords pre028-patch added

comment:31 Changed 20 months ago by mikeperry

  • Keywords TorCoreTeam201511 added

comment:32 Changed 19 months ago by mikeperry

  • Status changed from needs_revision to needs_review

Ok, this is finally ready! It passes all unit tests and chutney tests, I've tested it extensively myself on both chutney and my own TBB. It has been refactored a few times, and I fixed all of Nick's concerns, and almost all of my concerns from comment:18 and comment:19. I watched the log behavior quite closely, as well. Here is the commit to review:
https://gitweb.torproject.org/mikeperry/tor.git/commit/?h=netflow_padding-v4&id=8c2d53a33aff55c614ac48047dbbb0b15157b56f

The primary implementation is in channelpadding.c - that c file has 98% line coverage and 81% branch coverage via the unit tests. Not sure what chutney brings it up to.

I only have one more question: Are channel_timestamp_xmit, channel_timestamp_recv, and channel_timestamp_active the right places to signify network activity? They seem like it to me, but I am a bit lost in all of the channel to connection_or to orcon buffers to bufferevents (or not) to libevent to kernel layering and how it all shakes out in practice. The comment in channel_timestamp_active also makes it sound like it is not for actual wire activity, but it is also the only thing being called in several places where there is wire activity (such as the initial TLS conn setup). Unfortunately, it is also called in some places that don't clearly map to wire activity... Will that matter in practice? Should I make a separate activity callback just for this code?

That branch also has commits for the two child tickets of this bug (#17592 and #17604). Those are also well tested. I will set needs review on those tickets for those two commits.

comment:33 follow-up: Changed 19 months ago by teor

Code review:

Looks great, just a few questions:

I'm not sure about the comment in circuit_send_next_onion_skin:

+      /* If this is not a one-hop tunnel, the channel is being used for
+       * stuff other than non-directory traffic. That means we want to
+       * pad it.
+       */

Does it need to be updated?

And the implementation changes in circuit_receive_relay_cell:

+   * We assume we're more than one hop if either the previous hop
+   * is not a client, or if the previous hop is a client and there's
+   * a next hop. Then, circuit traffic starts at RELAY_EARLY, and

Tor2Web clients use one-hop tunnels for user traffic, and hidden servers running (Rendezvous) Single Onion Services are about to.

Does this code correctly distinguish between one-hop directory circuits and one-hop Tor2Web/(R)SOS circuits?

I think we need to remove the one-hop checks entirely, and just depend on the RELAY cells.

In rep_hist_get_predicted_ports:

+ // XXX: Change this if ReducedConnectionPadding is set.
 predicted_circs_relevance_time = get_options()->PredictedPortsRelevanceTime;

Do you still need to fix this?

Regarding MIN_CELL_COUNTS_TO_PUBLISH:

After the network is mainly 0.2.8, relays which aren't using padding may become quite rare.
Do we want to set MIN_CELL_COUNTS_TO_PUBLISH to something higher than 1, to hide relays with small counts and relays with 0 counts together?

Also, do we want to include a total padding amount in the heartbeat messages?

Nitpicks:

Do we want to define MAX_LINK_PROTO_TO_LOG based on MIN_LINK_PROTO_FOR_CHANNEL_PADDING, or doesn't that make sense?

+#define MAX_LINK_PROTO_TO_LOG 5

comment:34 Changed 19 months ago by nickm

  • Keywords TorCoreTeam201512 201511-deferred added; TorCoreTeam201511 removed

Bulk-move uncompleted items to december. :/

comment:35 in reply to: ↑ 33 Changed 19 months ago by mikeperry

Replying to teor:

Code review:

Looks great, just a few questions:

I'm not sure about the comment in circuit_send_next_onion_skin:

+      /* If this is not a one-hop tunnel, the channel is being used for
+       * stuff other than non-directory traffic. That means we want to
+       * pad it.
+       */

Does it need to be updated?

No. I clarified it in a fixup, though. Basically we want to know as early as possible if we should be padding the channel or not.

And the implementation changes in circuit_receive_relay_cell:

+   * We assume we're more than one hop if either the previous hop
+   * is not a client, or if the previous hop is a client and there's
+   * a next hop. Then, circuit traffic starts at RELAY_EARLY, and

Tor2Web clients use one-hop tunnels for user traffic, and hidden servers running (Rendezvous) Single Onion Services are about to.

Does this code correctly distinguish between one-hop directory circuits and one-hop Tor2Web/(R)SOS circuits?

It does not. The code assumes that 1 hop means "don't care about traffic analysis" and 3 hops means "do care". And remember that padding is a property of the channel, so if either Tor2Web or (R)SOS happen to build 3 hop circuits on a channel, the channel will become padded.

As we discussed I think basically we don't want to pad these, though, primarily because it will mean a lot of overhead (#17857).

I think we need to remove the one-hop checks entirely, and just depend on the RELAY cells.

We definitely don't want to pad directory connections. We also want to start padding as early as possible. For clients, circuit_send_next_onion_skin() is an earlier decision point than circuit_receive_relay_cell().

In rep_hist_get_predicted_ports:

+ // XXX: Change this if ReducedConnectionPadding is set.
 predicted_circs_relevance_time = get_options()->PredictedPortsRelevanceTime;

Do you still need to fix this?

Regarding MIN_CELL_COUNTS_TO_PUBLISH:

After the network is mainly 0.2.8, relays which aren't using padding may become quite rare.
Do we want to set MIN_CELL_COUNTS_TO_PUBLISH to something higher than 1, to hide relays with small counts and relays with 0 counts together?

This was initially higher, but Karsten actually suggested I set it at 1 when I asked him for a better value. He said the 24 hour binning+rounding was sufficient, and we should be focused on that value (which he gave some suggestions for).

Also, do we want to include a total padding amount in the heartbeat messages?

I think this may be risky, as we'd want a similar level of sanitization as the rephist data. I'm inclined to leave it out for now, and just keep an eye on the extra-info data.

Nitpicks:

Do we want to define MAX_LINK_PROTO_TO_LOG based on MIN_LINK_PROTO_FOR_CHANNEL_PADDING, or doesn't that make sense?

+#define MAX_LINK_PROTO_TO_LOG 5

I fixed this by making a single MAX_LINK_PROTO in connection_or.h.

A fixup commit is now pushed on top of the netflow_padding-v4 branch. I will also soon be pushing fixups for the other tickets, too.

comment:36 Changed 19 months ago by teor

I'm OK with these changes and explanations. The fixups all look good.

Are we going to merge this? :-)

comment:37 Changed 18 months ago by nickm

  • Keywords TorCoreTeam201601 201512-deferred added; TorCoreTeam201512 removed

Perhaps in January?

comment:38 Changed 18 months ago by nickm

I've squashed the three major commits here into a new "netflow_padding-v4_squashed" branch so I can review them one-by-one.

comment:39 follow-up: Changed 17 months ago by nickm

Reviewing the patch squashed as "8944ad2cb526f1f895d35290a86c8c546e2d4f44 : Netflow record collapsing defense".

I'll start with low-level review, looking for high-level questions as I go.

channel.c:

  • The channel_timestamp_active call to tor_gettimeofday() is a little worrisome; don't we have expensive gettimeofday() implementations in some places? Can we get away with tor_gettimeofday_cached() ? Probably that's premature optimization though.
  • When this is merged we should open a ticket to remove the redundant fields from channel_t. I see a comment in channel.h saying that we could consolodate it ; more on that below.

channel.h:

  • Looking at next_padding_time ... you need to document that 0 means "never".
  • You say that we set padding_timeout_low/high, but you never actually say what they do or mean or who checks them. Also, "0==unset" isn't documented.
  • You need to document what timestamp_active_hires actually means. Also, it's confusing to have it mean something different from "a more high-resolution version of timestamp_active".

channelpadding.h:

  • On many of these functions, more args could be const
  • The units of channelpadding_get_netflow_inactive_timeout_ms aren't documented in the function description; also, the 0 return value isn'tdocumented.
  • The documentation for channelpadding_get_netflow_inactive_timeout_ms doesn't seem right. It says that it gets the value from the consensus, but really it generates a random value for the channel based on the value from the consensus, the negotiated value (if any), and some defaults.
  • the warnings in channelpadding_update_padding_for_channel should be rate-limited, or LOG_PROTOCOL_WARN, or both.
  • Wrt the second check in channelpadding_update_padding_for_channel ... is there a check to make sure we only receive the cell as an outbound cell?
  • Trunnel idiom: You don't need to check the return values from chanelpadding_netgotiate_set*(). If any one of those fails, the encode() will fail later on.
  • In channelpadding_send_padding_cell_for_callback () -- that static cell_t makes me a little nervous. memset() is much faster than our crypto; is this a premature optimization?
  • WRT the whole channelpadding_channelinfo_* stuff:
    • When a connection is removed from the connection array, another connection gets its old index. This happens at the end of connection_remove(). In other words, a connection's index can change after it is created! Where do we handle that? (probable bug).
    • To the checks in channelpadding_channelinfo_to_channel let's add "it is an OR_CONN".
  • NM, NOTE I'm going to come back to this "timerslot" abstraction; I wonder if there's a cleaner way to do this.
  • I worry about clock jumps here; is this a use case for our monotonic timer?
  • This whole timer thing is a pretty huge amount of engineering to escape libevent's limitations when it comes to thousands of timers. I wonder if there isn't some better approach... (NM, NOTE)
  • On the other hand, sending padding via timers and callbacks is a pretty clean approach.

channeltls.c:

  • I don't think that CELL_PADDING and CELL_PADDING_NEGOTIATE are transport-specific; I think they probably apply to most channel types, yeah?

circuitbuild.c:

  • Wedging this test into circuit_send_next_onion_skin seems a bit wrong to me. Maybe we can set it earlier in circuit construction.

command.c:

  • What other ramifications are there from setting chan->is_client=0 like this?

relay.c:

  • circuit_receive_relay_cell() is already a huge function. Can we extract the usage-tracking block to a new function of its own?
    • This is_client check for the relay case there also looks a little kludgey; I wonder if we don't need a protocol-level improvement.

Meta:

  • Is there a torspec patch for merging the proposal's new behavior into tor-spec.txt?

comment:40 in reply to: ↑ 39 Changed 17 months ago by mikeperry

Replying to nickm:

Reviewing the patch squashed as "8944ad2cb526f1f895d35290a86c8c546e2d4f44 : Netflow record collapsing defense".

I'll start with low-level review, looking for high-level questions as I go.

Ok, my fixes for most of this are now in mikeperry/netflow_padding-v4_squashed in a fixup commit (721999a3e3e4bd51c2552bf03ea126b8ce7180e6).

Stuff I fixed I just removed from your comment. Inline below are any questions or comments about remaining issues.

channel.c:

  • The channel_timestamp_active call to tor_gettimeofday() is a little worrisome; don't we have expensive gettimeofday() implementations in some places? Can we get away with tor_gettimeofday_cached() ? Probably that's premature optimization though.

I asked Yawning about this a while ago, and did some reading, and I think the takeaway was that anywhere gettimeofday is slow, time() will also be slow (which we were already doing). I think I'd also rather not lose the resolution, unless tor_gettimeofday_cached() is only cached if gettimeofday is slow...

  • You need to document what timestamp_active_hires actually means. Also, it's confusing to have it mean something different from "a more high-resolution version of timestamp_active".

I improved the comment. I am not sure what to call it other than active. I could not think of another word that means "send or received actual data", which is what this means. Open to suggestions. xmit is also already taken..

channelpadding.h:

  • On many of these functions, more args could be const

Fixed. Left one as a comment that can become const after we fix the channel lookup stuff.

  • Wrt the second check in channelpadding_update_padding_for_channel ... is there a check to make sure we only receive the cell as an outbound cell?

How do you mean? These are link-level cells. I thought directionality was only for circuit-level?

  • WRT the whole channelpadding_channelinfo_* stuff:
    • When a connection is removed from the connection array, another connection gets its old index. This happens at the end of connection_remove(). In other words, a connection's index can change after it is created! Where do we handle that? (probable bug).

Yikes. Yes. In this case, we would see that the global identifiers don't match, log at info level, and not send a padding packet. We should improve this so that such that we don't expect a connection to disappear while the timer is still pending. I have not done that yet. Also I still think for safety we want to do an identifier-based lookup here, rather than throwing a pointer around. But if you end up not haveing time for that O(1) datastructure, we could possibly just to the cancel-on-free thing only and send a pointer. That probably is the sane pattern for this type of thing.

  • To the checks in channelpadding_channelinfo_to_channel let's add "it is an OR_CONN".

Isn't it enough that TO_OR_CONN that we use already has a tor_assert in it? I think if some other mystery connection gets all the way down to that type conversion, I want the tor process to stop dead. Something went horribly wrong at that point.

  • I worry about clock jumps here; is this a use case for our monotonic timer?

I remember looking into this before and I don't think clock jumps can do too much damage to us. I certainly had an XXX there about this that I removed after thinking about it. I can do so again. Would also be helpful to know what happens if the clock jumps while libevent has timers pending.

channeltls.c:

  • I don't think that CELL_PADDING and CELL_PADDING_NEGOTIATE are transport-specific; I think they probably apply to most channel types, yeah?

You mean move it to channel.c, or command.c? CELL_PADDING was already specific to channeltls.c. In fact, I don't think there is any generalized cell handling at all in channel.c. It just forwards it down to channeltls.c or command.c. I'm actually not 100% sure why some cell types are both in command.c *and* channeltls.c, or how that decision works in the channel code.

circuitbuild.c:

  • Wedging this test into circuit_send_next_onion_skin seems a bit wrong to me. Maybe we can set it earlier in circuit construction.

Suggestions?

command.c:

  • What other ramifications are there from setting chan->is_client=0 like this?

I found is_client to be an unreliable indicator of actual client status (and possibly confused about CREATE_FAST being still used), despite the comment for the field in channel_s. I guess I probably accidentally fixed some other bugs? The only other important use was to decide not to send an extend cell down the channel. Which seems fine to be more correct about.

relay.c:

  • circuit_receive_relay_cell() is already a huge function. Can we extract the usage-tracking block to a new function of its own?

Done.

  • This is_client check for the relay case there also looks a little kludgey; I wonder if we don't need a protocol-level improvement.

Not sure what you want here?

Meta:

  • Is there a torspec patch for merging the proposal's new behavior into tor-spec.txt?

Yes, I will do that as we discussed in the meeting.

comment:41 Changed 17 months ago by nickm

(Will write more soon, but wanted to point you towards fast_channel_lookup in my public repository, which adds an O(1) implementation of channel lookup.)

comment:42 Changed 17 months ago by mikeperry

Here's the spec updates: https://gitweb.torproject.org/user/mikeperry/torspec.git/commit/?h=padding_spec

Will set about making use of fast_channel_lookup and the subticket fixes tomorrow.

comment:43 Changed 17 months ago by nickm

  • Status changed from needs_review to needs_revision

comment:44 Changed 17 months ago by mikeperry

  • Status changed from needs_revision to needs_review

Ok, I have a new set of patches ready in mikeperry/netflow_padding-v4_squashed+rebased. They are rebased on top of nickm/fast_channel_lookup and are ready to be squashed. See mikeperry/netflow_padding-v4_squashed if you want to spot-check the full history. The two heads are identical in content.

For this bug, all of the issues from comment:40 should now be fixed, with the following provisos:

  1. I did notice another issue with time. After thinking about it, I think I still prefer explicit checks rather than simply using monotonic time, because if the clock jumps backwards, I don't want to sit around not sending padding forever while we wait for it to catch up with itself. I'd rather handle each case explicitly, though I admit it is kinda mind-bending either way we go. I think I did it right?
  2. I changed timestamp_active_highres to timestamp_xfer_highres. xfer seemed to capture what I want. I left it non-cached, though, due to it being equivalent in speed to time(), and I want the resolution.
  3. Am I right that directionality does not apply to link-level cells? Only circuit RELAY cells, yes?
  4. I left the CELL_PADDING and CELL_PADDING_NEGOTIATE handling in channeltls.c. Is command.c a better place for some reason?
  5. I left the test in circuit_send_next_onion_skin(). Happy for a suggestion for a better place. It seems OK to me, though, given that is where the control port decides to tell the controller about a very similar thing.
  6. I did not change the is_client stuff. If you have concrete suggestions here, happy to hear them. Still don't like purely behavioral decisions, though.

I think I'd prefer these fixup commits squashed when merged, so feel free to do that if you don't see any more major issues. Again, mikeperry/netflow_padding-v4_squashed+rebased should be all ready for squash and merge, or squash and another round of fixups. The head is 35fa80cc2d6a5cf2d8ea1f1395e1b43ffc7262ae, and the commit hash for this specific bug's fixup is 3179158574e19872c24577ab09793140aa2acc0d.

Last edited 17 months ago by mikeperry (previous) (diff)

comment:45 Changed 17 months ago by nickm

Bulk-modify: It is February 2016, and no longer possible that anything else will get done in January 2016. Time's arrow and all that.

comment:46 Changed 17 months ago by nickm

  • Keywords TorCoreTeam201602 added; TorCoreTeam201601 removed

comment:47 Changed 16 months ago by dgoulet

  • Milestone changed from Tor: 0.2.8.x-final to Tor: 0.2.9.x-final

comment:48 Changed 15 months ago by nickm

  • Points changed from medium to medium-remaining

comment:49 Changed 15 months ago by nickm

  • Keywords TorCoreTeam201602 removed

comment:50 Changed 15 months ago by nickm

  • Keywords TorCoreTeam201604 added
  • Reviewer set to nickm

comment:51 Changed 15 months ago by mikeperry

  • Keywords tor-guard added
  • Sponsor set to SponsorU-can

This is an alternate/additional Guard node improvement we can claim for SponsorU.

comment:52 Changed 14 months ago by nickm

  • Keywords TorCoreTeam-postponed-201604 TorCoreTeam201605 added; TorCoreTeam201604 removed

I will not be getting these revised and reviewed this week. I hold out hope for May. Sorry mike. Please let me know whether you want to revise them wrt my handles/timing patches, or whether I should. I'm happy either way.

comment:53 Changed 14 months ago by nickm

  • Status changed from needs_review to needs_revision

My current understanding here is that mike means to revise this branch based on other merges we're doing. Moving these to needs_revision in the meantime. Please let me know if I'm incorrect.

comment:54 Changed 14 months ago by isabela

  • Points changed from medium-remaining to 2

comment:55 Changed 13 months ago by nickm

  • Keywords TorCoreTeam201605 removed

Remove "TorCoreTeam201605" keyword. The time machine is broken.

comment:56 Changed 10 months ago by nickm

  • Keywords nickm-deferred-20160905 added
  • Milestone changed from Tor: 0.2.9.x-final to Tor: 0.2.???

Deferring many tickets that are in needs_revision with no progress noted for a while, where I think they could safely wait till 0.3.0 or later.

Please feel free to move these back to 0.2.9 if you finish the revisions soon.

comment:57 Changed 10 months ago by mikeperry

  • Milestone changed from Tor: 0.2.??? to Tor: 0.2.9.x-final
  • Status changed from needs_revision to needs_review

Alright. I switched the code over to using the new handle, monotonic timer, and timer wheel abstractions. All unit tests pass without leaks from this code (though the unit tests have grown new memory leaks of their own).

mikeperry/netflow_padding-v6. The commit specific to this bug is a51a998555b352043437cccbfc2758ab9b737925.

comment:58 Changed 10 months ago by teor

A reminder that if we merge Single Onion Services (#17178) and Netflow Padding (#16861) in 0.2.9, we'll likely want a Consensus Parameter to Disable Netflow Padding For Single Onion Services (#17857) as well.

comment:59 Changed 10 months ago by nickm

  • Keywords review-group-9 added

comment:60 Changed 9 months ago by nickm

I've opened https://gitlab.com/nickm_tor/tor/merge_requests/8 to do the commentary here.

comment:61 Changed 9 months ago by nickm

I've been over this one on gitlab, and asked a bunch of questions, and noted a few places that could use a small amount of tidying. This patch is looking nice!

Now that I've started on this review, please just add fixup commits to this branch, rather than rebasing or starting a new one: it will make changes much much easier for me to track.

(Also, can tell me about your performance testing?)

comment:62 Changed 9 months ago by nickm

  • Status changed from needs_review to needs_revision

comment:63 Changed 9 months ago by nickm

[I'll review the other tickets fixed by that branch separately]

comment:64 Changed 9 months ago by mikeperry

  • Status changed from needs_revision to needs_review

Ok, I commented to everything on that gitlab link and posted links to commits.

comment:65 Changed 9 months ago by nickm

  • Keywords nickm-deferred-20161005 added
  • Milestone changed from Tor: 0.2.9.x-final to Tor: 0.3.0.x-final

Deferring big/risky-feature things (even the ones I really love!) to 0.3.0. Please argue if I'm wrong.

comment:66 Changed 8 months ago by nickm

  • Keywords review-group-11 added

comment:67 Changed 8 months ago by nickm

  • Keywords review-group-12 added; review-group-11 removed

comment:68 Changed 7 months ago by nickm

  • Keywords review-group-13 added; review-group-12 removed

comment:69 Changed 6 months ago by nickm

  • Keywords review-group-14 added; review-group-13 removed

And that's all for review-group-13.

comment:70 Changed 6 months ago by nickm

  • Milestone changed from Tor: 0.3.0.x-final to Tor: 0.3.1.x-final

comment:71 Changed 6 months ago by nickm

  • Keywords review-group-14 removed

comment:72 Changed 4 months ago by nickm

  • Keywords review-group-16 added

comment:73 Changed 4 months ago by nickm

  • Sponsor SponsorU-can deleted

Clear sponsorS and sponsorU from open tickets in 0.3.1

comment:74 Changed 4 months ago by mikeperry

FYI this branch and the subtickets were rebased on to master a week or so ago in mikeperry/netflow_padding-v6-rebased (0b572b90f77bb466d8e1946aa6b0ff2fd131ca4d)

comment:75 Changed 3 months ago by nickm

  • Keywords review-group-16 removed

comment:76 Changed 3 months ago by dgoulet

Pushed the branch to my Gitlab for review:

Branch: netflow_padding-v6-rebased
https://gitlab.com/dgoulet/tor/merge_requests/24

comment:77 Changed 3 months ago by dgoulet

  • Points changed from 2 to 6
  • Status changed from needs_review to needs_revision

Made comments on the gitlab. Nothing show stopper but there are multiple things I would like addressed before merge so we don't end up forgetting it in the void of Tor Unspecified trac refactoring ticket.

comment:78 Changed 2 months ago by mikeperry

  • Status changed from needs_revision to needs_review

I fixed the issues from gitlab last week btw. Updates are in a couple of commits on my branch. Let me know if I should be rebasing again and/or squashing back down into a couple of commits for a merge. Otherwise I am waiting for more comments.

comment:79 Changed 2 months ago by dgoulet

  • Status changed from needs_review to needs_revision

Quite happy with the fixes!

*HOWEVER*, the branch doesn't compile :).

src/or/channelpadding.c: In function ‘channelpadding_get_channel_idle_timeout’:
src/or/channelpadding.c:567:42: error: passing argument 1 of ‘channel_is_client’ discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers]
   if (!is_canonical || CHANNEL_IS_CLIENT(chan, options)) {
                                          ^
src/or/channelpadding.c:69:56: note: in definition of macro ‘CHANNEL_IS_CLIENT’
   (!public_server_mode((options)) || channel_is_client(chan) || \
                                                        ^~~~
In file included from src/or/channelpadding.c:12:0:
src/or/channel.h:668:5: note: expected ‘channel_t * {aka struct channel_s *}’ but argument is of type ‘const channel_t * {aka const struct channel_s *}’
 int channel_is_client(channel_t *chan);
     ^~~~~~~~~~~~~~~~~

comment:80 Changed 2 months ago by mikeperry

Arg, I was compiling without --enable-fatal-warnings so I missed that.

Anyway that was another one of those places where Nick asked to make as many things const as possible, which conflicts with using channel_is_client(). I reverted removed that commit and pushed the previous commit hash to a new branch: mikeperry/netflow_padding-v6-rebased2. That branch head is bbef0dff69debfeeca4e4bc2980a8e1812aaffdd.

comment:81 Changed 2 months ago by dgoulet

Thanks!

Now I have this issue with the make test (with --enable-fragile-hardening):

channelpadding/channelpadding_decide_to_pad_channel: [forking] Apr 28 15:28:31.309 [warn] channelpadding_compute_time_until_pad_for_netflow(): Bug: Channel padding timeout scheduled 100ms in the past. Did the monotonic clock just jump? (on Tor 0.3.1.0-alpha-dev bbef0dff69debfee)
OK
channelpadding/channelpadding_negotiation: [forking] src/test/test_channelpadding.c:637:56: runtime error: member access within misaligned address 0x7fffd1aec815 for type 'struct channelpadding_negotiate_t', which requires 2 byte alignment
0x7fffd1aec815: note: pointer points here
 00 00 00 0c 00 01 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00
             ^ 
src/test/test_channelpadding.c:645:56: runtime error: member access within misaligned address 0x7fffd1aec815 for type 'struct channelpadding_negotiate_t', which requires 2 byte alignment
0x7fffd1aec815: note: pointer points here
 00 00 00 0c 01 01 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00
             ^ 
OK

And this but maybe this is not related to your code and has been fixed since (dunno..):

entrynodes/parse_from_state_full: [forking] 
  FAIL src/test/test_entrynodes.c:618: assert(smartlist_len(gs_br->sampled_entry_guards) OP_EQ 2): 0 vs 2
  [parse_from_state_full FAILED]

comment:82 Changed 2 months ago by dgoulet

  • Status changed from needs_revision to merge_ready

Latest update to mikeperry/netflow_padding-v6-rebased2 fixes the above issue (and the entrynodes test failing was already failing on the initial commit).

I'm moving this to merge_ready for nickm to take a final look/merge.

comment:83 Changed 2 months ago by mikeperry

So one other moving piece here is that I also change the is_client flag to be accurate (similar to #21406 and #21585). I check the consensus, where as checking for authentication usage might be cleaner, or it might not be. I am still inclined to think that the is_client checks should be based on the consensus rather than behavior-based, but we should probably decide on a single thing here.

comment:84 Changed 7 weeks ago by nickm

  • Resolution set to implemented
  • Status changed from merge_ready to closed

Merging this, at last! Also merging mikeperry/padding_spec to torspec. Thanks for all the hard work here. This is big, so let's keep an eye on it.

comment:85 Changed 6 weeks ago by mikeperry

nickm: based on behavior, #17592 and #17604 are bugfixes on 0.2.5.5-alpha. that is the last time we changed that behavior (for #6799)

Note: See TracTickets for help on using tickets.