Re: First patch for proposal 121

[Nick Mathewson <nickm@xxxxxxxxxxxxx>]

On Fri, Aug 08, 2008 at 07:24:58PM +0200, Karsten Loesing wrote:
> -----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.)

That's fine.  I personally think having 0 mean "NO_AUTH" is on the
okay side of good taste, but having NO_AUTH mean NO_AUTH is even
better. :)

> 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?

Ideally, it shouldn't be an integer, but rather converted back to a
string. for logging.

> |> +      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.

Great.

> |> +        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?

Let's go with an arbitrarily chosen power of 2 (16 or 32 seem
attractive colors for a bikeshed today), and make a #define for it,
and call ourselves done.

> |> +        /* 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.

Keen.
 
> |> +  /* 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.

I think disallowing non-whitespace data is the best here.  It's in a
descriptor IIUC, so no comments should really be expected or allowed.

Another note on this part of the code: strstr(ckstr, "client-name ")
will match lines that have "client-name" in the *middle* of the line
too.  That is really not at all what you want.

yrs,
-- 
Nick