Re: SSL (patch 1) - Mailing list pgsql-patches

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@gmx.net