Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add testing to the proxy #2007

Merged
merged 8 commits into from
Apr 13, 2022
Merged

Add testing to the proxy #2007

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3a5c4e2
refactor+test: refactored some functions to test
Mar 15, 2022
7176161
refactored tests
Mar 25, 2022
c7e7886
Merge branch 'master' of https://github.com/Azure/ARO-RP into proxy-r…
Mar 25, 2022
b763161
added license to test file
Mar 25, 2022
3d982e9
added err check on validateProxyResquest
Mar 25, 2022
2e873c5
made the errors more explicit
Mar 28, 2022
7eced21
fixed typo in function name
Apr 11, 2022
bfb4d4b
removed useless test case
Apr 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 35 additions & 19 deletions pkg/proxy/proxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package proxy
import (
"crypto/tls"
"crypto/x509"
"errors"
"io"
"io/ioutil"
"net"
Expand All @@ -24,13 +25,15 @@ type Server struct {
KeyFile string
ClientCertFile string
Subnet string
subnet *net.IPNet
}

func (s *Server) Run() error {
_, subnet, err := net.ParseCIDR(s.Subnet)
if err != nil {
return err
}
s.subnet = subnet

b, err := ioutil.ReadFile(s.ClientCertFile)
if err != nil {
Expand Down Expand Up @@ -91,25 +94,38 @@ func (s *Server) Run() error {
return err
}

return http.Serve(l, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if r.Method != http.MethodConnect {
http.Error(w, http.StatusText(http.StatusMethodNotAllowed), http.StatusMethodNotAllowed)
return
}

ip, _, err := net.SplitHostPort(r.Host)
if err != nil {
http.Error(w, err.Error(), http.StatusBadRequest)
return
}

if !subnet.Contains(net.ParseIP(ip)) {
http.Error(w, http.StatusText(http.StatusForbidden), http.StatusForbidden)
return
}

Proxy(s.Log, w, r, 0)
}))
return http.Serve(l, http.HandlerFunc(s.proxyHandler))
}

func (s Server) proxyHandler(w http.ResponseWriter, r *http.Request) {
err := s.validateProxyResquest(w, r)
bennerv marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return
}
Proxy(s.Log, w, r, 0)
}

// validateProxyResquest checks that the request is valid. If not, it writes the
// appropriate http headers and returns an error.
func (s Server) validateProxyResquest(w http.ResponseWriter, r *http.Request) error {
bennerv marked this conversation as resolved.
Show resolved Hide resolved

ip, _, err := net.SplitHostPort(r.Host)
if err != nil {
http.Error(w, err.Error(), http.StatusBadRequest)
return err
}

if r.Method != http.MethodConnect {
http.Error(w, http.StatusText(http.StatusMethodNotAllowed), http.StatusMethodNotAllowed)
return errors.New("Request is not valid, method is not CONNECT")
}

if !s.subnet.Contains(net.ParseIP(ip)) {
http.Error(w, http.StatusText(http.StatusForbidden), http.StatusForbidden)
return errors.New("Request is not allowed, the originating IP is not part of the allowed subnet")
}

return nil
}

// Proxy takes an HTTP/1.x CONNECT Request and ResponseWriter from the Golang
Expand Down
93 changes: 93 additions & 0 deletions pkg/proxy/proxy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
package proxy

// Copyright (c) Microsoft Corporation.
// Licensed under the Apache License 2.0.

import (
"fmt"
"net"
"net/http"
"net/http/httptest"
"testing"
)

func TestRequestValidation(t *testing.T) {
tests := []struct {
name string
method string
subnet string
hostname string
wantStatus int
wantErr bool
}{
Comment on lines +15 to +22
Copy link

@AldoFusterTurpin AldoFusterTurpin Apr 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a quick question.
Why instead of using a []struct, you use a map[string]struct where the key of the map is the test name and the value of the map is the struct you already have. This provides some benefits like:

  • ensuring unique test case names (as the keys of a map are unique)
  • Map iteration order is undefined. This means each time we run go test, our tests are going to be potentially run in a different order. This is super useful for spotting conditions where tests pass when run in statement order, but not otherwise. I don't think this is happening here but is a good practice to adopt.

More info in Dave Cheney blog, "Give your test cases names" section (or just look for "We can dry this up, even more, using a map literal syntax:").

This is just a comment, I would not reject this PR for that reason but don't feel enough confident to approve it either. 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually followed a recommendation from Mikalai from another PR. But this is a pattern that we have in most (if not all) other test files too.

The test cases will have a #0X appended to them if they have the same name in the struct. So it would probably be something we would lose with the map because the test would be overwritten, and we'd have to manually add the numbers to each of those. (now I actually don't know if having multiple subtests with the same name is something we want to have, so I'd wait for someone with more experience to weigh in)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More info in Dave Cheney blog, "Give your test cases names" section (or just look for "We can dry this up, even more, using a map literal syntax:").

This is interesting. We shouldn't have to nest test cases as your test cases would evolve into something like the below. It shouldn't require an #0X appended as long as the names of the tests are different.

	tests := map[string]struct {
		method     string
		subnet     string
		hostname   string
		wantStatus int
		wantErr    bool
	}{
               "get https same subnet": {
			method:     http.MethodGet,
			subnet:     "127.0.0.1/24",
			hostname:   "https://127.0.0.2:123",
			wantStatus: http.StatusMethodNotAllowed,
			wantErr:    true,
		}, 
                "test-2": {
			method:     http.MethodGet,
			subnet:     "127.0.0.1/24",
			hostname:   "https://127.0.0.2:123",
			wantStatus: http.StatusMethodNotAllowed,
			wantErr:    true,
		}, 
		...
	}

That being said, I'm fine conforming to our current standard. Definitely something I would approve if it was changed going forward as well though. It is a small addition of DRY

{
name: "get https same subnet",
method: http.MethodGet,
subnet: "127.0.0.1/24",
hostname: "https://127.0.0.2:123",
wantStatus: http.StatusMethodNotAllowed,
wantErr: true,
},
{
name: "get https different subnet",
method: http.MethodGet,
subnet: "127.0.0.1/24",
hostname: "https://10.0.0.2:123",
wantStatus: http.StatusMethodNotAllowed,
wantErr: true,
},
bennerv marked this conversation as resolved.
Show resolved Hide resolved
{
name: "connect http same subnet",
method: http.MethodConnect,
subnet: "127.0.0.1/24",
hostname: "127.0.0.2:123",
wantStatus: http.StatusOK,
wantErr: false,
},
{
name: "connect http different subnet",
method: http.MethodConnect,
subnet: "127.0.0.1/24",
hostname: "10.0.0.1:123",
wantStatus: http.StatusForbidden,
wantErr: true,
},
{
name: "wrong hostname",
method: http.MethodGet,
subnet: "127.0.0.1/24",
hostname: "https://127.0.0.1::",
wantStatus: http.StatusBadRequest,
wantErr: true,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
server := Server{Subnet: tt.subnet}
_, subnet, err := net.ParseCIDR(server.Subnet)
if err != nil {
t.FailNow()
}
server.subnet = subnet

recorder := httptest.NewRecorder()
request := httptest.NewRequest(tt.method, tt.hostname, nil)

err = server.validateProxyResquest(recorder, request)
bennerv marked this conversation as resolved.
Show resolved Hide resolved
if (err != nil && !tt.wantErr) || (err == nil && tt.wantErr) {
t.Error(err)
}

response := recorder.Result()

if response.StatusCode != tt.wantStatus {
fmt.Println(response.StatusCode, tt.wantStatus)
t.Error(tt.hostname)
}

})

}

}
facchettos marked this conversation as resolved.
Show resolved Hide resolved