topdown: http.send use provided CA without client certs

Previously it would ignore the provided CA parameters unless client cert info was provided. This change now allows for providing CA info without any client auth details. Fixes: open-policy-agent#1976 Signed-off-by: Patrick East <east.patrick@gmail.com>
patrick-east · Feb 6, 2020 · df39700 · df39700
1 parent 60cf8aa
commit df39700
Show file tree

Hide file tree

Showing 3 changed files with 152 additions and 8 deletions.
diff --git a/docs/content/policy-reference.md b/docs/content/policy-reference.md
@@ -537,12 +537,12 @@ The `request` object parameter may contain the following fields:
 | `tls_client_key_file` | no | `string` | Path to file containing a key  in PEM encoded format. |
 
 
-To trigger the use of HTTPs the user must provide one of the following combinations:
+When sending HTTPS requests with client certificates at least one the following combinations must be included
 
- * ``tls_client_cert_file``, ``tls_client_key_file``
- * ``tls_client_cert_env_variable``, ``tls_client_key_env_variable``
+ * ``tls_client_cert_file`` and ``tls_client_key_file``
+ * ``tls_client_cert_env_variable`` and ``tls_client_key_env_variable``
 
- The user must also provide a trusted root CA through tls_ca_cert_file or tls_ca_cert_env_variable. Alternatively the user could set tls_use_system_certs to ``true`` and the system certificate pool will be used.
+> The user must also provide a trusted root CA through tls_ca_cert_file or tls_ca_cert_env_variable. Alternatively the user could set tls_use_system_certs to ``true`` and the system certificate pool will be used.
 
 The `response` object parameter will contain the following fields:
 
@@ -557,9 +557,9 @@ The table below shows examples of calling `http.send`:
 
 | Example |  Comments |
 | -------- |-----------|
+| Accessing Google using System Cert Pool | ``http.send({"method": "get", "url": "https://www.google.com", "tls_use_system_certs": true })`` |
 | Files containing TLS material | ``http.send({"method": "get", "url": "https://127.0.0.1:65331", "tls_ca_cert_file": "testdata/ca.pem", "tls_client_cert_file": "testdata/client-cert.pem", "tls_client_key_file": "testdata/client-key.pem"})`` |
-|Environment variables containing TLS material | ``http.send({"method": "get", "url": "https://127.0.0.1:65360", "tls_ca_cert_env_variable": "CLIENT_CA_ENV", "tls_client_cert_env_variable": "CLIENT_CERT_ENV", "tls_client_key_env_variable": "CLIENT_KEY_ENV"})`` |
-| Accessing Google using System Cert Pool | ``http.send({"method": "get", "url": "https://www.google.com", "tls_use_system_certs": true, "tls_client_cert_file": "testdata/client-cert.pem", "tls_client_key_file": "testdata/client-key.pem"})`` |
+| Environment variables containing TLS material | ``http.send({"method": "get", "url": "https://127.0.0.1:65360", "tls_ca_cert_env_variable": "CLIENT_CA_ENV", "tls_client_cert_env_variable": "CLIENT_CERT_ENV", "tls_client_key_env_variable": "CLIENT_KEY_ENV"})`` |
 
 ### Net
 | Built-in | Description |

diff --git a/topdown/http.go b/topdown/http.go
@@ -239,14 +239,22 @@ func executeHTTPRequest(bctx BuiltinContext, obj ast.Object) (ast.Value, error)
 		clientCerts = append(clientCerts, clientCertFromEnv)
 	}
 
+	isTLS := false
 	if len(clientCerts) > 0 {
-		// this is a TLS connection
+		isTLS = true
+		tlsConfig.Certificates = append(tlsConfig.Certificates, clientCerts...)
+	}
+
+	if tlsUseSystemCerts || len(tlsCaCertFile) > 0 || len(tlsCaCertEnvVar) > 0 {
+		isTLS = true
 		connRootCAs, err := createRootCAs(tlsCaCertFile, tlsCaCertEnvVar, tlsUseSystemCerts)
 		if err != nil {
 			return nil, err
 		}
-		tlsConfig.Certificates = append(tlsConfig.Certificates, clientCerts...)
 		tlsConfig.RootCAs = connRootCAs
+	}
+
+	if isTLS {
 		client.Transport = &http.Transport{
 			TLSClientConfig: &tlsConfig,
 		}

diff --git a/topdown/http_test.go b/topdown/http_test.go
@@ -675,3 +675,139 @@ func TestHTTPSClient(t *testing.T) {
 		runTopDownTestCase(t, data, "http.send", rule, expectedResult)
 	})
 }
+
+func TestHTTPSNoClientCerts(t *testing.T) {
+
+	const (
+		localCaFile         = "testdata/ca.pem"
+		localServerCertFile = "testdata/server-cert.pem"
+		localServerKeyFile  = "testdata/server-key.pem"
+	)
+
+	caCertPEM, err := ioutil.ReadFile(localCaFile)
+	if err != nil {
+		t.Fatal(err)
+	}
+	caPool := x509.NewCertPool()
+	if ok := caPool.AppendCertsFromPEM(caCertPEM); !ok {
+		t.Fatal("failed to parse CA cert")
+	}
+
+	cert, err := tls.LoadX509KeyPair(localServerCertFile, localServerKeyFile)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	err = os.Setenv("CLIENT_CA_ENV", string(caCertPEM))
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Replicating some of what happens in the server's HTTPS listener
+	s := getTLSTestServer()
+	s.TLS = &tls.Config{
+		Certificates: []tls.Certificate{cert},
+		ClientCAs:    caPool,
+	}
+	s.StartTLS()
+	defer s.Close()
+
+	t.Run("HTTPS Get with CA Cert File", func(t *testing.T) {
+		// expected result
+		expectedResult := map[string]interface{}{
+			"status":      "200 OK",
+			"status_code": http.StatusOK,
+			"body":        nil,
+			"raw_body":    "",
+		}
+
+		resultObj, err := ast.InterfaceToValue(expectedResult)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		data := loadSmallTestData()
+		rule := []string{fmt.Sprintf(
+			`p = x { http.send({"method": "get", "url": "%s", "tls_ca_cert_file": "%s"}, x) }`, s.URL, localCaFile)}
+
+		// run the test
+		runTopDownTestCase(t, data, "http.send", rule, resultObj.String())
+	})
+
+	t.Run("HTTPS Get with CA Cert ENV", func(t *testing.T) {
+		// expected result
+		expectedResult := map[string]interface{}{
+			"status":      "200 OK",
+			"status_code": http.StatusOK,
+			"body":        nil,
+			"raw_body":    "",
+		}
+
+		resultObj, err := ast.InterfaceToValue(expectedResult)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		data := loadSmallTestData()
+		rule := []string{fmt.Sprintf(
+			`p = x { http.send({"method": "get", "url": "%s", "tls_ca_cert_env_variable": "CLIENT_CA_ENV"}, x) }`, s.URL)}
+
+		// run the test
+		runTopDownTestCase(t, data, "http.send", rule, resultObj.String())
+	})
+
+	t.Run("HTTPS Get with System CA Cert Pool", func(t *testing.T) {
+		// expected result
+		expectedResult := map[string]interface{}{
+			"status":      "200 OK",
+			"status_code": http.StatusOK,
+			"body":        nil,
+			"raw_body":    "",
+		}
+
+		resultObj, err := ast.InterfaceToValue(expectedResult)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		data := loadSmallTestData()
+		rule := []string{fmt.Sprintf(
+			`p = x { http.send({"method": "get", "url": "%s", "tls_ca_cert_env_variable": "CLIENT_CA_ENV"}, x) }`, s.URL)}
+
+		// run the test
+		runTopDownTestCase(t, data, "http.send", rule, resultObj.String())
+	})
+
+	t.Run("HTTPS Get with System Certs, Env and File Cert", func(t *testing.T) {
+		// expected result
+		expectedResult := map[string]interface{}{
+			"status":      "200 OK",
+			"status_code": http.StatusOK,
+			"body":        nil,
+			"raw_body":    "",
+		}
+
+		resultObj, err := ast.InterfaceToValue(expectedResult)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		data := loadSmallTestData()
+		rule := []string{fmt.Sprintf(
+			`p = x { http.send({"method": "get", "url": "%s", "tls_use_system_certs": true, "tls_ca_cert_env_variable": "CLIENT_CA_ENV", "tls_ca_cert_file": "%s"}, x) }`, s.URL, localCaFile)}
+
+		// run the test
+		runTopDownTestCase(t, data, "http.send", rule, resultObj.String())
+	})
+
+	t.Run("Negative Test: System Certs do not include local rootCA", func(t *testing.T) {
+
+		expectedResult := Error{Code: BuiltinErr, Message: "x509: certificate signed by unknown authority", Location: nil}
+		data := loadSmallTestData()
+		rule := []string{fmt.Sprintf(
+			`p = x { http.send({"method": "get", "url": "%s", "tls_use_system_certs": true}, x) }`, s.URL)}
+
+		// run the test
+		runTopDownTestCase(t, data, "http.send", rule, expectedResult)
+	})
+}