Adaptive padding final #624

asn-d6 · 2019-01-02T13:50:39Z

No description provided.

We need this for padding negotiation so that we can have later machine revisions supercede earlier ones. Co-authored-by: George Kadianakis <desnacked@riseup.net>

This is a good code review start point, to get an overview of the interfaces and types used in circuit padding. Co-authored-by: George Kadianakis <desnacked@riseup.net>

Co-authored-by: George Kadianakis <desnacked@riseup.net>

This helps us to determine if a middle node can pad to us or not. Co-authored-by: George Kadianakis <desnacked@riseup.net>

Co-authored-by: George Kadianakis <desnacked@riseup.net>

These event callbacks allow circuit padding to decide when to attempt to launch and negotiate new padding machines, and when to tear old ones down. Co-authored-by: George Kadianakis <desnacked@riseup.net>

These callbacks allow the padding state machines to react to various types of sent and received relay cells. Co-authored-by: George Kadianakis <desnacked@riseup.net>

This implements all of the event handling, state machines, and padding decisions for circuit padding. I recommend reviewing this after you look at the call-in points into it from the rest of Tor. Co-authored-by: George Kadianakis <desnacked@riseup.net>

Co-authored-by: George Kadianakis <desnacked@riseup.net>

Hope is this will make it easier to test on the live tor network. Does not need to be merged if we don't want to, but will come in handy for researchers. Co-authored-by: George Kadianakis <desnacked@riseup.net>

This project introduces the prob_distr.c subsystem which implements all the probability distributions that WTF-PAD needs. It also adds unittests for all of them. Code and tests courtesy of Riastradh. Co-authored-by: Taylor R Campbell <campbell+tor@mumble.net> Co-authored-by: Mike Perry <mikeperry-git@torproject.org>

Co-authored-by: Mike Perry <mikeperry-git@torproject.org> Co-authored-by: Taylor R Campbell <campbell+tor@mumble.net>

Co-authored-by: Mike Perry <mikeperry-git@torproject.org>

Note to self/others: don't merge this. Co-authored-by: George Kadianakis <desnacked@riseup.net>

coveralls · 2019-01-02T14:24:14Z

Pull Request Test Coverage Report for Build 3569

956 of 1018 (93.91%) changed or added relevant lines in 14 files are covered.
1127 unchanged lines in 6 files lost coverage.
Overall coverage increased (+0.5%) to 61.733%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/app/config/config.c	1	2	50.0%
src/feature/nodelist/routerlist.c	1	2	50.0%
src/core/or/circuituse.c	3	5	60.0%
src/core/or/connection_edge.c	0	2	0.0%
src/core/or/relay.c	5	12	41.67%
src/core/or/circuitbuild.c	2	13	15.38%
src/lib/math/prob_distr.c	220	238	92.44%
src/core/or/circuitpadding.c	712	732	97.27%

Files with Coverage Reduction	New Missed Lines	%
src/feature/hs/hs_service.c	1	73.78%
src/core/or/circuitbuild.c	2	41.84%
src/core/or/circuituse.c	110	29.37%
src/feature/rend/rendclient.c	171	22.76%
src/feature/hs/hs_client.c	234	51.32%
src/core/or/connection_edge.c	609	26.64%

Totals
Change from base Build 3538:	0.5%
Covered Lines:	45237
Relevant Lines:	73279

💛 - Coveralls

nmathewson

Okay, I think I've looked everything over. Many of these are just questions, or stuff that could be done postmerge, so let me know what you think.

nmathewson · 2019-01-09T16:09:04Z

src/lib/smartlist_core/smartlist_foreach.h

@@ -83,6 +83,14 @@
         ++var ## _sl_idx) {                                    \
      var = (sl)->list[var ## _sl_idx];

+#define SMARTLIST_FOREACH_REVERSE_BEGIN(sl, type, var)  \


I think this will be incompatible with SMARTLIST_DEL_CURRENT; we should maybe add a comment?

A comment about this was added in a336d81.

nmathewson · 2019-01-09T16:09:55Z

src/core/or/circuitpadding.h

+#ifndef TOR_CIRCUITPADDING_H
+#define TOR_CIRCUITPADDING_H
+
+#include "circpad_negotiation.h"


We try to include stuff like this by full path; can fix post merge though.

Fixed in 2b29bcc

nmathewson · 2019-01-09T16:12:13Z

src/core/or/circuitpadding.h

+typedef int8_t circpad_hist_index_t;
+
+/** The type for absolute time, from monotime_absolute_usec() */
+typedef uint64_t circpad_time_t;


It is probably faster to use something else if we are going to be calling this a lot; the non-coarse monotime functions are slower than the coarse ones, and converting to usec sometimes takes an expensive division on 32-bit platforms.

Hmm, not quite sure how to fix this.

Not sure if using coarse time sources is acceptable here given that padding delays are can be pretty small and might need to be accurate.

Any ideas?

So, how small are we talking here? Libevent won't reliably give us a timeout more fine-grained than tens of milliseconds on many platforms.

Even if coarse time isn't workable, we should at least figure out how to minimize how often this is called, and try to use monotime_t. But I think coarse time or timestamp-units is a better idea, probably.

Maybe the right answer is to define an abstraction that can be one or the other, and compile it conditionally?

This was discussed and resolved in the trac ticket: https://trac.torproject.org/projects/tor/ticket/28142#comment:35

We should fix this for good before enabling any machines.

nmathewson · 2019-01-09T16:14:15Z

src/core/or/circuitpadding.h

+
+/**
+ * \file circuitpadding.h
+ * \brief Header file for circuitpadding.c.


This could use a comment that explains WTF-PAD 101 for beginners, unless such a comment will appear somewhere else. Most of the stuff below in this file only makes sense if you know how the rest of wtfpad works.

Done in cdbd5c0.

nmathewson · 2019-01-09T16:15:41Z

src/core/or/circuitpadding.h

+ * A circuit must satisfy every sub-field in this type in order
+ * to be considered to match the conditions.
+ */
+typedef struct circpad_machine_conditions_t {


Are all of these structures are stuff that users of the wtf-pad api will need, or are some of them internal only, or only used in a few places? In the latter case, we now use a "foo_st.h" header for them.

I think we can do this, yes.

Folded this idea in https://trac.torproject.org/projects/tor/ticket/28777#comment:3 so that we can do it post-merge, if you think that's acceptable.

nmathewson · 2019-01-09T16:34:39Z

PADDING_TODO.txt

@@ -0,0 +1,58 @@
+TODO sketch for this branch, in rough priority order:


Does this file get removed later on in the branch? If not, then the commit message suggests we have an issue here. :)

Mike what should we do with this file in your opinion?

I removed the TODO file in b269ab5 and added the remaining items as children of #28632.

nmathewson · 2019-01-09T16:36:07Z

doc/tor.1.txt

@@ -1020,6 +1020,21 @@ The following options are useful only for clients (that is, if
    The .exit address notation, if enabled via MapAddress, overrides
    this option.

+[[MiddleNodes]] **MiddleNodes** __node__,__node__,__...__::


If this is going to get merged, we should mark it as experimental-only for some reason, and tell people what they should use instead for non-experimental use. We may also want to say explicitly that it might go away in the future.

Done in 2b29bcc.

nmathewson · 2019-01-09T16:37:45Z

src/lib/crypt_ops/crypto_rand.c

+ * Draw an unsigned 32-bit integer uniformly at random.
+ */
+uint32_t
+crypto_rand_uint32(void)


This function's behavior is inconsistent with crypto_rand_uint64(), which takes an argument. Maybe it should have a different name, like, crypto_rand_u32()? Or maybe we should rename the other one?

Done in e0e0338.

nmathewson · 2019-01-09T16:39:53Z

src/lib/math/.may_include

@@ -3,3 +3,5 @@ orconfig.h
 lib/cc/*.h
 lib/log/*.h
 lib/math/*.h
+lib/testsupport/*.h
+lib/crypt_ops/*.h


This change raises lib/math to a higher level than it was at before, which means that these libraries will no longer be able to use it: container, trace, thread, term, memarea, encoding, sandbox, fs, meminfo, crypt_os, and time. I think that's okay?

OK. If it ever becomes a problem, we can reevaluate.

nmathewson · 2019-01-09T16:40:22Z

src/lib/math/fp.c

+
+/* isinf() wrapper for tor */
+int
+tor_isinf(double x)


This could use a test.

Done in 4db9c3d.

See torproject#624 (comment)

In file included from src/core/or/connection_edge.c:70:0: ./src/core/or/circuitpadding.h:16:26: error: redefinition of typedef ‘circuit_t’ ./src/core/or/or.h:930:26: note: previous declaration of ‘circuit_t’ was here ./src/core/or/circuitpadding.h:17:33: error: redefinition of typedef ‘origin_circuit_t’ ./src/core/or/or.h:931:33: note: previous declaration of ‘origin_circuit_t’ was here ./src/core/or/circuitpadding.h:18:23: error: redefinition of typedef ‘cell_t’ ./src/core/or/or.h:628:23: note: previous declaration of ‘cell_t’ was here typedef doesn't work for forward declarations, but plain struct without a typedef wrapper does (and unlike the _t type aliases makes it clearer for everyone whether you're talking about the struct or the pointer).

In file included from ./src/core/or/or_circuit_st.h:12:0, from src/core/or/circuitlist.c:112: ./src/core/or/circuit_st.h:15:39: error: redefinition of typedef ‘circpad_machine_spec_t’ ./src/core/or/circuitpadding.h:572:3: note: previous declaration of ‘circpad_machine_spec_t’ was here ./src/core/or/circuit_st.h:16:40: error: redefinition of typedef ‘circpad_machine_state_t’ ./src/core/or/circuitpadding.h:517:3: note: previous declaration of ‘circpad_machine_state_t’ was here

Test exactly what the geometric sampler returns, because that's what the downstream callers of it are going to use. While here, also assert that the geometric sampler returns a positive integer. (Our geometric distribution is the one suported on {1, 2, 3, ...} that returns the number of trials before the first success, not the one supported on {0, 1, 2, ...} that returns the number of failures before the first success.)

NOTE: This commit breaks the build, because there was a mistake in an earlier change of exactly the sort that this is meant to detect! I'm leaving it broken for illustration.

All remaining tasks are now part of #28632.

Mike Perry and others added 17 commits January 2, 2019 15:09

Provide a smartlist reverse-order traversal.

3ba7581

We need this for padding negotiation so that we can have later machine revisions supercede earlier ones. Co-authored-by: George Kadianakis <desnacked@riseup.net>

Circuit padding header.

2a24e21

This is a good code review start point, to get an overview of the interfaces and types used in circuit padding. Co-authored-by: George Kadianakis <desnacked@riseup.net>

Hook up circuit padding to circuit_t.

2f7b5a2

Co-authored-by: George Kadianakis <desnacked@riseup.net>

Initialize circuit padding machines and global state.

70e9245

Co-authored-by: George Kadianakis <desnacked@riseup.net>

Circuit padding ProtoVer plumbing.

659a4f0

This helps us to determine if a middle node can pad to us or not. Co-authored-by: George Kadianakis <desnacked@riseup.net>

Add padding negotiation trunnel output.

4ca1df6

Co-authored-by: George Kadianakis <desnacked@riseup.net>

Circuit padding machine creation events.

43701e1

These event callbacks allow circuit padding to decide when to attempt to launch and negotiate new padding machines, and when to tear old ones down. Co-authored-by: George Kadianakis <desnacked@riseup.net>

Circuit padding cell event callbacks.

7be7190

These callbacks allow the padding state machines to react to various types of sent and received relay cells. Co-authored-by: George Kadianakis <desnacked@riseup.net>

Circuit padding implementation.

9aaf72e

This implements all of the event handling, state machines, and padding decisions for circuit padding. I recommend reviewing this after you look at the call-in points into it from the rest of Tor. Co-authored-by: George Kadianakis <desnacked@riseup.net>

Add relay crypto mock points for tests.

d623400

Co-authored-by: George Kadianakis <desnacked@riseup.net>

Circuit padding tests.

a336d81

Co-authored-by: George Kadianakis <desnacked@riseup.net>

Config option to specify specific MiddleNodes.

8ad497b

Hope is this will make it easier to test on the live tor network. Does not need to be merged if we don't want to, but will come in handy for researchers. Co-authored-by: George Kadianakis <desnacked@riseup.net>

Use the new probability distribution code in WTF-PAD.

dd04917

Co-authored-by: Mike Perry <mikeperry-git@torproject.org> Co-authored-by: Taylor R Campbell <campbell+tor@mumble.net>

Concentrate all TOR_USEC_PER_SEC definitions in a single header file.

926fc93

Co-authored-by: Mike Perry <mikeperry-git@torproject.org>

Disable current padding machines.

56a45eb

Co-authored-by: Mike Perry <mikeperry-git@torproject.org>

Add TODO file for padding work.

0658c72

Note to self/others: don't merge this. Co-authored-by: George Kadianakis <desnacked@riseup.net>

asn-d6 mentioned this pull request Jan 2, 2019

Adaptive padding 20181227 #623

Closed

nmathewson reviewed Jan 9, 2019

View reviewed changes

asn-d6 and others added 10 commits January 10, 2019 13:05

Add top-level file documentation for circuitpadding.c

cdbd5c0

Rename circpad_machine_t -> circpad_machine_spec_t

5738a0a

Rename circpad_machineinfo_t -> circpad_machine_state_t

f493817

Rename crypto_rand_uint32() -> crypto_rand_u32()

e0e0338

See torproject#624 (comment)

Unittest for tor_isinf().

4db9c3d

Misc trivial improvements around circpadding code.

2b29bcc

Use the distribution abstraction as an abstraction.

0f8253b

Taylor R Campbell and others added 3 commits January 11, 2019 14:41

Add some more type checking.

d82a8a7

NOTE: This commit breaks the build, because there was a mistake in an earlier change of exactly the sort that this is meant to detect! I'm leaving it broken for illustration.

Fix wrong bases.

9728d3f

Clarify immutability of global padding machine specs.

4dc189a

asn-d6 force-pushed the adaptive_padding-final branch from 94c33ea to 4dc189a Compare January 11, 2019 12:42

Remove TODO file.

b269ab5

All remaining tasks are now part of #28632.

ghost merged commit b269ab5 into torproject:master Jan 14, 2019

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adaptive padding final #624

Adaptive padding final #624

asn-d6 commented Jan 2, 2019

coveralls commented Jan 2, 2019 •

edited

nmathewson left a comment

nmathewson Jan 9, 2019

asn-d6 Jan 10, 2019

nmathewson Jan 9, 2019

asn-d6 Jan 10, 2019

nmathewson Jan 9, 2019

asn-d6 Jan 10, 2019

nmathewson Jan 10, 2019

asn-d6 Jan 14, 2019

nmathewson Jan 9, 2019

asn-d6 Jan 10, 2019

nmathewson Jan 9, 2019

asn-d6 Jan 10, 2019

nmathewson Jan 9, 2019

asn-d6 Jan 10, 2019

asn-d6 Jan 14, 2019

nmathewson Jan 9, 2019

asn-d6 Jan 10, 2019

nmathewson Jan 9, 2019

asn-d6 Jan 10, 2019

nmathewson Jan 9, 2019

asn-d6 Jan 10, 2019

nmathewson Jan 9, 2019

asn-d6 Jan 10, 2019

		@@ -0,0 +1,58 @@
		TODO sketch for this branch, in rough priority order:

Adaptive padding final #624

Adaptive padding final #624

Conversation

asn-d6 commented Jan 2, 2019

coveralls commented Jan 2, 2019 • edited

Pull Request Test Coverage Report for Build 3569

💛 - Coveralls

nmathewson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 2, 2019 •

edited