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

Ticket25610 034 01 #64

Closed
wants to merge 22 commits into from

Conversation

@dgoulet-tor
Copy link
Contributor

commented Apr 27, 2018

No description provided.

dgoulet-tor added 18 commits Apr 3, 2018
Make our build system support a disable dirauth module option. It can only be
disabled explicitly with:

  $ ./configure --disable-module-dirauth

If *not* specified that is enabled, an automake conditional variable is set to
true and a defined value for the C code:

  AM_CONDITIONAL: BUILD_MODULE_DIRAUTH
  AC_DEFINE: HAVE_MODULE_DIRAUTH=1

This introduces the dirauth/ module directory in src/or/ for which .c files
are only compiled if the BUILD_MODULE_DIRAUTH is set.

All the header files are compiled in regardless of the support so we can use
the alternative entry point functions of the dirauth subsystem.

Signed-off-by: David Goulet <dgoulet@torproject.org>
Don't access the AuthoritativeDir options directly. We do this so we can move
authdir_mode() to the dirauth module.

Signed-off-by: David Goulet <dgoulet@torproject.org>
Many functions become static to the C file or exposed to the tests within the
PRIVATE define of dirvote.h.

This commit moves a function to the top. No code behavior change.

Signed-off-by: David Goulet <dgoulet@torproject.org>
Signed-off-by: David Goulet <dgoulet@torproject.org>
Remove useless include.

Clearly identify functions that are used by other part of Tor, functions that
are only used by the dirauth subsystem and functions that are exposed for unit
tests.

This will help us in the dirauth modularization effort.

Signed-off-by: David Goulet <dgoulet@torproject.org>
This is a pretty big commit but it only moves these files to src/or/dirauth:

  dircollate.c dirvote.c shared_random.c shared_random_state.c
  dircollate.h dirvote.h shared_random.h shared_random_state.h

Then many files are modified to change the include line for those header files
that have moved into a new directory.

Without using --disable-module-dirauth, everything builds fine. When using the
flag to disable the module, tor doesn't build due to linking errors. This will
be addressed in the next commit(s).

No code behavior change.

Signed-off-by: David Goulet <dgoulet@torproject.org>
Move most of the shared random functions that are needed outside of the
dirauth module.

At this commit, because dirvote.c hasn't been refactor, it doesn't compile
because some SR functions need a dirvote function.

Furthermore, 5 functions haven't been touched yet because they are dirauth
only but are in used in other C files than the dirauth module ones.

No code behavior change. Only moving code around.

Signed-off-by: David Goulet <dgoulet@torproject.org>
Add static inline dirauth public functions used outside of the dirauth module
so they can be seen by the tor code but simply do nothing.

Signed-off-by: David Goulet <dgoulet@torproject.org>
No code behavior change.

Signed-off-by: David Goulet <dgoulet@torproject.org>
In order to follow the public namespace of dirvote.

Signed-off-by: David Goulet <dgoulet@torproject.org>
Renamed to follow the file namespace.

Signed-off-by: David Goulet <dgoulet@torproject.org>
From dirvote.c to networkstatus.c where it makes more sense both in terms of
namespace and subsystem responsability.

This removes one less dependency on the dirauth module.

Signed-off-by: David Goulet <dgoulet@torproject.org>
When parsing a vote in routerparse.c, only dirauth extract the commits from
the vote so move all this code into dirvote.c so we can make it specific to
the dirauth module.

If the dirauth module is disabled, the commit parsing does nothing.

Signed-off-by: David Goulet <dgoulet@torproject.org>
In order to make sr_commit_free() only used by the dirauth module, this
commits moves the commits free from a vote object into the dirvote.c file
which is now only for the module.

The function does nothing if the module is disabled.

Signed-off-by: David Goulet <dgoulet@torproject.org>
In order to further isolate the dirauth code into its module, this moves the
handling of the directory request GET /tor/status-vote/* into the module.

Signed-off-by: David Goulet <dgoulet@torproject.org>
Both functions are used for directory request but they can only be used if the
running tor instance is a directory authority.

For this reason, make those symbols visible but hard assert() if they are
called when the module is disabled. This would mean we failed to safeguard the
entry point into the module.

Signed-off-by: David Goulet <dgoulet@torproject.org>
This code is only for dirauth so this commit moves it into the module in
dirvote.c.

No code behavior change.

Signed-off-by: David Goulet <dgoulet@torproject.org>
The --disable-module-* configure option removes code from the final binary but
we still build the unit tests with the disable module(s) so we can actually
test that code path all the time and not forget about it.

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

This comment has been minimized.

Copy link

commented Apr 27, 2018

Coverage Status

Coverage decreased (-0.006%) to 58.892% when pulling 0765bf3 on dgoulet-tor:ticket25610_034_01 into 3a47dfe on torproject:master.

/* Public methods used _outside_ of the module.
*
* We need to nullify them if the module is disabled. */
#ifdef HAVE_MODULE_DIRAUTH

This comment has been minimized.

Copy link
@nmathewson

nmathewson Apr 30, 2018

Contributor

We should make sure we have "#ifdef HAVE_MODULE_DIRAUTH" surrounding all of the include files in dirauth.h, except for any parts that should be accessible with dirauths are disabled.

This comment has been minimized.

Copy link
@nmathewson

nmathewson May 1, 2018

Contributor

err, whoops. I meant "all the include files in dirauth/*.h"

This comment has been minimized.

Copy link
@dgoulet-tor

dgoulet-tor May 1, 2018

Author Contributor

We can't ... header file in dirauth/ have symbols used outside of the dirauth module which are NOP if no dirauth module.

If we ifdef the include statement, it will fail to compile by not finding the symbols.

}

static inline void
sr_act_post_consensus(const networkstatus_t *consensus)

This comment has been minimized.

Copy link
@nmathewson

nmathewson Apr 30, 2018

Contributor

I'm not sure that this function is one it makes sense to expose in the case when this module is disabled. Do we really need to do so? It doesn't seem as logical as the "init" and "cleanup" functions.

This comment has been minimized.

Copy link
@dgoulet-tor

dgoulet-tor May 1, 2018

Author Contributor

Indeed, it should be dirauth only. However, it is used in directory.c when a consensus is fetched and if we are acting as a dirauth, we call that post consensus function.

It should be made dirauth only when we refactor the directory.c file for all authdir_mode_v3() places that should go in src/or/dirauth/ at that point.

This comment has been minimized.

Copy link
@nmathewson

nmathewson May 1, 2018

Contributor

Okay. I'll look for things we can #ifdef postmerge...

This comment has been minimized.

Copy link
@dgoulet-tor

dgoulet-tor May 1, 2018

Author Contributor

I think what we might want is go over all the authdir_mode() stuff and see how we can move them to the dirauth module. Then for those functions, we should NOP them if no module.

Considering the above, ahf was thinking of having a way to register callbacks to directory command so for instance, you fetch a consensus, then the handler goes over the registered callbacks. If the dirauth module is enabled, we would do our "sr act post consensus" as a callback.

/* See LICENSE for licensing information */

/**
* \file shared_random_common.c

This comment has been minimized.

Copy link
@nmathewson

nmathewson Apr 30, 2018

Contributor

I don't think "common" is the right name for this kind of module. This isn't exactly "the parts of the API that are shared by some other things" -- it's "the parts of the API that are used even if you aren't a dirauth." Maybe "shared_random_user" or "shared_random_client"? Hm. Or maybe something else. This is tricky.

This comment has been minimized.

Copy link
@dgoulet-tor

dgoulet-tor May 1, 2018

Author Contributor

Yes tricky, we had a bikeshed moment but I'm still unsure to this moment...

Bottom line is that they are functions that other non-dirauth part of Tor needs. I like the _client.c idea. For us developers, it makes sense but looking from another angle, all the functions in those two _common.h files are only for HS.

Actually, everything in dirvote_common.h is for shared_random_common.c and then that file is for hidden service.

There could be an argument to actually isolate all of this in one hidden service file like hs_shared_random.c and dirvote_common.c into that file or in a new specific file?

This comment has been minimized.

Copy link
@nmathewson

nmathewson May 1, 2018

Contributor

I think we can rename these post-merge, but we should figure out what we're renaming these to.

This comment has been minimized.

Copy link
@dgoulet-tor

dgoulet-tor May 1, 2018

Author Contributor

Agree. The more we talk about it and the more I don't like _common.c.


/**
* \file dirvote_common.h
* \brief Header file for dirvote_common.c.

This comment has been minimized.

Copy link
@nmathewson

nmathewson Apr 30, 2018

Contributor

See comment previously on shared_random_common.c about why I don't like the "common" name here.

This module could probably be split up into the part about consensus schedules; the part about signatures; and the part about authority certs. But it would make sense to do that after this branch is merged, so let's open a ticket to remember?

This comment has been minimized.

Copy link
@dgoulet-tor

dgoulet-tor May 1, 2018

Author Contributor

Yes

@@ -200,7 +200,7 @@ dirvote_get_voter_sig_by_alg(const networkstatus_voter_info_t *voter,
/** Allocate and return a new authority_cert_t with the same contents as
* <b>cert</b>. */
authority_cert_t *
authority_cert_dup(authority_cert_t *cert)
dirvote_authority_cert_dup(authority_cert_t *cert)

This comment has been minimized.

Copy link
@nmathewson

nmathewson Apr 30, 2018

Contributor

I think the original name for this function was correct, since it acts on authority_cert_t. I think this whole commit should be removed or reverted.

This comment has been minimized.

Copy link
@dgoulet-tor

dgoulet-tor May 1, 2018

Author Contributor

Yeah actually, that function is not longer needed outside dirauth/dirvote.c so it should go back there and thus untouched.

I can't revert the commit as it is with other things but I made an extra commit putting it back in dirvote.c with original name: bc393fc

*
* This also sets the SR participation flag if present in the vote. */
void
dirvote_parse_sr_commits(networkstatus_t *ns, smartlist_t *tokens)

This comment has been minimized.

Copy link
@nmathewson

nmathewson Apr 30, 2018

Contributor

Should "tokens" be const?

This comment has been minimized.

Copy link
@dgoulet-tor

dgoulet-tor May 1, 2018

Author Contributor

In theory yes, it would be ideal but find_opt_by_keyword() doesn't take a const :S ... So as a minimal change, this is it. We could add a commit to make that find function take a const though...

This comment has been minimized.

Copy link
@nmathewson

nmathewson May 1, 2018

Contributor

okay, I'll fix postmerge

@@ -3899,3 +3899,16 @@ dirvote_parse_sr_commits(networkstatus_t *ns, smartlist_t *tokens)
}
}

/* For the given vote, free the shared random commits if any. */
void
dirvote_free_commits(networkstatus_t *ns)

This comment has been minimized.

Copy link
@nmathewson

nmathewson Apr 30, 2018

Contributor

I think this function should have a name like "clear_commits" or "remove_commits", not "free_commits", since it doesn't take the commits as an argument.

This comment has been minimized.

Copy link
@dgoulet-tor

dgoulet-tor May 1, 2018

Author Contributor

Done in fixup: 3300369

(void) status_out;
/* If the dirauth module is disabled, this should NEVER be called else we
* failed to safeguard the dirauth module. */
tor_assert(0);

This comment has been minimized.

Copy link
@nmathewson

nmathewson Apr 30, 2018

Contributor

This should probably be tor_assert_nonfatal_unreached() -- it's not worth crashing over, is it?

(void) msg_out;
/* If the dirauth module is disabled, this should NEVER be called else we
* failed to safeguard the dirauth module. */
tor_assert(0);

This comment has been minimized.

Copy link
@nmathewson

nmathewson Apr 30, 2018

Contributor

This should probably be tor_assert_nonfatal_unreached() -- it's not worth crashing over, is it?

This comment has been minimized.

Copy link
@dgoulet-tor

dgoulet-tor May 1, 2018

Author Contributor

Hmmm dunno, if your Tor is voting but without a dirauth module, I believe its worth stopping... I mean in theory it just can't happen with the amount of checks there is but if we happen to be in that path, I would prefer that tor stop instead of starting spamming dirauth with vote.

No strong opinion here, but if we get a crash, usually people notice and we can act quicker on it instead of going unnoticed in logs for a while :S

This comment has been minimized.

Copy link
@nmathewson

nmathewson May 1, 2018

Contributor

The counterargument here is that it's better to spam relays logs than to take relays down off the network because we have some stupid bug we forgot about.

This comment has been minimized.

Copy link
@dgoulet-tor

dgoulet-tor May 1, 2018

Author Contributor

Fair enough.

See fixup commit 0765bf3

@@ -9,11 +9,13 @@
**/

#define SHARED_RANDOM_STATE_PRIVATE
#define TOR_SHARED_RANDOM_COMMON_PRIVATE

This comment has been minimized.

Copy link
@nmathewson

nmathewson Apr 30, 2018

Contributor

One C file should never define another C file's "private" macro. Is there some way we can avoid doing this? If something is private to shared_random_common, literally nothing else besides the tests should be looking at it.

This comment has been minimized.

Copy link
@dgoulet-tor

dgoulet-tor May 1, 2018

Author Contributor

Yes actually... I believe this is an artifact and it builds perfectly without it!

Fixup commit 7941e45

dgoulet-tor added 4 commits May 1, 2018
No need for this private define.
Originally, it was made public outside of the dirauth module but it is no
longer needed. In doing so, we put it back in dirvote.c and reverted its name
to the original one:

dirvote_authority_cert_dup() --> authority_cert_dup()

Signed-off-by: David Goulet <dgoulet@torproject.org>
@nmathewson

This comment has been minimized.

Copy link
Contributor

commented May 7, 2018

We merged a squashed version of this with d018bf1

@nmathewson nmathewson closed this May 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.