rend_config_services should use service_is_ephemeral rather than old/new->directory

We missed this in the earlier refactor. (Does that make it a bug? I think so.)

changed milestone to %Tor: 0.3.0.x-final

added component::core tor/tor easy hs intro milestone::Tor: 0.3.0.x-final owner::jryans points::0.1 priority::medium refactor resolution::fixed severity::normal status::closed type::defect version::tor 0.2.9.5-alpha labels

I think this made it into 0.2.9.5, and it was #20526 (moved).

Trac:
Version: Tor: 0.3.0.0-alpha-dev to Tor: 0.2.9.5-alpha

Yes, I'd call it a bug.

I'd suggest no backport, though

I agree, I tend to avoid backports, I've been burnt before by backporting too many patches.

Trac:
Username: jryans
Owner: N/A to jryans
Status: new to assigned

Okay, this seems like a pretty straightforward refactoring. I believe I've made the requested changes:

https://github.com/jryans/tor/commits/service_is_ephemeral

Not sure if a changes file was needed for this, but I added one anyway just to get familiar with the process.

Trac:
Username: jryans
Status: assigned to needs_review

Replying to jryans:

Okay, this seems like a pretty straightforward refactoring. I believe I've made the requested changes:

https://github.com/jryans/tor/commits/service_is_ephemeral

Thanks for this patch!

rend_service_verify_single_onion_poison() needs an explicit check for the directory, because rend_service_private_key_exists() requires there to be a directory, or tor asserts. And we want to be able to vary how we check for ephemeral services in future. (But you're right that we should check for ephemeral services.)

So I think we can fix this by adding a check for s->directory as well as the ephemeral check.

Similarly, we can't change this part of rend_config_services:

        if (new->directory && old->directory &&
            !strcmp(old->directory, new->directory)) {

without adding explicit checks for the directories being valid before doing a strcmp on them.

For bonus points, these extra checks should probably be BUG() checks, which log a bug warning. (If they ever get triggered, it's a coder's error.)

A normal condition looks like:

if (boolean_condition) {
  action();
}

A BUG condition looks like:

if (BUG(boolean_condition)) {
  action();
}

Not sure if a changes file was needed for this, but I added one anyway just to get familiar with the process.

Thanks! Minor comment changes don't get a changes file, everything else does. (I've submitted a number of code changes that were "Refactoring, no behaviour change", and then ended up being "unexpected bug in ticket XXXXX because behaviour changed during refactor".)

One nitpick:

the changes file format starts with Minor bugfix (optional categories): https://gitweb.torproject.org/tor.git/tree/doc/HACKING/CodingStandards.md#n56

Trac:
Status: needs_review to needs_revision

Thanks for the quick review! Updated patch at:

https://github.com/jryans/tor/commits/service_is_ephemeral

Replying to teor:

rend_service_verify_single_onion_poison() needs an explicit check for the directory, because rend_service_private_key_exists() requires there to be a directory, or tor asserts. And we want to be able to vary how we check for ephemeral services in future. (But you're right that we should check for ephemeral services.)

So I think we can fix this by adding a check for s->directory as well as the ephemeral check.

Similarly, we can't change this part of rend_config_services: {{{ if (new->directory && old->directory && !strcmp(old->directory, new->directory)) { }}} without adding explicit checks for the directories being valid before doing a strcmp on them.

Okay, I added explicit BUG checks for the directory in addition to the ephemeral check for both of these cases.

Not sure if a changes file was needed for this, but I added one anyway just to get familiar with the process.

Thanks! Minor comment changes don't get a changes file, everything else does. (I've submitted a number of code changes that were "Refactoring, no behaviour change", and then ended up being "unexpected bug in ticket XXXXX because behaviour changed during refactor".)

One nitpick:

the changes file format starts with Minor bugfix (optional categories): https://gitweb.torproject.org/tor.git/tree/doc/HACKING/CodingStandards.md#n56

I wasn't sure if you wanted me to add an optional category or change to "Minor bugfix". The current file (using the group "Code simplification and refactoring") passes the lintChanges.py script. Let me know if there's still a change needed.

Trac:
Username: jryans
Status: needs_revision to needs_review

Replying to jryans:

Thanks for the quick review! Updated patch at:

https://github.com/jryans/tor/commits/service_is_ephemeral

Replying to teor:

rend_service_verify_single_onion_poison() needs an explicit check for the directory, because rend_service_private_key_exists() requires there to be a directory, or tor asserts. And we want to be able to vary how we check for ephemeral services in future. (But you're right that we should check for ephemeral services.)

So I think we can fix this by adding a check for s->directory as well as the ephemeral check.

Similarly, we can't change this part of rend_config_services: {{{ if (new->directory && old->directory && !strcmp(old->directory, new->directory)) { }}} without adding explicit checks for the directories being valid before doing a strcmp on them.

Okay, I added explicit BUG checks for the directory in addition to the ephemeral check for both of these cases.

Thanks for this change. The BUG(new->directory) and the old check are inverted, they will log a message when the directory exists, but we want to log a message when it doesn't.

Oh, and can we do them in the same order in rend_service_verify_single_onion_poison? (That is, the ephemeral check first, then the directory check?) Let's not log a BUG unless we really have to.

Not sure if a changes file was needed for this, but I added one anyway just to get familiar with the process.

Thanks! Minor comment changes don't get a changes file, everything else does. (I've submitted a number of code changes that were "Refactoring, no behaviour change", and then ended up being "unexpected bug in ticket XXXXX because behaviour changed during refactor".)

One nitpick:

the changes file format starts with Minor bugfix (optional categories): https://gitweb.torproject.org/tor.git/tree/doc/HACKING/CodingStandards.md#n56

I wasn't sure if you wanted me to add an optional category or change to "Minor bugfix". The current file (using the group "Code simplification and refactoring") passes the lintChanges.py script. Let me know if there's still a change needed.

Yes, please change it to 'Minor bugfix (hidden services)'.

Trac:
Status: needs_review to needs_revision

Updated patch at https://github.com/jryans/tor/commits/service_is_ephemeral.

Replying to teor:

Replying to jryans:

Replying to teor:

Similarly, we can't change this part of rend_config_services: {{{ if (new->directory && old->directory && !strcmp(old->directory, new->directory)) { }}} without adding explicit checks for the directories being valid before doing a strcmp on them.

Okay, I added explicit BUG checks for the directory in addition to the ephemeral check for both of these cases.

Thanks for this change. The BUG(new->directory) and the old check are inverted, they will log a message when the directory exists, but we want to log a message when it doesn't.

Ah, good catch! I inverted the part inside BUG to log in the correct case, but I also have to invert outside BUG to chain with the rest of the conditional. Not sure if && !BUG(!...) && is okay style.

Oh, and can we do them in the same order in rend_service_verify_single_onion_poison? (That is, the ephemeral check first, then the directory check?) Let's not log a BUG unless we really have to.

Makes sense, updated the order.

Not sure if a changes file was needed for this, but I added one anyway just to get familiar with the process.

Thanks! Minor comment changes don't get a changes file, everything else does. (I've submitted a number of code changes that were "Refactoring, no behaviour change", and then ended up being "unexpected bug in ticket XXXXX because behaviour changed during refactor".)

One nitpick:

the changes file format starts with Minor bugfix (optional categories): https://gitweb.torproject.org/tor.git/tree/doc/HACKING/CodingStandards.md#n56

I wasn't sure if you wanted me to add an optional category or change to "Minor bugfix". The current file (using the group "Code simplification and refactoring") passes the lintChanges.py script. Let me know if there's still a change needed.

Yes, please change it to 'Minor bugfix (hidden services)'.

Okay, updated. Added bugfix on ... as requested by lint tool as well. Comment 1 suspected that #20526 (moved) made it into 0.2.9.5, but git describe --contains on the commit from #20526 (moved) didn't find any tags, so I listed it as not in any released version.

Trac:
Username: jryans
Status: needs_revision to needs_review

Further updates to the patch at https://github.com/jryans/tor/commits/service_is_ephemeral after discussing on IRC:

• Cleaned up format of the conditionals to read more clearly
• Use return -1 with poison checks
• Add checks to rend_service_poison_new_single_onion_dir as well
  • I pulled up the log message for ephemeral services from rend_service_poison_new_single_onion_dir_impl since checking for this case in rend_service_poison_new_single_onion_dir would omit the message

Trac:
Username: jryans

Looks good, but we don't really need the extra log_info(LD_REND, "Ephemeral HS started in non-anonymous mode.");, because the BUG() macro logs a message anyway.

But it's harmless, so it's up to you whether you want to revise it. Otherwise, please flip it to "ready to merge".

Trac:
Status: needs_review to needs_revision

Okay, I wasn't sure, so was just being cautious. I updated the patch to remove the extra log.

Trac:
Username: jryans
Status: needs_revision to merge_ready

merging, per teor's review.

Trac:
Resolution: N/A to fixed
Status: merge_ready to closed

closed

changed time estimate to 48m

mentioned in issue #21054 (moved)

moved to tpo/core/tor#20853 (closed)

mentioned in issue tpo/core/tor#21054 (closed)