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

Feature/innkeeper api key UI #765

Merged
merged 6 commits into from
Aug 22, 2023
Merged

Feature/innkeeper api key UI #765

merged 6 commits into from
Aug 22, 2023

Conversation

loneil
Copy link
Collaborator

@loneil loneil commented Aug 16, 2023

Create an API Key as an Innkeeper for tenants.

image

image

image

image

@loneil loneil temporarily deployed to development August 16, 2023 22:01 — with GitHub Actions Inactive
@github-actions
Copy link

Signed-off-by: Lucas ONeil <[email protected]>
Signed-off-by: Lucas ONeil <[email protected]>
Signed-off-by: Lucas ONeil <[email protected]>
Signed-off-by: Lucas ONeil <[email protected]>
@loneil loneil temporarily deployed to development August 16, 2023 22:12 — with GitHub Actions Inactive
Signed-off-by: Lucas ONeil <[email protected]>
@loneil loneil temporarily deployed to development August 16, 2023 23:56 — with GitHub Actions Inactive
@esune
Copy link
Member

esune commented Aug 18, 2023

@loneil could we have the api key displayed in the UI instead of treating it as a password that is displayed only once? I think it would make it easier for tenants to fetch the value and manage their keys (i.e.: knowing who is assigned which key, when more than one is available). Additionally, it is going to be easy to delete and recreate a key so it is not too concerning in my opinion to have it listed somewhere in the UI.

@loneil
Copy link
Collaborator Author

loneil commented Aug 18, 2023

@loneil could we have the api key displayed in the UI instead of treating it as a password that is displayed only once? I think it would make it easier for tenants to fetch the value and manage their keys (i.e.: knowing who is assigned which key, when more than one is available). Additionally, it is going to be easy to delete and recreate a key so it is not too concerning in my opinion to have it listed somewhere in the UI.

The thing is it's not persisted like that. Same as the reservation password it's stored as a salt/hash instead (#740).

Figured that was the good security practice for API keys but we could step it down to just storing plain text for retrieval if required.
Since it can be regenerated it and a client could have more than one at once it allows them to get a new one (and an LOB system is unlikely to lose a key since they'd have to be storing it somehow to be using it)

@esune
Copy link
Member

esune commented Aug 21, 2023

I am leaning towards plaintext for that, I think it would make life easier, but am open to leaving it protected if the security concern is high.

@cvarjao
Copy link
Member

cvarjao commented Aug 21, 2023

I would suggest we handle keys and passwords in a secure way and avoid clear text as much as possible.

@esune
Copy link
Member

esune commented Aug 21, 2023

@loneil we will definitely need tenant self-management of api-keys if we keep them encrypted - having the innkeeper be able to generate the first one (or in edge cases) will help, but we don't want to have the overhead of having to communicate back-and-forth with the tenant every time they need a key rotated/recreated.

The issue with having the api-keys not visible even to the tenant is that, in case one gets compromised, ALL of them will need to be deleted and re-created. Could we maybe use a (known) part of the wallet key to salt the api-keys, so that the tenant should be able to enter it to decode the encrypted values if they need to do so?

@loneil
Copy link
Collaborator Author

loneil commented Aug 21, 2023

Yes the self-serve tenant creation will be an important next step.

It's a salt/hash pair so it's a hash of the salt and password that's stored, not an encryption of the plaintext password.
There's lots of ways to rewrite this feature if it's not sufficient to store in that pattern if we want to pivot, but losing a key would either mean an LOB's secret storage is compromised for all of them (should regenerate all anyways then), or they don't know which key they lost (not storing it labeled with the alias or anything). I don't know if being able to go into Traction to see the key's values if they find one got compromised is that useful given those cases is it?

@esune
Copy link
Member

esune commented Aug 21, 2023

If we have a label that they might use to identify which record they are looking at might be good enough at least for now.

@loneil
Copy link
Collaborator Author

loneil commented Aug 21, 2023

If we have a label that they might use to identify which record they are looking at might be good enough at least for now.

It's an optional field but might be worth making mandatory to ensure it?

@esune
Copy link
Member

esune commented Aug 21, 2023

If we have a label that they might use to identify which record they are looking at might be good enough at least for now.

It's an optional field but might be worth making mandatory to ensure it?

yes, I think so

Signed-off-by: Lucas ONeil <[email protected]>
@loneil loneil temporarily deployed to development August 21, 2023 22:58 — with GitHub Actions Inactive
@loneil loneil mentioned this pull request Aug 21, 2023
@esune esune merged commit 396ec65 into main Aug 22, 2023
11 checks passed
@esune esune deleted the feature/innkeeperApiKeyUi branch August 22, 2023 16:42
@esune esune temporarily deployed to development August 22, 2023 16:42 — with GitHub Actions Inactive
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