" Hi Brad,
"
" On Fri, Aug 29, 2008 at 02:37:29PM -0700, [EMAIL PROTECTED] 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. Thank you for your time.
" Note that you cannot nest comments; i.e., the first line of the above
" two is commented out, while the second and all the lines up to your
" 'final' end-of-comment marker below are not.
I'll remove that.
" 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.
" At least, if that is not the case, then our hash algorithm is broken. If
" you can indeed find an example where doing this is possible, by all
" means report it :-)
"
" The ability to remove these macros from the file would make it a bit
" more readable, I think.
"
" > +#define Addr6(y) (((struct sockaddr_in6*)y)->sin6_addr)
[...]
" I prefer if you'd use something like
" struct sockaddr_in* sa4 = (struct sockaddr_in*) sastg;
" struct sockaddr_in6* sa6 = (struct sockaddr_in6*) sastg;
Ok, that's probably straightforward.
" 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? All their source sources
(e.g., GNU) and OSs (XP, etc.) support IPv6, but Cygwin does not and
doesn't include it in their source (including their header include
files); they probably have to take the code out for IPv6 manually
these days since everyone else already has it.
"
" > +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?
" This is unsafe in the case of AF_INET6. struct sockaddr_storage is in
" fact _smaller_ than struct sockaddr_in6
Thank you. I did not know that.
" > +static void nbd_authhash(uint8_t *digest, unsigned char *hashdat, size_t
hashlen, char *pass, size_t passlen) {
" > + SHA512_CTX hashcontext;
" > +
" > + SHA512_Init(&hashcontext);
" > + SHA512_Update(&hashcontext, hashdat, hashlen);
" > + SHA512_Update(&hashcontext, (unsigned char*)pass, passlen);
" > + SHA512_Final(digest,&hashcontext);
" > +}
"
" It may be easier to understand if this function were to return the
" digest (in fact, my first comment said something along the lines of
" 'you forgot to actually do something with the hash here').
I copied the function pattern from the source examples (with my own
call path because of the type of data I have), and I have to say they
made sense to me, but I expect a wide range of things. So, I can call
the function void* instead of void, and stick a "return digest" at the
end. Considering that I have amalgamated hashing into one session,
there are more possibilities.
" > + 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.
" If we're not going to be compiling in the auth code, it's probably best
" to completely leave out the auth.c and sha2.c files, then. This is
" pretty easy in automake/autoconf; if you don't know how, don't worry,
" I'll do it before applying the patch :-)
I absolutely agree, and would love you doing the automake/autoconf stuff.
I barely got to the point where I can even invoke automake/autoconf.
I did put a flag in there for NBD_AUTH #define.
" > /* Client/server protocol is as follows:
" > - Send INIT_PASSWD
" > + Password authentication if specified
" > + Send NBD_HELLO
"
" No, that's not what I meant to suggest earlier.
"
" > Send 64-bit cliserv_magic
" > Send 64-bit size of exported device
" > Send 128 bytes of zeros (reserved for future use)
"
" This should have one extra bit set, signalling that password
" authentication is to be used; see NBD_FLAG_READ_ONLY in the current
" source code.
Ahh, so I'll do that.
" 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. Excellent. My first idea of obfuscation was weak anyway,
because obfuscation is weak.
" 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.
" > */
" >
" > +#include "config.h"
" > +
" > +#include "lfs.h"
"
" nbd-client and nbd-server both include these files themselves before
" this point. Also, the <> syntaxis is used for other things so that VPATH
" builds work (automake adds '-I.' for that purpose).
Ok, <> will do. But the include pattern is a bit tricky because there
are a bunch of things that clobber other things if they are in the
previous ordering; perhaps I took them all out, but that's why I did it
while I was doing it.
" > +#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.
" > void err(const char *s) {
" > @@ -111,14 +125,6 @@ void err(const char *s) {
" > exit(1);
" > }
" >
" > -void logging(void) {
" > -#ifdef ISSERVER
" > - openlog(MY_NAME, LOG_PID, LOG_DAEMON);
" > -#endif
" > - setvbuf(stdout, NULL, _IONBF, 0);
" > - setvbuf(stderr, NULL, _IONBF, 0);
" > -}
"
" Yes -- I know I said it should probably be in a separate .c file, but
" that doesn't mean it has to be copied to nbd-client.c and nbd-server.c
" :-)
It's a step in the right direction :) I originally made a cliserv.c,
but it got emptied out, so I removed it. Perhaps I can go back to
that.
-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Nbd-general mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/nbd-general
