Skip site navigation (1) Skip section navigation (2)

Re: SSL (patch 1)

From: Peter Eisentraut <peter_e(at)gmx(dot)net>
To: Bear Giles <bgiles(at)coyotesong(dot)com>
Cc: pgsql-patches(at)postgresql(dot)org
Subject: Re: SSL (patch 1)
Date: 2002-05-27 20:25:00
Message-ID: Pine.LNX.4.44.0205272144230.2460-100000@localhost.localdomain (view raw or flat)
Thread:
Lists: 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(at)gmx(dot)net



In response to

Responses

pgsql-patches by date

Next:From: Peter EisentrautDate: 2002-05-27 20:25:44
Subject: Re: SSL (patch 2)
Previous:From: Peter EisentrautDate: 2002-05-27 20:24:34
Subject: Re: revised sample SRF C function; proposed SRF API

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group