Merge pull request #690 from mirokuratczyk/master

Extend SkipVerify support to TransferURLs

psiphon/common/parameters/transferURLs.go

@@ -74,10 +74,9 @@ func (t TransferURLs) DecodeAndValidate() error {
 	hasOnlyAfterZero := false
 	for _, transferURL := range t {
 
-		// Currently, TransferURL FrontingSpecs are not permitted to specify
-		// SkipVerify as psiphon.makeFrontedHTTPClient uses
-		// MeekModePlaintextRoundTrip.
-		allowSkipVerify := false
+		// TransferURL FrontingSpecs are permitted to specify SkipVerify
+		// because transfers have additional security at the payload level.
+		allowSkipVerify := true
 		err := transferURL.FrontingSpecs.Validate(allowSkipVerify)
 		if err != nil {
 			return errors.Trace(err)

psiphon/common/parameters/transferURLs_test.go

@@ -65,6 +65,49 @@ func TestTransferURLs(t *testing.T) {
 			decodedA,
 			1,
 		},
+		{
+			"single URL, multiple attempts, fronting spec",
+			TransferURLs{
+				{
+					URL:               encodedA,
+					OnlyAfterAttempts: 0,
+					FrontingSpecs: []*FrontingSpec{
+						{
+							FrontingProviderID: "frontingProvider",
+							Addresses:          []string{"example.org"},
+							VerifyServerName:   "example.com",
+							Host:               "example.org",
+							SkipVerify:         false,
+						},
+					},
+				},
+			},
+			2,
+			true,
+			decodedA,
+			1,
+		},
+		{
+			"single URL, multiple attempts, fronting spec, skip verify set",
+			TransferURLs{
+				{
+					URL:               encodedA,
+					OnlyAfterAttempts: 0,
+					FrontingSpecs: []*FrontingSpec{
+						{
+							FrontingProviderID: "frontingProvider",
+							Addresses:          []string{"example.org"},
+							Host:               "example.org",
+							SkipVerify:         true,
+						},
+					},
+				},
+			},
+			2,
+			true,
+			decodedA,
+			1,
+		},
 		{
 			"multiple URLs, single attempt",
 			TransferURLs{

psiphon/feedback.go

@@ -204,12 +204,14 @@ func SendFeedback(ctx context.Context, config *Config, diagnostics, uploadPath s
 			feedbackUploadTimeout)
 		defer cancelFunc()
 
+		payloadSecure := true
 		client, _, err := MakeUntunneledHTTPClient(
 			feedbackUploadCtx,
 			config,
 			untunneledDialConfig,
 			uploadURL.SkipVerify,
 			config.DisableSystemRootCAs,
+			payloadSecure,
 			uploadURL.FrontingSpecs,
 			func(frontingProviderID string) {
 				NoticeInfo(

psiphon/meekConn_test.go

@@ -57,50 +57,72 @@ func TestMeekModePlaintextRoundTrip(t *testing.T) {
 		t.Fatalf("parameters.NewParameters failed: %v", err)
 	}
 
-	meekConfig := &MeekConfig{
-		Parameters:       params,
-		Mode:             MeekModePlaintextRoundTrip,
-		DialAddress:      serverAddr,
-		UseHTTPS:         true,
-		SNIServerName:    "not-" + serverName,
-		VerifyServerName: serverName,
-		VerifyPins:       []string{rootCACertificatePin, serverCertificatePin},
+	testCases := []struct {
+		description      string
+		meekMode         MeekMode
+		verifyServerName string
+		verifyPins       []string
+	}{
+		{
+			meekMode:         MeekModePlaintextRoundTrip,
+			verifyServerName: serverName,
+			verifyPins:       []string{rootCACertificatePin, serverCertificatePin},
+		},
+		{
+			meekMode:         MeekModeWrappedPlaintextRoundTrip,
+			verifyServerName: "",
+			verifyPins:       nil,
+		},
 	}
 
-	dialConfig := &DialConfig{
-		TrustedCACertificatesFilename: rootCAsFileName,
-		CustomDialer:                  dialer,
-	}
-
-	for _, tlsFragmentClientHello := range []bool{false, true} {
-
-		ctx, cancelFunc := context.WithTimeout(context.Background(), 1*time.Second)
-		defer cancelFunc()
-
-		meekConfig.TLSFragmentClientHello = tlsFragmentClientHello
-
-		meekConn, err := DialMeek(ctx, meekConfig, dialConfig)
-		if err != nil {
-			t.Fatalf("DialMeek failed: %v", err)
-		}
-
-		client := &http.Client{
-			Transport: meekConn,
-		}
-
-		response, err := client.Get("https://" + serverAddr + "/")
-		if err != nil {
-			t.Fatalf("http.Client.Get failed: %v", err)
-		}
-		response.Body.Close()
-
-		if response.StatusCode != http.StatusOK {
-			t.Fatalf("unexpected response code: %v", response.StatusCode)
-		}
-
-		err = meekConn.Close()
-		if err != nil {
-			t.Fatalf("MeekConn.Close failed: %v", err)
-		}
+	for _, testCase := range testCases {
+		t.Run(testCase.description, func(t *testing.T) {
+			meekConfig := &MeekConfig{
+				Parameters:       params,
+				Mode:             testCase.meekMode,
+				DialAddress:      serverAddr,
+				UseHTTPS:         true,
+				SNIServerName:    "not-" + serverName,
+				VerifyServerName: testCase.verifyServerName,
+				VerifyPins:       testCase.verifyPins,
+			}
+
+			dialConfig := &DialConfig{
+				TrustedCACertificatesFilename: rootCAsFileName,
+				CustomDialer:                  dialer,
+			}
+
+			for _, tlsFragmentClientHello := range []bool{false, true} {
+
+				ctx, cancelFunc := context.WithTimeout(context.Background(), 1*time.Second)
+				defer cancelFunc()
+
+				meekConfig.TLSFragmentClientHello = tlsFragmentClientHello
+
+				meekConn, err := DialMeek(ctx, meekConfig, dialConfig)
+				if err != nil {
+					t.Fatalf("DialMeek failed: %v", err)
+				}
+
+				client := &http.Client{
+					Transport: meekConn,
+				}
+
+				response, err := client.Get("https://" + serverAddr + "/")
+				if err != nil {
+					t.Fatalf("http.Client.Get failed: %v", err)
+				}
+				response.Body.Close()
+
+				if response.StatusCode != http.StatusOK {
+					t.Fatalf("unexpected response code: %v", response.StatusCode)
+				}
+
+				err = meekConn.Close()
+				if err != nil {
+					t.Fatalf("MeekConn.Close failed: %v", err)
+				}
+			}
+		})
 	}
 }

psiphon/net.go

@@ -401,6 +401,10 @@ func UntunneledResolveIP(
 // The context is applied to underlying TCP dials. The caller is responsible
 // for applying the context to requests made with the returned http.Client.
 //
+// payloadSecure must only be set if all HTTP plaintext payloads sent through
+// the returned net/http.Client will be wrapped in their own transport security
+// layer, which permits skipping of server certificate verification.
+//
 // Warning: it is not safe to call makeFrontedHTTPClient concurrently with the
 // same dialConfig when tunneled is true because dialConfig will be used
 // directly, instead of copied, which can lead to a crash when fields not safe
@@ -412,8 +416,13 @@ func makeFrontedHTTPClient(
 	dialConfig *DialConfig,
 	frontingSpecs parameters.FrontingSpecs,
 	selectedFrontingProviderID func(string),
-	skipVerify bool,
-	disableSystemRootCAs bool) (*http.Client, func() common.APIParameters, error) {
+	skipVerify,
+	disableSystemRootCAs,
+	payloadSecure bool) (*http.Client, func() common.APIParameters, error) {
+
+	if !payloadSecure && (skipVerify || disableSystemRootCAs) {
+		return nil, nil, errors.TraceNew("cannot skip certificate verification if payload insecure")
+	}
 
 	frontingProviderID,
 		frontingTransport,
@@ -494,10 +503,15 @@ func makeFrontedHTTPClient(
 		}
 	}
 
+	var meekMode MeekMode = MeekModePlaintextRoundTrip
+	if payloadSecure {
+		meekMode = MeekModeWrappedPlaintextRoundTrip
+	}
+
 	meekConfig := &MeekConfig{
 		DiagnosticID:             frontingProviderID,
 		Parameters:               config.GetParameters(),
-		Mode:                     MeekModePlaintextRoundTrip,
+		Mode:                     meekMode,
 		DialAddress:              meekDialAddress,
 		UseHTTPS:                 true,
 		TLSProfile:               tlsProfile,
@@ -680,6 +694,7 @@ func MakeUntunneledHTTPClient(
 	untunneledDialConfig *DialConfig,
 	skipVerify bool,
 	disableSystemRootCAs bool,
+	payloadSecure bool,
 	frontingSpecs parameters.FrontingSpecs,
 	selectedFrontingProviderID func(string)) (*http.Client, func() common.APIParameters, error) {
 
@@ -695,7 +710,8 @@ func MakeUntunneledHTTPClient(
 			frontingSpecs,
 			selectedFrontingProviderID,
 			false,
-			disableSystemRootCAs)
+			disableSystemRootCAs,
+			payloadSecure)
 		if err != nil {
 			return nil, nil, errors.Trace(err)
 		}
@@ -741,8 +757,9 @@ func MakeTunneledHTTPClient(
 	ctx context.Context,
 	config *Config,
 	tunnel *Tunnel,
-	skipVerify bool,
-	disableSystemRootCAs bool,
+	skipVerify,
+	disableSystemRootCAs,
+	payloadSecure bool,
 	frontingSpecs parameters.FrontingSpecs,
 	selectedFrontingProviderID func(string)) (*http.Client, func() common.APIParameters, error) {
 
@@ -775,7 +792,8 @@ func MakeTunneledHTTPClient(
 			frontingSpecs,
 			selectedFrontingProviderID,
 			false,
-			disableSystemRootCAs)
+			disableSystemRootCAs,
+			payloadSecure)
 		if err != nil {
 			return nil, nil, errors.Trace(err)
 		}
@@ -818,7 +836,8 @@ func MakeDownloadHTTPClient(
 	tunnel *Tunnel,
 	untunneledDialConfig *DialConfig,
 	skipVerify,
-	disableSystemRootCAs bool,
+	disableSystemRootCAs,
+	payloadSecure bool,
 	frontingSpecs parameters.FrontingSpecs,
 	selectedFrontingProviderID func(string)) (*http.Client, bool, func() common.APIParameters, error) {
 
@@ -836,6 +855,7 @@ func MakeDownloadHTTPClient(
 			tunnel,
 			skipVerify || disableSystemRootCAs,
 			disableSystemRootCAs,
+			payloadSecure,
 			frontingSpecs,
 			selectedFrontingProviderID)
 		if err != nil {
@@ -849,6 +869,7 @@ func MakeDownloadHTTPClient(
 			untunneledDialConfig,
 			skipVerify,
 			disableSystemRootCAs,
+			payloadSecure,
 			frontingSpecs,
 			selectedFrontingProviderID)
 		if err != nil {

psiphon/remoteServerList.go

@@ -461,13 +461,15 @@ func downloadRemoteServerListFile(
 	// MakeDownloadHttpClient will select either a tunneled
 	// or untunneled configuration.
 
+	payloadSecure := true
 	httpClient, tunneled, getParams, err := MakeDownloadHTTPClient(
 		ctx,
 		config,
 		tunnel,
 		untunneledDialConfig,
 		skipVerify,
 		disableSystemRootCAs,
+		payloadSecure,
 		frontingSpecs,
 		func(frontingProviderID string) {
 			NoticeInfo(

psiphon/upgradeDownload.go

@@ -87,13 +87,15 @@ func DownloadUpgrade(
 
 	downloadURL := urls.Select(attempt)
 
+	payloadSecure := true
 	httpClient, _, _, err := MakeDownloadHTTPClient(
 		ctx,
 		config,
 		tunnel,
 		untunneledDialConfig,
 		downloadURL.SkipVerify,
 		config.DisableSystemRootCAs,
+		payloadSecure,
 		downloadURL.FrontingSpecs,
 		func(frontingProviderID string) {
 			NoticeInfo(