ftp: use stdio for TLS connections

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

ftp: use stdio for TLS connections

Jeremie Courreges-Anglas-2

This unifies the TLS/non-TLS code paths, kills a bunch of local code,
saves some precious text size and makes the code easier to follow IMO.

The magic trick is funopen, which is not portable but I doubt that's
a problem.  I added a tls_handshake() call so that TLS errors are
spotted earlier (else they would be caught by the fflush() == EOF
check).

Size decrease:

russell /usr/src/usr.bin/ftp$ size 1/fetch.o 2/fetch.o
text    data    bss     dec     hex
17529   128     5       17662   44fe    1/fetch.o
16425   120     5       16550   40a6    2/fetch.o

russell /usr/src/distrib/special/ftp$ size 1/fetch.o 2/fetch.o
text    data    bss     dec     hex
10399   0       5       10404   28a4    1/fetch.o
10043   0       5       10048   2740    2/fetch.o

This went through an mkr on amd64.

ok?


Index: fetch.c
===================================================================
RCS file: /cvs/src/usr.bin/ftp/fetch.c,v
retrieving revision 1.175
diff -u -p -r1.175 fetch.c
--- fetch.c 23 Oct 2019 16:47:53 -0000 1.175
+++ fetch.c 2 Nov 2019 10:55:09 -0000
@@ -74,14 +74,13 @@ void abortfile(int);
 char hextochar(const char *);
 char *urldecode(const char *);
 char *recode_credentials(const char *_userinfo);
-int ftp_printf(FILE *, struct tls *, const char *, ...) __attribute__((format(printf, 3, 4)));
-char *ftp_readline(FILE *, struct tls *, size_t *);
-size_t ftp_read(FILE *, struct tls *, char *, size_t);
+char *ftp_readline(FILE *, size_t *);
 void ftp_close(FILE **fin, struct tls **tls, volatile int *fd);
+const char *sockerror(struct tls *);
 #ifndef NOSSL
 int proxy_connect(int, char *, char *);
-int SSL_vprintf(struct tls *, const char *, va_list);
-char *SSL_readline(struct tls *, size_t *);
+int stdio_tls_write_wrapper(void *, const char *, int);
+int stdio_tls_read_wrapper(void *, char *, int);
 #endif /* !NOSSL */
 
 #define FTP_URL "ftp://" /* ftp URL prefix */
@@ -638,6 +637,12 @@ noslash:
  fprintf(ttyout, "SSL failure: %s\n", tls_error(tls));
  goto cleanup_url_get;
  }
+ if (tls_handshake(tls) != 0) {
+ fprintf(ttyout, "SSL failure: %s\n", tls_error(tls));
+ goto cleanup_url_get;
+ }
+ fin = funopen(tls, stdio_tls_read_wrapper,
+    stdio_tls_write_wrapper, NULL, NULL);
  } else {
  fin = fdopen(fd, "r+");
  fd = -1;
@@ -682,13 +687,13 @@ noslash:
  * the original URI (path).
  */
  if (credentials)
- ftp_printf(fin, tls, "GET %s HTTP/1.0\r\n"
+ fprintf(fin, "GET %s HTTP/1.0\r\n"
     "Proxy-Authorization: Basic %s\r\n"
     "Host: %s\r\n%s%s\r\n\r\n",
     epath, credentials,
     proxyhost, buf ? buf : "", httpuseragent);
  else
- ftp_printf(fin, tls, "GET %s HTTP/1.0\r\n"
+ fprintf(fin, "GET %s HTTP/1.0\r\n"
     "Host: %s\r\n%s%s\r\n\r\n",
     epath, proxyhost, buf ? buf : "", httpuseragent);
  } else {
@@ -706,7 +711,7 @@ noslash:
 #endif /* SMALL */
 #ifndef NOSSL
  if (credentials) {
- ftp_printf(fin, tls,
+ fprintf(fin,
     "GET /%s %s\r\nAuthorization: Basic %s\r\nHost: ",
     epath, restart_point ?
     "HTTP/1.1\r\nConnection: close" : "HTTP/1.0",
@@ -715,13 +720,13 @@ noslash:
  credentials = NULL;
  } else
 #endif /* NOSSL */
- ftp_printf(fin, tls, "GET /%s %s\r\nHost: ", epath,
+ fprintf(fin, "GET /%s %s\r\nHost: ", epath,
 #ifndef SMALL
     restart_point ? "HTTP/1.1\r\nConnection: close" :
 #endif /* !SMALL */
     "HTTP/1.0");
  if (proxyhost) {
- ftp_printf(fin, tls, "%s", proxyhost);
+ fprintf(fin, "%s", proxyhost);
  port = NULL;
  } else if (strchr(host, ':')) {
  /*
@@ -733,10 +738,10 @@ noslash:
  errx(1, "Can't allocate memory.");
  if ((p = strchr(h, '%')) != NULL)
  *p = '\0';
- ftp_printf(fin, tls, "[%s]", h);
+ fprintf(fin, "[%s]", h);
  free(h);
  } else
- ftp_printf(fin, tls, "%s", host);
+ fprintf(fin, "%s", host);
 
  /*
  * Send port number only if it's specified and does not equal
@@ -745,15 +750,15 @@ noslash:
  */
 #ifndef NOSSL
  if (port && strcmp(port, (ishttpsurl ? "443" : "80")) != 0)
- ftp_printf(fin, tls, ":%s", port);
+ fprintf(fin, ":%s", port);
  if (restart_point)
- ftp_printf(fin, tls, "\r\nRange: bytes=%lld-",
+ fprintf(fin, "\r\nRange: bytes=%lld-",
  (long long)restart_point);
 #else /* !NOSSL */
  if (port && strcmp(port, "80") != 0)
- ftp_printf(fin, tls, ":%s", port);
+ fprintf(fin, ":%s", port);
 #endif /* !NOSSL */
- ftp_printf(fin, tls, "\r\n%s%s\r\n\r\n",
+ fprintf(fin, "\r\n%s%s\r\n\r\n",
     buf ? buf : "", httpuseragent);
  }
  free(epath);
@@ -763,12 +768,12 @@ noslash:
 #endif /* !NOSSL */
  buf = NULL;
 
- if (fin != NULL && fflush(fin) == EOF) {
- warn("Writing HTTP request");
+ if (fflush(fin) == EOF) {
+ warnx("Writing HTTP request: %s", sockerror(tls));
  goto cleanup_url_get;
  }
- if ((buf = ftp_readline(fin, tls, &len)) == NULL) {
- warn("Receiving HTTP reply");
+ if ((buf = ftp_readline(fin, &len)) == NULL) {
+ warnx("Receiving HTTP reply: %s", sockerror(tls));
  goto cleanup_url_get;
  }
 
@@ -842,8 +847,8 @@ noslash:
  filesize = -1;
 
  for (;;) {
- if ((buf = ftp_readline(fin, tls, &len)) == NULL) {
- warn("Receiving HTTP reply");
+ if ((buf = ftp_readline(fin, &len)) == NULL) {
+ warnx("Receiving HTTP reply: %s", sockerror(tls));
  goto cleanup_url_get;
  }
 
@@ -1004,7 +1009,7 @@ noslash:
  len = 1;
  oldinti = signal(SIGINFO, psummary);
  while (len > 0) {
- len = ftp_read(fin, tls, buf, buflen);
+ len = fread(buf, 1, buflen, fin);
  bytes += len;
  for (cp = buf, wlen = len; wlen > 0; wlen -= i, cp += i) {
  if ((i = write(out, cp, wlen)) == -1) {
@@ -1031,7 +1036,7 @@ noslash:
  (void)fflush(ttyout);
  }
  if (len != 0) {
- warn("Reading from socket");
+ warnx("Reading from socket: %s", sockerror(tls));
  goto cleanup_url_get;
  }
  progressmeter(1, NULL);
@@ -1514,67 +1519,9 @@ isurl(const char *p)
 }
 
 char *
-ftp_readline(FILE *fp, struct tls *tls, size_t *lenp)
-{
- if (fp != NULL)
- return fparseln(fp, lenp, NULL, "\0\0\0", 0);
-#ifndef NOSSL
- else if (tls != NULL)
- return SSL_readline(tls, lenp);
-#endif /* !NOSSL */
- else
- return NULL;
-}
-
-size_t
-ftp_read(FILE *fp, struct tls *tls, char *buf, size_t len)
-{
-#ifndef NOSSL
- ssize_t tret;
-#endif
- size_t ret = 0;
-
- if (fp != NULL)
- ret = fread(buf, sizeof(char), len, fp);
-#ifndef NOSSL
- else if (tls != NULL) {
- do {
- tret = tls_read(tls, buf, len);
- } while (tret == TLS_WANT_POLLIN || tret == TLS_WANT_POLLOUT);
- if (tret == -1)
- errx(1, "SSL read error: %s", tls_error(tls));
- ret = (size_t)tret;
- }
-#endif /* !NOSSL */
- return (ret);
-}
-
-int
-ftp_printf(FILE *fp, struct tls *tls, const char *fmt, ...)
+ftp_readline(FILE *fp, size_t *lenp)
 {
- int ret;
- va_list ap;
-
- va_start(ap, fmt);
-
- if (fp != NULL)
- ret = vfprintf(fp, fmt, ap);
-#ifndef NOSSL
- else if (tls != NULL)
- ret = SSL_vprintf(tls, fmt, ap);
-#endif /* !NOSSL */
- else
- ret = 0;
-
- va_end(ap);
-#ifndef SMALL
- if (debug) {
- va_start(ap, fmt);
- ret = vfprintf(ttyout, fmt, ap);
- va_end(ap);
- }
-#endif /* !SMALL */
- return (ret);
+ return fparseln(fp, lenp, NULL, "\0\0\0", 0);
 }
 
 void
@@ -1593,75 +1540,32 @@ ftp_close(FILE **fin, struct tls **tls,
  tls_free(*tls);
  *tls = NULL;
  }
-#endif
- if (*fin != NULL) {
- fclose(*fin);
- *fin = NULL;
- }
  if (*fd != -1) {
  close(*fd);
  *fd = -1;
  }
-}
-
-#ifndef NOSSL
-int
-SSL_vprintf(struct tls *tls, const char *fmt, va_list ap)
-{
- char *string, *buf;
- size_t len;
- int ret;
-
- if ((ret = vasprintf(&string, fmt, ap)) == -1)
- return ret;
- buf = string;
- len = ret;
- while (len > 0) {
- ret = tls_write(tls, buf, len);
- if (ret == TLS_WANT_POLLIN || ret == TLS_WANT_POLLOUT)
- continue;
- if (ret == -1)
- errx(1, "SSL write error: %s", tls_error(tls));
- buf += ret;
- len -= ret;
+#endif
+ if (*fin != NULL) {
+ fclose(*fin);
+ *fin = NULL;
  }
- free(string);
- return ret;
 }
 
-char *
-SSL_readline(struct tls *tls, size_t *lenp)
+const char *
+sockerror(struct tls *tls)
 {
- size_t i, len;
- char *buf, *q, c;
- int ret;
-
- len = 128;
- if ((buf = malloc(len)) == NULL)
- errx(1, "Can't allocate memory for transfer buffer");
- for (i = 0; ; i++) {
- if (i >= len - 1) {
- if ((q = reallocarray(buf, len, 2)) == NULL)
- errx(1, "Can't expand transfer buffer");
- buf = q;
- len *= 2;
- }
- do {
- ret = tls_read(tls, &c, 1);
- } while (ret == TLS_WANT_POLLIN || ret == TLS_WANT_POLLOUT);
- if (ret == -1)
- errx(1, "SSL read error: %s", tls_error(tls));
-
- buf[i] = c;
- if (c == '\n') {
- buf[i] = '\0';
- break;
- }
+ int save_errno = errno;
+#ifndef NOSSL
+ if (tls != NULL) {
+ const char *tlserr = tls_error(tls);
+ if (tlserr != NULL)
+ return tlserr;
  }
- *lenp = i;
- return (buf);
+#endif
+ return strerror(save_errno);
 }
 
+#ifndef NOSSL
 int
 proxy_connect(int socket, char *host, char *cookie)
 {
@@ -1702,5 +1606,31 @@ proxy_connect(int socket, char *host, ch
  read(socket, &buf, sizeof(buf)); /* only proxy header XXX: error handling? */
  free(connstr);
  return(200);
+}
+
+int
+stdio_tls_write_wrapper(void *arg, const char *buf, int len)
+{
+ struct tls *tls = arg;
+ ssize_t ret;
+
+ do {
+ ret = tls_write(tls, buf, len);
+ } while (ret == TLS_WANT_POLLIN || ret == TLS_WANT_POLLOUT);
+
+ return ret;
+}
+
+int
+stdio_tls_read_wrapper(void *arg, char *buf, int len)
+{
+ struct tls *tls = arg;
+ ssize_t ret;
+
+ do {
+ ret = tls_read(tls, buf, len);
+ } while (ret == TLS_WANT_POLLIN || ret == TLS_WANT_POLLOUT);
+
+ return ret;
 }
 #endif /* !NOSSL */


--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Reply | Threaded
Open this post in threaded view
|

Re: ftp: use stdio for TLS connections

Klemens Nanni-2
On Sat, Nov 02, 2019 at 12:03:17PM +0100, Jeremie Courreges-Anglas wrote:
> The magic trick is funopen, which is not portable but I doubt that's
> a problem.
A lot of Linux distributions use or at least package our netcat and I'm
really glad they do;  no idea what how those would work around
funopen(3) and how much we care about this, but it's worth mentioning.

Reply | Threaded
Open this post in threaded view
|

Re: ftp: use stdio for TLS connections

Jeremie Courreges-Anglas-2
On Sat, Nov 02 2019, Klemens Nanni <[hidden email]> wrote:
> On Sat, Nov 02, 2019 at 12:03:17PM +0100, Jeremie Courreges-Anglas wrote:
>> The magic trick is funopen, which is not portable but I doubt that's
>> a problem.
> A lot of Linux distributions use or at least package our netcat and I'm
> really glad they do;  no idea what how those would work around
> funopen(3) and how much we care about this, but it's worth mentioning.

ftp(1) is not nc(1), I'd be surprised if our current ftp(1) was ported
to another OS. :)

Anyway, the glibc provides fopencookie for use cases similar to funopen.

--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Reply | Threaded
Open this post in threaded view
|

Re: ftp: use stdio for TLS connections

Klemens Nanni-2
On Sat, Nov 02, 2019 at 12:32:09PM +0100, Jeremie Courreges-Anglas wrote:
> ftp(1) is not nc(1), I'd be surprised if our current ftp(1) was ported
> to another OS. :)
Meh, stupid me.

Diff looks good so far, I can test it with miniroot and/or bsd.rd on
amd64 and/or macppc if you want.

Reply | Threaded
Open this post in threaded view
|

Re: ftp: use stdio for TLS connections

Jeremie Courreges-Anglas-2
On Sat, Nov 02 2019, Klemens Nanni <[hidden email]> wrote:
> On Sat, Nov 02, 2019 at 12:32:09PM +0100, Jeremie Courreges-Anglas wrote:
>> ftp(1) is not nc(1), I'd be surprised if our current ftp(1) was ported
>> to another OS. :)
> Meh, stupid me.
>
> Diff looks good so far, I can test it with miniroot and/or bsd.rd on
> amd64 and/or macppc if you want.

Thanks for the proposal.  Testing on powerpc could be nice since iirc
that platform doesn't use ftp-ssl.  But the changes shouldn't negatively
affect non-TLS connections.  I have tested /usr/src/distrib/special/ftp
and /usr/src/distrib/special/ftp-ssl, and I could not spot regressions.
/usr/src/usr.bin/ftp fared well in a dpb -F run too.

Throughput for TLS connections doesn't seem positively nor negatively
affected either, the numbers don't change much even for connections to
localhost.  This change is really about making the code simpler and
cleaner, which can't hurt when looking at fetch.c.

PS: my initial diff also implemented a "closefn" function which would
properly close the tls connection and the underlying fd so we could just
call fclose(fin).  But in the end ftp_close() lead to shorter code, and
I wanted to keep the TLS session ticket logging, which looked weird in
a supposedly pure close(2) wrapper.

--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE