mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
fix!: use client ip when creating connection logs for workspace proxied app accesses (#19788)
Breaking API Change:
> The presence of the `ip` field on `codersdk.ConnectionLog` cannot be
guaranteed, and so the field has been made optional. It may be omitted
on API responses.
When running a scaletest, I noticed logs of the form:
```
2025-09-12 06:34:10.924 [erro] coderd.workspaceapps: upsert connection log failed trace=0xa17580 span=0xa17620 workspace_id=81b937d7-5777-4df5-b5cb-80241f30326f agent_id=78b2ff6d-b4a6-4a4e-88a7-283e05455a88 app_id=00000000-0000-0000-0000-000000000000 user_id=00000000-0000-0000-0000-000000000000 user_agent="" app_slug_or_port=terminal status_code=404 request_id=67f03cf8-9523-444a-97bc-90de080a54c8 ...
error= 1 error occurred:
* pq: null value in column "ip" of relation "connection_logs" violates not-null constraint
```
to ensure logs are never omitted from the connection log due to a
missing IP again (i.e. I'm not sure if we can always rely on a valid,
parseable, IP from `(http.Request).RemoteAddr`), I've removed the `NOT
NULL` constraint on `ip` on `connection_logs`, and made `ip` on the API
response optional.
The specific cause for these null IPs was the
`/workspaceproxies/me/issue-signed-app-token [post]` endpoint
constructing it's own `http.Request` without a `RemoteAddr` set, and
then passing that to the token issuer.
To solve this, we'll have workspace proxies send the real IP of the
client when calling `/workspaceproxies/me/issue-signed-app-token [post]`
via the header `Coder-Workspace-Proxy-Real-IP`.
This commit is contained in:
Ethan
@@ -93,7 +93,13 @@ func convertConnectionLogs(dblogs []database.GetConnectionLogsOffsetRow) []coder
|
||||
}
|
||||
|
||||
func convertConnectionLog(dblog database.GetConnectionLogsOffsetRow) codersdk.ConnectionLog {
|
||||
ip, _ := netip.AddrFromSlice(dblog.ConnectionLog.Ip.IPNet.IP)
|
||||
var ip *netip.Addr
|
||||
if dblog.ConnectionLog.Ip.Valid {
|
||||
parsedIP, ok := netip.AddrFromSlice(dblog.ConnectionLog.Ip.IPNet.IP)
|
||||
if ok {
|
||||
ip = &parsedIP
|
||||
}
|
||||
}
|
||||
|
||||
var user *codersdk.User
|
||||
if dblog.ConnectionLog.UserID.Valid {
|
||||
|
||||
@@ -490,6 +490,7 @@ func (api *API) workspaceProxyIssueSignedAppToken(rw http.ResponseWriter, r *htt
|
||||
return
|
||||
}
|
||||
userReq.Header.Set(codersdk.SessionTokenHeader, req.SessionToken)
|
||||
userReq.RemoteAddr = r.Header.Get(wsproxysdk.CoderWorkspaceProxyRealIPHeader)
|
||||
|
||||
// Exchange the token.
|
||||
token, tokenStr, ok := api.AGPL.WorkspaceAppsProvider.Issue(ctx, rw, userReq, req)
|
||||
|
||||
@@ -3,6 +3,7 @@ package coderd_test
|
||||
import (
|
||||
"database/sql"
|
||||
"fmt"
|
||||
"net"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"net/http/httputil"
|
||||
@@ -12,6 +13,7 @@ import (
|
||||
"time"
|
||||
|
||||
"github.com/google/uuid"
|
||||
"github.com/sqlc-dev/pqtype"
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
|
||||
@@ -19,6 +21,7 @@ import (
|
||||
"github.com/coder/coder/v2/agent/agenttest"
|
||||
"github.com/coder/coder/v2/buildinfo"
|
||||
"github.com/coder/coder/v2/coderd/coderdtest"
|
||||
"github.com/coder/coder/v2/coderd/connectionlog"
|
||||
"github.com/coder/coder/v2/coderd/database"
|
||||
"github.com/coder/coder/v2/coderd/database/db2sdk"
|
||||
"github.com/coder/coder/v2/coderd/database/dbgen"
|
||||
@@ -610,13 +613,18 @@ func TestProxyRegisterDeregister(t *testing.T) {
|
||||
func TestIssueSignedAppToken(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
connectionLogger := connectionlog.NewFake()
|
||||
|
||||
client, user := coderdenttest.New(t, &coderdenttest.Options{
|
||||
ConnectionLogging: true,
|
||||
Options: &coderdtest.Options{
|
||||
IncludeProvisionerDaemon: true,
|
||||
ConnectionLogger: connectionLogger,
|
||||
},
|
||||
LicenseOptions: &coderdenttest.LicenseOptions{
|
||||
Features: license.Features{
|
||||
codersdk.FeatureWorkspaceProxy: 1,
|
||||
codersdk.FeatureConnectionLog: 1,
|
||||
},
|
||||
},
|
||||
})
|
||||
@@ -653,7 +661,7 @@ func TestIssueSignedAppToken(t *testing.T) {
|
||||
// Invalid request.
|
||||
AppRequest: workspaceapps.Request{},
|
||||
SessionToken: client.SessionToken(),
|
||||
})
|
||||
}, "127.0.0.1")
|
||||
require.Error(t, err)
|
||||
})
|
||||
|
||||
@@ -669,18 +677,38 @@ func TestIssueSignedAppToken(t *testing.T) {
|
||||
t.Parallel()
|
||||
proxyClient := wsproxysdk.New(client.URL, proxyRes.ProxyToken)
|
||||
|
||||
fakeClientIP := "13.37.13.37"
|
||||
parsedFakeClientIP := pqtype.Inet{
|
||||
Valid: true, IPNet: net.IPNet{
|
||||
IP: net.ParseIP(fakeClientIP),
|
||||
Mask: net.CIDRMask(32, 32),
|
||||
},
|
||||
}
|
||||
|
||||
ctx := testutil.Context(t, testutil.WaitLong)
|
||||
_, err := proxyClient.IssueSignedAppToken(ctx, goodRequest)
|
||||
_, err := proxyClient.IssueSignedAppToken(ctx, goodRequest, fakeClientIP)
|
||||
require.NoError(t, err)
|
||||
|
||||
require.True(t, connectionLogger.Contains(t, database.UpsertConnectionLogParams{
|
||||
Ip: parsedFakeClientIP,
|
||||
}))
|
||||
})
|
||||
|
||||
t.Run("OKHTML", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
proxyClient := wsproxysdk.New(client.URL, proxyRes.ProxyToken)
|
||||
|
||||
fakeClientIP := "192.168.1.100"
|
||||
parsedFakeClientIP := pqtype.Inet{
|
||||
Valid: true, IPNet: net.IPNet{
|
||||
IP: net.ParseIP(fakeClientIP),
|
||||
Mask: net.CIDRMask(32, 32),
|
||||
},
|
||||
}
|
||||
|
||||
rw := httptest.NewRecorder()
|
||||
ctx := testutil.Context(t, testutil.WaitLong)
|
||||
_, ok := proxyClient.IssueSignedAppTokenHTML(ctx, rw, goodRequest)
|
||||
_, ok := proxyClient.IssueSignedAppTokenHTML(ctx, rw, goodRequest, fakeClientIP)
|
||||
if !assert.True(t, ok, "expected true") {
|
||||
resp := rw.Result()
|
||||
defer resp.Body.Close()
|
||||
@@ -688,22 +716,31 @@ func TestIssueSignedAppToken(t *testing.T) {
|
||||
require.NoError(t, err)
|
||||
t.Log(string(dump))
|
||||
}
|
||||
|
||||
require.True(t, connectionLogger.Contains(t, database.UpsertConnectionLogParams{
|
||||
Ip: parsedFakeClientIP,
|
||||
}))
|
||||
})
|
||||
}
|
||||
|
||||
func TestReconnectingPTYSignedToken(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
connectionLogger := connectionlog.NewFake()
|
||||
|
||||
db, pubsub := dbtestutil.NewDB(t)
|
||||
client, closer, api, user := coderdenttest.NewWithAPI(t, &coderdenttest.Options{
|
||||
ConnectionLogging: true,
|
||||
Options: &coderdtest.Options{
|
||||
Database: db,
|
||||
Pubsub: pubsub,
|
||||
IncludeProvisionerDaemon: true,
|
||||
ConnectionLogger: connectionLogger,
|
||||
},
|
||||
LicenseOptions: &coderdenttest.LicenseOptions{
|
||||
Features: license.Features{
|
||||
codersdk.FeatureWorkspaceProxy: 1,
|
||||
codersdk.FeatureConnectionLog: 1,
|
||||
},
|
||||
},
|
||||
})
|
||||
@@ -887,6 +924,15 @@ func TestReconnectingPTYSignedToken(t *testing.T) {
|
||||
|
||||
// The token is validated in the apptest suite, so we don't need to
|
||||
// validate it here.
|
||||
|
||||
require.True(t, connectionLogger.Contains(t, database.UpsertConnectionLogParams{
|
||||
Ip: pqtype.Inet{
|
||||
Valid: true, IPNet: net.IPNet{
|
||||
IP: net.ParseIP("127.0.0.1"),
|
||||
Mask: net.CIDRMask(32, 32),
|
||||
},
|
||||
},
|
||||
}))
|
||||
})
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user