Controller: add a command to fetch HS descriptor from HSdir(s)

changed milestone to %Tor: 0.2.7.x-final

added 027-triaged-1-in SponsorS component::core tor/tor controller milestone::Tor: 0.2.7.x-final nickm-review owner::dgoulet parent::3521 points::small-remaning priority::medium resolution::implemented sponsor::R status::closed tor-hs type::enhancement version::tor 0.2.7 labels

First part is to review and accept the specification for a new command in the control-spec.

See branch ticket14847_01 in https://git.torproject.org/user/dgoulet/torspec.git for the patch adding the HSFETCH command.

Trac:
Status: new to needs_review

Suggestions:

the server should be specified with digest only; nicknames are deprecated.
I'd recommend server=HEXDIGEST rather than just HEXDIGEST; that way, other extensions are easier in the future.

Replying to nickm:

Suggestions:

the server should be specified with digest only; nicknames are deprecated.

I'd recommend server=HEXDIGEST rather than just HEXDIGEST; that way, other extensions are easier in the future.

I actually used "Fingerprint" which is already defined in the spec for the "server=" value.

Fixed both in branch ticket14847_02.

Open question, should this command also trigger an HSDESC event? I think not because this command should be "self contained".

I think maybe it should; and probably it should be nonblocking. (Current designs in control world are that commands which might not finish immediately, and which cause Tor to hit the network, need to not block for the thing to finish.)

Replying to nickm:

I think maybe it should; and probably it should be nonblocking. (Current designs in control world are that commands which might not finish immediately, and which cause Tor to hit the network, need to not block for the thing to finish.)

This is not intended to block since HSDir fetch is asynchronous anyway. That means probably that "250-" replies should be replaced by "650-" then?

There should be a 250 OK immediately and a 6xx later. Not sure what the xx is. All other 6xx replies are sent only in response to a SETEVENTS.

haven't looked at the branch yet, but, i think a controller command to initiate the fetch, and if you want to see the answer you should listen for hsdesc events, is a totally reasonable design.

Replying to arma:

haven't looked at the branch yet, but, i think a controller command to initiate the fetch, and if you want to see the answer you should listen for hsdesc events, is a totally reasonable design.

That means no descriptor content dump but maybe that's fine considering that you can ask that the fetch results are cached and then use #14845 (moved) to get the content. Sounds much more simpler!

Ok, with all the comments above, here is a much simpler version.

See branch ticket14847_03.

Hi David, nice spec! I'm looking forward to finally having a reason to add HS Descriptor parsing support to Stem.

"HSFETCH" SP HSAddress *(SP "server=" Server) (SP "cache=" Cache) CRLF

Please use square brackets. That's what we usually do for optional keyword arguments...

https://gitweb.torproject.org/user/dgoulet/torspec.git/tree/control-spec.txt?h=ticket14847_03&id=88245a020fec7b7b4e20ab64adf99784ec4a3dcd#n2307

Also, lets use all caps for 'SERVER=' and 'CACHE='.

If one or more Server are given, they are used instead.

What happens when you specify multiple? Does it pick among them randomly?

If Cache is specified, the value "yes" means that the result will be
cached on the client.

Cached for how long? Permanently? Or do HS descriptors have a valid-until date? Can the cache be cleared?

The HS_DESC event should be used to get
the results of the fetches.

How long does it take to retrieve a hidden service descriptor in practice? This is a lot clunkier for controllers. How about a 'BLOCKING=n' call for "I'm willing to wait up to n ms to get this descriptor"?

Replying to dgoulet:

Ok, with all the comments above, here is a much simpler version.

See branch ticket14847_03.

What's the reasoning for the cache=yes or cache=no part? That is, why not just let rend_cache_store_v2_desc_as_client(() look at the hsdesc you get back and decide whether to keep it based on whatever rules it uses now ("I don't have a newer one, etc")?

I think to do the async thing here we'll want to extend the HSDESC event to just tell you the descriptor right then. Otherwise there's a three step process ("initiate the launch", "notice the event", "getinfo the response"), and if you initiate a bunch of launches, and then get a bunch of events, you'll only be able to getinfo one of them, and you might not even know which one it was, etc.

(Whether you extend the HSDESC event always, or add a separate HSDESC_AND_DUMP event, or what, is a matter of taste that I will leave to you and Nick if you like this approach.)

Now that I think about it, there may also be some adventure here with all of the implicit "oh a failure just happened, that means I should launch this other action" logic in hsdesc fetches. Maybe this is a good time to clean up some of that logic, or maybe it will turn out to be easier than we think to work with it. Or I guess option three is that this will just be no fun. :)

Replying to atagar:

{{{ If one or more Server are given, they are used instead. }}}

What happens when you specify multiple? Does it pick among them randomly?

I kind of thought that it would cause Tor to initiate multiple fetches, one from each.

But, good question. I wonder if that feature is valuable enough for the complexity, compared to just making the controller send you one HSFETCH per fetch you want it to launch.

Or maybe David did indeed mean to choose just one.

{{{ The HS_DESC event should be used to get the results of the fetches. }}}

How long does it take to retrieve a hidden service descriptor in practice? This is a lot clunkier for controllers. How about a 'BLOCKING=n' call for "I'm willing to wait up to n ms to get this descriptor"?

That's exactly what we've been heading away from with the async approach. That said, David, it would indeed be nice to give the controller writers some guess about how long they might need to wait until they see their HSDESC received or failed. I think the answer is "it's like fetching a thing over the Tor network -- typically pretty fast, but sometimes 5 to even 60 seconds."

That's exactly what we've been heading away from with the async approach.

Present world for descriptor fetching is...

Controllers can make a simple, synchronous request to read cached descriptors.
Scripts can contact a dirauth's DirPort to actively fetch the fresh thing.

This is nice. It means scripts can piggyback on a cache or download what they need, and in either case it's simple and synchronous.

If we go with an asynchronous approach the first method people will want is a simple blocking 'I want a hidden service descriptor, give it to me' method. If it doesn't live in tor it'll be in stem and that's fine. We already do something similar with creating circuits - tor doesn't provide a blocking method so stem adds a listener and waits for the event indicating that it's done...

https://gitweb.torproject.org/stem.git/tree/stem/control.py#n2705

Just makes for a more interesting dance on my end.

Please be very, very careful though that a HSDESC is always emitted, 1:1, with a call of this method. If there's any use case where the controller doesn't get either a success or failure message it'll be left hanging indefinitely.

Replying to arma:

Replying to dgoulet:

Ok, with all the comments above, here is a much simpler version.

See branch ticket14847_03.

What's the reasoning for the cache=yes or cache=no part? That is, why not just let rend_cache_store_v2_desc_as_client(() look at the hsdesc you get back and decide whether to keep it based on whatever rules it uses now ("I don't have a newer one, etc")?

The original idea was to give a choice to the user to keep the fetched descriptor or not. However, if we go with a new HSDESC_* event to dump the content when it arrives, the "cache=" part could be removed and by default keeps the latest.

I think to do the async thing here we'll want to extend the HSDESC event to just tell you the descriptor right then. Otherwise there's a three step process ("initiate the launch", "notice the event", "getinfo the response"), and if you initiate a bunch of launches, and then get a bunch of events, you'll only be able to getinfo one of them, and you might not even know which one it was, etc.

(Whether you extend the HSDESC event always, or add a separate HSDESC_AND_DUMP event, or what, is a matter of taste that I will leave to you and Nick if you like this approach.)

I'm not too familiar what are the best practices but could we do something like this with the EXTENDED events feature?

C: SETVENTS HS_DESC DUMP=yes
[...]
S: 650 HS_DESC RECEIVED ...
S: 650 HS_DESC EXTENDED DUMP xyz.onion
<dump here>

Now that I think about it, there may also be some adventure here with all of the implicit "oh a failure just happened, that means I should launch this other action" logic in hsdesc fetches. Maybe this is a good time to clean up some of that logic, or maybe it will turn out to be easier than we think to work with it. Or I guess option three is that this will just be no fun. :)

I know... this is why I want this command accepted asap so I can start working on it. The HS fetch code is very "monolithic" in a way that it's a big block that does a lot of diffrent things. It would need to be much more modularized so we can cherry-pick the actions we need for this command and not really go through the normal process of fetching a descriptor right now.

Replying to arma:

Replying to atagar:

{{{ If one or more Server are given, they are used instead. }}}

What happens when you specify multiple? Does it pick among them randomly?

I kind of thought that it would cause Tor to initiate multiple fetches, one from each.

But, good question. I wonder if that feature is valuable enough for the complexity, compared to just making the controller send you one HSFETCH per fetch you want it to launch.

Or maybe David did indeed mean to choose just one.

"They are used instead", I meant by that if there are more than one Server specified, they are all used and a fetch is triggered on all of them

{{{ The HS_DESC event should be used to get the results of the fetches. }}}

How long does it take to retrieve a hidden service descriptor in practice? This is a lot clunkier for controllers. How about a 'BLOCKING=n' call for "I'm willing to wait up to n ms to get this descriptor"?

That's exactly what we've been heading away from with the async approach. That said, David, it would indeed be nice to give the controller writers some guess about how long they might need to wait until they see their HSDESC received or failed. I think the answer is "it's like fetching a thing over the Tor network -- typically pretty fast, but sometimes 5 to even 60 seconds."

Yup, "few seconds" up to a dir request timeout of ? (I don't know the value here). Should be added to the spec!

Ok I took a stab at it so we can go forward. Pretty sure this is not the "silver bullet" we are looking for but I think it's a good start considering the previous discussion.

I've basically added a new event called HS_DESC_CONTENT and removed the cache= part of the HSFETCH command. Also, I fixed the issues raised by atagar in comment:10.

See branch ticket14847_04 in https://git.torproject.org/user/dgoulet/torspec.git

Good discussion with arma and weasel on IRC, here is the new branch fixing what has been discussed.

See branch ticket14847_05 in https://git.torproject.org/user/dgoulet/torspec.git

This new version adds the DescID, Replica and TimePeriod option to the HSFETCH command. After a discussion on IRC with arma, turns out it would be very useful to have the ability to control these.

See branch ticket14847_06 in https://git.torproject.org/user/dgoulet/torspec.git

DescId = 32*Base32Character

Out of curiosity where do hidden service descriptor ids reside? That is to say, what are the use cases where someone will have one of these ids and want to look it up?

TimePeriod = Specified in rend-spec.txt section 1.3.

Please say what this actually is. I'm sure it's discussed in there, but it's actually not a well defined field in the rend-spec...

% grep TimePeriod rend-spec.txt | wc -l
0

+ It is also possible to specify a TimePeriod and/or a Replica value for an
+ HSAddress. They can't be used with a DescId because they are part of a
+ descriptor id for an HSAddress. A 552 error is returned in that case.

Admittedly I'm not overly familiar with hidden services, but I'm not sure what this paragraph means or what the fields do.

+ Examples are:
+ C: HSFETCH v2-gezdgnbvgy3tqolbmjrwizlgm5ugs2tl
+ S: 250 OK

Oh, I appreciate the examples - thanks for including that!

Replying to atagar:

{{{ DescId = 32*Base32Character }}}

Out of curiosity where do hidden service descriptor ids reside? That is to say, what are the use cases where someone will have one of these ids and want to look it up?

One use case we have for this is to create a tool that will help us measure the impact of relay churn on HS reachability. With a descriptor id, we can use it to fetch HSdir from a couple consensus back in time and track movement of it. It's not entirely clear if it will help but our first assessment is yes. See #13209 (moved) for a bit more info.

{{{ TimePeriod = Specified in rend-spec.txt section 1.3. }}}

Please say what this actually is. I'm sure it's discussed in there, but it's actually not a well defined field in the rend-spec...

Yeah, it's "time-period" field in the rend-spec so I will add that to the description. The only thing I want to avoid here is to copy-paste text between spec document because if one changes, chances are we forgot the other one.

{{{

It is also possible to specify a TimePeriod and/or a Replica value for an

HSAddress. They can't be used with a DescId because they are part of a

descriptor id for an HSAddress. A 552 error is returned in that case. }}}

Admittedly I'm not overly familiar with hidden services, but I'm not sure what this paragraph means or what the fields do.

TimePeriod, Replica and HSAddress are all used to compute a descriptor ID. Thus if you provide a descriptor ID, they are useless. From your native english speaker perspective, how can I phrase that better? :)

{{{

Examples are:

C: HSFETCH v2-gezdgnbvgy3tqolbmjrwizlgm5ugs2tl

S: 250 OK }}}

Oh, I appreciate the examples - thanks for including that!

Yeah I figured, the definition of this command went from VERY simple to omgbbqwtf-medium-rare.

Yeah, it's "time-period" field in the rend-spec so I will add that to the description. The only thing I want to avoid here is to copy-paste text between spec document because if one changes, chances are we forgot the other one.

That makes sense. What I care about from a controller perspective is 'what does this field look like?'. That is to say, is it an iso timestamp, unix timestamp, whatever. From the example below seems it's the later.

TimePeriod, Replica and HSAddress are all used to compute a descriptor ID. Thus if you provide a descriptor ID, they are useless. From your native english speaker perspective, how can I phrase that better? :)

Nah, the language is fine. I mean, I'm unsure what these arguments do. Lets say I make the request in your second example...

HSFETCH ajkhdsfuygaesfaa
SERVER=9695DFC35FFEB861329B9F1AB04C46397020CE31
TIMEPERIOD=424344
REPLICA=1

I got that I'm fetching the descriptor for ajkhdsfuygaesfaa.onion from HsDir 9695DFC35FFEB861329B9F1AB04C46397020CE31. Guessing the TIMEPERIOD argument means 'the descriptor that was valid at unix timestamp 424344'? And replica... well, not sure what part means. :)

No doubt if I read through the rend-spec all would make sense. All I mean is that it would be nice if this spec addition went a little further in explaining what these arguments do. :)

Ok, again, more talk with arma on it. We agreed that the complexity was getting a bit out of hand. However, having the DescId is useful so Replica and Timeperiod are removed. The DescId is also added to the HS_DESC_CONTENT event.

See branch ticket14847_07 in https://git.torproject.org/user/dgoulet/torspec.git

I'll try to post asap the code for this because we are getting at a point where we actually agree on something and the command is fairly simple. The little-t tor code will need refactoring thus much review.

Ok, that was much more difficult than I anticipated and I expect quite a bit of rounds of review. Below are both the latest control-spec patch and little-t tor code. Make sure to read the commit message, they will be useful :).

See branch ticket14847_09 in https://git.torproject.org/user/dgoulet/torspec.git Code: branch bug14847_027_02 in my personal repo.

Hi dgoulet, looks good!

The HS_DESC and HS_DESC_CONTENT events should be used to get the results of the fetch(es).

Does the controller have a strong guarantee that if I run HSFETCH and it returns OK I'll always be assured to receive a HS_DESC_CONTENT for it? This is important since the usual way many controllers will use this is...

subscribe to HS_DESC and HS_DESC_CONTENT
call HSFETCH
wait until we receive the HS_DESC or HS_DESC_CONTENT for it
return the value or raise an exception

That is to say, most people want a synchronous method so they'll make a helper method to do it for 'em.

If we have a strong guarantee then great! We should say so. If we don't then that's... less great and should say so. ;)

I'm not sure if it's accurate but maybe something like the following?

"If this replies with "250 OK" then Tor MUST eventually follow this with both a HS_DESC and HS_DESC_CONTENT event with the results. If SERVER is specified then events are emitted for each location."

Thus resulting in two consecutive SP for any other actions.

Please instead fill in a value ('UNKNOWN' for instance). You list HSAddress as being a mandatory positional field so omitting it is problematic.

Replying to atagar:

Hi dgoulet, looks good!

The HS_DESC and HS_DESC_CONTENT events should be used to get the results of the fetch(es).

Does the controller have a strong guarantee that if I run HSFETCH and it returns OK I'll always be assured to receive a HS_DESC_CONTENT for it? This is important since the usual way many controllers will use this is...

subscribe to HS_DESC and HS_DESC_CONTENT

call HSFETCH

wait until we receive the HS_DESC or HS_DESC_CONTENT for it

return the value or raise an exception

That is to say, most people want a synchronous method so they'll make a helper method to do it for 'em.

If we have a strong guarantee then great! We should say so. If we don't then that's... less great and should say so. ;)

I'm not sure if it's accurate but maybe something like the following?

"If this replies with "250 OK" then Tor MUST eventually follow this with both a HS_DESC and HS_DESC_CONTENT event with the results. If SERVER is specified then events are emitted for each location."

Yes. Strong guarantee! :) Will add your suggestion.

Thus resulting in two consecutive SP for any other actions.

Please instead fill in a value ('UNKNOWN' for instance). You list HSAddress as being a mandatory positional field so omitting it is problematic.

I'm not sure to understand the "UNKNOWN" here :S...

Should the line be changed to something like ?

"650" SP "HS_DESC" SP Action SP [HSAddress] SP AuthType SP HsDir [SP DescriptorID]
    [SP "REASON=" Reason]

and thus justifying the double SP if no HSAddress?

Nope, I mean change...

HSAddress = 16*Base32Character

... to...

HSAddress = 16*Base32Character / "UNKNOWN"

That's all.

I've added Stem support for HS_DESC_CONTENT events. This helped highlight a few issues...

Events need to start with '650+HS_DESC_CONTENT' and end with a '650 OK'. The example DonnchaC had lacked both, causing Stem to treat it as a single line event (... which is actually precisely right according to the spec). For an example of what a multi-line event should look like see this.
The spec has 'HsDir = Fingerprint' but it's actually a LongName. Just need a spec tweak so we match HS_DESC: "HsDir = LongName / Fingerprint".
When implementing this I assumed HSAddress will be 'UNKNOWN' if undefined (see earlier comment).

Other than that looks good!

I think an extra new line is being added at the end of the descriptor. I'm not sure if the problem is with the control port command or with Stem but it seems to be causing an issue with Stem reading the HS_DESC_CONTENT response.

Replying to atagar:

I've added Stem support for HS_DESC_CONTENT events. This helped highlight a few issues...

Events need to start with '650+HS_DESC_CONTENT' and end with a '650 OK'. The example DonnchaC had lacked both, causing Stem to treat it as a single line event (... which is actually precisely right according to the spec). For an example of what a multi-line event should look like see this.

Fixed!

The spec has 'HsDir = Fingerprint' but it's actually a LongName. Just need a spec tweak so we match HS_DESC: "HsDir = LongName / Fingerprint".

Fixed!

When implementing this I assumed HSAddress will be 'UNKNOWN' if undefined (see earlier comment).

Yes, if the HSFETCH fails, the HS_DESC_CONTENT will print an "UNKNOWN". Fixed in control-spec/code.

Other than that looks good!

You can find all the fixes in the following branches:

Spec: ticket14847_10 Code: bug14847_027_03

Replying to donncha:

I think an extra new line is being added at the end of the descriptor. I'm not sure if the problem is with the control port command or with Stem but it seems to be causing an issue with Stem reading the HS_DESC_CONTENT response.

There is an extra line at the end of an HS descriptor. The rend-spec.txt doesn't mention it, I have a todo to submit a patch for it.

Following that, you have "\r\n." which is part of the control-spec as "escaped data".

As promised, here's the review.

Spec:

552 isn't the only failure code returned.
The DescId MUST have at least once Server specified else a 552 error is returned. feels awkward/confusing. If a DescId is specified, at least one Server MUST also be provided, otherwise a 552 error is returned..
If no Server are speficied should be If no DescId and Server(s) are specified.
The caching behavior of a fetched descriptor using this command is unchanged that is it's the same as the Tor client. should be something like The caching behavior when fetching a descriptor using this command is identical to normal Tor client behavior., if that's what you meant, otherwise clarification needed.
Detail on how to should be Details on how to.
If any values is unrecognized should be If any values are unrecognized.
Is there a good reason to specify the length of a HSAddress? This will require revision once Prop. 224 comes into play.
"650" SP "HS_DESC_CONTENT" SP HSAddress SP DescId SP HsDir CRLF should be "650" "+" "HS_DESC_CONTENT" SP HSAddress SP DescId SP HsDir CRLF per comment 29.
Behavioral question. If I issue 2 identical "HSFETCH" commands in rapid succession, how many events do I get? The spec implies that tor will send me duplicate events but that's not the case.

Branch:

New code:
- src/common/util.c:string_is_hex():
  - unsigned int i; -> size_t i;
  - I suggest adding a tor_assert(string);.
  - Consider return strlen(string) == strspn(string, HEX_CHARACTERS); instead of the loop.
- src/or/control.c:handle_control_hsfetch():
  - Use rend_valid_service_id() to check HS address validity.
  - When checking arg1 to see if it is a v2 descriptor ID, use strcmpstart() instead of strstr().
  - Instead of removing out arg1, just so you can use SMARTLIST_FOREACH, for (int i = 1; i < smartlist_len(args); ++i) followed by smartlist_get(args, i) will suffice (and is consistent with the rest of the control code).
  - The standard status code for unrecognized/unexpected arguments is 513, not 552.
  - The standard status code for missing arguments is 512, not 552.
  - smartlist_free() is fine with the list being NULL, there's no reason to check if hsdirs is valid.
  - (Style) I for one welcome our new C99 overlords, moving variable declarations closer to the code might make sense.
  - (Style) You can just return 0; if getargs_helper() returns NULL since there is 0 cleanup to be done. See above regarding moving variable declarations around.
  - (Style) if (hsaddress) { ... } else { tor_assert(desc_id); } instead of a branch containing an assert.
- src/or/control.c:control_event_hs_descriptor_content():
  - (Behavior bikeshedding) If I did this, I would have allowed NULL content, since the dot encoded form of that is ".\r\n", and is trivial to special case.
Changes to old code:
- src/or/rendclient.c:lookup_last_hid_serv_request():
  - rend_query should be removed from the Doxygen comment.
- src/or/rendclient.c:directory_get_from_hs_dir():
  - pick_hsdir() could/should take desc_id_base32 as an argument. The code is encoding it twice in rapid succession.
- src/or/rendclient.c:rend_client_refetch_v2_renddesc():
  - No longer need to log a warning when you fail to compute the v2 rend descriptor ID?
- src/or/directory.c:connection_dir_client_reached_eof()
  - memwipe() service_id. Some of the rend code appears to treat that as sensitive material, so I think it should be sanitized.

The rest of the refactor looks ok, sorry if the bulk of the things are nitpicky/personal taste.

Trac:
Status: needs_review to needs_revision

Big thanks Yawning for that thorough review!

Most of it has been fixed and pushed here. Will probably update those branches once I have some answers to questions below.

Spec: ticket14847_11 Code: bug14847_027_04

Here are some comments on the "C code philosophical part" and also technical. :)

src/common/util.c:string_is_hex():

This function is not used anymore so I removed the commit entirely.

Behavioral question. If I issue 2 identical "HSFETCH" commands in rapid succession, how many events do I get? The spec implies that tor will send me duplicate events but that's not the case.

Weird, one HSFETCH should trigger two events, HS_DESC and HS_DESC_CONTENT. Thus in your case 4 events. I just tested multiple times and I actually have the right amount. Maybe that's worth investigating more if you can reproduce that. Maybe if no more hsdirs are usable, we loose events?

Use rend_valid_service_id() to check HS address validity.

Fixed! Added a patch on top.

Instead of removing out arg1, just so you can use SMARTLIST_FOREACH, for (int i = 1; i < smartlist_len(args); ++i) followed by smartlist_get(args, i) will suffice (and is consistent with the rest of the control code).

Not sure I understand this one. I don't see this being used in control.c. Can you clarify the loop you have in mind for me to skip the first arg?

(Style) You can just return 0; if getargs_helper() returns NULL since there is 0 cleanup to be done. See >above regarding moving variable declarations around

Personally, I try to have only one return call site with goto labels for cleanup. In this case, I would prefer an exit: label instead of adding a return. I'm not strongly fighting against that tbh, I prefer consistent and maintainable code over stylish vanity thus here I would probably hand it over to the maintainer for a decision? :)

(Style) if (hsaddress) { ... } else { tor_assert(desc_id); } instead of a branch containing an assert.

I strongly advocate against if/else if statement that DOES NOT have a final else. Over time, that's only error prone but sometimes you don't have a choice thus a nice comment to explain why is important.

In this case, I went for the "else tor_assert" because desc_id should be treated as a single value and not a "fallback". Assuming future code that might add an other variable, we'll end up splitting the else statement and breaking the git blame :(. It's so much more easier to read an if/else statement when the value being tested is actually the one being worked on (in the case of input parsing).

src/or/rendclient.c:rend_client_refetch_v2_renddesc(): No longer need to log a warning when you fail to compute the v2 rend descriptor ID?

I'm not seeing any log warnings in that function about that. Maybe you had an other function in mind?

Replying to dgoulet:

Behavioral question. If I issue 2 identical "HSFETCH" commands in rapid succession, how many events do I get? The spec implies that tor will send me duplicate events but that's not the case.

Weird, one HSFETCH should trigger two events, HS_DESC and HS_DESC_CONTENT. Thus in your case 4 events. I just tested multiple times and I actually have the right amount. Maybe that's worth investigating more if you can reproduce that. Maybe if no more hsdirs are usable, we loose events?

Hmm, my mistake here, I misread the code.

Use rend_valid_service_id() to check HS address validity.

Fixed! Added a patch on top.

Instead of removing out arg1, just so you can use SMARTLIST_FOREACH, for (int i = 1; i < smartlist_len(args); ++i) followed by smartlist_get(args, i) will suffice (and is consistent with the rest of the control code).

Not sure I understand this one. I don't see this being used in control.c. Can you clarify the loop you have in mind for me to skip the first arg?

handle_control_closecircuit() has an example of what I have in mind:

    for (i=1; i < smartlist_len(args); ++i) {
      if (!strcasecmp(smartlist_get(args, i), "IfUnused"))
        safe = 1;
      else
        log_info(LD_CONTROL, "Skipping unknown option %s",
                 (char*)smartlist_get(args,i));
    }

Going through the trouble to remove the first arg, just to use an iteration macro, when a simple loop works just as well seems a bit excessive.

(Style) You can just return 0; if getargs_helper() returns NULL since there is 0 cleanup to be done. See >above regarding moving variable declarations around

Personally, I try to have only one return call site with goto labels for cleanup. In this case, I would prefer an exit: label instead of adding a return. I'm not strongly fighting against that tbh, I prefer consistent and maintainable code over stylish vanity thus here I would probably hand it over to the maintainer for a decision? :)

Yeah, that makes sense.

(Style) if (hsaddress) { ... } else { tor_assert(desc_id); } instead of a branch containing an assert.

I strongly advocate against if/else if statement that DOES NOT have a final else. Over time, that's only error prone but sometimes you don't have a choice thus a nice comment to explain why is important.

In this case, I went for the "else tor_assert" because desc_id should be treated as a single value and not a "fallback". Assuming future code that might add an other variable, we'll end up splitting the else statement and breaking the git blame :(. It's so much more easier to read an if/else statement when the value being tested is actually the one being worked on (in the case of input parsing).

This makes sense too.

src/or/rendclient.c:rend_client_refetch_v2_renddesc(): No longer need to log a warning when you fail to compute the v2 rend descriptor ID?

I'm not seeing any log warnings in that function about that. Maybe you had an other function in mind?

@@ -745,44 +895,12 @@ rend_client_refetch_v2_renddesc(const rend_data_t *rend_query)

fe452c39248e99165b8756f38446eda8493f7300
[stuff snipped]

-   if (rend_compute_v2_desc_id(descriptor_id, rend_query->onion_address,
-                               rend_query->auth_type == REND_STEALTH_AUTH ?
-                               rend_query->descriptor_cookie : NULL,
-                               time(NULL), chosen_replica) < 0) {
-     log_warn(LD_REND, "Internal error: Computing v2 rendezvous "
-                       "descriptor ID did not succeed.");
-     /*
-      * Hmm, can this write anything to descriptor_id and still fail?
-      * Let's clear it just to be safe.

That log_warn should be in fetch_v2_desc_by_addr() in the new code?

Let me know when you want me to do another pass over the new branch.

Replying to yawning:

Instead of removing out arg1, just so you can use SMARTLIST_FOREACH, for (int i = 1; i < smartlist_len(args); ++i) followed by smartlist_get(args, i) will suffice (and is consistent with the rest of the control code).

Not sure I understand this one. I don't see this being used in control.c. Can you clarify the loop you have in mind for me to skip the first arg?

handle_control_closecircuit() has an example of what I have in mind:

Fixed!

src/or/rendclient.c:rend_client_refetch_v2_renddesc(): No longer need to log a warning when you fail to compute the v2 rend descriptor ID?

I'm not seeing any log warnings in that function about that. Maybe you had an other function in mind?

{{{ @@ -745,44 +895,12 @@ rend_client_refetch_v2_renddesc(const rend_data_t *rend_query)

fe452c39248e99165b8756f38446eda8493f7300 [stuff snipped]
if (rend_compute_v2_desc_id(descriptor_id, rend_query->onion_address,
                          rend_query->auth_type == REND_STEALTH_AUTH ?
                          rend_query->descriptor_cookie : NULL,
                          time(NULL), chosen_replica) < 0) {
log_warn(LD_REND, "Internal error: Computing v2 rendezvous "
                  "descriptor ID did not succeed.");
/*
 * Hmm, can this write anything to descriptor_id and still fail?
 * Let's clear it just to be safe.
}}}

That log_warn should be in fetch_v2_desc_by_addr() in the new code?

Ah now I remember why I removed it, rend_compute_v2_desc_id() has already a log for each error path thus this one is redundant.

Hopefully everything is fixed!

New branch for review: bug14847_027_05 Spec: ticket14847_11

Trac:
Status: needs_revision to needs_review

Replying to dgoulet:

Hopefully everything is fixed!

New branch for review: bug14847_027_05 Spec: ticket14847_11

Looks good to me, I say it's time for upstream review, or if you want a 2nd opinion.

Trac:
Keywords: SponsorR tor-hs controller deleted, SponsorR tor-hs controller nickm-review added

Quick code review:

This expression should maybe be extracted into a separate function:

             strcmpstart(arg1, v2_str) == 0 &&
             strlen(arg1 + v2_str_len) == REND_DESC_ID_V2_LEN_BASE32 &&
             base32_decode(digest, sizeof(digest), arg1 + v2_str_len,
                           REND_DESC_ID_V2_LEN_BASE32) == 0)

The name and documentation on rend_hsaddress_value() are a little vague. Maybe rend_hsaddress_str_or_unknown() ?
~~(Nick, Take a closer look at all the rendclient.c changes)~~ Looks okay.
rend_cache_store_v2_desc_as_client() should set *entry to NULL on the no-reply case. (by convention)

Oh, also:

It would be pretty cool if the command parsing part of command handler were split into a separate function.

Marking more tickets as triaged-in for 0.2.7

Trac:
Keywords: SponsorR tor-hs controller nickm-review deleted, SponsorR, nickm-review, 027-triaged-1-in, controller, tor-hs added

Trac:
Keywords: N/A deleted, SponsorS added
Points: N/A to small-remaning
Version: N/A to Tor: 0.2.7

Whoops, wrong ticket.

New branch! :) Although, I had to squash commits and make a "Multiple fixes" commit. Too many conflicts with the current master while rebasing and thus this makes it simpler I think.

It would be pretty cool if the command parsing part of command handler were split into a separate function.

Hrm so currently HSFETCH has basic command parsing and I feel like we should try to split control.c into smaller files at some point and have a file with common parsing functions. The control C file is already at 5508 lines and pretty sure lots of duplicate! Also, if we do a refactoring, we could use your control parsing branch also?

In the meantime here is the branch with the fixes except the above.

See: bug14847_027_06