Changes based on feedback

- Merge SecureTransferURL into TransferURL
- Add default feedback encryption key to config
- Break out FeedbackUploadRetryDelaySeconds into a range
- Fix: call cancelFunc() after feedback each feedback upload attempt
- Fix: handle nil config if parsing JSON string fails in PsiphonTunnel.m

MobileLibrary/Android/PsiphonTunnel/PsiphonTunnel.java

@@ -69,11 +69,11 @@ import psi.PsiphonProvider;
 
 public class PsiphonTunnel {
 
-    public interface HostServiceLogger {
+    public interface HostLogger {
         default public void onDiagnosticMessage(String message) {}
     }
 
-    public interface HostService extends HostServiceLogger {
+    public interface HostService extends HostLogger {
 
         public String getAppName();
         public Context getContext();
@@ -299,7 +299,7 @@ public class PsiphonTunnel {
     // information in a particular format, then calls this function to upload it for later
     // investigation. The feedback compatible config and upload path must be provided by
     // Psiphon Inc.
-    public static void sendFeedback(Context context, HostServiceLogger logger, String feedbackConfigJson,
+    public static void sendFeedback(Context context, HostLogger logger, String feedbackConfigJson,
                                     String diagnosticsJson, String uploadPath,
                                     String clientPlatformPrefix, String clientPlatformSuffix) throws Exception {
 
@@ -641,7 +641,7 @@ public class PsiphonTunnel {
                 mLocalSocksProxyPort.get());
     }
 
-    private static String buildPsiphonConfig(Context context, HostServiceLogger logger, String psiphonConfig,
+    private static String buildPsiphonConfig(Context context, HostLogger logger, String psiphonConfig,
                                              String clientPlatformPrefix, String clientPlatformSuffix,
                                              boolean isVpnMode, Integer localSocksProxyPort)
             throws IOException, JSONException, Exception {
@@ -831,7 +831,7 @@ public class PsiphonTunnel {
         }
     }
 
-    private static String setupTrustedCertificates(Context context, HostServiceLogger logger) throws Exception {
+    private static String setupTrustedCertificates(Context context, HostLogger logger) throws Exception {
 
         // Copy the Android system CA store to a local, private cert bundle file.
         //

MobileLibrary/iOS/PsiphonTunnel/PsiphonTunnel/PsiphonTunnel.m

@@ -619,7 +619,10 @@ typedef NS_ERROR_ENUM(PsiphonTunnelErrorDomain, PsiphonTunnelErrorCode) {
         
         id eh = ^(NSError *err) {
             initialConfig = nil;
-            logMessage([NSString stringWithFormat: @"Config JSON parse failed: %@", err.description]);
+            NSString *s = [NSString stringWithFormat:@"Config JSON parse failed: %@", err.description];
+            *outError = [NSError errorWithDomain:PsiphonTunnelErrorDomain
+                                            code:PsiphonTunnelErrorCodeConfigError
+                                        userInfo:@{NSLocalizedDescriptionKey:s}];
         };
         
         id parser = [SBJson4Parser parserWithBlock:block allowMultiRoot:NO unwrapRootArray:NO errorHandler:eh];
@@ -633,11 +636,21 @@ typedef NS_ERROR_ENUM(PsiphonTunnelErrorDomain, PsiphonTunnelErrorCode) {
         *outError = [NSError errorWithDomain:PsiphonTunnelErrorDomain
                                         code:PsiphonTunnelErrorCodeConfigError
                                     userInfo:@{NSLocalizedDescriptionKey:
-                                                   @"getPsiphonConfig should either return an "
+                                                   @"configObject should reference either an "
                                                     "NSDictionary object or an NSString object"}];
         return nil;
     }
-    
+
+    if (*outError != nil) {
+        return nil;
+    } else if (initialConfig == nil) {
+        // Should never happen.
+        *outError = [NSError errorWithDomain:PsiphonTunnelErrorDomain
+                                        code:PsiphonTunnelErrorCodeConfigError
+                                    userInfo:@{NSLocalizedDescriptionKey:@"initialConfig nil"}];
+        return nil;
+    }
+
     NSMutableDictionary *config = [NSMutableDictionary dictionaryWithDictionary:initialConfig];
     
     //

psiphon/common/parameters/clientParameters.go

@@ -238,9 +238,11 @@ const (
 	BPFClientTCPProgram                              = "BPFClientTCPProgram"
 	BPFClientTCPProbability                          = "BPFClientTCPProbability"
 	FeedbackUploadURLs                               = "FeedbackUploadURLs"
+	FeedbackEncryptionPublicKey                      = "FeedbackEncryptionPublicKey"
 	FeedbackTacticsWaitPeriod                        = "FeedbackTacticsWaitPeriod"
 	FeedbackUploadMaxRetries                         = "FeedbackUploadMaxRetries"
-	FeedbackUploadRetryDelaySeconds                  = "FeedbackUploadRetryDelaySeconds"
+	FeedbackUploadRetryMinDelaySeconds               = "FeedbackUploadRetryMinDelaySeconds"
+	FeedbackUploadRetryMaxDelaySeconds               = "FeedbackUploadRetryMaxDelaySeconds"
 	FeedbackUploadTimeoutSeconds                     = "FeedbackUploadTimeoutSeconds"
 )
 
@@ -497,11 +499,13 @@ var defaultClientParameters = map[string]struct {
 	BPFClientTCPProgram:     {value: (*BPFProgramSpec)(nil)},
 	BPFClientTCPProbability: {value: 0.5, minimum: 0.0},
 
-	FeedbackUploadURLs:              {value: SecureTransferURLs{}},
-	FeedbackTacticsWaitPeriod:       {value: 5 * time.Second, minimum: 0 * time.Second, flags: useNetworkLatencyMultiplier},
-	FeedbackUploadMaxRetries:        {value: 5, minimum: 0},
-	FeedbackUploadRetryDelaySeconds: {value: 300 * time.Second, minimum: 0 * time.Second, flags: useNetworkLatencyMultiplier},
-	FeedbackUploadTimeoutSeconds:    {value: 30 * time.Second, minimum: 0 * time.Second, flags: useNetworkLatencyMultiplier},
+	FeedbackUploadURLs:                 {value: TransferURLs{}},
+	FeedbackEncryptionPublicKey:        {value: ""},
+	FeedbackTacticsWaitPeriod:          {value: 5 * time.Second, minimum: 0 * time.Second, flags: useNetworkLatencyMultiplier},
+	FeedbackUploadMaxRetries:           {value: 5, minimum: 0},
+	FeedbackUploadRetryMinDelaySeconds: {value: 1 * time.Minute, minimum: time.Duration(0), flags: useNetworkLatencyMultiplier},
+	FeedbackUploadRetryMaxDelaySeconds: {value: 5 * time.Minute, minimum: 1 * time.Second, flags: useNetworkLatencyMultiplier},
+	FeedbackUploadTimeoutSeconds:       {value: 30 * time.Second, minimum: 0 * time.Second, flags: useNetworkLatencyMultiplier},
 }
 
 // IsServerSideOnly indicates if the parameter specified by name is used
@@ -674,14 +678,6 @@ func (p *ClientParameters) Set(
 					}
 					return nil, errors.Trace(err)
 				}
-			case SecureTransferURLs:
-				err := v.DecodeAndValidate()
-				if err != nil {
-					if skipOnError {
-						continue
-					}
-					return nil, errors.Trace(err)
-				}
 			case protocol.TunnelProtocols:
 				if skipOnError {
 					newValue = v.PruneInvalid()
@@ -1105,13 +1101,6 @@ func (p ClientParametersAccessor) TransferURLs(name string) TransferURLs {
 	return value
 }
 
-// SecureTransferURLs returns a SecureTransferURLs parameter value.
-func (p ClientParametersAccessor) SecureTransferURLs(name string) SecureTransferURLs {
-	value := SecureTransferURLs{}
-	p.snapshot.getValue(name, &value)
-	return value
-}
-
 // RateLimits returns a common.RateLimits parameter value.
 func (p ClientParametersAccessor) RateLimits(name string) common.RateLimits {
 	value := common.RateLimits{}

psiphon/common/parameters/transferURLs.go

@@ -44,6 +44,16 @@ type TransferURL struct {
 	// candidate locations. For a value of N, this URL is only a candidate
 	// after N rounds of attempting the transfer to or from other URLs.
 	OnlyAfterAttempts int
+
+	// B64EncodedPublicKey is a base64-encoded RSA public key to be used for
+	// encrypting the resource, when uploading, or for verifying a signature of
+	// the resource, when downloading. Required by some operations, such as
+	// uploading feedback.
+	B64EncodedPublicKey string `json:",omitempty"`
+
+	// RequestHeaders are optional HTTP headers to set on any requests made to
+	// the destination.
+	RequestHeaders map[string]string `json:",omitempty"`
 }
 
 // TransferURLs is a list of transfer URLs.
@@ -75,96 +85,32 @@ func (t TransferURLs) DecodeAndValidate() error {
 	return nil
 }
 
-// Select chooses a TransferURL from the list.
-//
-// The first return value is the canonical URL, to be used
-// as a key when storing information related to the TransferURLs,
-// such as an ETag.
-//
-// The second return value is the chosen transfer URL, which is
-// selected based at random from the candidates allowed in the
-// specified attempt.
-func (t TransferURLs) Select(attempt int) (string, string, bool) {
+// CanonicalURL returns the canonical URL, to be used as a key when storing
+// information related to the TransferURLs, such as an ETag.
+func (t TransferURLs) CanonicalURL() string {
 
 	// 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 _, transferURL := range t {
 		if transferURL.OnlyAfterAttempts == 0 {
-			canonicalURL = transferURL.URL
-			break
+			return transferURL.URL
 		}
 	}
 
-	candidates := make([]int, 0)
-	for index, URL := range t {
-		if attempt >= URL.OnlyAfterAttempts {
-			candidates = append(candidates, index)
-		}
-	}
-
-	if len(candidates) < 1 {
-		// This case is not expected, as DecodeAndValidate should reject configs
-		// that would have no candidates for 0 attempts.
-		return "", "", true
-	}
-
-	selection := prng.Intn(len(candidates))
-	transferURL := t[candidates[selection]]
-
-	return transferURL.URL, canonicalURL, transferURL.SkipVerify
-}
-
-// SecureTransferURL specifies a URL for uploading or downloading a resource
-// along with a public key for encrypting the resource, when uploading, or
-// for verifying a signature of the resource, when downloading.
-type SecureTransferURL struct {
-	B64EncodedPublicKey string
-	RequestHeaders      map[string]string
-	TransferURL         TransferURL
-}
-
-// SecureTransferURLs is a list of secure transfer URLs.
-type SecureTransferURLs []*SecureTransferURL
-
-// TransferURLs returns the underlying TransferURLs with ordering preserved.
-func (t SecureTransferURLs) TransferURLs() (TransferURLs, error) {
-	transferURLs := make(TransferURLs, len(t))
-	for i, secureTransferURL := range t {
-		if secureTransferURL == nil {
-			return nil, errors.TraceNew("unexpected nil SecureTransferURL")
-		}
-		transferURLs[i] = &secureTransferURL.TransferURL
-	}
-	return transferURLs, nil
-}
-
-// DecodeAndValidate validates a list of secure transfer URLs.
-func (t SecureTransferURLs) DecodeAndValidate() error {
-	transferURLs, err := t.TransferURLs()
-	if err != nil {
-		return errors.Trace(err)
-	}
-	err = transferURLs.DecodeAndValidate()
-	if err != nil {
-		return errors.Trace(err)
-	}
-	return nil
+	return ""
 }
 
-// Select chooses a SecureTransferURL from the list.
+// Select chooses a TransferURL from the list.
 //
-// The secure transfer URL is selected based at random from the candidates
-// allowed in the specified attempt.
-func (t SecureTransferURLs) Select(attempt int) (*SecureTransferURL, error) {
+// The TransferURL is selected based at random from the candidates allowed in
+// the specified attempt.
+func (t TransferURLs) Select(attempt int) *TransferURL {
+
 	candidates := make([]int, 0)
-	for index, secureTransferURL := range t {
-		if secureTransferURL == nil {
-			return nil, errors.TraceNew("unexpected nil SecureTransferURL")
-		}
-		if attempt >= secureTransferURL.TransferURL.OnlyAfterAttempts {
+	for index, URL := range t {
+		if attempt >= URL.OnlyAfterAttempts {
 			candidates = append(candidates, index)
 		}
 	}
@@ -172,11 +118,11 @@ func (t SecureTransferURLs) Select(attempt int) (*SecureTransferURL, error) {
 	if len(candidates) < 1 {
 		// This case is not expected, as DecodeAndValidate should reject configs
 		// that would have no candidates for 0 attempts.
-		return nil, errors.TraceNew("no candiates found")
+		return nil
 	}
 
 	selection := prng.Intn(len(candidates))
 	transferURL := t[candidates[selection]]
 
-	return transferURL, nil
+	return transferURL
 }

psiphon/common/parameters/transferURLs_test.go

@@ -159,292 +159,15 @@ func TestTransferURLs(t *testing.T) {
 
 			attempt := 0
 			for i := 0; i < runs; i++ {
-				url, canonicalURL, skipVerify := testCase.transferURLs.Select(attempt)
+				canonicalURL := testCase.transferURLs.CanonicalURL()
 				if canonicalURL != testCase.expectedCanonicalURL {
 					t.Fatalf("unexpected canonical URL: %s", canonicalURL)
 				}
-				if skipVerify {
+				transferUrl := testCase.transferURLs.Select(attempt)
+				if transferUrl.SkipVerify {
 					t.Fatalf("unexpected skipVerify")
 				}
-				attemptDistinctSelections[attempt][url] += 1
-				attempt = (attempt + 1) % testCase.attempts
-			}
-
-			maxDistinctSelections := 0
-			for _, m := range attemptDistinctSelections {
-				if len(m) > maxDistinctSelections {
-					maxDistinctSelections = len(m)
-				}
-			}
-
-			if maxDistinctSelections != testCase.expectedDistinctSelections {
-				t.Fatalf("got %d distinct selections, expected %d",
-					maxDistinctSelections,
-					testCase.expectedDistinctSelections)
-			}
-		})
-	}
-
-}
-
-func TestSecureTransferURLs(t *testing.T) {
-
-	decodedA := "a.example.com"
-	encodedA := base64.StdEncoding.EncodeToString([]byte(decodedA))
-	decodedB := "b.example.com"
-	encodedB := base64.StdEncoding.EncodeToString([]byte(decodedB))
-	decodedC := "c.example.com"
-	encodedC := base64.StdEncoding.EncodeToString([]byte(decodedC))
-
-	type secureTransferURLSubTest struct {
-		secureTransferURL  *SecureTransferURL
-		expectedDecodedURL string
-	}
-
-	testCases := []struct {
-		description                string
-		secureTransferURLs         []secureTransferURLSubTest
-		attempts                   int
-		expectedValid              bool
-		expectedDistinctSelections int
-	}{
-		{
-			"missing OnlyAfterAttempts = 0",
-			[]secureTransferURLSubTest{
-				{
-					&SecureTransferURL{
-						TransferURL: TransferURL{
-							URL:               encodedA,
-							OnlyAfterAttempts: 1,
-						},
-						RequestHeaders:      nil,
-						B64EncodedPublicKey: "",
-					},
-					decodedA,
-				},
-			},
-			1,
-			false,
-			0,
-		},
-		{
-			"contains nil SecureTransferURL",
-			[]secureTransferURLSubTest{
-				{
-					&SecureTransferURL{
-						TransferURL: TransferURL{
-							URL:               encodedA,
-							OnlyAfterAttempts: 0,
-						},
-						RequestHeaders:      nil,
-						B64EncodedPublicKey: "",
-					},
-					decodedA,
-				},
-				{
-					nil,
-					decodedA,
-				},
-			},
-			1,
-			false,
-			0,
-		},
-		{
-			"single URL, multiple attempts",
-			[]secureTransferURLSubTest{
-				{
-					&SecureTransferURL{
-						TransferURL: TransferURL{
-							URL:               encodedA,
-							OnlyAfterAttempts: 0,
-						},
-						RequestHeaders:      nil,
-						B64EncodedPublicKey: "",
-					},
-					decodedA,
-				},
-			},
-			2,
-			true,
-			1,
-		},
-		{
-			"multiple URLs, single attempt",
-			[]secureTransferURLSubTest{
-				{
-					&SecureTransferURL{
-						TransferURL: TransferURL{
-							URL:               encodedA,
-							OnlyAfterAttempts: 0,
-						},
-						RequestHeaders:      nil,
-						B64EncodedPublicKey: "",
-					},
-					decodedA,
-				},
-				{
-					&SecureTransferURL{
-						TransferURL: TransferURL{
-							URL:               encodedB,
-							OnlyAfterAttempts: 1,
-						},
-						RequestHeaders:      nil,
-						B64EncodedPublicKey: "",
-					},
-					decodedB,
-				},
-				{
-					&SecureTransferURL{
-						TransferURL: TransferURL{
-							URL:               encodedC,
-							OnlyAfterAttempts: 1,
-						},
-						RequestHeaders:      nil,
-						B64EncodedPublicKey: "",
-					},
-					decodedC,
-				},
-			},
-			1,
-			true,
-			1,
-		},
-		{
-			"multiple URLs, multiple attempts",
-			[]secureTransferURLSubTest{
-				{
-					&SecureTransferURL{
-						TransferURL: TransferURL{
-							URL:               encodedA,
-							OnlyAfterAttempts: 0,
-						},
-						RequestHeaders:      nil,
-						B64EncodedPublicKey: "",
-					},
-					decodedA,
-				},
-				{
-					&SecureTransferURL{
-						TransferURL: TransferURL{
-							URL:               encodedB,
-							OnlyAfterAttempts: 1,
-						},
-						RequestHeaders:      nil,
-						B64EncodedPublicKey: "",
-					},
-					decodedB,
-				},
-				{
-					&SecureTransferURL{
-						TransferURL: TransferURL{
-							URL:               encodedC,
-							OnlyAfterAttempts: 1,
-						},
-						RequestHeaders:      nil,
-						B64EncodedPublicKey: "",
-					},
-					decodedC,
-				},
-			},
-			2,
-			true,
-			3,
-		},
-		{
-			"multiple URLs, multiple attempts",
-			[]secureTransferURLSubTest{
-				{
-					&SecureTransferURL{
-						TransferURL: TransferURL{
-							URL:               encodedA,
-							OnlyAfterAttempts: 0,
-						},
-						RequestHeaders:      nil,
-						B64EncodedPublicKey: "",
-					},
-					decodedA,
-				},
-				{
-					&SecureTransferURL{
-						TransferURL: TransferURL{
-							URL:               encodedB,
-							OnlyAfterAttempts: 1,
-						},
-						RequestHeaders:      nil,
-						B64EncodedPublicKey: "",
-					},
-					decodedB,
-				},
-				{
-					&SecureTransferURL{
-						TransferURL: TransferURL{
-							URL:               encodedC,
-							OnlyAfterAttempts: 3,
-						},
-						RequestHeaders:      nil,
-						B64EncodedPublicKey: "",
-					},
-					decodedC,
-				},
-			},
-			4,
-			true,
-			3,
-		},
-	}
-
-	for _, testCase := range testCases {
-		t.Run(testCase.description, func(t *testing.T) {
-
-			// Construct list of SecureTransferURLs.
-			secureTransferURLs := make(SecureTransferURLs, len(testCase.secureTransferURLs))
-			for i, subTest := range testCase.secureTransferURLs {
-				secureTransferURLs[i] = subTest.secureTransferURL
-			}
-
-			err := secureTransferURLs.DecodeAndValidate()
-
-			if testCase.expectedValid {
-				if err != nil {
-					t.Fatalf("unexpected validation error: %s", err)
-				}
-			} else {
-				if err == nil {
-					t.Fatalf("expected validation error")
-				}
-				return
-			}
-
-			// Check URLs are decoded as expected.
-			for _, subTest := range testCase.secureTransferURLs {
-				if subTest.expectedDecodedURL != subTest.secureTransferURL.TransferURL.URL {
-					t.Fatalf("unexpected URL: %s", subTest.secureTransferURL.TransferURL.URL)
-				}
-			}
-
-			// Track distinct selections for each attempt; the
-			// expected number of distinct should be for at least
-			// one particular attempt.
-			attemptDistinctSelections := make(map[int]map[string]int)
-			for i := 0; i < testCase.attempts; i++ {
-				attemptDistinctSelections[i] = make(map[string]int)
-			}
-
-			// Perform enough runs to account for random selection.
-			runs := 1000
-
-			attempt := 0
-			for i := 0; i < runs; i++ {
-				secureTransferURL, err := secureTransferURLs.Select(attempt)
-				if err != nil {
-					// Error should have been caught by DecodeAndVerify.
-					t.Fatalf("unexpected Select error")
-				}
-				if secureTransferURL.TransferURL.SkipVerify {
-					t.Fatalf("unexpected skipVerify")
-				}
-				attemptDistinctSelections[attempt][secureTransferURL.TransferURL.URL] += 1
+				attemptDistinctSelections[attempt][transferUrl.URL] += 1
 				attempt = (attempt + 1) % testCase.attempts
 			}
 

psiphon/config.go

@@ -709,7 +709,13 @@ type Config struct {
 	// value is supplied by and depends on the Psiphon Network, and is
 	// typically embedded in the client binary. At least one TransferURL must
 	// have OnlyAfterAttempts = 0.
-	FeedbackUploadURLs parameters.SecureTransferURLs
+	FeedbackUploadURLs parameters.TransferURLs
+
+	// FeedbackEncryptionPublicKey is a default base64-encoded, RSA public key
+	// value used to encrypt feedback data. Used when uploading feedback with a
+	// TransferURL which has no public key value configured, i.e.
+	// B64EncodedPublicKey = "".
+	FeedbackEncryptionPublicKey string
 
 	// clientParameters is the active ClientParameters with defaults, config
 	// values, and, optionally, tactics applied.
@@ -1027,6 +1033,12 @@ func (config *Config) Commit(migrateFromLegacyFields bool) error {
 		}
 	}
 
+	if config.FeedbackUploadURLs != nil {
+		if config.FeedbackEncryptionPublicKey == "" {
+			return errors.TraceNew("missing FeedbackEncryptionPublicKey")
+		}
+	}
+
 	// This constraint is expected by logic in Controller.runTunnels().
 
 	if config.PacketTunnelTunFileDescriptor > 0 && config.TunnelPoolSize != 1 {
@@ -1576,6 +1588,10 @@ func (config *Config) makeConfigParameters() map[string]interface{} {
 		applyParameters[parameters.FeedbackUploadURLs] = config.FeedbackUploadURLs
 	}
 
+	if config.FeedbackEncryptionPublicKey != "" {
+		applyParameters[parameters.FeedbackEncryptionPublicKey] = config.FeedbackEncryptionPublicKey
+	}
+
 	return applyParameters
 }
 

psiphon/feedback.go

@@ -33,6 +33,8 @@ import (
 	"encoding/base64"
 	"encoding/json"
 	"net/http"
+	"net/url"
+	"path"
 	"time"
 
 	"github.com/Psiphon-Labs/psiphon-tunnel-core/psiphon/common"
@@ -118,10 +120,11 @@ func SendFeedback(configJson, diagnosticsJson, uploadPath string) error {
 
 	// Get the latest client parameters
 	p = config.GetClientParameters().Get()
-	feedbackUploadRetryDelay := p.Duration(parameters.FeedbackUploadRetryDelaySeconds)
+	feedbackUploadMinRetryDelay := p.Duration(parameters.FeedbackUploadRetryMinDelaySeconds)
+	feedbackUploadMaxRetryDelay := p.Duration(parameters.FeedbackUploadRetryMaxDelaySeconds)
 	feedbackUploadTimeout := p.Duration(parameters.FeedbackUploadTimeoutSeconds)
 	feedbackUploadMaxRetries := p.Int(parameters.FeedbackUploadMaxRetries)
-	secureTransferURLs := p.SecureTransferURLs(parameters.FeedbackUploadURLs)
+	transferURLs := p.TransferURLs(parameters.FeedbackUploadURLs)
 	p.Close()
 
 	untunneledDialConfig := &DialConfig{
@@ -134,15 +137,27 @@ func SendFeedback(configJson, diagnosticsJson, uploadPath string) error {
 	}
 
 	uploadId := prng.HexString(8)
+	PRNG, err := prng.NewPRNG()
+	if err != nil {
+		return errors.TraceMsg(err, "failed to create PRNG")
+	}
 
 	for i := 0; i < feedbackUploadMaxRetries; i++ {
 
-		secureTransferURL, err := secureTransferURLs.Select(i)
-		if err != nil {
-			return errors.Tracef("Error selecting feedback transfer URL: %s", err)
+		uploadURL := transferURLs.Select(i)
+		if uploadURL == nil {
+			return errors.TraceNew("error no feedback upload URL selected")
 		}
 
-		secureFeedback, err := encryptFeedback(diagnosticsJson, secureTransferURL.B64EncodedPublicKey)
+		b64PublicKey := uploadURL.B64EncodedPublicKey
+		if b64PublicKey == "" {
+			if config.FeedbackEncryptionPublicKey == "" {
+				return errors.TraceNew("error no default encryption key")
+			}
+			b64PublicKey = config.FeedbackEncryptionPublicKey
+		}
+
+		secureFeedback, err := encryptFeedback(diagnosticsJson, b64PublicKey)
 		if err != nil {
 			return errors.Trace(err)
 		}
@@ -157,28 +172,35 @@ func SendFeedback(configJson, diagnosticsJson, uploadPath string) error {
 			config,
 			untunneledDialConfig,
 			nil,
-			secureTransferURL.TransferURL.SkipVerify)
+			uploadURL.SkipVerify)
 		if err != nil {
 			return errors.Trace(err)
 		}
 
-		url := secureTransferURL.TransferURL.URL + uploadPath + uploadId
+		parsedURL, err := url.Parse(uploadURL.URL)
+		if err != nil {
+			return errors.TraceMsg(err, "failed to parse feedback upload URL")
+		}
+
+		parsedURL.Path = path.Join(parsedURL.Path, uploadPath, uploadId)
 
-		req, err := http.NewRequest("PUT", url, bytes.NewBuffer(secureFeedback))
+		request, err := http.NewRequestWithContext(ctx, "PUT", parsedURL.String(), bytes.NewBuffer(secureFeedback))
 		if err != nil {
 			return errors.Trace(err)
 		}
 
-		for k, v := range secureTransferURL.RequestHeaders {
-			req.Header.Set(k, v)
+		for k, v := range uploadURL.RequestHeaders {
+			request.Header.Set(k, v)
 		}
-		req.Header.Set("User-Agent", MakePsiphonUserAgent(config))
+		request.Header.Set("User-Agent", MakePsiphonUserAgent(config))
 
-		err = uploadFeedback(client, req)
+		err = uploadFeedback(client, request)
+		cancelFunc()
 		if err != nil {
 			// Do not sleep after the last attempt
 			if i+1 < feedbackUploadMaxRetries {
-				time.Sleep(feedbackUploadRetryDelay)
+				retryDelay := PRNG.Period(feedbackUploadMinRetryDelay, feedbackUploadMaxRetryDelay)
+				time.Sleep(retryDelay)
 			}
 		} else {
 			break
@@ -199,7 +221,7 @@ func uploadFeedback(
 	defer resp.Body.Close()
 
 	if resp.StatusCode != http.StatusOK {
-		return errors.TraceNew("Unexpected HTTP status: " + resp.Status)
+		return errors.TraceNew("unexpected HTTP status: " + resp.Status)
 	}
 
 	return nil

psiphon/remoteServerList.go

@@ -59,7 +59,8 @@ func FetchCommonRemoteServerList(
 	downloadTimeout := p.Duration(parameters.FetchRemoteServerListTimeout)
 	p.Close()
 
-	downloadURL, canonicalURL, skipVerify := urls.Select(attempt)
+	downloadURL := urls.Select(attempt)
+	canonicalURL := urls.CanonicalURL()
 
 	newETag, downloadStatRecorder, err := downloadRemoteServerListFile(
 		ctx,
@@ -67,9 +68,9 @@ func FetchCommonRemoteServerList(
 		tunnel,
 		untunneledDialConfig,
 		downloadTimeout,
-		downloadURL,
+		downloadURL.URL,
 		canonicalURL,
-		skipVerify,
+		downloadURL.SkipVerify,
 		"",
 		config.GetRemoteServerListDownloadFilename())
 	if err != nil {
@@ -150,8 +151,9 @@ func FetchObfuscatedServerLists(
 	downloadTimeout := p.Duration(parameters.FetchRemoteServerListTimeout)
 	p.Close()
 
-	rootURL, canonicalRootURL, skipVerify := urls.Select(attempt)
-	downloadURL := osl.GetOSLRegistryURL(rootURL)
+	rootURL := urls.Select(attempt)
+	canonicalRootURL := urls.CanonicalURL()
+	downloadURL := osl.GetOSLRegistryURL(rootURL.URL)
 	canonicalURL := osl.GetOSLRegistryURL(canonicalRootURL)
 
 	downloadFilename := osl.GetOSLRegistryFilename(config.GetObfuscatedServerListDownloadDirectory())
@@ -185,7 +187,7 @@ func FetchObfuscatedServerLists(
 		downloadTimeout,
 		downloadURL,
 		canonicalURL,
-		skipVerify,
+		rootURL.SkipVerify,
 		"",
 		downloadFilename)
 	if err != nil {
@@ -261,9 +263,9 @@ func FetchObfuscatedServerLists(
 			tunnel,
 			untunneledDialConfig,
 			downloadTimeout,
-			rootURL,
+			rootURL.URL,
 			canonicalRootURL,
-			skipVerify,
+			rootURL.SkipVerify,
 			publicKey,
 			lookupSLOKs,
 			oslFileSpec) {

psiphon/upgradeDownload.go

@@ -84,14 +84,14 @@ func DownloadUpgrade(
 
 	// Select tunneled or untunneled configuration
 
-	downloadURL, _, skipVerify := urls.Select(attempt)
+	downloadURL := urls.Select(attempt)
 
 	httpClient, _, err := MakeDownloadHTTPClient(
 		ctx,
 		config,
 		tunnel,
 		untunneledDialConfig,
-		skipVerify)
+		downloadURL.SkipVerify)
 	if err != nil {
 		return errors.Trace(err)
 	}
@@ -102,7 +102,7 @@ func DownloadUpgrade(
 	availableClientVersion := handshakeVersion
 	if availableClientVersion == "" {
 
-		request, err := http.NewRequest("HEAD", downloadURL, nil)
+		request, err := http.NewRequest("HEAD", downloadURL.URL, nil)
 		if err != nil {
 			return errors.Trace(err)
 		}
@@ -159,7 +159,7 @@ func DownloadUpgrade(
 	n, _, err := ResumeDownload(
 		ctx,
 		httpClient,
-		downloadURL,
+		downloadURL.URL,
 		MakePsiphonUserAgent(config),
 		downloadFilename,
 		"")