-
Notifications
You must be signed in to change notification settings - Fork 2
fix: ensure fully qualified URL recorded for auditing #142
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Before: After: |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,132 @@ | ||
| package proxy | ||
|
|
||
| import ( | ||
| "net" | ||
| "net/http" | ||
| "net/http/httptest" | ||
| "net/url" | ||
| "sync" | ||
| "testing" | ||
|
|
||
| "github.com/coder/boundary/audit" | ||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| // capturingAuditor captures all audit requests for test verification. | ||
| type capturingAuditor struct { | ||
| mu sync.Mutex | ||
| requests []audit.Request | ||
| } | ||
|
|
||
| func (c *capturingAuditor) AuditRequest(req audit.Request) { | ||
| c.mu.Lock() | ||
| defer c.mu.Unlock() | ||
| c.requests = append(c.requests, req) | ||
| } | ||
|
|
||
| func (c *capturingAuditor) getRequests() []audit.Request { | ||
| c.mu.Lock() | ||
| defer c.mu.Unlock() | ||
| return append([]audit.Request{}, c.requests...) | ||
| } | ||
|
|
||
| func TestAuditURLIsFullyFormed_HTTP(t *testing.T) { | ||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| w.WriteHeader(http.StatusOK) | ||
| })) | ||
| defer server.Close() | ||
|
|
||
| serverURL, err := url.Parse(server.URL) | ||
| require.NoError(t, err) | ||
|
|
||
| auditor := &capturingAuditor{} | ||
|
|
||
| pt := NewProxyTest(t, | ||
| WithCertManager(t.TempDir()), | ||
| WithAllowedRule("domain="+serverURL.Hostname()+" path=/allowed/*"), | ||
| WithAuditor(auditor), | ||
| ).Start() | ||
| defer pt.Stop() | ||
|
|
||
| t.Run("allowed", func(t *testing.T) { | ||
| resp, err := pt.proxyClient.Get(server.URL + "/allowed/path?q=1") | ||
| require.NoError(t, err) | ||
| defer func() { | ||
| err = resp.Body.Close() | ||
| require.NoError(t, err) | ||
| }() | ||
| require.Equal(t, http.StatusOK, resp.StatusCode) | ||
|
|
||
| requests := auditor.getRequests() | ||
| require.NotEmpty(t, requests) | ||
|
|
||
| req := requests[len(requests)-1] | ||
| require.True(t, req.Allowed) | ||
|
|
||
| expectedURL := "http://" + net.JoinHostPort(serverURL.Hostname(), serverURL.Port()) + "/allowed/path?q=1" | ||
| assert.Equal(t, expectedURL, req.URL) | ||
| }) | ||
|
|
||
| t.Run("denied", func(t *testing.T) { | ||
| resp, err := pt.proxyClient.Get(server.URL + "/denied/path") | ||
| require.NoError(t, err) | ||
| defer func() { | ||
| err = resp.Body.Close() | ||
| require.NoError(t, err) | ||
| }() | ||
| require.Equal(t, http.StatusForbidden, resp.StatusCode) | ||
|
|
||
| requests := auditor.getRequests() | ||
| require.NotEmpty(t, requests) | ||
|
|
||
| req := requests[len(requests)-1] | ||
| require.False(t, req.Allowed) | ||
|
|
||
| expectedURL := "http://" + net.JoinHostPort(serverURL.Hostname(), serverURL.Port()) + "/denied/path" | ||
| assert.Equal(t, expectedURL, req.URL) | ||
| }) | ||
| } | ||
|
|
||
| func TestAuditURLIsFullyFormed_HTTPS(t *testing.T) { | ||
| auditor := &capturingAuditor{} | ||
|
|
||
| pt := NewProxyTest(t, | ||
| WithCertManager(t.TempDir()), | ||
| WithAllowedDomain("dev.coder.com"), | ||
| WithAuditor(auditor), | ||
| ).Start() | ||
| defer pt.Stop() | ||
|
|
||
| tunnel, err := pt.establishExplicitCONNECT("dev.coder.com:443") | ||
| require.NoError(t, err) | ||
| defer func() { | ||
| assert.NoError(t, tunnel.close()) | ||
| }() | ||
|
|
||
| t.Run("allowed", func(t *testing.T) { | ||
| _, err := tunnel.sendRequest("dev.coder.com", "/api/v2?q=1") | ||
| require.NoError(t, err) | ||
|
|
||
| requests := auditor.getRequests() | ||
| require.NotEmpty(t, requests) | ||
|
|
||
| req := requests[len(requests)-1] | ||
| require.True(t, req.Allowed) | ||
|
|
||
| assert.Equal(t, "https://dev.coder.com/api/v2?q=1", req.URL) | ||
| }) | ||
|
|
||
| t.Run("denied", func(t *testing.T) { | ||
| err := tunnel.sendRequestAndExpectDeny("blocked.example.com", "/some/path") | ||
| require.NoError(t, err) | ||
|
|
||
| requests := auditor.getRequests() | ||
| require.NotEmpty(t, requests) | ||
|
|
||
| req := requests[len(requests)-1] | ||
| require.False(t, req.Allowed) | ||
|
|
||
| assert.Equal(t, "https://blocked.example.com/some/path", req.URL) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Including the host here seems redundant if the URL is fully qualified. Should we remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for analyzing logs and building dashboards based on logs - it will be easier to have structured logs:
?
Probably we'll want to run some aggregations queries based on domain? It may be harder to run aggregations queries if we only have string with fully qualified URL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now I'm inclined to just make this the fully-qualified URL so that the application logs are accurate and the consumers actually get the full URL. Consumers can parse it, if desired.
That said, I think it would make sense to ultimately structure the logs a bit better in coder. I'll see how things pan out after dogfooding for a bit.