Skip to content

Commit 904fde2

Browse files
deansheatherdeansheather
committed
PR comments
1 parent 7a7a865 commit 904fde2

File tree

10 files changed

+111
-63
lines changed

10 files changed

+111
-63
lines changed

‎cli/server.go

Lines changed: 2 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -805,35 +805,9 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd.
805805

806806
// Read the coordinator resume token signing key from the
807807
// database.
808-
resumeTokenKey := [64]byte{}
809-
resumeTokenKeyStr, err := tx.GetCoordinatorResumeTokenSigningKey(ctx)
810-
if err != nil && !xerrors.Is(err, sql.ErrNoRows) {
811-
return xerrors.Errorf("get coordinator resume token key: %w", err)
812-
}
813-
if decoded, err := hex.DecodeString(resumeTokenKeyStr); err != nil || len(decoded) != len(resumeTokenKey) {
814-
b := make([]byte, len(resumeTokenKey))
815-
_, err := rand.Read(b)
816-
if err != nil {
817-
return xerrors.Errorf("generate fresh coordinator resume token key: %w", err)
818-
}
819-
820-
resumeTokenKeyStr = hex.EncodeToString(b)
821-
err = tx.UpsertCoordinatorResumeTokenSigningKey(ctx, resumeTokenKeyStr)
822-
if err != nil {
823-
return xerrors.Errorf("insert freshly generated coordinator resume token key to database: %w", err)
824-
}
825-
}
826-
827-
resumeTokenKeyBytes, err := hex.DecodeString(resumeTokenKeyStr)
808+
resumeTokenKey, err := tailnet.ResumeTokenSigningKeyFromDatabase(ctx, tx)
828809
if err != nil {
829-
return xerrors.Errorf("decode coordinator resume token key from database: %w", err)
830-
}
831-
if len(resumeTokenKeyBytes) != len(resumeTokenKey) {
832-
return xerrors.Errorf("coordinator resume token key in database is not the correct length, expect %d got %d", len(resumeTokenKey), len(resumeTokenKeyBytes))
833-
}
834-
copy(resumeTokenKey[:], resumeTokenKeyBytes)
835-
if resumeTokenKey == [64]byte{} {
836-
return xerrors.Errorf("coordinator resume token key in database is empty")
810+
return xerrors.Errorf("get coordinator resume token key from database: %w", err)
837811
}
838812
options.CoordinatorResumeTokenProvider = tailnet.NewResumeTokenKeyProvider(resumeTokenKey, tailnet.DefaultResumeTokenExpiry)
839813

‎coderd/coderdtest/coderdtest.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -492,7 +492,7 @@ func NewOptions(t testing.TB, options *Options) (func(http.Handler), context.Can
492492
TailnetCoordinator: options.Coordinator,
493493
BaseDERPMap: derpMap,
494494
DERPMapUpdateFrequency: 150 * time.Millisecond,
495-
CoordinatorResumeTokenProvider: tailnet.InsecureTestResumeTokenProvider,
495+
CoordinatorResumeTokenProvider: tailnet.NewInsecureTestResumeTokenProvider(),
496496
MetricsCacheRefreshInterval: options.MetricsCacheRefreshInterval,
497497
AgentStatsRefreshInterval: options.AgentStatsRefreshInterval,
498498
DeploymentValues: options.DeploymentValues,

‎coderd/workspaceagents.go

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -853,11 +853,14 @@ func (api *API) workspaceAgentClientCoordinate(rw http.ResponseWriter, r *http.R
853853
)
854854
if resumeToken != "" {
855855
var err error
856-
peerID, err = api.Options.CoordinatorResumeTokenProvider.ParseResumeToken(resumeToken)
856+
peerID, err = api.Options.CoordinatorResumeTokenProvider.VerifyResumeToken(resumeToken)
857857
if err != nil {
858-
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
858+
httpapi.Write(ctx, rw, http.StatusUnauthorized, codersdk.Response{
859859
Message: workspacesdk.CoordinateAPIInvalidResumeToken,
860860
Detail: err.Error(),
861+
Validations: []codersdk.ValidationError{
862+
{Field: "resume_token", Detail: workspacesdk.CoordinateAPIInvalidResumeToken},
863+
},
861864
})
862865
return
863866
}

‎codersdk/workspacesdk/connector.go

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -178,20 +178,22 @@ func (tac *tailnetAPIConnector) dial() (proto.DRPCTailnetClient, error) {
178178
close(tac.connected)
179179
}
180180
if err != nil {
181-
if !errors.Is(err, context.Canceled) {
182-
tac.logger.Error(tac.ctx, "failed to dial tailnet v2+ API", slog.Error(err))
183-
}
184-
if res.StatusCode == http.StatusBadRequest {
185-
err = codersdk.ReadBodyAsError(res)
186-
var sdkErr *codersdk.Error
187-
if xerrors.As(err, &sdkErr) {
188-
if sdkErr.Message == CoordinateAPIInvalidResumeToken {
181+
didLog := false
182+
bodyErr := codersdk.ReadBodyAsError(res)
183+
var sdkErr *codersdk.Error
184+
if xerrors.As(bodyErr, &sdkErr) {
185+
for _, v := range sdkErr.Validations {
186+
if v.Field == "resume_token" {
189187
// Unset the resume token for the next attempt
190-
tac.logger.Debug(tac.ctx, "server replied invalid resume token; unsetting for next connection attempt")
188+
tac.logger.Warn(tac.ctx, "failed to dial tailnet v2+ API: server replied invalid resume token; unsetting for next connection attempt")
191189
tac.resumeToken.Store(nil)
190+
didLog = true
192191
}
193192
}
194193
}
194+
if !didLog && !errors.Is(err, context.Canceled) {
195+
tac.logger.Error(tac.ctx, "failed to dial tailnet v2+ API", slog.Error(err), slog.F("sdk_err", sdkErr))
196+
}
195197
return nil, err
196198
}
197199
client, err := tailnet.NewDRPCClient(

‎codersdk/workspacesdk/connector_internal_test.go

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ func TestTailnetAPIConnector_Disconnects(t *testing.T) {
6161
DERPMapUpdateFrequency: time.Millisecond,
6262
DERPMapFn: func() *tailcfg.DERPMap { return <-derpMapCh },
6363
NetworkTelemetryHandler: func(batch []*proto.TelemetryEvent) {},
64-
ResumeTokenProvider: tailnet.InsecureTestResumeTokenProvider,
64+
ResumeTokenProvider: tailnet.NewInsecureTestResumeTokenProvider(),
6565
})
6666
require.NoError(t, err)
6767

@@ -185,12 +185,15 @@ func TestTailnetAPIConnector_ResumeToken(t *testing.T) {
185185
t.Logf("received resume token: %s", resumeToken)
186186
assert.Equal(t, expectResumeToken, resumeToken)
187187
if resumeToken != "" {
188-
peerID, err = resumeTokenProvider.ParseResumeToken(resumeToken)
188+
peerID, err = resumeTokenProvider.VerifyResumeToken(resumeToken)
189189
assert.NoError(t, err, "failed to parse resume token")
190190
if err != nil {
191-
httpapi.Write(ctx, w, http.StatusBadRequest, codersdk.Response{
191+
httpapi.Write(ctx, w, http.StatusUnauthorized, codersdk.Response{
192192
Message: CoordinateAPIInvalidResumeToken,
193193
Detail: err.Error(),
194+
Validations: []codersdk.ValidationError{
195+
{Field: "resume_token", Detail: CoordinateAPIInvalidResumeToken},
196+
},
194197
})
195198
return
196199
}
@@ -253,8 +256,8 @@ func (r resumeTokenProvider) GenerateResumeToken(peerID uuid.UUID) (*proto.Refre
253256
return r.genFn(peerID)
254257
}
255258

256-
// ParseResumeToken implements tailnet.ResumeTokenProvider.
257-
func (r resumeTokenProvider) ParseResumeToken(token string) (uuid.UUID, error) {
259+
// VerifyResumeToken implements tailnet.ResumeTokenProvider.
260+
func (r resumeTokenProvider) VerifyResumeToken(token string) (uuid.UUID, error) {
258261
return r.parseFn(token)
259262
}
260263

@@ -307,12 +310,15 @@ func TestTailnetAPIConnector_ResumeTokenFailure(t *testing.T) {
307310
)
308311
t.Logf("received resume token: %s", resumeToken)
309312
if resumeToken != "" {
310-
_, err = resumeTokenProvider.ParseResumeToken(resumeToken)
313+
_, err = resumeTokenProvider.VerifyResumeToken(resumeToken)
311314
assert.Error(t, err, "parse resume token should return an error")
312315
atomic.AddInt64(&didFail, 1)
313-
httpapi.Write(ctx, w, http.StatusBadRequest, codersdk.Response{
316+
httpapi.Write(ctx, w, http.StatusUnauthorized, codersdk.Response{
314317
Message: CoordinateAPIInvalidResumeToken,
315318
Detail: err.Error(),
319+
Validations: []codersdk.ValidationError{
320+
{Field: "resume_token", Detail: CoordinateAPIInvalidResumeToken},
321+
},
316322
})
317323
return
318324
}
@@ -385,7 +391,7 @@ func TestTailnetAPIConnector_TelemetrySuccess(t *testing.T) {
385391
NetworkTelemetryHandler: func(batch []*proto.TelemetryEvent) {
386392
testutil.RequireSendCtx(ctx, t, eventCh, batch)
387393
},
388-
ResumeTokenProvider: tailnet.InsecureTestResumeTokenProvider,
394+
ResumeTokenProvider: tailnet.NewInsecureTestResumeTokenProvider(),
389395
})
390396
require.NoError(t, err)
391397

‎enterprise/wsproxy/wsproxysdk/wsproxysdk_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -177,7 +177,7 @@ func TestDialCoordinator(t *testing.T) {
177177
DERPMapUpdateFrequency: time.Hour,
178178
DERPMapFn: func() *tailcfg.DERPMap { panic("not implemented") },
179179
NetworkTelemetryHandler: func(batch []*proto.TelemetryEvent) { panic("not implemented") },
180-
ResumeTokenProvider: agpl.InsecureTestResumeTokenProvider,
180+
ResumeTokenProvider: agpl.NewInsecureTestResumeTokenProvider(),
181181
})
182182
require.NoError(t, err)
183183

‎tailnet/coordinator_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -630,7 +630,7 @@ func TestRemoteCoordination(t *testing.T) {
630630
DERPMapUpdateFrequency: time.Hour,
631631
DERPMapFn: func() *tailcfg.DERPMap { panic("not implemented") },
632632
NetworkTelemetryHandler: func(batch []*proto.TelemetryEvent) { panic("not implemented") },
633-
ResumeTokenProvider: tailnet.InsecureTestResumeTokenProvider,
633+
ResumeTokenProvider: tailnet.NewInsecureTestResumeTokenProvider(),
634634
})
635635
require.NoError(t, err)
636636
sC, cC := net.Pipe()
@@ -682,7 +682,7 @@ func TestRemoteCoordination_SendsReadyForHandshake(t *testing.T) {
682682
DERPMapUpdateFrequency: time.Hour,
683683
DERPMapFn: func() *tailcfg.DERPMap { panic("not implemented") },
684684
NetworkTelemetryHandler: func(batch []*proto.TelemetryEvent) { panic("not implemented") },
685-
ResumeTokenProvider: tailnet.InsecureTestResumeTokenProvider,
685+
ResumeTokenProvider: tailnet.NewInsecureTestResumeTokenProvider(),
686686
})
687687
require.NoError(t, err)
688688
sC, cC := net.Pipe()

‎tailnet/resume.go

Lines changed: 72 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,10 @@
11
package tailnet
22

33
import (
4+
"context"
5+
"crypto/rand"
6+
"database/sql"
7+
"encoding/hex"
48
"encoding/json"
59
"time"
610

@@ -19,22 +23,81 @@ const (
1923
resumeTokenSigningAlgorithm = jose.HS512
2024
)
2125

22-
var InsecureTestResumeTokenProvider ResumeTokenProvider = ResumeTokenKeyProvider{
23-
key: [64]byte{1},
24-
expiry: time.Hour,
26+
// NewInsecureTestResumeTokenProvider returns a ResumeTokenProvider that uses a
27+
// random key with short expiry for testing purposes. If any errors occur while
28+
// generating the key, the function panics.
29+
func NewInsecureTestResumeTokenProvider() ResumeTokenProvider {
30+
key, err := GenerateResumeTokenSigningKey()
31+
if err != nil {
32+
panic(err)
33+
}
34+
return NewResumeTokenKeyProvider(key, time.Hour)
2535
}
2636

2737
type ResumeTokenProvider interface {
2838
GenerateResumeToken(peerID uuid.UUID) (*proto.RefreshResumeTokenResponse, error)
29-
ParseResumeToken(token string) (uuid.UUID, error)
39+
VerifyResumeToken(token string) (uuid.UUID, error)
40+
}
41+
42+
type ResumeTokenSigningKey [64]byte
43+
44+
func GenerateResumeTokenSigningKey() (ResumeTokenSigningKey, error) {
45+
var key ResumeTokenSigningKey
46+
_, err := rand.Read(key[:])
47+
if err != nil {
48+
return key, xerrors.Errorf("generate random key: %w", err)
49+
}
50+
return key, nil
51+
}
52+
53+
type ResumeTokenSigningKeyDatabaseStore interface {
54+
GetCoordinatorResumeTokenSigningKey(ctx context.Context) (string, error)
55+
UpsertCoordinatorResumeTokenSigningKey(ctx context.Context, key string) error
56+
}
57+
58+
// ResumeTokenSigningKeyFromDatabase retrieves the coordinator resume token
59+
// signing key from the database. If the key is not found, a new key is
60+
// generated and inserted into the database.
61+
func ResumeTokenSigningKeyFromDatabase(ctx context.Context, db ResumeTokenSigningKeyDatabaseStore) (ResumeTokenSigningKey, error) {
62+
var resumeTokenKey ResumeTokenSigningKey
63+
resumeTokenKeyStr, err := db.GetCoordinatorResumeTokenSigningKey(ctx)
64+
if err != nil && !xerrors.Is(err, sql.ErrNoRows) {
65+
return resumeTokenKey, xerrors.Errorf("get coordinator resume token key: %w", err)
66+
}
67+
if decoded, err := hex.DecodeString(resumeTokenKeyStr); err != nil || len(decoded) != len(resumeTokenKey) {
68+
b := make([]byte, len(resumeTokenKey))
69+
_, err := rand.Read(b)
70+
if err != nil {
71+
return resumeTokenKey, xerrors.Errorf("generate fresh coordinator resume token key: %w", err)
72+
}
73+
74+
resumeTokenKeyStr = hex.EncodeToString(b)
75+
err = db.UpsertCoordinatorResumeTokenSigningKey(ctx, resumeTokenKeyStr)
76+
if err != nil {
77+
return resumeTokenKey, xerrors.Errorf("insert freshly generated coordinator resume token key to database: %w", err)
78+
}
79+
}
80+
81+
resumeTokenKeyBytes, err := hex.DecodeString(resumeTokenKeyStr)
82+
if err != nil {
83+
return resumeTokenKey, xerrors.Errorf("decode coordinator resume token key from database: %w", err)
84+
}
85+
if len(resumeTokenKeyBytes) != len(resumeTokenKey) {
86+
return resumeTokenKey, xerrors.Errorf("coordinator resume token key in database is not the correct length, expect %d got %d", len(resumeTokenKey), len(resumeTokenKeyBytes))
87+
}
88+
copy(resumeTokenKey[:], resumeTokenKeyBytes)
89+
if resumeTokenKey == [64]byte{} {
90+
return resumeTokenKey, xerrors.Errorf("coordinator resume token key in database is empty")
91+
}
92+
return resumeTokenKey, nil
3093
}
3194

3295
type ResumeTokenKeyProvider struct {
33-
key [64]byte
96+
key ResumeTokenSigningKey
3497
expiry time.Duration
3598
}
3699

37-
func NewResumeTokenKeyProvider(key [64]byte, expiry time.Duration) ResumeTokenProvider {
100+
func NewResumeTokenKeyProvider(key ResumeTokenSigningKey, expiry time.Duration) ResumeTokenProvider {
38101
if expiry <= 0 {
39102
expiry = DefaultResumeTokenExpiry
40103
}
@@ -45,8 +108,8 @@ func NewResumeTokenKeyProvider(key [64]byte, expiry time.Duration) ResumeTokenPr
45108
}
46109

47110
type resumeTokenPayload struct {
48-
PeerID uuid.UUID `json:"peer_id"`
49-
Expiry time.Time `json:"expiry"`
111+
PeerID uuid.UUID `json:"sub"`
112+
Expiry time.Time `json:"exp"`
50113
}
51114

52115
func (p ResumeTokenKeyProvider) GenerateResumeToken(peerID uuid.UUID) (*proto.RefreshResumeTokenResponse, error) {
@@ -87,7 +150,7 @@ func (p ResumeTokenKeyProvider) GenerateResumeToken(peerID uuid.UUID) (*proto.Re
87150
// VerifySignedToken parses a signed workspace app token with the given key and
88151
// returns the payload. If the token is invalid or expired, an error is
89152
// returned.
90-
func (p ResumeTokenKeyProvider) ParseResumeToken(str string) (uuid.UUID, error) {
153+
func (p ResumeTokenKeyProvider) VerifyResumeToken(str string) (uuid.UUID, error) {
91154
object, err := jose.ParseSigned(str)
92155
if err != nil {
93156
return uuid.Nil, xerrors.Errorf("parse JWS: %w", err)

‎tailnet/service_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ func TestClientService_ServeClient_V2(t *testing.T) {
4040
NetworkTelemetryHandler: func(batch []*proto.TelemetryEvent) {
4141
telemetryEvents <- batch
4242
},
43-
ResumeTokenProvider: tailnet.InsecureTestResumeTokenProvider,
43+
ResumeTokenProvider: tailnet.NewInsecureTestResumeTokenProvider(),
4444
})
4545
require.NoError(t, err)
4646

@@ -145,7 +145,7 @@ func TestClientService_ServeClient_V1(t *testing.T) {
145145
DERPMapUpdateFrequency: 0,
146146
DERPMapFn: nil,
147147
NetworkTelemetryHandler: nil,
148-
ResumeTokenProvider: tailnet.InsecureTestResumeTokenProvider,
148+
ResumeTokenProvider: tailnet.NewInsecureTestResumeTokenProvider(),
149149
})
150150
require.NoError(t, err)
151151

‎tailnet/test/integration/integration.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -181,7 +181,7 @@ func (o SimpleServerOptions) Router(t *testing.T, logger slog.Logger) *chi.Mux {
181181
}
182182
},
183183
NetworkTelemetryHandler: func(batch []*tailnetproto.TelemetryEvent) {},
184-
ResumeTokenProvider: tailnet.InsecureTestResumeTokenProvider,
184+
ResumeTokenProvider: tailnet.NewInsecureTestResumeTokenProvider(),
185185
})
186186
require.NoError(t, err)
187187

0 commit comments

Comments
 (0)