Re: patch: Client certificate requirements

On Thu, Oct 23, 2008 at 08:51, Magnus Hagander <magnus(at)hagander(dot)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)
{

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 not load 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 certificate file \"%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.