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-dev&m=118766345824094&w=2 I found another thread from 2006. http://marc.info/?t=115101262200001&r=1&w=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 -0000 1.2 +++ usr.sbin/syslogd/evbuffer_tls.c 19 Feb 2015 16:47:44 -0000 @@ -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 -0000 1.1 +++ usr.sbin/syslogd/evbuffer_tls.h 19 Feb 2015 16:47:44 -0000 @@ -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 -0000 1.156 +++ usr.sbin/syslogd/syslogd.c 19 Feb 2015 16:47:44 -0000 @@ -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 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) {
