ldapbindpasswdfile
Hello hackers,
Some users don't like the fact that ldapbindpasswd can leak into logs
(and now system views?). Also, some users don't like the fact that it
is in cleartext rather than some encryption scheme (though I don't
know what, since we'd presumably also need the key). I propose a new
option $SUBJECT so that users can at least add a level of indirection
and put the password in a file. A motivated user could point it at an
encrypted loopback device so that they need a passphrase at mount
time, or a named pipe that performs arbitrary magic. Some of these
topics were discussed last time someone had this idea[1]/messages/by-id/20140617175511.2589.45249@wrigleys.postgresql.org.
Using a separate file for the bind password is fairly common in other
software: see the ldapsearch's -y switch, and I think it probably
makes sense at the very least as a convenience, without getting into
hand-wringing discussions about whether any security is truly added.
Draft patch attached.
Hi Stephen!
I also know that a motivated user could also use GSSAPI instead of
LDAP. Do you think we should update the manual to say so, perhaps in
a "tip" box on the LDAP auth page?
[1]: /messages/by-id/20140617175511.2589.45249@wrigleys.postgresql.org
--
Thomas Munro
https://enterprisedb.com
Attachments:
0001-Add-ldapbindpasswdfile-option-for-pg_hba.conf.patchapplication/octet-stream; name=0001-Add-ldapbindpasswdfile-option-for-pg_hba.conf.patchDownload
From d081a4f99d8b776fdebae601f5579a6e8d93be33 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@gmail.com>
Date: Tue, 14 May 2019 12:52:03 +1200
Subject: [PATCH] Add ldapbindpasswdfile option for pg_hba.conf.
Some users would prefer to keep the bind password for their LDAP server
in a separate file.
---
doc/src/sgml/client-auth.sgml | 13 +++++++
src/backend/libpq/auth.c | 69 ++++++++++++++++++++++++++++++++++-
src/backend/libpq/hba.c | 21 ++++++++---
src/include/libpq/hba.h | 1 +
src/test/ldap/t/001_auth.pl | 47 +++++++++++++++++++++++-
5 files changed, 144 insertions(+), 7 deletions(-)
diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index ffed887ab7..3820252d55 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -1525,6 +1525,9 @@ omicron bryanh guest1
performed over the subtree at <replaceable>ldapbasedn</replaceable>, and will try to
do an exact match of the attribute specified in
<replaceable>ldapsearchattribute</replaceable>.
+ As an alternative to <replaceable>ldapbindpasswd</replaceable>, the
+ password may be stored in a separate file specified with
+ <replaceable>ldapbindpasswdfile</replaceable>.
Once the user has been found in
this search, the server disconnects and re-binds to the directory as
this user, using the password specified by the client, to verify that the
@@ -1642,6 +1645,16 @@ omicron bryanh guest1
when doing search+bind authentication.
</para>
</listitem>
+ </varlistentry>
+ <varlistentry>
+ <term><literal>ldapbindpasswdfile</literal></term>
+ <listitem>
+ <para>
+ File containing the password for user to bind to the directory with to
+ perform the search when doing search+bind authentication, as an
+ alternative to <literal>ldapbindpasswd</literal>.
+ </para>
+ </listitem>
</varlistentry>
<varlistentry>
<term><literal>ldapsearchattribute</literal></term>
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 62466be702..3036cef59e 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -35,6 +35,7 @@
#include "miscadmin.h"
#include "port/pg_bswap.h"
#include "replication/walsender.h"
+#include "storage/fd.h"
#include "storage/ipc.h"
#include "utils/memutils.h"
#include "utils/timestamp.h"
@@ -2563,6 +2564,45 @@ FormatSearchFilter(const char *pattern, const char *user_name)
return output.data;
}
+/*
+ * Read the entire contents of the password file into output. Don't trim any
+ * trailing newline character, because that is the convention established by
+ * the ldapsearch -y commandline tool.
+ */
+static int
+read_ldapbindpasswdfile(const char *path, StringInfo output)
+{
+ int fd;
+ char buffer[80];
+ ssize_t size;
+
+ fd = OpenTransientFile(path, O_RDONLY);
+ if (fd < 0)
+ return -1;
+
+ for (;;)
+ {
+ size = read(fd, buffer, sizeof(buffer));
+ if (size < 0)
+ {
+ int save_errno = errno;
+
+ CloseTransientFile(fd);
+ errno = save_errno;
+
+ return -1;
+ }
+ else if (size == 0)
+ break;
+ appendBinaryStringInfo(output, buffer, size);
+ }
+
+ if (CloseTransientFile(fd) < 0)
+ return -1;
+
+ return 0;
+}
+
/*
* Perform LDAP authentication
*/
@@ -2638,6 +2678,8 @@ CheckLDAPAuth(Port *port)
char *dn;
char *c;
int count;
+ const char *bind_passwd;
+ StringInfoData bind_passwd_buffer = {0};
/*
* Disallow any characters that we would otherwise need to escape,
@@ -2664,10 +2706,35 @@ CheckLDAPAuth(Port *port)
/*
* Bind with a pre-defined username/password (if available) for
* searching. If none is specified, this turns into an anonymous bind.
+ * If the password was specified in a separate file, read it.
*/
+ if (port->hba->ldapbindpasswdfile)
+ {
+ initStringInfo(&bind_passwd_buffer);
+ if (read_ldapbindpasswdfile(port->hba->ldapbindpasswdfile,
+ &bind_passwd_buffer))
+ {
+ ereport(LOG,
+ (errmsg("could not read ldapbindpasswd from \"%s\": %m",
+ port->hba->ldapbindpasswdfile)));
+ ldap_unbind(ldap);
+ pfree(passwd);
+ return STATUS_ERROR;
+ }
+ bind_passwd = bind_passwd_buffer.data;
+ }
+ else if (port->hba->ldapbindpasswd)
+ bind_passwd = port->hba->ldapbindpasswd;
+ else
+ bind_passwd = "";
+
r = ldap_simple_bind_s(ldap,
port->hba->ldapbinddn ? port->hba->ldapbinddn : "",
- port->hba->ldapbindpasswd ? port->hba->ldapbindpasswd : "");
+ bind_passwd);
+
+ if (bind_passwd_buffer.data)
+ pfree(bind_passwd_buffer.data);
+
if (r != LDAP_SUCCESS)
{
ereport(LOG,
diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
index 37d5ad44a5..fd3945ba3a 100644
--- a/src/backend/libpq/hba.c
+++ b/src/backend/libpq/hba.c
@@ -1536,7 +1536,7 @@ parse_hba_line(TokenizedLine *tok_line, int elevel)
/*
* LDAP can operate in two modes: either with a direct bind, using
* ldapprefix and ldapsuffix, or using a search+bind, using
- * ldapbasedn, ldapbinddn, ldapbindpasswd and one of
+ * ldapbasedn, ldapbinddn, ldapbindpasswd/ldapbindpasswdfile and one of
* ldapsearchattribute or ldapsearchfilter. Disallow mixing these
* parameters.
*/
@@ -1545,15 +1545,16 @@ parse_hba_line(TokenizedLine *tok_line, int elevel)
if (parsedline->ldapbasedn ||
parsedline->ldapbinddn ||
parsedline->ldapbindpasswd ||
+ parsedline->ldapbindpasswdfile ||
parsedline->ldapsearchattribute ||
parsedline->ldapsearchfilter)
{
ereport(elevel,
(errcode(ERRCODE_CONFIG_FILE_ERROR),
- errmsg("cannot use ldapbasedn, ldapbinddn, ldapbindpasswd, ldapsearchattribute, ldapsearchfilter, or ldapurl together with ldapprefix"),
+ errmsg("cannot use ldapbasedn, ldapbinddn, ldapbindpasswd, ldapbindpasswdfile, ldapsearchattribute, ldapsearchfilter, or ldapurl together with ldapprefix"),
errcontext("line %d of configuration file \"%s\"",
line_num, HbaFileName)));
- *err_msg = "cannot use ldapbasedn, ldapbinddn, ldapbindpasswd, ldapsearchattribute, ldapsearchfilter, or ldapurl together with ldapprefix";
+ *err_msg = "cannot use ldapbasedn, ldapbinddn, ldapbindpasswd, ldapbindpasswdfile, ldapsearchattribute, ldapsearchfilter, or ldapurl together with ldapprefix";
return NULL;
}
}
@@ -1856,6 +1857,11 @@ parse_hba_auth_opt(char *name, char *val, HbaLine *hbaline,
REQUIRE_AUTH_OPTION(uaLDAP, "ldapbindpasswd", "ldap");
hbaline->ldapbindpasswd = pstrdup(val);
}
+ else if (strcmp(name, "ldapbindpasswdfile") == 0)
+ {
+ REQUIRE_AUTH_OPTION(uaLDAP, "ldapbindpasswdfile", "ldap");
+ hbaline->ldapbindpasswdfile = pstrdup(val);
+ }
else if (strcmp(name, "ldapsearchattribute") == 0)
{
REQUIRE_AUTH_OPTION(uaLDAP, "ldapsearchattribute", "ldap");
@@ -2275,12 +2281,12 @@ load_hba(void)
/*
* This macro specifies the maximum number of authentication options
* that are possible with any given authentication method that is supported.
- * Currently LDAP supports 11, and there are 3 that are not dependent on
+ * Currently LDAP supports 12, and there are 3 that are not dependent on
* the auth method here. It may not actually be possible to set all of them
* at the same time, but we'll set the macro value high enough to be
* conservative and avoid warnings from static analysis tools.
*/
-#define MAX_HBA_OPTIONS 14
+#define MAX_HBA_OPTIONS 15
/*
* Create a text array listing the options specified in the HBA line.
@@ -2352,6 +2358,11 @@ gethba_options(HbaLine *hba)
CStringGetTextDatum(psprintf("ldapbindpasswd=%s",
hba->ldapbindpasswd));
+ if (hba->ldapbindpasswdfile)
+ options[noptions++] =
+ CStringGetTextDatum(psprintf("ldapbindpasswdfile=%s",
+ hba->ldapbindpasswdfile));
+
if (hba->ldapsearchattribute)
options[noptions++] =
CStringGetTextDatum(psprintf("ldapsearchattribute=%s",
diff --git a/src/include/libpq/hba.h b/src/include/libpq/hba.h
index 186e433574..985fd98f11 100644
--- a/src/include/libpq/hba.h
+++ b/src/include/libpq/hba.h
@@ -89,6 +89,7 @@ typedef struct HbaLine
int ldapport;
char *ldapbinddn;
char *ldapbindpasswd;
+ char *ldapbindpasswdfile;
char *ldapsearchattribute;
char *ldapsearchfilter;
char *ldapbasedn;
diff --git a/src/test/ldap/t/001_auth.pl b/src/test/ldap/t/001_auth.pl
index 6c02f2530b..bf92ace060 100644
--- a/src/test/ldap/t/001_auth.pl
+++ b/src/test/ldap/t/001_auth.pl
@@ -6,7 +6,7 @@ use Test::More;
if ($ENV{with_ldap} eq 'yes')
{
- plan tests => 22;
+ plan tests => 27;
}
else
{
@@ -63,6 +63,7 @@ my $ldap_basedn = 'dc=example,dc=net';
my $ldap_rootdn = 'cn=Manager,dc=example,dc=net';
my $ldap_rootpw = 'secret';
my $ldap_pwfile = "${TestLib::tmp_check}/ldappassword";
+my $ldap_bpwfile = "${TestLib::tmp_check}/ldappassword.bad";
note "setting up slapd";
@@ -120,6 +121,8 @@ END
append_to_file($ldap_pwfile, $ldap_rootpw);
chmod 0600, $ldap_pwfile or die;
+append_to_file($ldap_bpwfile, "this-is-a-bad-password");
+
$ENV{'LDAPURI'} = $ldap_url;
$ENV{'LDAPBINDDN'} = $ldap_rootdn;
$ENV{'LDAPCONF'} = $ldap_conf;
@@ -310,3 +313,45 @@ $node->restart;
$ENV{"PGPASSWORD"} = 'secret1';
test_access($node, 'test1', 2, 'bad combination of LDAPS and StartTLS');
+
+note "Non-anonymous bind";
+
+# bad ldapbindpasswd fails (note "x" prepended to password)
+unlink($node->data_dir . '/pg_hba.conf');
+$node->append_conf('pg_hba.conf',
+ qq{local all all ldap ldapbinddn="$ldap_rootdn" ldapbindpasswd="x$ldap_rootpw" ldapurl="$ldap_url/$ldap_basedn?uid?sub"});
+$node->restart;
+
+test_access($node, 'test1', 2, 'bad ldapbindpasswd fails');
+
+# good ldapbindpasswd succeeds
+unlink($node->data_dir . '/pg_hba.conf');
+$node->append_conf('pg_hba.conf',
+ qq{local all all ldap ldapbinddn="$ldap_rootdn" ldapbindpasswd="$ldap_rootpw" ldapurl="$ldap_url/$ldap_basedn?uid?sub"});
+$node->restart;
+
+test_access($node, 'test1', 0, 'good ldapbindpasswd succeeds');
+
+# bad ldapbindpasswdfile path fails (note "x" prepended to path)
+unlink($node->data_dir . '/pg_hba.conf');
+$node->append_conf('pg_hba.conf',
+ qq{local all all ldap ldapbinddn="$ldap_rootdn" ldapbindpasswdfile="x$ldap_pwfile" ldapurl="$ldap_url/$ldap_basedn?uid?sub"});
+$node->restart;
+
+test_access($node, 'test1', 2, 'bad ldapbindpasswdpath path fails');
+
+# bad ldapbindpasswdfile contents fails
+unlink($node->data_dir . '/pg_hba.conf');
+$node->append_conf('pg_hba.conf',
+ qq{local all all ldap ldapbinddn="$ldap_rootdn" ldapbindpasswdfile="$ldap_bpwfile" ldapurl="$ldap_url/$ldap_basedn?uid?sub"});
+$node->restart;
+
+test_access($node, 'test1', 2, 'bad ldapbindpasswdfile contents fails');
+
+# good ldapbindpasswdfile succeeds
+unlink($node->data_dir . '/pg_hba.conf');
+$node->append_conf('pg_hba.conf',
+ qq{local all all ldap ldapbinddn="$ldap_rootdn" ldapbindpasswdfile="$ldap_pwfile" ldapurl="$ldap_url/$ldap_basedn?uid?sub"});
+$node->restart;
+
+test_access($node, 'test1', 0, 'good ldapbindpasswdfile succeeds');
--
2.21.0
On Tue, May 14, 2019 at 1:49 PM Thomas Munro <thomas.munro@gmail.com> wrote:
... or a named pipe that performs arbitrary magic.
(Erm, that bit might not make much sense...)
--
Thomas Munro
https://enterprisedb.com
On 14 May 2019, at 03:49, Thomas Munro <thomas.munro@gmail.com> wrote:
I propose a new option $SUBJECT so that users can at least add a level of
indirection and put the password in a file.
+1, seems like a reasonable option to give.
Draft patch attached.
I might be a bit thick, but this is somewhat hard to parse IMO:
+ File containing the password for user to bind to the directory with to
+ perform the search when doing search+bind authenticationTo add a little bit more security around this, does it make sense to check (on
unix filesystems) that the file isn’t world readable/editable?
+ fd = OpenTransientFile(path, O_RDONLY);
+ if (fd < 0)
+ return -1;cheers ./daniel
On Tue, May 14, 2019 at 1:24 PM Daniel Gustafsson <daniel@yesql.se> wrote:
On 14 May 2019, at 03:49, Thomas Munro <thomas.munro@gmail.com> wrote:
I propose a new option $SUBJECT so that users can at least add a level of
indirection and put the password in a file.+1, seems like a reasonable option to give.
Thanks for the review!
Draft patch attached.
I might be a bit thick, but this is somewhat hard to parse IMO:
+ File containing the password for user to bind to the directory with to + perform the search when doing search+bind authenticationTo add a little bit more security around this, does it make sense to check (on
unix filesystems) that the file isn’t world readable/editable?+ fd = OpenTransientFile(path, O_RDONLY); + if (fd < 0) + return -1;
Good point.
However, I realised that this patch is nearly but not quite redundant.
You can already write @somefile given a file somefile that contains
ldapbindpasswd=secret. It'd be a bit nicer if you could also write
ldapbindpasswd=@somefile to include just the value, and not have to
include the option name in the file. Then you could use the same
password file that you use for the ldapsearch command line tool, and
in general that seems nicer. That syntax might have backwards
compatibility problems though. You could probably resolve any
problems by requiring quote marks around @ signs that are not acting
as include directives, or something like that. If we do that, I'd
also like to be able to write ldapbindpasswd=$SOME_ENV_VAR.
Anyway, I hereby withdraw the earlier patch; it seems silly to do
per-option ad hoc read-from-file variants. Perhaps we can do
something much better and more general, or perhaps what we have is
enough already.
--
Thomas Munro
https://enterprisedb.com
Greetings,
* Thomas Munro (thomas.munro@gmail.com) wrote:
I also know that a motivated user could also use GSSAPI instead of
LDAP. Do you think we should update the manual to say so, perhaps in
a "tip" box on the LDAP auth page?
Hrm, not sure how I missed this before, but, yes, I'm all for adding a
'tip' box on the LDAP auth page which recommends use of GSSAPI when
available (such as when operating in an Active Directory
environment...). Note that, technically, you can run LDAP without using
Active Directory and without running any kind of KDC, so we can't just
blanket say "use GSSAPI" because there exists use-cases where that isn't
an option.
Not that I've ever actually *encountered* such an environment, but
people have assured me that they do, in fact, exist, and that there are
users of PG LDAP auth with such a setup who would be upset to see
support for it removed.
Anyhow, yes, a 'tip' would be great to add.
Thanks,
Stephen