Re: patch: Client certificate requirements

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.