Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ticket27215 035 01 #288

Merged
merged 4 commits into from Aug 28, 2018
Merged

Conversation

Labels
None yet
Projects
None yet
4 participants
@dgoulet-tor
Copy link
Contributor

@dgoulet-tor dgoulet-tor commented Aug 22, 2018

No description provided.

dgoulet-tor added 3 commits Aug 22, 2018
Part of #27215, we need to call the ed_key_init_from_file function during
option_validate() which is before the global_options variable is set.

This commit make ed_key_init_from_file() stop using get_options() and instead
now has a or_options_t parameter.

Signed-off-by: David Goulet <dgoulet@torproject.org>
In order to switch the default HS version from 2 to 3, we need tor to be smart
and be able to decide on the version by trying to load the service keys during
configuration validation.

Part of #27215

Signed-off-by: David Goulet <dgoulet@torproject.org>
Closes #27215

Signed-off-by: David Goulet <dgoulet@torproject.org>
@coveralls
Copy link

@coveralls coveralls commented Aug 22, 2018

Coverage Status

Coverage increased (+0.02%) to 59.559% when pulling d9bfc9e on dgoulet-tor:ticket27215_035_01 into ac44e70 on torproject:master.

Copy link
Contributor

@teor2345 teor2345 left a comment

I think we should avoid fatal asserts.
And there's one typo that needs fixing.

int version = -1; /* Unknown version. */
const char *directory_path;

tor_assert(service);
Copy link
Contributor

@teor2345 teor2345 Aug 28, 2018

We try to avoid fatal asserts.

We could say:

if (BUG(!service)) {
  return -1;
}

Copy link
Contributor Author

@dgoulet-tor dgoulet-tor Aug 28, 2018

I'm not entirely keen on using non fatal BUG(). This is a function parameter assert() that is I want it to crash if the function is used without a service that is incorrectly. It can't and shouldn't be used with a NULL service and if it is, I want to find it quickly and us to fix the code flow.

Accepting a NULL service there will lead to tor basically segfaulting. So we can't recover from the BUG() in this case.

Not only that, but returning -1 for this function means "unknown version for given service object" implying that the object exists. It is a "context dependent function" that can't return that same reason for a non existent object.

We do use those parameter asserts() a lot in the HS subsystem because there is a point where we need to consider certain function input for granted so we can only error on specific use cases related to what has been given and not returning error code on "if you failed to use the function properly".

tor_assert(service);

/* We'll try to load the key for version 3. If not found, we'll try version
* 2 and if not found, we'll send back an unknown version (0). */
Copy link
Contributor

@teor2345 teor2345 Aug 28, 2018

unknown version is -1

@@ -2826,7 +2826,7 @@ The following options are used to configure a hidden service.

[[HiddenServiceVersion]] **HiddenServiceVersion** **2**|**3**::
A list of rendezvous service descriptor versions to publish for the hidden
service. Currently, versions 2 and 3 are supported. (Default: 2)
service. Currently, versions 2 and 3 are supported. (Default: 3)
Copy link
Contributor

@teor2345 teor2345 Aug 28, 2018

😎

@torproject-pusher torproject-pusher merged commit d9bfc9e into torproject:master Aug 28, 2018
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment