Http proxy basic authentication on windows #1410

sylvlecl · 2024-05-20T16:20:50Z

This is a proposition to solve the issue reported in this discussion.

Instead of just providing the content of HTTPS_PROXY environment variable, we now parse it to extract proxy credentials, then provide it to WinHTTP API (which is not as simple as one could expect).

I've been able to validate the behaviour in a test environment with a squid proxy.

Notes

use of regex may be overkill ...
In order to have the whole chain working, this would need to be ported to C executable tls12-download. Otherwise, windows users behind a proxy still need to download vcpkg.exe without the bootstrap script

Signed-off-by: Sylvain Leclerc <sylvain.leclerc@rte-france.com>

sylvlecl · 2024-05-21T09:26:46Z

@microsoft-github-policy-service agree company="RTE"

guilpier-code · 2024-05-22T15:34:55Z

I tried it and it worked.
Good job !
For hours I tried to make it work, even trying to involve my company's IT.
Thanks, it's being of great help

BillyONeal · 2024-05-28T22:25:26Z

src/vcpkg/base/downloads.cpp

+
+    ProxyUrlParts parse_proxy_url(const std::wstring& url)
+    {
+        std::wregex scheme_pattern(L"^(.+://)(.*)");           // Matches scheme part


Parsing URIs with regex is notoriously difficult and I'm not convinced that there wouldn't be ways to exploit this.

We've tried really really hard to break the std::regex dependency entirely because all the std::regex implementations are awful.

Perhaps this means we should be looking at deploying something like trurl?

Thank you for the feedback!
Actually here it should be pretty easy to use simple string search instead of regex. I can implement this.

For "deploying trurl", would you mean adding a dependency to libcurl, on which trurl seems to rely behind the scene?
But I'm not sure it's easy to use on windows. I guess in that case it would make sense to completely rely on libcurl for the download.
Or do you mean something else, like requiring the user to install trurl in addition to vcpkg.exe?

I removed the regex use in last commit, to use only basic string searches.

BillyONeal · 2024-05-28T22:26:42Z

In order to have the whole chain working, this would need to be ported to C executable tls12-download. Otherwise, windows users behind a proxy still need to download vcpkg.exe without the bootstrap script

tls12-download needs to be very very tiny in order to do its job since it needs to be checked in. Customers in this condition can manually download a vcpkg.exe with any browser they want.

Signed-off-by: Sylvain Leclerc <sylvain.leclerc@rte-france.com>

BillyONeal · 2024-06-05T18:17:44Z

src/vcpkg-test/downloads.cpp

+            ProxyUrlParts{L"my-hostname.com:8080", ProxyCredentials{L"Joe", L"secret"}});
+    REQUIRE(parse_proxy_url(L"https://my-hostname.com:8080") == ProxyUrlParts{L"https://my-hostname.com:8080", {}});
+    REQUIRE(parse_proxy_url(L"https://Joe:secret@my-hostname.com:8080") ==
+            ProxyUrlParts{L"https://my-hostname.com:8080", ProxyCredentials{L"Joe", L"secret"}});


It seems like this should at least contain all the forms in the RFC: https://www.rfc-editor.org/rfc/rfc3986#section-1.1.2

BillyONeal · 2024-06-05T18:24:18Z

src/vcpkg/base/downloads.cpp

+        auto credentials_separator_pos = remaining_parts.find(L"@");
+        auto password_separator_pos = remaining_parts.find(L":");


This seems incorrect as the password separator, if present, must be inside the userinfo. Perhaps the STL algorithm form would be clearer since it makes chopping ranges into subranges like this easy:

auto user_info_end = std::find(remaining_parts.begin(), remaining_parts.end(), '@'); if (user_info_end != remaining_parts.end()) { auto password_separator = std::find(remaining_parts.begin(), user_info_end, ':'); if (password_separator != user_info_end) { auto password_start = password_separator + 1; credentials = ProxyCredentials{std::wstring(remaining_parts.begin(), password_separator - remaining_parts.begin()), std::wstring(password_start, user_info_end - password_start)}; } }

Also, you want the character finds (L'@') not string finds (L"@").

BillyONeal · 2024-06-05T18:25:41Z

src/vcpkg/base/downloads.cpp

                std::wstring env_proxy_settings = Strings::to_utf16(*p_https_proxy);
+                ProxyUrlParts parts = parse_proxy_url(env_proxy_settings);


I think it would be a good idea to make the parsing stuff etc. use chars rather than wchar_ts making that parsing code consistent with all other parsing code in the product.

BillyONeal · 2024-06-05T18:26:31Z

src/vcpkg/base/downloads.cpp

+        constexpr auto npos = std::wstring::npos;
+        auto scheme_separator_pos = url.find(L"://");
+        std::wstring scheme;
+        std::wstring_view remaining_parts = url;


Please use StringView rather than std::wstring_view as we don't want to add a dependency on a new C++17 feature in order to have this. (Note that this would also require the char* rather than wchar_t* suggestion below)

BillyONeal · 2024-06-05T18:28:14Z

src/vcpkg/base/downloads.cpp

+        explicit WinHttpContext(const T& value)
+        {
+            T* value_ptr = new T(value);
+            m_value_ptr_ptr = new T*(value_ptr);


This leaks a T if that new throws. Can you just use unique_ptr please?

BillyONeal · 2024-06-05T18:28:56Z

src/vcpkg/base/downloads.cpp

+    // which needs to be provided as a "DWORD_PTR" to WinHttpSendRequest
+    // (pointer to a pointer-sized variable).


DWORD_PTR is a pointer-sized variable itself. It models uintptr_t not uintptr_t*. You should be able to pass T* directly.

BillyONeal · 2024-06-05T18:35:14Z

src/vcpkg/base/downloads.cpp

+                                               WINHTTP_NO_REQUEST_DATA,
+                                               0,
+                                               0,
+                                               ret.m_proxy_credentials.as_dword_ptr());


Given that this data is tied to the lifetime of the request, is there any reason we don't just use an ordinary member variable and pass reinterpret_cast<DWORD_PTR>(this) for that?

Then set_proxy_credentials_on_redirect does reinterpret_cast<WinHttpRequest*>(pContext)->any data you want.

BillyONeal · 2024-06-05T18:38:15Z

src/vcpkg/base/downloads.cpp

+            ProxyCredentials& ctxt = **ctxt_ptr_ptr;
+            WinHttpSetCredentials(hInternet,
+                                  WINHTTP_AUTH_TARGET_PROXY,
+                                  WINHTTP_AUTH_SCHEME_BASIC,


It seems very concerning to me to just arbitrarily staple basic auth creds when a redirect happens, in particular because basic auth is not safe to use without TLS. So if there is TLS for the first request, and that redirects to a not TLS place, stapling creds on again leaked the creds to the whole internet.

This kind of thing makes me extremely nervous to take this change. Perhaps the right behavior is that if we see proxy stuff that looks like it needs auth to always use curl.exe instead...

I understand your concerns, I am clearly not a security expert. For sure relying on a proven solution would be more comfortable.

For the case you describe, I don't think this can happen: from my company network, the redirected request will go through the proxy again (it cannot go to the internet without going through the proxy), and I assume the proxy removes the credentials from the transferred request.

Indeed curl.exe seems to be widely present on windows at least in developer environments (comes with git), so it may be reasonable to rely on it!

We already rely on it all over the place :)

Ok, so I understand that just changing that condition (= use winhttp only if there are no headers, else use curl), to use curl also when we have credentials in the URL, would be the best solution ?

vcpkg-tool/src/vcpkg/base/downloads.cpp

Line 764 in fb6c0b5

if (headers.size() == 0)

See alternative proposition based on this idea in #1434 : we can close this PR if it is indeed a better way to go.

BillyONeal · 2024-07-05T22:58:07Z

Closing this PR because #1434 was merged and fixes the problem. Thanks!

sylvlecl added 5 commits May 19, 2024 18:55

WIP

d9d20a8

Signed-off-by: Sylvain Leclerc <sylvain.leclerc@rte-france.com>

Fix authentication after redirection

e4ac0ef

Signed-off-by: Sylvain Leclerc <sylvain.leclerc@rte-france.com>

Improvements and tests

681cd57

Signed-off-by: Sylvain Leclerc <sylvain.leclerc@rte-france.com>

Fix compiler warning

8a5599d

Signed-off-by: Sylvain Leclerc <sylvain.leclerc@rte-france.com>

Fix formatting

ec2dcc1

Signed-off-by: Sylvain Leclerc <sylvain.leclerc@rte-france.com>

sylvlecl changed the title ~~Http proxy authentication on windows~~ Http proxy basic authentication on windows May 20, 2024

sylvlecl marked this pull request as ready for review May 20, 2024 17:57

BillyONeal reviewed May 28, 2024

View reviewed changes

Re-implement parsing without regex

7781789

Signed-off-by: Sylvain Leclerc <sylvain.leclerc@rte-france.com>

sylvlecl requested a review from BillyONeal June 2, 2024 19:34

BillyONeal reviewed Jun 5, 2024

View reviewed changes

BillyONeal requested changes Jun 5, 2024

View reviewed changes

sylvlecl mentioned this pull request Jun 14, 2024

Use curl for download on windows when proxy auth is required #1434

Merged

BillyONeal closed this Jul 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Http proxy basic authentication on windows #1410

Http proxy basic authentication on windows #1410

sylvlecl commented May 20, 2024 •

edited

Loading

sylvlecl commented May 21, 2024

guilpier-code commented May 22, 2024 •

edited

Loading

BillyONeal May 28, 2024

sylvlecl May 29, 2024

sylvlecl Jun 1, 2024

BillyONeal commented May 28, 2024

BillyONeal Jun 5, 2024

BillyONeal Jun 5, 2024

BillyONeal Jun 5, 2024

BillyONeal Jun 5, 2024

BillyONeal Jun 5, 2024

BillyONeal Jun 5, 2024

BillyONeal Jun 5, 2024

BillyONeal Jun 5, 2024

sylvlecl Jun 6, 2024

sylvlecl Jun 6, 2024

BillyONeal Jun 6, 2024

sylvlecl Jun 9, 2024

sylvlecl Jun 14, 2024

BillyONeal commented Jul 5, 2024

		auto credentials_separator_pos = remaining_parts.find(L"@");
		auto password_separator_pos = remaining_parts.find(L":");

		std::wstring env_proxy_settings = Strings::to_utf16(*p_https_proxy);
		ProxyUrlParts parts = parse_proxy_url(env_proxy_settings);

		// which needs to be provided as a "DWORD_PTR" to WinHttpSendRequest
		// (pointer to a pointer-sized variable).

Http proxy basic authentication on windows #1410

Http proxy basic authentication on windows #1410

Conversation

sylvlecl commented May 20, 2024 • edited Loading

sylvlecl commented May 21, 2024

guilpier-code commented May 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BillyONeal commented May 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BillyONeal commented Jul 5, 2024

sylvlecl commented May 20, 2024 •

edited

Loading

guilpier-code commented May 22, 2024 •

edited

Loading