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

feat: Longer lived api keys for cli#1935

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
Emyrk merged 5 commits intomainfromstevenmasley/long_api_keys
Jun 1, 2022
Merged
Show file tree
Hide file tree
Changes fromall commits
Commits
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletionscoderd/database/databasefake/databasefake.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -1126,9 +1126,14 @@ func (q *fakeQuerier) InsertAPIKey(_ context.Context, arg database.InsertAPIKeyP
q.mutex.Lock()
defer q.mutex.Unlock()

if arg.LifetimeSeconds == 0 {
arg.LifetimeSeconds = 86400
}

//nolint:gosimple
key := database.APIKey{
ID: arg.ID,
LifetimeSeconds: arg.LifetimeSeconds,
HashedSecret: arg.HashedSecret,
UserID: arg.UserID,
ExpiresAt: arg.ExpiresAt,
Expand Down
3 changes: 2 additions & 1 deletioncoderd/database/dump.sql
View file
Open in desktop

Some generated files are not rendered by default. Learn more abouthow customized files appear on GitHub.

View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
ALTER TABLE api_keys DROP COLUMN lifetime_seconds;
2 changes: 2 additions & 0 deletionscoderd/database/migrations/000016_api_key_lifetime.up.sql
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
-- Default lifetime is 24hours.
ALTER TABLE api_keys ADD COLUMN lifetime_seconds bigint default 86400 NOT NULL;
1 change: 1 addition & 0 deletionscoderd/database/models.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more abouthow customized files appear on GitHub.

15 changes: 13 additions & 2 deletionscoderd/database/queries.sql.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more abouthow customized files appear on GitHub.

9 changes: 8 additions & 1 deletioncoderd/database/queries/apikeys.sql
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -12,6 +12,7 @@ LIMIT
INSERT INTO
api_keys (
id,
lifetime_seconds,
hashed_secret,
user_id,
last_used,
Expand All@@ -25,7 +26,13 @@ INSERT INTO
oauth_expiry
)
VALUES
($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12) RETURNING *;
(@id,
-- If the lifetime is set to 0, default to 24hrs
CASE @lifetime_seconds::bigint
WHEN 0 THEN 86400
ELSE @lifetime_seconds::bigint
END
Comment on lines +31 to +34
Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

If the value is 0, we default to 24hrs

Comment on lines +31 to +34
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Shouldn't this be a default when creating the column rather than on insert?

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

It is a default on the column, but you cannot insert anil value to the auto-generated code. So the default zero value in the insert code is0. I want to treat that as a default of 24hrs. The column default was mainly for the migration to be happy.

The alternative is to put this default in the Go code, but we don't have a layer to gatekeep db functions. So I'd rather put this in the SQL where it's guaranteed to be checked.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

If desired, we can fix this to useNULL to indicate a default value rather than0 when the next version of sqlc is released, with support forsqlc.narg.

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@dwahler yes. Ifnarg or I also sawsqlc.arg(name, nullable) is supported, then we can do exactly that 👍. And use the column default.

, @hashed_secret, @user_id, @last_used, @expires_at, @created_at, @updated_at, @login_type, @oauth_access_token, @oauth_refresh_token, @oauth_id_token, @oauth_expiry) RETURNING *;

-- name: UpdateAPIKeyByID :exec
UPDATE
Expand Down
2 changes: 1 addition & 1 deletioncoderd/httpmw/apikey.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -153,7 +153,7 @@ func ExtractAPIKey(db database.Store, oauth *OAuth2Configs) func(http.Handler) h
}
// Only update the ExpiresAt once an hour to prevent database spam.
// We extend the ExpiresAt to reduce re-authentication.
apiKeyLifetime :=24 * time.Hour
apiKeyLifetime :=time.Duration(key.LifetimeSeconds) * time.Second
if key.ExpiresAt.Sub(now) <= apiKeyLifetime-time.Hour {
key.ExpiresAt = now.Add(apiKeyLifetime)
changed = true
Expand Down
22 changes: 19 additions & 3 deletionscoderd/users.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -658,9 +658,14 @@ func (api *API) postAPIKey(rw http.ResponseWriter, r *http.Request) {
return
}

lifeTime := time.Hour * 24 * 7
sessionToken, created := api.createAPIKey(rw, r, database.InsertAPIKeyParams{
UserID: user.ID,
LoginType: database.LoginTypePassword,
// All api generated keys will last 1 week. Browser login tokens have
// a shorter life.
ExpiresAt: database.Now().Add(lifeTime),
LifetimeSeconds: int64(lifeTime.Seconds()),
Comment on lines +661 to +668
Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

/login is still 24hrs. Creating api keys from the api (eg/cli-auth) is 1 week. Refreshes every hour as normal, so any activity every 7 days keeps it alive.

})
if !created {
return
Expand DownExpand Up@@ -721,10 +726,21 @@ func (api *API) createAPIKey(rw http.ResponseWriter, r *http.Request, params dat
}
hashed := sha256.Sum256([]byte(keySecret))

// Default expires at to now+lifetime, or just 24hrs if not set
if params.ExpiresAt.IsZero() {
if params.LifetimeSeconds != 0 {
params.ExpiresAt = database.Now().Add(time.Duration(params.LifetimeSeconds) * time.Second)
} else {
params.ExpiresAt = database.Now().Add(24 * time.Hour)
}
}
Comment on lines +729 to +736
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Do you think we need an arbitrary amount of seconds for an API key to live for? I'm curious for@dwahler's thoughts.

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

We do if we want api keys to have different lifetimes. Currently the browser login api keys are valid for 24hrs. When we refresh them, we need to know how long to refresh them for (24hrs).

The cli keys will have a longer life (1 week). When you refresh, you need to know to add 1 week onto the expiresAt.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Not sure where it falls on the spectrum of "needs", but I can definitely imagine this being useful, especially to enterprise users.

If the point of expiring keys is to mitigate risk, then you might want to make that tradeoff differently when the risk level is different. For instance, shorter lifetimes for keys that are stored on laptops (potentially prone to theft) or for admin users (where the consequences of key compromise are greater).

For now, I like specifying the lifetime as a request parameter, because it seems like the simplest way to expose this functionality. In the future it might make more sense for admins to be able to control the lifetimes with some kind of policy.

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@dwahler It's not a configurable param from the api atm. There is just 2 endpoints (login and cli-auth) that make keys. But this backend does lead to it being easily configurable in the future.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Ah, gotcha, I missed thatparams wasn't directly populated from the request.

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@dwahler I didn't think it was necessary at this time, since it's 2 different routes.


_, err = api.Database.InsertAPIKey(r.Context(), database.InsertAPIKeyParams{
ID: keyID,
UserID: params.UserID,
ExpiresAt: database.Now().Add(24 * time.Hour),
ID: keyID,
UserID: params.UserID,
LifetimeSeconds: params.LifetimeSeconds,
// Make sure in UTC time for common time zone
ExpiresAt: params.ExpiresAt.UTC(),
CreatedAt: database.Now(),
UpdatedAt: database.Now(),
HashedSecret: hashed[:],
Expand Down
95 changes: 95 additions & 0 deletionscoderd/users_test.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -8,11 +8,13 @@ import (
"sort"
"strings"
"testing"
"time"

"github.com/google/uuid"
"github.com/stretchr/testify/require"

"github.com/coder/coder/coderd/coderdtest"
"github.com/coder/coder/coderd/database"
"github.com/coder/coder/coderd/database/databasefake"
"github.com/coder/coder/coderd/httpmw"
"github.com/coder/coder/coderd/rbac"
Expand DownExpand Up@@ -130,6 +132,99 @@ func TestPostLogin(t *testing.T) {
})
require.NoError(t, err)
})

t.Run("Lifetime&Expire", func(t *testing.T) {
t.Parallel()
var (
ctx = context.Background()
)
client, api := coderdtest.NewWithAPI(t, nil)
admin := coderdtest.CreateFirstUser(t, client)

split := strings.Split(client.SessionToken, "-")
loginKey, err := api.Database.GetAPIKeyByID(ctx, split[0])
require.NoError(t, err, "fetch login key")
require.Equal(t, int64(86400), loginKey.LifetimeSeconds, "default should be 86400")

// Generated tokens have a longer life
token, err := client.CreateAPIKey(ctx, admin.UserID.String())
require.NoError(t, err, "make new api key")
split = strings.Split(token.Key, "-")
apiKey, err := api.Database.GetAPIKeyByID(ctx, split[0])
require.NoError(t, err, "fetch api key")

require.True(t, apiKey.ExpiresAt.After(time.Now().Add(time.Hour*24*6)), "api key lasts more than 6 days")
require.True(t, apiKey.ExpiresAt.After(loginKey.ExpiresAt.Add(time.Hour)), "api key should be longer expires")
require.Greater(t, apiKey.LifetimeSeconds, loginKey.LifetimeSeconds, "api key should have longer lifetime")
})

t.Run("APIKeyExtend", func(t *testing.T) {
t.Parallel()
var (
ctx = context.Background()
)
client, api := coderdtest.NewWithAPI(t, nil)
admin := coderdtest.CreateFirstUser(t, client)

token, err := client.CreateAPIKey(ctx, admin.UserID.String())
require.NoError(t, err, "make new api key")
client.SessionToken = token.Key
split := strings.Split(token.Key, "-")

apiKey, err := api.Database.GetAPIKeyByID(ctx, split[0])
require.NoError(t, err, "fetch api key")

err = api.Database.UpdateAPIKeyByID(ctx, database.UpdateAPIKeyByIDParams{
ID: apiKey.ID,
LastUsed: apiKey.LastUsed,
// This should cause a refresh
ExpiresAt: apiKey.ExpiresAt.Add(time.Hour * -2),
OAuthAccessToken: apiKey.OAuthAccessToken,
OAuthRefreshToken: apiKey.OAuthRefreshToken,
OAuthExpiry: apiKey.OAuthExpiry,
})
require.NoError(t, err, "update api key")

_, err = client.User(ctx, codersdk.Me)
require.NoError(t, err, "fetch user")

apiKey, err = api.Database.GetAPIKeyByID(ctx, split[0])
require.NoError(t, err, "fetch refreshed api key")
// 1 minute tolerance
require.True(t, apiKey.ExpiresAt.After(time.Now().Add(time.Hour*24*7).Add(time.Minute*-1)), "api key lasts 7 days")
})

t.Run("LoginKeyExtend", func(t *testing.T) {
t.Parallel()
var (
ctx = context.Background()
)
client, api := coderdtest.NewWithAPI(t, nil)
_ = coderdtest.CreateFirstUser(t, client)
split := strings.Split(client.SessionToken, "-")

apiKey, err := api.Database.GetAPIKeyByID(ctx, split[0])
require.NoError(t, err, "fetch login key")

err = api.Database.UpdateAPIKeyByID(ctx, database.UpdateAPIKeyByIDParams{
ID: apiKey.ID,
LastUsed: apiKey.LastUsed,
// This should cause a refresh
ExpiresAt: apiKey.ExpiresAt.Add(time.Hour * -2),
OAuthAccessToken: apiKey.OAuthAccessToken,
OAuthRefreshToken: apiKey.OAuthRefreshToken,
OAuthExpiry: apiKey.OAuthExpiry,
})
require.NoError(t, err, "update login key")

_, err = client.User(ctx, codersdk.Me)
require.NoError(t, err, "fetch user")

apiKey, err = api.Database.GetAPIKeyByID(ctx, split[0])
require.NoError(t, err, "fetch refreshed login key")
// 1 minute tolerance
require.True(t, apiKey.ExpiresAt.After(time.Now().Add(time.Hour*24).Add(time.Minute*-1)), "login key lasts 24 hrs")
})
}

func TestPostLogout(t *testing.T) {
Expand Down
[8]ページ先頭
©2009-2025 Movatter.jp