Обсуждение: patch: Client certificate requirements
This patch adds a configuration option to pg_hba.conf for "clientcert".
This makes it possible to have different client certificate requirements
on different connections. It also makes sure that if you specify that
you want client cert verification and the root store isn't there, we
give an error instead of silently allowing the user in (like we do now).
This still does not implement actual client certificate validation -
that's for a later step. It just cleans up the handling we have now.
//Magnus
*** a/src/backend/libpq/auth.c
--- b/src/backend/libpq/auth.c
***************
*** 273,278 **** ClientAuthentication(Port *port)
--- 273,304 ----
errmsg("missing or erroneous pg_hba.conf file"),
errhint("See server log for details.")));
+ /*
+ * This is the first point where we have access to the hba record for
+ * the current connection, so perform any verifications based on the
+ * hba options field that should be done *before* the authentication
+ * here.
+ */
+ if (port->hba->clientcert)
+ {
+ /*
+ * When we parse pg_hba.conf, we have already made sure that we have
+ * been able to load a certificate store. Thus, if a certificate is
+ * present on the client, it has been verified against our root
+ * certificate store, and the connection would have been aborted
+ * already if it didn't verify ok.
+ */
+ if (!port->peer)
+ {
+ ereport(FATAL,
+ (errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
+ errmsg("connection requires a valid client certificate")));
+ }
+ }
+
+ /*
+ * Now proceed to do the actual authentication check
+ */
switch (port->hba->auth_method)
{
case uaReject:
*** a/src/backend/libpq/be-secure.c
--- b/src/backend/libpq/be-secure.c
***************
*** 128,133 **** static const char *SSLerrmessage(void);
--- 128,134 ----
#define RENEGOTIATION_LIMIT (512 * 1024 * 1024)
static SSL_CTX *SSL_context = NULL;
+ static bool ssl_loaded_verify_locations = false;
/* GUC variable controlling SSL cipher list */
char *SSLCipherSuites = NULL;
***************
*** 229,234 **** secure_destroy(void)
--- 230,248 ----
}
/*
+ * Indicate if we have loaded the root CA store to verify certificates
+ */
+ bool
+ secure_loaded_verify_locations(void)
+ {
+ #ifdef USE_SSL
+ return ssl_loaded_verify_locations;
+ #endif
+
+ return false;
+ }
+
+ /*
* Attempt to negotiate secure session.
*/
int
***************
*** 780,794 **** initialize_SSL(void)
elog(FATAL, "could not set the cipher list (no valid ciphers available)");
/*
! * Require and check client certificates only if we have a root.crt file.
*/
! if (!SSL_CTX_load_verify_locations(SSL_context, ROOT_CERT_FILE, NULL))
{
! /* Not fatal - we do not require client certificates */
ereport(LOG,
(errmsg("could not load root certificate file \"%s\": %s",
ROOT_CERT_FILE, SSLerrmessage()),
! errdetail("Will not verify client certificates.")));
}
else
{
--- 794,821 ----
elog(FATAL, "could not set the cipher list (no valid ciphers available)");
/*
! * Attempt to load CA store, so we can verify client certificates if needed.
*/
! if (access(ROOT_CERT_FILE, R_OK))
{
! /*
! * Root certificate file simply not found. Don't log an error here, because
! * it's quite likely the user isn't planning on using client certificates.
! */
! ssl_loaded_verify_locations = false;
! }
! else if (!SSL_CTX_load_verify_locations(SSL_context, ROOT_CERT_FILE, NULL))
! {
! /*
! * File was there, but we could not load it. This means the file is somehow
! * broken, and we should log this. Don't log it as a fatal error, because
! * there is still a chance that the user isn't going to use client certs.
! */
! ssl_loaded_verify_locations = false;
ereport(LOG,
(errmsg("could not load root certificate file \"%s\": %s",
ROOT_CERT_FILE, SSLerrmessage()),
! errdetail("Will not be able to verify client certificates.")));
}
else
{
***************
*** 821,833 **** initialize_SSL(void)
ROOT_CRL_FILE, SSLerrmessage()),
errdetail("Certificates will not be checked against revocation list.")));
}
- }
! SSL_CTX_set_verify(SSL_context,
! (SSL_VERIFY_PEER |
! SSL_VERIFY_FAIL_IF_NO_PEER_CERT |
! SSL_VERIFY_CLIENT_ONCE),
! verify_cb);
}
}
--- 848,865 ----
ROOT_CRL_FILE, SSLerrmessage()),
errdetail("Certificates will not be checked against revocation list.")));
}
! /*
! * Always ask for SSL client cert, but don't fail if it's not presented. We'll fail later in this case,
! * based on what we find in pg_hba.conf.
! */
! SSL_CTX_set_verify(SSL_context,
! (SSL_VERIFY_PEER |
! SSL_VERIFY_CLIENT_ONCE),
! verify_cb);
!
! ssl_loaded_verify_locations = true;
! }
}
}
*** a/src/backend/libpq/hba.c
--- b/src/backend/libpq/hba.c
***************
*** 873,878 **** parse_hba_line(List *line, int line_num, HbaLine *parsedline)
--- 873,910 ----
INVALID_AUTH_OPTION("map", "ident, krb5, gssapi and sspi");
parsedline->usermap = pstrdup(c);
}
+ else if (strcmp(token, "clientcert") == 0)
+ {
+ /*
+ * Since we require ctHostSSL, this really can never happen on non-SSL-enabled
+ * builds, so don't bother checking for USE_SSL.
+ */
+ if (parsedline->conntype != ctHostSSL)
+ {
+ ereport(LOG,
+ (errcode(ERRCODE_CONFIG_FILE_ERROR),
+ errmsg("clientcert can only be configured for \"hostssl\" rows"),
+ errcontext("line %d of configuration file \"%s\"",
+ line_num, HbaFileName)));
+ goto hba_other_error;
+ }
+ if (strcmp(c, "1") == 0)
+ {
+ if (!secure_loaded_verify_locations())
+ {
+ ereport(LOG,
+ (errcode(ERRCODE_CONFIG_FILE_ERROR),
+ errmsg("client certificates can only be checked if a root certificate store is
available"),
+ errdetail("make sure the root certificate store is present and readable"),
+ errcontext("line %d of configuration file \"%s\"",
+ line_num, HbaFileName)));
+ goto hba_other_error;
+ }
+ parsedline->clientcert = true;
+ }
+ else
+ parsedline->clientcert = false;
+ }
else if (strcmp(token, "pamservice") == 0)
{
REQUIRE_AUTH_OPTION(uaPAM, "pamservice", "pam");
*** a/src/include/libpq/hba.h
--- b/src/include/libpq/hba.h
***************
*** 55,60 **** typedef struct
--- 55,61 ----
int ldapport;
char *ldapprefix;
char *ldapsuffix;
+ bool clientcert;
} HbaLine;
typedef struct Port hbaPort;
*** a/src/include/libpq/libpq.h
--- b/src/include/libpq/libpq.h
***************
*** 67,72 **** extern void pq_endcopyout(bool errorAbort);
--- 67,73 ----
* prototypes for functions in be-secure.c
*/
extern int secure_initialize(void);
+ extern bool secure_loaded_verify_locations(void);
extern void secure_destroy(void);
extern int secure_open_server(Port *port);
extern void secure_close(Port *port);
Magnus Hagander wrote:
> This patch adds a configuration option to pg_hba.conf for "clientcert".
> This makes it possible to have different client certificate requirements
> on different connections. It also makes sure that if you specify that
> you want client cert verification and the root store isn't there, we
> give an error instead of silently allowing the user in (like we do now).
>
> This still does not implement actual client certificate validation -
> that's for a later step. It just cleans up the handling we have now.
Uh, with docs.
//Magnus
*** a/doc/src/sgml/runtime.sgml
--- b/doc/src/sgml/runtime.sgml
***************
*** 1646,1658 **** $ <userinput>kill -INT `head -1 /usr/local/pgsql/data/postmaster.pid`</userinput
been entered.
</para>
! <para>
To require the client to supply a trusted certificate, place
certificates of the certificate authorities (<acronym>CA</acronym>)
you trust in the file <filename>root.crt</filename> in the data
! directory. A certificate will then be requested from the client during
SSL connection startup. (See <xref linkend="libpq-ssl"> for a
! description of how to set up client certificates.) The server will
verify that the client's certificate is signed by one of the trusted
certificate authorities. Certificate Revocation List (CRL) entries
are also checked if the file <filename>root.crl</filename> exists.
--- 1646,1662 ----
been entered.
</para>
! <sect2 id="ssl-client-certificates">
! <title>Using client certificates</title>
! <para>
To require the client to supply a trusted certificate, place
certificates of the certificate authorities (<acronym>CA</acronym>)
you trust in the file <filename>root.crt</filename> in the data
! directory, and set the <literal>clientcert</literal> parameter
! to <literal>1</literal> on the appropriate line(s) in pg_hba.conf.
! A certificate will then be requested from the client during
SSL connection startup. (See <xref linkend="libpq-ssl"> for a
! description of how to set up certificates on the client.) The server will
verify that the client's certificate is signed by one of the trusted
certificate authorities. Certificate Revocation List (CRL) entries
are also checked if the file <filename>root.crl</filename> exists.
***************
*** 1663,1673 **** $ <userinput>kill -INT `head -1 /usr/local/pgsql/data/postmaster.pid`</userinput
</para>
<para>
! If the <filename>root.crt</filename> file is not present, client
! certificates will not be requested or checked. In this mode, SSL
! provides encrypted communication but not authentication.
</para>
<para>
The files <filename>server.key</>, <filename>server.crt</>,
<filename>root.crt</filename>, and <filename>root.crl</filename>
--- 1667,1689 ----
</para>
<para>
! The <literal>clientcert</literal> option in <filename>pg_hba.conf</>
! is available for all authentication methods, but only for rows
! specified as <literal>hostssl</>. Unless specified, the default is
! not to verify the client certificate.
! </para>
!
! <para>
! <productname>PostgreSQL</> currently does not support authentication
! using client certificates, since it cannot differentiate between
! different users. As long as the user holds any certificate issued
! by a trusted CA it will be accepted, regardless of what account the
! user is trying to connect with.
</para>
+ </sect2>
+ <sect2 id="ssl-server-files">
+ <title>SSL Server File Usage</title>
<para>
The files <filename>server.key</>, <filename>server.crt</>,
<filename>root.crt</filename>, and <filename>root.crl</filename>
***************
*** 1704,1710 **** $ <userinput>kill -INT `head -1 /usr/local/pgsql/data/postmaster.pid`</userinput
<row>
<entry><filename>root.crt</></entry>
<entry>trusted certificate authorities</entry>
! <entry>requests client certificate; checks certificate is
signed by a trusted certificate authority</entry>
</row>
--- 1720,1726 ----
<row>
<entry><filename>root.crt</></entry>
<entry>trusted certificate authorities</entry>
! <entry>checks that client certificate is
signed by a trusted certificate authority</entry>
</row>
***************
*** 1717,1722 **** $ <userinput>kill -INT `head -1 /usr/local/pgsql/data/postmaster.pid`</userinput
--- 1733,1739 ----
</tbody>
</tgroup>
</table>
+ </sect2>
<sect2 id="ssl-certificate-creation">
<title>Creating a Self-Signed Certificate</title>
*** a/src/backend/libpq/auth.c
--- b/src/backend/libpq/auth.c
***************
*** 273,278 **** ClientAuthentication(Port *port)
--- 273,304 ----
errmsg("missing or erroneous pg_hba.conf file"),
errhint("See server log for details.")));
+ /*
+ * This is the first point where we have access to the hba record for
+ * the current connection, so perform any verifications based on the
+ * hba options field that should be done *before* the authentication
+ * here.
+ */
+ if (port->hba->clientcert)
+ {
+ /*
+ * When we parse pg_hba.conf, we have already made sure that we have
+ * been able to load a certificate store. Thus, if a certificate is
+ * present on the client, it has been verified against our root
+ * certificate store, and the connection would have been aborted
+ * already if it didn't verify ok.
+ */
+ if (!port->peer)
+ {
+ ereport(FATAL,
+ (errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
+ errmsg("connection requires a valid client certificate")));
+ }
+ }
+
+ /*
+ * Now proceed to do the actual authentication check
+ */
switch (port->hba->auth_method)
{
case uaReject:
*** a/src/backend/libpq/be-secure.c
--- b/src/backend/libpq/be-secure.c
***************
*** 128,133 **** static const char *SSLerrmessage(void);
--- 128,134 ----
#define RENEGOTIATION_LIMIT (512 * 1024 * 1024)
static SSL_CTX *SSL_context = NULL;
+ static bool ssl_loaded_verify_locations = false;
/* GUC variable controlling SSL cipher list */
char *SSLCipherSuites = NULL;
***************
*** 229,234 **** secure_destroy(void)
--- 230,248 ----
}
/*
+ * Indicate if we have loaded the root CA store to verify certificates
+ */
+ bool
+ secure_loaded_verify_locations(void)
+ {
+ #ifdef USE_SSL
+ return ssl_loaded_verify_locations;
+ #endif
+
+ return false;
+ }
+
+ /*
* Attempt to negotiate secure session.
*/
int
***************
*** 780,794 **** initialize_SSL(void)
elog(FATAL, "could not set the cipher list (no valid ciphers available)");
/*
! * Require and check client certificates only if we have a root.crt file.
*/
! if (!SSL_CTX_load_verify_locations(SSL_context, ROOT_CERT_FILE, NULL))
{
! /* Not fatal - we do not require client certificates */
ereport(LOG,
(errmsg("could not load root certificate file \"%s\": %s",
ROOT_CERT_FILE, SSLerrmessage()),
! errdetail("Will not verify client certificates.")));
}
else
{
--- 794,821 ----
elog(FATAL, "could not set the cipher list (no valid ciphers available)");
/*
! * Attempt to load CA store, so we can verify client certificates if needed.
*/
! if (access(ROOT_CERT_FILE, R_OK))
{
! /*
! * Root certificate file simply not found. Don't log an error here, because
! * it's quite likely the user isn't planning on using client certificates.
! */
! ssl_loaded_verify_locations = false;
! }
! else if (!SSL_CTX_load_verify_locations(SSL_context, ROOT_CERT_FILE, NULL))
! {
! /*
! * File was there, but we could not load it. This means the file is somehow
! * broken, and we should log this. Don't log it as a fatal error, because
! * there is still a chance that the user isn't going to use client certs.
! */
! ssl_loaded_verify_locations = false;
ereport(LOG,
(errmsg("could not load root certificate file \"%s\": %s",
ROOT_CERT_FILE, SSLerrmessage()),
! errdetail("Will not be able to verify client certificates.")));
}
else
{
***************
*** 821,833 **** initialize_SSL(void)
ROOT_CRL_FILE, SSLerrmessage()),
errdetail("Certificates will not be checked against revocation list.")));
}
- }
! SSL_CTX_set_verify(SSL_context,
! (SSL_VERIFY_PEER |
! SSL_VERIFY_FAIL_IF_NO_PEER_CERT |
! SSL_VERIFY_CLIENT_ONCE),
! verify_cb);
}
}
--- 848,865 ----
ROOT_CRL_FILE, SSLerrmessage()),
errdetail("Certificates will not be checked against revocation list.")));
}
! /*
! * Always ask for SSL client cert, but don't fail if it's not presented. We'll fail later in this case,
! * based on what we find in pg_hba.conf.
! */
! SSL_CTX_set_verify(SSL_context,
! (SSL_VERIFY_PEER |
! SSL_VERIFY_CLIENT_ONCE),
! verify_cb);
!
! ssl_loaded_verify_locations = true;
! }
}
}
*** a/src/backend/libpq/hba.c
--- b/src/backend/libpq/hba.c
***************
*** 873,878 **** parse_hba_line(List *line, int line_num, HbaLine *parsedline)
--- 873,910 ----
INVALID_AUTH_OPTION("map", "ident, krb5, gssapi and sspi");
parsedline->usermap = pstrdup(c);
}
+ else if (strcmp(token, "clientcert") == 0)
+ {
+ /*
+ * Since we require ctHostSSL, this really can never happen on non-SSL-enabled
+ * builds, so don't bother checking for USE_SSL.
+ */
+ if (parsedline->conntype != ctHostSSL)
+ {
+ ereport(LOG,
+ (errcode(ERRCODE_CONFIG_FILE_ERROR),
+ errmsg("clientcert can only be configured for \"hostssl\" rows"),
+ errcontext("line %d of configuration file \"%s\"",
+ line_num, HbaFileName)));
+ goto hba_other_error;
+ }
+ if (strcmp(c, "1") == 0)
+ {
+ if (!secure_loaded_verify_locations())
+ {
+ ereport(LOG,
+ (errcode(ERRCODE_CONFIG_FILE_ERROR),
+ errmsg("client certificates can only be checked if a root certificate store is
available"),
+ errdetail("make sure the root certificate store is present and readable"),
+ errcontext("line %d of configuration file \"%s\"",
+ line_num, HbaFileName)));
+ goto hba_other_error;
+ }
+ parsedline->clientcert = true;
+ }
+ else
+ parsedline->clientcert = false;
+ }
else if (strcmp(token, "pamservice") == 0)
{
REQUIRE_AUTH_OPTION(uaPAM, "pamservice", "pam");
*** a/src/include/libpq/hba.h
--- b/src/include/libpq/hba.h
***************
*** 55,60 **** typedef struct
--- 55,61 ----
int ldapport;
char *ldapprefix;
char *ldapsuffix;
+ bool clientcert;
} HbaLine;
typedef struct Port hbaPort;
*** a/src/include/libpq/libpq.h
--- b/src/include/libpq/libpq.h
***************
*** 67,72 **** extern void pq_endcopyout(bool errorAbort);
--- 67,73 ----
* prototypes for functions in be-secure.c
*/
extern int secure_initialize(void);
+ extern bool secure_loaded_verify_locations(void);
extern void secure_destroy(void);
extern int secure_open_server(Port *port);
extern void secure_close(Port *port);
On Thu, Oct 23, 2008 at 08:51, Magnus Hagander <magnus@hagander.net> wrote:
> Magnus Hagander wrote:
>> This patch adds a configuration option to pg_hba.conf for "clientcert".
>> This makes it possible to have different client certificate requirements
>> on different connections. It also makes sure that if you specify that
>> you want client cert verification and the root store isn't there, we
>> give an error instead of silently allowing the user in (like we do now).
>>
>> This still does not implement actual client certificate validation -
>> that's for a later step. It just cleans up the handling we have now.
>
> Uh, with docs.
>
> //Magnus
Hi in getting ready to view the other clientcert patch, I thought I
should give this a quick look over.
this hunk will break non ssl builds (due to port->peer):
*** a/src/backend/libpq/auth.c
--- b/src/backend/libpq/auth.c
***************
*** 272,277 **** ClientAuthentication(Port *port)
--- 272,303 ---- errmsg("missing or erroneous pg_hba.conf file"), errhint("See server
logfor details.")));
+ /*
+ * This is the first point where we have access to the hba record for
+ * the current connection, so perform any verifications based on the
+ * hba options field that should be done *before* the authentication
+ * here.
+ */
+ if (port->hba->clientcert)
+ {
+ /*
+ * When we parse pg_hba.conf, we have already made sure that we have
+ * been able to load a certificate store. Thus, if a certificate is
+ * present on the client, it has been verified against our root
+ * certificate store, and the connection would have been aborted
+ * already if it didn't verify ok.
+ */
+ if (!port->peer)
+ {
+ ereport(FATAL,
+ (errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
+ errmsg("connection requires a valid client certificate")));
+ }
+ }
+
+ /*
+ * Now proceed to do the actual authentication check
+ */ switch (port->hba->auth_method) {
and how about instead of:
***************
*** 754,768 **** initialize_SSL(void) elog(FATAL, "could not set the cipher list (no valid ciphers
available)");
/*
! * Require and check client certificates only if we have a root.crt file. */
! if (!SSL_CTX_load_verify_locations(SSL_context, ROOT_CERT_FILE, NULL)) {
! /* Not fatal - we do not require client certificates */ ereport(LOG, (errmsg("could
notload root certificate file \"%s\": %s", ROOT_CERT_FILE, SSLerrmessage()),
! errdetail("Will not verify client certificates."))); } else {
--- 768,795 ---- elog(FATAL, "could not set the cipher list (no valid ciphers available)");
/*
! * Attempt to load CA store, so we can verify client certificates if needed. */
! if (access(ROOT_CERT_FILE, R_OK)) {
! /*
! * Root certificate file simply not found. Don't log an error here, because
! * it's quite likely the user isn't planning on using client certificates.
! */
! ssl_loaded_verify_locations = false;
! }
! else if (!SSL_CTX_load_verify_locations(SSL_context, ROOT_CERT_FILE, NULL))
! {
! /*
! * File was there, but we could not load it. This means the file is somehow
! * broken, and we should log this. Don't log it as a fatal error, because
! * there is still a chance that the user isn't going to use client certs.
! */
! ssl_loaded_verify_locations = false; ereport(LOG, (errmsg("could not load root
certificatefile \"%s\": %s", ROOT_CERT_FILE, SSLerrmessage()),
! errdetail("Will not be able to verify client certificates."))); } else {
***************
we do something like:
+ if (access(ROOT_CERT_FILE, R_OK))
+ {
+ ssl_loaded_verify_locations = false;
+
+ /*
+ * If root certificate file simply not found. Don't log
an error here, because
+ * it's quite likely the user isn't planning on using
client certificates.
+ *
+ * Anything else gets logged (permission errors etc)
+ */
+ if (errno != ENOENT)
+ ereport(LOG,
+ (errmsg("could not load root
certificate file \"%s\": %s",
+ ROOT_CERT_FILE,
strerror(errno)),
+ errdetail("Will not be able to verify
client certificates.")));
+ }
+ else if (!SSL_CTX_load_verify_locations(SSL_context,
ROOT_CERT_FILE, NULL))
??
Other than that it looks good.
On Sat, Nov 15, 2008 at 15:20, Alex Hunsaker <badalex@gmail.com> wrote:
> we do something like:
>
> + if (access(ROOT_CERT_FILE, R_OK))
> + {
> + ssl_loaded_verify_locations = false;
> +
> + /*
> + * If root certificate file simply not found. Don't log
> an error here, because
> + * it's quite likely the user isn't planning on using
> client certificates.
> + *
> + * Anything else gets logged (permission errors etc)
> + */
> + if (errno != ENOENT)
> + ereport(LOG,
> + (errmsg("could not load root
> certificate file \"%s\": %s",
> + ROOT_CERT_FILE,
> strerror(errno)),
> + errdetail("Will not be able to verify
> client certificates.")));
Err that really should be ereport(FATAL,
"Alex Hunsaker" <badalex@gmail.com> writes:
> Err that really should be ereport(FATAL,
I don't think that's a particularly user-friendly design.
The behavior I'd expect to see is
1. Root cert file not there: issue existing LOG message. Maybe the user is
expecting client cert verification, and maybe he isn't, but it is a good
idea to put out the LOG message just to make sure he knows what will
happen.
2. Root cert file present but we fail to load it: FATAL is probably okay
here, but not with that hint message.
regards, tom lane
On Sat, Nov 15, 2008 at 17:39, Tom Lane <tgl@sss.pgh.pa.us> wrote: > "Alex Hunsaker" <badalex@gmail.com> writes: >> Err that really should be ereport(FATAL, > > I don't think that's a particularly user-friendly design. > > The behavior I'd expect to see is > > 1. Root cert file not there: issue existing LOG message. Maybe the user is > expecting client cert verification, and maybe he isn't, but it is a good > idea to put out the LOG message just to make sure he knows what will > happen. Right, pre patch I agree with you. The problem I noticed with the patch is we say nothing if its there and you can't read it. The whole point of the patch is to give you the option of forcing client certs. So why LOG every server start up just because I turned on ssl. If pg_hba is setup right you'll get the appropriate error message when the client connects. Maybe NOTICE is more appropriate? I don't really care either way though. Ill let you and Mangus duke it out : > 2. Root cert file present but we fail to load it: FATAL is probably okay > here, but not with that hint message. Err, I was just trying to be congruent with HEAD. Currently that's the message you get if we could not "read" the root cert. (as a LOG, not FATAL). Should just drop the hint and keep the FATAL for this case? Also we check that the private key is at least 0600, should we be doing the same for the root cert?
Alex Hunsaker wrote:
> On Thu, Oct 23, 2008 at 08:51, Magnus Hagander <magnus@hagander.net> wrote:
>> Magnus Hagander wrote:
>>> This patch adds a configuration option to pg_hba.conf for "clientcert".
>>> This makes it possible to have different client certificate requirements
>>> on different connections. It also makes sure that if you specify that
>>> you want client cert verification and the root store isn't there, we
>>> give an error instead of silently allowing the user in (like we do now).
>>>
>>> This still does not implement actual client certificate validation -
>>> that's for a later step. It just cleans up the handling we have now.
>> Uh, with docs.
>>
>> //Magnus
>
> Hi in getting ready to view the other clientcert patch, I thought I
> should give this a quick look over.
>
> this hunk will break non ssl builds (due to port->peer):
>
> *** a/src/backend/libpq/auth.c
> --- b/src/backend/libpq/auth.c
> ***************
> *** 272,277 **** ClientAuthentication(Port *port)
> --- 272,303 ----
> errmsg("missing or erroneous pg_hba.conf file"),
> errhint("See server log for details.")));
>
> + /*
> + * This is the first point where we have access to the hba record for
> + * the current connection, so perform any verifications based on the
> + * hba options field that should be done *before* the authentication
> + * here.
> + */
> + if (port->hba->clientcert)
> + {
> + /*
> + * When we parse pg_hba.conf, we have already made sure that we have
> + * been able to load a certificate store. Thus, if a certificate is
> + * present on the client, it has been verified against our root
> + * certificate store, and the connection would have been aborted
> + * already if it didn't verify ok.
> + */
> + if (!port->peer)
> + {
> + ereport(FATAL,
> + (errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
> + errmsg("connection requires a valid client certificate")));
> + }
> + }
> +
> + /*
> + * Now proceed to do the actual authentication check
> + */
> switch (port->hba->auth_method)
> {
Good point, thanks!
Added #ifdef USE_SSL around it.
(will address the other part of your response in a separate mail)
//Magnus
Alex Hunsaker wrote:
> On Sat, Nov 15, 2008 at 17:39, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> "Alex Hunsaker" <badalex@gmail.com> writes:
>>> Err that really should be ereport(FATAL,
>> I don't think that's a particularly user-friendly design.
>>
>> The behavior I'd expect to see is
>>
>> 1. Root cert file not there: issue existing LOG message. Maybe the user is
>> expecting client cert verification, and maybe he isn't, but it is a good
>> idea to put out the LOG message just to make sure he knows what will
>> happen.
>
> Right, pre patch I agree with you. The problem I noticed with the
> patch is we say nothing if its there and you can't read it. The whole
> point of the patch is to give you the option of forcing client certs.
> So why LOG every server start up just because I turned on ssl. If
> pg_hba is setup right you'll get the appropriate error message when
> the client connects.
Yeah, I agree with Alex on this. It makes sense not to log it in that case.
If it's there and we fail, we must log the details, but if it's just not
there, it can be logged at a later stage.
>> 2. Root cert file present but we fail to load it: FATAL is probably okay
>> here, but not with that hint message.
>
> Err, I was just trying to be congruent with HEAD. Currently that's
> the message you get if we could not "read" the root cert. (as a LOG,
> not FATAL). Should just drop the hint and keep the FATAL for this
> case?
Yes, I think so.
New version of the patch attached.
> Also we check that the private key is at least 0600, should we be
> doing the same for the root cert?
No need. The certificate is public information. The first thing we do on
an SSL connection is to send the thing to the client anyway.
We *could* check that it's not writable by anybody else - but do we
check that for our datafiles which contain the actual passwords and
such? If not, that would just be strange to do here, really..
//Magnus
*** a/doc/src/sgml/runtime.sgml
--- b/doc/src/sgml/runtime.sgml
***************
*** 1646,1658 **** $ <userinput>kill -INT `head -1 /usr/local/pgsql/data/postmaster.pid`</userinput
been entered.
</para>
! <para>
To require the client to supply a trusted certificate, place
certificates of the certificate authorities (<acronym>CA</acronym>)
you trust in the file <filename>root.crt</filename> in the data
! directory. A certificate will then be requested from the client during
SSL connection startup. (See <xref linkend="libpq-ssl"> for a
! description of how to set up client certificates.) The server will
verify that the client's certificate is signed by one of the trusted
certificate authorities. Certificate Revocation List (CRL) entries
are also checked if the file <filename>root.crl</filename> exists.
--- 1646,1662 ----
been entered.
</para>
! <sect2 id="ssl-client-certificates">
! <title>Using client certificates</title>
! <para>
To require the client to supply a trusted certificate, place
certificates of the certificate authorities (<acronym>CA</acronym>)
you trust in the file <filename>root.crt</filename> in the data
! directory, and set the <literal>clientcert</literal> parameter
! to <literal>1</literal> on the appropriate line(s) in pg_hba.conf.
! A certificate will then be requested from the client during
SSL connection startup. (See <xref linkend="libpq-ssl"> for a
! description of how to set up certificates on the client.) The server will
verify that the client's certificate is signed by one of the trusted
certificate authorities. Certificate Revocation List (CRL) entries
are also checked if the file <filename>root.crl</filename> exists.
***************
*** 1663,1673 **** $ <userinput>kill -INT `head -1 /usr/local/pgsql/data/postmaster.pid`</userinput
</para>
<para>
! If the <filename>root.crt</filename> file is not present, client
! certificates will not be requested or checked. In this mode, SSL
! provides encrypted communication but not authentication.
</para>
<para>
The files <filename>server.key</>, <filename>server.crt</>,
<filename>root.crt</filename>, and <filename>root.crl</filename>
--- 1667,1689 ----
</para>
<para>
! The <literal>clientcert</literal> option in <filename>pg_hba.conf</>
! is available for all authentication methods, but only for rows
! specified as <literal>hostssl</>. Unless specified, the default is
! not to verify the client certificate.
! </para>
!
! <para>
! <productname>PostgreSQL</> currently does not support authentication
! using client certificates, since it cannot differentiate between
! different users. As long as the user holds any certificate issued
! by a trusted CA it will be accepted, regardless of what account the
! user is trying to connect with.
</para>
+ </sect2>
+ <sect2 id="ssl-server-files">
+ <title>SSL Server File Usage</title>
<para>
The files <filename>server.key</>, <filename>server.crt</>,
<filename>root.crt</filename>, and <filename>root.crl</filename>
***************
*** 1704,1710 **** $ <userinput>kill -INT `head -1 /usr/local/pgsql/data/postmaster.pid`</userinput
<row>
<entry><filename>root.crt</></entry>
<entry>trusted certificate authorities</entry>
! <entry>requests client certificate; checks certificate is
signed by a trusted certificate authority</entry>
</row>
--- 1720,1726 ----
<row>
<entry><filename>root.crt</></entry>
<entry>trusted certificate authorities</entry>
! <entry>checks that client certificate is
signed by a trusted certificate authority</entry>
</row>
***************
*** 1717,1722 **** $ <userinput>kill -INT `head -1 /usr/local/pgsql/data/postmaster.pid`</userinput
--- 1733,1739 ----
</tbody>
</tgroup>
</table>
+ </sect2>
<sect2 id="ssl-certificate-creation">
<title>Creating a Self-Signed Certificate</title>
*** a/src/backend/libpq/auth.c
--- b/src/backend/libpq/auth.c
***************
*** 272,277 **** ClientAuthentication(Port *port)
--- 272,311 ----
errmsg("missing or erroneous pg_hba.conf file"),
errhint("See server log for details.")));
+ /*
+ * This is the first point where we have access to the hba record for
+ * the current connection, so perform any verifications based on the
+ * hba options field that should be done *before* the authentication
+ * here.
+ */
+ if (port->hba->clientcert)
+ {
+ /*
+ * When we parse pg_hba.conf, we have already made sure that we have
+ * been able to load a certificate store. Thus, if a certificate is
+ * present on the client, it has been verified against our root
+ * certificate store, and the connection would have been aborted
+ * already if it didn't verify ok.
+ */
+ #ifdef USE_SSL
+ if (!port->peer)
+ {
+ ereport(FATAL,
+ (errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
+ errmsg("connection requires a valid client certificate")));
+ }
+ #else
+ /*
+ * hba.c makes sure hba->clientcert can't be set unless OpenSSL
+ * is present.
+ */
+ Assert(false);
+ #endif
+ }
+
+ /*
+ * Now proceed to do the actual authentication check
+ */
switch (port->hba->auth_method)
{
case uaReject:
*** a/src/backend/libpq/be-secure.c
--- b/src/backend/libpq/be-secure.c
***************
*** 102,107 **** static const char *SSLerrmessage(void);
--- 102,108 ----
#define RENEGOTIATION_LIMIT (512 * 1024 * 1024)
static SSL_CTX *SSL_context = NULL;
+ static bool ssl_loaded_verify_locations = false;
/* GUC variable controlling SSL cipher list */
char *SSLCipherSuites = NULL;
***************
*** 203,208 **** secure_destroy(void)
--- 204,222 ----
}
/*
+ * Indicate if we have loaded the root CA store to verify certificates
+ */
+ bool
+ secure_loaded_verify_locations(void)
+ {
+ #ifdef USE_SSL
+ return ssl_loaded_verify_locations;
+ #endif
+
+ return false;
+ }
+
+ /*
* Attempt to negotiate secure session.
*/
int
***************
*** 754,768 **** initialize_SSL(void)
elog(FATAL, "could not set the cipher list (no valid ciphers available)");
/*
! * Require and check client certificates only if we have a root.crt file.
*/
! if (!SSL_CTX_load_verify_locations(SSL_context, ROOT_CERT_FILE, NULL))
{
! /* Not fatal - we do not require client certificates */
! ereport(LOG,
(errmsg("could not load root certificate file \"%s\": %s",
! ROOT_CERT_FILE, SSLerrmessage()),
! errdetail("Will not verify client certificates.")));
}
else
{
--- 768,801 ----
elog(FATAL, "could not set the cipher list (no valid ciphers available)");
/*
! * Attempt to load CA store, so we can verify client certificates if needed.
*/
! if (access(ROOT_CERT_FILE, R_OK))
{
! ssl_loaded_verify_locations = false;
!
! /*
! * If root certificate file simply not found. Don't log an error here, because
! * it's quite likely the user isn't planning on using client certificates.
! * If we can't access it for other reasons, it is an error.
! */
! if (errno != ENOENT)
! {
! ereport(FATAL,
! (errmsg("could not access root certificate file \"%s\": %m",
! ROOT_CERT_FILE)));
! }
! }
! else if (!SSL_CTX_load_verify_locations(SSL_context, ROOT_CERT_FILE, NULL))
! {
! /*
! * File was there, but we could not load it. This means the file is somehow
! * broken, and we cannot do verification at all - so abort here.
! */
! ssl_loaded_verify_locations = false;
! ereport(FATAL,
(errmsg("could not load root certificate file \"%s\": %s",
! ROOT_CERT_FILE, SSLerrmessage())));
}
else
{
***************
*** 795,807 **** initialize_SSL(void)
ROOT_CRL_FILE, SSLerrmessage()),
errdetail("Certificates will not be checked against revocation list.")));
}
- }
! SSL_CTX_set_verify(SSL_context,
! (SSL_VERIFY_PEER |
! SSL_VERIFY_FAIL_IF_NO_PEER_CERT |
! SSL_VERIFY_CLIENT_ONCE),
! verify_cb);
}
}
--- 828,845 ----
ROOT_CRL_FILE, SSLerrmessage()),
errdetail("Certificates will not be checked against revocation list.")));
}
! /*
! * Always ask for SSL client cert, but don't fail if it's not presented. We'll fail later in this case,
! * based on what we find in pg_hba.conf.
! */
! SSL_CTX_set_verify(SSL_context,
! (SSL_VERIFY_PEER |
! SSL_VERIFY_CLIENT_ONCE),
! verify_cb);
!
! ssl_loaded_verify_locations = true;
! }
}
}
*** a/src/backend/libpq/hba.c
--- b/src/backend/libpq/hba.c
***************
*** 927,932 **** parse_hba_line(List *line, int line_num, HbaLine *parsedline)
--- 927,964 ----
INVALID_AUTH_OPTION("map", "ident, krb5, gssapi and sspi");
parsedline->usermap = pstrdup(c);
}
+ else if (strcmp(token, "clientcert") == 0)
+ {
+ /*
+ * Since we require ctHostSSL, this really can never happen on non-SSL-enabled
+ * builds, so don't bother checking for USE_SSL.
+ */
+ if (parsedline->conntype != ctHostSSL)
+ {
+ ereport(LOG,
+ (errcode(ERRCODE_CONFIG_FILE_ERROR),
+ errmsg("clientcert can only be configured for \"hostssl\" rows"),
+ errcontext("line %d of configuration file \"%s\"",
+ line_num, HbaFileName)));
+ return false;
+ }
+ if (strcmp(c, "1") == 0)
+ {
+ if (!secure_loaded_verify_locations())
+ {
+ ereport(LOG,
+ (errcode(ERRCODE_CONFIG_FILE_ERROR),
+ errmsg("client certificates can only be checked if a root certificate store is
available"),
+ errdetail("make sure the root certificate store is present and readable"),
+ errcontext("line %d of configuration file \"%s\"",
+ line_num, HbaFileName)));
+ return false;
+ }
+ parsedline->clientcert = true;
+ }
+ else
+ parsedline->clientcert = false;
+ }
else if (strcmp(token, "pamservice") == 0)
{
REQUIRE_AUTH_OPTION(uaPAM, "pamservice", "pam");
*** a/src/include/libpq/hba.h
--- b/src/include/libpq/hba.h
***************
*** 54,59 **** typedef struct
--- 54,60 ----
int ldapport;
char *ldapprefix;
char *ldapsuffix;
+ bool clientcert;
} HbaLine;
typedef struct Port hbaPort;
*** a/src/include/libpq/libpq.h
--- b/src/include/libpq/libpq.h
***************
*** 67,72 **** extern void pq_endcopyout(bool errorAbort);
--- 67,73 ----
* prototypes for functions in be-secure.c
*/
extern int secure_initialize(void);
+ extern bool secure_loaded_verify_locations(void);
extern void secure_destroy(void);
extern int secure_open_server(Port *port);
extern void secure_close(Port *port);
On Mon, Nov 17, 2008 at 03:04, Magnus Hagander <magnus@hagander.net> wrote: > Alex Hunsaker wrote: >> On Sat, Nov 15, 2008 at 17:39, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> 2. Root cert file present but we fail to load it: FATAL is probably okay >>> here, but not with that hint message. >> >> Err, I was just trying to be congruent with HEAD. Currently that's >> the message you get if we could not "read" the root cert. (as a LOG, >> not FATAL). Should just drop the hint and keep the FATAL for this >> case? > > Yes, I think so. > > New version of the patch attached. Looks good to me. >> Also we check that the private key is at least 0600, should we be >> doing the same for the root cert? > > No need. The certificate is public information. The first thing we do on > an SSL connection is to send the thing to the client anyway. > > We *could* check that it's not writable by anybody else - but do we > check that for our datafiles which contain the actual passwords and > such? If not, that would just be strange to do here, really.. Makes sense. > //Magnus