You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Karl Fogel <kf...@red-bean.com> on 2008/06/26 16:27:32 UTC
Review requested on issue #2410 (SSL client certs option)
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 <da...@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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Review requested on issue #2410 (SSL client certs option)
Posted by Senthil Kumaran S <se...@collab.net>.
Karl Fogel wrote:
> Senthil Kumaran S <se...@collab.net> writes:
>> The complete patch with all the modifications as a result of review
>> from arfrever and stsp is given as attachment here
>> http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=139442
>
> Senthil, I hate to do this to you, but: the patch doesn't apply cleanly
> on trunk (see below); could you repost an updated (if necessary)
> version, this time as a 'text/plain' attachment? Or else just attach it
> to issue #2489. I'd love to review this (and commit!), but it's much
> easier to do review by applying to trunk and then looking at the change
> in context.
It was my fault, that I didn't post a new patch, since it was months old and a
lot of stuff had gone into simple_provider.c. I shall do it asap and post a new
patch :)
--
Senthil Kumaran S
http://www.stylesen.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Review requested on issue #2410 (SSL client certs option)
Posted by Senthil Kumaran S <se...@collab.net>.
Hi Karl,
Karl Fogel wrote:
> Senthil, I hate to do this to you, but: the patch doesn't apply cleanly
> on trunk (see below); could you repost an updated (if necessary)
> version, this time as a 'text/plain' attachment? Or else just attach it
> to issue #2489. I'd love to review this (and commit!), but it's much
> easier to do review by applying to trunk and then looking at the change
> in context.
I ve attached the updated patch to issue #2489 at desc7. You can get it from
here - http://subversion.tigris.org/nonav/issues/showattachment.cgi/900/2489.patch
This updated patch applies cleanly to r31970 of trunk as of now :) and passes
'make *check'.
Thank You.
--
Senthil Kumaran S
http://www.stylesen.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Review requested on issue #2410 (SSL client certs option)
Posted by Karl Fogel <kf...@red-bean.com>.
Senthil Kumaran S <se...@collab.net> writes:
> The complete patch with all the modifications as a result of review
> from arfrever and stsp is given as attachment here
> http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=139442
Senthil, I hate to do this to you, but: the patch doesn't apply cleanly
on trunk (see below); could you repost an updated (if necessary)
version, this time as a 'text/plain' attachment? Or else just attach it
to issue #2489. I'd love to review this (and commit!), but it's much
easier to do review by applying to trunk and then looking at the change
in context.
Maybe the patch was mangled because it was inline in your email, or
maybe it just doesn't apply cleanly to trunk anymore due to code
changes.
See below for what happened when I tried to apply to r31953 of trunk.
Thanks,
-Karl
$ patch -p0 < carefully_cleaned_up_patch_file
patching file subversion/libsvn_ra/ra_loader.c
patching file subversion/libsvn_subr/config_file.c
patching file subversion/libsvn_subr/cmdline.c
Hunk #1 FAILED at 355.
Hunk #2 FAILED at 373.
Hunk #3 FAILED at 383.
Hunk #4 FAILED at 476.
Hunk #5 FAILED at 493.
Hunk #6 succeeded at 506 (offset -2 lines).
Hunk #7 succeeded at 529 (offset -2 lines).
5 out of 7 hunks FAILED -- saving rejects to file subversion/libsvn_subr/cmdline.c.rej
patching file subversion/libsvn_subr/ssl_client_cert_pw_providers.c
patching file subversion/libsvn_subr/prompt.c
Hunk #1 FAILED at 379.
Hunk #2 succeeded at 438 (offset 7 lines).
1 out of 2 hunks FAILED -- saving rejects to file subversion/libsvn_subr/prompt.c.rej
patching file subversion/libsvn_auth_gnome_keyring/gnome_keyring.c
patching file subversion/include/svn_config.h
patching file subversion/include/svn_auth_dso.h
patching file subversion/include/svn_cmdline.h
patching file subversion/include/private/svn_auth_private.h
patching file subversion/include/svn_auth.h
patching file subversion/libsvn_ra_neon/session.c
$
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Review requested on issue #2410 (SSL client certs option)
Posted by Senthil Kumaran S <se...@collab.net>.
Senthil Kumaran S wrote:
> Karl Fogel wrote:
>> * When we prompt the user for a client cert passphrase, do *not*
>> automagically store that passphrase in ~/.subversion/servers
>> (because it's bad to automagically store passphrases).
>>
>> Instead, present a prompt like the one we do for regular plaintext
>> passwords, wherein we ask they user if they want to store it, and
>> also tell them what to set to avoid being asked in the future (a
>> new boolean that says whether or not to store client cert
>> passphrases after they've been read from prompt).
>
> The patch attached here
> http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=140222 for
> issue #2849 handles this. If there is no gnome-keyring mechanism, the
> user is prompted for storing plaintext passphrases as we do for storing
> plaintext passwords. There is also a boolean type already coded in that
> patch which decides whether or not to store client cert passphrases
> after they've been read from the prompt. Yet I need to receive a +1
> (long awaited patch, more than 2 months now) from some full committer to
> get this patch into trunk which will enable me to resume work on Windows
> CryptAPI, Keychain, Kwallet, etc :(
The complete patch with all the modifications as a result of review from
arfrever and stsp is given as attachment here
http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=139442
NOTE: The previous thread I ve mentioned includes the patch inline to my reply,
which may be difficult for people who wants to review the patch, to cut and
paste it in their working copy.
--
Senthil Kumaran S
http://www.stylesen.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Review requested on issue #2410 (SSL client certs option)
Posted by Senthil Kumaran S <se...@collab.net>.
Karl Fogel wrote:
> * When we prompt the user for a client cert passphrase, do *not*
> automagically store that passphrase in ~/.subversion/servers
> (because it's bad to automagically store passphrases).
>
> Instead, present a prompt like the one we do for regular plaintext
> passwords, wherein we ask they user if they want to store it, and
> also tell them what to set to avoid being asked in the future (a
> new boolean that says whether or not to store client cert
> passphrases after they've been read from prompt).
The patch attached here
http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=140222 for issue
#2849 handles this. If there is no gnome-keyring mechanism, the user is
prompted for storing plaintext passphrases as we do for storing plaintext
passwords. There is also a boolean type already coded in that patch which
decides whether or not to store client cert passphrases after they've been read
from the prompt. Yet I need to receive a +1 (long awaited patch, more than 2
months now) from some full committer to get this patch into trunk which will
enable me to resume work on Windows CryptAPI, Keychain, Kwallet, etc :(
Thank You.
--
Senthil Kumaran S
http://www.stylesen.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Review requested on issue #2410 (SSL client certs option)
Posted by Karl Fogel <kf...@red-bean.com>.
Daniel Shahaf <d....@daniel.shahaf.co.il> writes:
> Karl Fogel wrote on Sun, 29 Jun 2008 at 22:17 -0400:
>> Also, if we want to be really fancy ("fancy" meaning, uh, "useable"):
>>
>> * When we do prompt for a cert file, automagically update
>> ~/.subversion/servers to point to the cert file location.
>>
>
> Don't change my ~/.subversion/servers, please; it's meant to be
> user-maintained, not libsvn-maintained. We can cache the location in the
> same place(s) we already cache usernames/passwords in.
Good point; agreed.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Review requested on issue #2410 (SSL client certs option)
Posted by Daniel Shahaf <d....@daniel.shahaf.co.il>.
Karl Fogel wrote on Sun, 29 Jun 2008 at 22:17 -0400:
> Also, if we want to be really fancy ("fancy" meaning, uh, "useable"):
>
> * When we do prompt for a cert file, automagically update
> ~/.subversion/servers to point to the cert file location.
>
Don't change my ~/.subversion/servers, please; it's meant to be
user-maintained, not libsvn-maintained. We can cache the location in the
same place(s) we already cache usernames/passwords in.
Daniel
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Review requested on issue #2410 (SSL client certs option)
Posted by Karl Fogel <kf...@red-bean.com>.
"Mark Phippard" <ma...@gmail.com> writes:
> AIUI, the only way to configure client certs is the servers file.
> This is not automatic at all. The book says the same:
>
> http://svnbook.red-bean.com/nightly/en/svn.serverconfig.httpd.html#svn.serverconfig.httpd.authn.sslcerts
Oops, I mis-assumed, sorry. You're right, there isn't a default
location (no automagic ~/.subversion/client-certs/SERVER_NAME/cert.p12
or whatever). Instead, you put the cert.p12 somewhere safe, anywhere
you want, and edit your ~/.subversion/servers file to point to it.
I now grok this in fullness; thank you for pointing to that section of
the book :-).
So what Joe is suggesting is that when no ~/.subversion/servers option
is available to point to the cert file, we do not prompt for a location
by default; instead, throw an error describing how to set the path in
~/.subversion/servers. But, also offer a boolean config option to say
"Yes, I want to be prompted when the client cert file is not found."
Also, if we want to be really fancy ("fancy" meaning, uh, "useable"):
* When we do prompt for a cert file, automagically update
~/.subversion/servers to point to the cert file location.
* When we prompt the user for a client cert passphrase, do *not*
automagically store that passphrase in ~/.subversion/servers
(because it's bad to automagically store passphrases).
Instead, present a prompt like the one we do for regular plaintext
passwords, wherein we ask they user if they want to store it, and
also tell them what to set to avoid being asked in the future (a
new boolean that says whether or not to store client cert
passphrases after they've been read from prompt).
Those last two ideas are nice-to-haves, not must-haves, of course.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Review requested on issue #2410 (SSL client certs option)
Posted by Mark Phippard <ma...@gmail.com>.
On Sun, Jun 29, 2008 at 9:58 PM, Karl Fogel <kf...@red-bean.com> wrote:
> "Mark Phippard" <ma...@gmail.com> writes:
>> On Sun, Jun 29, 2008 at 9:23 PM, Karl Fogel <kf...@red-bean.com> wrote:
>>> "Mark Phippard" <ma...@gmail.com> writes:
>>>> So then how does the client find a cert in the default location? I
>>>> thought what Joe was saying is that anyone that needs to use a client
>>>> cert knows they have to configure it in some way.
>>>
>>> I don't know how client certs get into the default location now, but
>>> whatever way they do, that isn't affected by the proposed change. Joe
>>> is merely saying that if the client does *not* find the cert there, that
>>> (by default) we shouldn't prompt for some other path.
>>>
>>> He also suggests that we improve the couldn't-find-it error to give some
>>> more information about where to put certs, and (I assume) to mention the
>>> new config option in case the user would like to be prompted.
>>
>> Well that is kind of what I am getting at. He seems to be proposing
>> we replace our code with new code that finds certs somewhere. I am
>> asking for the details. It could be relevant to caching the
>> passphrases if the thing that stores the certificates also secures
>> them.
>
> Oh. I thought we already had code that looks for client certs in some
> default location (under ~/.subversion/, presumably), and that the
> prompting only happens if the client doesn't find any in that default
> location.
>
> If that's not the case, then I don't understand how client certs could
> work at all without prompting (but I also don't understand how they
> could possibly ever have been useful, since that would imply that
> everyone gets prompted every time).
AIUI, the only way to configure client certs is the servers file.
This is not automatic at all. The book says the same:
http://svnbook.red-bean.com/nightly/en/svn.serverconfig.httpd.html#svn.serverconfig.httpd.authn.sslcerts
--
Thanks
Mark Phippard
http://markphip.blogspot.com/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Review requested on issue #2410 (SSL client certs option)
Posted by Karl Fogel <kf...@red-bean.com>.
"Mark Phippard" <ma...@gmail.com> writes:
> On Sun, Jun 29, 2008 at 9:23 PM, Karl Fogel <kf...@red-bean.com> wrote:
>> "Mark Phippard" <ma...@gmail.com> writes:
>>> So then how does the client find a cert in the default location? I
>>> thought what Joe was saying is that anyone that needs to use a client
>>> cert knows they have to configure it in some way.
>>
>> I don't know how client certs get into the default location now, but
>> whatever way they do, that isn't affected by the proposed change. Joe
>> is merely saying that if the client does *not* find the cert there, that
>> (by default) we shouldn't prompt for some other path.
>>
>> He also suggests that we improve the couldn't-find-it error to give some
>> more information about where to put certs, and (I assume) to mention the
>> new config option in case the user would like to be prompted.
>
> Well that is kind of what I am getting at. He seems to be proposing
> we replace our code with new code that finds certs somewhere. I am
> asking for the details. It could be relevant to caching the
> passphrases if the thing that stores the certificates also secures
> them.
Oh. I thought we already had code that looks for client certs in some
default location (under ~/.subversion/, presumably), and that the
prompting only happens if the client doesn't find any in that default
location.
If that's not the case, then I don't understand how client certs could
work at all without prompting (but I also don't understand how they
could possibly ever have been useful, since that would imply that
everyone gets prompted every time).
> The issue is: http://subversion.tigris.org/issues/show_bug.cgi?id=2489
>
> Thread with patch:
>
> [...]
Thread added, and issues linked. Thanks.
> Not sure which message has the most recent patch. An issue that
> Senthil is dealing with is that he has to write the code for each
> place we can cache these (Gnome-keyring, KWallet, Windows crypt, OSX
> keychain) separately. Apparently, our current password encryption is
> not architected in a way that it can be reused. Perhaps after the
> patch is done, this can be modified so they can share common code?
Sounds sane; noted in issue #2489.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Review requested on issue #2410 (SSL client certs option)
Posted by Mark Phippard <ma...@gmail.com>.
On Sun, Jun 29, 2008 at 9:23 PM, Karl Fogel <kf...@red-bean.com> wrote:
> "Mark Phippard" <ma...@gmail.com> writes:
>> So then how does the client find a cert in the default location? I
>> thought what Joe was saying is that anyone that needs to use a client
>> cert knows they have to configure it in some way.
>
> I don't know how client certs get into the default location now, but
> whatever way they do, that isn't affected by the proposed change. Joe
> is merely saying that if the client does *not* find the cert there, that
> (by default) we shouldn't prompt for some other path.
>
> He also suggests that we improve the couldn't-find-it error to give some
> more information about where to put certs, and (I assume) to mention the
> new config option in case the user would like to be prompted.
Well that is kind of what I am getting at. He seems to be proposing
we replace our code with new code that finds certs somewhere. I am
asking for the details. It could be relevant to caching the
passphrases if the thing that stores the certificates also secures
them.
>> It is a separate issue in our issue tracker. Patches have been on
>> list. I only mention it because if we were going to rethink how we
>> handle client certs, this is an important aspect of them where we need
>> improvements.
>
> Thanks. If you can tell me the issue number, I'll link it up to #2410
> and #2597, so all the client cert issues know about each other.
The issue is: http://subversion.tigris.org/issues/show_bug.cgi?id=2489
Thread with patch:
http://subversion.tigris.org/servlets/BrowseList?list=dev&by=thread&from=656943
Not sure which message has the most recent patch. An issue that
Senthil is dealing with is that he has to write the code for each
place we can cache these (Gnome-keyring, KWallet, Windows crypt, OSX
keychain) separately. Apparently, our current password encryption is
not architected in a way that it can be reused. Perhaps after the
patch is done, this can be modified so they can share common code?
--
Thanks
Mark Phippard
http://markphip.blogspot.com/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Review requested on issue #2410 (SSL client certs option)
Posted by Karl Fogel <kf...@red-bean.com>.
"Mark Phippard" <ma...@gmail.com> writes:
> So then how does the client find a cert in the default location? I
> thought what Joe was saying is that anyone that needs to use a client
> cert knows they have to configure it in some way.
I don't know how client certs get into the default location now, but
whatever way they do, that isn't affected by the proposed change. Joe
is merely saying that if the client does *not* find the cert there, that
(by default) we shouldn't prompt for some other path.
He also suggests that we improve the couldn't-find-it error to give some
more information about where to put certs, and (I assume) to mention the
new config option in case the user would like to be prompted.
> It is a separate issue in our issue tracker. Patches have been on
> list. I only mention it because if we were going to rethink how we
> handle client certs, this is an important aspect of them where we need
> improvements.
Thanks. If you can tell me the issue number, I'll link it up to #2410
and #2597, so all the client cert issues know about each other.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Review requested on issue #2410 (SSL client certs option)
Posted by Mark Phippard <ma...@gmail.com>.
On Sun, Jun 29, 2008 at 8:48 PM, Karl Fogel <kf...@red-bean.com> wrote:
> "Mark Phippard" <ma...@gmail.com> writes:
>> So in the end, in terms of implementation, what is Joe suggesting? It
>> sounds like he'd expect anyone using client certs to configure them in
>> the servers file? And we should just not prompt for a certificate and
>> instead error out if one is required but not configured?
>
> I think Joe is proposing a new boolean client-side config option in the
> 'servers' file:
>
> # Prompt for path to client cert file when server requires a client
> # cert but none could be found in the default location(s). Off by
> # default.
> # ssl-client-cert-prompt = no
>
> I presume we'd list it in the [global] section, and it would also be
> valid in a server-specific section, where it would behave in the usual
> way (i.e., override the global).
So then how does the client find a cert in the default location? I
thought what Joe was saying is that anyone that needs to use a client
cert knows they have to configure it in some way.
>> I am not sure if that is better or worse. What I do think is
>> important is that Senthil's patch to allow the passphrase for the
>> client certificate to be cached like we cache passwords. We have a
>> customer that is eager to get this feature.
>
> Which patch is that? (It's not in issue #2410, AFAICT.)
>
> I think it's not necessarily related to what Joe is talking about,
> because finding a client cert and caching the password for it (if any)
> are two different things. But there could be some interaction I'm not
> understanding here.
It is a separate issue in our issue tracker. Patches have been on
list. I only mention it because if we were going to rethink how we
handle client certs, this is an important aspect of them where we need
improvements.
--
Thanks
Mark Phippard
http://markphip.blogspot.com/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Review requested on issue #2410 (SSL client certs option)
Posted by Karl Fogel <kf...@red-bean.com>.
"Mark Phippard" <ma...@gmail.com> writes:
> So in the end, in terms of implementation, what is Joe suggesting? It
> sounds like he'd expect anyone using client certs to configure them in
> the servers file? And we should just not prompt for a certificate and
> instead error out if one is required but not configured?
I think Joe is proposing a new boolean client-side config option in the
'servers' file:
# Prompt for path to client cert file when server requires a client
# cert but none could be found in the default location(s). Off by
# default.
# ssl-client-cert-prompt = no
I presume we'd list it in the [global] section, and it would also be
valid in a server-specific section, where it would behave in the usual
way (i.e., override the global).
> I am not sure if that is better or worse. What I do think is
> important is that Senthil's patch to allow the passphrase for the
> client certificate to be cached like we cache passwords. We have a
> customer that is eager to get this feature.
Which patch is that? (It's not in issue #2410, AFAICT.)
I think it's not necessarily related to what Joe is talking about,
because finding a client cert and caching the password for it (if any)
are two different things. But there could be some interaction I'm not
understanding here.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Review requested on issue #2410 (SSL client certs option)
Posted by Mark Phippard <ma...@gmail.com>.
On Sun, Jun 29, 2008 at 7:14 PM, Karl Fogel <kf...@red-bean.com> wrote:
> Joe Orton <jo...@redhat.com> writes:
>> The current behaviour (svn prompting for a filename when an SSL client
>> cert is requested) is pretty unusual - I don't know why it works like
>> this. I can't think of a common use case where the behaviour is
>> particularly useful, and certainly there are lots where it is actively
>> unhelpful, e.g. as per the referenced bug.
>>
>> If you are using an SSL server which requires client cert auth, you will
>> most likely have configured that beforehand. If you are using it
>> regularly you certainly won't be typing in that filename every commit.
>>
>> If you are using such a server, and you *don't know* that it requires
>> client cert auth, chances are you don't have one.
>>
>> If you're using some global-ish PKI with lots of servers which might
>> require client cert auth, you will have configured that beforehand too.
>>
>> Rather than pushing yet-more config knobs down into ra_* I would suggest
>> adding a config toggle which only adds the prompting provider if a
>> config boolean is enabled (but is off by default). That would solve
>> this bug and make the default behaviour correct to boot.
>
> I rarely use client certs myself, so I don't have much direct experience
> with our client cert UI, but what you say seems reasonable to me. And
> it's a much simpler solution.
>
>> Possible problems:
>>
>> 1) this is arguably a backwards compat break, but it's not like this is
>> going to break scripts since it's only removing a case which always
>> requires interactive input.
>
> Right; it's not a compatibility problem with any practical consequences,
> I think.
>
>> 2) the default error for the "SSL server requested a client cert but
>> none was provided" is probably an obscure SSL error message; this is the
>> only real value of the current prompt. This could probably be improved.
>
> Agreed.
>
>> Thoughts?
>
> Uhmph. I've attached this thread to the issue. I'd like to make your
> suggested patch, but I've got some (read: twenty) other threads to take
> care of first. So if someone were to beat me to it, that'd be fine.
So in the end, in terms of implementation, what is Joe suggesting? It
sounds like he'd expect anyone using client certs to configure them in
the servers file? And we should just not prompt for a certificate and
instead error out if one is required but not configured?
I am not sure if that is better or worse. What I do think is
important is that Senthil's patch to allow the passphrase for the
client certificate to be cached like we cache passwords. We have a
customer that is eager to get this feature.
--
Thanks
Mark Phippard
http://markphip.blogspot.com/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Review requested on issue #2410 (SSL client certs option)
Posted by Karl Fogel <kf...@red-bean.com>.
Joe Orton <jo...@redhat.com> writes:
> The current behaviour (svn prompting for a filename when an SSL client
> cert is requested) is pretty unusual - I don't know why it works like
> this. I can't think of a common use case where the behaviour is
> particularly useful, and certainly there are lots where it is actively
> unhelpful, e.g. as per the referenced bug.
>
> If you are using an SSL server which requires client cert auth, you will
> most likely have configured that beforehand. If you are using it
> regularly you certainly won't be typing in that filename every commit.
>
> If you are using such a server, and you *don't know* that it requires
> client cert auth, chances are you don't have one.
>
> If you're using some global-ish PKI with lots of servers which might
> require client cert auth, you will have configured that beforehand too.
>
> Rather than pushing yet-more config knobs down into ra_* I would suggest
> adding a config toggle which only adds the prompting provider if a
> config boolean is enabled (but is off by default). That would solve
> this bug and make the default behaviour correct to boot.
I rarely use client certs myself, so I don't have much direct experience
with our client cert UI, but what you say seems reasonable to me. And
it's a much simpler solution.
> Possible problems:
>
> 1) this is arguably a backwards compat break, but it's not like this is
> going to break scripts since it's only removing a case which always
> requires interactive input.
Right; it's not a compatibility problem with any practical consequences,
I think.
> 2) the default error for the "SSL server requested a client cert but
> none was provided" is probably an obscure SSL error message; this is the
> only real value of the current prompt. This could probably be improved.
Agreed.
> Thoughts?
Uhmph. I've attached this thread to the issue. I'd like to make your
suggested patch, but I've got some (read: twenty) other threads to take
care of first. So if someone were to beat me to it, that'd be fine.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Review requested on issue #2410 (SSL client certs option)
Posted by Karl Fogel <kf...@red-bean.com>.
Joe Orton <jo...@redhat.com> writes:
> The current behaviour (svn prompting for a filename when an SSL client
> cert is requested) is pretty unusual - I don't know why it works like
> this. I can't think of a common use case where the behaviour is
> particularly useful, and certainly there are lots where it is actively
> unhelpful, e.g. as per the referenced bug.
>
> If you are using an SSL server which requires client cert auth, you will
> most likely have configured that beforehand. If you are using it
> regularly you certainly won't be typing in that filename every commit.
>
> If you are using such a server, and you *don't know* that it requires
> client cert auth, chances are you don't have one.
>
> If you're using some global-ish PKI with lots of servers which might
> require client cert auth, you will have configured that beforehand too.
>
> Rather than pushing yet-more config knobs down into ra_* I would suggest
> adding a config toggle which only adds the prompting provider if a
> config boolean is enabled (but is off by default). That would solve
> this bug and make the default behaviour correct to boot. Possible
> problems:
>
> 1) this is arguably a backwards compat break, but it's not like this is
> going to break scripts since it's only removing a case which always
> requires interactive input.
>
> 2) the default error for the "SSL server requested a client cert but
> none was provided" is probably an obscure SSL error message; this is the
> only real value of the current prompt. This could probably be improved.
>
> Thoughts?
I started a patch to do what Joe suggested. There are some unexpected
difficulties, however, due to the way our code is arranged. Search for
"### TODO" in the patch below for details.
An easy solution would be to just put the new ssl-client-cert-prompt
option into the 'config' file instead of into 'servers'. On the one
hand, 'servers' seems like the more logical place for it; on the other
hand, putting it into 'config' would work right now without massive code
reorganization... And really, who would ever want to control on a
server-by-server basis whether or not they get prompted for client cert
paths?
Feedback welcome. While awaiting others' thoughts, I'm going to move on
to issue #2489.
[[[
##################################################################
### ###
### NOT READY FOR COMMIT; SEARCH FOR "### TODO" IN THE PATCH ###
### FOR DISCUSSION OF KNOWN ISSUES. ###
### ###
##################################################################
Fix issue #2410: don't prompt for client cert path unless configured to.
* subversion/libsvn_subr/config_file.c
(svn_config_ensure): Document new ssl-client-cert-prompt option.
Include a comment about how it might not be in the right place.
* subversion/libsvn_subr/cmdline.c
(svn_cmdline_set_up_auth_baton): Don't register the client cert
prompter unless cfg says it's okay. Include a comment about why
this doesn't work the way we'd want it to.
* subversion/include/svn_config.h
(SVN_CONFIG_OPTION_SSL_CLIENT_CERT_PROMPT): Define new option.
]]]
Index: subversion/libsvn_subr/config_file.c
===================================================================
--- subversion/libsvn_subr/config_file.c (revision 32000)
+++ subversion/libsvn_subr/config_file.c (working copy)
@@ -747,6 +747,8 @@
"### ssl-client-cert-file PKCS#12 format client certificate file"
NL
"### ssl-client-cert-password Client Key password, if needed." NL
+ "### ssl-client-cert-prompt Prompt for client cert file path" NL
+ "### (see farther down for details)." NL
"### ssl-pkcs11-provider Name of PKCS#11 provider to use." NL
"### http-library Which library to use for http/https"
NL
@@ -787,6 +789,18 @@
"### saving of *new* credentials; it doesn't invalidate existing" NL
"### caches. (To do that, remove the cache files by hand.)" NL
"###" NL
+ /* ### TODO: Right now, ssl-client-cert-prompt can only appear
+ * ### in the [global] section of the 'servers' file, because
+ * ### we don't have the server name available when we call
+ * ### svn_cmdline_set_up_auth_baton() to register the authn
+ * ### providers. Therefore, should ssl-client-cert-prompt go
+ * ### in the 'config' file instead of in 'servers'? Or... ?
+ */
+ "### Set ssl-client-cert-prompt to 'yes' to cause the client to" NL
+ "### prompt for a path to a client cert file when the server" NL
+ "### requests a client cert but no client cert file is found in the" NL
+ "### expected place (see ssl-client-cert-file). Defaults to 'no'." NL
+ "###" NL
"### HTTP timeouts, if given, are specified in seconds. A timeout" NL
"### of 0, i.e. zero, causes a builtin default to be used." NL
"###" NL
@@ -858,6 +872,8 @@
"# No neon-debug-mask, so neon debugging is disabled." NL
"# ssl-authority-files = /path/to/CAcert.pem;/path/to/CAcert2.pem" NL
"#" NL
+ "# ssl-client-cert-prompt = no" NL
+ "#" NL
"# Password caching parameters:" NL
"# store-passwords = no" NL
"# store-plaintext-passwords = no" NL;
Index: subversion/libsvn_subr/cmdline.c
===================================================================
--- subversion/libsvn_subr/cmdline.c (revision 32000)
+++ subversion/libsvn_subr/cmdline.c (working copy)
@@ -568,19 +568,45 @@
2, /* retry limit */ pool);
APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
- /* Three ssl prompt providers, for server-certs, client-certs,
- and client-cert-passphrases. */
+ /* SSL prompt providers for server-certs and client-cert-passphrases. */
svn_auth_get_ssl_server_trust_prompt_provider
(&provider, svn_cmdline_auth_ssl_server_trust_prompt, pb, pool);
APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
- svn_auth_get_ssl_client_cert_prompt_provider
- (&provider, svn_cmdline_auth_ssl_client_cert_prompt, pb, 2, pool);
- APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
-
svn_auth_get_ssl_client_cert_pw_prompt_provider
(&provider, svn_cmdline_auth_ssl_client_cert_pw_prompt, pb, 2, pool);
APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
+
+ /* Register an SSL prompt provider for client-cert files, but
+ only if config says that prompting for them is okay. */
+ {
+ svn_boolean_t do_prompt;
+
+ /* ### TODO: Aha, but this doesn't work, because here cfg
+ ### represents only the 'config' file, not the 'servers'
+ ### file. We have to either move the option over to the
+ ### 'config' file, which seems like a less than ideal place
+ ### for it, or get access to 'servers' here. But see the
+ ### "### TODO" comment in svn_config_ensure() in
+ ### libsvn_subr/config_file.c, where we initialize the
+ ### servers file, for discussion of another problem: the
+ ### fact that we don't have the server name yet, so we can
+ ### only use the option if it appears in the [global]
+ ### section.
+ */
+ SVN_ERR(svn_config_get_server_setting_bool(
+ cfg, &do_prompt, NULL,
+ SVN_CONFIG_OPTION_SSL_CLIENT_CERT_PROMPT, FALSE));
+
+ if (do_prompt)
+ {
+ svn_auth_get_ssl_client_cert_prompt_provider(
+ &provider, svn_cmdline_auth_ssl_client_cert_prompt,
+ pb, 2, pool);
+ APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *)
+ = provider;
+ }
+ }
}
else if (trust_server_cert)
{
Index: subversion/include/svn_config.h
===================================================================
--- subversion/include/svn_config.h (revision 32000)
+++ subversion/include/svn_config.h (working copy)
@@ -72,6 +72,7 @@
#define SVN_CONFIG_OPTION_SSL_TRUST_DEFAULT_CA "ssl-trust-default-ca"
#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_CLIENT_CERT_PROMPT "ssl-client-cert-prompt"
#define SVN_CONFIG_OPTION_SSL_PKCS11_PROVIDER "ssl-pkcs11-provider"
#define SVN_CONFIG_OPTION_HTTP_LIBRARY "http-library"
#define SVN_CONFIG_OPTION_STORE_PASSWORDS "store-passwords"
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Review requested on issue #2410 (SSL client certs option)
Posted by Joe Orton <jo...@redhat.com>.
On Thu, Jun 26, 2008 at 12:27:32PM -0400, Karl Fogel wrote:
> 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.
The current behaviour (svn prompting for a filename when an SSL client
cert is requested) is pretty unusual - I don't know why it works like
this. I can't think of a common use case where the behaviour is
particularly useful, and certainly there are lots where it is actively
unhelpful, e.g. as per the referenced bug.
If you are using an SSL server which requires client cert auth, you will
most likely have configured that beforehand. If you are using it
regularly you certainly won't be typing in that filename every commit.
If you are using such a server, and you *don't know* that it requires
client cert auth, chances are you don't have one.
If you're using some global-ish PKI with lots of servers which might
require client cert auth, you will have configured that beforehand too.
Rather than pushing yet-more config knobs down into ra_* I would suggest
adding a config toggle which only adds the prompting provider if a
config boolean is enabled (but is off by default). That would solve
this bug and make the default behaviour correct to boot. Possible
problems:
1) this is arguably a backwards compat break, but it's not like this is
going to break scripts since it's only removing a case which always
requires interactive input.
2) the default error for the "SSL server requested a client cert but
none was provided" is probably an obscure SSL error message; this is the
only real value of the current prompt. This could probably be improved.
Thoughts?
joe
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Review requested on issue #2410 (SSL client certs option)
Posted by Karl Fogel <kf...@red-bean.com>.
Senthil Kumaran S <se...@collab.net> writes:
> Karl Fogel wrote:
>> @@ -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)
>> {
>
> I tested with a PKCS#12 client certificate, it works perfectly.
Thank you for the patch and the testing summary, Senthil. I'm looking
at Joe Orton's mail now, and seeing that we may end up resolving this
issue a completely different way, but I'm still glad to know this patch
got tested, just in case we have to use it after all.
-K
> Following is a summary of what I did to test this patch:
>
> My apache config parameters
>
> <apache-config>
> <Location /svn/repos>
> DAV svn
> SVNPath /tmp/repos
> AuthType Basic
> AuthName "TEST SVN repository"
> AuthUserFile /etc/svn-auth-file
> Require valid-user
> <IfDefine SSL>
> SSLRequireSSL
> SSLRequire %{SSL_CLIENT_S_DN_O} eq "CollabNet Inc" and
> %{SSL_CLIENT_S_DN_OU} in {"Administration"}
> </IfDefine>
> </Location>
>
> SSLCertificateFile /usr/local/ssl/CollabCA/server/certs/serverWEB.crt
> SSLCertificateKeyFile /usr/local/ssl/CollabCA/server/keys/serverWEB.key
> SSLCACertificateFile /usr/local/ssl/CollabCA/CollabCA.crt
> SSLVerifyClient optional
> SSLVerifyDepth 2
> </apache-config>
>
> First run with default servers file:
>
> <snip>
> $ svn co https://localhost/svn/repos wc
> Authentication realm: https://localhost:443
> Client certificate filename: /usr/local/ssl/CollabCA/user/certs/stylesen.p12
> Passphrase for '/usr/local/ssl/CollabCA/user/certs/stylesen.p12':
> Authentication realm: <https://localhost:443> TEST SVN repository
> Password for 'stylesen':
> A wc/file1
> A wc/file2
> Checked out revision 2.
> </snip>
>
> Second run with "ssl-use-client-certs = no" servers file:
>
> <snip>
> $ svn co https://localhost/svn/repos wc
> Authentication realm: <https://localhost:443> TEST SVN repository
> Password for 'stylesen':
> A wc/file1
> A wc/file2
> Checked out revision 2.
> </snip>
>
> NOTE: This patch is valid only if you have "SSLVerifyClient optional"
> in your apache config. If you have something lie "SSLVerifyClient
> require" it will result in following error:
>
> <snip>
> ../subversion/libsvn_ra_neon/util.c:603: (apr_err=175002)
> svn: OPTIONS of 'https://localhost/svn/repos': SSL negotiation failed:
> SSL error: sslv3 alert handshake failure (https://localhost)
> </snip>
>
> Thank You.
> --
> Senthil Kumaran S
> http://www.stylesen.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Review requested on issue #2410 (SSL client certs option)
Posted by Senthil Kumaran S <se...@collab.net>.
Hi Karl,
Karl Fogel wrote:
> @@ -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)
> {
I tested with a PKCS#12 client certificate, it works perfectly.
Following is a summary of what I did to test this patch:
My apache config parameters
<apache-config>
<Location /svn/repos>
DAV svn
SVNPath /tmp/repos
AuthType Basic
AuthName "TEST SVN repository"
AuthUserFile /etc/svn-auth-file
Require valid-user
<IfDefine SSL>
SSLRequireSSL
SSLRequire %{SSL_CLIENT_S_DN_O} eq "CollabNet Inc" and
%{SSL_CLIENT_S_DN_OU} in {"Administration"}
</IfDefine>
</Location>
SSLCertificateFile /usr/local/ssl/CollabCA/server/certs/serverWEB.crt
SSLCertificateKeyFile /usr/local/ssl/CollabCA/server/keys/serverWEB.key
SSLCACertificateFile /usr/local/ssl/CollabCA/CollabCA.crt
SSLVerifyClient optional
SSLVerifyDepth 2
</apache-config>
First run with default servers file:
<snip>
$ svn co https://localhost/svn/repos wc
Authentication realm: https://localhost:443
Client certificate filename: /usr/local/ssl/CollabCA/user/certs/stylesen.p12
Passphrase for '/usr/local/ssl/CollabCA/user/certs/stylesen.p12':
Authentication realm: <https://localhost:443> TEST SVN repository
Password for 'stylesen':
A wc/file1
A wc/file2
Checked out revision 2.
</snip>
Second run with "ssl-use-client-certs = no" servers file:
<snip>
$ svn co https://localhost/svn/repos wc
Authentication realm: <https://localhost:443> TEST SVN repository
Password for 'stylesen':
A wc/file1
A wc/file2
Checked out revision 2.
</snip>
NOTE: This patch is valid only if you have "SSLVerifyClient optional" in your
apache config. If you have something lie "SSLVerifyClient require" it will
result in following error:
<snip>
../subversion/libsvn_ra_neon/util.c:603: (apr_err=175002)
svn: OPTIONS of 'https://localhost/svn/repos': SSL negotiation failed: SSL
error: sslv3 alert handshake failure (https://localhost)
</snip>
Thank You.
--
Senthil Kumaran S
http://www.stylesen.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Review requested on issue #2410 (SSL client certs option)
Posted by Karl Fogel <kf...@red-bean.com>.
svnlgo@mobsol.be writes:
>> @@ -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_CONFIG_SECTION_GLOBAL should be server_group.
D'oh, dumb copy-and-pasto. Thanks, Lieven.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Review requested on issue #2410 (SSL client certs option)
Posted by sv...@mobsol.be.
Karl,
Quoting Karl Fogel <kf...@red-bean.com>:
> 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.
>
I didn't review the ra_neon part. The ra_serf looks basically okay to me, expect
one remark.
> [[[
> #################################################################
> ### ###
> ### 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 <da...@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.
This should be (svn_ra_serf__session_t).
>
> * 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
[Skipping the ra_neon part]
> 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_CONFIG_SECTION_GLOBAL should be server_group.
[..]
Lieven
----------------------------------------------------------------
This message was sent using IMP, the Internet Messaging Program.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org