Re: [PATCH v20] GSSAPI encryption support

Hi,

On 2018-12-18 14:12:46 -0500, Robbie Harwood wrote:
> From 6915ae2507bf7910c5eecfbd0b84805531c16a07 Mon Sep 17 00:00:00 2001
> From: Robbie Harwood <rharwood@redhat.com>
> Date: Thu, 10 May 2018 16:12:03 -0400
> Subject: [PATCH] libpq GSSAPI encryption support
> 
> On both the frontend and backend, prepare for GSSAPI encryption
> support by moving common code for error handling into a separate file.
> Fix a TODO for handling multiple status messages in the process.
> Eliminate the OIDs, which have not been needed for some time.
> Add frontend and backend encryption support functions.  Keep the
> context initiation for authentication-only separate on both the
> frontend and backend in order to avoid concerns about changing the
> requested flags to include encryption support.
> 
> In postmaster, pull GSSAPI authorization checking into a shared
> function.  Also share the initiator name between the encryption and
> non-encryption codepaths.
> 
> Modify pqsecure_write() to take a non-const pointer.  The pointer will
> not be modified, but this change (or a const-discarding cast, or a
> malloc()+memcpy()) is necessary for GSSAPI due to const/struct
> interactions in C.
> 
> For HBA, add "hostgss" and "hostnogss" entries that behave similarly
> to their SSL counterparts.  "hostgss" requires either "gss", "trust",
> or "reject" for its authentication.
> 
> Simiarly, add a "gssmode" parameter to libpq.  Supported values are
> "disable", "require", and "prefer".  Notably, negotiation will only be
> attempted if credentials can be acquired.  Move credential acquisition
> into its own function to support this behavior.
> 
> Finally, add documentation for everything new, and update existing
> documentation on connection security.
> 
> Thanks to Michael Paquier for the Windows fixes.

Could some of these be split into separate patches that could be more
eagerly merged? This is a somewhat large patch...


> diff --git a/src/interfaces/libpq/fe-secure.c b/src/interfaces/libpq/fe-secure.c
> index a06fc7dc82..f4f196e3b4 100644
> --- a/src/interfaces/libpq/fe-secure.c
> +++ b/src/interfaces/libpq/fe-secure.c
> @@ -220,6 +220,13 @@ pqsecure_read(PGconn *conn, void *ptr, size_t len)
>          n = pgtls_read(conn, ptr, len);
>      }
>      else
> +#endif
> +#ifdef ENABLE_GSS
> +    if (conn->gssenc)
> +    {
> +        n = pg_GSS_read(conn, ptr, len);
> +    }
> +    else
>  #endif
>      {
>          n = pqsecure_raw_read(conn, ptr, len);
> @@ -287,7 +294,7 @@ pqsecure_raw_read(PGconn *conn, void *ptr, size_t len)
>   * to determine whether to continue/retry after error.
>   */
>  ssize_t
> -pqsecure_write(PGconn *conn, const void *ptr, size_t len)
> +pqsecure_write(PGconn *conn, void *ptr, size_t len)
>  {
>      ssize_t        n;
>  
> @@ -297,6 +304,13 @@ pqsecure_write(PGconn *conn, const void *ptr, size_t len)
>          n = pgtls_write(conn, ptr, len);
>      }
>      else
> +#endif
> +#ifdef ENABLE_GSS
> +    if (conn->gssenc)
> +    {
> +        n = pg_GSS_write(conn, ptr, len);
> +    }
> +    else
>  #endif
>      {
>          n = pqsecure_raw_write(conn, ptr, len);

Not a fan of this. Seems like we'll grow more and more such branches
over time? Wouldn't the right thing be to have callbacks in PGconn (and
similarly in the backend)?  Seems like if that's done reasonably it'd
also make integration of compression easier, because that could just
layer itself between encryption and wire?


Greetings,

Andres Freund