Re: [Nbd] NBD passwords (was Re: nbd-server working easily in cygwin in XP)

[Wouter Verhelst <w@...112...>]

On Fri, Sep 05, 2008 at 02:59:17PM -0700, Brad Allen wrote:
> " Hi Brad,
> " 
> " On Fri, Aug 29, 2008 at 02:37:29PM -0700, ulmo@...205... wrote:
> " > I worked a little on the password stuff.  It's far from done.
> " > 
> " > This is against the svn latest.  I tested it in Linux and Cygwin.
> " > 
> " > It's still not ENDIAN and non-Linux/Cygwin safe.
> " > 
> " > Note that I fixed the auth IP# stuff, which wasn't working right before.
> " 
> " Sorry for the late reply -- I was out on a Debian/m68k porters meeting,
> " and had little time for anything but that this weekend.
> 
> I've been busy, too.  I read your email just now, and I think I can do all
> the things you said that I comprehend this weekend or next.  They seem
> reasonable.

Great :-)

> Thank you for your time.

You're welcome.

[...]
> " I don't think it's necessary to also put the address family in the
> " checksum. An IPv4 address is, by definition, different from an IPv6
> " address; so it should be impossible for anyone to get the same has that
> " would result from an IPv6 address by using an IPv4 address.
> 
> That's true.  I guess I try to do the right thing like think of cases
> where there might be another family.

Well, we can deal with that when it happens. For now, I don't think it's
very useful; and defining our own constant for the address family just
so it'd work everywhere makes the code somewhat more hairy.

[...]
> " Also, I don't care for non-modern operating systems. If something
> " doesn't support AF_INET6 in this day and age, well...
> 
> Cygwin, under Microsoft.  Byzantine, huh?

Oh, bugger.

Well, that settles it, then; guess we'll have to conditionally compile
for v6.

[...]
> " > +static void socktonum(struct sockaddr_storage *sastg, socklen_t sastg_len, unsigned char *dat, size_t *pos) {
> " > +	/* typedef unsigned short uint16_t; / * comment out if already defined */
> " 
> " uint16_t is part of C99, so should be defined everywhere. However, C99
> " does not specify that 'uint16_t' is always 'unsigned short'; all it
> " defines is sizeof(char) == 1 && sizeof(char) <= sizeof(short) <=
> " sizeof(int), etc.
> 
> Does it at least define a 16 bit unsigned type to be a 16 bit unsigned
> type?  Or are you telling me that it does not?

Eh, sure. What I'm saying is that doing a typedef of uint16_t should not
be necessary; uint16_t should always be defined as 'the unsigned type
that provides a 16bit integer'. Unless the particular system doesn't
/have/ a 16bit unsigned integer, in which case it shouldn't be defined
-- but then you're fucked, anyway.

> " > +	if(who==NBD_WHO_CLIENT) goto getrandom;
> " 
> " Ewww. There are valid cases for goto, when it increases the readability
> " of code (e.g., for errorhandling). However, this is not one of them;
> " this is spaghetti code.
> 
> I've never been a Pascal type, and never will be.  I'll have to take
> your word for it, and expand the code to be to me hard to read and
> hard to work with and more buggy, but perhaps I'm just dumb about that
> and after doing it for a while I'll see how to streamline it.  As it
> is, it is just 6 lines of code and, to me, no fuss and no muss.  It
> will take pages upon pages of code changes to rearrange it, but I'll
> try.

All you need are two functions.

Something like:

if(server) {
	auth_from_remote();
	auth_to_remote();
} else {
	auth_to_remote();
	auth_from_remote();
}

with auth_from_remote() being a function that sends a random number,
fetches a hash, and compares it for validity; and auth_to_remote() being
a function that fetches a random number, computes, and sends a hash

[...]
> " We'll probably be using something like
> " 
> " #define NBD_FLAG_AUTH (1 << 2)
> 
> Will do.
> 
> " Which, if set, triggers the authentication after negotiation.
> 
> That's curious.  I'll go ahead and set it, but checking for it being
> set isn't something I'll do unless there is an error.  Actually, ok,
> that is why to use it: for errors, so the administrator knows what is
> going on.

Indeed. It would also allow either peer to fall back to a
non-authenticated connection, provided the peer allows it.

> " I'm actually also thinking about splitting the 'reserved' field in
> " two; one half would be for required options (if the remote end of
> " the connection sees a bit set there that it knows nothing about, it
> " aborts the connection), while the other would be for optional flags
> " (that can be safely ignored). 128 bytes is more than enough, anyway.
> 
> Ok.  Is read only optional or required?  I'll just put the flag in
> there and not worry about the optional or required.

Optional, I guess (if the client ignores it, the server will drop the
connection when the client tries to read anyway; but that's been the
historical behaviour even before that flag was set).

[...]
> " > +#ifndef NBD_AUTH_C
> " > +
> " >  void err(const char *s) G_GNUC_NORETURN;
> " 
> " Why the ifdef here?
> 
> See above.  I'll have to re-examine.  I don't promise to do this in my
> next pass; header files can be a pain.  I'll try.

Right.

-- 
<Lo-lan-do> Home is where you have to wash the dishes.
  -- #debian-devel, Freenode, 2004-09-22