Re: [Nbd] [PATCHv2 5/6] Add TLS support to server

2016-04-12 Thread Wouter Verhelst 
On Tue, Apr 12, 2016 at 03:15:27PM +0100, Alex Bligh wrote:
> Wouter,
> 
> On 12 Apr 2016, at 15:04, Wouter Verhelst  wrote:
> 
> > On Mon, Apr 11, 2016 at 06:15:38PM +0100, Alex Bligh wrote:
> > [...]
> >> +#ifdef WITH_GNUTLS
> > [...]
> >> +#else
> >> +
> >> +  send_reply(opt, *net, NBD_REP_ERR_UNSUP, 0, NULL);
> > 
> > NBD_REP_ERR_POLICY (or NBD_REP_ERR_PLATFORM, perhaps).
> 
> But this is *without* TLS support compiled in. Surely the correct error
> is NBD_REP_ERR_UNSUP, i.e. the same error as it gives now without
> the TLS code? IE it can NEVER work against this server, unless
> the server's code is changed. It's not a local policy thing.

PLATFORM is "this option is not supported on the platform where it was
compiled". If that platform doesn't have GnuTLS, then you disable
STARTTLS, so it can't work.

Maybe the names were wrong, but the plan was:

INVALID -- client sent something obviously wrong
POLICY -- server admin did something wrong or disabled the option
UNSUP -- server does not have code to handle request
PLATFORM -- server does have code to handle request, but it's not
  compiled in for whatever reason (e.g., something required on the
  platform is not available)

Obviously the last applies here.

(and just as obviously POLICY doesn't, either -- the "perhaps" line
above is the wrong way round, sorry)

The solution to an INVALID error is "fix the damn client"
The solution to a POLICY error is "fix the damn server config"
The solution to a PLATFORM error is "recompile the server and/or run it
on the right system"
The solution to an UNSUP error is "implement missing functionality"

There's a clear difference between the latter two.

> > You should think of NBD_REP_ERR_INVALID as 4xx errors (i.e., "you're
> > doing it wrong"), and of NBD_REP_ERR_POLICY (or NBD_REP_ERR_PLATFORM) as
> > 5xx errors ("something went wrong on my end").
> 
> So nothing went wrong server end. He's running a server without
> TLS built in.

It's a detail, I realize, but that's not what this was meant to do.

> > NBD_REP_ERR_UNSUP really is only meant for "I don't know what the 
> > you're talking about". It should only be referenced just once in a
> > server (in a "default:" case of a switch statement)
> 
> He doesn't know what you're talking about - no TLS!

He does, you've gone into a separate function.

> > [...]
> >> -sock_flags_old = fcntl(net, F_GETFL, 0);
> >> -if (sock_flags_old == -1) {
> >> -msg(LOG_ERR, "Failed to get socket flags");
> >> -goto handler_err;
> >> -}
> >> -
> >> -sock_flags_new = sock_flags_old & ~O_NONBLOCK;
> >> -if (sock_flags_new != sock_flags_old &&
> >> -fcntl(net, F_SETFL, sock_flags_new) == -1) {
> >> -msg(LOG_ERR, "Failed to set socket to blocking mode");
> >> -goto handler_err;
> >> -}
> >> +  if (set_nonblocking(client->net, 0) < 0) {
> >> +  msg(LOG_ERR, "Failed to set socket to blocking mode");
> >> +  goto handler_err;
> >> +  }
> > 
> > Some whitespace errors there.
> 
> Couldn't see them, but reindented it for v4.

:)

> >> if (set_peername(net, client)) {
> >> msg(LOG_ERR, "Failed to set peername");
> >> @@ -2241,11 +2375,15 @@ handle_modern_connection(GArray *const servers, 
> >> const int sock)
> >> 
> >> msg(LOG_INFO, "Starting to serve");
> >> serveconnection(client);
> >> +  close (net);
> >> +  if (client->net != net)
> >> +  close (client->net);
> > 
> > Probably safer to have a block here, rather than a single line?
> 
> You mean around "close (client->net)"? Sure, will do for v4. Is there
> some guideline you use? (CodingStyle is remarkably tolerant).

CodingStyle is also remarkably ancient ;-)

I should probably update or ditch it. Some other time.

-- 
< ron> I mean, the main *practical* problem with C++, is there's like a dozen
   people in the world who think they really understand all of its rules,
   and pretty much all of them are just lying to themselves too.
 -- #debian-devel, OFTC, 2016-02-12

--
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial!
https://ad.doubleclick.net/ddm/clk/302982198;130105516;z
___
Nbd-general mailing list
Nbd-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nbd-general

Re: [Nbd] [PATCHv2 5/6] Add TLS support to server

2016-04-12 Thread Alex Bligh 
Wouter,

On 12 Apr 2016, at 15:04, Wouter Verhelst  wrote:

> On Mon, Apr 11, 2016 at 06:15:38PM +0100, Alex Bligh wrote:
> [...]
>> +#ifdef WITH_GNUTLS
> [...]
>> +#else
>> +
>> +send_reply(opt, *net, NBD_REP_ERR_UNSUP, 0, NULL);
> 
> NBD_REP_ERR_POLICY (or NBD_REP_ERR_PLATFORM, perhaps).

But this is *without* TLS support compiled in. Surely the correct error
is NBD_REP_ERR_UNSUP, i.e. the same error as it gives now without
the TLS code? IE it can NEVER work against this server, unless
the server's code is changed. It's not a local policy thing.

> You should think of NBD_REP_ERR_INVALID as 4xx errors (i.e., "you're
> doing it wrong"), and of NBD_REP_ERR_POLICY (or NBD_REP_ERR_PLATFORM) as
> 5xx errors ("something went wrong on my end").

So nothing went wrong server end. He's running a server without
TLS built in.

> NBD_REP_ERR_UNSUP really is only meant for "I don't know what the 
> you're talking about". It should only be referenced just once in a
> server (in a "default:" case of a switch statement)

He doesn't know what you're talking about - no TLS!

> [...]
>> -sock_flags_old = fcntl(net, F_GETFL, 0);
>> -if (sock_flags_old == -1) {
>> -msg(LOG_ERR, "Failed to get socket flags");
>> -goto handler_err;
>> -}
>> -
>> -sock_flags_new = sock_flags_old & ~O_NONBLOCK;
>> -if (sock_flags_new != sock_flags_old &&
>> -fcntl(net, F_SETFL, sock_flags_new) == -1) {
>> -msg(LOG_ERR, "Failed to set socket to blocking mode");
>> -goto handler_err;
>> -}
>> +if (set_nonblocking(client->net, 0) < 0) {
>> +msg(LOG_ERR, "Failed to set socket to blocking mode");
>> +goto handler_err;
>> +}
> 
> Some whitespace errors there.

Couldn't see them, but reindented it for v4.

>> if (set_peername(net, client)) {
>> msg(LOG_ERR, "Failed to set peername");
>> @@ -2241,11 +2375,15 @@ handle_modern_connection(GArray *const servers, 
>> const int sock)
>> 
>> msg(LOG_INFO, "Starting to serve");
>> serveconnection(client);
>> +close (net);
>> +if (client->net != net)
>> +close (client->net);
> 
> Probably safer to have a block here, rather than a single line?

You mean around "close (client->net)"? Sure, will do for v4. Is there
some guideline you use? (CodingStyle is remarkably tolerant).

-- 
Alex Bligh





--
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial!
https://ad.doubleclick.net/ddm/clk/302982198;130105516;z
___
Nbd-general mailing list
Nbd-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nbd-general

Re: [Nbd] [PATCHv2 5/6] Add TLS support to server

2016-04-12 Thread Wouter Verhelst 
On Mon, Apr 11, 2016 at 06:15:38PM +0100, Alex Bligh wrote:
[...]
> +#ifdef WITH_GNUTLS
[...]
> +#else
> +
> + send_reply(opt, *net, NBD_REP_ERR_UNSUP, 0, NULL);

NBD_REP_ERR_POLICY (or NBD_REP_ERR_PLATFORM, perhaps).

You should think of NBD_REP_ERR_INVALID as 4xx errors (i.e., "you're
doing it wrong"), and of NBD_REP_ERR_POLICY (or NBD_REP_ERR_PLATFORM) as
5xx errors ("something went wrong on my end").

NBD_REP_ERR_UNSUP really is only meant for "I don't know what the 
you're talking about". It should only be referenced just once in a
server (in a "default:" case of a switch statement)

[...]
> -sock_flags_old = fcntl(net, F_GETFL, 0);
> -if (sock_flags_old == -1) {
> -msg(LOG_ERR, "Failed to get socket flags");
> -goto handler_err;
> -}
> -
> -sock_flags_new = sock_flags_old & ~O_NONBLOCK;
> -if (sock_flags_new != sock_flags_old &&
> -fcntl(net, F_SETFL, sock_flags_new) == -1) {
> -msg(LOG_ERR, "Failed to set socket to blocking mode");
> -goto handler_err;
> -}
> + if (set_nonblocking(client->net, 0) < 0) {
> + msg(LOG_ERR, "Failed to set socket to blocking mode");
> + goto handler_err;
> + }

Some whitespace errors there.

>  if (set_peername(net, client)) {
>  msg(LOG_ERR, "Failed to set peername");
> @@ -2241,11 +2375,15 @@ handle_modern_connection(GArray *const servers, const 
> int sock)
>  
>  msg(LOG_INFO, "Starting to serve");
>  serveconnection(client);
> + close (net);
> + if (client->net != net)
> + close (client->net);

Probably safer to have a block here, rather than a single line?

[...]

-- 
< ron> I mean, the main *practical* problem with C++, is there's like a dozen
   people in the world who think they really understand all of its rules,
   and pretty much all of them are just lying to themselves too.
 -- #debian-devel, OFTC, 2016-02-12

--
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial!
https://ad.doubleclick.net/ddm/clk/302982198;130105516;z
___
Nbd-general mailing list
Nbd-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nbd-general

[Nbd] [PATCHv2 5/6] Add TLS support to server

2016-04-11 Thread Alex Bligh 
Known problems / potential issues:

* It now passes a pointer to genconf around so handle_starttls can
  get at the certificates. This is a pity.

* It forks() the TLS proxy child using spawn_child. If we use fork()
  we get complaints about unknown children on SIGCHILD. If we use
  this method, each TLS connection potentially counts as 2 rather than one
  connection as far as maxconnections is concerned. In fact I don't
  *think* this happens because the hashtable itself isn't shared
  across fork(). We need to have a deeper think about how best to
  do this.

* Does not currently set minimum TLS version.

* Minimal testing to date.

Signed-off-by: Alex Bligh 
---
 cliserv.h|   1 +
 nbd-server.c | 196 +++
 2 files changed, 170 insertions(+), 27 deletions(-)

diff --git a/cliserv.h b/cliserv.h
index 9c71cb8..2039cf5 100644
--- a/cliserv.h
+++ b/cliserv.h
@@ -95,6 +95,7 @@ void readit(int f, void *buf, size_t len);
 #define NBD_OPT_EXPORT_NAME(1) /** Client wants to select a named 
export (is followed by name of export) */
 #define NBD_OPT_ABORT  (2) /** Client wishes to abort negotiation 
*/
 #define NBD_OPT_LIST   (3) /** Client request list of supported 
exports (not followed by data) */
+#define NBD_OPT_STARTTLS   (5) /** Client wishes to start TLS */
 
 /* Replies the server can send during negotiation */
 #define NBD_REP_ACK(1) /** ACK a request. Data: option number 
to be acked */
diff --git a/nbd-server.c b/nbd-server.c
index 729156b..cb5756d 100644
--- a/nbd-server.c
+++ b/nbd-server.c
@@ -129,6 +129,10 @@
 #include 
 #endif
 
+#ifdef WITH_GNUTLS
+#include "crypto-gnutls.h"
+#endif
+
 /** Default position of the config file */
 #ifndef SYSCONFDIR
 #define SYSCONFDIR "/etc"
@@ -246,6 +250,8 @@ struct generic_conf {
gint threads;   /**< maximum number of parallel threads we want 
to run */
 };
 
+static pid_t spawn_child();
+
 /**
  * Translate a command name into human readable form
  *
@@ -888,6 +894,20 @@ static void sighup_handler(const int s G_GNUC_UNUSED) {
 }
 
 /**
+ * Set a socket to blocking or non-blocking
+ *
+ * @param fd The socket's FD
+ * @param nb non-zero to set to non-blocking, else 0 to set to blocking
+ * @return 0 - OK, -1 failed
+ */
+int set_nonblocking(int fd, int nb) {
+   int sf = fcntl (fd, F_GETFL, 0);
+   if (sf == -1)
+   return -1;
+   return fcntl (fd, F_SETFL, nb ? (sf | O_NONBLOCK) : (sf & ~O_NONBLOCK));
+}
+
+/**
  * Get the file handle and offset, given an export offset.
  *
  * @param client The client we're serving for
@@ -1256,7 +1276,7 @@ static void send_reply(uint32_t opt, int net, uint32_t 
reply_type, size_t datasi
}
 }
 
-static CLIENT* handle_export_name(uint32_t opt, int net, GArray* servers, 
uint32_t cflags) {
+static CLIENT* handle_export_name(uint32_t opt, int net, GArray* servers, 
uint32_t cflags, int tlsfd) {
uint32_t namelen;
char* name;
int i;
@@ -1280,6 +1300,11 @@ static CLIENT* handle_export_name(uint32_t opt, int net, 
GArray* servers, uint32
for(i=0; ilen; i++) {
SERVER* serve = &(g_array_index(servers, SERVER, i));
if(!strcmp(serve->servename, name)) {
+   if ((tlsfd < 0) && (serve->flags & F_TLSONLY || 
glob_flags & F_TLSONLY)) {
+   err("Negotiation failed: Attempt to read from 
TLS only export without TLS");
+   free(name);
+   return NULL;
+   }
CLIENT* client = g_new0(CLIENT, 1);
client->server = serve;
client->exportsize = OFFT_MAX;
@@ -1324,16 +1349,125 @@ static void handle_list(uint32_t opt, int net, GArray* 
servers, uint32_t cflags)
send_reply(opt, net, NBD_REP_ACK, 0, NULL);
 }
 
+static int tlserrout (void *opaque, const char *format, va_list ap) {
+   char buf[1024];
+   if (vsnprintf(buf, 1024, format, ap) < 0)
+   return -1;
+   buf[1023] = 0; // in case it overran the buffer
+   err_nonfatal(buf);
+   return 0;
+}
+
+static void handle_starttls(uint32_t opt, int *net, int *tlsfd, GArray* 
servers, uint32_t cflags,
+   struct generic_conf *genconf) {
+   uint32_t len;
+   int i;
+   char buf[1024];
+   char *ptr = buf + sizeof(len);
+   int plainfd[2]; // [0] is used by the proxy, [1] is used by NBD
+#ifdef WITH_GNUTLS
+   tlssession_t *s = NULL;
+   int ret;
+#endif
+   if (read(*net, , sizeof(len)) < 0)
+   err("Negotiation failed/8: %m");
+   len = ntohl(len);
+   if (len || (*tlsfd >= 0)) {
+   send_reply(opt, *net, NBD_REP_ERR_INVALID, 0, NULL);
+   }
+
+#ifdef WITH_GNUTLS
+   if (socketpair(AF_UNIX, SOCK_STREAM, 0, plainfd) < 0) {
+