You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Senthil Kumaran S <se...@collab.net> on 2008/07/17 18:56:51 UTC
[PATCH] Fix issue #3239
Hi,
I am attaching a patch along with this email which fixes issue #3239 - client
cert passphrase cache keyed by relative path, not absolute path. This includes
a change to the public API "svn_cmdline_auth_ssl_client_cert_prompt" though it
does not affect the existing functionality and the function prototype.
I am not sure whether this is the right way to put such trivial fix into the
public API, so I am expecting comments whether my change is right. I enquired
about this in IRC to Arfrever, who asked me to send the patch to dev list.
[[[
Fix issue #3239 - client cert passphrase cache keyed by relative path,
not absolute path
* subversion/libsvn_subr/prompt.c
(svn_cmdline_auth_ssl_client_cert_prompt): Get the absolute path of cert
file and set it in the credentials.
Patch by: stylesen
]]]
Thank You.
--
Senthil Kumaran S
http://www.stylesen.org/
Re: [PATCH] Fix issue #3239
Posted by Neels Janosch Hofmeyr <ne...@elego.de>.
This patch looks perfect to me, especially given kfogel's comments.
It's been this perfect for almost a month, I gather.
If a committer could have the pity... ;)
Senthil Kumaran S wrote:
> Hi,
>
> I would like to get some review on this patch, so that it could be
> backported to issue2489 branch.
>
> Thank You.
>
> Senthil Kumaran S wrote:
> Hi Karl,
>
> Karl Fogel wrote:
>>>> Actually, I think this brings the code into compliance with this API:
>>>>
>>>> typedef struct svn_auth_cred_ssl_client_cert_t
>>>> {
>>>> /** Full paths to the certificate file */
>>>> const char *cert_file;
>>>> /** Indicates if the credentials may be saved (to disk). For
>>>> example, a
>>>> * GUI prompt implementation with a remember certificate
>>>> checkbox shall
>>>> * set @a may_save to TRUE if the checkbox is checked.
>>>> */
>>>> svn_boolean_t may_save;
>>>> } svn_auth_cred_ssl_client_cert_t;
>>>>
>>>> Since svn_cmdline_auth_ssl_client_cert_prompt() just sets the
>>>> 'cert_file' field in *cred, and that is documented to be the "full
>>>> paths", there is no incompatibility here. This can be considered a
>>>> bugfix, really.
>
> As per your comments I am attaching an updated patch.
>
> [[[
> Fix issue #3239 - client cert passphrase cache keyed by relative path,
> not absolute path
>
> * subversion/libsvn_subr/prompt.c
> (svn_cmdline_auth_ssl_client_cert_prompt): Get the absolute path of
> cert
> file and set it in the credentials.
>
> * subversion/include/svn_cmdline.h
> (svn_cmdline_auth_ssl_client_cert_prompt): Document that an absolute
> path
> of SSL client certificate filename will be recorded.
>
> * subversion/include/svn_auth.h
> (svn_auth_cred_ssl_client_cert_t): In the documentation
> s/Full paths/Absolute path/
>
> Patch by: stylesen
> ]]]
>
> Thank You.
>>
>>
------------------------------------------------------------------------
>>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
--
Neels Hofmeyr -- elego Software Solutions GmbH
Gustav-Meyer-Allee 25 / Gebäude 12, 13355 Berlin, Germany
phone: +49 30 23458696 mobile: +49 177 2345869 fax: +49 30 23458695
http://www.elegosoft.com | Geschäftsführer: Olaf Wagner | Sitz: Berlin
Handelsreg: Amtsgericht Charlottenburg HRB 77719 | USt-IdNr: DE163214194
Re: [PATCH] Fix issue #3239
Posted by Senthil Kumaran S <se...@collab.net>.
Karl Fogel wrote:
> Senthil Kumaran S <se...@collab.net> writes:
>> I would like to get some review on this patch, so that it could be
>> backported to issue2489 branch.
>
> r32550, and sorry for the delay in reviewing the patch.
Backported to 1.5.x-issue2489 branch in r32561.
--
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: [PATCH] Fix issue #3239
Posted by Senthil Kumaran S <se...@collab.net>.
Karl Fogel wrote:
> Senthil Kumaran S <se...@collab.net> writes:
>> I would like to get some review on this patch, so that it could be
>> backported to issue2489 branch.
>
> r32550, and sorry for the delay in reviewing the patch.
Thanks a ton :)
--
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: [PATCH] Fix issue #3239
Posted by Karl Fogel <kf...@red-bean.com>.
Senthil Kumaran S <se...@collab.net> writes:
> I would like to get some review on this patch, so that it could be
> backported to issue2489 branch.
r32550, and sorry for the delay in reviewing the patch.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix issue #3239
Posted by Senthil Kumaran S <se...@collab.net>.
Hi,
I would like to get some review on this patch, so that it could be backported
to issue2489 branch.
Thank You.
Senthil Kumaran S wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Hi Karl,
>
> Karl Fogel wrote:
>> Actually, I think this brings the code into compliance with this API:
>>
>> typedef struct svn_auth_cred_ssl_client_cert_t
>> {
>> /** Full paths to the certificate file */
>> const char *cert_file;
>> /** Indicates if the credentials may be saved (to disk). For example, a
>> * GUI prompt implementation with a remember certificate checkbox shall
>> * set @a may_save to TRUE if the checkbox is checked.
>> */
>> svn_boolean_t may_save;
>> } svn_auth_cred_ssl_client_cert_t;
>>
>> Since svn_cmdline_auth_ssl_client_cert_prompt() just sets the
>> 'cert_file' field in *cred, and that is documented to be the "full
>> paths", there is no incompatibility here. This can be considered a
>> bugfix, really.
>
> As per your comments I am attaching an updated patch.
>
> [[[
> Fix issue #3239 - client cert passphrase cache keyed by relative path,
> not absolute path
>
> * subversion/libsvn_subr/prompt.c
> (svn_cmdline_auth_ssl_client_cert_prompt): Get the absolute path of cert
> file and set it in the credentials.
>
> * subversion/include/svn_cmdline.h
> (svn_cmdline_auth_ssl_client_cert_prompt): Document that an absolute path
> of SSL client certificate filename will be recorded.
>
> * subversion/include/svn_auth.h
> (svn_auth_cred_ssl_client_cert_t): In the documentation
> s/Full paths/Absolute path/
>
> Patch by: stylesen
> ]]]
>
> Thank You.
> - --
> Senthil Kumaran S
> http://www.stylesen.org/
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.6 (GNU/Linux)
>
> iD8DBQFIht4z9o1G+2zNQDgRAhhmAKCUgVRI7hdUve7Q83V2++X7XSqKAwCfc8TH
> dNSXtP5Nts+hHjXFp3wFK7g=
> =fsWK
> -----END PGP SIGNATURE-----
>
>
> ------------------------------------------------------------------------
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
--
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: [PATCH] Fix issue #3239
Posted by Senthil Kumaran S <se...@collab.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Hi Karl,
Karl Fogel wrote:
> Actually, I think this brings the code into compliance with this API:
>
> typedef struct svn_auth_cred_ssl_client_cert_t
> {
> /** Full paths to the certificate file */
> const char *cert_file;
> /** Indicates if the credentials may be saved (to disk). For example, a
> * GUI prompt implementation with a remember certificate checkbox shall
> * set @a may_save to TRUE if the checkbox is checked.
> */
> svn_boolean_t may_save;
> } svn_auth_cred_ssl_client_cert_t;
>
> Since svn_cmdline_auth_ssl_client_cert_prompt() just sets the
> 'cert_file' field in *cred, and that is documented to be the "full
> paths", there is no incompatibility here. This can be considered a
> bugfix, really.
As per your comments I am attaching an updated patch.
[[[
Fix issue #3239 - client cert passphrase cache keyed by relative path,
not absolute path
* subversion/libsvn_subr/prompt.c
(svn_cmdline_auth_ssl_client_cert_prompt): Get the absolute path of cert
file and set it in the credentials.
* subversion/include/svn_cmdline.h
(svn_cmdline_auth_ssl_client_cert_prompt): Document that an absolute path
of SSL client certificate filename will be recorded.
* subversion/include/svn_auth.h
(svn_auth_cred_ssl_client_cert_t): In the documentation
s/Full paths/Absolute path/
Patch by: stylesen
]]]
Thank You.
- --
Senthil Kumaran S
http://www.stylesen.org/
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
iD8DBQFIht4z9o1G+2zNQDgRAhhmAKCUgVRI7hdUve7Q83V2++X7XSqKAwCfc8TH
dNSXtP5Nts+hHjXFp3wFK7g=
=fsWK
-----END PGP SIGNATURE-----
Re: [PATCH] Fix issue #3239
Posted by Karl Fogel <kf...@red-bean.com>.
Senthil Kumaran S <se...@collab.net> writes:
> Hi,
>
> I am attaching a patch along with this email which fixes issue #3239 -
> client cert passphrase cache keyed by relative path, not absolute
> path. This includes a change to the public API
> "svn_cmdline_auth_ssl_client_cert_prompt" though it does not affect
> the existing functionality and the function prototype.
>
> I am not sure whether this is the right way to put such trivial fix
> into the public API, so I am expecting comments whether my change is
> right. I enquired about this in IRC to Arfrever, who asked me to send
> the patch to dev list.
Actually, I think this brings the code into compliance with this API:
typedef struct svn_auth_cred_ssl_client_cert_t
{
/** Full paths to the certificate file */
const char *cert_file;
/** Indicates if the credentials may be saved (to disk). For example, a
* GUI prompt implementation with a remember certificate checkbox shall
* set @a may_save to TRUE if the checkbox is checked.
*/
svn_boolean_t may_save;
} svn_auth_cred_ssl_client_cert_t;
Since svn_cmdline_auth_ssl_client_cert_prompt() just sets the
'cert_file' field in *cred, and that is documented to be the "full
paths", there is no incompatibility here. This can be considered a
bugfix, really.
However, it might be good to change "Full paths" above to "Absolute
path", to be completely clear (and to fix the grammar -- I don't know
why it was plural before). And in the documentation for
svn_cmdline_auth_ssl_client_cert_prompt(), it would be good to state
that an absolute path will be recorded too, even though the receiving
data structure will also say that.
-Karl
> [[[
> Fix issue #3239 - client cert passphrase cache keyed by relative path,
> not absolute path
>
> * subversion/libsvn_subr/prompt.c
> (svn_cmdline_auth_ssl_client_cert_prompt): Get the absolute path of cert
> file and set it in the credentials.
>
> Patch by: stylesen
> ]]]
>
> Thank You.
> --
> Senthil Kumaran S
> http://www.stylesen.org/
>
>
> Index: subversion/libsvn_subr/prompt.c
> ===================================================================
> --- subversion/libsvn_subr/prompt.c (revision 32158)
> +++ subversion/libsvn_subr/prompt.c (working copy)
> @@ -29,6 +29,7 @@
> #include "svn_string.h"
> #include "svn_auth.h"
> #include "svn_error.h"
> +#include "svn_path.h"
>
> #include "svn_private_config.h"
>
> @@ -340,14 +341,16 @@
> {
> svn_auth_cred_ssl_client_cert_t *cred = NULL;
> const char *cert_file = NULL;
> + const char *abs_cert_file = NULL;
> svn_cmdline_prompt_baton2_t *pb = baton;
>
> SVN_ERR(maybe_print_realm(realm, pool));
> SVN_ERR(prompt(&cert_file, _("Client certificate filename: "),
> FALSE, pb, pool));
> + SVN_ERR(svn_path_get_absolute(&abs_cert_file, cert_file, pool));
>
> cred = apr_palloc(pool, sizeof(*cred));
> - cred->cert_file = cert_file;
> + cred->cert_file = abs_cert_file;
> cred->may_save = may_save;
> *cred_p = cred;
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org