Re: syslogd SSL3_WRITE_PENDING:bad write retry

2015-02-19 Thread Alexander Bluhm 
On Tue, Feb 17, 2015 at 09:10:05AM +0100, Reyk Floeter wrote:
 But workaround is a harsh word - this is the way you were supposed to
 use SSL_write().  It is adapted from relayd and was turned into
 tls_write().

I came from the other direction.  I had no idea about SSL API and
expected to use libtls as a simple replacement for plain TCP.  The
partial write and moving buffer semantics are not documented within
libtls, so I tried the naive way.

 I even wonder why you didn't pick this up in your
 evbuffer TLS implementation for syslogd;  looks a bit reinvented.

My goal was to keep my TLS buffer imlementation as close as possible
to libevent.  So I can merge it back into libevent.  If we don't
want this, I can still clean it up.

  - set SSL_MODE_ENABLE_PARTIAL_WRITE in libtls
  - remove the clt_buf workaround in httpd
  - ignore ftp client
  
 
 This approach sounds sane and I would love to have tls_write(3) behave
 just like write(2).

Yes, but after unlock.

 - What is the impact of adding the flags by default?

I think it makes sense for non-blocking operations in httpd and
syslogd.  They do propper buffer handling anyway.  The ftp client
is blocking and just wants to write the HTTP header.  There short
writes are uncomfortable.

 - What is the reason for OpenSSL's defaults?
 
 There is an old thread with some dicussion about it:
 http://marc.info/?l=openssl-devm=118766345824094w=2

I found another thread from 2006.
http://marc.info/?t=11510126221r=1w=2

I have the impression Bodo Moeller knows something about it.

| If SSL_write() has started writing a first record, but delayed other
| data to later records, then it may have to return -1 to indicate
| a WANT_WRITE or WANT_READ condition.

My interpretation is, OpenSSL cannot indicate a short write in a
want read condition.  You must not change the buffer content or
decrease its length.  This buffer move check is a hint that you
must be careful.

Anyway, libtls is locked.  We can either release a broken syslogd
over TLS implementation or commit this diff.  It has the smallest
impact of everything I have tried.

ok or better idea?

bluhm

Index: usr.sbin/syslogd/evbuffer_tls.c
===
RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/evbuffer_tls.c,v
retrieving revision 1.2
diff -u -p -r1.2 evbuffer_tls.c
--- usr.sbin/syslogd/evbuffer_tls.c 30 Jan 2015 14:00:55 -  1.2
+++ usr.sbin/syslogd/evbuffer_tls.c 19 Feb 2015 16:47:44 -
@@ -186,6 +186,7 @@ buffertls_writecb(int fd, short event, v
if (res = 0)
goto error;
}
+   buftls-bt_flags = ~BT_WRITE_AGAIN;
 
event_set(bufev-ev_write, fd, EV_WRITE, buffertls_writecb, buftls);
if (EVBUFFER_LENGTH(bufev-output) != 0)
@@ -202,6 +203,7 @@ buffertls_writecb(int fd, short event, v
return;
 
  reschedule:
+   buftls-bt_flags |= BT_WRITE_AGAIN;
if (EVBUFFER_LENGTH(bufev-output) != 0)
bufferevent_add(bufev-ev_write, bufev-timeout_write);
return;
@@ -276,6 +278,7 @@ buffertls_set(struct buffertls *buftls, 
event_set(bufev-ev_write, fd, EV_WRITE, buffertls_writecb, buftls);
buftls-bt_bufev = bufev;
buftls-bt_ctx = ctx;
+   buftls-bt_flags = 0;
 }
 
 void
Index: usr.sbin/syslogd/evbuffer_tls.h
===
RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/evbuffer_tls.h,v
retrieving revision 1.1
diff -u -p -r1.1 evbuffer_tls.h
--- usr.sbin/syslogd/evbuffer_tls.h 18 Jan 2015 19:37:59 -  1.1
+++ usr.sbin/syslogd/evbuffer_tls.h 19 Feb 2015 16:47:44 -
@@ -28,6 +28,8 @@ struct buffertls {
struct bufferevent  *bt_bufev;
struct tls  *bt_ctx;
const char  *bt_hostname;
+   int  bt_flags;
+#define BT_WRITE_AGAIN 0x1
 };
 
 void   buffertls_set(struct buffertls *, struct bufferevent *, struct tls *,
Index: usr.sbin/syslogd/syslogd.c
===
RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/syslogd.c,v
retrieving revision 1.156
diff -u -p -r1.156 syslogd.c
--- usr.sbin/syslogd/syslogd.c  14 Feb 2015 09:02:15 -  1.156
+++ usr.sbin/syslogd/syslogd.c  19 Feb 2015 16:47:44 -
@@ -1265,8 +1265,22 @@ fprintlog(struct filed *f, int flags, ch
}
break;
 
-   case F_FORWTCP:
case F_FORWTLS:
+   if (f-f_un.f_forw.f_buftls.bt_flags  BT_WRITE_AGAIN) {
+   /*
+* After an OpenSSL SSL_ERROR_WANT_WRITE you must not
+* modify the buffer pointer or length until the next
+* successful write.  Otherwise there will be an
+* error SSL3_WRITE_PENDING:bad write retry.
+* XXX This should be handled in

Re: syslogd SSL3_WRITE_PENDING:bad write retry

2015-02-19 Thread Ted Unangst 
Alexander Bluhm wrote:
 Anyway, libtls is locked.  We can either release a broken syslogd
 over TLS implementation or commit this diff.  It has the smallest
 impact of everything I have tried.
 
 ok or better idea?

ok

Re: syslogd SSL3_WRITE_PENDING:bad write retry

2015-02-16 Thread Alexander Bluhm 
On Mon, Feb 16, 2015 at 10:17:05AM +0100, Reyk Floeter wrote:
 On Sun, Feb 15, 2015 at 11:21:45PM +0100, Alexander Bluhm wrote:
  On Fri, Feb 13, 2015 at 02:44:18PM -0500, Ted Unangst wrote:
   I think this is ok, but it needs some basic load testing with httpd (and 
   ftp)
   as well.
  
  I have tested ftp https by downloading ports distfiles.
  I have done basic testing with httpd.
  
  Could someone test this diff who has a busy httpd server using
  https?
  
  bluhm
  
 
 I'm running it on some domains without problems so far but the pages
 aren't so busy.  Otherwise OK reyk@

I have tried to download the 227 MB install56.iso from httpd.  It
is very slow and fails after half the data.  There is a copied
buffer in server_tls_writecb() to workaround the pending write.
Without that httpd works.

The ftp client does not check for parital writes in tls_write().
That seems to work anyway as it only writes a short HTTP header and
the socket is blocking.

What to do now?

- set SSL_MODE_ENABLE_PARTIAL_WRITE in libtls
- remove the clt_buf workaround in httpd
- ignore ftp client

- set SSL_MODE_ENABLE_PARTIAL_WRITE in libtls optionally
- only syslogd does that

bluhm

Index: usr.sbin/httpd/httpd.h
===
RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/httpd/httpd.h,v
retrieving revision 1.80
diff -u -p -r1.80 httpd.h
--- usr.sbin/httpd/httpd.h  12 Feb 2015 10:05:29 -  1.80
+++ usr.sbin/httpd/httpd.h  16 Feb 2015 20:11:55 -
@@ -283,8 +283,6 @@ struct client {
in_port_tclt_port;
struct sockaddr_storage  clt_ss;
struct bufferevent  *clt_bev;
-   char*clt_buf;
-   size_t   clt_buflen;
struct evbuffer *clt_output;
struct event clt_ev;
void*clt_descreq;
Index: usr.sbin/httpd/server.c
===
RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/httpd/server.c,v
retrieving revision 1.59
diff -u -p -r1.59 server.c
--- usr.sbin/httpd/server.c 12 Feb 2015 04:40:23 -  1.59
+++ usr.sbin/httpd/server.c 16 Feb 2015 20:21:05 -
@@ -613,17 +613,8 @@ server_tls_writecb(int fd, short event, 
}
 
if (EVBUFFER_LENGTH(bufev-output)) {
-   if (clt-clt_buf == NULL) {
-   clt-clt_buflen = EVBUFFER_LENGTH(bufev-output);
-   if ((clt-clt_buf = malloc(clt-clt_buflen)) == NULL) {
-   what |= EVBUFFER_ERROR;
-   goto err;
-   }
-   bcopy(EVBUFFER_DATA(bufev-output),
-   clt-clt_buf, clt-clt_buflen);
-   }
-   ret = tls_write(clt-clt_tls_ctx, clt-clt_buf,
-   clt-clt_buflen, len);
+   ret = tls_write(clt-clt_tls_ctx, EVBUFFER_DATA(bufev-output),
+   EVBUFFER_LENGTH(bufev-output), len);
if (ret == TLS_READ_AGAIN || ret == TLS_WRITE_AGAIN) {
goto retry;
} else if (ret != 0) {
@@ -632,11 +623,6 @@ server_tls_writecb(int fd, short event, 
}
evbuffer_drain(bufev-output, len);
}
-   if (clt-clt_buf != NULL) {
-   free(clt-clt_buf);
-   clt-clt_buf = NULL;
-   clt-clt_buflen = 0;
-   }
 
if (EVBUFFER_LENGTH(bufev-output) != 0)
server_bufferevent_add(bufev-ev_write, bufev-timeout_write);
@@ -647,16 +633,11 @@ server_tls_writecb(int fd, short event, 
return;
 
  retry:
-   if (clt-clt_buflen != 0)
+   if (EVBUFFER_LENGTH(bufev-output) != 0)
server_bufferevent_add(bufev-ev_write, bufev-timeout_write);
return;
 
  err:
-   if (clt-clt_buf != NULL) {
-   free(clt-clt_buf);
-   clt-clt_buf = NULL;
-   clt-clt_buflen = 0;
-   }
(*bufev-errorcb)(bufev, what, bufev-cbarg);
 }
 
Index: lib/libtls/tls.c
===
RCS file: /data/mirror/openbsd/cvs/src/lib/libtls/tls.c,v
retrieving revision 1.7
diff -u -p -r1.7 tls.c
--- lib/libtls/tls.c7 Feb 2015 09:50:09 -   1.7
+++ lib/libtls/tls.c16 Feb 2015 19:58:35 -
@@ -183,6 +183,9 @@ err:
 int
 tls_configure_ssl(struct tls *ctx)
 {
+   SSL_CTX_set_mode(ctx-ssl_ctx, SSL_MODE_ENABLE_PARTIAL_WRITE);
+   SSL_CTX_set_mode(ctx-ssl_ctx, SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER);
+
SSL_CTX_set_options(ctx-ssl_ctx, SSL_OP_NO_SSLv2);
SSL_CTX_set_options(ctx-ssl_ctx, SSL_OP_NO_SSLv3);

Re: syslogd SSL3_WRITE_PENDING:bad write retry

2015-02-16 Thread Reyk Floeter 
On Mon, Feb 16, 2015 at 10:03:51PM +0100, Alexander Bluhm wrote:
 On Mon, Feb 16, 2015 at 10:17:05AM +0100, Reyk Floeter wrote:
  On Sun, Feb 15, 2015 at 11:21:45PM +0100, Alexander Bluhm wrote:
   On Fri, Feb 13, 2015 at 02:44:18PM -0500, Ted Unangst wrote:
I think this is ok, but it needs some basic load testing with httpd 
(and ftp)
as well.
   
   I have tested ftp https by downloading ports distfiles.
   I have done basic testing with httpd.
   
   Could someone test this diff who has a busy httpd server using
   https?
   
   bluhm
   
  
  I'm running it on some domains without problems so far but the pages
  aren't so busy.  Otherwise OK reyk@
 
 I have tried to download the 227 MB install56.iso from httpd.  It
 is very slow and fails after half the data.  There is a copied
 buffer in server_tls_writecb() to workaround the pending write.
 Without that httpd works.
 

OK, good finding.  I haven't tested large files yet.

But workaround is a harsh word - this is the way you were supposed to
use SSL_write().  It is adapted from relayd and was turned into
tls_write().  I even wonder why you didn't pick this up in your
evbuffer TLS implementation for syslogd;  looks a bit reinvented.

 The ftp client does not check for parital writes in tls_write().
 That seems to work anyway as it only writes a short HTTP header and
 the socket is blocking.
 
 What to do now?
 
 - set SSL_MODE_ENABLE_PARTIAL_WRITE in libtls
 - remove the clt_buf workaround in httpd
 - ignore ftp client
 

This approach sounds sane and I would love to have tls_write(3) behave
just like write(2).

But It was not a workaround.  The OpenSSL people didn't like
SSL_MODE_ENABLE_PARTIAL_WRITE and SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER
and they weren't the default for a reason.  httpd and relayd are
currently doing it the recommended way - and we have to figure out if
this recommendation was wrong.  I initially asked tedu@ about the
impacts of these flags in httpd and I had the described workaround
in mind but I failed to point it out.

As I said, I would like to get your patch applied but I'd also like to
understand OpenSSL's reasons to change the semantics:

- What is the impact of adding the flags by default?

- What is the reason for OpenSSL's defaults?

There is an old thread with some dicussion about it:
http://marc.info/?l=openssl-devm=118766345824094w=2

But I still don't get it.  Was it just an optimization for speed?

Reyk

 - set SSL_MODE_ENABLE_PARTIAL_WRITE in libtls optionally
 - only syslogd does that
 
 bluhm
 
 Index: usr.sbin/httpd/httpd.h
 ===
 RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/httpd/httpd.h,v
 retrieving revision 1.80
 diff -u -p -r1.80 httpd.h
 --- usr.sbin/httpd/httpd.h12 Feb 2015 10:05:29 -  1.80
 +++ usr.sbin/httpd/httpd.h16 Feb 2015 20:11:55 -
 @@ -283,8 +283,6 @@ struct client {
   in_port_tclt_port;
   struct sockaddr_storage  clt_ss;
   struct bufferevent  *clt_bev;
 - char*clt_buf;
 - size_t   clt_buflen;
   struct evbuffer *clt_output;
   struct event clt_ev;
   void*clt_descreq;
 Index: usr.sbin/httpd/server.c
 ===
 RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/httpd/server.c,v
 retrieving revision 1.59
 diff -u -p -r1.59 server.c
 --- usr.sbin/httpd/server.c   12 Feb 2015 04:40:23 -  1.59
 +++ usr.sbin/httpd/server.c   16 Feb 2015 20:21:05 -
 @@ -613,17 +613,8 @@ server_tls_writecb(int fd, short event, 
   }
  
   if (EVBUFFER_LENGTH(bufev-output)) {
 - if (clt-clt_buf == NULL) {
 - clt-clt_buflen = EVBUFFER_LENGTH(bufev-output);
 - if ((clt-clt_buf = malloc(clt-clt_buflen)) == NULL) {
 - what |= EVBUFFER_ERROR;
 - goto err;
 - }
 - bcopy(EVBUFFER_DATA(bufev-output),
 - clt-clt_buf, clt-clt_buflen);
 - }
 - ret = tls_write(clt-clt_tls_ctx, clt-clt_buf,
 - clt-clt_buflen, len);
 + ret = tls_write(clt-clt_tls_ctx, EVBUFFER_DATA(bufev-output),
 + EVBUFFER_LENGTH(bufev-output), len);
   if (ret == TLS_READ_AGAIN || ret == TLS_WRITE_AGAIN) {
   goto retry;
   } else if (ret != 0) {
 @@ -632,11 +623,6 @@ server_tls_writecb(int fd, short event, 
   }
   evbuffer_drain(bufev-output, len);
   }
 - if (clt-clt_buf != NULL) {
 - free(clt-clt_buf);
 - clt-clt_buf = NULL;
 - clt-clt_buflen = 0;
 - }
  
   if (EVBUFFER_LENGTH(bufev-output) != 0)
   server_bufferevent_add(bufev-ev_write, bufev-timeout_write);
 @@ -647,16 +633,11 @@

Re: syslogd SSL3_WRITE_PENDING:bad write retry

2015-02-16 Thread Reyk Floeter 
On Sun, Feb 15, 2015 at 11:21:45PM +0100, Alexander Bluhm wrote:
 On Fri, Feb 13, 2015 at 02:44:18PM -0500, Ted Unangst wrote:
  I think this is ok, but it needs some basic load testing with httpd (and 
  ftp)
  as well.
 
 I have tested ftp https by downloading ports distfiles.
 I have done basic testing with httpd.
 
 Could someone test this diff who has a busy httpd server using
 https?
 
 bluhm
 

I'm running it on some domains without problems so far but the pages
aren't so busy.  Otherwise OK reyk@

Reyk

   Index: lib/libtls/tls.c
   ===
   RCS file: /data/mirror/openbsd/cvs/src/lib/libtls/tls.c,v
   retrieving revision 1.7
   diff -u -p -r1.7 tls.c
   --- lib/libtls/tls.c  7 Feb 2015 09:50:09 -   1.7
   +++ lib/libtls/tls.c  13 Feb 2015 18:33:31 -
   @@ -183,6 +183,9 @@ err:
int
tls_configure_ssl(struct tls *ctx)
{
   + SSL_CTX_set_mode(ctx-ssl_ctx, SSL_MODE_ENABLE_PARTIAL_WRITE);
   + SSL_CTX_set_mode(ctx-ssl_ctx, SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER);
   +
 SSL_CTX_set_options(ctx-ssl_ctx, SSL_OP_NO_SSLv2);
 SSL_CTX_set_options(ctx-ssl_ctx, SSL_OP_NO_SSLv3);

 

--

Re: syslogd SSL3_WRITE_PENDING:bad write retry

2015-02-15 Thread Alexander Bluhm 
On Fri, Feb 13, 2015 at 02:44:18PM -0500, Ted Unangst wrote:
 I think this is ok, but it needs some basic load testing with httpd (and ftp)
 as well.

I have tested ftp https by downloading ports distfiles.
I have done basic testing with httpd.

Could someone test this diff who has a busy httpd server using
https?

bluhm

  Index: lib/libtls/tls.c
  ===
  RCS file: /data/mirror/openbsd/cvs/src/lib/libtls/tls.c,v
  retrieving revision 1.7
  diff -u -p -r1.7 tls.c
  --- lib/libtls/tls.c7 Feb 2015 09:50:09 -   1.7
  +++ lib/libtls/tls.c13 Feb 2015 18:33:31 -
  @@ -183,6 +183,9 @@ err:
   int
   tls_configure_ssl(struct tls *ctx)
   {
  +   SSL_CTX_set_mode(ctx-ssl_ctx, SSL_MODE_ENABLE_PARTIAL_WRITE);
  +   SSL_CTX_set_mode(ctx-ssl_ctx, SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER);
  +
  SSL_CTX_set_options(ctx-ssl_ctx, SSL_OP_NO_SSLv2);
  SSL_CTX_set_options(ctx-ssl_ctx, SSL_OP_NO_SSLv3);

Re: syslogd SSL3_WRITE_PENDING:bad write retry

2015-02-13 Thread Alexander Bluhm 
On Thu, Feb 12, 2015 at 08:34:23PM +0100, Alexander Bluhm wrote:
 On Wed, Feb 11, 2015 at 11:30:03PM -0500, Ted Unangst wrote:
  Ted Unangst wrote:
   Alexander Bluhm wrote:
Hi,

During testing syslogd I got some strange error messages from libtls:
syslogd: loghost @tls://localhost:15878 connection error: write 
failed: error:1409F07F:SSL routines:SSL3_WRITE_PENDING:bad write retry

I found out that after an SSL_ERROR_WANT_WRITE, SSL_write must be
called with exactly the same parameter.  So I remember this event
and do not modify the event buffer.
   
   This is one of the hardest things to fix with the current tls_write
   implementation. Yes, we inherited the underlying API's requirement to 
   reuse
   the same buffer until it's completely written out. I dream of making 
   tls_write
   work just like write, but until then you'll need to do this.
   
  
  Actually, I think this may be much easier to fix. SSL_write can be 
  configured
  to support short writes.
  
  Try this.
 
 This has no effect.  I still get
 
 syslogd: loghost @tls://localhost:7972 connection error: write failed: 
 error:1409F07F:SSL routines:SSL3_WRITE_PENDING:bad write retry

libevent buffer are reallocated when growing.  So I also need
SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER.  This works.

Do we want both options in libtls?
ok?

bluhm

Index: lib/libtls/tls.c
===
RCS file: /data/mirror/openbsd/cvs/src/lib/libtls/tls.c,v
retrieving revision 1.7
diff -u -p -r1.7 tls.c
--- lib/libtls/tls.c7 Feb 2015 09:50:09 -   1.7
+++ lib/libtls/tls.c13 Feb 2015 18:33:31 -
@@ -183,6 +183,9 @@ err:
 int
 tls_configure_ssl(struct tls *ctx)
 {
+   SSL_CTX_set_mode(ctx-ssl_ctx, SSL_MODE_ENABLE_PARTIAL_WRITE);
+   SSL_CTX_set_mode(ctx-ssl_ctx, SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER);
+
SSL_CTX_set_options(ctx-ssl_ctx, SSL_OP_NO_SSLv2);
SSL_CTX_set_options(ctx-ssl_ctx, SSL_OP_NO_SSLv3);

Re: syslogd SSL3_WRITE_PENDING:bad write retry

2015-02-13 Thread Ted Unangst 
Alexander Bluhm wrote:
 On Thu, Feb 12, 2015 at 08:34:23PM +0100, Alexander Bluhm wrote:
  On Wed, Feb 11, 2015 at 11:30:03PM -0500, Ted Unangst wrote:
   Ted Unangst wrote:
Alexander Bluhm wrote:
 Hi,
 
 During testing syslogd I got some strange error messages from libtls:
 syslogd: loghost @tls://localhost:15878 connection error: write 
 failed: error:1409F07F:SSL routines:SSL3_WRITE_PENDING:bad write retry
 
 I found out that after an SSL_ERROR_WANT_WRITE, SSL_write must be
 called with exactly the same parameter.  So I remember this event
 and do not modify the event buffer.

This is one of the hardest things to fix with the current tls_write
implementation. Yes, we inherited the underlying API's requirement to 
reuse
the same buffer until it's completely written out. I dream of making 
tls_write
work just like write, but until then you'll need to do this.

   
   Actually, I think this may be much easier to fix. SSL_write can be 
   configured
   to support short writes.
   
   Try this.
  
  This has no effect.  I still get
  
  syslogd: loghost @tls://localhost:7972 connection error: write failed: 
  error:1409F07F:SSL routines:SSL3_WRITE_PENDING:bad write retry
 
 libevent buffer are reallocated when growing.  So I also need
 SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER.  This works.
 
 Do we want both options in libtls?
 ok?

This makes sense. These are the semantics we want tls_write() to have.
I wasn't sure if both flags were needed or not, exactly the kind of confusion
we're trying to avoid.

I think this is ok, but it needs some basic load testing with httpd (and ftp)
as well.


 
 bluhm
 
 Index: lib/libtls/tls.c
 ===
 RCS file: /data/mirror/openbsd/cvs/src/lib/libtls/tls.c,v
 retrieving revision 1.7
 diff -u -p -r1.7 tls.c
 --- lib/libtls/tls.c  7 Feb 2015 09:50:09 -   1.7
 +++ lib/libtls/tls.c  13 Feb 2015 18:33:31 -
 @@ -183,6 +183,9 @@ err:
  int
  tls_configure_ssl(struct tls *ctx)
  {
 + SSL_CTX_set_mode(ctx-ssl_ctx, SSL_MODE_ENABLE_PARTIAL_WRITE);
 + SSL_CTX_set_mode(ctx-ssl_ctx, SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER);
 +
   SSL_CTX_set_options(ctx-ssl_ctx, SSL_OP_NO_SSLv2);
   SSL_CTX_set_options(ctx-ssl_ctx, SSL_OP_NO_SSLv3);

Re: syslogd SSL3_WRITE_PENDING:bad write retry

2015-02-12 Thread Alexander Bluhm 
On Wed, Feb 11, 2015 at 11:30:03PM -0500, Ted Unangst wrote:
 Ted Unangst wrote:
  Alexander Bluhm wrote:
   Hi,
   
   During testing syslogd I got some strange error messages from libtls:
   syslogd: loghost @tls://localhost:15878 connection error: write failed: 
   error:1409F07F:SSL routines:SSL3_WRITE_PENDING:bad write retry
   
   I found out that after an SSL_ERROR_WANT_WRITE, SSL_write must be
   called with exactly the same parameter.  So I remember this event
   and do not modify the event buffer.
  
  This is one of the hardest things to fix with the current tls_write
  implementation. Yes, we inherited the underlying API's requirement to reuse
  the same buffer until it's completely written out. I dream of making 
  tls_write
  work just like write, but until then you'll need to do this.
  
 
 Actually, I think this may be much easier to fix. SSL_write can be configured
 to support short writes.
 
 Try this.

This has no effect.  I still get

syslogd: loghost @tls://localhost:7972 connection error: write failed: 
error:1409F07F:SSL routines:SSL3_WRITE_PENDING:bad write retry

bluhm

 
 
 Index: tls.c
 ===
 RCS file: /cvs/src/lib/libtls/tls.c,v
 retrieving revision 1.7
 diff -u -p -r1.7 tls.c
 --- tls.c 7 Feb 2015 09:50:09 -   1.7
 +++ tls.c 12 Feb 2015 04:28:27 -
 @@ -183,6 +183,8 @@ err:
  int
  tls_configure_ssl(struct tls *ctx)
  {
 + SSL_CTX_set_mode(ctx-ssl_ctx, SSL_MODE_ENABLE_PARTIAL_WRITE);
 +
   SSL_CTX_set_options(ctx-ssl_ctx, SSL_OP_NO_SSLv2);
   SSL_CTX_set_options(ctx-ssl_ctx, SSL_OP_NO_SSLv3);

syslogd SSL3_WRITE_PENDING:bad write retry

2015-02-11 Thread Alexander Bluhm 
Hi,

During testing syslogd I got some strange error messages from libtls:
syslogd: loghost @tls://localhost:15878 connection error: write failed: 
error:1409F07F:SSL routines:SSL3_WRITE_PENDING:bad write retry

I found out that after an SSL_ERROR_WANT_WRITE, SSL_write must be
called with exactly the same parameter.  So I remember this event
and do not modify the event buffer.

ok?

bluhm

Index: usr.sbin/syslogd/evbuffer_tls.c
===
RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/evbuffer_tls.c,v
retrieving revision 1.2
diff -u -p -r1.2 evbuffer_tls.c
--- usr.sbin/syslogd/evbuffer_tls.c 30 Jan 2015 14:00:55 -  1.2
+++ usr.sbin/syslogd/evbuffer_tls.c 12 Feb 2015 00:39:23 -
@@ -168,6 +168,7 @@ buffertls_writecb(int fd, short event, v
buffertls_writecb, buftls);
goto reschedule;
case TLS_WRITE_AGAIN:
+   buftls-bt_flags |= BT_WRITE_AGAIN;
event_set(bufev-ev_write, fd, EV_WRITE,
buffertls_writecb, buftls);
goto reschedule;
@@ -186,6 +187,7 @@ buffertls_writecb(int fd, short event, v
if (res = 0)
goto error;
}
+   buftls-bt_flags = ~BT_WRITE_AGAIN;
 
event_set(bufev-ev_write, fd, EV_WRITE, buffertls_writecb, buftls);
if (EVBUFFER_LENGTH(bufev-output) != 0)
@@ -276,6 +278,7 @@ buffertls_set(struct buffertls *buftls, 
event_set(bufev-ev_write, fd, EV_WRITE, buffertls_writecb, buftls);
buftls-bt_bufev = bufev;
buftls-bt_ctx = ctx;
+   buftls-bt_flags = 0;
 }
 
 void
Index: usr.sbin/syslogd/evbuffer_tls.h
===
RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/evbuffer_tls.h,v
retrieving revision 1.1
diff -u -p -r1.1 evbuffer_tls.h
--- usr.sbin/syslogd/evbuffer_tls.h 18 Jan 2015 19:37:59 -  1.1
+++ usr.sbin/syslogd/evbuffer_tls.h 12 Feb 2015 00:03:58 -
@@ -28,6 +28,8 @@ struct buffertls {
struct bufferevent  *bt_bufev;
struct tls  *bt_ctx;
const char  *bt_hostname;
+   int  bt_flags;
+#define BT_WRITE_AGAIN 0x1
 };
 
 void   buffertls_set(struct buffertls *, struct bufferevent *, struct tls *,
Index: usr.sbin/syslogd/syslogd.c
===
RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/syslogd.c,v
retrieving revision 1.153
diff -u -p -r1.153 syslogd.c
--- usr.sbin/syslogd/syslogd.c  10 Feb 2015 18:30:20 -  1.153
+++ usr.sbin/syslogd/syslogd.c  12 Feb 2015 00:03:27 -
@@ -1266,8 +1266,22 @@ fprintlog(struct filed *f, int flags, ch
}
break;
 
-   case F_FORWTCP:
case F_FORWTLS:
+   if (f-f_un.f_forw.f_buftls.bt_flags  BT_WRITE_AGAIN) {
+   /*
+* After an OpenSSL SSL_ERROR_WANT_WRITE you must not
+* modify the buffer pointer or length until the next
+* successful write.  Otherwise there will be an
+* error SSL3_WRITE_PENDING:bad write retry.
+* XXX This should be handled in the buffertls layer.
+*/
+   dprintf( %s (dropped tls write again)\n,
+   f-f_un.f_forw.f_loghost);
+   f-f_un.f_forw.f_dropped++;
+   break;
+   }
+   /* FALLTHROUGH */
+   case F_FORWTCP:
dprintf( %s, f-f_un.f_forw.f_loghost);
if (EVBUFFER_LENGTH(f-f_un.f_forw.f_bufev-output) =
MAX_TCPBUF) {

Re: syslogd SSL3_WRITE_PENDING:bad write retry

2015-02-11 Thread Ted Unangst 
Alexander Bluhm wrote:
 Hi,
 
 During testing syslogd I got some strange error messages from libtls:
 syslogd: loghost @tls://localhost:15878 connection error: write failed: 
 error:1409F07F:SSL routines:SSL3_WRITE_PENDING:bad write retry
 
 I found out that after an SSL_ERROR_WANT_WRITE, SSL_write must be
 called with exactly the same parameter.  So I remember this event
 and do not modify the event buffer.

This is one of the hardest things to fix with the current tls_write
implementation. Yes, we inherited the underlying API's requirement to reuse
the same buffer until it's completely written out. I dream of making tls_write
work just like write, but until then you'll need to do this.

Re: syslogd SSL3_WRITE_PENDING:bad write retry

2015-02-11 Thread Ted Unangst 
Ted Unangst wrote:
 Alexander Bluhm wrote:
  Hi,
  
  During testing syslogd I got some strange error messages from libtls:
  syslogd: loghost @tls://localhost:15878 connection error: write failed: 
  error:1409F07F:SSL routines:SSL3_WRITE_PENDING:bad write retry
  
  I found out that after an SSL_ERROR_WANT_WRITE, SSL_write must be
  called with exactly the same parameter.  So I remember this event
  and do not modify the event buffer.
 
 This is one of the hardest things to fix with the current tls_write
 implementation. Yes, we inherited the underlying API's requirement to reuse
 the same buffer until it's completely written out. I dream of making tls_write
 work just like write, but until then you'll need to do this.
 

Actually, I think this may be much easier to fix. SSL_write can be configured
to support short writes.

Try this.


Index: tls.c
===
RCS file: /cvs/src/lib/libtls/tls.c,v
retrieving revision 1.7
diff -u -p -r1.7 tls.c
--- tls.c   7 Feb 2015 09:50:09 -   1.7
+++ tls.c   12 Feb 2015 04:28:27 -
@@ -183,6 +183,8 @@ err:
 int
 tls_configure_ssl(struct tls *ctx)
 {
+   SSL_CTX_set_mode(ctx-ssl_ctx, SSL_MODE_ENABLE_PARTIAL_WRITE);
+
SSL_CTX_set_options(ctx-ssl_ctx, SSL_OP_NO_SSLv2);
SSL_CTX_set_options(ctx-ssl_ctx, SSL_OP_NO_SSLv3);