Ticket30924 042 01

[dgoulet-tor]

No description provided.

[asn-d6]

Hmm this commit was confusing to me. I need to look into this more. But perhaps we can make param_introduce_defense_enabled private so that it's obvious that it should not be used by public functions like this one?

In general, I'm kinda confused by the various priorities and interactions between consensusparam variables and other dos variaables.

[dgoulet-tor]

param_introduce_defense_enabled needs to be public from a function. I believe one of the last commits makes it public so the intro point sets the service intro circuit with that value by default to know if it enables the defenses or not.

See hs_dos_get_intro2_enabled_param()

[asn-d6]

Hmm, I'm still confused by this. Here are some reasons:

• I don't like the name of hs_dos_get_intro2_rate_param() and its two friends. The name I don't like because "param" is an ambiguous word in general, but even more so in this particular context given that the code means "consensus param" but the first thing that came to my mind were the new params from the ESTABLISH_INTRO cell.

• I don't like that hs_dos_get_intro2_enabled_param() is public since it's not clear whether that function knows the ground truth, or I need to consult the s_intro_circ->introduce2_dos_defense_enabled to know the ground truth.

• It's a bit confusing to have get_param_intro_dos_enabled() and also hs_dos_get_intro2_rate_param() and neither of these two things gives me the ground truth in some cases (because if the values are specified in the establish_intro cell the ground truth is different).

Here is a suggestion. Please tell me if you like it:

• Dump the get_param_intro_dos_enabled etc. functions and do the consensus check inline in set_consensus_parameters(). This way we dont have two similarly named functions doing almost the same thing.

• Abstract the following code of hs_intropoint.c into its own hs_dos_setup_default_introduce2_defences() hs_dos function:

  circ->introduce2_dos_defense_enabled = hs_dos_get_intro2_enabled_param();
  token_bucket_ctr_init(&circ->introduce2_bucket,
                        hs_dos_get_intro2_rate_param(),
                        hs_dos_get_intro2_burst_param(),
                        (uint32_t) approx_time());
  

  This way we can then make the hs_dos_get_intro2_ functions into private for the hs_dos module.

• Finally, rename the hs_dos_get_intro2_rate_param() functions into get_intro2_rate_default_consensus_value() because now they are private.

I can also try to do this if you want me , and then we can see if we like it.

[dgoulet-tor]

Ok I've pushed several new commits to address this. Not very complicated. I did not follow exactly what you said, made some changes, you'll see. Main thing is that I kept the consensus parameter variables (see start of hs_dos.c) but now I only use those in the hs_dos.c code.

Last commit is also a fix that I found while doing this refactoring.

[coveralls]

Pull Request Test Coverage Report for Build 5811

• 112 of 136 (82.35%) changed or added relevant lines in 7 files are covered.
• 13 unchanged lines in 3 files lost coverage.
• Overall coverage remained the same at 62.887%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/feature/hs/hs_config.c	6	30	20.0%

Files with Coverage Reduction	New Missed Lines	%
src/feature/hs/hs_dos.c	1	95.12%
src/feature/dirauth/shared_random.c	3	85.54%
src/feature/dirauth/dirvote.c	9	64.99%

Totals
Change from base Build 5808:	0.0%
Covered Lines:	47366
Relevant Lines:	75319

---

💛 - Coveralls

[asn-d6]

What happens if HiddenServiceEnableIntroDoSDefense is enabled but no rate/burst values are set in the torrc? I was assuming that the default rate/burst values are picked (as documented below) but I think that's not the case, and the defense stays disabled. Is this true? Maybe this should be documented on the man page, or add a warn in the code?

[dgoulet-tor]

Default rate and burst are used. See set_service_default_config().

[asn-d6]

s/consider/considered ?

[dgoulet-tor]

Fixup commit done.

[asn-d6]

Let's add a log_info here with the values we chose, because there is no service-side log about this I think. We can have it log_info since building intro circuits is not so frequent.

[dgoulet-tor]

Commit 5e256d5

[asn-d6]

Let's document all of these and specify how each of them is used. For validation or as default values? By the service or the intro point?

[dgoulet-tor]

Well hmmm... you sure that is necessary? We have man page explaining each of them. We have a full proposal explaining what they do.

And there is _DEFAULT for each param for the exact purpose of indicating the default value? And MIN/MAX is obviously for validation.

Not sure what more would be useful here without duplicating quite a bit the same documentation?

[asn-d6]

Perhaps we should document that if there is a value in establish_intro cell these values get lower priority over the cell values?

[dgoulet-tor]

Commit 9eeb2a5

[asn-d6]

Let's split these three checks into individual if blocks and document each of them on what exactly they are doing, so that it's more clear what we are checking exactly.

[dgoulet-tor]

I've majorly refactor this. And fixed a bug in there! There was a typo between RATE/BURST... And now even unit testing that ;)

Commit 24c49a5

[asn-d6]

Why isn't intro2_burst_per_sec > HS_CONFIG_V3_DOS_DEFENSE_RATE_PER_SEC_MAX allowed? Is there an implicit limit to burst values based on the rate values? As suggested above some documentation on these checks would be useful.

[dgoulet-tor]

This is one of the bug I fixed in previous commit.

[asn-d6]

Let's print the offending values in this case also.

[dgoulet-tor]

Done in the refactored commit above.

[asn-d6]

Perhaps we need some unittests for the torrc parsing and validation? And maybe cross-checking them with the intro-point validation checks?

[dgoulet-tor]

See commit 667e6db