Reject master keys shorter than 16 bytes in `production` #209

dureuill · 2022-12-26T16:18:06Z

No API change

Summary

Update specs after the changes of meilisearch/meilisearch#3272.

Related to meilisearch/meilisearch#3274.

Changes

Specify that too short a Master Key is a cause for error

Out Of Scope

Warnings emitted in dev mode are considered out of scope as far as the spec is regarded. This is consistent with other specifications that generally leave warnings up to the implementation.

Attention To Reviewers

N/A

Misc

[] Update OpenAPI specification file (if needed; Apply the OpenApi label)
[] Update telemetry datapoints (if needed; Apply the Telemetry label)

text/0085-api-keys.md

gmourier · 2023-01-02T15:08:56Z

Signal: should also solve meilisearch/product#565

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

3295: Adjust Master Key-related messages r=irevoire a=dureuill # Pull Request ## Related issue Follow up for #3272 ## What does this PR do? - Consistently capitalize "master key" (instead of "Master Key" sometimes) (see meilisearch/specifications#209 (comment)) - Clarify that the counted unit for master key length is bytes, not characters (see meilisearch/documentation#2069 (comment)) ## PR checklist Please check if your PR fulfills the following requirements: - [x] Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)? - [x] Have you read the contributing guidelines? - [x] Have you made sure that the title is accurate and descriptive of the changes? Thank you so much for contributing to Meilisearch! Co-authored-by: Louis Dureuil <louis@meilisearch.com>

dureuill · 2023-01-03T14:29:06Z

Clarified that the master key length is tested against its size in bytes, not characters

3295: Adjust Master Key-related messages r=dureuill a=dureuill # Pull Request ## Related issue Follow up for #3272 ## What does this PR do? - Consistently capitalize "master key" (instead of "Master Key" sometimes) (see meilisearch/specifications#209 (comment)) - Clarify that the counted unit for master key length is bytes, not characters (see meilisearch/documentation#2069 (comment)) ## PR checklist Please check if your PR fulfills the following requirements: - [x] Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)? - [x] Have you read the contributing guidelines? - [x] Have you made sure that the title is accurate and descriptive of the changes? Thank you so much for contributing to Meilisearch! Co-authored-by: Louis Dureuil <louis@meilisearch.com>

dureuill · 2023-01-11T10:29:25Z

Thanks for the warning and error messages, @gmourier !

text/0119-instance-options.md

gmourier

@dureuill @brunoocasali I've suggested some changes regarding the error and warning messages. WDYT?

dureuill · 2023-01-12T09:16:53Z

@gmourier: answered inline above with a concern in the case of development mode with too short a master key.

Other changes look fine to me ❤️

brunoocasali

I left one comment, and I like the @gmourier's suggestions

Amazing work @dureuill 🎉

text/0119-instance-options.md

gmourier · 2023-01-13T10:53:03Z

I completely understand the message's purpose, but if the user does it literally, it may not work when using Docker.

Because if you run the export command, the ENV var will be applied to the host, and the user will still have to pass it > through the docker command.
So it does not work if the user "just restart Meilisearch".

Should we give the --master-key options instead of the export command to overcome that @brunoocasali?

…6 bytes in env development

dureuill · 2023-01-16T14:10:43Z

The differences I see between the export command and the --master-key CLI flag are the following:

With things like command history, etc, it is more dangerous to put a "master key" in clear as a CLI flag. The user is also more likely to lose the master key in the future, as opposed to the export MASTER_KEY in their bashrc. So I'd say the export command is better than the --master-key CLI flag from a security standpoint.
The export command will not work as-is with docker as noted by @brunoocasali, and will also not work with some OSes (e.g., Windows) and different shells (e.g. fish). In contrast the CLI flag will always work the same. So I'd say the CLI flag is better than the export command from a usability standpoint.

I'm not sure how to resolve the tension between (1) and (2). I'd say that since the generated master key is already printed to stdout, maybe we can favor usability and suggest the CLI option, and have users who want the best security use another tool to generate their master key?

gmourier · 2023-01-16T15:08:51Z

I agree with you @dureuill; we can favor the --master-key option instead of the export one. One solution could be to display both... @brunoocasali WDYT?

I think we need to facilitate the use of this information for docker users (it's a super huge part of utilization) and at the same time, windows users.

If we don't have a proper answer before Wednesday, we will move onto the --master-key option @dureuill.

brunoocasali · 2023-01-16T18:24:46Z

@dureuill made excellent observations about it.

I think the --master-key option is the best because it will work across all the environments as Louis pointed out. And even if the user does not like the option, they will have to review the docs to check another option to use it (config file, env var, or keep the CLI).

Could we also have some opinions from the docs team? Because maybe by changing the way the message is written, we could avoid misunderstandings.

gmourier · 2023-01-17T08:53:37Z

Hi @maryamsulemani97 👋

Can you check if the text messages are understandable and correct from the doc team's POV?
It would be super helpful 🙇‍♂️ Please, let me know if I should ask someone else

maryamsulemani97

Made a few minor suggestions 🪴

text/0119-instance-options.md

Co-authored-by: Maryam <90181761+maryamsulemani97@users.noreply.github.com>

gmourier · 2023-01-18T14:27:44Z

Thank you, @maryamsulemani97, for your review! I've integrated all the suggested changes.

@dureuill, @brunoocasali I've changed the export MEILI_MASTER_KEY= by --master-key

dureuill

Saw 2 little changes to make

text/0119-instance-options.md

Co-authored-by: Louis Dureuil <louis.dureuil@gmail.com>

gmourier

LGTM! Thank you!

3406: Master Key: Implements errors and warnings from the specification r=irevoire a=dureuill <sub>Now in technicolor</sub> # Pull Request ## What does this PR do? - Uses `atty` and `termcolor` as dependency - Use these dependencies to print colored background for warning messages - Update messages to match meilisearch/specifications#209 ## 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! Co-authored-by: Louis Dureuil <louis@meilisearch.com>

* Bump open-api spec * Simplify dump version section since v1.0 can import all dumps (#203) * Simplify dump version section since v1.0 can import all dumps * Relax target import version Co-authored-by: Clémentine Urquizar - curqui <clementine@meilisearch.com> --------- Co-authored-by: Clémentine Urquizar - curqui <clementine@meilisearch.com> * Rename dump command from `--dumps-dir` to `--dump-dir` (#204) * Propagate the change to --dumps-dir to the specification * Rename section header in openAPI documentation * Dump destination -> Dump directory * Finish destination -> directory renaming Co-authored-by: Guillaume Mourier <guillaume@meilisearch.com> --------- Co-authored-by: Guillaume Mourier <guillaume@meilisearch.com> * Update the specification of soft-deleted documents (#206) * Grammarly'd soft-deletion page * Soft-deleted: update the functional specification * Remove `--max-index-size` and `--max-task-db-size` (#207) * Robustify Primary Key Inference - Update related errors (#208) * update inference errors * Add `index_` prefix to newly-introduced error codes Co-authored-by: Guillaume Mourier <guillaume@meilisearch.com> * Update error messages following meilisearch/meilisearch#3301 --------- Co-authored-by: Guillaume Mourier <guillaume@meilisearch.com> * Reject master keys shorter than 16 bytes in `production` (#209) * Specify what happens when too short a master key is provided * Change error message * Characters -> bytes for master key length * Specify Error and Warning messages regarding the master-key * Update text/0119-instance-options.md * Update text/0119-instance-options.md * Specify warning messages when a master key is missing or lower than 16 bytes in env development * Apply suggestions from code review Co-authored-by: Maryam <90181761+maryamsulemani97@users.noreply.github.com> * Replace export MEILI_MASTER_KEY by --master-key * Apply suggestions from code review Co-authored-by: Louis Dureuil <louis.dureuil@gmail.com> --------- Co-authored-by: Guillaume Mourier <guillaume@meilisearch.com> Co-authored-by: Maryam <90181761+maryamsulemani97@users.noreply.github.com> * Add `OFF` log level to spec (#211) * Errors handling enhancement (#212) * Replace invalid_duplicate_index_found error code to invalid_swap_duplicate_index_found * Remove bad_request for settings properties error; replaced with dedicated error codes * cleaning old bad usage of the spec and replaces it with relevant information * Split search bad_request error code to dedicated error code * Catch-up: remove unused dump_not_found error code * Do an update pass on old specs format * Details context, variant cases * Add system type;Split invalid_task_date_filter to dedicated error codes * Precise index API bad_request error code to dedicated one * Precise bad_request to a dedicated code for the swap indexes API * Use invalid_index_primary_key when ?primaryKey is invalid instead of bad_request; + add TODO bad_request we forgot * marking missed bad_request on /keys * Add missed bad_request codes for GET /keys * Add missed bad_request codes for GET /documents * Removes _filter suffix from query parameter error_code for tasks API; Add missing bad_request error_code on GET /tasks * Add missing bad_request error codes on GET /indexes * remove future possibilities * Replace search_Parameter by search into the /search error codes naming * Replace bad_request code when the mandatory uid is missing when posting an index resource; split the generic missing_parameter to dedicated error codes * fix sentence sense * Split immutable_field to dedicated api key fields * Removes missing todo marker; Add missing part * Add suggested review comments * Catch-up/Add when an invalid index uid is gen for the :indexUid path parameter * Add immutable_index_uid error code on PATCH /indexes/:indexUid when uid is specified in the request payload * invalid_settings_ranking_rules can only be synchronous * Details async/sync case for error related to minWordsSizeForTypos object * Add immutable_index_created_at and immutable_index_updated_at for the Index API Resource * Add missing_swap_indexes on the POST swap-indexes resource * Update text/0061-error-format-and-definitions.md Co-authored-by: Bruno Casali <brunoocasali@gmail.com> * merge the :deserr_helper and the :deserr_location * Rephrase and catch-up miss * get rids of the index_not_accessible error code since no one remember about it --------- Co-authored-by: Bruno Casali <brunoocasali@gmail.com> Co-authored-by: Tamo <tamo@meilisearch.com> * Add error message when doing a migration (#213) * Add the migration CLI error * Update text/0105-dumps-api.md --------- Co-authored-by: Guillaume Mourier <guillaume@meilisearch.com> * Merge snapshot options (#214) * Merge `--schedule-snapshot` and `--snapshot-interval-sec` options * Update text/0119-instance-options.md --------- Co-authored-by: Guillaume Mourier <guillaume@meilisearch.com> * Remove any reference to the disable-auto-batching argument (#215) Co-authored-by: Guillaume Mourier <guillaume@meilisearch.com> * Specify the effect of the `*` value when used in the task filters (#218) --------- Co-authored-by: Louis Dureuil <louis.dureuil@gmail.com> Co-authored-by: Clémentine Urquizar - curqui <clementine@meilisearch.com> Co-authored-by: Maryam <90181761+maryamsulemani97@users.noreply.github.com> Co-authored-by: Bruno Casali <brunoocasali@gmail.com> Co-authored-by: Tamo <tamo@meilisearch.com> Co-authored-by: Clément Renault <clement@meilisearch.com>

Specify what happens when too short a master key is provided

ad4f807

dureuill changed the title ~~Reject master keys with a length shorter than 16~~ Reject master keys shorter than 16 characters Dec 26, 2022

dureuill changed the base branch from main to release-v1.0.0 December 26, 2022 16:18

dureuill added Ready For Review Feature specification must be reviewed. Implemented Feature specification has been implemented. v1.0.0 ⚠️ Breaking Introduce a breaking change. Q1:2023 labels Dec 26, 2022

dureuill requested a review from gmourier December 26, 2022 16:20

maryamsulemani97 mentioned this pull request Jan 2, 2023

v1.0: Reject master keys with a size lower than 16 bytes meilisearch/documentation#2069

Closed

2 tasks

gmourier mentioned this pull request Jan 2, 2023

Release v1.0.0 #201

Merged

1 task

gmourier suggested changes Jan 2, 2023

View reviewed changes

text/0085-api-keys.md Outdated Show resolved Hide resolved

gmourier suggested changes Jan 2, 2023

View reviewed changes

text/0085-api-keys.md Outdated Show resolved Hide resolved

irevoire added a commit that referenced this pull request Jan 2, 2023

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

b3a3afd

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

irevoire mentioned this pull request Jan 2, 2023

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

Closed

dureuill mentioned this pull request Jan 3, 2023

Adjust Master Key-related messages meilisearch/meilisearch#3295

Merged

3 tasks

dureuill added 2 commits January 3, 2023 15:24

Change error message

f9af78d

Characters -> bytes for master key length

f1cff87

dureuill changed the title ~~Reject master keys shorter than 16 characters~~ Reject master keys shorter than 16 bytes Jan 3, 2023

dureuill requested a review from gmourier January 3, 2023 14:29

gmourier changed the title ~~Reject master keys shorter than 16 bytes~~ Reject master keys shorter than 16 bytes in production Jan 5, 2023

Specify Error and Warning messages regarding the master-key

9facc5e

gmourier reviewed Jan 11, 2023

View reviewed changes

text/0119-instance-options.md Outdated Show resolved Hide resolved

gmourier reviewed Jan 11, 2023

View reviewed changes

text/0119-instance-options.md Outdated Show resolved Hide resolved

gmourier reviewed Jan 11, 2023

View reviewed changes

text/0119-instance-options.md Outdated Show resolved Hide resolved

gmourier reviewed Jan 11, 2023

View reviewed changes

gmourier requested a review from brunoocasali January 11, 2023 12:30

brunoocasali approved these changes Jan 12, 2023

View reviewed changes

text/0119-instance-options.md Outdated Show resolved Hide resolved

gmourier added 3 commits January 16, 2023 14:33

Update text/0119-instance-options.md

09090df

Update text/0119-instance-options.md

da65d2a

Specify warning messages when a master key is missing or lower than 1…

16a430b

…6 bytes in env development

gmourier self-requested a review January 16, 2023 13:43

gmourier requested a review from maryamsulemani97 January 17, 2023 08:52

maryamsulemani97 reviewed Jan 17, 2023

View reviewed changes

maryamsulemani97 reviewed Jan 18, 2023

View reviewed changes

text/0119-instance-options.md Outdated Show resolved Hide resolved

gmourier and others added 2 commits January 18, 2023 15:23

Apply suggestions from code review

eb35ed7

Co-authored-by: Maryam <90181761+maryamsulemani97@users.noreply.github.com>

Replace export MEILI_MASTER_KEY by --master-key

e3c1a1c

gmourier requested a review from brunoocasali January 18, 2023 17:49

dureuill commented Jan 19, 2023

View reviewed changes

text/0119-instance-options.md Outdated Show resolved Hide resolved

text/0119-instance-options.md Outdated Show resolved Hide resolved

Apply suggestions from code review

4ab2d88

Co-authored-by: Louis Dureuil <louis.dureuil@gmail.com>

gmourier approved these changes Jan 19, 2023

View reviewed changes

dureuill mentioned this pull request Jan 19, 2023

Master Key: Implements errors and warnings from the specification meilisearch/meilisearch#3406

Merged

3 tasks

gmourier merged commit 30a63af into release-v1.0.0 Feb 6, 2023

gmourier deleted the too_short_master_keys branch February 6, 2023 10:11

gmourier removed the Ready For Review Feature specification must be reviewed. label Feb 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reject master keys shorter than 16 bytes in `production` #209

Reject master keys shorter than 16 bytes in `production` #209

dureuill commented Dec 26, 2022 •

edited

Loading

gmourier commented Jan 2, 2023

dureuill commented Jan 3, 2023

dureuill commented Jan 11, 2023

gmourier left a comment •

edited

Loading

dureuill commented Jan 12, 2023

brunoocasali left a comment

gmourier commented Jan 13, 2023 •

edited

Loading

dureuill commented Jan 16, 2023

gmourier commented Jan 16, 2023 •

edited

Loading

brunoocasali commented Jan 16, 2023

gmourier commented Jan 17, 2023 •

edited

Loading

maryamsulemani97 left a comment

gmourier commented Jan 18, 2023

dureuill left a comment

gmourier left a comment

Reject master keys shorter than 16 bytes in production #209

Reject master keys shorter than 16 bytes in production #209

Conversation

dureuill commented Dec 26, 2022 • edited Loading

Summary

Changes

Out Of Scope

Attention To Reviewers

Misc

gmourier commented Jan 2, 2023

dureuill commented Jan 3, 2023

dureuill commented Jan 11, 2023

gmourier left a comment • edited Loading

Choose a reason for hiding this comment

dureuill commented Jan 12, 2023

brunoocasali left a comment

Choose a reason for hiding this comment

gmourier commented Jan 13, 2023 • edited Loading

dureuill commented Jan 16, 2023

gmourier commented Jan 16, 2023 • edited Loading

brunoocasali commented Jan 16, 2023

gmourier commented Jan 17, 2023 • edited Loading

maryamsulemani97 left a comment

Choose a reason for hiding this comment

gmourier commented Jan 18, 2023

dureuill left a comment

Choose a reason for hiding this comment

gmourier left a comment

Choose a reason for hiding this comment

Reject master keys shorter than 16 bytes in `production` #209

Reject master keys shorter than 16 bytes in `production` #209

dureuill commented Dec 26, 2022 •

edited

Loading

gmourier left a comment •

edited

Loading

gmourier commented Jan 13, 2023 •

edited

Loading

gmourier commented Jan 16, 2023 •

edited

Loading

gmourier commented Jan 17, 2023 •

edited

Loading