Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#18760 closed enhancement (implemented)

I'd like to customize rejection message from dir auths to relay

Reported by: arma Owned by: arma
Priority: Medium Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 029-accepted, review-group-1, TorCoreTeam201605
Cc: Actual Points:
Parent ID: Points:
Reviewer: nickm Sponsor:

Description

Right now if you're a relay and you try to post your descriptor and the dir auth rejects it, you get this in your logs:

        log_warn(LD_GENERAL,"http status 400 (%s) response from "
                 "dirserver '%s:%d'. Please correct.",

where the first %s comes from various possible choices in dirserv_get_status_impl(), e.g.

      *msg = "Fingerprint is marked rejected";

or

      *msg = "Tor version is insecure or unsupported. Please upgrade!";

We have been encountering cases lately where we want to get a message to a relay operator who is doing something scary (for example, running way too many relays) but who has not set any contactinfo. It would be neat to be able to customize our rejection message, for example to a more friendly one like "Wow, that's a lot of relays! Please contact us?"

Child Tickets

Change History (20)

comment:1 Changed 4 years ago by arma

The infrastructure is already in place for this on the relay end, so all we need is some config mechanism on the dir auth side.

Would the world end if we just made the AuthDirReject line take more than one argument, and everything after the first argument is the message that we want to send along with the rejection?

Can somebody improve on that design I hope? :)

comment:2 Changed 4 years ago by arma

Summary: Would like to customize rejection message from dir auths to relayI'd like to customize rejection message from dir auths to relay

comment:3 Changed 4 years ago by nickm

Keywords: 029-proposed added

comment:4 Changed 4 years ago by arma

Yuck -- the configuration interface I describe would be easy to do, but inside the code, it sure is unprepared for this idea. The code that makes the decisions includes stuff like

  if (! addr_policy_permits_address(addr, port, authdir_reject_policy))
    return 0;

and our policy linked-lists don't come with const char *reason and maybe they shouldn't. In any case, this is no longer a simple patch, because of how our current code is shaped.

So! Here is my much simplified plan:

diff --git a/src/or/dirserv.c b/src/or/dirserv.c
index ae67e8e..68c7d18 100644
--- a/src/or/dirserv.c
+++ b/src/or/dirserv.c
@@ -367,7 +367,7 @@ dirserv_get_status_impl(const char *id_digest, const char *n
     log_fn(severity, LD_DIRSERV, "Rejecting '%s' because of address '%s'",
                nickname, fmt_addr32(addr));
     if (msg)
-      *msg = "Authdir is rejecting routers in this range.";
+      *msg = "Suspicious relay address range -- please contact us?";
     return FP_REJECT;
   }
   if (!authdir_policy_valid_address(addr, or_port)) {

is good?

comment:5 Changed 4 years ago by teor

Looks good to me!
Do we want to provide an email address or something as a contact detail?

comment:6 Changed 4 years ago by nickm

go for it, but include an email address.

comment:7 Changed 4 years ago by arma

My feature18760 branch now has the change.

It doesn't include an email address though (but the commit msg does say why it doesn't).

What email address would you like me to include? Mine? :)

comment:8 in reply to:  7 Changed 4 years ago by arma

Replying to arma:

What email address would you like me to include? Mine? :)

Actually! Hm. The authority does have a ContactInfo set, doesn't it.

comment:9 Changed 4 years ago by arma

For posterity, here was the patch I was running on moria1 before this one:

diff --git a/src/or/dirserv.c b/src/or/dirserv.c
index f012b7b..062d077 100644
--- a/src/or/dirserv.c
+++ b/src/or/dirserv.c
@@ -61,8 +61,8 @@ struct authdir_config_t;
 static uint32_t
 dirserv_get_status_impl(const char *fp, const char *nickname,
                         uint32_t addr, uint16_t or_port,
-                        const char *platform, const char **msg,
-                        int severity);
+                        const char *platform, const char *contact_info,
+                        const char **msg, int severity);
 static void clear_cached_dir(cached_dir_t *d);
 static const signed_descriptor_t *get_signed_descriptor_by_fp(
                                                         const char *fp,
@@ -295,7 +295,8 @@ dirserv_router_get_status(const routerinfo_t *router, const char **msg,
 
   return dirserv_get_status_impl(d, router->nickname,
                                  router->addr, router->or_port,
-                                 router->platform, msg, severity);
+                                 router->platform, router->contact_info,
+                                 msg, severity);
 }
 
 /** Return true if there is no point in downloading the router described by
@@ -307,7 +308,7 @@ dirserv_would_reject_router(const routerstatus_t *rs)
 
   res = dirserv_get_status_impl(rs->identity_digest, rs->nickname,
                                 rs->addr, rs->or_port,
-                                NULL, NULL, LOG_DEBUG);
+                                NULL, NULL, NULL, LOG_DEBUG);
 
   return (res & FP_REJECT) != 0;
 }
@@ -322,7 +323,8 @@ dirserv_would_reject_router(const routerstatus_t *rs)
 static uint32_t
 dirserv_get_status_impl(const char *id_digest, const char *nickname,
                         uint32_t addr, uint16_t or_port,
-                        const char *platform, const char **msg, int severity)
+                        const char *platform, const char *contact_info,
+                        const char **msg, int severity)
 {
   uint32_t result = 0;
   router_status_t *status_by_digest;
@@ -364,10 +366,16 @@ dirserv_get_status_impl(const char *id_digest, const char *nickname,
   }
 
   if (!authdir_policy_permits_address(addr, or_port)) {
-    log_fn(severity, LD_DIRSERV, "Rejecting '%s' because of address '%s'",
-               nickname, fmt_addr32(addr));
-    if (msg)
-      *msg = "Authdir is rejecting routers in this range.";
+    log_fn(severity, LD_DIRSERV,
+           "Rejecting '%s' because of address '%s' (contactinfo %s)",
+           nickname, fmt_addr32(addr),
+           contact_info ? escaped(contact_info) : "not set");
+    if (msg) {
+      if (!contact_info)
+        *msg = "Suspicious relay, no ContactInfo set -- please contact us?";
+      else
+        *msg = "Authdir is rejecting routers in this range.";
+    }
     return FP_REJECT;
   }
   if (!authdir_policy_valid_address(addr, or_port)) {

(It didn't really accomplish my goal in this case, since all the jerk relays I was trying to reach had set their ContactInfo -- but set it to something generic and useless.)

comment:10 Changed 4 years ago by arma

Status: newneeds_review

(Oh, and in the case where I want to reach a specific relay with a specific message, I can just patch dirserv_get_status_impl() on moria1 to special-case that relay. Not a great solution for real engineers, but it will get the job done.)

comment:11 Changed 3 years ago by nickm

Keywords: 029-accepted added; 029-proposed removed
Milestone: Tor: 0.2.???Tor: 0.2.9.x-final

These have code, so I am okay with reviewing them for this release. (no objections at today's meeting)

comment:12 Changed 3 years ago by arma

Great.

I still don't have a good email address to put in.

(We have systematically removed most ways to contact us over the past few months, and the remaining ones are under increasing pressure to be removed too.)

We could point to a url like https://www.torproject.org/about/contact but I think that's pretty easy to find with your favorite search engine too.

I'm inclined to not include specifics about where to contact us.

comment:13 Changed 3 years ago by nickm

Owner: set to arma
Status: needs_reviewassigned

comment:14 Changed 3 years ago by arma

Status: assignedneeds_review

comment:15 Changed 3 years ago by nickm

Keywords: review-group-1 added

comment:16 Changed 3 years ago by nickm

Status: needs_reviewneeds_revision

Looks good then. Please get me a changes file, and let me know whether you want a backport to 0.2.7.

comment:17 Changed 3 years ago by nickm

Reviewer: nickm

comment:18 Changed 3 years ago by arma

Status: needs_revisionmerge_ready

I used my time machine to include a changes file in the commit that you reviewed.

No backport to 0.2.7 required. In fact, this has a milestone of 0.2.9, so I would be fine with no backport to 0.2.8 also.

comment:19 Changed 3 years ago by nickm

Resolution: implemented
Status: merge_readyclosed

Okay. Merged it to 0.2.9. Thanks!

comment:20 Changed 3 years ago by arma

Keywords: TorCoreTeam201605 added
Note: See TracTickets for help on using tickets.