Re: SSL (patch 1)

Bear Giles writes:

> First of many patches on SSL code. The first patch just sets
> the groundwork for future patches by pulling all SSL-specific
> (and by implication all secure session) code into two new files,
> be-secure.c and fe-secure.c

Looks like the right direction. Specific comments below.

> Index: postgresql/src/backend/libpq/pqcomm.c

> ***************
> *** 80,85 ****
> --- 80,88 ----
> #include "libpq/libpq.h"
> #include "miscadmin.h"
>
> + extern void secure_close(Port *);
> + extern ssize_t secure_read(Port *, void *, size_t);
> + extern ssize_t secure_write(Port *, const void *, size_t);

Should be in a header file. ssize_t is probably not portable.

> ***************
> *** 137,142 ****
> --- 140,146 ----
> {
> if (MyProcPort != NULL)
> {
> + secure_close(MyProcPort);
> close(MyProcPort->sock);
> /* make sure any subsequent attempts to do I/O fail cleanly */
> MyProcPort->sock = -1;

Shouldn't secure_close() do all the other closing actions as well? That's
how secure_read() etc. appear to work.

> Index: postgresql/src/backend/postmaster/be-secure.c

This file seems to be belong under backend/libpq.

> diff -c /dev/null postgresql/src/backend/postmaster/be-secure.c:1.1
> *** /dev/null Fri May 24 12:50:07 2002
> --- postgresql/src/backend/postmaster/be-secure.c Fri May 24 12:41:53 2002
> ***************
> *** 0 ****
> --- 1,327 ----
> + /*-------------------------------------------------------------------------
> + *
> + * be-connect.c
> + * functions related to setting up a secure connection to the frontend.
> + * Secure connections are expected to provide confidentiality,
> + * message integrity and endpoint authentication.
> + *
> + *
> + * Portions Copyright (c) 1996-2001, PostgreSQL Global Development Group
> + * Portions Copyright (c) 1994, Regents of the University of California
> + *
> + *
> + * IDENTIFICATION
> + * $Header: /usr/local/cvsroot/postgresql/src/backend/postmaster/be-secure.c,v 1.1 2002/05/24 18:41:53 bear Exp $
> + *
> + * PATCH LEVEL
> + * milestone 1: fix basic coding errors

I don't think we need to record that in a code file.

> + #ifdef WIN32
> + #include "win32.h"
> + #else
> + #include <sys/socket.h>
> + #include <unistd.h>
> + #include <netdb.h>
> + #include <netinet/in.h>
> + #ifdef HAVE_NETINET_TCP_H
> + #include <netinet/tcp.h>
> + #endif
> + #include <arpa/inet.h>
> + #endif

There is no WIN32 in the backend.

> + extern void ExitPostmaster(int);
> + extern void postmaster_error(const char *fmt,...);
> +
> + int secure_initialize(void);
> + void secure_destroy(void);
> + int secure_open_server(Port *);
> + void secure_close(Port *);
> + ssize_t secure_read(Port *, void *ptr, size_t len);
> + ssize_t secure_write(Port *, const void *ptr, size_t len);

Header files...

> + #define PING() elog(DEBUG,"%s, line %d, %s", __FILE__, __LINE__, __func__);

__func__ is definitely not portable. Nor should there be unconditional
elog(DEBUG)'s in the code. Actually, it should be elog(DEBUG[1-5]) now I
think.

> + /*
> + * Obtain reason string for last SSL error
> + *
> + * Some caution is needed here since ERR_reason_error_string will
> + * return NULL if it doesn't recognize the error code. We don't
> + * want to return NULL ever.
> + */
> + static const char *
> + SSLerrmessage(void)
> + {
> + unsigned long errcode;
> + const char *errreason;
> + static char errbuf[32];
> +
> + errcode = ERR_get_error();
> + if (errcode == 0)
> + return "No SSL error reported";
> + errreason = ERR_reason_error_string(errcode);
> + if (errreason != NULL)
> + return errreason;
> + snprintf(errbuf, sizeof(errbuf), "SSL error code %lu", errcode);
> + return errbuf;
> + }

Should add some gettext() calls here?

> Index: postgresql/src/backend/postmaster/postmaster.c

> #ifdef USE_SSL
> ! extern int secure_initialize(void);
> ! extern void secure_destroy(void);
> ! extern int secure_open_server(Port *);
> ! extern void secure_close(Port *);
> ! #endif /* USE_SSL */

Header files...

> Index: postgresql/src/interfaces/libpq/fe-connect.c

> ***************
> *** 61,69 ****
> }
> #endif
>
> -
> #ifdef USE_SSL
> ! static SSL_CTX *SSL_context = NULL;
> #endif
>
> #define NOTIFYLIST_INITIAL_SIZE 10
> --- 61,72 ----
> }
> #endif
>
> #ifdef USE_SSL
> ! extern int secure_initialize(PGconn *);
> ! extern void secure_destroy(void);
> ! extern int secure_open_client(PGconn *);
> ! extern void secure_close(PGconn *);
> ! extern SSL * PQgetssl(PGconn *);
> #endif

These guys either need to be static or need to have less likely to
conflict names. Keep in mind that this is a library. Preprending pg_
should be OK. And the gratuitous header file comment belongs here as
well.

> Index: postgresql/src/interfaces/libpq/fe-secure.c

> + static const char *
> + SSLerrmessage(void)
> + {
> + unsigned long errcode;
> + const char *errreason;
> + static char errbuf[32];
> +
> + errcode = ERR_get_error();
> + if (errcode == 0)
> + return "No SSL error reported";
> + errreason = ERR_reason_error_string(errcode);
> + if (errreason != NULL)
> + return errreason;
> + snprintf(errbuf, sizeof(errbuf), "SSL error code %lu", errcode);
> + return errbuf;
> + }

Could also use some libpq_gettext() calls around the strings. Also, you
shouldn't use fixed buffers for strings. Or at least make it a lot bigger
in case someone translates it into Welsh. ;-) Look at how the other error
messages are typically copied around.

--
Peter Eisentraut peter_e(at)gmx(dot)net