[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: First patch for proposal 121
On Wed, Aug 06, 2008 at 03:52:10PM +0200, Karsten Loesing wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Hi Nick,
>
> as discussed on IRC I have started splitting up the implementation of
> proposal 121 into smaller patches. I've come to the conclusion that four
> main patches would be a good trade-off between patch size and number in
> this case. There might be further, smaller patches afterwards, but these
> four patches cover roughly everything to make proposal 121 work.
>
> 1. This is the first patch that includes configuration of client
> authorization on hidden-service side, but without actually using that
> data when advertising hidden services.
Hi, Karsten! I'm checking this in with some modifications and
questions, as noted below. Please compare my patch to yours, and let
me know about anything I screwed up.
> Index: /home/karsten/tor/tor-trunk/src/common/crypto.c
> ===================================================================
> --- /home/karsten/tor/tor-trunk/src/common/crypto.c (revision 16440)
> +++ /home/karsten/tor/tor-trunk/src/common/crypto.c (working copy)
> @@ -515,6 +515,47 @@
> return 0;
> }
>
> +/** PEM-encode the private key portion of <b>env</b> and write it to a
> + * newly allocated string. On success, set *<b>dest</b> to the new
> + * string, *<b>len</b> to the string's length, and return 0. On
> + * failure, return -1.
> + */
> +int
> +crypto_pk_write_private_key_to_string(crypto_pk_env_t *env, char **dest,
> + size_t *len)
This is mostly duplicate code with crypto_pk_write_public_key_to_string:
only one line is different between the two. I've extracted the code
into a new static function that takes a flag, and replaced both
functions with calls to it.
Copy-and-paste coding like this is usually to be avoided, for several
reasons. The most significant is that if there is a bug in the code,
or a feature we need to add to it, there are now two places that need
to be changed.
> Index: /home/karsten/tor/tor-trunk/src/or/rendservice.c
> ===================================================================
> --- /home/karsten/tor/tor-trunk/src/or/rendservice.c (revision 16440)
> +++ /home/karsten/tor/tor-trunk/src/or/rendservice.c (working copy)
> @@ -47,6 +47,10 @@
> smartlist_t *ports; /**< List of rend_service_port_config_t */
> int descriptor_version; /**< Rendezvous descriptor version that will be
> * published. */
> + int auth_type; /**< Client authorization type or 0 if no client
> + * authorization is performed. */
This should be an enumeration. Using "0" to mean no authentication
and "1" to mean "key-based authentication" is a fine way for computers
to communicate, but it will make our code insane.
> + smartlist_t *clients; /**< List of rend_authorized_client_t's of
> + * clients that may access our service. */
Am I right in thinking this can be NULL? If so, the doxygen comment
should probably mention that.
> /* Other fields */
> crypto_pk_env_t *private_key; /**< Permanent hidden-service key. */
> char service_id[REND_SERVICE_ID_LEN_BASE32+1]; /**< Onion address without
> @@ -79,6 +83,18 @@
> return smartlist_len(rend_service_list);
> }
>
> +/** Helper: free storage held by a single service authorized client entry. */
> +static void
> +rend_authorized_client_free(void *authorized_client)
> +{
This should not take a void*. We use it in a few contexts, and in all
but one of them, we're passing in a rend_authorized_client_t pointer.
Let's not undermine the compiler's typechecking facilities any more
than we must.
[...]
> + if (smartlist_len(type_names_split) > 2) {
> + log_warn(LD_CONFIG, "HiddenServiceAuthorizeClient contains "
> + "illegal value '%s'. Must be formatted "
> + "as 'HiddenServiceAuthorizeClient auth-type "
> + "client-name,client-name,...' (without "
> + "additional spaces in comma-separated client "
> + "list).",
Why this requirement?
> + SMARTLIST_FOREACH_BEGIN(clients, char *, client_name)
This should be a "const char *".
> + {
> + rend_authorized_client_t *client;
> + size_t len = strlen(client_name);
> + int found_duplicate = 0;
> + if (len < 1 || len > 19) {
> + log_warn(LD_CONFIG, "HiddenServiceAuthorizeClient contains an "
> + "illegal client name: '%s'. Length must be "
> + "between 1 and 19 characters.",
> + client_name);
Why 19? The reason that server nicknames top out at 19 characters is
so that we can never confuse them for hex-encoded SHA-1 digests (which
are 20 characters). Does this also apply to client names?
[Also, this maximum should be a constant.]
[...]
> + /* Check if client name is duplicate. */
> + SMARTLIST_FOREACH(service->clients, rend_authorized_client_t *, c, {
> + if (!strcmp(c->client_name, client_name)) {
> + log_warn(LD_CONFIG, "HiddenServiceAuthorizeClient contains a "
> + "duplicate client name: '%s'; ignoring.", client_name);
> + found_duplicate = 1;
> + break;
> + }
> + });
Ug. This is O(N^2). I suppose it won't matter for a while.
> + if (found_duplicate)
> + continue;
> + client = tor_malloc_zero(sizeof(rend_authorized_client_t));
> + client->client_name = strdup(client_name);
You mean "tor_strdup(client_name)".
> + /* If client authorization is configured, load or generate keys. */
> + if (s->auth_type) {
> + char *client_keys_str;
> + strmap_t *parsed_clients = strmap_new();
> + char cfname[512];
> +
> + /* Load client keys and descriptor cookies, if available. */
> + if (strlcpy(cfname,s->directory,sizeof(cfname)) >= sizeof(cfname) ||
> + strlcat(cfname,PATH_SEPARATOR"client_keys",sizeof(cfname))
> + >= sizeof(cfname)) {
tor_snprintf is usually more readable for this kind of thing.
> + /* Prepare client_keys and hostname files. */
> + if (write_str_to_file(cfname, "", 0) < 0) {
> + log_warn(LD_CONFIG, "Could not clear client_keys file.");
> + strmap_free(parsed_clients, rend_authorized_client_free);
> + return -1;
> + }
> + if (write_str_to_file(fname, "", 0) < 0) {
> + log_warn(LD_CONFIG, "Could not clear hostname file.");
> + strmap_free(parsed_clients, rend_authorized_client_free);
> + return -1;
> + }
This is really bad: If there's an error or crash or anything during
this step, then you'll have erased all the old keys. Instead, you
want to open new files, write into them, and when you're done, replace
the old files with the new files.
Fortunately, Tor already has functions for doing this: see
start_writing_to_file() and friends in util.c.
[...]
> + strmap_free(parsed_clients, rend_authorized_client_free);
> + return -1;
This is duplicated a _whole lot_. When you have repeated cleanup
code, consider using a "goto err" pattern.
> Index: /home/karsten/tor/tor-trunk/src/or/routerparse.c
> ===================================================================
[...]
> @@ -2948,10 +2961,14 @@
> ebuf[sizeof(ebuf)-1] = '\0';
> RET_ERR(ebuf);
> }
> - if (!strcmp(tok->object_type, "RSA PUBLIC KEY")) { /* If it's a key... */
> + if (!strcmp(tok->object_type, "RSA PUBLIC KEY")) { /* If it's a public key */
> tok->key = crypto_new_pk_env();
> if (crypto_pk_read_public_key_from_string(tok->key, obstart, eol-obstart))
> RET_ERR("Couldn't parse public key.");
> + } else if (!strcmp(tok->object_type, "RSA PRIVATE KEY")) { /* private key */
> + tok->key = crypto_new_pk_env();
> + if (crypto_pk_read_private_key_from_string(tok->key, obstart))
> + RET_ERR("Couldn't parse private key.");
Nice thinking, but there's a problematic aspect: We do not want
private keys accepted in existing descriptors when currently only
public keys are accepted. I've added a new NEED_SKEY_1024 to handle
this.
[...]
> + /* Begin parsing with first entry, skipping comments or whitespace at the
> + * beginning. */
This skips *everything*, not just comments and whitespace. Is that
really wise?
many thanks,
--
Nick