Re: Allow matching whole DN from a client certificate
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, _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
Description: PGP signature
Re: Allow matching whole DN from a client certificate
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, _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
Description: PGP signature
Re: Allow matching whole DN from a client certificate
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
diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index b420486a0a..951af49e9a 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -598,7 +598,7 @@ hostnogssenc database user
- In addition to the method-specific options listed below, there is one
+ In addition to the method-specific options listed below, there is a
method-independent authentication option clientcert, which
can be specified in any hostssl record.
This option can be set to verify-ca or
@@ -612,6 +612,28 @@ hostnogssenc database userhostssl entries.
+
+ On any record using client certificate authentication (i.e. one
+ using the cert authentication method or one
+ using the clientcert option), you can specify
+ which part of the client certificate credentials to match using
+ the clientname option. This option can have one
+ of two values. If you specify clientname=CN, which
+ is the default, the username is matched against the certificate's
+ Common Name (CN). If instead you specify
+ clientname=DN the username is matched against the
+ entire Distinguished Name (DN) of the certificate.
+ This option is probably best used in conjunction with a username map.
+ The comparison is done with the DN in
+ https://tools.ietf.org/html/rfc2253;>RFC 2253
+ format. To see the DN of a client certificate
+ in this format, do
+
+openssl x509 -in myclient.crt -noout --subject -nameopt RFC2253 | sed "s/^subject=//"
+
+Care needs to be taken when using this option, especially when using
+regular expression matching against the DN.
+
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 994251e7d9..32ef05783a 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -2800,12 +2800,23 @@ static int
CheckCertAuth(Port *port)
{
int status_check_usermap = STATUS_ERROR;
+ char *peer_username;
Assert(port->ssl);
+ /* select the correct field to compare */
+ switch (port->hba->clientcertname)
+ {
+ case clientCertDN:
+ peer_username = port->peer_dn;
+ break;
+ default:
+ peer_username = port->peer_cn;
+ }
+
/* Make sure we have received a username in the certificate */
- if (port->peer_cn == NULL ||
- strlen(port->peer_cn) <= 0)
+ if (peer_username == NULL ||
+ strlen(peer_username) <= 0)
{
ereport(LOG,
(errmsg("certificate authentication failed for user \"%s\": client certificate contains no user name",
@@ -2813,8 +2824,8 @@ CheckCertAuth(Port *port)
return STATUS_ERROR;
}
- /* Just pass the certificate cn to the usermap check */
- status_check_usermap = check_usermap(port->hba->usermap, port->user_name, port->peer_cn, false);
+ /* Just pass the certificate cn/dn to the usermap check */
+ status_check_usermap = check_usermap(port->hba->usermap, port->user_name, peer_username, false);
if (status_check_usermap != STATUS_OK)
{
/*
@@ -2824,9 +2835,18 @@ CheckCertAuth(Port *port)
*/
if (port->hba->clientcert == clientCertFull && port->hba->auth_method != uaCert)
{
- ereport(LOG,
- (errmsg("certificate validation (clientcert=verify-full) failed for user \"%s\": CN mismatch",
- port->user_name)));
+ if (port->hba->clientcertname == clientCertDN)
+ {
+ereport(LOG,
+ (errmsg("certificate validation (clientcert=verify-full) failed for user \"%s\": DN mismatch",
+port->user_name)));
+ }
+ else
+ {
+ereport(LOG,
+ (errmsg("certificate validation (clientcert=verify-full) failed for user \"%s\": CN mismatch",
+port->user_name)));
+ }
}
}
return status_check_usermap;
diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index 5ce3f27855..65b07c7d3f 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -551,22 +551,26 @@ aloop:
/* Get client certificate, if available. */
port->peer = SSL_get_peer_certificate(port->ssl);
- /* and extract the Common Name from it. */
+ /* and extract the Common Name and Distinguished Name from it. */
port->peer_cn = NULL;
+ port->peer_dn = NULL;
port->peer_cert_valid = false;
if (port->peer != NULL)
{
int len;
+ X509_NAME *x509name = X509_get_subject_name(port->peer);
+ char *peer_dn;
+ BIO *bio = NULL;
+ BUF_MEM*bio_buf = NULL;
- len = X509_NAME_get_text_by_NID(X509_get_subject_name(port->peer),
- NID_commonName, NULL, 0);
+ len = X509_NAME_get_text_by_NID(x509name, NID_commonName, NULL, 0);
if (len != -1)
{
char *peer_cn;
peer_cn = MemoryContextAlloc(TopMemoryContext, len + 1);
- r =
Re: Allow matching whole DN from a client certificate
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 DN 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, _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
Description: PGP signature
Re: Allow matching whole DN from a client certificate
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
diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index b420486a0a..61b646c20a 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -598,7 +598,7 @@ hostnogssenc database user
- In addition to the method-specific options listed below, there is one
+ In addition to the method-specific options listed below, there is a
method-independent authentication option clientcert, which
can be specified in any hostssl record.
This option can be set to verify-ca or
@@ -612,6 +612,27 @@ hostnogssenc database userhostssl entries.
+
+ On any record using client certificate authentication (i.e. one
+ using the cert authentication method or one
+ using the clientcert option), you can specify
+ which part of the client certificate credentials to match using
+ the clientname option. This option can have one
+ of two values. If you specify clientname=CN, which
+ is the default, the username is matched against the certificate's
+ Common Name (CN). If instead you specify
+ clientname=DN the username is matched against the
+ entire Distinguished Name (DN) of the certificate.
+ This option is probably best used in conjunction with a username map.
+ The comparison is done with the DN in RFC2253 format.
+ To see the DN of a client certificate in this format,
+ do
+
+openssl x509 -in myclient.crt -noout --subject -nameopt RFC2253 | sed "s/^subject=//"
+
+Care need to be taken when using this option, especially when using
+regular expression matching against the DN.
+
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 994251e7d9..5b4bad4cff 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -2800,12 +2800,15 @@ static int
CheckCertAuth(Port *port)
{
int status_check_usermap = STATUS_ERROR;
+ char *peer_username;
Assert(port->ssl);
/* 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;
+
+ if (peer_username == NULL ||
+ strlen(peer_username) <= 0)
{
ereport(LOG,
(errmsg("certificate authentication failed for user \"%s\": client certificate contains no user name",
@@ -2813,8 +2816,8 @@ CheckCertAuth(Port *port)
return STATUS_ERROR;
}
- /* Just pass the certificate cn to the usermap check */
- status_check_usermap = check_usermap(port->hba->usermap, port->user_name, port->peer_cn, false);
+ /* Just pass the certificate cn/dn to the usermap check */
+ status_check_usermap = check_usermap(port->hba->usermap, port->user_name, peer_username, false);
if (status_check_usermap != STATUS_OK)
{
/*
@@ -2825,7 +2828,7 @@ CheckCertAuth(Port *port)
if (port->hba->clientcert == clientCertFull && port->hba->auth_method != uaCert)
{
ereport(LOG,
- (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",
port->user_name)));
}
}
diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index 4c4f025eb1..d0184a2ce2 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -543,22 +543,25 @@ aloop:
/* Get client certificate, if available. */
port->peer = SSL_get_peer_certificate(port->ssl);
- /* and extract the Common Name from it. */
+ /* and extract the Common Name / Distinguished Name from it. */
port->peer_cn = NULL;
+ port->peer_dn = NULL;
port->peer_cert_valid = false;
if (port->peer != NULL)
{
int len;
+ X509_NAME *x509name = X509_get_subject_name(port->peer);
+ char *peer_cn;
+ char *peer_dn;
+ BIO *bio = NULL;
+ BUF_MEM*bio_buf = NULL;
- len = X509_NAME_get_text_by_NID(X509_get_subject_name(port->peer),
- NID_commonName, NULL, 0);
+ len = X509_NAME_get_text_by_NID(x509name, NID_commonName, NULL, 0);
if (len != -1)
{
- char *peer_cn;
-
peer_cn = MemoryContextAlloc(TopMemoryContext, len + 1);
- r = X509_NAME_get_text_by_NID(X509_get_subject_name(port->peer),
- NID_commonName, peer_cn, len + 1);
+ r = X509_NAME_get_text_by_NID(x509name, NID_commonName, peer_cn,
+ len + 1);
peer_cn[len] = '\0';
if (r != len)
{
@@ -582,6 +585,36 @@ aloop:
Re: Allow matching whole DN from a client certificate
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
Re: Allow matching whole DN from a client certificate
On Wed, Mar 3, 2021 at 3:03 AM Jacob Champion 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
Re: Allow matching whole DN from a client certificate
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
Re: Allow matching whole DN from a client certificate
On Sat, Jan 30, 2021 at 04:18:12PM -0500, Andrew Dunstan wrote: > @@ -610,6 +610,19 @@ hostnogssenc database > user the verification of client certificates with any authentication > method that supports hostssl entries. > > + > + On any record using client certificate authentication, that is one > + using the cert authentication method or one > + using the clientcert option, you can specify I suggest instead of "that is" to instead parenthesize this part: | (one using the cert authentication method or the | clientcert option), you can specify > + which part of the client certificate credentials to match using > + the clientname option. This option can have one > + of two values. If you specify clientname=CN, which > + is the default, the username is matched against the certificate's > + Common Name (CN). If instead you specify > + clientname=DN the username is matched against the > + entire Distinguished Name (DN) of the certificate. > + This option is probably best used in comjunction with a username map. spell: conjunction
Re: Allow matching whole DN from a client certificate
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
Re: Allow matching whole DN from a client certificate
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
Re: Allow matching whole DN from a client certificate
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
Re: Allow matching whole DN from a client certificate
On Sat, 2021-01-30 at 16:18 -0500, Andrew Dunstan wrote:
> Here's a version of the patch that does it that way. For fun I have
> modified the certificate so it has two OU fields in the DN.
> diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
> [...]
> + Common Name (CN). If instead you specify
> + clientname=DN the username is matched against the
> + entire Distinguished Name (DN) of the certificate.
> + This option is probably best used in comjunction with a username map.
sp: comjunction -> conjunction
> diff --git a/src/backend/libpq/be-secure-openssl.c
> b/src/backend/libpq/be-secure-openssl.c
> [...]
> +
> + bio = BIO_new(BIO_s_mem());
> + if (!bio)
> + {
> + pfree(port->peer_cn);
> + port->peer_cn = NULL;
> + return -1;
> + }
Should this case have a log entry, DEBUG or otherwise?
> + /* use commas instead of slashes */
> + X509_NAME_print_ex(bio, x509name, 0, XN_FLAG_RFC2253);
> + BIO_get_mem_ptr(bio, _buf);
> + 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;
> + }
Since the definition of XN_FLAG_RFC2253 includes ASN1_STRFLGS_ESC_CTRL,
this case shouldn't be possible. I think it's still worth it to double-
check, but maybe it should assert as well? Or at least have a comment
explaining that this is an internal error and not a client error.
> diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
> +# correct client cert using whole DN
> +my $dn_connstr = $common_connstr;
> +$dn_connstr =~ s/certdb/certdb_dn/;
> +
> +test_connect_ok(
> + $dn_connstr,
> + "user=ssltestuser sslcert=ssl/client-dn.crt
> sslkey=ssl/client-dn_tmp.key",
> + "certificate authorization succeeds with DN mapping"
> +);
A negative case for the new DN code paths would be good to add.
--Jacob
Re: Allow matching whole DN from a client certificate
On 1/29/21 10:10 AM, Andrew Dunstan wrote:
> On 1/28/21 5:10 PM, Andrew Dunstan wrote:
>>> (I'd still recommend switching to use the RFC
>>> flag to OpenSSL, to ease future improvements.) There should be a bunch
>>> of warning documentation saying not to do anything more complex unless
>>> you're an expert, and that the exact regular expression needed may
>>> change depending on the TLS backend.
>> I'll play around with the RFC flag.
>>
>>
>>> If you want to add UTF-8 support to the above, so that things outside
>>> ASCII can be matched nicely, then the ASN1_STRFLGS_ESC_MSB flag should
>>> be removed from the _print_ex() call per OpenSSL documentation, and we
>>> should investigate how it plays with other non-UTF-8 database
>>> encodings. That seems like work but not a huge amount of it.
>> How about if we remove ASN1_STRFLGS_ESC_MSB when the server encoding is
>> UTF8? That should be an extremely simple change.
>>
>>
>>
> Of course, we don't have any idea what the database is at this stage, so
> that wasn't well thought out.
>
>
> I'm inclined at this stage just to turn on RFC2253. If someone wants to
> deal with UTF8 (or other encodings) at a later stage they can.
>
>
Here's a version of the patch that does it that way. For fun I have
modified the certificate so it has two OU fields in the DN.
Finding out how to generate the new cert without regenerating everything
else was also fun :-) Here's what I did in a pristine source with patch
applied, but where configure hasn't been run:
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
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?
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index c4b9971a20..948c4f8aab 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -596,7 +596,7 @@ hostnogssenc database user
- In addition to the method-specific options listed below, there is one
+ In addition to the method-specific options listed below, there is a
method-independent authentication option clientcert, which
can be specified in any hostssl record.
This option can be set to verify-ca or
@@ -610,6 +610,19 @@ hostnogssenc database userhostssl entries.
+
+ On any record using client certificate authentication, that is one
+ using the cert authentication method or one
+ using the clientcert option, you can specify
+ which part of the client certificate credentials to match using
+ the clientname option. This option can have one
+ of two values. If you specify clientname=CN, which
+ is the default, the username is matched against the certificate's
+ Common Name (CN). If instead you specify
+ clientname=DN the username is matched against the
+ entire Distinguished Name (DN) of the certificate.
+ This option is probably best used in comjunction with a username map.
+
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 545635f41a..d2e4c0b8a8 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -2848,12 +2848,15 @@ static int
CheckCertAuth(Port *port)
{
int status_check_usermap = STATUS_ERROR;
+ char *peer_username;
Assert(port->ssl);
/* 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;
+
+ if (peer_username == NULL ||
+ strlen(peer_username) <= 0)
{
ereport(LOG,
(errmsg("certificate authentication failed for user \"%s\": client certificate contains no user name",
@@ -2861,8 +2864,8 @@ CheckCertAuth(Port *port)
return STATUS_ERROR;
}
- /* Just pass the certificate cn to the usermap check */
- status_check_usermap = check_usermap(port->hba->usermap, port->user_name, port->peer_cn, false);
+ /* Just pass the certificate cn/dn to the usermap check */
+ status_check_usermap = check_usermap(port->hba->usermap, port->user_name, peer_username, false);
if (status_check_usermap != STATUS_OK)
{
/*
@@ -2873,7 +2876,7 @@ CheckCertAuth(Port *port)
if (port->hba->clientcert == clientCertFull && port->hba->auth_method != uaCert)
{
ereport(LOG,
- (errmsg("certificate validation (clientcert=verify-full) failed for user \"%s\": CN mismatch",
+ (errmsg("certificate validation (clientcert=verify-full) failed for user \"%s\":
Re: Allow matching whole DN from a client certificate
On 1/28/21 5:10 PM, Andrew Dunstan wrote: > >> (I'd still recommend switching to use the RFC >> flag to OpenSSL, to ease future improvements.) There should be a bunch >> of warning documentation saying not to do anything more complex unless >> you're an expert, and that the exact regular expression needed may >> change depending on the TLS backend. > > I'll play around with the RFC flag. > > >> If you want to add UTF-8 support to the above, so that things outside >> ASCII can be matched nicely, then the ASN1_STRFLGS_ESC_MSB flag should >> be removed from the _print_ex() call per OpenSSL documentation, and we >> should investigate how it plays with other non-UTF-8 database >> encodings. That seems like work but not a huge amount of it. > > How about if we remove ASN1_STRFLGS_ESC_MSB when the server encoding is > UTF8? That should be an extremely simple change. > > > Of course, we don't have any idea what the database is at this stage, so that wasn't well thought out. I'm inclined at this stage just to turn on RFC2253. If someone wants to deal with UTF8 (or other encodings) at a later stage they can. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Allow matching whole DN from a client certificate
On 1/29/21 8:18 AM, Daniel Gustafsson wrote: >> On 28 Jan 2021, at 23:10, Andrew Dunstan wrote: >> On 1/28/21 11:39 AM, Jacob Champion wrote: >>> Unfortunately I don't really know what that solution should look like. >>> A DSL for filtering on RDNs would be a lot of work, but it could >>> potentially allow LDAP to be mapped through pg_ident as well >> In the end it will be up to users to come up with expressions that meet >> their usage. Yes they could get it wrong, but then they can get so many >> things wrong ;-) > My main concern with this isn't that it's easy to get it wrong, but that it > may > end up being hard to get it right (with false positives in the auth path as a > result). Right now I'm not sure where it leans. > > Maybe it will be easier to judge the proposal when the documentation has been > updated warnings for the potential pitfalls? > Feel free to make suggestions for wording :-) cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Allow matching whole DN from a client certificate
> On 28 Jan 2021, at 23:10, Andrew Dunstan wrote: > On 1/28/21 11:39 AM, Jacob Champion wrote: >> >> Unfortunately I don't really know what that solution should look like. >> A DSL for filtering on RDNs would be a lot of work, but it could >> potentially allow LDAP to be mapped through pg_ident as well > > In the end it will be up to users to come up with expressions that meet > their usage. Yes they could get it wrong, but then they can get so many > things wrong ;-) My main concern with this isn't that it's easy to get it wrong, but that it may end up being hard to get it right (with false positives in the auth path as a result). Right now I'm not sure where it leans. Maybe it will be easier to judge the proposal when the documentation has been updated warnings for the potential pitfalls? -- Daniel Gustafsson https://vmware.com/
Re: Allow matching whole DN from a client certificate
On 1/28/21 11:39 AM, Jacob Champion wrote: > On Wed, 2021-01-27 at 15:42 -0500, Andrew Dunstan wrote: >> I'm not sure where we want to go with the present patch. Do we want to >> go with what we have and document the limitations more, or try to do >> something more elaborate? If so, exactly what? > After sleeping on it: > > I think that if you expect the 99% use case to be in the vein of what > you originally proposed: > > dn /^C=US,ST=North.Carolina,O=test,OU=eng,CN=andrew$ andrew > > where the *entire* DN is pinned, there are no characters outside the > ASCII subset, and no subgroup matching is required to find the mapped > username (i.e. there's one line for every user of the system), then > this approach would work. I think this is the 99% use case, TBH. It's certainly what I was originally asked for. > (I'd still recommend switching to use the RFC > flag to OpenSSL, to ease future improvements.) There should be a bunch > of warning documentation saying not to do anything more complex unless > you're an expert, and that the exact regular expression needed may > change depending on the TLS backend. I'll play around with the RFC flag. > > If you want to add UTF-8 support to the above, so that things outside > ASCII can be matched nicely, then the ASN1_STRFLGS_ESC_MSB flag should > be removed from the _print_ex() call per OpenSSL documentation, and we > should investigate how it plays with other non-UTF-8 database > encodings. That seems like work but not a huge amount of it. How about if we remove ASN1_STRFLGS_ESC_MSB when the server encoding is UTF8? That should be an extremely simple change. > But if you think users are going to try to match more complex regular > expressions, for example to be able to peel out a portion of the DN to > use as a mapped username, or match just a few specific RDNs for > authorization, then I think a more elaborate approach is needed to > avoid handing people a dangerous tool. Most of the DN-matching regex > examples I've seen on self-help sites have been wrong, in that they > match too much. > > Unfortunately I don't really know what that solution should look like. > A DSL for filtering on RDNs would be a lot of work, but it could > potentially allow LDAP to be mapped through pg_ident as well? > In the end it will be up to users to come up with expressions that meet their usage. Yes they could get it wrong, but then they can get so many things wrong ;-) cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Allow matching whole DN from a client certificate
On Wed, 2021-01-27 at 15:42 -0500, Andrew Dunstan wrote: > I'm not sure where we want to go with the present patch. Do we want to > go with what we have and document the limitations more, or try to do > something more elaborate? If so, exactly what? After sleeping on it: I think that if you expect the 99% use case to be in the vein of what you originally proposed: dn /^C=US,ST=North.Carolina,O=test,OU=eng,CN=andrew$ andrew where the *entire* DN is pinned, there are no characters outside the ASCII subset, and no subgroup matching is required to find the mapped username (i.e. there's one line for every user of the system), then this approach would work. (I'd still recommend switching to use the RFC flag to OpenSSL, to ease future improvements.) There should be a bunch of warning documentation saying not to do anything more complex unless you're an expert, and that the exact regular expression needed may change depending on the TLS backend. If you want to add UTF-8 support to the above, so that things outside ASCII can be matched nicely, then the ASN1_STRFLGS_ESC_MSB flag should be removed from the _print_ex() call per OpenSSL documentation, and we should investigate how it plays with other non-UTF-8 database encodings. That seems like work but not a huge amount of it. But if you think users are going to try to match more complex regular expressions, for example to be able to peel out a portion of the DN to use as a mapped username, or match just a few specific RDNs for authorization, then I think a more elaborate approach is needed to avoid handing people a dangerous tool. Most of the DN-matching regex examples I've seen on self-help sites have been wrong, in that they match too much. Unfortunately I don't really know what that solution should look like. A DSL for filtering on RDNs would be a lot of work, but it could potentially allow LDAP to be mapped through pg_ident as well? --Jacob
Re: Allow matching whole DN from a client certificate
On 1/27/21 3:14 PM, Jacob Champion wrote: > On Tue, 2021-01-26 at 18:43 +, Jacob Champion wrote: >> On Tue, 2021-01-26 at 13:49 +0100, Daniel Gustafsson wrote: >>> The OpenSSL X509_NAME_cmp function use RFC 5280 section 7.1 and RFC 4517 >>> section 4.2.15 (which in turn reference RFC 4514 for the DN string format). >>> libnss has CERT_AsciiToName which is referencing RFCs 1485, 1779 and 2253 in >>> lib/certdb/alg1485.c. Comparing the two would be interesting. >> Yeah. I'll poke around a bit. > Here's some output from a test program I threw together, which parses > identical DER using OpenSSL and NSS and writes the corresponding string > representation. > >> input/basic.conf: >> ssl: CN=pchampion,OU=VMware >> nss: CN=pchampion,OU=VMware >> >> input/escape.conf: >> ssl: CN=\,\+\\\;\"\<\> >> nss: CN=",+\\;\"<>" >> >> input/multivalue.conf: >> ssl: CN=pchampion+SN=Champion+GN=Jacob,OU=VMware >> nss: givenName=Jacob+SN=Champion+CN=pchampion,OU=VMware >> >> input/unicode.conf: >> ssl: CN=οδυσσέας,OU=VMware >> nss: CN=οδυσσέας,OU=VMware >> >> input/unprintable.conf: >> ssl: CN=\01\,\02\,\03,OU=\01\02\03 >> nss: CN="\01,\02,\03",OU=\01\02\03 > basic.conf is exactly what it looks like: CN=pchampion,OU=VMware. Both > implementations agree. > > escape.conf contains a CN with the literal value > > ,+\;"<> > > and you can see that NSS doesn't follow RFC 4514 here; it uses the > older double-quoting form of escaping. There's a 15-year-old bug on > this in NSS [1]. > > multivalue.conf contains a multivalued AVA with commonName "pchampion", > givenName "Jacob", and surname "Champion". They aren't sorted in the > same order, and the implementations even disagree on how to represent > the givenName attribute. (I'm not convinced that either choice is RFC- > 4514-compliant; it doesn't look like GN is registered with IANA as a > short name for givenName.) > > unicode.conf contains a commonName of "οδυσσέας". Both implementations > agree, but the only way I was able to get OpenSSL to produce this > (rather than a string of escaped hex) was by using the flags > > XN_FLAG_RFC2253 & ~ASN1_STRFLGS_ESC_MSB > > in the call to X509_NAME_print_ex(). This should work fine for a > database encoding of UTF-8, but would we need to convert for other > encodings? Also, I'm not sure how this would handle certificates that > aren't UTF-8 encoded. It looks like some UCS variants are legal? > > unprintable.conf contains the bytes 0x01, 0x02, and 0x03 in the > commonName and organizationalUnit. They're backslash-escaped by both > implementations, but if you add any other printable escaped characters > (such as comma, in the CN example here) NSS will still double-quote the > whole thing. > > --Jacob > > [1] https://bugzilla.mozilla.org/show_bug.cgi?id=355096 OK, that bug is a bit ugly. I'm not sure where we want to go with the present patch. Do we want to go with what we have and document the limitations more, or try to do something more elaborate? If so, exactly what? cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Allow matching whole DN from a client certificate
On Tue, 2021-01-26 at 18:43 +, Jacob Champion wrote: > On Tue, 2021-01-26 at 13:49 +0100, Daniel Gustafsson wrote: > > The OpenSSL X509_NAME_cmp function use RFC 5280 section 7.1 and RFC 4517 > > section 4.2.15 (which in turn reference RFC 4514 for the DN string format). > > libnss has CERT_AsciiToName which is referencing RFCs 1485, 1779 and 2253 in > > lib/certdb/alg1485.c. Comparing the two would be interesting. > > Yeah. I'll poke around a bit. Here's some output from a test program I threw together, which parses identical DER using OpenSSL and NSS and writes the corresponding string representation. > input/basic.conf: > ssl: CN=pchampion,OU=VMware > nss: CN=pchampion,OU=VMware > > input/escape.conf: > ssl: CN=\,\+\\\;\"\<\> > nss: CN=",+\\;\"<>" > > input/multivalue.conf: > ssl: CN=pchampion+SN=Champion+GN=Jacob,OU=VMware > nss: givenName=Jacob+SN=Champion+CN=pchampion,OU=VMware > > input/unicode.conf: > ssl: CN=οδυσσέας,OU=VMware > nss: CN=οδυσσέας,OU=VMware > > input/unprintable.conf: > ssl: CN=\01\,\02\,\03,OU=\01\02\03 > nss: CN="\01,\02,\03",OU=\01\02\03 basic.conf is exactly what it looks like: CN=pchampion,OU=VMware. Both implementations agree. escape.conf contains a CN with the literal value ,+\;"<> and you can see that NSS doesn't follow RFC 4514 here; it uses the older double-quoting form of escaping. There's a 15-year-old bug on this in NSS [1]. multivalue.conf contains a multivalued AVA with commonName "pchampion", givenName "Jacob", and surname "Champion". They aren't sorted in the same order, and the implementations even disagree on how to represent the givenName attribute. (I'm not convinced that either choice is RFC- 4514-compliant; it doesn't look like GN is registered with IANA as a short name for givenName.) unicode.conf contains a commonName of "οδυσσέας". Both implementations agree, but the only way I was able to get OpenSSL to produce this (rather than a string of escaped hex) was by using the flags XN_FLAG_RFC2253 & ~ASN1_STRFLGS_ESC_MSB in the call to X509_NAME_print_ex(). This should work fine for a database encoding of UTF-8, but would we need to convert for other encodings? Also, I'm not sure how this would handle certificates that aren't UTF-8 encoded. It looks like some UCS variants are legal? unprintable.conf contains the bytes 0x01, 0x02, and 0x03 in the commonName and organizationalUnit. They're backslash-escaped by both implementations, but if you add any other printable escaped characters (such as comma, in the CN example here) NSS will still double-quote the whole thing. --Jacob [1] https://bugzilla.mozilla.org/show_bug.cgi?id=355096
Re: Allow matching whole DN from a client certificate
On Tue, 2021-01-26 at 13:49 +0100, Daniel Gustafsson wrote:
> Reading more on this it seems we would essentially have to go with RFC 4514,
> as
> it's the closest to a standard which seems to exist. Having done a lot
> research on this topic, do you have a gut feeling on direction?
Yeah, if we're going to use string matching then I agree with the use
of RFC 4514. I think a string-based approach is going to work for only
the most simple cases, though. Anything more complicated than plain
ASCII and a fixed base DN is going to become pretty unmanageable with a
regex; you'd need a filter system of some sort that understands DN
structure. The documentation should be clear on the limitations.
> The OpenSSL X509_NAME_cmp function use RFC 5280 section 7.1 and RFC 4517
> section 4.2.15 (which in turn reference RFC 4514 for the DN string format).
> libnss has CERT_AsciiToName which is referencing RFCs 1485, 1779 and 2253 in
> lib/certdb/alg1485.c. Comparing the two would be interesting.
Yeah. I'll poke around a bit.
> Right, multi-value RDN's are defined as sets so C=US,A=B+C=D is equivalent to
> C=US,C=D+A=B according to RFC 5280.
Other potential differences, by my understanding of that RFC, include
the quoting of the "escaped" character class (e.g. a comma can be
escaped as either "\," or "\2C"), the case of hex characters ("\FF" vs
"\ff"), and the option to not quote unprintable control characters
(e.g. ASCII DEL, 0x7F, can be included verbatim or quoted as "\7F").
--Jacob
Re: Allow matching whole DN from a client certificate
> On 20 Jan 2021, at 20:07, Jacob Champion wrote: > > On Mon, 2021-01-18 at 11:23 +0100, Daniel Gustafsson wrote: >> + /* use commas instead of slashes */ >> + X509_NAME_print_ex(bio, x509name, 0, XN_FLAG_SEP_COMMA_PLUS); >> I don't have strong opinions on whether we shold use slashes or commas, but I >> think it needs to be documented that commas are required since slashes is the >> more common way to print a DN. > > There's also a XN_FLAG_RFC2253 flag, which claims to print in an RFC- > compatible escape system. (It includes XN_FLAG_SEP_COMMA_PLUS.) I think > NSS uses a similar RFC-based escape scheme, but I haven't checked to > see whether it's fully compatible. > > I think you'll want to be careful to specify the format as much as > possible, both to make sure that other backend TLS implementations can > actually use the same escaping system and to ensure that user regexes > don't suddenly start matching different things at some point in the > future. Reading more on this it seems we would essentially have to go with RFC 4514, as it's the closest to a standard which seems to exist. Having done a lot research on this topic, do you have a gut feeling on direction? The OpenSSL X509_NAME_cmp function use RFC 5280 section 7.1 and RFC 4517 section 4.2.15 (which in turn reference RFC 4514 for the DN string format). libnss has CERT_AsciiToName which is referencing RFCs 1485, 1779 and 2253 in lib/certdb/alg1485.c. Comparing the two would be interesting. > Even when using RFC 2253 (now 4514) escaping, there are things left to the > implementation, such as the order of AVAs inside multivalued RDNs. The RFC > warns not to consider these representations to be canonical forms, so it's > possible that switching (or upgrading) the TLS library in use could force > users to change their regexes at some point in the future. Right, multi-value RDN's are defined as sets so C=US,A=B+C=D is equivalent to C=US,C=D+A=B according to RFC 5280. -- Daniel Gustafsson https://vmware.com/
Re: Allow matching whole DN from a client certificate
On Wed, 2021-01-20 at 19:07 +, Jacob Champion wrote: > I think you'll want to be careful to specify the format as much as > possible, both to make sure that other backend TLS implementations can > actually use the same escaping system and to ensure that user regexes > don't suddenly start matching different things at some point in the > future. Along those lines: the current implementation doesn't escape commas in fields, which means you can inject them to force a bad regex match. For instance, when using the usermap that's in the patch: dn"/^.*OU=Testing,.*$"username if I create a certificate with the Organizational Unit name "Testing, or something", then that will also match. Switching to RFC 2253/4514 quoting fixes comma injection (and reverses the order of the RDNs, which requires a change to the regex patterns). But I think that the regex as supplied still isn't strong enough to prevent problems. For example, the equals sign isn't a delimiter and therefore isn't quoted. So if I get my CA to sign a certificate with some arbitrary field value of "HEY YOU=Testing", then that will also match the above usermap. You'd need to write the regex with extreme attention to detail and a full understanding of the escaping scheme to get around that -- assuming that the scheme is generally parsable with regexes to begin with. > I'm going to test this patch with some UTF-8 DNs later today; I'll share my > findings. UTF-8 has the opposite issue; it's escaped in a way that makes it unusable in a regex match. For example, say I have a (simple for the sake of example, but broken as noted above) usermap of dn"/^CN=([^,]*).*$"\1 which is supposed to emulate the functionality of the "clientname=CN" mode, and two users named "postgres" and "οδυσσέας". The "CN=postgres" user will work just fine, but the UTF-8 CN of "οδυσσέας" will be escaped into "\U03BF\U03B4\U03C5\U03C3\U03C3\U03AD\U03B1\U03C2" and fail to match the internal user. (I'm not seeing an RFC describe the "\U" escaping scheme; maybe it's OpenSSL-specific?) --Jacob
Re: Allow matching whole DN from a client certificate
On Mon, 2021-01-18 at 11:23 +0100, Daniel Gustafsson wrote: > + /* use commas instead of slashes */ > + X509_NAME_print_ex(bio, x509name, 0, XN_FLAG_SEP_COMMA_PLUS); > I don't have strong opinions on whether we shold use slashes or commas, but I > think it needs to be documented that commas are required since slashes is the > more common way to print a DN. There's also a XN_FLAG_RFC2253 flag, which claims to print in an RFC- compatible escape system. (It includes XN_FLAG_SEP_COMMA_PLUS.) I think NSS uses a similar RFC-based escape scheme, but I haven't checked to see whether it's fully compatible. I think you'll want to be careful to specify the format as much as possible, both to make sure that other backend TLS implementations can actually use the same escaping system and to ensure that user regexes don't suddenly start matching different things at some point in the future. As a cautionary tale, nginx is stuck with two versions of their similar feature (ssl_client_s_dn_legacy vs ssl_client_s_dn) after their switch to an RFC-compatible system [1]. Even when using RFC 2253 (now 4514) escaping, there are things left to the implementation, such as the order of AVAs inside multivalued RDNs. The RFC warns not to consider these representations to be canonical forms, so it's possible that switching (or upgrading) the TLS library in use could force users to change their regexes at some point in the future. I'm going to test this patch with some UTF-8 DNs later today; I'll share my findings. I'm also going to read up on [2] a bit more. --Jacob [1] https://serverfault.com/questions/829754/why-did-the-format-of-nginx-ssl-client-i-dn-suddenly-change [2] https://frasertweedale.github.io/blog-redhat/posts/2019-05-28-a-dn-is-not-a-string.html
Re: Allow matching whole DN from a client certificate
I've circled back to this and tested/read it more, and I'm still of the opinion that it's a good feature addition. A few small comments on the patch: + is the default, the username is matched against the certificate's In the docs we use "user name" instead of "username" in descriptive text. There are a couple of "username" in this added paragraph. + This option is probably best used in comjunction with a username map. Typo: s/comjunction/conjunction/ + /* use commas instead of slashes */ + X509_NAME_print_ex(bio, x509name, 0, XN_FLAG_SEP_COMMA_PLUS); I don't have strong opinions on whether we shold use slashes or commas, but I think it needs to be documented that commas are required since slashes is the more common way to print a DN. pg_stat_ssl and sslinfo are also displaying the DN with slashes. /* 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; Nitpickering nitpickery perhaps, but when we can inspect the DN and not just the CN it seems a bit odd to talk about "username", which is again echoed in the errormessage just below here. Not sure what would be better though, since "user information" doesn't really convey enough detail/meaning. + /* and extract the Common Name / Distinguished Name from it. */ Not introduced in this patch, but single line comments should not be punctuated IIRC. The patch still applies and tests pass, I'm moving this to ready for committer. cheers ./daniel
Re: Allow matching whole DN from a client certificate
> On 18 Nov 2020, at 19:01, Andrew Dunstan wrote: > OK, here's a new patch, including docco and tests. Looks good on a quick skim, the only thing that stood out was: + This option is probably best used in comjunction with a username map. s/comjunction/conjunction/ Will do more testing and review, and give a think on how it affects the libnss patch. cheers ./daniel
Re: Allow matching whole DN from a client certificate
On 11/12/20 4:21 PM, Andrew Dunstan wrote:
> On 11/12/20 8:37 AM, Daniel Gustafsson wrote:
>>> On 11 Nov 2020, at 21:44, Andrew Dunstan wrote:
>>> If people like this idea I'll add tests and docco and add it to the next CF.
>> Sounds like a good idea, please do.
>>
>> Can this case really happen in non-ancient OpenSSL version?
>> +if (!x509name)
> Probably not. I'll get rid of that.
>
>
>> Doesn't this returnpath need a pfree(peer_cn)?
>> +bio = BIO_new(BIO_s_mem());
>> +if (!bio)
>> +{
>> +return -1;
>> +}
>>
> Yeah, I'll make another pass over the cleanups.
>
OK, here's a new patch, including docco and tests.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index bad3c3469c..b039522f96 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -604,7 +604,7 @@ hostnogssenc database user
- In addition to the method-specific options listed below, there is one
+ In addition to the method-specific options listed below, there is a
method-independent authentication option clientcert, which
can be specified in any hostssl record.
This option can be set to verify-ca or
@@ -618,6 +618,19 @@ hostnogssenc database userhostssl entries.
+
+ On any record using client certificate authentication, that is one
+ using the cert authentication method or one
+ using the clientcert option, you can specify
+ which part of the client certificate credentials to match using
+ the clientname option. This option can have one
+ of two values. If you specify clientname=CN, which
+ is the default, the username is matched against the certificate's
+ Common Name (CN). If instead you specify
+ clientname=DN the username is matched against the
+ entire Distinguished Name (DN) of the certificate.
+ This option is probably best used in comjunction with a username map.
+
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index d132c5cb48..7e40de82e0 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -2869,12 +2869,15 @@ static int
CheckCertAuth(Port *port)
{
int status_check_usermap = STATUS_ERROR;
+ char *peer_username;
Assert(port->ssl);
/* 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;
+
+ if (peer_username == NULL ||
+ strlen(peer_username) <= 0)
{
ereport(LOG,
(errmsg("certificate authentication failed for user \"%s\": client certificate contains no user name",
@@ -2882,8 +2885,8 @@ CheckCertAuth(Port *port)
return STATUS_ERROR;
}
- /* Just pass the certificate cn to the usermap check */
- status_check_usermap = check_usermap(port->hba->usermap, port->user_name, port->peer_cn, false);
+ /* Just pass the certificate cn/dn to the usermap check */
+ status_check_usermap = check_usermap(port->hba->usermap, port->user_name, peer_username, false);
if (status_check_usermap != STATUS_OK)
{
/*
@@ -2894,7 +2897,7 @@ CheckCertAuth(Port *port)
if (port->hba->clientcert == clientCertFull && port->hba->auth_method != uaCert)
{
ereport(LOG,
- (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",
port->user_name)));
}
}
diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index e10260051f..6fe06c8d2a 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -520,22 +520,25 @@ aloop:
/* Get client certificate, if available. */
port->peer = SSL_get_peer_certificate(port->ssl);
- /* and extract the Common Name from it. */
+ /* and extract the Common Name / Distinguished Name from it. */
port->peer_cn = NULL;
+ port->peer_dn = NULL;
port->peer_cert_valid = false;
if (port->peer != NULL)
{
int len;
+ X509_NAME *x509name = X509_get_subject_name(port->peer);
+ char *peer_cn;
+ char *peer_dn;
+ BIO *bio = NULL;
+ BUF_MEM*bio_buf = NULL;
- len = X509_NAME_get_text_by_NID(X509_get_subject_name(port->peer),
- NID_commonName, NULL, 0);
+ len = X509_NAME_get_text_by_NID(x509name, NID_commonName, NULL, 0);
if (len != -1)
{
- char *peer_cn;
-
peer_cn = MemoryContextAlloc(TopMemoryContext, len + 1);
- r = X509_NAME_get_text_by_NID(X509_get_subject_name(port->peer),
- NID_commonName, peer_cn, len + 1);
+ r = X509_NAME_get_text_by_NID(x509name, NID_commonName, peer_cn,
+ len + 1);
peer_cn[len] = '\0';
if (r != len)
{
@@ -559,6
Re: Allow matching whole DN from a client certificate
On 11/12/20 8:37 AM, Daniel Gustafsson wrote:
>> On 11 Nov 2020, at 21:44, Andrew Dunstan wrote:
>> If people like this idea I'll add tests and docco and add it to the next CF.
> Sounds like a good idea, please do.
>
> Can this case really happen in non-ancient OpenSSL version?
> + if (!x509name)
Probably not. I'll get rid of that.
> Doesn't this returnpath need a pfree(peer_cn)?
> + bio = BIO_new(BIO_s_mem());
> + if (!bio)
> + {
> + return -1;
> + }
>
Yeah, I'll make another pass over the cleanups.
Thanks for reviewing.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Re: Allow matching whole DN from a client certificate
> On 11 Nov 2020, at 21:44, Andrew Dunstan wrote:
> If people like this idea I'll add tests and docco and add it to the next CF.
Sounds like a good idea, please do.
Can this case really happen in non-ancient OpenSSL version?
+ if (!x509name)
Doesn't this returnpath need a pfree(peer_cn)?
+ bio = BIO_new(BIO_s_mem());
+ if (!bio)
+ {
+ return -1;
+ }
cheers ./daniel
Re: Allow matching whole DN from a client certificate
Greetings, * Andrew Dunstan (and...@dunslane.net) wrote: > Currently we only match the Common Name (CN) of a client certificate > when authenticating a user. The attached patch allows matching the > entire Distinguished Name (DN) of the certificate. This is enabled by > the HBA line option "clientname", which can take the values "CN" or > "DN". "CN" is the default. > > The idea is that you might have a role with a CN of, say, "dbauser" in > two different parts of the organization, say one with "OU=marketing" and > the other with "OU=engineering", and you only want to allow access to > one of them. > > This feature is best used in conjunction with a map. e.g. in testing I > have this pg_hba.conf line: > > hostssl all all 127.0.0.1/32 cert clientname=DN map=dn > > and this pg_ident.conf line: > > dn /^C=US,ST=North.Carolina,O=test,OU=eng,CN=andrew$ andrew > > If people like this idea I'll add tests and docco and add it to the next CF. Yeah, this is definitely a worthwhile feature. Thanks, Stephen signature.asc Description: PGP signature
Allow matching whole DN from a client certificate
Currently we only match the Common Name (CN) of a client certificate
when authenticating a user. The attached patch allows matching the
entire Distinguished Name (DN) of the certificate. This is enabled by
the HBA line option "clientname", which can take the values "CN" or
"DN". "CN" is the default.
The idea is that you might have a role with a CN of, say, "dbauser" in
two different parts of the organization, say one with "OU=marketing" and
the other with "OU=engineering", and you only want to allow access to
one of them.
This feature is best used in conjunction with a map. e.g. in testing I
have this pg_hba.conf line:
hostssl all all 127.0.0.1/32 cert clientname=DN map=dn
and this pg_ident.conf line:
dn /^C=US,ST=North.Carolina,O=test,OU=eng,CN=andrew$ andrew
If people like this idea I'll add tests and docco and add it to the next CF.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
"
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index d132c5cb48..7e40de82e0 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -2869,12 +2869,15 @@ static int
CheckCertAuth(Port *port)
{
int status_check_usermap = STATUS_ERROR;
+ char *peer_username;
Assert(port->ssl);
/* 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;
+
+ if (peer_username == NULL ||
+ strlen(peer_username) <= 0)
{
ereport(LOG,
(errmsg("certificate authentication failed for user \"%s\": client certificate contains no user name",
@@ -2882,8 +2885,8 @@ CheckCertAuth(Port *port)
return STATUS_ERROR;
}
- /* Just pass the certificate cn to the usermap check */
- status_check_usermap = check_usermap(port->hba->usermap, port->user_name, port->peer_cn, false);
+ /* Just pass the certificate cn/dn to the usermap check */
+ status_check_usermap = check_usermap(port->hba->usermap, port->user_name, peer_username, false);
if (status_check_usermap != STATUS_OK)
{
/*
@@ -2894,7 +2897,7 @@ CheckCertAuth(Port *port)
if (port->hba->clientcert == clientCertFull && port->hba->auth_method != uaCert)
{
ereport(LOG,
- (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",
port->user_name)));
}
}
diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index e10260051f..dbb32c86bb 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -520,22 +520,30 @@ aloop:
/* Get client certificate, if available. */
port->peer = SSL_get_peer_certificate(port->ssl);
- /* and extract the Common Name from it. */
+ /* and extract the Common Name / Distinguished Name from it. */
port->peer_cn = NULL;
+ port->peer_dn = NULL;
port->peer_cert_valid = false;
if (port->peer != NULL)
{
int len;
+ X509_NAME *x509name = X509_get_subject_name(port->peer);
+ char *peer_cn;
+ char *peer_dn;
+ BIO *bio = NULL;
+ BUF_MEM*bio_buf = NULL;
- len = X509_NAME_get_text_by_NID(X509_get_subject_name(port->peer),
- NID_commonName, NULL, 0);
- if (len != -1)
+ if (!x509name)
{
- char *peer_cn;
+ return -1;
+ }
+ len = X509_NAME_get_text_by_NID(x509name, NID_commonName, NULL, 0);
+ if (len != -1)
+ {
peer_cn = MemoryContextAlloc(TopMemoryContext, len + 1);
- r = X509_NAME_get_text_by_NID(X509_get_subject_name(port->peer),
- NID_commonName, peer_cn, len + 1);
+ r = X509_NAME_get_text_by_NID(x509name, NID_commonName, peer_cn,
+ len + 1);
peer_cn[len] = '\0';
if (r != len)
{
@@ -559,6 +567,32 @@ aloop:
port->peer_cn = peer_cn;
}
+
+ bio = BIO_new(BIO_s_mem());
+ if (!bio)
+ {
+ return -1;
+ }
+ /* use commas instead of slashes */
+ X509_NAME_print_ex(bio, x509name, 0, XN_FLAG_SEP_COMMA_PLUS);
+ BIO_get_mem_ptr(bio, _buf);
+ 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);
+ return -1;
+ }
+
+ BIO_free(bio);
+
+ port->peer_dn = peer_dn;
+
port->peer_cert_valid = true;
}
@@ -590,6 +624,12 @@ be_tls_close(Port *port)
pfree(port->peer_cn);
port->peer_cn = NULL;
}
+
+ if (port->peer_dn)
+ {
+ pfree(port->peer_dn);
+ port->peer_dn = NULL;
+ }
}
ssize_t
diff --git a/src/backend/libpq/be-secure.c b/src/backend/libpq/be-secure.c
index 2ae507a902..ef1320e686 100644
--- a/src/backend/libpq/be-secure.c
+++
