Skip to content

Commit b38c1c8

Browse files
deansheatherdeansheather
committed
chore: change app signing key to be 96 bytes
1 parent 2e710e2 commit b38c1c8

File tree

13 files changed

+99
-59
lines changed

13 files changed

+99
-59
lines changed

‎cli/server.go

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -778,23 +778,31 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd.
778778
}
779779
}
780780

781-
// Read the app signing key from the DB. We store it hex
782-
// encoded since the config table uses strings for the value and
783-
// we don't want to deal with automatic encoding issues.
781+
// Read the app signing key from the DB. We store it hex encoded
782+
// since the config table uses strings for the value and we
783+
// don't want to deal with automatic encoding issues.
784784
appSigningKeyStr, err := tx.GetAppSigningKey(ctx)
785785
if err != nil && !xerrors.Is(err, sql.ErrNoRows) {
786786
return xerrors.Errorf("get app signing key: %w", err)
787787
}
788-
if appSigningKeyStr == "" {
789-
// Generate 64 byte secure random string.
790-
b := make([]byte, 64)
788+
// If the string in the DB is an invalid hex string or the
789+
// length is not equal to the current key length, generate a new
790+
// one.
791+
//
792+
// If the key is regenerated, old signed tokens and encrypted
793+
// strings will become invalid. New signed app tokens will be
794+
// generated automatically on failure. Any workspace app token
795+
// smuggling operations in progress may fail, although with a
796+
// helpful error.
797+
if decoded, err := hex.DecodeString(appSigningKeyStr); err != nil || len(decoded) != len(workspaceapps.SigningKey{}) {
798+
b := make([]byte, len(workspaceapps.SigningKey{}))
791799
_, err := rand.Read(b)
792800
if err != nil {
793801
return xerrors.Errorf("generate fresh app signing key: %w", err)
794802
}
795803

796804
appSigningKeyStr = hex.EncodeToString(b)
797-
err = tx.InsertAppSigningKey(ctx, appSigningKeyStr)
805+
err = tx.UpsertAppSigningKey(ctx, appSigningKeyStr)
798806
if err != nil {
799807
return xerrors.Errorf("insert freshly generated app signing key to database: %w", err)
800808
}

‎coderd/coderd.go

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -328,7 +328,7 @@ func New(options *Options) *API {
328328
api.workspaceAgentCache = wsconncache.New(api.dialWorkspaceAgentTailnet, 0)
329329
api.TailnetCoordinator.Store(&options.TailnetCoordinator)
330330

331-
workspaceAppServer := workspaceapps.Server{
331+
api.workspaceAppServer = &workspaceapps.Server{
332332
Logger: options.Logger.Named("workspaceapps"),
333333

334334
DashboardURL: api.AccessURL,
@@ -380,7 +380,7 @@ func New(options *Options) *API {
380380
//
381381
// Workspace apps do their own auth and must be BEFORE the auth
382382
// middleware.
383-
workspaceAppServer.SubdomainAppMW(apiRateLimiter),
383+
api.workspaceAppServer.SubdomainAppMW(apiRateLimiter),
384384
// Build-Version is helpful for debugging.
385385
func(next http.Handler) http.Handler {
386386
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
@@ -406,7 +406,7 @@ func New(options *Options) *API {
406406
// Attach workspace apps routes.
407407
r.Group(func(r chi.Router) {
408408
r.Use(apiRateLimiter)
409-
workspaceAppServer.Attach(r)
409+
api.workspaceAppServer.Attach(r)
410410
})
411411

412412
r.Route("/derp", func(r chi.Router) {
@@ -796,6 +796,7 @@ type API struct {
796796
workspaceAgentCache *wsconncache.Cache
797797
updateChecker *updatecheck.Checker
798798
WorkspaceAppsProvider workspaceapps.SignedTokenProvider
799+
workspaceAppServer *workspaceapps.Server
799800

800801
// Experiments contains the list of experiments currently enabled.
801802
// This is used to gate features that are not yet ready for production.

‎coderd/coderdtest/coderdtest.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -80,9 +80,9 @@ import (
8080
"github.com/coder/coder/testutil"
8181
)
8282

83-
// AppSigningKey is a 64-byte key used to sign JWTs for workspace app tokens in
84-
// tests.
85-
var AppSigningKey = must(workspaceapps.KeyFromString("64656164626565666465616462656566646561646265656664656164626565666465616462656566646561646265656664656164626565666465616462656566"))
83+
// AppSigningKey is a 64-byte key used to sign JWTs and encrypt JWEs for
84+
// workspace app tokens in tests.
85+
var AppSigningKey = must(workspaceapps.KeyFromString("6465616e207761732068657265206465616e207761732068657265206465616e207761732068657265206465616e207761732068657265206465616e207761732068657265206465616e207761732068657265206465616e2077617320686572"))
8686

8787
type Options struct {
8888
// AccessURL denotes a custom access URL. By default we use the httptest

‎coderd/database/dbauthz/querier.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -384,9 +384,9 @@ func (q *querier) GetAppSigningKey(ctx context.Context) (string, error) {
384384
return q.db.GetAppSigningKey(ctx)
385385
}
386386

387-
func (q *querier) InsertAppSigningKey(ctx context.Context, data string) error {
387+
func (q *querier) UpsertAppSigningKey(ctx context.Context, data string) error {
388388
// No authz checks as this is done during startup
389-
return q.db.InsertAppSigningKey(ctx, data)
389+
return q.db.UpsertAppSigningKey(ctx, data)
390390
}
391391

392392
func (q *querier) GetServiceBanner(ctx context.Context) (string, error) {

‎coderd/database/dbfake/databasefake.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4451,7 +4451,7 @@ func (q *fakeQuerier) GetAppSigningKey(_ context.Context) (string, error) {
44514451
return q.appSigningKey, nil
44524452
}
44534453

4454-
func (q *fakeQuerier) InsertAppSigningKey(_ context.Context, data string) error {
4454+
func (q *fakeQuerier) UpsertAppSigningKey(_ context.Context, data string) error {
44554455
q.mutex.Lock()
44564456
defer q.mutex.Unlock()
44574457

‎coderd/database/dbgen/generator.go

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,12 +77,23 @@ func APIKey(t testing.TB, db database.Store, seed database.APIKey) (key database
7777
secret, _ := cryptorand.String(22)
7878
hashed := sha256.Sum256([]byte(secret))
7979

80+
ip := seed.IPAddress
81+
if !ip.Valid {
82+
ip = pqtype.Inet{
83+
IPNet: net.IPNet{
84+
IP: net.IPv4(127, 0, 0, 1),
85+
Mask: net.IPv4Mask(255, 255, 255, 255),
86+
},
87+
Valid: true,
88+
}
89+
}
90+
8091
key, err := db.InsertAPIKey(context.Background(), database.InsertAPIKeyParams{
8192
ID: takeFirst(seed.ID, id),
8293
// 0 defaults to 86400 at the db layer
8394
LifetimeSeconds: takeFirst(seed.LifetimeSeconds, 0),
8495
HashedSecret: takeFirstSlice(seed.HashedSecret, hashed[:]),
85-
IPAddress: pqtype.Inet{},
96+
IPAddress: ip,
8697
UserID: takeFirst(seed.UserID, uuid.New()),
8798
LastUsed: takeFirst(seed.LastUsed, database.Now()),
8899
ExpiresAt: takeFirst(seed.ExpiresAt, database.Now().Add(time.Hour)),

‎coderd/database/querier.go

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries.sql.go

Lines changed: 4 additions & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries/siteconfig.sql

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,5 +34,6 @@ SELECT value FROM site_configs WHERE key = 'logo_url';
3434
-- name: GetAppSigningKey :one
3535
SELECT value FROM site_configs WHERE key = 'app_signing_key';
3636

37-
-- name: InsertAppSigningKey :exec
38-
INSERT INTO site_configs (key, value) VALUES ('app_signing_key', $1);
37+
-- name: UpsertAppSigningKey :exec
38+
INSERT INTO site_configs (key, value) VALUES ('app_signing_key', $1)
39+
ON CONFLICT (key) DO UPDATE set value = $1 WHERE site_configs.key = 'app_signing_key';

‎coderd/workspaceapps/db.go

Lines changed: 5 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -32,16 +32,12 @@ type DBTokenProvider struct {
3232
DeploymentValues *codersdk.DeploymentValues
3333
OAuth2Configs *httpmw.OAuth2Configs
3434
WorkspaceAgentInactiveTimeout time.Duration
35-
TokenSigningKey SigningKey
35+
SigningKey SigningKey
3636
}
3737

3838
var _ SignedTokenProvider = &DBTokenProvider{}
3939

40-
func NewDBTokenProvider(log slog.Logger, accessURL *url.URL, authz rbac.Authorizer, db database.Store, cfg *codersdk.DeploymentValues, oauth2Cfgs *httpmw.OAuth2Configs, workspaceAgentInactiveTimeout time.Duration, tokenSigningKey SigningKey) SignedTokenProvider {
41-
if len(tokenSigningKey) != 64 {
42-
panic("token signing key must be 64 bytes")
43-
}
44-
40+
func NewDBTokenProvider(log slog.Logger, accessURL *url.URL, authz rbac.Authorizer, db database.Store, cfg *codersdk.DeploymentValues, oauth2Cfgs *httpmw.OAuth2Configs, workspaceAgentInactiveTimeout time.Duration, signingKey SigningKey) SignedTokenProvider {
4541
if workspaceAgentInactiveTimeout == 0 {
4642
workspaceAgentInactiveTimeout = 1 * time.Minute
4743
}
@@ -54,15 +50,15 @@ func NewDBTokenProvider(log slog.Logger, accessURL *url.URL, authz rbac.Authoriz
5450
DeploymentValues: cfg,
5551
OAuth2Configs: oauth2Cfgs,
5652
WorkspaceAgentInactiveTimeout: workspaceAgentInactiveTimeout,
57-
TokenSigningKey: tokenSigningKey,
53+
SigningKey: signingKey,
5854
}
5955
}
6056

6157
func (p *DBTokenProvider) TokenFromRequest(r *http.Request) (*SignedToken, bool) {
6258
// Get the existing token from the request.
6359
tokenCookie, err := r.Cookie(codersdk.DevURLSignedAppTokenCookie)
6460
if err == nil {
65-
token, err := p.TokenSigningKey.VerifySignedToken(tokenCookie.Value)
61+
token, err := p.SigningKey.VerifySignedToken(tokenCookie.Value)
6662
if err == nil {
6763
req := token.Request.Normalize()
6864
err := req.Validate()
@@ -130,9 +126,6 @@ func (p *DBTokenProvider) CreateToken(ctx context.Context, rw http.ResponseWrite
130126
token.AgentID = dbReq.Agent.ID
131127
token.AppURL = dbReq.AppURL
132128

133-
// TODO(@deansheather): return an error if the agent is offline or the app
134-
// is not running.
135-
136129
// Verify the user has access to the app.
137130
authed, err := p.authorizeRequest(r.Context(), authz, dbReq)
138131
if err != nil {
@@ -196,7 +189,7 @@ func (p *DBTokenProvider) CreateToken(ctx context.Context, rw http.ResponseWrite
196189

197190
// Sign the token.
198191
token.Expiry = time.Now().Add(DefaultTokenExpiry)
199-
tokenStr, err := p.TokenSigningKey.SignToken(token)
192+
tokenStr, err := p.SigningKey.SignToken(token)
200193
if err != nil {
201194
WriteWorkspaceApp500(p.Logger, p.AccessURL, rw, r, &appReq, err, "generate token")
202195
return nil, "", false

‎coderd/workspaceapps/proxy.go

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"regexp"
1111
"strconv"
1212
"strings"
13+
"sync"
1314

1415
"github.com/go-chi/chi/v5"
1516
"github.com/google/uuid"
@@ -84,6 +85,22 @@ type Server struct {
8485
SignedTokenProvider SignedTokenProvider
8586
WorkspaceConnCache *wsconncache.Cache
8687
AppSigningKey SigningKey
88+
89+
websocketWaitMutex sync.Mutex
90+
websocketWaitGroup sync.WaitGroup
91+
}
92+
93+
// Close waits for all reconnecting-pty WebSocket connections to drain before
94+
// returning.
95+
func (s *Server) Close() error {
96+
s.websocketWaitMutex.Lock()
97+
s.websocketWaitGroup.Wait()
98+
s.websocketWaitMutex.Unlock()
99+
100+
// The caller must close the SignedTokenProvider (if necessary) and the
101+
// wsconncache.
102+
103+
return nil
87104
}
88105

89106
func (s *Server) Attach(r chi.Router) {
@@ -503,11 +520,10 @@ func (s *Server) proxyWorkspaceApp(rw http.ResponseWriter, r *http.Request, appT
503520
func (s *Server) workspaceAgentPTY(rw http.ResponseWriter, r *http.Request) {
504521
ctx := r.Context()
505522

506-
// TODO: Fix this later
507-
// s.WebsocketWaitMutex.Lock()
508-
// s.WebsocketWaitGroup.Add(1)
509-
// s.WebsocketWaitMutex.Unlock()
510-
// defer s.WebsocketWaitGroup.Done()
523+
s.websocketWaitMutex.Lock()
524+
s.websocketWaitGroup.Add(1)
525+
s.websocketWaitMutex.Unlock()
526+
defer s.websocketWaitGroup.Done()
511527

512528
appToken, ok := ResolveRequest(s.Logger, s.AccessURL, s.SignedTokenProvider, rw, r, Request{
513529
AccessMethod: AccessMethodTerminal,

‎coderd/workspaceapps/token.go

Lines changed: 17 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,10 @@ import (
1313
"github.com/coder/coder/coderd/database"
1414
)
1515

16-
const tokenSigningAlgorithm = jose.HS512
16+
const (
17+
tokenSigningAlgorithm = jose.HS512
18+
apiKeyEncryptionAlgorithm = jose.A256GCMKW
19+
)
1720

1821
// SignedToken is the struct data contained inside a workspace app JWE. It
1922
// contains the details of the workspace app that the token is valid for to
@@ -41,14 +44,11 @@ func (t SignedToken) MatchesRequest(req Request) bool {
4144
t.AppSlugOrPort == req.AppSlugOrPort
4245
}
4346

44-
// SigningKey is used for signing and encrypting app tokens and API keys.
45-
// TODO: we cannot use the same key for signing and encrypting with two
46-
// different algorithms, that's a security issue. We should use a different key
47-
// for each.
48-
// OR
49-
// We get rid of signing and use encryption for both api keys and tickets.
50-
// Do this by switching to JWE.
51-
type SigningKey [64]byte
47+
// AppSigningKey is used for signing and encrypting app tokens and API keys.
48+
//
49+
// The first 64 bytes of the key are used for signing tokens with HMAC-SHA256,
50+
// and the last 32 bytes are used for encrypting API keys with AES-256-GCM.
51+
type SigningKey [96]byte
5252

5353
func KeyFromString(str string) (SigningKey, error) {
5454
var key SigningKey
@@ -78,7 +78,7 @@ func (k SigningKey) SignToken(payload SignedToken) (string, error) {
7878

7979
signer, err := jose.NewSigner(jose.SigningKey{
8080
Algorithm: tokenSigningAlgorithm,
81-
Key: k[:],
81+
Key: k[:64],
8282
}, nil)
8383
if err != nil {
8484
return "", xerrors.Errorf("create signer: %w", err)
@@ -112,7 +112,7 @@ func (k SigningKey) VerifySignedToken(str string) (SignedToken, error) {
112112
return SignedToken{}, xerrors.Errorf("expected token signing algorithm to be %q, got %q", tokenSigningAlgorithm, object.Signatures[0].Header.Algorithm)
113113
}
114114

115-
output, err := object.Verify(k[:])
115+
output, err := object.Verify(k[:64])
116116
if err != nil {
117117
return SignedToken{}, xerrors.Errorf("verify JWS: %w", err)
118118
}
@@ -154,8 +154,8 @@ func (k SigningKey) EncryptAPIKey(payload EncryptedAPIKeyPayload) (string, error
154154
encrypter, err := jose.NewEncrypter(
155155
jose.A256GCM,
156156
jose.Recipient{
157-
Algorithm: jose.A256GCMKW,
158-
Key: k[:32],
157+
Algorithm: apiKeyEncryptionAlgorithm,
158+
Key: k[64:],
159159
},
160160
&jose.EncrypterOptions{
161161
Compression: jose.DEFLATE,
@@ -184,9 +184,12 @@ func (k SigningKey) DecryptAPIKey(encryptedAPIKey string) (string, error) {
184184
if err != nil {
185185
return "", xerrors.Errorf("parse encrypted API key: %w", err)
186186
}
187+
if object.Header.Algorithm != string(apiKeyEncryptionAlgorithm) {
188+
return "", xerrors.Errorf("expected API key encryption algorithm to be %q, got %q", apiKeyEncryptionAlgorithm, object.Header.Algorithm)
189+
}
187190

188191
// Decrypt using the hashed secret.
189-
decrypted, err := object.Decrypt(k[:32])
192+
decrypted, err := object.Decrypt(k[64:])
190193
if err != nil {
191194
return "", xerrors.Errorf("decrypt API key: %w", err)
192195
}

‎coderd/workspaceapps/token_test.go

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -301,8 +301,11 @@ func Test_ParseToken(t *testing.T) {
301301
t.Parallel()
302302

303303
// Create a valid token using a different key.
304-
otherKey, err := workspaceapps.KeyFromString("62656566646561646265656664656164626565666465616462656566646561646265656664656164626565666465616462656566646561646265656664656164")
305-
require.NoError(t, err)
304+
var otherKey workspaceapps.SigningKey
305+
copy(otherKey[:], coderdtest.AppSigningKey[:])
306+
for i := range otherKey {
307+
otherKey[i] ^= 0xff
308+
}
306309
require.NotEqual(t, coderdtest.AppSigningKey, otherKey)
307310

308311
tokenStr, err := otherKey.SignToken(workspaceapps.SignedToken{
@@ -334,7 +337,7 @@ func Test_ParseToken(t *testing.T) {
334337
t.Parallel()
335338

336339
// Create a signature for an invalid body.
337-
signer, err := jose.NewSigner(jose.SigningKey{Algorithm: jose.HS512, Key: coderdtest.AppSigningKey[:]}, nil)
340+
signer, err := jose.NewSigner(jose.SigningKey{Algorithm: jose.HS512, Key: coderdtest.AppSigningKey[:64]}, nil)
338341
require.NoError(t, err)
339342
signedObject, err := signer.Sign([]byte("hi"))
340343
require.NoError(t, err)
@@ -395,8 +398,11 @@ func TestAPIKeyEncryption(t *testing.T) {
395398
t.Parallel()
396399

397400
// Create a valid token using a different key.
398-
otherKey, err := workspaceapps.KeyFromString("62656566646561646265656664656164626565666465616462656566646561646265656664656164626565666465616462656566646561646265656664656164")
399-
require.NoError(t, err)
401+
var otherKey workspaceapps.SigningKey
402+
copy(otherKey[:], coderdtest.AppSigningKey[:])
403+
for i := range otherKey {
404+
otherKey[i] ^= 0xff
405+
}
400406
require.NotEqual(t, coderdtest.AppSigningKey, otherKey)
401407

402408
// Encrypt with the other key.

0 commit comments

Comments
 (0)