#27550 closed defect (fixed)

hs-v3: Don't warn so loudly when tor is unable to decode a descriptor

Reported by: dgoulet Owned by:
Priority: Medium Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-hs
Cc: Actual Points:
Parent ID: #27544 Points:
Reviewer: asn Sponsor:

Description

With #20700, we introduce client authorization making tor client without it trying to access a .onion with it to be unable to decode the descriptor. This leads to big warnings:

Sep 07 13:55:44.156 [info] handle_response_fetch_hsdesc_v3(): Received v3 hsdesc (body size 14111, status 200 ("OK"))
Sep 07 13:55:44.157 [warn] Encrypted service descriptor MAC check failed
Sep 07 13:55:44.157 [warn] Decrypting encrypted desc failed.
Sep 07 13:55:44.157 [warn] Service descriptor decryption failed.
Sep 07 13:55:44.157 [warn] Could not parse received descriptor as client.
...

We should definitely not print warning if decoding fails but maybe a "unable to use descriptor" instead and the rest at info level.

Second, there is the retry behavior. Two cases:

1) Tor is configured with client authorization for A.onion:

If we get the descriptor and unable to decode A.onion while we know we have a client authorization configured, I think we should make Tor stop and just tell the user that it didn't worked.

2) Tor doesn't have client authorization for A.onion

In that case, if the decoding fails, we should *probably* make Tor stop trying on all HSDir and instead go at notice level saying "Unable to access A.onion. Maybe you need authorization?" kind of message.

Failing to decode a descriptor now is imo highly unlikely so we could assume that in this case, chances are that you'll get a better descriptor at the next HSDir are thin!

Child Tickets

Change History (15)

comment:1 Changed 14 months ago by dgoulet

Status: newneeds_review

Branch: bug27550_035_01

Notice the added logs can't use the .onion address which is kind of annoying but we would need to refactor quite a bit the HS API there to propagate the address for logging purposes.

comment:2 Changed 14 months ago by dgoulet

Summary: hs-v3: Don't warn so loundly when tor is unable to decode a descriptorhs-v3: Don't warn so loudly when tor is unable to decode a descriptor

comment:3 Changed 14 months ago by asn

Reviewer: asn

comment:4 Changed 14 months ago by asn

Review:

Why did you demote the following log msg to info? I think it has nothing to do with client auth:

-    log_warn(LD_REND, "Length of descriptor's encrypted data is too small. "
+    log_info(LD_REND, "Length of descriptor's encrypted data is too small. "

Other than that, LGTM.

comment:5 Changed 14 months ago by dgoulet

Because it can be triggered by anyone uploading a malicious or malformed descriptor. It made sense to me that if we move some decoding warnings to info then better to move them all?

Last edited 14 months ago by dgoulet (previous) (diff)

comment:6 Changed 14 months ago by dgoulet

Pushed fixup commit for the above after IRC discussion: 49e4bda50bdf723d6cdc8e7a0fb24619fded5f2c

comment:7 Changed 14 months ago by asn

Status: needs_reviewmerge_ready

LGTM!

comment:8 Changed 14 months ago by nickm

changes file please? :)

comment:9 in reply to:  8 Changed 14 months ago by dgoulet

Replying to nickm:

changes file please? :)

You got it now! Warning, fixup commits in that branch! :)

comment:10 Changed 14 months ago by dgoulet

asn unsure about backport. I'm for it if this can avoid useless warnings client side. We can maybe not backport for now and if it becomes an annoyance for users on 033/034, we can then reconsider.

comment:11 Changed 14 months ago by nickm

If this is a maybe-backport-someday, then let's have the branch based on maint-0.3.? so that the backport isn't hard if we decide to do it?

comment:12 Changed 14 months ago by dgoulet

Branch: bug27550_032_01 for 032
Branch: bug27550_033_01 for 033 and 034
Branch: bug27550_035_01 for 035.

comment:13 Changed 14 months ago by nickm

Milestone: Tor: 0.3.5.x-finalTor: 0.3.4.x-final

Merging to 0.3.5; marking for possible backport.

comment:14 Changed 14 months ago by nickm

Keywords: 035-must removed

Already done in 035, so not 035-must.

comment:15 Changed 12 months ago by nickm

Milestone: Tor: 0.3.4.x-finalTor: 0.3.3.x-final
Resolution: fixed
Status: merge_readyclosed

merged to 0.3.3 and forward: risk is low, and it seems not to have broken anything.

Note: See TracTickets for help on using tickets.