[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #18620 [Core Tor/Tor]: HSFORGET command to clear cached client state for a HS
#18620: HSFORGET command to clear cached client state for a HS
-------------------------------------------------+-------------------------
Reporter: str4d | Owner: str4d
Type: enhancement | Status:
Priority: Medium | needs_revision
Component: Core Tor/Tor | Milestone: Tor:
Severity: Normal | 0.2.9.x-final
Keywords: tor-hs, 029-accepted, review- | Version: Tor:
group-1 | 0.2.7.6
Parent ID: | Resolution:
Reviewer: asn | Actual Points:
| Points: small
| Sponsor:
| SponsorR-can
-------------------------------------------------+-------------------------
Changes (by asn):
* status: needs_review => needs_revision
Comment:
Hello and sorry for the slow review! This seems like a great feature for
mobile devices, and the patch seems to be almost there. The main issue is
the unittest not passing.
Here are some comments:
- Your tests don't work because your descriptor string was prepared for
the control port tests. If you remove the carriage returns "\r" characters
from the end of each line then the parsing should proceed.
However, that's not enough for it to go through
`rend_cache_store_v2_desc_as_client()`. You also need to fix up the desc
id which currently is not correct, and you also need a dynamic descriptor
timestamp to bypass the age check. I had to uncomment the following checks
to complete the test (which passes fine btw):
{{{
/* if (parsed->timestamp < now - REND_CACHE_MAX_AGE-REND_CACHE_MAX_SKEW)
{ */
/* printf( "Service descriptor with service ID %s is too old.", */
/* safe_str_client(service_id)); */
/* goto err; */
/* } */
}}}
{{{
+ /* if (tor_memneq(desc_id, want_desc_id, DIGEST_LEN)) { */
+ /* printf( "Received service descriptor for %s with incorrect " */
+ /* "descriptor ID (%s).", service_id, want_desc_id); */
+ /* goto err; */
+ /* } */
}}}
To debug this, I turned all the `log_warn()`s in the parsing functions to
`printf`s, and then I could see the offending error messages when I ran
the tests.
- I think there is no need to add the base64 padding yourself in
`descriptor_cookie_base64ext()`. Instead you can use
`base64_decode_nopad()`. I think this should work, but there were no tests
about descriptor cookies so I'm not 100% sure.
- Also, in `rend_cache_remove_entry()` there is no need to handle v0
descriptors. These have been deprecated, feel free to kill that code.
- The patch does not compile with `./configure --enable-gcc-warnings`
because you have a few unused function arguments in the tests. You can
solve this by doing `(void) arg;`.
- Also, it would be great if you could provide a git branch based on the
current master, instead of a standalone patch because I had trouble
merging it. Feel free to upload it ot any repo (github, gitlab) or even
just post a detached git-bundle. If that's too hard, no worries.
- Finally, I noticed that in `rend_client_note_connection_attempt_ended()`
we do:
{{{
if (cache_entry != NULL) {
SMARTLIST_FOREACH(cache_entry->parsed->intro_nodes,
rend_intro_point_t *, ip,
ip->timed_out = 0; );
}
}}}
Do you think that would be useful for this patch?
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/18620#comment:15>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs