Allow matching whole DN from a client certificate

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
32 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Re: Allow matching whole DN from a client certificate

Jacob Champion-2
On Sat, 2021-01-30 at 16:18 -0500, Andrew Dunstan wrote:
>     cd src/test/ssl
>     touch ../../Makefile.global
>     rm -f ssl/client-dn.crt  ssl/client-dn.key
>     touch ssl/root_ca-certindex
>     echo 01> ssl/root_ca.srl

Note that, on my machine at least, the root_ca serial counter is at 03
after running `make sslfiles`. 1 and 2 are already assigned to
server_ca and client_ca, respectively.

Speaking of which, what's the reason you need to recreate the root_ca
machinery when it's the client_ca that issues the new certificate?

>     make ssl/client-dn.crt
>     rm -rf ssl/*certindex* ssl/root_ca.srl ssl/new_certs_dir
>     rm ../../Makefile.global
>
> Making incremental additions to the certificate set easier wouldn't be a
> bad thing.
>
> I wonder if we should really be setting 1 as the serial number, though.
> Might it not be better to use, say, `date +%Y%m%d01` rather like we do
> with catalog version numbers?

You could also check in the CA state files.

--Jacob
Reply | Threaded
Open this post in threaded view
|

Re: Allow matching whole DN from a client certificate

Jacob Champion-2
In reply to this post by Andrew Dunstan
On Sat, 2021-01-30 at 16:18 -0500, Andrew Dunstan wrote:
> Making incremental additions to the certificate set easier wouldn't be a
> bad thing.
>
> I wonder if we should really be setting 1 as the serial number, though.
> Might it not be better to use, say, `date +%Y%m%d01` rather like we do
> with catalog version numbers?

I have been experimenting a bit with both of these suggestions; hope to
have something in time for commitfest on Monday. Writing new tests for
NSS has run into the same problems you've mentioned.

FYI, I've pulled the port->peer_dn functionality you've presented here
into my authenticated identity patchset at [1].

--Jacob

[1] https://www.postgresql.org/message-id/flat/c55788dd1773c521c862e8e0dddb367df51222be.camel%40vmware.com
Reply | Threaded
Open this post in threaded view
|

Re: Allow matching whole DN from a client certificate

Andrew Dunstan

On 2/26/21 2:55 PM, Jacob Champion wrote:

> On Sat, 2021-01-30 at 16:18 -0500, Andrew Dunstan wrote:
>> Making incremental additions to the certificate set easier wouldn't be a
>> bad thing.
>>
>> I wonder if we should really be setting 1 as the serial number, though.
>> Might it not be better to use, say, `date +%Y%m%d01` rather like we do
>> with catalog version numbers?
> I have been experimenting a bit with both of these suggestions; hope to
> have something in time for commitfest on Monday. Writing new tests for
> NSS has run into the same problems you've mentioned.
>
> FYI, I've pulled the port->peer_dn functionality you've presented here
> into my authenticated identity patchset at [1].
>
> --Jacob
>
> [1] https://www.postgresql.org/message-id/flat/c55788dd1773c521c862e8e0dddb367df51222be.camel%40vmware.com


Cool.


I think the thing that's principally outstanding w.r.t. this patch is
what format we should use to extract the DN. Should we use RFC2253,
which reverses the field order, as has been suggested upthread and is in
the latest patch? I'm slightly worried that it might be a POLA
violation. But I don't have terribly strong feelings about it.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com



Reply | Threaded
Open this post in threaded view
|

Re: Allow matching whole DN from a client certificate

Justin Pryzby
In reply to this post by Andrew Dunstan
On Sat, Jan 30, 2021 at 04:18:12PM -0500, Andrew Dunstan wrote:
> @@ -610,6 +610,19 @@ hostnogssenc  <replaceable>database</replaceable>  <replaceable>user</replaceabl
>         the verification of client certificates with any authentication
>         method that supports <literal>hostssl</literal> entries.
>        </para>
> +      <para>
> +       On any record using client certificate authentication, that is one
> +       using the <literal>cert</literal> authentication method or one
> +       using the <literal>clientcert</literal> option, you can specify

I suggest instead of "that is" to instead parenthesize this part:
| (one using the <literal>cert</literal> authentication method or the
| <literal>clientcert</literal> option), you can specify

> +       which part of the client certificate credentials to match using
> +       the <literal>clientname</literal> option. This option can have one
> +       of two values. If you specify <literal>clientname=CN</literal>, which
> +       is the default, the username is matched against the certificate's
> +       <literal>Common Name (CN)</literal>. If instead you specify
> +       <literal>clientname=DN</literal> the username is matched against the
> +       entire <literal>Distinguished Name (DN)</literal> of the certificate.
> +       This option is probably best used in comjunction with a username map.

spell: conjunction


Reply | Threaded
Open this post in threaded view
|

Re: Allow matching whole DN from a client certificate

Jacob Champion-2
In reply to this post by Andrew Dunstan
On Fri, 2021-02-26 at 15:40 -0500, Andrew Dunstan wrote:
> I think the thing that's principally outstanding w.r.t. this patch is
> what format we should use to extract the DN.

That and the warning label for sharp edges.

> Should we use RFC2253,
> which reverses the field order, as has been suggested upthread and is in
> the latest patch? I'm slightly worried that it might be a POLA
> violation.

All I can provide is the hindsight from httpd. [1] is the thread that
gave rise to its LegacyDNStringFormat.

Since RFC 2253 isn't a canonical encoding scheme, and we've already
established that different TLS implementations do things slightly
differently even when providing RFC-compliant output, maybe it doesn't
matter in the end: to get true compatibility, we need to implement a DN
matching scheme rather than checking string equality. But using RFC2253
for version 1 of the feature at least means that the *simplest* cases
are the same across backends, since I doubt the NSS implementation is
going to try to recreate OpenSSL's custom format.

--Jacob

[1] https://lists.apache.org/thread.html/2055b56985c69e7a6977151bf9817a0f982a4ad3b78a6a1984977fd0%401289507617%40%3Cusers.httpd.apache.org%3E
Reply | Threaded
Open this post in threaded view
|

Re: Allow matching whole DN from a client certificate

Ibrar Ahmed-4

On Wed, Mar 3, 2021 at 3:03 AM Jacob Champion <[hidden email]> wrote:
On Fri, 2021-02-26 at 15:40 -0500, Andrew Dunstan wrote:
> I think the thing that's principally outstanding w.r.t. this patch is
> what format we should use to extract the DN.

That and the warning label for sharp edges.

> Should we use RFC2253,
> which reverses the field order, as has been suggested upthread and is in
> the latest patch? I'm slightly worried that it might be a POLA
> violation.

All I can provide is the hindsight from httpd. [1] is the thread that
gave rise to its LegacyDNStringFormat.

Since RFC 2253 isn't a canonical encoding scheme, and we've already
established that different TLS implementations do things slightly
differently even when providing RFC-compliant output, maybe it doesn't
matter in the end: to get true compatibility, we need to implement a DN
matching scheme rather than checking string equality. But using RFC2253
for version 1 of the feature at least means that the *simplest* cases
are the same across backends, since I doubt the NSS implementation is
going to try to recreate OpenSSL's custom format.

--Jacob

[1] https://lists.apache.org/thread.html/2055b56985c69e7a6977151bf9817a0f982a4ad3b78a6a1984977fd0%401289507617%40%3Cusers.httpd.apache.org%3E


This patch set no longer applies
http://cfbot.cputube.org/patch_32_2835.log

Can we get a rebase? 

I marked the patch "Waiting on Author".

 

--
Ibrar Ahmed
Reply | Threaded
Open this post in threaded view
|

Re: Allow matching whole DN from a client certificate

Joel Jacobson-3
In reply to this post by Andrew Dunstan
On Sat, Jan 30, 2021, at 22:18, Andrew Dunstan wrote:
ssl-match-client-cert-dn-v3.patch

Spelling error of "conjunction":
+       This option is probably best used in comjunction with a username map.

/Joel
Reply | Threaded
Open this post in threaded view
|

Re: Allow matching whole DN from a client certificate

Andrew Dunstan

On 3/4/21 2:16 PM, Joel Jacobson wrote:
> On Sat, Jan 30, 2021, at 22:18, Andrew Dunstan wrote:
>> ssl-match-client-cert-dn-v3.patch
>
> Spelling error of "conjunction":
> +       This option is probably best used in comjunction with a
> username map.
>
>



Yeah, fixed this, added a bit more docco, and got rid of some bitrot.


cheers


andrew



--
Andrew Dunstan
EDB: https://www.enterprisedb.com


ssl-match-client-cert-dn-v4.patch (17K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Allow matching whole DN from a client certificate

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Allow matching whole DN from a client certificate

Andrew Dunstan

On 3/24/21 12:54 AM, Michael Paquier wrote:

[numerous useful comments]


OK, here's a new patch. I hope to commit this within a few days.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com


ssl-match-client-cert-dn-v5.patch (14K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Allow matching whole DN from a client certificate

Michael Paquier-2
On Fri, Mar 26, 2021 at 09:34:03AM -0400, Andrew Dunstan wrote:
> OK, here's a new patch. I hope to commit this within a few days.

Thanks!

+   switch (port->hba->clientcertname)
+   {
+       case clientCertDN:
+           peer_username = port->peer_dn;
+           break;
+       default:
+           peer_username = port->peer_cn;
+   }

This does not need a "default".  I think that you should use "case
clientCertCN" instead here.

+              BIO_get_mem_ptr(bio, &bio_buf);
No status checks?  OpenSSL calls return 1 on success and 0 on failure,
so I would check after <= 0 here.

++                      if (port->hba->clientcertname == clientCertDN)
++                      {
++                              ereport(LOG,
May be better to use a switch() here as well.

It looks like this patch misses src/test/ssl/ssl/client-dn.crt,
causing the SSL tests to fail.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Allow matching whole DN from a client certificate

Michael Paquier-2
On Mon, Mar 29, 2021 at 10:57:00AM +0900, Michael Paquier wrote:

> +   switch (port->hba->clientcertname)
> +   {
> +       case clientCertDN:
> +           peer_username = port->peer_dn;
> +           break;
> +       default:
> +           peer_username = port->peer_cn;
> +   }
>
> This does not need a "default".  I think that you should use "case
> clientCertCN" instead here.
>
> +              BIO_get_mem_ptr(bio, &bio_buf);
> No status checks?  OpenSSL calls return 1 on success and 0 on failure,
> so I would check after <= 0 here.
>
> ++                      if (port->hba->clientcertname == clientCertDN)
> ++                      {
> ++                              ereport(LOG,
> May be better to use a switch() here as well.
>
> It looks like this patch misses src/test/ssl/ssl/client-dn.crt,
> causing the SSL tests to fail.
For the sake of the archives, this has been applied as of 6d7a6fe with
all those nits from me addressed.
--
Michael

signature.asc (849 bytes) Download Attachment
12