Re: Allow matching whole DN from a client certificate - Mailing list pgsql-hackers

On Fri, Mar 05, 2021 at 04:01:38PM -0500, Andrew Dunstan wrote:
> Yeah, fixed this, added a bit more docco, and got rid of some bitrot.

I had a look at this patch.  What you have here looks in good shape,
and I have come comments.

> +       This option is probably best used in conjunction with a username map.
> +       The comparison is done with the <literal>DN</literal> in RFC2253 format.

You should add a link to the RFC, using https://tools.ietf.org/html/
as a base like the other parts of the docs.

>      /* Make sure we have received a username in the certificate */
> -    if (port->peer_cn == NULL ||
> -        strlen(port->peer_cn) <= 0)
> +    peer_username = port->hba->clientcertname == clientCertCN ? port->peer_cn : port->peer_dn;

Should have some parenthesis for clarity.  But, couldn't you just
use a switch based on ClientCertName to select the data you wish to
use for the match?  If a new option is added then it is impossible to
miss that peer_username needs a value as a compiler would warn on
that.

> -        (errmsg("certificate validation (clientcert=verify-full) failed for user \"%s\": CN mismatch",
> +        (errmsg("certificate validation (clientcert=verify-full) failed for user \"%s\": CN/DN mismatch",

It would be cleaner to show in this LOG that it is either a CN or a DN
mismatch, but not both of them.  And you can make the difference with
hba->clientcertname for that.

> +        len = X509_NAME_get_text_by_NID(x509name, NID_commonName, NULL, 0);
>          if (len != -1)
>          {
> -            char       *peer_cn;

I think that you should keep this local declaration here.

> +        /* use commas instead of slashes */
> +        X509_NAME_print_ex(bio, x509name, 0, XN_FLAG_RFC2253);

The reason is not obvious for the reader here (aka that commas are
required as slashes are common when printing the DN, quoting
upthread).  Hence, wouldn't it be better to add a comment explaining
that here?

> +        BIO_get_mem_ptr(bio, &bio_buf);

BIO_get_mem_ptr() (BIO_ctrl() in the OpenSSL code) returns a status
code.  I think that we had better check after it.

> +        peer_dn = MemoryContextAlloc(TopMemoryContext, bio_buf->length + 1);
> +        memcpy(peer_dn, bio_buf->data, bio_buf->length);
> +        peer_dn[bio_buf->length] = '\0';
> +        if (bio_buf->length != strlen(peer_dn))
> +        {
> +            ereport(COMMERROR,
> +                    (errcode(ERRCODE_PROTOCOL_VIOLATION),
> +                     errmsg("SSL certificate's distinguished name contains embedded null")));
> +            BIO_free(bio);
> +            pfree(peer_dn);
> +            pfree(port->peer_cn);
> +            port->peer_cn = NULL;
> +            return -1;
> +        }
> +
> +        BIO_free(bio);

You could just do one BIO_free() once the memcpy() is done, no?

> @@ -121,7 +121,7 @@ secure_open_server(Port *port)
>
>      ereport(DEBUG2,
>          (errmsg_internal("SSL connection from \"%s\"",
> -            port->peer_cn ? port->peer_cn : "(anonymous)")));
> +            port->peer_dn ? port->peer_dn : "(anonymous)")));

Could it be better for debugging to show both the CN and DN if both
are available?

> -} ClientCertMode;
> +}            ClientCertMode;
> +
> +typedef enum ClientCertName
> +{
> +    clientCertCN,
> +    clientCertDN
> +}            ClientCertName;

Missing some indentation stuff here.

> +# correct client cert using whole DN
> +my $dn_connstr = $common_connstr;
> +$dn_connstr =~ s/certdb/certdb_dn/;

I would use a separate variable rather than enforcing an update of the
existing $common_connstr created a couple of lines above.

> +# same thing but with a regex
> +$dn_connstr =~ s/certdb_dn/certdb_dn_re/;

Same here.  This depends on the last variable assignment, which itself
depends on the assignment of $common_connstr.
--
Michael