Skip to content
This repository has been archived by the owner on Mar 21, 2024. It is now read-only.

Introduce the --generate-master-key cli-flag #210

Closed
wants to merge 1 commit into from

Conversation

irevoire
Copy link
Member

@irevoire irevoire commented Jan 2, 2023

Associated issue; meilisearch/meilisearch#3287
Associated PR; #209


Summary

Introduce the --generate-master-key cli-flag.
It generates a cryptographically safe to use master-key.

Changes

Introduce the --generate-master-key cli-flag

Attention To Reviewers

Should we really introduce an env-var + allow this parameter to be specified with the config file?

@irevoire irevoire changed the base branch from main to release-v1.0.0 January 2, 2023 15:32
@irevoire irevoire requested a review from gmourier January 2, 2023 15:32
Copy link
Member

@gmourier gmourier left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you @irevoire.

Edit: I approved to fast 😇

@gmourier gmourier self-requested a review January 2, 2023 16:11
Copy link
Member

@gmourier gmourier left a comment

Choose a reason for hiding this comment

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

Could we indicate the message being shown when generate-master-key is used?

About the Attention To Reviewers questions, it doesn't shock me to keep something consistent (i.e. it can be used as env var but also in the configuration file).

The only concern I have is that a user may be stuck without knowing how to disable the generation of a master key. One idea could be to indicate in the message that the master-key is given without launching the instance because generate-master-key is used. WDYT?

@irevoire
Copy link
Member Author

irevoire commented Jan 2, 2023

Currently, only the master-key is outputted with no text at all. This allows you to run something like meilisearch --generate-master-key | xargs meilisearch --db-path truc --master-key.
Or more generally, export MEILI_MASTER_KEY=$(meilisearch --generate-master-key).
I don't know if it's really important.

One idea could be to indicate in the message that the master-key is given without launching the instance because generate-master-key is used.

It's ok-ish to me.

Another idea could be to make the --generate-master-key incompatible with settings your master-key so people would get a better error message once they start to run their meilisearch 🤔


The last solution I can think of is to output the master-key in the logs and thus when you use the --generate-master-key it starts meilisearch.
But I think there is a security concern with this one, people would not be able to share their logs anymore and even storing them would get complicated 😩

@gmourier
Copy link
Member

gmourier commented Jan 2, 2023

But I think there is a security concern with this one, people would not be able to share their logs anymore and even storing them would get complicated 😩

💯

I've been thinking about it again, and IMO it's perfectly ok to go as that for v1.0

  1. This flag is a bonus. The user flow already indicates pre-generated master keys when possible. See the different screenshots (Regarding that we could always output a generated master key plus give the helper to generate another one by hand to be more consistent)
  2. We'll see if keeping it as such (just outputting a master key) is a problem depending on user feedback we may collect.

@dureuill
Copy link
Contributor

dureuill commented Jan 5, 2023

I'd like to call your attention on the fact that there are currently discussions (internal link) about removing this flag.

bors bot added a commit to meilisearch/meilisearch that referenced this pull request Jan 5, 2023
3308: Remove `--generate-master-key` option r=Kerollmops a=dureuill

# Pull Request

## Related issue

Related to meilisearch/specifications#210 (comment)

## What does this PR do?
- Remove the short-lived `--generate-master-key` flag that was too beautiful for this world :D.

Removal of this option proceeds of the following reasoning:

1. It is the only option that starts meilisearch and then immediately exits
2. We are unsure if we want to keep it under this form in the future or switch to a subcommand.
3. Releasing this option in v1 would make it insta-stable.
5. The option is only marginally useful, as users will be presented with freshly generated key directly in the error messages if their master key is absent/too short.
6. If we remove this option now, we can still add it back in a future v1 release. If we add it now, we won't be able to remove it in any future v1 version.

## PR checklist
Please check if your PR fulfills the following requirements:
- [ ] Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
- [ ] Have you read the contributing guidelines?
- [ ] Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!

### Impacts

this impacts the docs team as they would previously have had to document this option, and they may have wanted to use it in the user workflow.

Co-authored-by: Louis Dureuil <louis@meilisearch.com>
@gmourier
Copy link
Member

gmourier commented Jan 5, 2023

Closing following meilisearch/meilisearch#3308

Removal of this option proceeds of the following reasoning:

  1. It is the only option that starts meilisearch and then immediately exits
  2. We are unsure if we want to keep it under this form in the future or switch to a subcommand.
  3. Releasing this option in v1 would make it insta-stable.
  4. The option is only marginally useful, as users will be presented with freshly generated key directly in the error messages if their master key is absent/too short.
  5. If we remove this option now, we can still add it back in a future v1 release. If we add it now, we won't be able to remove it in any future v1 version.

@gmourier gmourier closed this Jan 5, 2023
@curquiza curquiza deleted the irevoire-patch-1 branch January 3, 2024 16:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants