-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Hi Nick,
|> + 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.
Yes, right, an enumeration is better suited here.
The question is: should comparisons of this variable always be performed
with an enumeration constant? For example, should "if
(service->auth_type)" be changed to "if (service->auth_type !=
REND_NO_AUTH)"? (I changed it that way in the attached patch.)
And, at one place you explicitly converted the enumeration value to an
int before writing it to a log statement, but at another place you
didn't. Should this conversion be performed consistently in all places?
|> + 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?
You mean the requirement that there are no additional spaces in the
list? That can be relaxed. I'll add it to the second patch.
|> + 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?
No, there is no such reason with SHA-1 digests here. The limit of 19
characters is more or less arbitrary. There should be _some_ limit, or
the other way round, there is no need to use names of arbitrary length.
But it does not necessarily have to be 19. Maybe the 19 are even
confusing, because people might think that there is some relation to
node nicknames. Don't know. Suggestions?
|> + /* 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.
Right. I'll have a second look at the other data structures that are
provided in Tor and find something more efficient (like putting all
strings in a set or map that discards duplicates and read them out
afterwards. That will also be included in the second patch.
|> + /* Begin parsing with first entry, skipping comments or whitespace
at the
|> + * beginning. */
|
| This skips *everything*, not just comments and whitespace. Is that
| really wise?
Hmm, what would you suggest? The only thing I could imagine is to check
if there are non-comment, non-whitespace characters and warn/err on
that. Heh, and the other approach would be to change the comment.
Thanks a lot for these suggestions and for even implementing most of
them! :)
See the attached patch with some minor changes.
Thanks!
- --Karsten
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iD8DBQFInIFj0M+WPffBEmURAkcCAJ40UFUF7ivde7TpTJg3tDq0L3+6LACfRqjr
+6gYHDVxUeDH18tRA3hR4Qg=
=DdxU
-----END PGP SIGNATURE-----
Index: /home/karsten/tor/tor-trunk-121-patches/src/or/rendservice.c
===================================================================
--- /home/karsten/tor/tor-trunk-121-patches/src/or/rendservice.c (revision 16477)
+++ /home/karsten/tor/tor-trunk-121-patches/src/or/rendservice.c (working copy)
@@ -57,7 +57,8 @@
rend_auth_type_t auth_type; /**< Client authorization type or 0 if no client
* authorization is performed. */
smartlist_t *clients; /**< List of rend_authorized_client_t's of
- * clients that may access our service. */
+ * clients that may access our service. Can be NULL
+ * if no client authorization is peformed. */
/* Other fields */
crypto_pk_env_t *private_key; /**< Permanent hidden-service key. */
char service_id[REND_SERVICE_ID_LEN_BASE32+1]; /**< Onion address without
@@ -181,7 +182,7 @@
service->descriptor_version = 2; /* Versioned descriptor. */
}
- if (service->auth_type && !service->descriptor_version) {
+ if (service->auth_type != REND_NO_AUTH && !service->descriptor_version) {
log_warn(LD_CONFIG, "Hidden service with client authorization and "
"version 0 descriptors configured; ignoring.");
rend_service_free(service);
@@ -188,7 +189,8 @@
return;
}
- if (service->auth_type && smartlist_len(service->clients) == 0) {
+ if (service->auth_type != REND_NO_AUTH &&
+ smartlist_len(service->clients) == 0) {
log_warn(LD_CONFIG, "Hidden service with client authorization but no "
"clients; ignoring.");
rend_service_free(service);
@@ -329,7 +331,7 @@
* of authorized clients. */
smartlist_t *type_names_split, *clients;
const char *authname;
- if (service->auth_type) {
+ if (service->auth_type != REND_NO_AUTH) {
log_warn(LD_CONFIG, "Got multiple HiddenServiceAuthorizeClient "
"lines for a single service.");
rend_service_free(service);
@@ -363,7 +365,7 @@
if (smartlist_len(type_names_split) < 2) {
log_warn(LD_CONFIG, "HiddenServiceAuthorizeClient contains "
"authorization type %d, but no client names.",
- service->auth_type);
+ (int)service->auth_type);
SMARTLIST_FOREACH(type_names_split, char *, cp, tor_free(cp));
smartlist_free(type_names_split);
continue;
@@ -583,7 +585,7 @@
}
/* If client authorization is configured, load or generate keys. */
- if (s->auth_type) {
+ if (s->auth_type != REND_NO_AUTH) {
char *client_keys_str = NULL;
strmap_t *parsed_clients = strmap_new();
char cfname[512];
@@ -676,7 +678,6 @@
if (written < 0) {
log_warn(LD_BUG, "Could not write client entry.");
goto err;
-
}
if (client->client_key) {
char *client_key_out;
@@ -710,7 +711,8 @@
char extended_desc_cookie[REND_DESC_COOKIE_LEN+1];
memcpy(extended_desc_cookie, client->descriptor_cookie,
REND_DESC_COOKIE_LEN);
- extended_desc_cookie[REND_DESC_COOKIE_LEN] = (s->auth_type - 1) << 4;
+ extended_desc_cookie[REND_DESC_COOKIE_LEN] =
+ ((int)s->auth_type - 1) << 4;
if (base64_encode(desc_cook_out, 3*REND_DESC_COOKIE_LEN_BASE64+1,
extended_desc_cookie,
REND_DESC_COOKIE_LEN+1) < 0) {
Attachment:
patch-121-1a.txt.sig
Description: Binary data