Fix: use a single canonical URL when referencing

persisted ETags for entities with multiple download URLs.

psiphon/config.go

@@ -716,7 +716,19 @@ func decodeAndValidateDownloadURLs(name string, downloadURLs []*DownloadURL) err
 	return err
 }
 
-func selectDownloadURL(attempt int, downloadURLs []*DownloadURL) (string, bool) {
+func selectDownloadURL(attempt int, downloadURLs []*DownloadURL) (string, string, bool) {
+
+	// The first OnlyAfterAttempts = 0 URL is the canonical URL. This
+	// is the value used as the key for SetUrlETag when multiple download
+	// URLs can be used to fetch a single entity.
+
+	canonicalURL := ""
+	for _, downloadURL := range downloadURLs {
+		if downloadURL.OnlyAfterAttempts == 0 {
+			canonicalURL = downloadURL.URL
+			break
+		}
+	}
 
 	candidates := make([]int, 0)
 	for index, URL := range downloadURLs {
@@ -729,13 +741,14 @@ func selectDownloadURL(attempt int, downloadURLs []*DownloadURL) (string, bool)
 		// This case is not expected, as decodeAndValidateDownloadURLs
 		// should reject configs that would have no candidates for
 		// 0 attempts.
-		return "", true
+		return "", "", true
 	}
 
 	selection, err := common.MakeSecureRandomInt(len(candidates))
 	if err != nil {
 		selection = 0
 	}
+	downloadURL := downloadURLs[candidates[selection]]
 
-	return downloadURLs[candidates[selection]].URL, downloadURLs[candidates[selection]].SkipVerify
+	return downloadURL.URL, canonicalURL, downloadURL.SkipVerify
 }

psiphon/config_test.go

@@ -161,99 +161,106 @@ func (suite *ConfigTestSuite) Test_LoadConfig_GoodJson() {
 
 func TestDownloadURLs(t *testing.T) {
 
-	a := base64.StdEncoding.EncodeToString([]byte("a.example.com"))
-	b := base64.StdEncoding.EncodeToString([]byte("b.example.com"))
-	c := base64.StdEncoding.EncodeToString([]byte("c.example.com"))
+	decodedA := "a.example.com"
+	encodedA := base64.StdEncoding.EncodeToString([]byte(decodedA))
+	encodedB := base64.StdEncoding.EncodeToString([]byte("b.example.com"))
+	encodedC := base64.StdEncoding.EncodeToString([]byte("c.example.com"))
 
 	testCases := []struct {
 		description                string
 		downloadURLs               []*DownloadURL
 		attempts                   int
 		expectedValid              bool
+		expectedCanonicalURL       string
 		expectedDistinctSelections int
 	}{
 		{
 			"missing OnlyAfterAttempts = 0",
 			[]*DownloadURL{
 				&DownloadURL{
-					URL:               a,
+					URL:               encodedA,
 					OnlyAfterAttempts: 1,
 				},
 			},
 			1,
 			false,
+			decodedA,
 			0,
 		},
 		{
 			"single URL, multiple attempts",
 			[]*DownloadURL{
 				&DownloadURL{
-					URL:               a,
+					URL:               encodedA,
 					OnlyAfterAttempts: 0,
 				},
 			},
 			2,
 			true,
+			decodedA,
 			1,
 		},
 		{
 			"multiple URLs, single attempt",
 			[]*DownloadURL{
 				&DownloadURL{
-					URL:               a,
+					URL:               encodedA,
 					OnlyAfterAttempts: 0,
 				},
 				&DownloadURL{
-					URL:               b,
+					URL:               encodedB,
 					OnlyAfterAttempts: 1,
 				},
 				&DownloadURL{
-					URL:               c,
+					URL:               encodedC,
 					OnlyAfterAttempts: 1,
 				},
 			},
 			1,
 			true,
+			decodedA,
 			1,
 		},
 		{
 			"multiple URLs, multiple attempts",
 			[]*DownloadURL{
 				&DownloadURL{
-					URL:               a,
+					URL:               encodedA,
 					OnlyAfterAttempts: 0,
 				},
 				&DownloadURL{
-					URL:               b,
+					URL:               encodedB,
 					OnlyAfterAttempts: 1,
 				},
 				&DownloadURL{
-					URL:               c,
+					URL:               encodedC,
 					OnlyAfterAttempts: 1,
 				},
 			},
 			2,
 			true,
+			decodedA,
 			3,
 		},
 		{
 			"multiple URLs, multiple attempts",
 			[]*DownloadURL{
 				&DownloadURL{
-					URL:               a,
+					URL:               encodedA,
 					OnlyAfterAttempts: 0,
 				},
 				&DownloadURL{
-					URL:               b,
+					URL:               encodedB,
 					OnlyAfterAttempts: 1,
 				},
 				&DownloadURL{
-					URL:               c,
+					URL:               encodedC,
 					OnlyAfterAttempts: 3,
 				},
 			},
 			4,
 			true,
+			decodedA,
 			3,
 		},
 	}
@@ -267,11 +274,11 @@ func TestDownloadURLs(t *testing.T) {
 
 			if testCase.expectedValid {
 				if err != nil {
-					t.Fatal("unexpected validation error: %s", err)
+					t.Fatalf("unexpected validation error: %s", err)
 				}
 			} else {
 				if err == nil {
-					t.Fatal("expected validation error")
+					t.Fatalf("expected validation error")
 				}
 				return
 			}
@@ -289,9 +296,12 @@ func TestDownloadURLs(t *testing.T) {
 
 			attempt := 0
 			for i := 0; i < runs; i++ {
-				url, skipVerify := selectDownloadURL(attempt, testCase.downloadURLs)
+				url, canonicalURL, skipVerify := selectDownloadURL(attempt, testCase.downloadURLs)
+				if canonicalURL != testCase.expectedCanonicalURL {
+					t.Fatalf("unexpected canonical URL: %s", canonicalURL)
+				}
 				if skipVerify {
-					t.Fatal("expected skipVerify")
+					t.Fatalf("expected skipVerify")
 				}
 				attemptDistinctSelections[attempt][url] += 1
 				attempt = (attempt + 1) % testCase.attempts
@@ -305,7 +315,7 @@ func TestDownloadURLs(t *testing.T) {
 			}
 
 			if maxDistinctSelections != testCase.expectedDistinctSelections {
-				t.Fatal("got %d distinct selections, expected %d",
+				t.Fatalf("got %d distinct selections, expected %d",
 					maxDistinctSelections,
 					testCase.expectedDistinctSelections)
 			}

psiphon/remoteServerList.go

@@ -51,13 +51,14 @@ func FetchCommonRemoteServerList(
 
 	NoticeInfo("fetching common remote server list")
 
-	downloadURL, skipVerify := selectDownloadURL(attempt, config.RemoteServerListURLs)
+	downloadURL, canonicalURL, skipVerify := selectDownloadURL(attempt, config.RemoteServerListURLs)
 
 	newETag, err := downloadRemoteServerListFile(
 		config,
 		tunnel,
 		untunneledDialConfig,
 		downloadURL,
+		canonicalURL,
 		skipVerify,
 		"",
 		config.RemoteServerListDownloadFilename)
@@ -82,7 +83,7 @@ func FetchCommonRemoteServerList(
 
 	// Now that the server entries are successfully imported, store the response
 	// ETag so we won't re-download this same data again.
-	err = SetUrlETag(downloadURL, newETag)
+	err = SetUrlETag(canonicalURL, newETag)
 	if err != nil {
 		NoticeAlert("failed to set ETag for common remote server list: %s", common.ContextError(err))
 		// This fetch is still reported as a success, even if we can't store the etag
@@ -112,8 +113,9 @@ func FetchObfuscatedServerLists(
 
 	downloadFilename := osl.GetOSLRegistryFilename(config.ObfuscatedServerListDownloadDirectory)
 
-	rootURL, skipVerify := selectDownloadURL(attempt, config.ObfuscatedServerListRootURLs)
+	rootURL, canonicalRootURL, skipVerify := selectDownloadURL(attempt, config.ObfuscatedServerListRootURLs)
 	downloadURL := osl.GetOSLRegistryURL(rootURL)
+	canonicalURL := osl.GetOSLRegistryURL(canonicalRootURL)
 
 	// failed is set if any operation fails and should trigger a retry. When the OSL registry
 	// fails to download, any cached registry is used instead; when any single OSL fails
@@ -129,6 +131,7 @@ func FetchObfuscatedServerLists(
 		tunnel,
 		untunneledDialConfig,
 		downloadURL,
+		canonicalURL,
 		skipVerify,
 		"",
 		downloadFilename)
@@ -181,7 +184,7 @@ func FetchObfuscatedServerLists(
 	// When a new registry is downloaded, validated, and parsed, store the
 	// response ETag so we won't re-download this same data again.
 	if !failed && newETag != "" {
-		err = SetUrlETag(downloadURL, newETag)
+		err = SetUrlETag(canonicalURL, newETag)
 		if err != nil {
 			NoticeAlert("failed to set ETag for obfuscated server list registry: %s", common.ContextError(err))
 			// This fetch is still reported as a success, even if we can't store the etag
@@ -211,6 +214,7 @@ func FetchObfuscatedServerLists(
 		downloadFilename := osl.GetOSLFilename(config.ObfuscatedServerListDownloadDirectory, oslID)
 
 		downloadURL := osl.GetOSLFileURL(rootURL, oslID)
+		canonicalURL := osl.GetOSLFileURL(canonicalRootURL, oslID)
 
 		hexID := hex.EncodeToString(oslID)
 
@@ -229,6 +233,7 @@ func FetchObfuscatedServerLists(
 			tunnel,
 			untunneledDialConfig,
 			downloadURL,
+			canonicalURL,
 			skipVerify,
 			remoteETag,
 			downloadFilename)
@@ -267,7 +272,7 @@ func FetchObfuscatedServerLists(
 
 		// Now that the server entries are successfully imported, store the response
 		// ETag so we won't re-download this same data again.
-		err = SetUrlETag(downloadURL, newETag)
+		err = SetUrlETag(canonicalURL, newETag)
 		if err != nil {
 			failed = true
 			NoticeAlert("failed to set Etag for obfuscated server list file (%s): %s", hexID, common.ContextError(err))
@@ -293,11 +298,14 @@ func downloadRemoteServerListFile(
 	tunnel *Tunnel,
 	untunneledDialConfig *DialConfig,
 	sourceURL string,
+	canonicalURL string,
 	skipVerify bool,
 	sourceETag string,
 	destinationFilename string) (string, error) {
 
-	lastETag, err := GetUrlETag(sourceURL)
+	// All download URLs with the same canonicalURL
+	// must have the same entity and ETag.
+	lastETag, err := GetUrlETag(canonicalURL)
 	if err != nil {
 		return "", common.ContextError(err)
 	}

psiphon/upgradeDownload.go

@@ -60,6 +60,9 @@ func DownloadUpgrade(
 	tunnel *Tunnel,
 	untunneledDialConfig *DialConfig) error {
 
+	// Note: this downloader doesn't use ETags since many client binaries, with
+	// different embedded values, exist for a single version.
+
 	// Check if complete file already downloaded
 
 	if _, err := os.Stat(config.UpgradeDownloadFilename); err == nil {
@@ -69,7 +72,7 @@ func DownloadUpgrade(
 
 	// Select tunneled or untunneled configuration
 
-	downloadURL, skipVerify := selectDownloadURL(attempt, config.UpgradeDownloadURLs)
+	downloadURL, _, skipVerify := selectDownloadURL(attempt, config.UpgradeDownloadURLs)
 
 	httpClient, requestUrl, err := MakeDownloadHttpClient(
 		config,