Opened 8 months ago

Last modified 7 months ago

#33361 merge_ready defect

relay: Warn about the lack of ContactInfo and the consequence

Reported by: dgoulet Owned by: dgoulet
Priority: Medium Milestone: Tor: 0.4.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 035-backport, 041-backport, 042-backport, 043-backport, consider-backport-after-0433, network-health
Cc: gk, arma Actual Points:
Parent ID: Points: 0.1
Reviewer: gk Sponsor:

Description

The network health team (including bad relays) as been rejecting group of relays more and more that do not have their ContactInfo or/and MyFamily set (for a group of relays from the same operator).

These are for safety and health purposes of the network. See the relay guidelines for that.

That being said, we should improve the log notice that an operator get when failing to set ContactInfo to a warning and adding a sentence that says they might get rejected from the network due to a lack of valid responsive ContactInfo.

I would argue that this needs a backport for our maintained versions. Please, let me know if not agreeing with that

Child Tickets

Change History (14)

comment:1 Changed 8 months ago by gk

Keywords: network-health added

comment:2 Changed 8 months ago by dgoulet

Status: assignedneeds_review

035:

PR: https://github.com/torproject/tor/pull/1748
Branch: ticket33361_035_01

043+:

PR: https://github.com/torproject/tor/pull/1749
Branch: ticket33361_043_01

comment:3 Changed 8 months ago by dgoulet

Reviewer: gk

comment:4 Changed 8 months ago by cypherpunks

Describes a way to contact the relay's administrator, preferably including an email address and a PGP key fingerprint.

comment:5 in reply to:  2 Changed 8 months ago by gk

Status: needs_reviewneeds_revision

Replying to dgoulet:

035:

PR: https://github.com/torproject/tor/pull/1748
Branch: ticket33361_035_01

043+:

PR: https://github.com/torproject/tor/pull/1749
Branch: ticket33361_043_01

I hope review comments here are fine (I don't have a Github account). So, generally this looks good to me.

One thing I am not sure is whether this is actually a bugfix in tor's context or more a small feature. You know that better than me as I am not as well-versed regarding the network-team policies as you, so I am fine with whatever you pick, but I thought I'd mention it anyway.

Some nits (the first two at least are for the needs_revision)

1) Let's keep the comma after "setting it"

2) s/your servers is/your server is/

3) Just adding "end-of-life" here seems a bit vague. In the context of the changed warning message it seems we are concerned about their *server* getting end-of-life'd (whatever that means). Maybe operators start to think about their operating system when reading the message? I am not sure whether we want to cover that, though. Rather, I guess we are concerned about the Tor version getting EOL, right? If so, what about s/end-of-life/your Tor version reaches end-of-life/?

4) Adding the enf-of-life part raises the question about Oxford comma or not. I am not sure either what the stance in the network-team here is. But if it's the case that the policy is "Yes, Oxford comma" then please do s/end-of-life or something/end-of-life, or something/.

comment:6 Changed 7 months ago by dgoulet

Status: needs_revisionneeds_review

Revised. See the fixup commits on both branches.

comment:7 Changed 7 months ago by gk

Status: needs_reviewmerge_ready

The fixup commits look good to me (6a4912fab5c80cf2e64b0a7909497ae8c2da1952 on ticket33361_043_01 and e7829ce2a4ac1c1cd0728c1a6b6bdf89ea545375 on ticket33361_035_01). Not sure what the process in the network-team is to get those branches merged, like does the one merging patches require a squashed branch before? Or do they do the squashing while merging? Either way, this looks ready from my PoV. I am happy to look over a squashed and rebased branch against master again to make sure no issues crept in while squashing, if that's part of the procedure. :)

comment:8 Changed 7 months ago by teor

The merger will squash the branches before merging, and get back to you if the squash fails.

comment:9 Changed 7 months ago by nickm

Milestone: Tor: 0.4.4.x-finalTor: 0.4.2.x-final

I've squashed the branch as ticket33361_035_01_squashed with a PR at https://github.com/torproject/tor/pull/1793 .

I've merged it to 0.4.3 and forward, and am marking it as an easy backport.

comment:10 Changed 7 months ago by nickm

I've added another commit to the branch , to make the unit tests pass. I'm testing it on 0.4.3, and if it passes, I'll merge it to 0.4.3 and forward.

(Please remember to check CI, folks!)

comment:11 Changed 7 months ago by teor

Keywords: consider-backport-after-0433 added

It looks like Nick has merged the CI fix.

Scheduling for backport after the next alpha.

comment:12 in reply to:  10 Changed 7 months ago by gk

Status: merge_readyneeds_revision

Replying to nickm:

I've added another commit to the branch , to make the unit tests pass. I'm testing it on 0.4.3, and if it passes, I'll merge it to 0.4.3 and forward.

(Please remember to check CI, folks!)

Okay, it seems I can't see any CI reports without logging in to Github, so I missed that and I forgot to run make test, locally to have at least some more indicators whether things are okay or not. Doing that for the not yet merged 0.3.5 patch indicates actually two test failures instead of one for 0.4.3:

options/validate__uname_for_server: [forking] 
  FAIL ../src/test/test_options.c:518: expected log to not contain entries  Captured logs:
    1. warn: "Your ContactInfo config option is not set. Please strongly consider setting it, so we can contact you if your relay is misconfigured, end-of-life, or something else goes wrong. It is also possible that your relay might get rejected from the network due to a missing valid contact address.\n"

  [validate__uname_for_server FAILED]
options/validate__outbound_addresses: [forking] OK
options/validate__data_directory: [forking] OK
options/validate__nickname: [forking] OK
options/validate__contactinfo: [forking] 
  FAIL ../src/test/test_options.c:635: expected log to contain "Your ContactInfo config option is not set. Please consider setting it, so we can contact you if your server is misconfigured or something else goes wrong.

Not sure what the network-team policy is for such a case like this but I am marking this ticket as needs_revision so we get a branch that properly applies to 0.3.5.x.

comment:13 Changed 7 months ago by nickm

It should suffice to merge my ticket33361_035_01_squashed (https://github.com/torproject/tor/pull/1793 ) instead of the original branch; does that one give the same errors?

comment:14 in reply to:  13 Changed 7 months ago by gk

Status: needs_revisionmerge_ready

Replying to nickm:

It should suffice to merge my ticket33361_035_01_squashed (https://github.com/torproject/tor/pull/1793 ) instead of the original branch; does that one give the same errors?

Nope, looks good. Thanks.

Note: See TracTickets for help on using tickets.