Change Access Keys API + CLI #3654

pdevine · 2022-11-16T01:29:35Z

Summary

This change makes a number of quality of life changes to both the access keys API and CLI.

Change access keys to make their names unique to a specific user, instead of for an entire org
Make infra keys list default to the current user
Add an --all flag to infra keys list for admins which can list all access keys in the org
Make infra keys add not require a user name
Add a --user= argument to infra keys add to be consistent with infra keys list
Add a --connector flag to infra keys add to create the key for a connector
Make infra keys remove to be specific to the current user
Add a --user= argument similar to infra keys list and infra keys add
Changed the API to call DELETE /api/access-keys/:id instead of DELETE /api/access-keys
Added LastUsed field to the API and updated infra keys list to show the last time a specific key was used

Checklist

Wrote appropriate unit tests
Considered security implications of the change
Updated associated docs where necessary
Change is backwards compatible if it needs to be (user can upgrade without manual steps?)
Nothing sensitive logged
Considered data migrations for smooth upgrades

Note: This change should be backwards compatible, but there are a number of caveats. I didn't remove the DELETE /api/access-keys endpoint because older CLI clients will still need to use it, and it's not 100% clear how to accomplish this with the current API migrator. Also, if a client upgrades and hits an older version of the server, it will have issues displaying the LAST USED column because the API won't populate it.

BruceMacD

Looks good, will approve once the loose ends in the docs are resolved

docs/reference/cli.md

dnephin

Looks good! Some minor suggestions, but I think there are a few things here that we should fix before merge. I left some comments inline, but I noticed one more thing.

access.DeleteAccessKeys does a get query with only a name, which is no longer valid with these changes. The query could return multiple results. I think that needs to be changed to use ListAccessKeys. I think data.GetAccessKey needs to change to require both an IssusedFor and a ByName (or just an ID) because otherwise it's a broken query.

If we're no longer using the DELETE /api/access-keys (no id in the path) endpoint, we should create a github issue to look at removing it in the future (unless of course we find another need for it over the next month or so).

Test coverage of this change is pretty light, and what we do have uses a fake handler. It would be great to get some test coverage of these CLI commands with the new behaviour using a real API server as a follow up to this PR.

api/access_key.go

internal/cmd/keys.go

dnephin · 2022-11-16T15:57:47Z

internal/cmd/keys.go

+$ infra keys add --connector
+
+# Set an environment variable with the newly created access key
+$ MY_ACCESS_KEY=$(infra keys add -q --name my-key)


This is cool!

internal/cmd/keys.go

internal/cmd/keys_test.go

dnephin · 2022-11-16T16:09:44Z

internal/cmd/testdata/TestKeysListCmd/list_all

-  side-door   admin       24 hours ago  6 hours from now  26 hours from now   
-  storage     clerk       20 hours ago  6 hours from now  never               
+  NAME        ISSUED FOR  CREATED       LAST USED  EXPIRES           EXTENSION DEADLINE  
+  front-door  admin       24 hours ago  never      never             never               


Maybe as a follow up, once we've converted the test to use a real API server, but I guess we should add some data that has a last used value, so that we see it formatted in this expected output.

I'll make an issue for this. I also want to potentially change the ISSUED FOR column to not appear unless you're using the --all flag, or the --user= argument.

dnephin · 2022-11-16T16:16:18Z

internal/server/models/access_key.go

@@ -46,6 +46,7 @@ func (ak *AccessKey) ToAPI() *api.AccessKey {
 		ID:                ak.ID,
 		Name:              ak.Name,
 		Created:           api.Time(ak.CreatedAt),
+		LastUsed:          api.Time(ak.UpdatedAt),


On first read this seemed wrong, but I guess it works in some cases because of the extension deadline. If the key has an extension deadline then we'll update it on use, so UpdatedAt will be updated.

However I believe there are 2 cases where that may not work:

if the key doesn't have an extension deadline then we don't update it, which means LastUsed will be wrong (it'll show the created time always)

if we get to the period of time where the extension deadline has reached the expiration, so it can't be extended anymore. This could potentially be a long period of time if the extension deadline is large. I think today that may not be an issue because I'm not sure we're handling this correctly, but once we fix that bug it'll be a problem here.

I think we may need a new database row for LastUsed to accurately show the used time.

If we want to leave this fix to a follow up, maybe add a comment here, or on the api.AccessKey.LastUsed field to indicate that it's not accurate yet and why.

This is interesting, however I don't think we currently would hit either of those cases.

Right now we require the user to include the extensionDeadline if you're creating the key through the API, otherwise you'll get a 400. This could be abused by us if someone creates an access key outside of the API, so it feels a little bit brittle.

Once the expiry is hit, the key won't work anymore. The extensionDeadline is ignored, so UpdatedAt will never get updated, but I think this is the correct value (i.e. the last time the access key was successfully used). I can see an argument for changing it if we want to record attempted use instead of actual use.

For now, I'll leave some comments in the code.

There are ways to create an access key without an extension deadline I think.

Signup and Server.loadConfig both create an access key without an extension deadline, it can't be extended.

For 2, I don't think I explained it well. The issue is not that the key is expired, it's that the key can't be extended anymore. For example:

Extension deadline set to Dec 1st 12:00, with an extension value of 30 days

After Nov 1st, we shouldn't be making the expiry later than Dec 1st 12:00, because then the expiry would be past the extension deadline.

Today that's what we do, which means we are forced to look at two different values to determine if the key is valid or not. See #3118

During that period of time (nov 1st to dec 1st) the key won't be extended anymore, so the lastSeenAt will be wrong. In this example it's only 30 days, but it could be much longer if the extension value is longer.

internal/server/data/migrations_test.go

dnephin · 2022-11-16T16:30:00Z

I suspect we can fix the API migration if we use #2006, but I'd have to look at the query parameter some more to get a better idea of exactly what we need.

pdevine · 2022-11-17T02:03:04Z

Thanks for the review @dnephin ! I think I've addressed each of the comments.

Ideally we will deprecate DELETE /api/access-keys in favour of DELETE /api/access-keys/:id, however we currently are calling that endpoint through the CLI. For now I've changed the DELETE /api/access-keys endpoint to only allow deleting an access key which belongs to the current user. If an admin wants to delete a key for a different user through the API, they will have to use the other endpoint. Through the CLI, they can delete by key name, but will need to specify a user name.

mxyng · 2022-11-17T02:16:04Z

internal/cmd/keys.go

+			if options.Quiet {
+				cli.Output(resp.AccessKey)
+				return nil
+			}


I'm not convinced this needs a quiet option right now. The output is simple enough that any non essential outputs, like the pretext, should be printed to stderr while the essential outputs, in this case resp.AccessKey should be printed to stdout. The examples will remain the same since only stdout will be captured

dnephin

Thanks for making those changes!

I think unfortunately access key extension deadline needs some cleanup, which makes it hard to trust UpdatedAt for the LastSeen. We should at least track that in an issue.

I pushed a fix for data.GetAccessKey here: b7f11ed. I can push it to this branch if you are ok with that fix (or you are welcome to cherry-pick it).

I think with that fix, and the two github issues for adding test coverage and fixing LastSeen, this is in a good place to merge.

internal/access/access_key.go

Co-authored-by: Daniel Nephin <dnephin@infrahq.com>

pdevine requested review from technovangelist, dnephin, jmorganca, mchiang0610 and BruceMacD as code owners November 16, 2022 01:29

vercel bot temporarily deployed to Preview November 16, 2022 01:38 Inactive

BruceMacD reviewed Nov 16, 2022

View reviewed changes

docs/reference/cli.md Show resolved Hide resolved

dnephin reviewed Nov 16, 2022

View reviewed changes

pdevine requested a review from mxyng as a code owner November 17, 2022 01:54

vercel bot temporarily deployed to Preview November 17, 2022 01:55 Inactive

mxyng reviewed Nov 17, 2022

View reviewed changes

dnephin approved these changes Nov 17, 2022

View reviewed changes

internal/access/access_key.go Outdated Show resolved Hide resolved

pdevine force-pushed the access-keys-index branch from be27535 to d341f02 Compare November 17, 2022 23:20

vercel bot temporarily deployed to Preview November 18, 2022 00:05 Inactive

pdevine and others added 12 commits November 17, 2022 16:25

maintain: allow the same name to be used by access keys

ea770b0

maintain: improve infra keys add

b4697a7

maintain: improve infra keys list with default user

cb8b320

maintain: update infra keys rm to default to current user

05fd1fc

maintain: add lastused to access key api and cli

68a5ded

maintain: fix infra keys tests

3c000a1

maintain: update cli reference doc

dd71458

maintain: address comments

52ae1e5

maintain: address more comments

8d8340c

Make data.GetAccessKey safe again

949f20c

maintain: fix config test

cb33b17

maintain: update cli docs

85ee9fb

pdevine force-pushed the access-keys-index branch from 1449fc7 to 85ee9fb Compare November 18, 2022 00:26

vercel bot temporarily deployed to Preview November 18, 2022 00:26 Inactive

pdevine merged commit f321f96 into main Nov 18, 2022

pdevine deleted the access-keys-index branch November 18, 2022 00:42

This was referenced Nov 18, 2022

Simplify access key query logic related to expiry time #3118

Closed

CLI: scope commands to the user requesting them by default #3298

Closed

technovangelist pushed a commit that referenced this pull request Nov 18, 2022

Change Access Keys API + CLI (#3654)

354f055

Co-authored-by: Daniel Nephin <dnephin@infrahq.com>

dnephin mentioned this pull request Dec 13, 2022

API request authorization #3866

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change Access Keys API + CLI #3654

Change Access Keys API + CLI #3654

pdevine commented Nov 16, 2022

BruceMacD left a comment

dnephin left a comment •

edited

Loading

dnephin Nov 16, 2022

dnephin Nov 16, 2022

pdevine Nov 17, 2022

dnephin Nov 16, 2022 •

edited

Loading

pdevine Nov 17, 2022

dnephin Nov 17, 2022

dnephin commented Nov 16, 2022

pdevine commented Nov 17, 2022

mxyng Nov 17, 2022

dnephin left a comment

Change Access Keys API + CLI #3654

Change Access Keys API + CLI #3654

Conversation

pdevine commented Nov 16, 2022

Summary

Checklist

BruceMacD left a comment

Choose a reason for hiding this comment

dnephin left a comment • edited Loading

Choose a reason for hiding this comment

dnephin Nov 16, 2022

Choose a reason for hiding this comment

dnephin Nov 16, 2022

Choose a reason for hiding this comment

pdevine Nov 17, 2022

Choose a reason for hiding this comment

dnephin Nov 16, 2022 • edited Loading

Choose a reason for hiding this comment

pdevine Nov 17, 2022

Choose a reason for hiding this comment

dnephin Nov 17, 2022

Choose a reason for hiding this comment

dnephin commented Nov 16, 2022

pdevine commented Nov 17, 2022

mxyng Nov 17, 2022

Choose a reason for hiding this comment

dnephin left a comment

Choose a reason for hiding this comment

dnephin left a comment •

edited

Loading

dnephin Nov 16, 2022 •

edited

Loading