Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit6a9b896

Browse files
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 beguaranteed, and so the field has been made optional. It may be omittedon 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 amissing 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 `NOTNULL` constraint on `ip` on `connection_logs`, and made `ip` on the APIresponse optional.The specific cause for these null IPs was the`/workspaceproxies/me/issue-signed-app-token [post]` endpointconstructing it's own `http.Request` without a `RemoteAddr` set, andthen passing that to the token issuer.To solve this, we'll have workspace proxies send the real IP of theclient when calling `/workspaceproxies/me/issue-signed-app-token [post]`via the header `Coder-Workspace-Proxy-Real-IP`.
1 parent088d149 commit6a9b896

File tree

11 files changed

+79
-12
lines changed

11 files changed

+79
-12
lines changed

‎coderd/database/dump.sql‎

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more aboutcustomizing how changed files appear on GitHub.
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
ALTERTABLE connection_logs ALTER COLUMN ipSETNOT NULL;
Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
-- We can't guarantee that an IP will always be available, and omitting an IP
2+
-- is preferable to not creating a connection log at all.
3+
ALTERTABLE connection_logs ALTER COLUMN ip DROPNOT NULL;

‎codersdk/connectionlog.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ type ConnectionLog struct {
2020
WorkspaceID uuid.UUID`json:"workspace_id" format:"uuid"`
2121
WorkspaceNamestring`json:"workspace_name"`
2222
AgentNamestring`json:"agent_name"`
23-
IP netip.Addr`json:"ip"`
23+
IP*netip.Addr`json:"ip,omitempty"`
2424
TypeConnectionType`json:"type"`
2525

2626
// WebInfo is only set when `type` is one of:

‎enterprise/coderd/connectionlog.go‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,13 @@ func convertConnectionLogs(dblogs []database.GetConnectionLogsOffsetRow) []coder
9393
}
9494

9595
funcconvertConnectionLog(dblog database.GetConnectionLogsOffsetRow) codersdk.ConnectionLog {
96-
ip,_:=netip.AddrFromSlice(dblog.ConnectionLog.Ip.IPNet.IP)
96+
varip*netip.Addr
97+
ifdblog.ConnectionLog.Ip.Valid {
98+
parsedIP,ok:=netip.AddrFromSlice(dblog.ConnectionLog.Ip.IPNet.IP)
99+
ifok {
100+
ip=&parsedIP
101+
}
102+
}
97103

98104
varuser*codersdk.User
99105
ifdblog.ConnectionLog.UserID.Valid {

‎enterprise/coderd/workspaceproxy.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -490,6 +490,7 @@ func (api *API) workspaceProxyIssueSignedAppToken(rw http.ResponseWriter, r *htt
490490
return
491491
}
492492
userReq.Header.Set(codersdk.SessionTokenHeader,req.SessionToken)
493+
userReq.RemoteAddr=r.Header.Get(wsproxysdk.CoderWorkspaceProxyRealIPHeader)
493494

494495
// Exchange the token.
495496
token,tokenStr,ok:=api.AGPL.WorkspaceAppsProvider.Issue(ctx,rw,userReq,req)

‎enterprise/coderd/workspaceproxy_test.go‎

Lines changed: 49 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package coderd_test
33
import (
44
"database/sql"
55
"fmt"
6+
"net"
67
"net/http"
78
"net/http/httptest"
89
"net/http/httputil"
@@ -12,13 +13,15 @@ import (
1213
"time"
1314

1415
"github.com/google/uuid"
16+
"github.com/sqlc-dev/pqtype"
1517
"github.com/stretchr/testify/assert"
1618
"github.com/stretchr/testify/require"
1719

1820
"cdr.dev/slog/sloggers/slogtest"
1921
"github.com/coder/coder/v2/agent/agenttest"
2022
"github.com/coder/coder/v2/buildinfo"
2123
"github.com/coder/coder/v2/coderd/coderdtest"
24+
"github.com/coder/coder/v2/coderd/connectionlog"
2225
"github.com/coder/coder/v2/coderd/database"
2326
"github.com/coder/coder/v2/coderd/database/db2sdk"
2427
"github.com/coder/coder/v2/coderd/database/dbgen"
@@ -610,13 +613,18 @@ func TestProxyRegisterDeregister(t *testing.T) {
610613
funcTestIssueSignedAppToken(t*testing.T) {
611614
t.Parallel()
612615

616+
connectionLogger:=connectionlog.NewFake()
617+
613618
client,user:=coderdenttest.New(t,&coderdenttest.Options{
619+
ConnectionLogging:true,
614620
Options:&coderdtest.Options{
615621
IncludeProvisionerDaemon:true,
622+
ConnectionLogger:connectionLogger,
616623
},
617624
LicenseOptions:&coderdenttest.LicenseOptions{
618625
Features: license.Features{
619626
codersdk.FeatureWorkspaceProxy:1,
627+
codersdk.FeatureConnectionLog:1,
620628
},
621629
},
622630
})
@@ -653,7 +661,7 @@ func TestIssueSignedAppToken(t *testing.T) {
653661
// Invalid request.
654662
AppRequest: workspaceapps.Request{},
655663
SessionToken:client.SessionToken(),
656-
})
664+
},"127.0.0.1")
657665
require.Error(t,err)
658666
})
659667

@@ -669,41 +677,70 @@ func TestIssueSignedAppToken(t *testing.T) {
669677
t.Parallel()
670678
proxyClient:=wsproxysdk.New(client.URL,proxyRes.ProxyToken)
671679

680+
fakeClientIP:="13.37.13.37"
681+
parsedFakeClientIP:= pqtype.Inet{
682+
Valid:true,IPNet: net.IPNet{
683+
IP:net.ParseIP(fakeClientIP),
684+
Mask:net.CIDRMask(32,32),
685+
},
686+
}
687+
672688
ctx:=testutil.Context(t,testutil.WaitLong)
673-
_,err:=proxyClient.IssueSignedAppToken(ctx,goodRequest)
689+
_,err:=proxyClient.IssueSignedAppToken(ctx,goodRequest,fakeClientIP)
674690
require.NoError(t,err)
691+
692+
require.True(t,connectionLogger.Contains(t, database.UpsertConnectionLogParams{
693+
Ip:parsedFakeClientIP,
694+
}))
675695
})
676696

677697
t.Run("OKHTML",func(t*testing.T) {
678698
t.Parallel()
679699
proxyClient:=wsproxysdk.New(client.URL,proxyRes.ProxyToken)
680700

701+
fakeClientIP:="192.168.1.100"
702+
parsedFakeClientIP:= pqtype.Inet{
703+
Valid:true,IPNet: net.IPNet{
704+
IP:net.ParseIP(fakeClientIP),
705+
Mask:net.CIDRMask(32,32),
706+
},
707+
}
708+
681709
rw:=httptest.NewRecorder()
682710
ctx:=testutil.Context(t,testutil.WaitLong)
683-
_,ok:=proxyClient.IssueSignedAppTokenHTML(ctx,rw,goodRequest)
711+
_,ok:=proxyClient.IssueSignedAppTokenHTML(ctx,rw,goodRequest,fakeClientIP)
684712
if!assert.True(t,ok,"expected true") {
685713
resp:=rw.Result()
686714
deferresp.Body.Close()
687715
dump,err:=httputil.DumpResponse(resp,true)
688716
require.NoError(t,err)
689717
t.Log(string(dump))
690718
}
719+
720+
require.True(t,connectionLogger.Contains(t, database.UpsertConnectionLogParams{
721+
Ip:parsedFakeClientIP,
722+
}))
691723
})
692724
}
693725

694726
funcTestReconnectingPTYSignedToken(t*testing.T) {
695727
t.Parallel()
696728

729+
connectionLogger:=connectionlog.NewFake()
730+
697731
db,pubsub:=dbtestutil.NewDB(t)
698732
client,closer,api,user:=coderdenttest.NewWithAPI(t,&coderdenttest.Options{
733+
ConnectionLogging:true,
699734
Options:&coderdtest.Options{
700735
Database:db,
701736
Pubsub:pubsub,
702737
IncludeProvisionerDaemon:true,
738+
ConnectionLogger:connectionLogger,
703739
},
704740
LicenseOptions:&coderdenttest.LicenseOptions{
705741
Features: license.Features{
706742
codersdk.FeatureWorkspaceProxy:1,
743+
codersdk.FeatureConnectionLog:1,
707744
},
708745
},
709746
})
@@ -887,6 +924,15 @@ func TestReconnectingPTYSignedToken(t *testing.T) {
887924

888925
// The token is validated in the apptest suite, so we don't need to
889926
// validate it here.
927+
928+
require.True(t,connectionLogger.Contains(t, database.UpsertConnectionLogParams{
929+
Ip: pqtype.Inet{
930+
Valid:true,IPNet: net.IPNet{
931+
IP:net.ParseIP("127.0.0.1"),
932+
Mask:net.CIDRMask(32,32),
933+
},
934+
},
935+
}))
890936
})
891937
}
892938

‎enterprise/wsproxy/tokenprovider.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ func (p *TokenProvider) Issue(ctx context.Context, rw http.ResponseWriter, r *ht
3939
}
4040
issueReq.AppRequest=appReq
4141

42-
resp,ok:=p.Client.IssueSignedAppTokenHTML(ctx,rw,issueReq)
42+
resp,ok:=p.Client.IssueSignedAppTokenHTML(ctx,rw,issueReq,r.RemoteAddr)
4343
if!ok {
4444
returnnil,"",false
4545
}

‎enterprise/wsproxy/wsproxysdk/wsproxysdk.go‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,12 @@ import (
2121
"github.com/coder/websocket"
2222
)
2323

24+
const (
25+
// CoderWorkspaceProxyAuthTokenHeader is the header that contains the
26+
// resolved real IP address of the client that made the request to the proxy.
27+
CoderWorkspaceProxyRealIPHeader="Coder-Workspace-Proxy-Real-IP"
28+
)
29+
2430
// Client is a HTTP client for a subset of Coder API routes that external
2531
// proxies need.
2632
typeClientstruct {
@@ -84,10 +90,11 @@ type IssueSignedAppTokenResponse struct {
8490
// IssueSignedAppToken issues a new signed app token for the provided app
8591
// request. The error page will be returned as JSON. For use in external
8692
// proxies, use IssueSignedAppTokenHTML instead.
87-
func (c*Client)IssueSignedAppToken(ctx context.Context,req workspaceapps.IssueTokenRequest) (IssueSignedAppTokenResponse,error) {
93+
func (c*Client)IssueSignedAppToken(ctx context.Context,req workspaceapps.IssueTokenRequest,clientIPstring) (IssueSignedAppTokenResponse,error) {
8894
resp,err:=c.RequestIgnoreRedirects(ctx,http.MethodPost,"/api/v2/workspaceproxies/me/issue-signed-app-token",req,func(r*http.Request) {
8995
// This forces any HTML error pages to be returned as JSON instead.
9096
r.Header.Set("Accept","application/json")
97+
r.Header.Set(CoderWorkspaceProxyRealIPHeader,clientIP)
9198
})
9299
iferr!=nil {
93100
returnIssueSignedAppTokenResponse{},xerrors.Errorf("make request: %w",err)
@@ -105,7 +112,7 @@ func (c *Client) IssueSignedAppToken(ctx context.Context, req workspaceapps.Issu
105112
// IssueSignedAppTokenHTML issues a new signed app token for the provided app
106113
// request. The error page will be returned as HTML in most cases, and will be
107114
// written directly to the provided http.ResponseWriter.
108-
func (c*Client)IssueSignedAppTokenHTML(ctx context.Context,rw http.ResponseWriter,req workspaceapps.IssueTokenRequest) (IssueSignedAppTokenResponse,bool) {
115+
func (c*Client)IssueSignedAppTokenHTML(ctx context.Context,rw http.ResponseWriter,req workspaceapps.IssueTokenRequest,clientIPstring) (IssueSignedAppTokenResponse,bool) {
109116
writeError:=func(rw http.ResponseWriter,errerror) {
110117
res:= codersdk.Response{
111118
Message:"Internal server error",
@@ -117,6 +124,7 @@ func (c *Client) IssueSignedAppTokenHTML(ctx context.Context, rw http.ResponseWr
117124

118125
resp,err:=c.RequestIgnoreRedirects(ctx,http.MethodPost,"/api/v2/workspaceproxies/me/issue-signed-app-token",req,func(r*http.Request) {
119126
r.Header.Set("Accept","text/html")
127+
r.Header.Set(CoderWorkspaceProxyRealIPHeader,clientIP)
120128
})
121129
iferr!=nil {
122130
writeError(rw,xerrors.Errorf("perform issue signed app token request: %w",err))

‎enterprise/wsproxy/wsproxysdk/wsproxysdk_test.go‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@ import (
2222
funcTest_IssueSignedAppTokenHTML(t*testing.T) {
2323
t.Parallel()
2424

25+
fakeClientIP:="127.0.0.1"
26+
2527
t.Run("OK",func(t*testing.T) {
2628
t.Parallel()
2729

@@ -68,7 +70,7 @@ func Test_IssueSignedAppTokenHTML(t *testing.T) {
6870
tokenRes,ok:=client.IssueSignedAppTokenHTML(ctx,rw, workspaceapps.IssueTokenRequest{
6971
AppRequest:expectedAppReq,
7072
SessionToken:expectedSessionToken,
71-
})
73+
},fakeClientIP)
7274
if!assert.True(t,ok) {
7375
t.Log("issue request failed when it should've succeeded")
7476
t.Log("response dump:")
@@ -118,7 +120,7 @@ func Test_IssueSignedAppTokenHTML(t *testing.T) {
118120
tokenRes,ok:=client.IssueSignedAppTokenHTML(ctx,rw, workspaceapps.IssueTokenRequest{
119121
AppRequest: workspaceapps.Request{},
120122
SessionToken:"user-session-token",
121-
})
123+
},fakeClientIP)
122124
require.False(t,ok)
123125
require.Empty(t,tokenRes)
124126
require.True(t,rw.WasWritten())

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp