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
"

Greetings,

* Andrew Dunstan (andrew(at)dunslane(dot)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

> On 11 Nov 2020, at 21:44, Andrew Dunstan <andrew(at)dunslane(dot)net> 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

On 11/12/20 8:37 AM, Daniel Gustafsson wrote:
>> On 11 Nov 2020, at 21:44, Andrew Dunstan <andrew(at)dunslane(dot)net> 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

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 <andrew(at)dunslane(dot)net> 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

> On 18 Nov 2020, at 19:01, Andrew Dunstan <andrew(at)dunslane(dot)net> 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

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

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

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

> On 20 Jan 2021, at 20:07, Jacob Champion <pchampion(at)vmware(dot)com> 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/

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

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

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

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

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

> On 28 Jan 2021, at 23:10, Andrew Dunstan <andrew(at)dunslane(dot)net> 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/

On 1/29/21 8:18 AM, Daniel Gustafsson wrote:
>> On 28 Jan 2021, at 23:10, Andrew Dunstan <andrew(at)dunslane(dot)net> 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

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

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

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

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

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

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

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

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

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

spell: conjunction

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

On Wed, Mar 3, 2021 at 3:03 AM Jacob Champion <pchampion(at)vmware(dot)com> 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

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

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

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

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

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

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

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

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

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

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

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

I think that you should keep this local declaration here.

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

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

> + BIO_get_mem_ptr(bio, &bio_buf);

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

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

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

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

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

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

Missing some indentation stuff here.

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

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

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

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

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

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

Thanks!

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

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

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

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

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

On Mon, Mar 29, 2021 at 10:57:00AM +0900, Michael Paquier wrote:
> + switch (port->hba->clientcertname)
> + {
> + case clientCertDN:
> + peer_username = port->peer_dn;
> + break;
> + default:
> + peer_username = port->peer_cn;
> + }
>
> This does not need a "default". I think that you should use "case
> clientCertCN" instead here.
>
> + BIO_get_mem_ptr(bio, &bio_buf);
> No status checks? OpenSSL calls return 1 on success and 0 on failure,
> so I would check after <= 0 here.
>
> ++ if (port->hba->clientcertname == clientCertDN)
> ++ {
> ++ ereport(LOG,
> May be better to use a switch() here as well.
>
> It looks like this patch misses src/test/ssl/ssl/client-dn.crt,
> causing the SSL tests to fail.

For the sake of the archives, this has been applied as of 6d7a6fe with
all those nits from me addressed.
--
Michael