Allow matching whole DN from a client certificate

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

Allow matching whole DN from a client certificate

Andrew Dunstan

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
"


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

Re: Allow matching whole DN from a client certificate

Stephen Frost
Greetings,

* Andrew Dunstan ([hidden email]) 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 (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Allow matching whole DN from a client certificate

Daniel Gustafsson
In reply to this post by Andrew Dunstan
> On 11 Nov 2020, at 21:44, Andrew Dunstan <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Allow matching whole DN from a client certificate

Andrew Dunstan

On 11/12/20 8:37 AM, Daniel Gustafsson wrote:
>> On 11 Nov 2020, at 21:44, Andrew Dunstan <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

Re: Allow matching whole DN from a client certificate

Andrew Dunstan

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 <[hidden email]> 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


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

Re: Allow matching whole DN from a client certificate

Daniel Gustafsson
> On 18 Nov 2020, at 19:01, Andrew Dunstan <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Allow matching whole DN from a client certificate

Daniel Gustafsson
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

Reply | Threaded
Open this post in threaded view
|

Re: Allow matching whole DN from a client certificate

Jacob Champion-2
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
Reply | Threaded
Open this post in threaded view
|

Re: Allow matching whole DN from a client certificate

Jacob Champion-2
On Wed, 2021-01-20 at 19:07 +0000, 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




Reply | Threaded
Open this post in threaded view
|

Re: Allow matching whole DN from a client certificate

Daniel Gustafsson
In reply to this post by Jacob Champion-2
> On 20 Jan 2021, at 20:07, Jacob Champion <[hidden email]> 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/

Reply | Threaded
Open this post in threaded view
|

Re: Allow matching whole DN from a client certificate

Jacob Champion-2
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
Reply | Threaded
Open this post in threaded view
|

Re: Allow matching whole DN from a client certificate

Jacob Champion-2
On Tue, 2021-01-26 at 18:43 +0000, 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
Reply | Threaded
Open this post in threaded view
|

Re: Allow matching whole DN from a client certificate

Andrew Dunstan

On 1/27/21 3:14 PM, Jacob Champion wrote:

> On Tue, 2021-01-26 at 18:43 +0000, 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



Reply | Threaded
Open this post in threaded view
|

Re: Allow matching whole DN from a client certificate

Jacob Champion-2
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
Reply | Threaded
Open this post in threaded view
|

Re: Allow matching whole DN from a client certificate

Andrew Dunstan

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



Reply | Threaded
Open this post in threaded view
|

Re: Allow matching whole DN from a client certificate

Daniel Gustafsson
> On 28 Jan 2021, at 23:10, Andrew Dunstan <[hidden email]> 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/



Reply | Threaded
Open this post in threaded view
|

Re: Allow matching whole DN from a client certificate

Andrew Dunstan

On 1/29/21 8:18 AM, Daniel Gustafsson wrote:

>> On 28 Jan 2021, at 23:10, Andrew Dunstan <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

Re: Allow matching whole DN from a client certificate

Andrew Dunstan
In reply to this post by Andrew Dunstan

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



Reply | Threaded
Open this post in threaded view
|

Re: Allow matching whole DN from a client certificate

Andrew Dunstan

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


ssl-match-client-cert-dn-v3.patch (16K) Download Attachment
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:
> 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
> [...]
> +       <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.

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, &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
12