10000 Use new secret prefix · coder/coder@8bbb308 · GitHub
[go: up one dir, main page]
Skip to content

Commit 8bbb308

Browse files
code-ashercode-asher
committed
Use new secret prefix
1 parent 2bebfbc commit 8bbb308

File tree

5 files changed

+193
-79
lines changed

5 files changed

+193
-79
lines changed

‎enterprise/coderd/identityprovider/authorize.go

Lines changed: 4 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,7 @@ import (
1414
"github.com/coder/coder/v2/coderd/database/dbtime"
1515
"github.com/coder/coder/v2/coderd/httpapi"
1616
"github.com/coder/coder/v2/coderd/httpmw"
17-
"github.com/coder/coder/v2/coderd/userpassword"
1817
"github.com/coder/coder/v2/codersdk"
19-
"github.com/coder/coder/v2/cryptorand"
2018
)
2119

2220
type authorizeParams struct {
@@ -81,21 +79,13 @@ func Authorize(db database.Store, accessURL *url.URL) http.HandlerFunc {
8179
}
8280

8381
// TODO: Ignoring scope for now, but should look into implementing.
84-
// 40 characters matches the length of GitHub's client secrets.
85-
rawCode, err := cryptorand.String(40)
82+
code, err := GenerateSecret()
8683
if err != nil {
8784
httpapi.Write(r.Context(), rw, http.StatusInternalServerError, codersdk.Response{
8885
Message: "Failed to generate OAuth2 app authorization code.",
8986
})
9087
return
9188
}
92-
hashedCode, err := userpassword.Hash(rawCode)
93-
if err != nil {
94-
httpapi.Write(r.Context(), rw, http.StatusInternalServerError, codersdk.Response{
95-
Message: "Failed to hash OAuth2 app authorization code.",
96-
})
97-
return
98-
}
9989
err = db.InTx(func(tx database.Store) error {
10090
// Delete any previous codes.
10191
err = tx.DeleteOAuth2ProviderAppCodesByAppAndUserID(ctx, database.DeleteOAuth2ProviderAppCodesByAppAndUserIDParams{
@@ -118,7 +108,8 @@ func Authorize(db database.Store, accessURL *url.URL) http.HandlerFunc {
118108
// token (for example suppose they ask the user to confirm and the user
119109
// has left) then they can just retry immediately and get a new code.
120110
ExpiresAt: dbtime.Now().Add(time.Duration(10) * time.Minute),
121-
HashedSecret: []byte(hashedCode),
111+
SecretPrefix: []byte(code.Prefix),
112+
HashedSecret: []byte(code.Hashed),
122113
AppID: app.ID,
123114
UserID: apiKey.UserID,
124115
})
@@ -137,7 +128,7 @@ func Authorize(db database.Store, accessURL *url.URL) http.HandlerFunc {
137128
}
138129

139130
newQuery := params.redirectURL.Query()
140-
newQuery.Add("code", rawCode)
131+
newQuery.Add("code", code.Formatted)
141132
newQuery.Add("state", params.state)
142133
params.redirectURL.RawQuery = newQuery.Encode()
143134

‎enterprise/coderd/identityprovider/secrets.go

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
package identityprovider
2+
3+
import (
4+
"fmt"
5+
"strings"
6+
7+
"golang.org/x/xerrors"
8+
9+
"github.com/coder/coder/v2/coderd/userpassword"
10+
"github.com/coder/coder/v2/cryptorand"
11+
)
12+
13+
type OAuth2ProviderAppSecret struct {
14+
Formatted string
15+
Prefix string
16+
Hashed string
17+
}
18+
19+
// GenerateSecret generates a secret to be used as a client secret, refresh
20+
// token, or authorization code.
21+
func GenerateSecret() (OAuth2ProviderAppSecret, error) {
22+
// 40 characters matches the length of GitHub's client secrets.
23+
secret, err := cryptorand.String(40)
24+
if err != nil {
25+
return OAuth2ProviderAppSecret{}, err
26+
}
27+
28+
// This ID is prefixed to the secret so it can be used to look up the secret
29+
// when the user provides it, since we cannot just re-hash it to match as we
30+
// will not have the salt.
31+
prefix, err := cryptorand.String(10)
32+
if err != nil {
33+
return OAuth2ProviderAppSecret{}, err
34+
}
35+
36+
hashed, err := userpassword.Hash(secret)
37+
if err != nil {
38+
return OAuth2ProviderAppSecret{}, err
39+
}
40+
41+
return OAuth2ProviderAppSecret{
42+
Formatted: fmt.Sprintf("coder_%s_%s", prefix, secret),
43+
Prefix: prefix,
44+
Hashed: hashed,
45+
}, nil
46+
}
47+
48+
// parseSecret extracts the ID and original secret from a secret.
49+
func parseSecret(secret string) (string, string, error) {
50+
parts := strings.Split(secret, "_")
51+
if len(parts) != 3 {
52+
return "", "", xerrors.Errorf("incorrect number of parts: %d", len(parts))
53+
}
54+
if parts[0] != "coder" {
55+
return "", "", xerrors.Errorf("incorrect scheme: %s", parts[0])
56+
}
57+
if len(parts[1]) == 0 {
58+
return "", "", xerrors.Errorf("prefix is invalid")
59+
}
60+
if len(parts[2]) == 0 {
61+
return "", "", xerrors.Errorf("invalid")
62+
}
63+
return parts[1], parts[2], nil
64+
}

‎enterprise/coderd/identityprovider/tokens.go

Lines changed: 51 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -22,9 +22,14 @@ import (
2222
"github.com/coder/coder/v2/coderd/rbac"
2323
"github.com/coder/coder/v2/coderd/userpassword"
2424
"github.com/coder/coder/v2/codersdk"
25-
"github.com/coder/coder/v2/cryptorand"
2625
)
2726

27+
// errBadSecret means the user provided a bad secret.
28+
var errBadSecret = errors.New("Invalid client secret")
29+
30+
// errBadCode means the user provided a bad code.
31+
var errBadCode = errors.New("Invalid code")
32+
2833
type tokenParams struct {
2934
clientID string
3035
clientSecret string
@@ -86,12 +91,12 @@ func Tokens(db database.Store, defaultLifetime time.Duration) http.HandlerFunc {
8691
switch params.grantType {
8792
// TODO: Client creds, device code, refresh.
8893
default:
89-
token, err = authorizationCodeGrant(ctx, db, app, defaultLifetime, params.clientSecret, params.code)
94+
token, err = authorizationCodeGrant(ctx, db, app, defaultLifetime, params)
9095
}
9196

92-
if err != nil && errors.Is(err, sql.ErrNoRows) {
97+
if errors.Is(err, errBadCode) || errors.Is(err, errBadSecret) {
9398
httpapi.Write(r.Context(), rw, http.StatusUnauthorized, codersdk.Response{
94-
Message: "Invalid client secret or code",
99+
Message: err.Error(),
95100
})
96101
return
97102
}
@@ -109,59 +114,68 @@ func Tokens(db database.Store, defaultLifetime time.Duration) http.HandlerFunc {
109114
}
110115
}
111116

112-
func authorizationCodeGrant(ctx context.Context, db database.Store, app database.OAuth2ProviderApp, defaultLifetime time.Duration, clientSecret, code string) (oauth2.Token, error) {
117+
func authorizationCodeGrant(ctx context.Context, db database.Store, app database.OAuth2ProviderApp, defaultLifetime time.Duration, params tokenParams) (oauth2.Token, error) {
113118
// Validate the client secret.
114-
secretHash, err := userpassword.Hash(clientSecret)
119+
secretPrefix, originalSecret, err := parseSecret(params.clientSecret)
115120
if err != nil {
116-
return oauth2.Token{}, err
121+
return oauth2.Token{}, errBadSecret
122+
}
123+
//nolint:gocritic // Users cannot read secrets so we must use the system.
124+
secret, err := db.GetOAuth2ProviderAppSecretByPrefix(dbauthz.AsSystemRestricted(ctx), []byte(secretPrefix))
125+
if errors.Is(err, sql.ErrNoRows) {
126+
return oauth2.Token{}, errBadSecret
117127
}
118-
secret, err := db.GetOAuth2ProviderAppSecretByAppIDAndSecret(
119-
//nolint:gocritic // Users cannot read secrets so we must use the system.
120-
dbauthz.AsSystemRestricted(ctx),
121-
database.GetOAuth2ProviderAppSecretByAppIDAndSecretParams{
122< E377 /td>-
AppID: app.ID,
123-
HashedSecret: []byte(secretHash),
124-
})
125128
if err != nil {
126129
return oauth2.Token{}, err
127130
}
131+
equal, err := userpassword.Compare(string(secret.HashedSecret), originalSecret)
132+
if err != nil {
133+
return oauth2.Token{}, xerrors.Errorf("unable to compare secret: %w", err)
134+
}
135+
if !equal {
136+
return oauth2.Token{}, errBadSecret
137+
}
128138

129139
// Validate the authorization code.
130-
codeHash, err := userpassword.Hash(code)
140+
codePrefix, originalCode, err := parseSecret(params.code)
131141
if err != nil {
132-
return oauth2.Token{}, err
142+
return oauth2.Token{}, errBadCode
143+
}
144+
//nolint:gocritic // There is no user yet so we must use the system.
145+
code, err := db.GetOAuth2ProviderAppCodeByPrefix(dbauthz.AsSystemRestricted(ctx), []byte(codePrefix))
146+
if errors.Is(err, sql.ErrNoRows) {
147+
return oauth2.Token{}, errBadCode
133148
}
134-
dbCode, err := db.GetOAuth2ProviderAppCodeByAppIDAndSecret(
135-
//nolint:gocritic // There is no user yet so we must use the system.
136-
dbauthz.AsSystemRestricted(ctx),
137-
database.GetOAuth2ProviderAppCodeByAppIDAndSecretParams{
138-
AppID: app.ID,
139-
HashedSecret: []byte(codeHash),
140-
})
141149
if err != nil {
142150
return oauth2.Token{}, err
143151
}
152+
equal, err = userpassword.Compare(string(code.HashedSecret), originalCode)
153+
if err != nil {
154+
return oauth2.Token{}, xerrors.Errorf("unable to compare code: %w", err)
155+
}
156+
if !equal {
157+
return oauth2.Token{}, errBadCode
158+
}
144159

145-
// Ensure the code has not expired. Make it look like no code.
146-
if dbCode.ExpiresAt.Before(dbtime.Now()) {
147-
return oauth2.Token{}, sql.ErrNoRows
160+
// Ensure the code has not expired.
161+
if code.ExpiresAt.Before(dbtime.Now()) {
162+
return oauth2.Token{}, errBadCode
148163
}
149164

150165
// Generate a refresh token.
151166
// The refresh token is not currently used or exposed though as API keys can
152167
// already be refreshed by just using them.
153168
// TODO: However, should we implement the refresh grant anyway?
154-
// 40 characters matches the length of GitHub's client secrets.
155-
rawRefreshToken, err := cryptorand.String(40)
169+
refreshToken, err := GenerateSecret()
156170
if err != nil {
157171
return oauth2.Token{}, err
158172
}
159173

160174
// Generate the API key we will swap for the code.
161175
// TODO: We are ignoring scopes for now.
162-
tokenName := fmt.Sprintf("%s_%s_oauth_session_token", dbCode.UserID, app.ID)
176+
tokenName := fmt.Sprintf("%s_%s_oauth_session_token", code.UserID, app.ID)
163177
key, sessionToken, err := apikey.Generate(apikey.CreateParams{
164-
UserID: dbCode.UserID,
178+
UserID: code.UserID,
165179
LoginType: database.LoginTypeOAuth2ProviderApp,
166180
// TODO: This is just the lifetime for api keys, maybe have its own config
167181
// settings. #11693
@@ -175,12 +189,12 @@ func authorizationCodeGrant(ctx context.Context, db database.Store, app database
175189

176190
// Grab the user roles so we can perform the exchange as the user.
177191
//nolint:gocritic // In the token exchange, there is no user actor.
178-
roles, err := db.GetAuthorizationUserRoles(dbauthz.AsSystemRestricted(ctx), dbCode.UserID)
192+
roles, err := db.GetAuthorizationUserRoles(dbauthz.AsSystemRestricted(ctx), code.UserID)
179193
if err != nil {
180194
return oauth2.Token{}, err
181195
}
182196
userSubj := rbac.Subject{
183-
ID: dbCode.UserID.String(),
197+
ID: code.UserID.String(),
184198
Roles: rbac.RoleNames(roles.Roles),
185199
Groups: roles.Groups,
186200
Scope: rbac.ScopeAll,
@@ -189,14 +203,14 @@ func authorizationCodeGrant(ctx context.Context, db database.Store, app database
189203
// Do the actual token exchange in the database.
190204
err = db.InTx(func(tx database.Store) error {
191205
ctx := dbauthz.As(ctx, userSubj)
192-
err = tx.DeleteOAuth2ProviderAppCodeByID(ctx, dbCode.ID)
206+
err = tx.DeleteOAuth2ProviderAppCodeByID(ctx, code.ID)
193207
if err != nil {
194208
return xerrors.Errorf("delete oauth2 app code: %w", err)
195209
}
196210

197211
// Delete the previous key, if any.
198212
prevKey, err := tx.GetAPIKeyByName(ctx, database.GetAPIKeyByNameParams{
199-
UserID: dbCode.UserID,
213+
UserID: code.UserID,
200214
TokenName: tokenName,
201215
})
202216
if err == nil {
@@ -211,15 +225,12 @@ func authorizationCodeGrant(ctx context.Context, db database.Store, app database
211225
return xerrors.Errorf("insert oauth2 access token: %w", err)
212226
}
213227

214-
refreshHash, err := userpassword.Hash(rawRefreshToken)
215-
if err != nil {
216-
return xerrors.Errorf("hash oauth2 refresh token: %w", err)
217-
}
218228
_, err = tx.InsertOAuth2ProviderAppToken(ctx, database.InsertOAuth2ProviderAppTokenParams{
219229
ID: uuid.New(),
220230
CreatedAt: dbtime.Now(),
221231
ExpiresAt: key.ExpiresAt,
222-
RefreshHash: []byte(refreshHash),
232+
HashPrefix: []byte(refreshToken.Prefix),
233+
RefreshHash: []byte(refreshToken.Hashed),
223234
AppSecretID: secret.ID,
224235
APIKeyID: newKey.ID,
225236
})
@@ -236,7 +247,7 @@ func authorizationCodeGrant(ctx context.Context, db database.Store, app database
236247
AccessToken: sessionToken,
237248
TokenType: "Bearer",
238249
// TODO: Exclude until refresh grant is implemented.
239-
// RefreshToken: rawRefreshToken,
250+
// RefreshToken: refreshToken.formatted,
240251
// Expiry: key.ExpiresAt,
241252
}, nil
242253
}

‎enterprise/coderd/oauth2.go

Lines changed: 8 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -12,9 +12,7 @@ import (
1212
"github.com/coder/coder/v2/coderd/database/dbtime"
1313
"github.com/coder/coder/v2/coderd/httpapi"
1414
"github.com/coder/coder/v2/coderd/httpmw"
15-
"github.com/coder/coder/v2/coderd/userpassword"
1615
"github.com/coder/coder/v2/codersdk"
17-
"github.com/coder/coder/v2/cryptorand"
1816
"github.com/coder/coder/v2/enterprise/coderd/identityprovider"
1917
)
2018

@@ -229,30 +227,23 @@ func (api *API) oAuth2ProviderAppSecrets(rw http.ResponseWriter, r *http.Request
229227
func (api *API) postOAuth2ProviderAppSecret(rw http.ResponseWriter, r *http.Request) {
230228
ctx := r.Context()
231229
app := httpmw.OAuth2ProviderApp(r)
232-
// 40 characters matches the length of GitHub's client secrets.
233-
rawSecret, err := cryptorand.String(40)
234-
if err != nil {
235-
httpapi.Write(r.Context(), rw, http.StatusInternalServerError, codersdk.Response{
236-
Message: "Failed to generate OAuth2 client secret.",
237-
})
238-
return
239-
}
240-
hashed, err := userpassword.Hash(rawSecret)
230+
secret, err := identityprovider.GenerateSecret()
241231
if err != nil {
242232
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
243-
Message: "Failed to hash OAuth2 client secret.",
233+
Message: "Failed to generate OAuth2 client secret.",
244234
Detail: err.Error(),
245235
})
246236
return
247237
}
248-
secret, err := api.Database.InsertOAuth2ProviderAppSecret(ctx, database.InsertOAuth2ProviderAppSecretParams{
238+
dbSecret, err := api.Database.InsertOAuth2ProviderAppSecret(ctx, database.InsertOAuth2ProviderAppSecretParams{
249239
ID: uuid.New(),
250240
CreatedAt: dbtime.Now(),
251-
HashedSecret: []byte(hashed),
241+
SecretPrefix: []byte(secret.Prefix),
242+
HashedSecret: []byte(secret.Hashed),
252243
// DisplaySecret is the last six characters of the original unhashed secret.
253244
// This is done so they can be differentiated and it matches how GitHub
254245
// displays their client secrets.
255-
DisplaySecret: rawSecret[len(rawSecret)-6:],
246+
DisplaySecret: secret.Formatted[len(secret.Formatted)-6:],
256247
AppID: app.ID,
257248
})
258249
if err != nil {
@@ -263,8 +254,8 @@ func (api *API) postOAuth2ProviderAppSecret(rw http.ResponseWriter, r *http.Requ
263254
return
264255
}
265256
httpapi.Write(ctx, rw, http.StatusOK, codersdk.OAuth2ProviderAppSecretFull{
266-
ID: secret.ID,
267-
ClientSecretFull: rawSecret,
257+
ID: dbSecret.ID,
258+
ClientSecretFull: secret.Formatted,
268259
})
269260
}
270261

0 commit comments

Comments
 (0)
0