[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Review requested on issue #2410 (SSL client certs option)

From: Karl Fogel <kfogel_at_red-bean.com>
Date: Thu, 26 Jun 2008 12:27:32 -0400

Issue #2410 is about allowing the client to avoid SSL certificate
prompts, by setting a config value that says "don't do client certs".

The original patch was way out of date. I attached an updated patch to
the issue (see below). It needs review and has at least one outstanding
question (detailed in the log message below). If anyone has time to
look at this, that'd be great.

[[[
    #################################################################
    ### ###
    ### NOT READY TO BE COMMITTED YET. Search for the comment ###
    ### beginning "### RFC" in the diff for outstanding issues. ###
    ### ###
    #################################################################

Add a configuration setting to allow the user to tell Subversion that
they don't have any SSL client certificates. This option can be set
globally or in a [server] block.

This stops subversion from asking repeatedly for a certificate if the
server implies that client certificates are an acceptable form of
authentication. The default is 'yes', so applying this patch does not
change the behaviour of Subversion unless the user chooses to.

Patch by: David Reid <david_at_jetnet.co.uk>
          kfogel
(Heavily reworked, as the issue #2410 patch was against Subversion 1.2.)

* subversion/include/svn_config.h
  (SVN_CONFIG_OPTION_SSL_USE_CLIENT_CERTS): New config option.

* subversion/libsvn_subr/config_file.c
  (svn_config_ensure): Document the new option.

Handle the new option in ra_neon:

* subversion/libsvn_ra_neon/session.c
  (get_server_settings): Take new parameter use_client_certs, use it
    to return a boolean value (by reference) from global or
    server-specific settings as applicable.
  (svn_ra_neon__open): Don't use client certs if config says not to.
    But, leave a comment asking some important questions about what
    kind of client certs fall into the protected category.

Handle it in ra_serf:

* subversion/libsvn_ra_serf/ra_serf.h
  (svn_ra_serf__conn_setup): New boolean field use_client_certs.

* subversion/libsvn_ra_serf/serf.c
  (svn_ra_serf__open): Initialize serf_sess->use_client_certs.
  (load_config): Get the use_client_certs setting from config.

* subversion/libsvn_ra_serf/util.c
  (svn_ra_serf__conn_setup, svn_ra_serf__setup_serf_req): Don't use
    client certs if config says not to.
]]]

Index: subversion/include/svn_config.h
===================================================================
--- subversion/include/svn_config.h (revision 31887)
+++ subversion/include/svn_config.h (working copy)
@@ -70,6 +70,7 @@
 #define SVN_CONFIG_OPTION_HTTP_AUTH_TYPES "http-auth-types"
 #define SVN_CONFIG_OPTION_SSL_AUTHORITY_FILES "ssl-authority-files"
 #define SVN_CONFIG_OPTION_SSL_TRUST_DEFAULT_CA "ssl-trust-default-ca"
+#define SVN_CONFIG_OPTION_SSL_USE_CLIENT_CERTS "ssl-use-client-certs"
 #define SVN_CONFIG_OPTION_SSL_CLIENT_CERT_FILE "ssl-client-cert-file"
 #define SVN_CONFIG_OPTION_SSL_CLIENT_CERT_PASSWORD "ssl-client-cert-password"
 #define SVN_CONFIG_OPTION_SSL_PKCS11_PROVIDER "ssl-pkcs11-provider"
Index: subversion/libsvn_subr/config_file.c
===================================================================
--- subversion/libsvn_subr/config_file.c (revision 31887)
+++ subversion/libsvn_subr/config_file.c (working copy)
@@ -744,6 +744,8 @@
         "### ssl-authority-files List of files, each of a trusted CAs"
                                                                              NL
         "### ssl-trust-default-ca Trust the system 'default' CAs" NL
+ "### ssl-use-client-certs Whether or not to use client " NL
+ "### certificates (default: yes)." NL
         "### ssl-client-cert-file PKCS#12 format client certificate file"
                                                                              NL
         "### ssl-client-cert-password Client Key password, if needed." NL
Index: subversion/libsvn_ra_neon/session.c
===================================================================
--- subversion/libsvn_ra_neon/session.c (revision 31887)
+++ subversion/libsvn_ra_neon/session.c (working copy)
@@ -417,13 +417,17 @@
 
 /* Set *PROXY_HOST, *PROXY_PORT, *PROXY_USERNAME, *PROXY_PASSWORD,
  * *TIMEOUT_SECONDS, *NEON_DEBUG, *COMPRESSION, *NEON_AUTH_TYPES, and
- * *PK11_PROVIDER to the information for REQUESTED_HOST, allocated in
- * POOL, if there is any applicable information. If there is no
- * applicable information or if there is an error, then set
- * *PROXY_PORT to (unsigned int) -1, *TIMEOUT_SECONDS and *NEON_DEBUG
- * to zero, *COMPRESSION to TRUE, *NEON_AUTH_TYPES is left untouched,
- * and the rest are set to NULL. This function can return an error,
- * so before examining any values, check the error return value.
+ * *PK11_PROVIDER, and *USE_CLIENT_CERTS to the information for
+ * REQUESTED_HOST, allocated in POOL, if there is any applicable
+ * information.
+ *
+ * If there is no applicable information or if there is an error, then
+ * set *PROXY_PORT to (unsigned int) -1, set *TIMEOUT_SECONDS and
+ * *NEON_DEBUG to zero, set *COMPRESSION and *USE_CLIENT_CERTS to
+ * TRUE, don't touch *NEON_AUTH_TYPES, and set the rest to NULL.
+ *
+ * This function can return an error, so before examining any values,
+ * check the error return value.
  */
 static svn_error_t *get_server_settings(const char **proxy_host,
                                         unsigned int *proxy_port,
@@ -434,6 +438,7 @@
                                         svn_boolean_t *compression,
                                         unsigned int *neon_auth_types,
                                         const char **pk11_provider,
+ svn_boolean_t *use_client_certs,
                                         svn_config_t *cfg,
                                         const char *requested_host,
                                         apr_pool_t *pool)
@@ -480,6 +485,8 @@
                               SVN_CONFIG_OPTION_HTTP_COMPRESSION, TRUE));
   svn_config_get(cfg, &debug_str, SVN_CONFIG_SECTION_GLOBAL,
                  SVN_CONFIG_OPTION_NEON_DEBUG_MASK, NULL);
+ SVN_ERR(svn_config_get_bool(cfg, use_client_certs, SVN_CONFIG_SECTION_GLOBAL,
+ SVN_CONFIG_OPTION_SSL_USE_CLIENT_CERTS, TRUE));
 #ifdef SVN_NEON_0_26
   svn_config_get(cfg, &http_auth_types, SVN_CONFIG_SECTION_GLOBAL,
                  SVN_CONFIG_OPTION_HTTP_AUTH_TYPES, NULL);
@@ -508,6 +515,9 @@
       SVN_ERR(svn_config_get_bool(cfg, compression, server_group,
                                   SVN_CONFIG_OPTION_HTTP_COMPRESSION,
                                   *compression));
+ SVN_ERR(svn_config_get_bool(cfg, use_client_certs, server_group,
+ SVN_CONFIG_OPTION_SSL_USE_CLIENT_CERTS,
+ *use_client_certs));
       svn_config_get(cfg, &debug_str, server_group,
                      SVN_CONFIG_OPTION_NEON_DEBUG_MASK, debug_str);
 #ifdef SVN_NEON_0_26
@@ -971,6 +981,7 @@
   svn_ra_neon__session_t *ras;
   int is_ssl_session;
   svn_boolean_t compression;
+ svn_boolean_t use_client_certs;
   svn_config_t *cfg;
   const char *server_group;
   char *itr;
@@ -1055,6 +1066,7 @@
                                 &compression,
                                 &neon_auth_types,
                                 &pkcs11_provider,
+ &use_client_certs,
                                 cfg,
                                 uri->host,
                                 pool));
@@ -1232,6 +1244,10 @@
          with the normal one here. */
       else
 #endif
+ /* ### RFC: Should the 'use_client_certs' condition also cover the
+ ### PKCS#11 case above? What about the "PKCS#12" referred to
+ ### in libsvn_subr/config_file.c:svn_config_ensure()? */
+ if (use_client_certs)
         {
           ne_ssl_provide_clicert(sess, client_ssl_callback, ras);
           ne_ssl_provide_clicert(sess2, client_ssl_callback, ras);
Index: subversion/libsvn_ra_serf/ra_serf.h
===================================================================
--- subversion/libsvn_ra_serf/ra_serf.h (revision 31887)
+++ subversion/libsvn_ra_serf/ra_serf.h (working copy)
@@ -201,6 +201,9 @@
 
   /* Repository UUID */
   const char *uuid;
+
+ /* Are we using client certs at all? */
+ svn_boolean_t use_client_certs;
 };
 
 /*
Index: subversion/libsvn_ra_serf/serf.c
===================================================================
--- subversion/libsvn_ra_serf/serf.c (revision 31887)
+++ subversion/libsvn_ra_serf/serf.c (working copy)
@@ -359,6 +359,10 @@
                               SVN_CONFIG_SECTION_GLOBAL,
                               SVN_CONFIG_OPTION_HTTP_COMPRESSION, TRUE));
 
+ SVN_ERR(svn_config_get_bool(config, &session->use_client_certs,
+ SVN_CONFIG_SECTION_GLOBAL,
+ SVN_CONFIG_OPTION_SSL_USE_CLIENT_CERTS, TRUE));
+
   svn_auth_set_parameter(session->wc_callbacks->auth_baton,
                          SVN_AUTH_PARAM_CONFIG, config);
 
@@ -394,6 +398,10 @@
                                   server_group,
                                   SVN_CONFIG_OPTION_HTTP_COMPRESSION,
                                   session->using_compression));
+ SVN_ERR(svn_config_get_bool(config, &session->use_client_certs,
+ SVN_CONFIG_SECTION_GLOBAL,
+ SVN_CONFIG_OPTION_SSL_USE_CLIENT_CERTS,
+ session->use_client_certs));
       svn_auth_set_parameter(session->wc_callbacks->auth_baton,
                              SVN_AUTH_PARAM_SERVER_GROUP, server_group);
 
Index: subversion/libsvn_ra_serf/util.c
===================================================================
--- subversion/libsvn_ra_serf/util.c (revision 31887)
+++ subversion/libsvn_ra_serf/util.c (working copy)
@@ -241,12 +241,15 @@
         {
           conn->ssl_context = serf_bucket_ssl_decrypt_context_get(bucket);
 #if SERF_VERSION_AT_LEAST(0,1,1)
- serf_ssl_client_cert_provider_set(conn->ssl_context,
- svn_ra_serf__handle_client_cert,
- conn, conn->session->pool);
- serf_ssl_client_cert_password_set(conn->ssl_context,
- svn_ra_serf__handle_client_cert_pw,
- conn, conn->session->pool);
+ if (conn->session->use_client_certs)
+ {
+ serf_ssl_client_cert_provider_set(
+ conn->ssl_context, svn_ra_serf__handle_client_cert,
+ conn, conn->session->pool);
+ serf_ssl_client_cert_password_set(
+ conn->ssl_context, svn_ra_serf__handle_client_cert_pw,
+ conn, conn->session->pool);
+ }
 #endif
 #if SERF_VERSION_AT_LEAST(0,1,3)
           serf_ssl_server_cert_callback_set(conn->ssl_context,
@@ -501,12 +504,15 @@
         {
           conn->ssl_context = serf_bucket_ssl_encrypt_context_get(*req_bkt);
 #if SERF_VERSION_AT_LEAST(0,1,1)
- serf_ssl_client_cert_provider_set(conn->ssl_context,
- svn_ra_serf__handle_client_cert,
- conn, conn->session->pool);
- serf_ssl_client_cert_password_set(conn->ssl_context,
- svn_ra_serf__handle_client_cert_pw,
- conn, conn->session->pool);
+ if (conn->session->use_client_certs)
+ {
+ serf_ssl_client_cert_provider_set(
+ conn->ssl_context, svn_ra_serf__handle_client_cert,
+ conn, conn->session->pool);
+ serf_ssl_client_cert_password_set(
+ conn->ssl_context, svn_ra_serf__handle_client_cert_pw,
+ conn, conn->session->pool);
+ }
 #endif
         }
     }

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-06-26 20:14:09 CEST

This is an archived mail posted to the Subversion Dev mailing list.