Re: [Openvpn-devel] [PATCH] Remove static global allocation of HTTP proxy user/passwords

[David S. <op...@sf...>]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 22/09/16 22:51, Selva Nair wrote:
> Hi,
> 
> On Thu, Sep 22, 2016 at 3:40 PM, David Sommerseth
> <da...@op... <mailto:da...@op...>> wrote:
> 
> This avoids allocating static memory which is not used unless the a
> HTTP proxy with authentication is configured.
> 
> 
> .....
> 
> The only place the original code referred to the global 
> static_proxy_user_pass appears to be in this function and from its
> usage it appears redundant.
> 
> static void get_user_pass_http (struct http_proxy_info *p, const
> bool force) { -  if (!static_proxy_user_pass.defined || force) +
> if (!proxy_user_pass_cache || !proxy_user_pass_cache->defined|| 
> force) { unsigned int flags = GET_USER_PASS_MANAGEMENT; + +      if
> (!proxy_user_pass_cache) +        { +          ALLOC_OBJ_CLEAR
> (proxy_user_pass_cache, struct user_pass); +        } +      else +
> { +          CLEAR (*proxy_user_pass_cache); +        } + if
> (p->queried_creds) flags |= GET_USER_PASS_PREVIOUS_CREDS_FAILED; if
> (p->options.inline_creds) flags |= GET_USER_PASS_INLINE_CREDS; -
> get_user_pass (&static_proxy_user_pass, +      get_user_pass
> (proxy_user_pass_cache, p->options.auth_file, UP_TYPE_PROXY, 
> flags); p->queried_creds = true; -      p->up =
> static_proxy_user_pass;
> 
> 
> A copy of the struct is made only when a new password is obtained
> using get_user_pass. Then, what was the purpose of the static? Or
> was it a bug that the copy of the cached password was not made
> every time this function was called? If not, we can just delete the
> static and use p->up directly.
> 
> } +  p->up = proxy_user_pass_cache; }

I completely agree!  I did consider this too, but figured I needed to
understand better what happens if a client uses connection blocks and
if the caching of username/password goes across these connection blocks.

If you have --http-proxy configured which requires authentication and
 two connection blocks.  By removing the caching, I worry that OpenVPN
would ask for a new proxy username/password each time it tries a
different connection block.

I haven't had enough time to dig into this scenario, but if that is
not the issue then I don't see any point in keeping this cache.  But
it is hard to get a complete overview where each of these
configuration structs are unique and singular and where they are
re-used and all points at the same "object".

But I'm happy to see more spotting the same issue.  And I'm always
thankful for your thorough reviews, Selva!


- -- 
kind regards,

David Sommerseth
OpenVPN Technologies, Inc

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJX5OrxAAoJEIbPlEyWcf3yULsP/RuL/+qm3svPCyatylCIupC5
cAXb480V+gyw+acE0IJMYGwQ7tQ7mjNKVDhV+BP5nSROX+Kgc5VPy/sHULkuNDLM
NKw4pa0X53PGPIiGSGMryphheFb22hiBVi0RPzGRC453IRSQSasKXuRMur+3Jlkf
J9T6i/WlrOD2c1VROGPY7PXjHXZct7+P42rERxWaJTTQ6KA+yS6Nzx+EZw/v9EO4
dnZhORPkq6IBu+cZ7JshhA6S2wY0CUDVZ0wSIFG2KcrJi/JjP5GkeTS+74RVbL3V
+OPcGBdlHC08wDCQpRdKsAAv7M/GmdR2sQJKoPlhT7JTkLr87LC1vV+e5tx3FonN
n45jB1bVV0VZ0Bo+lVG+4j3eyNaN/hhdMLHsQDdxQ9n1v7ajGFYf29K+nxsymb4V
GFMw45LHitHkCQfmptOjRMkmoRQE2tOjniiUyLWj1a9uEJwesPfHBpa1qYP9LprQ
rqUbscDiH5VPATUzAlv7O1UMo7mJVQdkYd9y9X9ayc+8uY5uARqf/sPoga8QGRcD
0v7/CvrXCKhVDYpIV2UWgstJ2aw2Rmv3voKC8rNlchIK5ivHly7PO0Hh4KjW2qXO
ZnW1kvYhThw0nz/W63v6QHZa/JY+p4BiybTS3mANAwvy4uzYlZugG7oq/DpFj+5/
eYH5gDWPMtJxACZzlfM5
=Ndg9
-----END PGP SIGNATURE-----