Add a `--yes` flag + checks to `garage key import`

kinovic commented

2022-03-30 16:54:21 +00:00

We are thinking about deploying garage as our main S3 storage, we like idea about it a lot. we have found nice feature, which in most cases is good, but is a bit painfull in another.

Matching resources as startsWith is usually great, fe setting up cluster, where I can reference it by two characters. But when working with keys it comes with some problems.

root@garage-1-a:~# garage -c /etc/garage/spin.toml key list
List of keys:
  backup             Imported key
  deb-prod-repo      Imported key
  deb-prod-repo-rw   Imported key
  deb-stage-repo     Imported key
  deb-stage-repo-rw  Imported key
  dupla              Imported key
root@garage-1-a:~# garage -c /etc/garage/spin.toml key info deb-prod-repo
Error: 2 matching keys

it would be nice if we can match the key ids exact, would make our lifes easier, something like
garage -c /etc/garage/spin.toml key info deb-prod-repo --exact-id-match

anyway guys, great work.

We are thinking about deploying garage as our main S3 storage, we like idea about it a lot. we have found nice feature, which in most cases is good, but is a bit painfull in another. Matching resources as `startsWith` is usually great, fe setting up cluster, where I can reference it by two characters. But when working with keys it comes with some problems. ``` root@garage-1-a:~# garage -c /etc/garage/spin.toml key list List of keys: backup Imported key deb-prod-repo Imported key deb-prod-repo-rw Imported key deb-stage-repo Imported key deb-stage-repo-rw Imported key dupla Imported key root@garage-1-a:~# garage -c /etc/garage/spin.toml key info deb-prod-repo Error: 2 matching keys ``` it would be nice if we can match the key ids `exact`, would make our lifes easier, something like `garage -c /etc/garage/spin.toml key info deb-prod-repo --exact-id-match` anyway guys, great work.

kinovic commented

2022-03-30 18:34:49 +00:00

Author

when using name instead keyId it works as expected, so we might stay with name now.

root@garage-1-a:~# garage -c /etc/garage/spin.toml key list
List of keys:
  GK2ac2b18ef4bcf0cff87d5ee0  backup
  GK4f42d16d563213de1fa3ad9e  deb-stage-repo
  GK7189cff9b9a7ea7e268bbc96  deb-prod-repo-rw
  GKa00ec29b46b28a5218caac25  deb-prod-repo
  GKa67f7b11a714be63ae336e95  dupla
  GKfe8139f7030d7352e061dcee  deb-stage-repo-rw
root@garage-1-a:~# garage -c /etc/garage/spin.toml key info deb-prod-repo
Key name: deb-prod-repo
Key ID: GKa00ec29b46b28a5218caac25
Secret key: 53c7b5ff375669cc640870de4f83f1adc4a1e61e549549d612fbc73176547057
Can create buckets: false

Key-specific bucket aliases:

Authorized buckets:

when using name instead keyId it works as expected, so we might stay with name now. ``` root@garage-1-a:~# garage -c /etc/garage/spin.toml key list List of keys: GK2ac2b18ef4bcf0cff87d5ee0 backup GK4f42d16d563213de1fa3ad9e deb-stage-repo GK7189cff9b9a7ea7e268bbc96 deb-prod-repo-rw GKa00ec29b46b28a5218caac25 deb-prod-repo GKa67f7b11a714be63ae336e95 dupla GKfe8139f7030d7352e061dcee deb-stage-repo-rw root@garage-1-a:~# garage -c /etc/garage/spin.toml key info deb-prod-repo Key name: deb-prod-repo Key ID: GKa00ec29b46b28a5218caac25 Secret key: 53c7b5ff375669cc640870de4f83f1adc4a1e61e549549d612fbc73176547057 Can create buckets: false Key-specific bucket aliases: Authorized buckets: ```

quentin commented

2022-03-31 09:00:40 +00:00

Owner

Thanks for your feedback, it is valuable to us!

I will rephrase your issue within our context and our assumptions, it will help you understand the current behavior of the CLI.

Early in Garage's development, we chose arbitrarily that access keys would be generated strings starting with GK followed by 12 random bytes encoded in hexadecimal. This is a choice similar to AWS but different from Minio that allow users to set the access key to whatever value they want.

At this moment, we were also making some heavy refactors to the data model, one of them involved manually reimporting the keys, hence we introduced the import subcommand to reinject keys previously generated by Garage, but we did not expect that people would use it to create keys with a custom identifier.

In the context of Garage, there is an advantage to use generated keys instead of user defined ones. As you may know, Garage tries to reduce synchronization in the cluster to avoid amplifying the existing latency between nodes. If 2 operators are creating a key with the same identifier from 2 different nodes more or less at the same time, the action will succeed from both sides, but later both identifiers will be merged and only one secret key will remain. However, if you rely on our randomly generated keys, and put names (aliases) on them, we overcome this problem: 2 different keys can have the same name on the cluster as long as their identifier is different.

As a conclusion, we never expected to match a key identifier that is not exactly 26 characters long, which is why we did not consider the case where one identifier is a substring of another one. In the end, we must know if we want to modify garage to officially supports custom key identifiers, and thus adapt our subcommands additionnaly to our matching logic, or we should disable it. Your issue raises multiple questions:

Should we allow users to set their own key identifier despite the risks?
Should we remove the import subcommand? Or hide it somewhere else?
Should we add a warning? What should we write?
Should we check in the import subcommand that key identifiers and secret keys follow a specific pattern (exactly 26 characters long, starting with GK, etc.)

I would recommend at least implementing the last point, checking the format of the identifier and secret. We might also discuss adding an hidden --yes or --force flag displaying a message that this command is reserved for migrations and should not be used to add custom values.

@kinovic: your second approach, by setting a --name, is the "correct" one. You can use these aliases on all other commands, for example garage bucket allow --read --key dupla my-bucket. Please not that you also "leaked" the secret key of deb-prod-repo. If you intend to use this cluster for production use, you should rotate this key.

Thanks for your feedback, it is valuable to us! I will rephrase your issue within our context and our assumptions, it will help you understand the current behavior of the CLI. Early in Garage's development, we chose arbitrarily that access keys would be generated strings starting with `GK` followed by 12 random bytes encoded in hexadecimal. This is a choice similar to AWS but different from Minio that allow users to set the access key to whatever value they want. At this moment, we were also making some heavy refactors to the data model, one of them involved manually reimporting the keys, hence we introduced the `import` subcommand to reinject keys previously generated by Garage, but we did not expect that people would use it to create keys with a custom identifier. In the context of Garage, there is an advantage to use generated keys instead of user defined ones. As you may know, Garage tries to reduce synchronization in the cluster to avoid amplifying the existing latency between nodes. If 2 operators are creating a key with the same identifier from 2 different nodes more or less at the same time, the action will succeed from both sides, but later both identifiers will be merged and only one secret key will remain. However, if you rely on our randomly generated keys, and put names (aliases) on them, we overcome this problem: 2 different keys can have the same name on the cluster as long as their identifier is different. As a conclusion, we never expected to match a key identifier that is not exactly 26 characters long, which is why we did not consider the case where one identifier is a substring of another one. In the end, we must know if we want to modify garage to officially supports custom key identifiers, and thus adapt our subcommands additionnaly to our matching logic, or we should disable it. Your issue raises multiple questions: - Should we allow users to set their own key identifier despite the risks? - Should we remove the `import` subcommand? Or hide it somewhere else? - Should we add a warning? What should we write? - Should we check in the `import` subcommand that key identifiers and secret keys follow a specific pattern (exactly 26 characters long, starting with `GK`, etc.) I would recommend at least implementing the last point, checking the format of the identifier and secret. We might also discuss adding an hidden `--yes` or `--force` flag displaying a message that this command is reserved for migrations and should not be used to add custom values. @kinovic: your second approach, by setting a `--name`, is the "correct" one. You can use these aliases on all other commands, for example `garage bucket allow --read --key dupla my-bucket`. Please not that you also "leaked" the secret key of `deb-prod-repo`. If you intend to use this cluster for production use, you should rotate this key.

quentin changed title from ~~Improve way how to detect/select items~~ to `garage key import` is used to create custom key identifiers and lead to weird behaviors

2022-03-31 09:03:27 +00:00

quentin added the

Bug

label 2022-03-31 09:03:58 +00:00

quentin added the

Documentation

label 2022-03-31 09:12:57 +00:00

kinovic commented

2022-03-31 17:04:02 +00:00

Author

thx for pointing out of leaked key, but this is pure development/testing (probably already wiped multiple of times).

for us/me import feature is nice and great in what we are doing, so having it would save us if we loose the cluster and need to rebuild, since read keys would be distributed on multiple places already.

in our flow, what we are doing right now we are OK only with new keys (randomly generated), because it is done only once and secrets are exported (future into some vault), they get distributed into our ci, developer machines etc. What would be a bit painfull is, if we loose cluster. In that case import would help us a lot, because we do not have to redistribute all the keys again.

IMHO:

Should we allow users to set their own key identifier despite the risks?
yes, just document it. Like: it was designed to use GK(24chars) exactly and if you use something else there are some limitation. You cannot have key as substring of another otherwise it will lead to unexpected scenarios
Should we remove the import subcommand? Or hide it somewhere else?
please leave it somewhere
Should we add a warning? What should we write?
dude you are doing unsuported thing... It might and will lead into unexpected behavior
Should we check in the import subcommand that key identifiers and secret keys follow a specific pattern (exactly 26 characters long, starting with GK, etc.)
no, see above, just note that you are using this feature at own risk and might and will lead into unexpected behavior, even when you enforce some logic, you will hit problem with repeating keys, as I understand you cannot use deleted key anyway

thx for pointing out of leaked key, but this is pure development/testing (probably already wiped multiple of times). for us/me import feature is nice and great in what we are doing, so having it would save us if we loose the cluster and need to rebuild, since read keys would be distributed on multiple places already. in our flow, what we are doing right now we are OK only with new keys (randomly generated), because it is done only once and secrets are exported (future into some vault), they get distributed into our ci, developer machines etc. What would be a bit painfull is, if we loose cluster. In that case import would help us a lot, because we do not have to redistribute all the keys again. IMHO: * Should we allow users to set their own key identifier despite the risks? *yes, just document it. Like: it was designed to use GK(24chars) exactly and if you use something else there are some limitation. You cannot have key as substring of another otherwise it will lead to unexpected scenarios* * Should we remove the import subcommand? Or hide it somewhere else? *please leave it somewhere* * Should we add a warning? What should we write? *dude you are doing unsuported thing... It might and will lead into unexpected behavior* * Should we check in the import subcommand that key identifiers and secret keys follow a specific pattern (exactly 26 characters long, starting with GK, etc.) *no, see above, just note that you are using this feature at own risk and might and will lead into unexpected behavior, even when you enforce some logic, you will hit problem with repeating keys, as I understand you cannot use deleted key anyway*

quentin commented

2022-04-05 10:33:17 +00:00

Owner

After discussing with the other maintainers, we think the following changes would be the best tradeoff between guiding our users to the best practises while giving them some margin:

Adding a --yes flag to garage key import (here) similarly to the one on garage migrate (here)
Aborting the command (here) if the --yes flag is not provided, similarly to garage migrate command (here). Saying something in the error message like This command is intended to re-import keys that were previously generated by Garage. If you want to create a new key, use "garage key new" instead. Add the --yes flag if you really want to re-import a key.
Checking that the key is exactly 26 characters long (it will prevent beginners from shooting themself in the foot while allowing expert users to break the system as they want ^^)

Let us know if you want to write a PR for this issue, otherwise we will put it in our backlog and write a patch in the coming weeks :-)

After discussing with the other maintainers, we think the following changes would be the best tradeoff between guiding our users to the best practises while giving them some margin: - Adding a `--yes` flag to `garage key import` ([here](https://git.deuxfleurs.fr/Deuxfleurs/garage/src/branch/main/src/garage/cli/structs.rs#L342-L352)) similarly to the one on `garage migrate` ([here](https://git.deuxfleurs.fr/Deuxfleurs/garage/src/branch/main/src/garage/cli/structs.rs#L354-L362)) - Aborting the command ([here](https://git.deuxfleurs.fr/Deuxfleurs/garage/src/branch/main/src/garage/admin.rs#L555-L583)) if the `--yes` flag is not provided, similarly to `garage migrate` command ([here](https://git.deuxfleurs.fr/Deuxfleurs/garage/src/branch/main/src/garage/admin.rs#L585-L599)). Saying something in the error message like *This command is intended to re-import keys that were previously generated by Garage. If you want to create a new key, use "garage key new" instead. Add the --yes flag if you really want to re-import a key.* - Checking that the key is exactly 26 characters long (it will prevent beginners from shooting themself in the foot while allowing expert users to break the system as they want ^^) Let us know if you want to write a PR for this issue, otherwise we will put it in our backlog and write a patch in the coming weeks :-)

👍 1

kinovic commented

2022-04-11 07:38:25 +00:00

Author

unfortunatelly I do not know about Rust a thing... So I cannot help :/ for me it would be totally new language to leard and right now I do have different priorities.

As I understand how it works now, I'm happy with current solution.

thx

unfortunatelly I do not know about Rust a thing... So I cannot help :/ for me it would be totally new language to leard and right now I do have different priorities. As I understand how it works now, I'm happy with current solution. thx

👍 1

quentin changed title from ~~`garage key import` is used to create custom key identifiers and lead to weird behaviors~~ to Add a `--yes` flag + checks to `garage key import`

2022-04-11 07:56:39 +00:00

quentin added

Newcomer

and removed

Documentation

labels 2022-04-11 09:22:10 +00:00