Add new valid BridgeDistribution methods

This adds settings, telegram and reserved as valid BridgeDistribution methods.

This change prevents the warning "Unrecognized BridgeDistribution value X. I'll assume you know what you are doing..." from being logged, which is potentially confusing to bridge operators who set the values using this option.

Note that I have added reserved which is valid (and tested), but is not new. Please let me know if this was intentionally excluded and I will remove it again.

requested review from @ahf

@meskio Didn't we want BridgeDistribution to accept arbitrary strings (with some constraints to it) rather than the current set of approved methods?

The patch looks good otherwise.

I think accepting arbitrary strings in BridgeDistribution is a good idea. As this merge-request shows we might add new distribution mechanisms and forget to update tor about them.

Do I understand correctly that the current implementation does accept arbitrary strings but gives a warning if is an unknown one? I think this is a good behavior, if rdsys gets a distribution request that can't recognize it will ignore it and assign the bridge automatically as if any was set. I think is good to warn users that their configured mechanism might be wrong.

I wonder if instead of ignoring unknown BridgeDistribution is not better to treat them as none and don't distribute those bridges. I just created an issue in rdsys to explore if we should change it tpo/anti-censorship/rdsys#110 (closed).

@meskio Right now it does warn in that function, but later you can see the following code:

  if (options->BridgeDistribution) {
    if (!options->BridgeRelay) {
      REJECT("You set BridgeDistribution, but you didn't set BridgeRelay!");
    }
    if (check_bridge_distribution_setting(options->BridgeDistribution) < 0) {
      REJECT("Invalid BridgeDistribution value.");
    }
  }

This means we hard reject the configuration file if it's an unknown string.

resolved all threads

changed milestone to %Tor: 0.4.8.x-freeze

@meskio I think with the above we need to decide if we want this in for 0.4.8 or not. As I described above, it does reject unknown strings here with an "Invalid BridgeDistribution value." error message.

Do we prefer to accept arbitrary strings or do we need to support the 3 newly enumerated strings here with "settings", "telegram", and "reserved" ?

I have no strong opinions here and will let it be up to you what the anti-censorship team wants here :-)

I think we should accept any arbitrary string, and give a warning if the string is not in the known list (that should include settings, telegram and reserved). I think we should accept arbitrary strings, so people don't need to have the latest tor version if we add a new distributor to be able pin it. But a warning will be nice, as people might misspell the distribution mechanism and we'll treat anything we don't know about as none and the bridge will not be used.

If you prefer it I will be ok with it not having the warning neither the list and just accept anything in that field.

Thank you for coming back to this.

Sounds good, I am assigning this to myself since it requires a small addition in the feature set from you!

Thanks.

requested review from @triage-bot and removed review request for @ahf

assigned to @ahf

changed milestone to %Tor: 0.4.9.x-freeze

added For Anticensorship Team label

To the list of recognized distributors we should add lox, as we'll start testing it soon.

mentioned in merge request !806 (closed)