Tobias Ringström wrote:
> Ben Reser wrote:
>
>> On Fri, Dec 12, 2003 at 11:02:08AM -0500, Greg Hudson wrote:
>>
>> Add another one to the list:
>>
>> #10: svn_client_get_ssl_client_cert_prompt_provider and
>> svn_client_get_ssl_client_cert_pw_prompt_provider
>> do not have a retry limit. IMPACT OF PROBLEM: Clients
>> implemnting SSL certificate
>> support will only be able to give end users one chance
>> at providing the proper file for the cert and the pass
>> for that certificate. It seems almost certain that
>> people are going to want this fixed. To do it would
>> require a change in the API.
>> DIFFICULTY OF FIX: Medium, there may be complications
>> of trying to fix this that I'm not aware of. But
>> given that simple_prompt already supports retries
>> I'd image it would be straightforward unless neon
>> presents a roadblock on this.
>>
>> I mentioned this in the past but nobody replied to it.
>
> I'm working on fixing some other authentication annoyances at the
> moment, so I'll fix this one as well. If it ends up in 1.0 or not is
> another issue of course. I think it should be included because the fix
> is straightforward, and it does not touch any sensitive code, but we'll
> see.
And here is the patch. It looks big, but it's mostly code that's been
moved and reindented. The patch does not touch code outside the SSL
client certificate functionality, so I think it's quite safe.
Is this code desirable/acceptable for 1.0?
/Tobias
-----------------------------------------------------------------------
Implement retries for the SSL client certificate filename and password
prompts. Add a realm string to the prompt functions so that the user
can know what information that being asked for.
* subversion/include/svn_client.h
(svn_client_get_ssl_client_cert_prompt_provider,
svn_client_get_ssl_client_cert_pw_prompt_provider):
Add a retry_limit argument.
* subversion/include/svn_auth.h
(svn_auth_ssl_client_cert_prompt_func_t,
svn_auth_ssl_client_cert_pw_prompt_func_t):
Add a realm argument.
* subversion/libsvn_client/ssl_client_cert_pw_providers.c
(ssl_client_cert_pw_prompt_provider_baton_t):
Add a retry_limit struct member.
(ssl_client_cert_pw_prompt_iter_baton_t): New struct.
(ssl_client_cert_pw_prompt_first_cred): Allocate and initialize an
iteration baton. Provide the realmstring to the prompt function.
(ssl_client_cert_pw_prompt_next_cred): New function to handle
authentication retries.
(client_cert_pw_prompt_provider): Initialize the next_cred member.
(svn_client_get_ssl_client_cert_pw_prompt_provider): Initialize the
new retry_limit in the provider baton.
* subversion/libsvn_client/ssl_client_cert_providers.c
(ssl_client_cert_prompt_provider_baton_t):
Add a retry_limit struct member.
(ssl_client_cert_prompt_iter_baton_t): New struct.
(ssl_client_cert_prompt_first_cred): Allocate and initialize an
iteration baton. Provide the realmstring to the prompt function.
(ssl_client_cert_prompt_next_cred): New function to handle
authentication retries.
(client_cert_prompt_provider): Initialize the next_cred member.
(svn_client_get_ssl_client_cert_prompt_provider): Initialize the
new retry_limit in the provider baton.
* subversion/clients/cmdline/cl.h
(svn_cl__auth_ssl_client_cert_prompt): Add a realm argument.
(svn_cl__auth_ssl_client_cert_pw_prompt): Add a realm argument.
* subversion/clients/cmdline/prompt.c
(svn_cl__auth_ssl_client_cert_prompt): Print the realm.
(svn_cl__auth_ssl_client_cert_pw_prompt): Print the realm.
Do not handle the empty string specially, and do not check for NULL
since it cannot happen (and no other user seems to do it).
* subversion/clients/cmdline/main.c
(main): Use 2 as retry limit for the client cert prompt functions.
* subversion/libsvn_ra_dav/session.c
(client_ssl_keypw_callback): Removed.
(client_ssl_decrypt_cert): New function used to decrypt an encrypted
client cerificate using the auth system. Handles retries.
(client_ssl_callback): Implement a realm string for client certs.
Handle retries if the cert cannot not be loaded. Use the new
client_ssl_decrypt_cert to decrypt the certificate if needed.
Index: subversion/include/svn_client.h
===================================================================
--- subversion/include/svn_client.h (revision 7997)
+++ subversion/include/svn_client.h (working copy)
@@ -226,7 +226,7 @@
* @a *provider retrieves its credentials by using the @a prompt_func
* and @a prompt_baton. The returned credential is used to load the
* appropriate client certificate for authentication when requested by
- * a server.
+ * a server. The prompt will be retried @a retry_limit times.
*
* @a *provider requires no run-time parameters.
*/
@@ -234,6 +234,7 @@
svn_auth_provider_object_t **provider,
svn_auth_ssl_client_cert_prompt_func_t prompt_func,
void *prompt_baton,
+ int retry_limit,
apr_pool_t *pool);
/** Create and return @a *provider, an authentication provider of type @c
@@ -241,7 +242,8 @@
*
* @a *provider retrieves its credentials by using the @a prompt_func
* and @a prompt_baton. The returned credential is used when a loaded
- * client certificate is protected by a passphrase.
+ * client certificate is protected by a passphrase. The prompt will
+ * be retried @a retry_limit times.
*
* @a *provider requires no run-time parameters.
*/
@@ -249,6 +251,7 @@
svn_auth_provider_object_t **provider,
svn_auth_ssl_client_cert_pw_prompt_func_t prompt_func,
void *prompt_baton,
+ int retry_limit,
apr_pool_t *pool);
Index: subversion/include/svn_auth.h
===================================================================
--- subversion/include/svn_auth.h (revision 7997)
+++ subversion/include/svn_auth.h (working copy)
@@ -179,7 +179,9 @@
/** SSL client passphrase.
*
* @a password gets set with the appropriate password for the
- * certificate.
+ * certificate. The realmstring use with this credential type
+ * must be a name that makes it possible for the user to identify
+ * the certificate.
*/
#define SVN_AUTH_CRED_SSL_CLIENT_CERT_PW "svn.ssl.client-passphrase"
typedef struct
@@ -294,6 +296,7 @@
typedef svn_error_t *(*svn_auth_ssl_client_cert_prompt_func_t) (
svn_auth_cred_ssl_client_cert_t **cred,
void *baton,
+ const char *realm,
apr_pool_t *pool);
@@ -303,6 +306,7 @@
typedef svn_error_t *(*svn_auth_ssl_client_cert_pw_prompt_func_t) (
svn_auth_cred_ssl_client_cert_pw_t **cred,
void *baton,
+ const char *realm,
apr_pool_t *pool);
Index: subversion/libsvn_client/ssl_client_cert_pw_providers.c
===================================================================
--- subversion/libsvn_client/ssl_client_cert_pw_providers.c (revision 7997)
+++ subversion/libsvn_client/ssl_client_cert_pw_providers.c (working copy)
@@ -98,9 +98,25 @@
{
svn_auth_ssl_client_cert_pw_prompt_func_t prompt_func;
void *prompt_baton;
+
+ /* how many times to re-prompt after the first one fails */
+ int retry_limit;
} ssl_client_cert_pw_prompt_provider_baton_t;
+/* Iteration baton. */
+typedef struct
+{
+ /* The original provider baton */
+ ssl_client_cert_pw_prompt_provider_baton_t *pb;
+ /* The original realmstring */
+ const char *realmstring;
+
+ /* how many times we've reprompted */
+ int retries;
+} ssl_client_cert_pw_prompt_iter_baton_t;
+
+
static svn_error_t *
ssl_client_cert_pw_prompt_first_cred (void **credentials_p,
void **iter_baton,
@@ -110,20 +126,50 @@
apr_pool_t *pool)
{
ssl_client_cert_pw_prompt_provider_baton_t *pb = provider_baton;
+ ssl_client_cert_pw_prompt_iter_baton_t *ib =
+ apr_pcalloc (pool, sizeof (*ib));
SVN_ERR (pb->prompt_func ((svn_auth_cred_ssl_client_cert_pw_t **)
credentials_p,
- pb->prompt_baton, pool));
+ pb->prompt_baton, realmstring, pool));
- *iter_baton = NULL;
+ ib->pb = pb;
+ ib->realmstring = apr_pstrdup (pool, realmstring);
+ ib->retries = 0;
+ *iter_baton = ib;
+
return SVN_NO_ERROR;
}
+static svn_error_t *
+ssl_client_cert_pw_prompt_next_cred (void **credentials_p,
+ void *iter_baton,
+ apr_hash_t *parameters,
+ apr_pool_t *pool)
+{
+ ssl_client_cert_pw_prompt_iter_baton_t *ib = iter_baton;
+
+ if (ib->retries >= ib->pb->retry_limit)
+ {
+ /* give up, go on to next provider. */
+ *credentials_p = NULL;
+ return SVN_NO_ERROR;
+ }
+ ib->retries++;
+
+ SVN_ERR (ib->pb->prompt_func ((svn_auth_cred_ssl_client_cert_pw_t **)
+ credentials_p, ib->pb->prompt_baton,
+ ib->realmstring, pool));
+
+ return SVN_NO_ERROR;
+}
+
+
static const svn_auth_provider_t client_cert_pw_prompt_provider = {
SVN_AUTH_CRED_SSL_CLIENT_CERT_PW,
ssl_client_cert_pw_prompt_first_cred,
- NULL,
+ ssl_client_cert_pw_prompt_next_cred,
NULL
};
@@ -132,13 +178,17 @@
svn_auth_provider_object_t **provider,
svn_auth_ssl_client_cert_pw_prompt_func_t prompt_func,
void *prompt_baton,
+ int retry_limit,
apr_pool_t *pool)
{
svn_auth_provider_object_t *po = apr_pcalloc (pool, sizeof(*po));
ssl_client_cert_pw_prompt_provider_baton_t *pb =
apr_palloc (pool, sizeof(*pb));
+
pb->prompt_func = prompt_func;
pb->prompt_baton = prompt_baton;
+ pb->retry_limit = retry_limit;
+
po->vtable = &client_cert_pw_prompt_provider;
po->provider_baton = pb;
*provider = po;
Index: subversion/libsvn_client/ssl_client_cert_providers.c
===================================================================
--- subversion/libsvn_client/ssl_client_cert_providers.c (revision 7997)
+++ subversion/libsvn_client/ssl_client_cert_providers.c (working copy)
@@ -106,9 +106,25 @@
{
svn_auth_ssl_client_cert_prompt_func_t prompt_func;
void *prompt_baton;
+
+ /* how many times to re-prompt after the first one fails */
+ int retry_limit;
} ssl_client_cert_prompt_provider_baton_t;
+/* Iteration baton. */
+typedef struct
+{
+ /* The original provider baton */
+ ssl_client_cert_prompt_provider_baton_t *pb;
+ /* The original realmstring */
+ const char *realm;
+
+ /* how many times we've reprompted */
+ int retries;
+} ssl_client_cert_prompt_iter_baton_t;
+
+
static svn_error_t *
ssl_client_cert_prompt_first_cred (void **credentials_p,
void **iter_baton,
@@ -118,19 +134,49 @@
apr_pool_t *pool)
{
ssl_client_cert_prompt_provider_baton_t *pb = provider_baton;
+ ssl_client_cert_prompt_iter_baton_t *ib =
+ apr_pcalloc (pool, sizeof (*ib));
SVN_ERR (pb->prompt_func ((svn_auth_cred_ssl_client_cert_t **) credentials_p,
- pb->prompt_baton, pool));
+ pb->prompt_baton, realmstring, pool));
- *iter_baton = NULL;
+ ib->pb = pb;
+ ib->realm = apr_pstrdup (pool, realmstring);
+ ib->retries = 0;
+ *iter_baton = ib;
+
return SVN_NO_ERROR;
}
+static svn_error_t *
+ssl_client_cert_prompt_next_cred (void **credentials_p,
+ void *iter_baton,
+ apr_hash_t *parameters,
+ apr_pool_t *pool)
+{
+ ssl_client_cert_prompt_iter_baton_t *ib = iter_baton;
+
+ if (ib->retries >= ib->pb->retry_limit)
+ {
+ /* give up, go on to next provider. */
+ *credentials_p = NULL;
+ return SVN_NO_ERROR;
+ }
+ ib->retries++;
+
+ SVN_ERR (ib->pb->prompt_func ((svn_auth_cred_ssl_client_cert_t **)
+ credentials_p, ib->pb->prompt_baton,
+ ib->realm, pool));
+
+ return SVN_NO_ERROR;
+}
+
+
static const svn_auth_provider_t ssl_client_cert_prompt_provider = {
SVN_AUTH_CRED_SSL_CLIENT_CERT,
ssl_client_cert_prompt_first_cred,
- NULL,
+ ssl_client_cert_prompt_next_cred,
NULL
};
@@ -141,12 +187,16 @@
svn_auth_provider_object_t **provider,
svn_auth_ssl_client_cert_prompt_func_t prompt_func,
void *prompt_baton,
+ int retry_limit,
apr_pool_t *pool)
{
svn_auth_provider_object_t *po = apr_pcalloc (pool, sizeof(*po));
ssl_client_cert_prompt_provider_baton_t *pb = apr_palloc (pool, sizeof(*pb));
+
pb->prompt_func = prompt_func;
pb->prompt_baton = prompt_baton;
+ pb->retry_limit = retry_limit;
+
po->vtable = &ssl_client_cert_prompt_provider;
po->provider_baton = pb;
*provider = po;
Index: subversion/clients/cmdline/cl.h
===================================================================
--- subversion/clients/cmdline/cl.h (revision 7997)
+++ subversion/clients/cmdline/cl.h (working copy)
@@ -296,6 +296,7 @@
svn_cl__auth_ssl_client_cert_prompt (
svn_auth_cred_ssl_client_cert_t **cred_p,
void *baton,
+ const char *realm,
apr_pool_t *pool);
@@ -304,6 +305,7 @@
svn_cl__auth_ssl_client_cert_pw_prompt (
svn_auth_cred_ssl_client_cert_pw_t **cred_p,
void *baton,
+ const char *realm,
apr_pool_t *pool);
Index: subversion/clients/cmdline/prompt.c
===================================================================
--- subversion/clients/cmdline/prompt.c (revision 7997)
+++ subversion/clients/cmdline/prompt.c (working copy)
@@ -277,12 +277,14 @@
svn_error_t *
svn_cl__auth_ssl_client_cert_prompt (svn_auth_cred_ssl_client_cert_t **cred_p,
void *baton,
+ const char *realm,
apr_pool_t *pool)
{
+ svn_auth_cred_ssl_client_cert_t *cred = NULL;
const char *cert_file = NULL;
- svn_auth_cred_ssl_client_cert_t *cred;
- SVN_ERR (prompt (&cert_file, "client certificate filename: ", FALSE, pool));
+ SVN_ERR (maybe_print_realm (realm, pool));
+ SVN_ERR (prompt (&cert_file, "Client certificate filename: ", FALSE, pool));
cred = apr_palloc (pool, sizeof(*cred));
cred->cert_file = cert_file;
@@ -297,24 +299,18 @@
svn_cl__auth_ssl_client_cert_pw_prompt (
svn_auth_cred_ssl_client_cert_pw_t **cred_p,
void *baton,
+ const char *realm,
apr_pool_t *pool)
{
-
+ svn_auth_cred_ssl_client_cert_pw_t *cred = NULL;
const char *result;
+ const char *text = apr_psprintf (pool, "Passphrase for '%s': ", realm);
- SVN_ERR (prompt (&result, "client certificate passphrase: ", TRUE, pool));
+ SVN_ERR (prompt (&result, text, TRUE, pool));
- if (result && result[0])
- {
- svn_auth_cred_ssl_client_cert_pw_t *ret =
- apr_pcalloc (pool, sizeof (*ret));
- ret->password = result;
- *cred_p = ret;
- }
- else
- {
- *cred_p = NULL;
- }
+ cred = apr_pcalloc (pool, sizeof (*cred));
+ cred->password = result;
+ *cred_p = cred;
return SVN_NO_ERROR;
}
Index: subversion/clients/cmdline/main.c
===================================================================
--- subversion/clients/cmdline/main.c (revision 7997)
+++ subversion/clients/cmdline/main.c (working copy)
@@ -1181,11 +1181,11 @@
APR_ARRAY_PUSH (providers, svn_auth_provider_object_t *) = provider;
svn_client_get_ssl_client_cert_prompt_provider
- (&provider, svn_cl__auth_ssl_client_cert_prompt, NULL, pool);
+ (&provider, svn_cl__auth_ssl_client_cert_prompt, NULL, 2, pool);
APR_ARRAY_PUSH (providers, svn_auth_provider_object_t *) = provider;
svn_client_get_ssl_client_cert_pw_prompt_provider
- (&provider, svn_cl__auth_ssl_client_cert_pw_prompt, NULL, pool);
+ (&provider, svn_cl__auth_ssl_client_cert_pw_prompt, NULL, 2, pool);
APR_ARRAY_PUSH (providers, svn_auth_provider_object_t *) = provider;
}
Index: subversion/libsvn_ra_dav/session.c
===================================================================
--- subversion/libsvn_ra_dav/session.c (revision 7997)
+++ subversion/libsvn_ra_dav/session.c (working copy)
@@ -224,36 +224,55 @@
return !server_creds;
}
-static int
-client_ssl_keypw_callback(void *userdata, char *pwbuf, size_t len)
+static svn_boolean_t
+client_ssl_decrypt_cert(svn_ra_session_t *ras,
+ const char *cert_file,
+ ne_ssl_client_cert *clicert)
{
- svn_ra_session_t *ras = userdata;
- void *creds;
- svn_auth_cred_ssl_client_cert_pw_t *pw_creds = NULL;
svn_auth_iterstate_t *state;
+ svn_error_t *error;
apr_pool_t *pool;
- svn_error_t *error;
+ svn_boolean_t ok = FALSE;
+ void *creds;
+ int try;
apr_pool_create(&pool, ras->pool);
- error = svn_auth_first_credentials(&creds, &state,
- SVN_AUTH_CRED_SSL_CLIENT_CERT_PW,
- "none", /* ### fix? */
- ras->callbacks->auth_baton,
- pool);
- if (error || !creds)
+ for (try = 0; TRUE; ++try)
{
- svn_error_clear(error);
- }
- else
- {
- pw_creds = creds;
- if (pw_creds)
+ if (try == 0)
{
- apr_cpystrn(pwbuf, pw_creds->password, len);
+ error = svn_auth_first_credentials(&creds, &state,
+ SVN_AUTH_CRED_SSL_CLIENT_CERT_PW,
+ cert_file,
+ ras->callbacks->auth_baton,
+ pool);
}
+ else
+ {
+ error = svn_auth_next_credentials(&creds, state, pool);
+ }
+
+ if (error || !creds)
+ {
+ /* Failure or too many attempts */
+ svn_error_clear(error);
+ break;
+ }
+ else
+ {
+ svn_auth_cred_ssl_client_cert_pw_t *pw_creds = creds;
+
+ if (ne_ssl_clicert_decrypt(clicert, pw_creds->password) == 0)
+ {
+ /* Success */
+ ok = TRUE;
+ break;
+ }
+ }
}
apr_pool_destroy(pool);
- return (pw_creds == NULL);
+
+ return ok;
}
@@ -263,44 +282,58 @@
int dncount)
{
svn_ra_session_t *ras = userdata;
+ ne_ssl_client_cert *clicert = NULL;
void *creds;
svn_auth_iterstate_t *state;
+ const char *realmstring;
apr_pool_t *pool;
svn_error_t *error;
+ int try;
+
apr_pool_create(&pool, ras->pool);
- error = svn_auth_first_credentials(&creds, &state,
- SVN_AUTH_CRED_SSL_CLIENT_CERT,
- "none", /* ### fix? */
- ras->callbacks->auth_baton,
- pool);
- if (error || !creds)
+
+ realmstring = apr_psprintf (pool, "%s://%s:%d", ras->root.scheme,
+ ras->root.host, ras->root.port);
+
+ for (try = 0; TRUE; ++try)
{
- svn_error_clear(error);
- }
- else
- {
- svn_auth_cred_ssl_client_cert_t *client_creds = creds;
- ne_ssl_client_cert *clicert
- = ne_ssl_clicert_read(client_creds->cert_file);
- if (clicert != NULL)
+ if (try == 0)
{
- if (ne_ssl_clicert_encrypted(clicert))
- {
- char pw[128];
- if ((client_ssl_keypw_callback(userdata, pw, sizeof(pw)) == 0)
- && (ne_ssl_clicert_decrypt(clicert, pw) == 0))
- /* Successfully decrypted */
- ne_ssl_set_clicert(sess, clicert);
+ error = svn_auth_first_credentials(&creds, &state,
+ SVN_AUTH_CRED_SSL_CLIENT_CERT,
+ realmstring,
+ ras->callbacks->auth_baton,
+ pool);
+ }
+ else
+ {
+ error = svn_auth_next_credentials(&creds, state, pool);
+ }
- /* ### Notify user if decrypt fails? */
- }
- else
+ if (error || !creds)
+ {
+ /* Failure or too many attempts */
+ svn_error_clear(error);
+ break;
+ }
+ else
+ {
+ svn_auth_cred_ssl_client_cert_t *client_creds = creds;
+
+ clicert = ne_ssl_clicert_read(client_creds->cert_file);
+ if (clicert)
{
- /* Cert isn't encrypted, so just attach it. */
- ne_ssl_set_clicert(sess, clicert);
+ if (!ne_ssl_clicert_encrypted(clicert) ||
+ client_ssl_decrypt_cert(ras, client_creds->cert_file,
+ clicert))
+ {
+ ne_ssl_set_clicert(sess, clicert);
+ }
+ break;
}
}
}
+
apr_pool_destroy(pool);
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Dec 13 15:42:47 2003