Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#18761 closed defect (fixed)

Make logging of rendezvous to private address quieter

Reported by: teor Owned by: nickm
Priority: Medium Milestone: Tor: 0.2.8.x-final
Component: Core Tor/Tor Version: Tor: 0.2.8.2-alpha
Severity: Normal Keywords: must-fix-before-028-rc, tor-hs, TorCoreTeam201605, TorCoreTeam-postponed-201604, review-group-1
Cc: Actual Points: very small
Parent ID: Points: 1
Reviewer: asn Sponsor:

Description

In #8976, we blocked rendezvous from hidden services to private addresses.

But the warning is log_warn LD_REND, when arma wonders if it should be a protocol warning to avoid excessive logging on busy services:

  log_warn(LD_REND, "%s on circ %u", err_msg,
           (unsigned)circuit->base_.n_circ_id);

There's also a whitespace issue we might as well fix while we're there:

  if (err_msg_out) *err_msg_out = err_msg;
  else tor_free(err_msg);

Child Tickets

Change History (13)

comment:1 Changed 4 years ago by nickm

Keywords: TorCoreTeam201604 added
Owner: set to nickm
Status: newaccepted

let's make it rate-limited.

comment:2 Changed 4 years ago by nickm

Status: acceptedneeds_review

let's make it rate-limited.

Actually let's not. PROTOCOL_WARN is fine. See branch bug18761_028. Please review? Very small patch; review should be fast if we don't start painting the bikeshed.

comment:3 Changed 4 years ago by nickm

Actual Points: very small

comment:4 Changed 4 years ago by nickm

Keywords: TorCoreTeam201605 added

Calling all non-needs_information tickets for May.

comment:5 Changed 4 years ago by nickm

Keywords: TorCoreTeam-postponed-201604 added; TorCoreTeam201604 removed

April is over; calling these april tickets postponed into may.

comment:6 Changed 4 years ago by asn

Reviewer: asn

comment:7 Changed 4 years ago by arma

Bug:

-  log_warn(LD_REND, "%s on circ %u", err_msg,
+  log_warn(err_msg_severity, "%s on circ %u", err_msg,
            (unsigned)circuit->base_.n_circ_id);

this wants to be tor_log(err_msg_severity)

comment:8 Changed 4 years ago by arma

Also tor_log wants a log domain (LD_REND), so let's not lose that one either.

comment:9 Changed 4 years ago by nickm

bug18761_028 now has a fix for that. Ready for review again.

comment:10 Changed 4 years ago by nickm

Keywords: review-group-1 added

comment:11 Changed 4 years ago by asn

Status: needs_reviewmerge_ready

LGTM.

Might be worth pointing out that this patch will also suppress the following log message:

    node = node_get_by_nickname(rp_nickname, 0);
    if (!node) {
      if (err_msg_out) {
        tor_asprintf(&err_msg,
                     "Couldn't find router %s named in INTRODUCE2 cell",
                     escaped_safe_str_client(rp_nickname));
      }

      goto err;
    }

Are we OK with hiding this from HS operators? I guess yes because it's not like they can do anything about it (and it's probably caused by consensus desynchronization between client and service).

comment:12 Changed 4 years ago by nickm

Resolution: fixed
Status: merge_readyclosed

Agreed. Merged. Thanks!

comment:13 Changed 4 years ago by isabela

Points: small1
Note: See TracTickets for help on using tickets.