8000 Refactor key management and enhance logging · coder/coder@7557ed2 · GitHub
[go: up one dir, main page]
Skip to content

Commit 7557ed2

Browse files
sreyasreya
committed
Refactor key management and enhance logging
- Improve clarity by naming loggers used in key cache creation. - Adjust key cache context to utilize KeyReader for consistent context handling. - Refactor API to use central key cache management approach. - Enhance error messages for crypto key fetching. - Update test to add safety against unexpected panics.
1 parent b770762 commit 7557ed2

File tree

9 files changed

+75
-61
lines changed

9 files changed

+75
-61
lines changed

‎coderd/coderd.go

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -461,7 +461,7 @@ func New(options *Options) *API {
461461

462462
if options.OIDCConvertKeyCache == nil {
463463
options.OIDCConvertKeyCache, err = cryptokeys.NewSigningCache(ctx,
464-
options.Logger,
464+
options.Logger.Named("oidc_convert_keycache"),
465465
fetcher,
466466
codersdk.CryptoKeyFeatureOIDCConvert,
467467
)
@@ -470,7 +470,7 @@ func New(options *Options) *API {
470470

471471
if options.AppSigningKeyCache == nil {
472472
options.AppSigningKeyCache, err = cryptokeys.NewSigningCache(ctx,
473-
options.Logger,
473+
options.Logger.Named("app_signing_keycache"),
474474
fetcher,
475475
codersdk.CryptoKeyFeatureWorkspaceAppsToken,
476476
)
@@ -683,10 +683,9 @@ func New(options *Options) *API {
683683
AgentProvider: api.agentProvider,
684684
StatsCollector: workspaceapps.NewStatsCollector(options.WorkspaceAppsStatsCollectorOptions),
685685

686-
DisablePathApps: options.DeploymentValues.DisablePathApps.Value(),
687-
SecureAuthCookie: options.DeploymentValues.SecureAuthCookie.Value(),
688-
Signer: options.AppSigningKeyCache,
689-
EncryptingKeyManager: options.AppEncryptionKeyCache,
686+
DisablePathApps: options.DeploymentValues.DisablePathApps.Value(),
687+
SecureAuthCookie: options.DeploymentValues.SecureAuthCookie.Value(),
688+
APIKeyEncryptionKey: options.AppEncryptionKeyCache,
690689
}
691690

692691
apiKeyMiddleware := httpmw.ExtractAPIKeyMW(httpmw.ExtractAPIKeyConfig{

‎coderd/cryptokeys/cache.go

Lines changed: 16 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import (
1414
"cdr.dev/slog"
1515
"github.com/coder/coder/v2/coderd/database"
1616
"github.com/coder/coder/v2/coderd/database/db2sdk"
17+
"github.com/coder/coder/v2/coderd/database/dbauthz"
1718
"github.com/coder/coder/v2/codersdk"
1819
"github.com/coder/quartz"
1920
)
@@ -77,12 +78,12 @@ func (d *DBFetcher) Fetch(ctx context.Context, feature codersdk.CryptoKeyFeature
7778

7879
// cache implements the caching functionality for both signing and encryption keys.
7980
type cache struct {
80-
clock quartz.Clock
81-
refreshCtx context.Context
82-
refreshCancel context.CancelFunc
83-
fetcher Fetcher
84-
logger slog.Logger
85-
feature codersdk.CryptoKeyFeature
81+
ctx context.Context
82+
cancel context.CancelFunc
83+
clock quartz.Clock
84+
fetcher Fetcher
85+
logger slog.Logger
86+
feature codersdk.CryptoKeyFeature
8687

8788
mu sync.Mutex
8889
keys map[int32]codersdk.CryptoKey
@@ -136,12 +137,13 @@ func newCache(ctx context.Context, logger slog.Logger, fetcher Fetcher, feature
136137
}
137138

138139
cache.cond = sync.NewCond(&cache.mu)
139-
cache.refreshCtx, cache.refreshCancel = context.WithCancel(ctx)
140+
//nolint:gocritic // We need to be able to read the keys in order to cache them.
141+
cache.ctx, cache.cancel = context.WithCancel(dbauthz.AsKeyReader(ctx))
140142
cache.refresher = cache.clock.AfterFunc(refreshInterval, cache.refresh)
141143

142-
keys, err := cache.cryptoKeys(ctx)
144+
keys, err := cache.cryptoKeys(cache.ctx)
143145
if err != nil {
144-
cache.refreshCancel()
146+
cache.cancel()
145147
return nil, xerrors.Errorf("initial fetch: %w", err)
146148
}
147149
cache.keys = keys
@@ -205,7 +207,7 @@ func isEncryptionKeyFeature(feature codersdk.CryptoKeyFeature) bool {
205207

206208
func isSigningKeyFeature(feature codersdk.CryptoKeyFeature) bool {
207209
switch feature {
208-
case codersdk.CryptoKeyFeatureTailnetResume, codersdk.CryptoKeyFeatureOIDCConvert:
210+
case codersdk.CryptoKeyFeatureTailnetResume, codersdk.CryptoKeyFeatureOIDCConvert, codersdk.CryptoKeyFeatureWorkspaceAppsToken:
209211
return true
210212
default:
211213
return false
@@ -315,9 +317,9 @@ func (c *cache) refresh() {
315317
c.fetching = true
316318

317319
c.mu.Unlock()
318-
keys, err := c.cryptoKeys(c.refreshCtx)
320+
keys, err := c.cryptoKeys(c.ctx)
319321
if err != nil {
320-
c.logger.Error(c.refreshCtx, "fetch crypto keys", slog.Error(err))
322+
c.logger.Error(c.ctx, "fetch crypto keys", slog.Error(err))
321323
return
322324
}
323325

@@ -336,7 +338,7 @@ func (c *cache) refresh() {
336338
func (c *cache) cryptoKeys(ctx context.Context) (map[int32]codersdk.CryptoKey, error) {
337339
keys, err := c.fetcher.Fetch(ctx, c.feature)
338340
if err != nil {
339-
return nil, xerrors.Errorf("crypto keys: %w", err)
341+
return nil, xerrors.Errorf("fetch: %w", err)
340342
}
341343
cache := toKeyMap(keys, c.clock.Now())
342344
return cache, nil
@@ -363,7 +365,7 @@ func (c *cache) Close() error {
363365
}
364366

365367
c.closed = true
366-
c.refreshCancel()
368+
c.cancel()
367369
c.refresher.Stop()
368370
c.cond.Broadcast()
369371

‎coderd/userauth.go

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,9 @@ func (api *API) postConvertLoginType(rw http.ResponseWriter, r *http.Request) {
167167
ToLoginType: req.ToType,
168168
}
169169

170-
token, err := jwtutils.Sign(dbauthz.AsKeyRotator(ctx), api.OIDCConvertKeyCache, claims)
170+
//nolint:gocritic // We need to read the system signing key
171+
// in order to sign the token.
172+
token, err := jwtutils.Sign(dbauthz.AsKeyReader(ctx), api.OIDCConvertKeyCache, claims)
171173
if err != nil {
172174
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
173175
Message: "Internal error signing state jwt.",
@@ -1676,7 +1678,10 @@ func (api *API) convertUserToOauth(ctx context.Context, r *http.Request, db data
16761678
}
16771679
}
16781680
var claims OAuthConvertStateClaims
1679-
err = jwtutils.Verify(dbauthz.AsKeyRotator(ctx), api.OIDCConvertKeyCache, jwtCookie.Value, &claims)
1681+
1682+
//nolint:gocritic // We need to read the system signing key
1683+
// in order to verify the token.
1684+
err = jwtutils.Verify(dbauthz.AsKeyReader(ctx), api.OIDCConvertKeyCache, jwtCookie.Value, &claims)
16801685
if xerrors.Is(err, cryptokeys.ErrKeyNotFound) || xerrors.Is(err, cryptokeys.ErrKeyInvalid) || xerrors.Is(err, jose.ErrCryptoFailure) {
16811686
// These errors are probably because the user is mixing 2 coder deployments.
16821687
return database.User{}, idpsync.HTTPError{

‎coderd/workspaceapps/proxy.go

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -100,9 +100,8 @@ type Server struct {
100100
HostnameRegex *regexp.Regexp
101101
RealIPConfig *httpmw.RealIPConfig
102102

103-
SignedTokenProvider SignedTokenProvider
104-
Signer jwtutils.SigningKeyManager
105-
EncryptingKeyManager jwtutils.EncryptingKeyManager
103+
SignedTokenProvider SignedTokenProvider
104+
APIKeyEncryptionKey jwtutils.EncryptingKeyManager
106105

107106
// DisablePathApps disables path-based apps. This is a security feature as path
108107
// based apps share the same cookie as the dashboard, and are susceptible to XSS
@@ -181,7 +180,7 @@ func (s *Server) handleAPIKeySmuggling(rw http.ResponseWriter, r *http.Request,
181180

182181
// Exchange the encoded API key for a real one.
183182
var payload EncryptedAPIKeyPayload
184-
err := jwtutils.Decrypt(ctx, s.EncryptingKeyManager, encryptedAPIKey, &payload, jwtutils.WithDecryptExpected(jwt.Expected{
183+
err := jwtutils.Decrypt(ctx, s.APIKeyEncryptionKey, encryptedAPIKey, &payload, jwtutils.WithDecryptExpected(jwt.Expected{
185184
Time: time.Now(),
186185
}))
187186
if err != nil {

‎coderd/workspaceapps/token.go

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,19 +5,13 @@ import (
55
"strings"
66
"time"
77

8-
"github.com/go-jose/go-jose/v4"
98
"github.com/go-jose/go-jose/v4/jwt"
109
"github.com/google/uuid"
1110

1211
"github.com/coder/coder/v2/coderd/jwtutils"
1312
"github.com/coder/coder/v2/codersdk"
1413
)
1514

16-
const (
17-
tokenSigningAlgorithm = jose.HS512
18-
apiKeyEncryptionAlgorithm = jose.A256GCMKW
19-
)
20-
2115
// SignedToken is the struct data contained inside a workspace app JWE. It
2216
// contains the details of the workspace app that the token is valid for to
2317
// avoid database queries.

‎enterprise/wsproxy/tokenprovider.go

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -19,14 +19,14 @@ type TokenProvider struct {
1919
AccessURL *url.URL
2020
AppHostname string
2121

22-
Client *wsproxysdk.Client
23-
SigningKey jwtutils.SigningKeyManager
24-
EncryptingKey jwtutils.EncryptingKeyManager
25-
Logger slog.Logger
22+
Client *wsproxysdk.Client
23+
TokenSigningKey jwtutils.SigningKeyManager
24+
APIKeyEncryptionKey jwtutils.EncryptingKeyManager
25+
Logger slog.Logger
2626
}
2727

2828
func (p *TokenProvider) FromRequest(r *http.Request) (*workspaceapps.SignedToken, bool) {
29-
return workspaceapps.FromRequest(r, p.SigningKey)
29+
return workspaceapps.FromRequest(r, p.TokenSigningKey)
3030
}
3131

3232
func (p *TokenProvider) Issue(ctx context.Context, rw http.ResponseWriter, r *http.Request, issueReq workspaceapps.IssueTokenRequest) (*workspaceapps.SignedToken, string, bool) {
@@ -45,7 +45,7 @@ func (p *TokenProvider) Issue(ctx context.Context, rw http.ResponseWriter, r *ht
4545

4646
// Check that it verifies properly and matches the string.
4747
var token workspaceapps.SignedToken
48-
err = jwtutils.Verify(ctx, p.SigningKey, resp.SignedTokenStr, &token)
48+
err = jwtutils.Verify(ctx, p.TokenSigningKey, resp.SignedTokenStr, &token)
4949
if err != nil {
5050
workspaceapps.WriteWorkspaceApp500(p.Logger, p.DashboardURL, rw, r, &appReq, err, "failed to verify newly generated signed token")
5151
return nil, "", false

‎enterprise/wsproxy/wsproxy.go

Lines changed: 31 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -131,8 +131,12 @@ type Server struct {
131131
// the moon's token.
132132
SDKClient *wsproxysdk.Client
133133

134-
WorkspaceAppsEncryptionKeycache cryptokeys.EncryptionKeycache
135-
WorkspaceAppsSigningKeycache cryptokeys.SigningKeycache
134+
// apiKeyEncryptionKeycache manages the encryption keys for smuggling API
135+
// tokens to the alternate domain when using workspace apps.
136+
apiKeyEncryptionKeycache cryptokeys.EncryptionKeycache
137+
// appTokenSigningKeycache manages the signing keys for signing the app
138+
// tokens we use for workspace apps.
139+
appTokenSigningKeycache cryptokeys.SigningKeycache
136140

137141
// DERP
138142
derpMesh *derpmesh.Mesh
@@ -206,6 +210,7 @@ func New(ctx context.Context, opts *Options) (*Server, error) {
206210
codersdk.CryptoKeyFeatureWorkspaceAppsAPIKey,
207211
)
208212
if err != nil {
213+
cancel()
209214
return nil, xerrors.Errorf("create api key encryption cache: %w", err)
210215
}
211216
signingCache, err := cryptokeys.NewSigningCache(ctx,
@@ -214,6 +219,7 @@ func New(ctx context.Context, opts *Options) (*Server, error) {
214219
codersdk.CryptoKeyFeatureWorkspaceAppsToken,
215220
)
216221
if err != nil {
222+
cancel()
217223
return nil, xerrors.Errorf("create api token signing cache: %w", err)
218224
}
219225

@@ -222,15 +228,17 @@ func New(ctx context.Context, opts *Options) (*Server, error) {
222228
ctx: ctx,
223229
cancel: cancel,
224230

225-
Options: opts,
226-
Handler: r,
227-
DashboardURL: opts.DashboardURL,
228-
Logger: opts.Logger.Named("net.workspace-proxy"),
229-
TracerProvider: opts.Tracing,
230-
PrometheusRegistry: opts.PrometheusRegistry,
231-
SDKClient: client,
232-
derpMesh: derpmesh.New(opts.Logger.Named("net.derpmesh"), derpServer, meshTLSConfig),
233-
derpMeshTLSConfig: meshTLSConfig,
231+
Options: opts,
232+
Handler: r,
233+
DashboardURL: opts.DashboardURL,
234+
Logger: opts.Logger.Named("net.workspace-proxy"),
235+
TracerProvider: opts.Tracing,
236+
PrometheusRegistry: opts.PrometheusRegistry,
237+
SDKClient: client,
238+
derpMesh: derpmesh.New(opts.Logger.Named("net.derpmesh"), derpServer, meshTLSConfig),
239+
derpMeshTLSConfig: meshTLSConfig,
240+
apiKeyEncryptionKeycache: encryptionCache,
241+
appTokenSigningKeycache: signingCache,
234242
}
235243

236244
// Register the workspace proxy with the primary coderd instance and start a
@@ -295,20 +303,21 @@ func New(ctx context.Context, opts *Options) (*Server, error) {
295303
HostnameRegex: opts.AppHostnameRegex,
296304
RealIPConfig: opts.RealIPConfig,
297305
SignedTokenProvider: &TokenProvider{
298-
DashboardURL: opts.DashboardURL,
299-
AccessURL: opts.AccessURL,
300-
AppHostname: opts.AppHostname,
301-
Client: client,
302-
SigningKey: signingCache,
303-
EncryptingKey: encryptionCache,
304-
Logger: s.Logger.Named("proxy_token_provider"),
306+
DashboardURL: opts.DashboardURL,
307+
AccessURL: opts.AccessURL,
308+
AppHostname: opts.AppHostname,
309+
Client: client,
310+
TokenSigningKey: signingCache,
311+
APIKeyEncryptionKey: encryptionCache,
312+
Logger: s.Logger.Named("proxy_token_provider"),
305313
},
306314

307315
DisablePathApps: opts.DisablePathApps,
308316
SecureAuthCookie: opts.SecureAuthCookie,
309317

310-
AgentProvider: agentProvider,
311-
StatsCollector: workspaceapps.NewStatsCollector(opts.StatsCollectorOptions),
318+
AgentProvider: agentProvider,
319+
StatsCollector: workspaceapps.NewStatsCollector(opts.StatsCollectorOptions),
320+
APIKeyEncryptionKey: encryptionCache,
312321
}
313322

314323
derpHandler := derphttp.Handler(derpServer)
@@ -451,8 +460,8 @@ func (s *Server) Close() error {
451460
err = multierror.Append(err, agentProviderErr)
452461
}
453462
s.SDKClient.SDKClient.HTTPClient.CloseIdleConnections()
454-
_ = s.WorkspaceAppsSigningKeycache.Close()
455-
_ = s.WorkspaceAppsEncryptionKeycache.Close()
463+
_ = s.appTokenSigningKeycache.Close()
464+
_ = s.apiKeyEncryptionKeycache.Close()
456465
return err
457466
}
458467

‎enterprise/wsproxy/wsproxy_test.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"net/http"
99
"net/http/httptest"
1010
"net/url"
11+
"runtime/debug"
1112
"testing"
1213
"time"
1314

@@ -321,6 +322,11 @@ resourceLoop:
321322

322323
func TestDERPEndToEnd(t *testing.T) {
323324
t.Parallel()
325+
defer func() {
326+
if r := recover(); r != nil {
327+
debug.PrintStack()
328+
}
329+
}()
324330

325331
deploymentValues := coderdtest.DeploymentValues(t)
326332
deploymentValues.Experiments = []string{

‎enterprise/wsproxy/wsproxysdk/wsproxysdk.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -581,7 +581,7 @@ type CryptoKeysResponse struct {
581581

582582
func (c *Client) CryptoKeys(ctx context.Context, feature codersdk.CryptoKeyFeature) (CryptoKeysResponse, error) {
583583
res, err := c.Request(ctx, http.MethodGet,
584-
"/api/v2/workspaceproxies/me/crypto-keys",
584+
"/api/v2/workspaceproxies/me/crypto-keys", nil,
585585
codersdk.WithQueryParam("feature", string(feature)),
586586
)
587587
if err != nil {

0 commit comments

Comments
 (0)
0