reload HTTP proxy credentials when moving to the next connection profile
PATCH 1/1 reload HTTP proxy credentials when moving to the next connection profile
The HTTP proxy credentials are stored in a static variable that is possibly initialized before each connection attempt.
However, the variable is never "released" therefore get_user_pass() refuses to overwrite its content and leaves it as it is. Consequently, if the user config contains multiple connection profiles with different http-proxy, each having its own credentials, only the first user/pass couple is loaded and the others are all ignored. This leads to connection failures because the proper credentials are not associated with the right proxy server.
The root of the misbehaviour seems to be located in the fact that, despite the argument force passed to get_user_pass_http() being true, no action is taken to release the static object containing the credentials.
Fix the misbehaviour by releasing the http-proxy credential object when the reload is "forced".
Trac: #836
--- src/openvpn/proxy.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
src/openvpn/proxy.c +10 -1
@@ -256,7 +256,16 @@ username_password_as_base64(const struct http_proxy_info *p,static void get_user_pass_http (struct http_proxy_info *p,const bool force) { -if (!static_proxy_user_pass.defined || force) +/* +* in case of forced (re)load, make sure the static storage is set as +* undefined, otherwise get_user_pass() won't try to load any credential +*/ +if (force) + { + static_proxy_user_pass.defined = false; + } + +if (!static_proxy_user_pass.defined) {unsigned int flags =GET_USER_PASS_MANAGEMENT ;if (p->queried_creds)
Signature · 1 line
2.11.0
Hi,
Quoted 15 lines · Antonio Quartulli
iff --git
a/src/openvpn/proxy.cb/src/openvpn/proxy.cindexb0ed3279..27f34bed100644src/openvpn/proxy.c +3
-1@@ -256,7 +256,16 @@ username_password_as_base64(const struct http_proxy_info *p, static void get_user_pass_http(struct http_proxy_info *p, const bool force) { -if (!static_proxy_user_pass.defined || force) +/* +* in case of forced (re)load, make sure the static storage is set as +* undefined, otherwise get_user_pass() won't try to load any credential
src/openvpn/proxy.c username_password_as_base64(const struct http_proxy_info *p, · line 266
Quoted 5 lines
src/openvpn/proxy.c +5
-0+*/ +if (force) + { + static_proxy_user_pass.defined = false; + }
While unsetting the define attribute appears to have no unintended side effects, purge_user_pass(&static_proxy_user_pass, true) would be cleaner.
src/openvpn/proxy.c username_password_as_base64(const struct http_proxy_info *p, · line 271
Quoted 6 lines
src/openvpn/proxy.c +2
-0+ +if (!static_proxy_user_pass.defined) {unsigned int flags =GET_USER_PASS_MANAGEMENT ;if (p->queried_creds)
That said, there is one issue with this approach. Looks like SIGUSR1 restarts will now always prompt for proxy password, which is not proper. If so, a more nuanced fix that preserves the current behaviour for a single proxy credentials is required. May be, do the "purge proxy pass" in init.c only if the connection entry has http-proxy specified?
I think the intent of the original code/man page is to have proxy password work the same way as auth-user-pass with an implicit auth-nocache assumed. Then multiple credentials is not expected, but could still work if the proxy password is purged when an authentication error happens instead of when the remote changes.
Finally, socks credentials may also be affected.
Selva
Hi Selva,
thank you very much for your feedback!
Quoted 19 lines · Selva Nair
Hi,
Quoted 15 lines · Antonio Quartulli
iff --git
a/src/openvpn/proxy.cb/src/openvpn/proxy.cindexb0ed3279..27f34bed100644src/openvpn/proxy.c +3
-1@@ -256,7 +256,16 @@ username_password_as_base64(const struct http_proxy_info *p, static void get_user_pass_http(struct http_proxy_info *p, const bool force) { -if (!static_proxy_user_pass.defined || force) +/* +* in case of forced (re)load, make sure the static storage is set as +* undefined, otherwise get_user_pass() won't try to load any credential
+ */
Quoted 9 lines
src/openvpn/proxy.c username_password_as_base64(const struct http_proxy_info *p, · line 266
Quoted 5 lines
src/openvpn/proxy.c +4
-0+if (force) + { + static_proxy_user_pass.defined = false; + }While unsetting the define attribute appears to have no unintended side effects,
purge_user_pass(&static_proxy_user_pass, true)would be cleaner.
yeah, I Was thinking about that too, but I was not sure. I will re-check, it might really be cleaner.
Quoted 10 lines
src/openvpn/proxy.c username_password_as_base64(const struct http_proxy_info *p, · line 271
Quoted 7 lines
src/openvpn/proxy.c +2
-0+ +if (!static_proxy_user_pass.defined) {unsigned int flags =GET_USER_PASS_MANAGEMENT ;if (p->queried_creds)That said, there is one issue with this approach. Looks like SIGUSR1 restarts will now always prompt for proxy password, which is not proper.
Right! Thanks for pointing this out!
Quoted 11 lines
If
so, a more nuanced fix that preserves the current behaviour for a single proxy credentials is required. May be, do the "purge proxy pass" ininit.conly if the connection entry has http-proxy specified?I think the intent of the original code/man page is to have proxy password work the same way as auth-user-pass with an implicit auth-nocache assumed. Then multiple credentials is not expected, but could still work if the proxy password is purged when an authentication error happens instead of when the remote changes.
I am thinking that maybe I could just restructure the way the user/pass are stored. Right now there is one static variable and I am trying to hack around it to make it work with multiple http proxies....but why not just storing one user/pass object per proxy? (like we currently do with the other proxy data)
That would probably be much cleaner, but will require a more intrusive change.
Quoted 1 line
Finally, socks credentials may also be affected.
I am not sure, because they use a different object/code-path. But I will re-check that too
Thanks a lot!
Cheers,
Signature · 1 line
Antonio Quartulli
Hi,
Quoted 4 lines · Antonio Quartulli
Quoted 2 lines
That said, there is one issue with this approach. Looks like SIGUSR1 restarts will now always prompt for proxy password, which is not proper.
Right! Thanks for pointing this out!
Actually I'd like to understand the expected behaviour: if SIGUSR1 is issued, openvpn should re-read the file containing the HTTP proxy credentials, but, if stdin was specified, it should not ask the user for user/pass again ?
So in case of stdin the only way to re-enter the user/pass is to restart openvpn?
What's your opinion on this?
Signature · 1 line
Antonio Quartulli
Quoted 13 lines · Antonio Quartulli
Quoted 2 lines · Antonio Quartulli
Quoted 2 lines
That said, there is one issue with this approach. Looks like SIGUSR1 restarts will now always prompt for proxy password, which is not
proper.
Quoted 2 lines
Right! Thanks for pointing this out!
Actually I'd like to understand the expected behaviour: if SIGUSR1 is issued,
openvpn should re-read the file containing the HTTP proxy credentials, but, if
stdin was specified, it should not ask the user for user/pass again ?
Quoted 4 lines
So in case of stdin the only way to re-enter the user/pass is to restart openvpn?
What's your opinion on this?
I wrote SIGUSR1 but its the same with SIGHUP. All [*] passwords are cached by default and http-proxy password is always cached (auth-nocache or not). So the expected behaviour is password to be read only once during the lifetime of the process unless an auth error happens.
The source of the password does not matter (stdin, systemd, file, mgmt, inline et..), its always read using get_user_pass which gets it afresh if no cached copy is available.
With only one cache per password type, this expects passwords are not remote-specific. As we support only one auth-user-pass or one private key pass per config I think the intent was to support only one proxy password. Allowing http-proxy in <connection/> block gives a different impression, though..
Selva
[*] Not sure about socks password