Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: v0.12 #102

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

feat: v0.12 #102

wants to merge 1 commit into from

Conversation

alexluong
Copy link
Collaborator

@alexluong alexluong commented Sep 3, 2024

This is a PR for the base branch of some new changes involving config updates. Currently, there's nothing to be reviewed here. I'll open a few PRs against this branch so we can keep things moving without merging to main.

Requirements from @alexbouchardd:

Scope:

  • Remove --api-key flag
  • Change hookdeck ci to take the project api key as param so hookdeck ci some-key (we can keep --api-key on this command just for backward compatibility, not document it and print a warning)
  • Change hookdeck login to accept an optional cli key as param so hookdeck login some-key
  • Add a warning when using --cli-key thats it’s deprecated and login with hookdeck login some-key and remove it from the documentation
  • Change config terminology from api_key to cli_key and workspace_ to project_
  • Fix the issue with hookdeck login --cli-key abcxyz if already logged in. It should replace your current credentials for the current profile

Other tasks related to project management that may or may not be included in this PR:

CLI

  • Support a --project_id flag
  • Allow non-interactive use of hookdeck project use by allowing passing a project id hookdeck project use tm_123

Dashboard (not related to this PR)

  • Expose the project_id in the project settings
  • Add CLI key management UI

@leggetter
Copy link
Collaborator

leggetter commented Sep 3, 2024

Prefer flags over optional args

This means login should take a cli-key flag and not an optional arg. ci should take an api-key (see next point). We don't presently have a CLI style guide, but there's the reasoning: https://devcenter.heroku.com/articles/cli-style-guide#flags

project use <project_id> may be different because the use terminology implies another parameter is effectively required, as does the following interactive functionality currently in place.

Consistent flag casing

project_id should be project-id as a flag. project_id is probably accurate for the config file.

hookdeck ci still takes an API Key

AFAICT hookdeck ci still takes an API Key, not a CI Key. It then uses the API Key to generate a CI Key. So, we should keep the api-key flag and terminology because it is accurate.

Discussion: CI Key management in the CLI?

hookdeck ci allows for the creation of a CLI key. Why not more directly support the creation (and eventual deletion) of CI CLI Keys from the CLI using an API Key.

This also removes the dependency on the UI change but may require an API endpoint that supports CLI Key deletion (which would be required by the UI change anyway).

@alexluong
Copy link
Collaborator Author

alexluong commented Sep 3, 2024

Thanks for writing these out, super clear and we're on the same page originally but you make things very clear here!

hookdeck ci allows for the creation of a CLI key. Why not more directly support the creation (and eventual deletion) of CI Keys from the CLI using an API Key.

This is a very interesting idea. I do want to clarify that we may want to manage not just "ci key" but general "cli key" as there's a slight difference between the 2. But I do think it may make a lot of sense that we support this functionality from the CLI.

Is this a sufficient solution for the security audit, @alexbouchardd?

@leggetter
Copy link
Collaborator

Thanks for writing these out, super clear and we're on the same page originally but you make things very clear here!

hookdeck ci allows for the creation of a CLI key. Why not more directly support the creation (and eventual deletion) of CI Keys from the CLI using an API Key.

This is a very interesting idea. I do want to clarify that we may want to manage not just "ci key" but general "cli key" as there's a slight difference between the 2. But I do think it may make a lot of sense that we support this functionality from the CLI.

Is this a sufficient solution for the security audit, @alexbouchardd?

Ah, sorry, I do mean "CLI Keys".

@alexbouchardd
Copy link
Contributor

alexbouchardd commented Sep 3, 2024

Given that you'd be authenticated with specific CLI keys, doesn't it seem odd that you could delete other keys with it?

There are two distinct considerations here:

hookdeck ci generated keys are not bound to any user but rather a project. From a user perspective, they are not aware that any CLI keys exist, and that leads me to think we shouldn't be generating CLI keys at all and use the actual API key as credentials

hookdeck login assigns a CLI key, which shouldn't grant you access to delete other CLI keys. That seems better suited to the UI where you can generate a CLI client (which gives a key) and delete clients (which invalidates the key)

@alexluong
Copy link
Collaborator Author

alexluong commented Sep 3, 2024

I think of it more like $ hookdeck login is an authentication mechanism. CLI key is just implementation details. After a user has logged in, their requests using the CLI key are operations made by the user. And as a user, they should be able to revoke other sessions (delete other CLI keys), and therefore I don't see that as a problem.

It's the same when folks use session tokens or something like that for the web, right? Once logged in, using one session token, a user can delete other session tokens.


hookdeck ci generated keys are not bound to any user but rather a project. From a user perspective, they are not aware that any CLI keys exist, and that leads me to think we shouldn't be generating CLI keys at all and use the actual API key as credentials

For this point, I remembered the reason we introduced the concept of "CI CLI key" (different from CLI key) because the API key and CLI key have different authorization logic. Some operations were doable with CLI key and not API key. So, CI CLI key is basically a CLI key but with a slightly different authentication context (project scope instead of user scope).

@leggetter
Copy link
Collaborator

leggetter commented Sep 3, 2024

CLI Key management would need to be authenticated with an API Key. This is the same way that hookdeck ci has to take an API Key in order to generate a CLI key. Or, that's my understanding of how it works. So, hookdeck ci is effectively generating a CLI Key and doing a hookdeck cli-key create (made-up command for effect) already.

Addition:

hookdeck login also generates a CLI Key. However, it knows the user so that CLI Key can be bound to a user.

hookdeck ci --api-key {key} can't have a user associated since there is no user context (the user isn't logged in via the browser)

@alexluong
Copy link
Collaborator Author

alexluong commented Sep 3, 2024

CLI Key management would need to be authenticated with an API Key.

I don't think this is the right model because we're mixing scope here.

API key: project scope, it only knows about 1 project and manages resources of that project
CLI key: user scope, it has access to everything the user has access to, so if a user is in multiple organizations with multiple projects, it can manage all of those

Why would API key be able to manage CLI key?

Regarding hookdeck ci:

API key: project scope
CI key: also project scope, so only manage resources from that project. It can be used to make a few extra calls that API keys can't. Those endpoints are related to CLI like creating CLI sessions, for example.

But because the auth context is the same, API key can manage the CI key.

@leggetter
Copy link
Collaborator

Ah! CI Key vs. CLI Key 🤯

Are we sure the platform differentiates between the two? Given the scope you speak of, I'd hope it does to some extent.

But maybe both hookdeck ci --api-key {key} and hookdeck login generate the same type of key (or token?), but those tokens are scoped? ci scopes the token to the project and login scopes to the user?

So, does Hookdeck actually have the concept of all three of:

  1. CI Key
  2. CLI Key
  3. API Keys

?

How does it then change what we should call the property in the config file because calling it cli_key is inaccurate if we do differentiate between the types of keys?

@alexluong
Copy link
Collaborator Author

Yes, we have 3 types of tokens. They're mapped to 3 different tables in the DB. I worked on the PR that introduced the CI token, and I was mindful about differentiating it and making sure it has the right scope. Obviously I don't know if there has been any changes since.

I don't think we persist the API key in the CLI. But yes, currently in the CLI we always call the keys "API key" and store them as such in the config. Definitely something to improve for clarity sake, which is part of the scope laid out above.

@leggetter
Copy link
Collaborator

@alexluong - do we need to re-write some of the issue description to reflect the above discussions?

Scope:

  • Remove --api-key flag

I think we're good with the above.

  • Change hookdeck ci to take the project api key as param so hookdeck ci some-key (we can keep --api-key on this command just for backward compatibility, not document it and print a warning)

Despite my comment about preferring flags over args, I think this is a required arg so makes sense.

  • Change hookdeck login to accept an optional cli key as param so hookdeck login some-key
    Add a warning when using --cli-key thats it’s deprecated and login with hookdeck login some-key and remove it from the documentation

If we add this, it should be a flag.

However, I'm actually unsure how this works and where it's used. Does it set the cli_key in the config (see below) and then perform a login with it to get the CI key?

  • Change config terminology from api_key to cli_key and workspace_ to project_

All good.

  • Fix the issue with hookdeck login --cli-key abcxyz if already logged in. It should replace your current credentials for the current profile

I don't know of this problem, but makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants