[vcpkg] Add sources for TLS 1.2 downloader tool. #15516

BillyONeal · 2021-01-08T06:13:44Z

These are the sources for tls12-download.exe in #15474.

BillyONeal · 2021-01-08T06:42:49Z

This passes in official signed builds but not here because here we try to build the program with the optimizer off, which means the optimizer doesn't eliminate all the references to the CRT, and for some reason a CRT isn't being linked with despite setting MSVC_RUNTIME_LIBRARY to MultiThreaded. Will do some more investigation tomorrow...

…tool

toolsrc/src/tls12-download.c

strega-nil

I've written comments on the C code, but not the winhttp code; I don't understand winhttp, so I don't feel comfortable making suggestions on the code using it (at least without more deep study).

The other thing I'd like to see changed is the camelCase; this project uses snek_case, and I'd like to see this rewritten to use the project's style.

toolsrc/CMakeLists.txt

toolsrc/src/tls12-download.c

ras0219

I think we need to do better proxy handling along the lines of

vcpkg/toolsrc/src/vcpkg/base/downloads.cpp

Lines 95 to 145 in 9c63f97

    
           auto h = WinHttpOpen(L"vcpkg/1.0", 
        
                                IsWindows8Point1OrGreater() ? WINHTTP_ACCESS_TYPE_AUTOMATIC_PROXY 
        
                                                            : WINHTTP_ACCESS_TYPE_DEFAULT_PROXY, 
        
                                WINHTTP_NO_PROXY_NAME, 
        
                                WINHTTP_NO_PROXY_BYPASS, 
        
                                0); 
        
           if (!h) return Strings::concat("WinHttpOpen() failed: ", GetLastError()); 
        
           WinHttpSession ret; 
        
           ret.m_hSession.reset(h); 
        
           // If the environment variable HTTPS_PROXY is set 
        
           // use that variable as proxy. This situation might exist when user is in a company network 
        
           // with restricted network/proxy settings 
        
           auto maybe_https_proxy_env = System::get_environment_variable("HTTPS_PROXY"); 
        
           if (auto p_https_proxy = maybe_https_proxy_env.get()) 
        
           { 
        
               std::wstring env_proxy_settings = Strings::to_utf16(*p_https_proxy); 
        
               WINHTTP_PROXY_INFO proxy; 
        
               proxy.dwAccessType = WINHTTP_ACCESS_TYPE_NAMED_PROXY; 
        
               proxy.lpszProxy = env_proxy_settings.data(); 
        
               proxy.lpszProxyBypass = nullptr; 
        
               WinHttpSetOption(ret.m_hSession.get(), WINHTTP_OPTION_PROXY, &proxy, sizeof(proxy)); 
        
           } 
        
           // Win7 IE Proxy fallback 
        
           else if (IsWindows7OrGreater() && !IsWindows8Point1OrGreater()) 
        
           { 
        
               // First check if any proxy has been found automatically 
        
               WINHTTP_PROXY_INFO proxyInfo; 
        
               DWORD proxyInfoSize = sizeof(WINHTTP_PROXY_INFO); 
        
               auto noProxyFound = 
        
                   !WinHttpQueryOption(ret.m_hSession.get(), WINHTTP_OPTION_PROXY, &proxyInfo, &proxyInfoSize) || 
        
                   proxyInfo.dwAccessType == WINHTTP_ACCESS_TYPE_NO_PROXY; 
        
               // If no proxy was found automatically, use IE's proxy settings, if any 
        
               if (noProxyFound) 
        
               { 
        
                   WINHTTP_CURRENT_USER_IE_PROXY_CONFIG ieProxy; 
        
                   if (WinHttpGetIEProxyConfigForCurrentUser(&ieProxy) && ieProxy.lpszProxy != nullptr) 
        
                   { 
        
                       WINHTTP_PROXY_INFO proxy; 
        
                       proxy.dwAccessType = WINHTTP_ACCESS_TYPE_NAMED_PROXY; 
        
                       proxy.lpszProxy = ieProxy.lpszProxy; 
        
                       proxy.lpszProxyBypass = ieProxy.lpszProxyBypass; 
        
                       WinHttpSetOption(ret.m_hSession.get(), WINHTTP_OPTION_PROXY, &proxy, sizeof(proxy)); 
        
                       GlobalFree(ieProxy.lpszProxy); 
        
                       GlobalFree(ieProxy.lpszProxyBypass); 
        
                       GlobalFree(ieProxy.lpszAutoConfigUrl); 
        
                   } 
        
               } 
        
           }

ras0219 · 2021-01-12T04:01:35Z

toolsrc/src/tls12-download.c

+    }
+
+    write_message(hStdOut, L"\r\n");
+    FlushFileBuffers(hStdOut);


Perhaps it makes sense to add FlushFileBuffers() to win32_abort()?

abort does not traditionally flush buffers; it's called in cases where e.g. memory corruption has occurred and attempting to flush is not safe.

toolsrc/src/tls12-download.c

ras0219 · 2021-01-12T04:13:28Z

toolsrc/src/tls12-download.c

+
+    BOOL results = FALSE;
+    const HINTERNET session = WinHttpOpen(
+        L"tls12-download/1.0", WINHTTP_ACCESS_TYPE_DEFAULT_PROXY, WINHTTP_NO_PROXY_NAME, WINHTTP_NO_PROXY_BYPASS, 0);


We probably need to do

vcpkg/toolsrc/src/vcpkg/base/downloads.cpp

Line 96 in 9c63f97

IsWindows8Point1OrGreater() ? WINHTTP_ACCESS_TYPE_AUTOMATIC_PROXY

Suggested change

L"tls12-download/1.0", WINHTTP_ACCESS_TYPE_DEFAULT_PROXY, WINHTTP_NO_PROXY_NAME, WINHTTP_NO_PROXY_BYPASS, 0);

L"tls12-download/1.0", IsWindows8Point1OrGreater() ? WINHTTP_ACCESS_TYPE_AUTOMATIC_PROXY : WINHTTP_ACCESS_TYPE_DEFAULT_PROXY, WINHTTP_NO_PROXY_NAME, WINHTTP_NO_PROXY_BYPASS, 0);

for proxy support

IsWindows8Point1OrGreater() is false the way this program is built. Certainly for the only platform on which we intend its use.

BillyONeal · 2021-01-12T04:23:09Z

@ras0219 / @ras0219-msft Considering the absolute mess wpad proxy autodetection has been for cpprestsdk I don't think we want to add that; we certainly don't care about wpad on any other platform or anywhere else in vcpkg since we regularly use curl.

BillyONeal · 2021-01-12T04:33:52Z

I note that neither cmake nor curl.exe do WPAD.

strega-nil · 2021-01-12T21:22:51Z

This also needs an update to Format-CxxCode.ps1:

--- a/scripts/azure-pipelines/Format-CxxCode.ps1
+++ b/scripts/azure-pipelines/Format-CxxCode.ps1
@@ -37,5 +37,6 @@ try
 {
     $files = Get-ChildItem -Recurse -LiteralPath "$toolsrc/src" -Filter "*.cpp"
+    $files += Get-ChildItem -Recurse -LiteralPath "$toolsrc/src" -Filter "*.c"
     $files += Get-ChildItem -Recurse -LiteralPath "$toolsrc/include/vcpkg" -Filter '*.h'
     $files += Get-ChildItem -Recurse -LiteralPath "$toolsrc/include/vcpkg-test" -Filter '*.h'
     $files += Get-Item "$toolsrc/include/pch.h"

toolsrc/CMakeLists.txt

strega-nil · 2021-01-12T21:24:54Z

toolsrc/CMakeLists.txt

@@ -198,3 +203,11 @@ if(CLANG_FORMAT)

        COMMAND ${CLANG_FORMAT} -i -verbose ${VCPKG_FUZZ_SOURCES})


Suggested change

COMMAND ${CLANG_FORMAT} -i -verbose ${VCPKG_FUZZ_SOURCES})

COMMAND ${CLANG_FORMAT} -i -verbose ${VCPKG_FUZZ_SOURCES}

COMMAND ${CLANG_FORMAT} -i -verbose ${TLS12_DOWNLOAD_SOURCES})

strega-nil · 2021-01-12T21:26:51Z

toolsrc/src/tls12-download.c

+ * hello world program that does link with the CRT is ~300kb)
+ */
+
+void __declspec(noreturn) win32_abort() {


There's no reason for these to have external linkage; I'd prefer they be marked static.

toolsrc/src/tls12-download.c

strega-nil · 2021-01-12T21:44:51Z

toolsrc/src/tls12-download.c

+    }
+
+    DWORD httpCode = 0;
+    DWORD unused = sizeof(DWORD);


Suggested change

DWORD unused = sizeof(DWORD);

DWORD httpCodeSize = sizeof httpCode;

Alternatively, see next comment.

toolsrc/src/tls12-download.c

strega-nil · 2021-01-12T21:48:23Z

toolsrc/src/tls12-download.c

+
+    if (httpCode != 200)
+    {
+        write_message(stdOut, L"Download failed, server returned HTTP status ");


Suggested change

write_message(stdOut, L"Download failed, server returned HTTP status ");

write_message(stdOut, L"Download failed, server returned HTTP status: ");

ras0219-msft · 2021-01-13T00:18:27Z

I'm ok with skipping WPAD if we add support for %HTTPS_PROXY% (which curl & cmake do support).

[vcpkg] Add sources for TLS 1.2 downloader tool.

11df538

BillyONeal added category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed info:internal This PR or Issue was filed by the vcpkg team. labels Jan 8, 2021

BillyONeal mentioned this pull request Jan 8, 2021

[vcpkg] Download vcpkg.exe rather than building it in bootstrap on Windows. #15474

Merged

BillyONeal added 2 commits January 7, 2021 22:19

Fix directory.

4517725

Always use the release CRT linkage for the downloader tool.

89e4770

BillyONeal added 2 commits January 11, 2021 13:56

Merge remote-tracking branch 'origin/master' into add_tls_downloader_…

ea67ea3

…tool

Fix debug builds of the downloader.

d722eaf

BillyONeal commented Jan 12, 2021

View reviewed changes

toolsrc/src/tls12-download.c Outdated Show resolved Hide resolved

strega-nil suggested changes Jan 12, 2021

View reviewed changes

ras0219 reviewed Jan 12, 2021

View reviewed changes

CR feedback.

1845abd

BillyONeal requested a review from strega-nil January 12, 2021 19:38

strega-nil suggested changes Jan 12, 2021

View reviewed changes

BillyONeal added 2 commits January 12, 2021 20:27

Apply Nicole's CR feedback.

0b4d7f9

Add support for the HTTPS_PROXY environment variable.

daf7e1b

BillyONeal requested a review from strega-nil January 13, 2021 05:16

BillyONeal added 2 commits January 12, 2021 21:18

Fix write_hex bug.

df8f6b2

Use underscore_naming.

1bef6c8

strega-nil approved these changes Jan 13, 2021

View reviewed changes

BillyONeal merged commit 4da47f7 into microsoft:master Jan 13, 2021

BillyONeal deleted the add_tls_downloader_tool branch January 15, 2021 22:18

strega-nil pushed a commit to strega-nil/vcpkg that referenced this pull request May 5, 2021

[vcpkg] Add sources for TLS 1.2 downloader tool. (microsoft#15516)

44b1c9f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[vcpkg] Add sources for TLS 1.2 downloader tool. #15516

[vcpkg] Add sources for TLS 1.2 downloader tool. #15516

BillyONeal commented Jan 8, 2021

BillyONeal commented Jan 8, 2021

strega-nil left a comment

ras0219 left a comment

ras0219 Jan 12, 2021

BillyONeal Jan 12, 2021

ras0219 Jan 12, 2021

BillyONeal Jan 12, 2021

BillyONeal commented Jan 12, 2021

BillyONeal commented Jan 12, 2021

strega-nil commented Jan 12, 2021 •

edited

Loading

strega-nil Jan 12, 2021

strega-nil Jan 12, 2021

strega-nil Jan 12, 2021

strega-nil Jan 12, 2021

strega-nil Jan 12, 2021

ras0219-msft commented Jan 13, 2021

	auto h = WinHttpOpen(L"vcpkg/1.0",
	IsWindows8Point1OrGreater() ? WINHTTP_ACCESS_TYPE_AUTOMATIC_PROXY
	: WINHTTP_ACCESS_TYPE_DEFAULT_PROXY,
	WINHTTP_NO_PROXY_NAME,
	WINHTTP_NO_PROXY_BYPASS,
	0);
	if (!h) return Strings::concat("WinHttpOpen() failed: ", GetLastError());
	WinHttpSession ret;
	ret.m_hSession.reset(h);

	// If the environment variable HTTPS_PROXY is set
	// use that variable as proxy. This situation might exist when user is in a company network
	// with restricted network/proxy settings
	auto maybe_https_proxy_env = System::get_environment_variable("HTTPS_PROXY");
	if (auto p_https_proxy = maybe_https_proxy_env.get())
	{
	std::wstring env_proxy_settings = Strings::to_utf16(*p_https_proxy);
	WINHTTP_PROXY_INFO proxy;
	proxy.dwAccessType = WINHTTP_ACCESS_TYPE_NAMED_PROXY;
	proxy.lpszProxy = env_proxy_settings.data();
	proxy.lpszProxyBypass = nullptr;

	WinHttpSetOption(ret.m_hSession.get(), WINHTTP_OPTION_PROXY, &proxy, sizeof(proxy));
	}
	// Win7 IE Proxy fallback
	else if (IsWindows7OrGreater() && !IsWindows8Point1OrGreater())
	{
	// First check if any proxy has been found automatically
	WINHTTP_PROXY_INFO proxyInfo;
	DWORD proxyInfoSize = sizeof(WINHTTP_PROXY_INFO);
	auto noProxyFound =
	!WinHttpQueryOption(ret.m_hSession.get(), WINHTTP_OPTION_PROXY, &proxyInfo, &proxyInfoSize) \|\|
	proxyInfo.dwAccessType == WINHTTP_ACCESS_TYPE_NO_PROXY;

	// If no proxy was found automatically, use IE's proxy settings, if any
	if (noProxyFound)
	{
	WINHTTP_CURRENT_USER_IE_PROXY_CONFIG ieProxy;
	if (WinHttpGetIEProxyConfigForCurrentUser(&ieProxy) && ieProxy.lpszProxy != nullptr)
	{
	WINHTTP_PROXY_INFO proxy;
	proxy.dwAccessType = WINHTTP_ACCESS_TYPE_NAMED_PROXY;
	proxy.lpszProxy = ieProxy.lpszProxy;
	proxy.lpszProxyBypass = ieProxy.lpszProxyBypass;
	WinHttpSetOption(ret.m_hSession.get(), WINHTTP_OPTION_PROXY, &proxy, sizeof(proxy));
	GlobalFree(ieProxy.lpszProxy);
	GlobalFree(ieProxy.lpszProxyBypass);
	GlobalFree(ieProxy.lpszAutoConfigUrl);
	}
	}
	}

	L"tls12-download/1.0", WINHTTP_ACCESS_TYPE_DEFAULT_PROXY, WINHTTP_NO_PROXY_NAME, WINHTTP_NO_PROXY_BYPASS, 0);
	L"tls12-download/1.0", IsWindows8Point1OrGreater() ? WINHTTP_ACCESS_TYPE_AUTOMATIC_PROXY : WINHTTP_ACCESS_TYPE_DEFAULT_PROXY, WINHTTP_NO_PROXY_NAME, WINHTTP_NO_PROXY_BYPASS, 0);

		@@ -198,3 +203,11 @@ if(CLANG_FORMAT)

		COMMAND ${CLANG_FORMAT} -i -verbose ${VCPKG_FUZZ_SOURCES})

	DWORD unused = sizeof(DWORD);
	DWORD httpCodeSize = sizeof httpCode;

	write_message(stdOut, L"Download failed, server returned HTTP status ");
	write_message(stdOut, L"Download failed, server returned HTTP status: ");

[vcpkg] Add sources for TLS 1.2 downloader tool. #15516

[vcpkg] Add sources for TLS 1.2 downloader tool. #15516

Conversation

BillyONeal commented Jan 8, 2021

BillyONeal commented Jan 8, 2021

strega-nil left a comment

Choose a reason for hiding this comment

ras0219 left a 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 Jan 12, 2021

BillyONeal commented Jan 12, 2021

strega-nil commented Jan 12, 2021 • edited Loading

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

ras0219-msft commented Jan 13, 2021

strega-nil commented Jan 12, 2021 •

edited

Loading