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

API Keys - Determinist API Keys + Security changes #148

Merged
merged 17 commits into from
Jul 7, 2022

Conversation

gmourier
Copy link
Member

@gmourier gmourier commented May 4, 2022

🤖 API Diff
📄 Rendered


Summary

This update to the Keys API makes several changes:

  • It is possible to create a deterministic key value by specifying a uid field at creation.
  • It is possible to get a key from the key field or the uid field.
  • Specifying a key to update or delete is made with the uid field.
  • Adds a name field to give a human-readable name to ease API key retrieval in a list at the convenience of the user.
  • The master key is only dedicated to API keys management, it can't be used for other endpoints. Making Meilisearch more secure by design and thus preventing users from introducing a security vulnerability.
  • Updating a key only allows the name and description fields.
  • Introduce new actions to manage API Keys (keys.get, keys.create, keys.update, keys.delete). The Default Admin API Key is able to manage keys by default.
  • apiKeyPrefix tenant token claim is renamed apiKeyUid.

Changes

uid Field Addition

The generation of a randomized prefix of 8 characters is removed by the existence of the uid field. This field accepts a string representing a UUID v4.

It is possible to specify this uid field when creating a key, if not specified it is generated by Meilisearch.

As the value of key is a hash of the uid and the master key, it is possible to have deterministic key values between instances sharing the same configuration thus keeping some interesting security features as:

  • If the master key changes, all key values are automatically changed.
  • The API key resources are propagated into the dumps and snapshot without displaying the key field in clear.

A key is retrieved from its uid rather than its key value. e.g. GET /keys/:uid_or_key instead of GET /keys/:key.

Specifying a uid that already exists returns a 409 Conflict.

name Field Addition

A name field is added to allow a human-readable name to better read the information within a list of keys. The description field is kept to add additional information if needed.

For example, the description field is useful to give important information about the keys generated by default that would not necessarily have its place in the name. This makes it easier to parse in a dashboard when building a UI or in the GET /keys response.

The default generated API keys are updated to reflect this change.

{
    ...,
    "name": "Default Admin API Key",
    "description": "Use it for all other than search operations. Caution! Do not expose it on a public frontend",
    ...
}

Note that the description is updated. ( / ) are removed.

Master key

The master key can only be used to manage API keys.

Several times, it has been found that users use the master key to make client-side requests, which is problematic for their security. In order to ensure that they do not inadvertently put themselves in danger, the master key can no longer access routes other than /keys.

keys.* actions

The specification adds new rights dedicated to the management of API keys.

These actions are keys.get, keys.create, keys.update, keys.delete.

The default Admin key also has these rights. actions is equal to * at its creation.

The goal is to give the possibility to users to create dedicated keys for that purpose and to reduce the use of the master key for this endpoint.

API Key Update

It is no longer possible to update the sensitive information of an API key which could introduce undesired permissions escalation as a side effect.

However, it is possible to update the name and description fields.

Out Of Scope

Adding a unique human-readable field as an alias to retrieve an API Key other than its uid is not considered in this iteration. It should be easy to add it later without breaking changes.


📡 Attention To Reviewers 📡

@meilisearch/cloud-team We basically need your attention on all these changes.

The big question mark could be the update of the permissions of an API key on your side, which users could request. This could be done within a transaction on your side to allow the update if it is a vital need.

The workaround would be to delete the key that needs to be updated and recreate one with the new permissions by giving it the uid of the previous one. This way, the key value used by clients to authorize requests is unchanged.

The regeneration of a key would be done in the same way by creating a key copying the permissions but giving it a new uid.

Also, do you have any particular constraints concerning the master key, which is now restricted to endpoint /keys access only?

@nicolasvienot The addition of the name field should allow you to have more fluidity in the construction of the UI and thus avoid having to parse the description field to display the default keys properly.


Misc

  • Update OpenAPI specification file
  • Reorganize the specification

@gmourier gmourier added OpenAPI Update OpenAPI specification. In Progress Feature specification is in elaboration. Important changes can still occurs in the specification. labels May 4, 2022
@gmourier gmourier force-pushed the determinist-api-keys branch from 1971dbe to 1961502 Compare May 4, 2022 10:30
@github-actions
Copy link

github-actions bot commented May 4, 2022

🚨 Breaking API change detected:

Modified (4)

  • GET /keys
    • Response modified: 200
      • Body attribute modified: results
  • GET /keys/{uid_or_key}
    • Path parameter added: uidOrKey
    • Response modified: 200
      • Body attributes added: uid, name
  • PATCH /keys/{uid_or_key}
    • Body modified
      • Body attributes added: name, Additional properties are NOT allowed
      • [Breaking] Body attributes removed: key, actions, indexes, expiresAt, createdAt, updatedAt
    • Response modified: 200
      • Body attributes added: uid, name
  • [Breaking] POST /keys
    • [Breaking] Body modified
      • Body attributes added: uid, name
    • [Breaking] Response modified: 200
      • Body attributes added: uid, name

View documentation diff

Powered by Bump

@gmourier gmourier force-pushed the determinist-api-keys branch 2 times, most recently from 3415848 to 004e256 Compare May 4, 2022 10:38
@gmourier gmourier added Ready For Review Feature specification must be reviewed. and removed In Progress Feature specification is in elaboration. Important changes can still occurs in the specification. labels May 4, 2022
@gmourier gmourier marked this pull request as ready for review May 4, 2022 12:39
@gmourier gmourier added In Progress Feature specification is in elaboration. Important changes can still occurs in the specification. and removed Ready For Review Feature specification must be reviewed. labels May 4, 2022
Copy link
Member

@brunoocasali brunoocasali left a comment

Choose a reason for hiding this comment

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

Awesome work @gmourier 💪

open-api.yaml Show resolved Hide resolved
text/0085-api-keys.md Show resolved Hide resolved
text/0085-api-keys.md Outdated Show resolved Hide resolved
text/0085-api-keys.md Outdated Show resolved Hide resolved
text/0085-api-keys.md Outdated Show resolved Hide resolved
text/0085-api-keys.md Outdated Show resolved Hide resolved
text/0085-api-keys.md Outdated Show resolved Hide resolved
text/0085-api-keys.md Show resolved Hide resolved
@curquiza curquiza added the v0.28 label May 5, 2022
@gmourier gmourier force-pushed the determinist-api-keys branch from 004e256 to 6c0fdaf Compare May 16, 2022 12:18
@gmourier gmourier mentioned this pull request May 19, 2022
1 task
@gmourier gmourier changed the title API Keys - Introduces a uid field to make API Key determinists and a name field to ease information reading. API Keys - Introduces a uid field to make API Key determinists and a name field to ease information reading... May 23, 2022
@gmourier
Copy link
Member Author

gmourier commented May 23, 2022

@meilisearch/integration-team @ManyTheFish There remains a design question regarding the JWT payload for tenant token.

Currently, the JWT requires an apiKeyPrefix. It should be renamed since the prefix won't exist anymore. Here is 2 possibilities I can think of:

  • apiKey: Fill in the signing API key's full key field. (Not sure about this one!)
  • apiKeyUid: Fill in the uid of the signing API key.

What will be the best choice according to you?

@gmourier
Copy link
Member Author

gmourier commented May 23, 2022

@ManyTheFish will explore two additional things if the time permits it. Otherwise, it will be considered for future iterations.

GET /keys/:uid_or_key

See if it's possible to have GET /keys/:uid_or_key to transparently get the details of an API Key given uid or key.

Add specific rights to manage keys

Since the master key will only be used to manage /keys, we are exploring if we can easily add new actions to manage keys.

This way, the master key will be less shared and mostly used one time to fetch the defaults Search Key and Admin Key. The Admin Key will gain the rights to manage /keys by default.


Depending on the exploration results, the specification could be modified to add these points.

@gmourier gmourier added Ready For Review Feature specification must be reviewed. Q3:2022 and removed In Progress Feature specification is in elaboration. Important changes can still occurs in the specification. labels May 23, 2022
Copy link
Member

@ManyTheFish ManyTheFish left a comment

Choose a reason for hiding this comment

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

@gmourier, I suggested changes about key derivation.

text/0085-api-keys.md Outdated Show resolved Hide resolved
text/0085-api-keys.md Outdated Show resolved Hide resolved
open-api.yaml Outdated Show resolved Hide resolved
@Kerollmops
Copy link
Member

It would be much better if we could add a sentence about the secret for the HMACSHA256 hashing must be the original API Key value (not the UID and not base64 encoded). I can't add this sentence myself, the area where I put that is out of the diff.

Copy link
Member

@ManyTheFish ManyTheFish left a comment

Choose a reason for hiding this comment

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

Request changes related to meilisearch/meilisearch#2584

text/0085-api-keys.md Outdated Show resolved Hide resolved
text/0085-api-keys.md Outdated Show resolved Hide resolved
@gmourier gmourier force-pushed the determinist-api-keys branch from 9912e49 to 826bfb5 Compare July 7, 2022 12:00
@gmourier gmourier merged commit 979b6ae into develop Jul 7, 2022
@gmourier gmourier deleted the determinist-api-keys branch July 7, 2022 13:55
gmourier added a commit that referenced this pull request Jul 7, 2022
* Add an uid to make API Keys determinists, plus add a non-unique human readable name field to ease reading information

* Describe errors for uid and name fields

* Apply suggestions from code review

Co-authored-by: Bruno Casali <brunoocasali@gmail.com>

* misc: add precisions

* Reorganize route descriptions

* Update error_code when API Key already exists for a given uid

* Apply suggestions from code review

Co-authored-by: Many the fish <legendre.maxime.isn@gmail.com>

* Add new keys actions, remove master-key changes, introduce a new error for immutable field and update tenant token

* Update open-api spec

* Update immutable_field error message

* Apply suggestions from code review

Co-authored-by: Many the fish <legendre.maxime.isn@gmail.com>

* Mention that the Default Admin API Key can manage keys

* Specify that the JWT Tenant Token must be enrypted with the API Key value

* Update the spec regarding the description of the Admin API Key to be up-to-date

* Add uid_or_key url param to update and delete a key

* Update text/0085-api-keys.md

Co-authored-by: Many the fish <legendre.maxime.isn@gmail.com>

* Update text/0085-api-keys.md

Co-authored-by: Many the fish <legendre.maxime.isn@gmail.com>

Co-authored-by: Bruno Casali <brunoocasali@gmail.com>
Co-authored-by: Many the fish <legendre.maxime.isn@gmail.com>
Co-authored-by: Kerollmops <clement@meilisearch.com>
gmourier added a commit that referenced this pull request Jul 11, 2022
* Bump open-api.yml to v0.28

* Telemetry - Add `x-meilisearch-client` query parameter (#145)

* Introduce the x-meilisearch-client query parameter

* Update text/0034-telemetry-policies.md

Co-authored-by: Bruno Casali <brunoocasali@gmail.com>

* Update text/0034-telemetry-policies.md

Co-authored-by: Bruno Casali <brunoocasali@gmail.com>

Co-authored-by: Bruno Casali <brunoocasali@gmail.com>

* GeoSearch — Support string type for `_geo` `lat` and `lng` fields (#83)

* Update specification to support string type for _geo lat and lng fields

* mention types mixing for _geo object

* Tasks API - Rename `uid` to `taskUid` in the `202 - Accepted` Summarized Task Response (#144)

* Rename 202 uid to taskUid

* Update text/0060-tasks-api.md

Co-authored-by: cvermand <33010418+bidoubiwa@users.noreply.github.com>

Co-authored-by: cvermand <33010418+bidoubiwa@users.noreply.github.com>

* Tasks API - Seek/Keyset based pagination (#115)

* Move cursor based pagination spec to tasks API spec

* remove pagination as a future capability

* Clarify boundaries for limit query parameter

* Update OpenApi specification

* Remove limit field boundaries

* Apply suggestions from code review

Co-authored-by: Clément Renault <renault.cle@gmail.com>

* Update open-api.yml and removes cursor term mentions

* Update text/0060-tasks-api.md

Co-authored-by: Tommy <68053732+dichotommy@users.noreply.github.com>

* Remove  route mention in seek-keyset pagination section

Co-authored-by: Clément Renault <renault.cle@gmail.com>
Co-authored-by: Tommy <68053732+dichotommy@users.noreply.github.com>

* Tasks API - Filter tasks list by `type`/`status`/`indexUid` (#116)

* move filtering tasks by status/type parameter to task api spec

* Update specification

* Add details about case-sensitivy + rework error message

* Introducing naming changes plus make the specification a source of truth instead of a changelog

* Remove a future possibility being introduced

* misc - replace createIndex to the right type and add the missing type field to the 202 Response resource

* Dumps API - Make dump creation an asynchronous task (#139)

* wip

* Make a dump creation a visible asynchronous task

* Add precisions

* Update open-api.yml

* Add ommited type field for summarized task response

* Add future possibilities

* Apply suggestions from code review

Co-authored-by: cvermand <33010418+bidoubiwa@users.noreply.github.com>

* Precise that indexUid can be null

* Precise priorization of dumpCreation task over other task types

* Keep taskUid for 202 response

* remove dumps.get API key action

Co-authored-by: cvermand <33010418+bidoubiwa@users.noreply.github.com>

* Search API - Remove/Rename confusing fields (#135)

* Rename nbHits, remove exhaustive* boolean fields

* Rename approximativeNbHits to estimatedTotalHits

* Update open-api.yaml

* Apply naming changes for facet distribution and showing matches position

* Add a telemetry for facet distribution usage

* API Guideline - Return list of API resources under a `results` array (#138)

* Place list of documents under a results array on /documents

* Add results array for indexes object list

* Add the future of indexes pagination

* Update open-api.yml

* Fix typo

* Apply suggestions from code review

Co-authored-by: cvermand <33010418+bidoubiwa@users.noreply.github.com>

* Add offset/limit pagination for indexes and API keys

* Try to add multipe refs to a response object

Co-authored-by: cvermand <33010418+bidoubiwa@users.noreply.github.com>

* Remove name field (#140)

* Documents API - `displayedAttributes` should not impact the documents API / Rename `attributesToRetrieve` to `fields` (#143)

* Specifies that displayedAttributes setting does not impact the GET documents endpoint

* Rename attributeToRetrieve to fields on /documents

* Add a future possibily to rejectt a field from a document in the given response

* Precise behavior details about fields query parameter

* Add fields query parameter on GET /indexes/{index}/documents/{docId}

* API Keys - Determinist API Keys + Security changes (#148)

* Add an uid to make API Keys determinists, plus add a non-unique human readable name field to ease reading information

* Describe errors for uid and name fields

* Apply suggestions from code review

Co-authored-by: Bruno Casali <brunoocasali@gmail.com>

* misc: add precisions

* Reorganize route descriptions

* Update error_code when API Key already exists for a given uid

* Apply suggestions from code review

Co-authored-by: Many the fish <legendre.maxime.isn@gmail.com>

* Add new keys actions, remove master-key changes, introduce a new error for immutable field and update tenant token

* Update open-api spec

* Update immutable_field error message

* Apply suggestions from code review

Co-authored-by: Many the fish <legendre.maxime.isn@gmail.com>

* Mention that the Default Admin API Key can manage keys

* Specify that the JWT Tenant Token must be enrypted with the API Key value

* Update the spec regarding the description of the Admin API Key to be up-to-date

* Add uid_or_key url param to update and delete a key

* Update text/0085-api-keys.md

Co-authored-by: Many the fish <legendre.maxime.isn@gmail.com>

* Update text/0085-api-keys.md

Co-authored-by: Many the fish <legendre.maxime.isn@gmail.com>

Co-authored-by: Bruno Casali <brunoocasali@gmail.com>
Co-authored-by: Many the fish <legendre.maxime.isn@gmail.com>
Co-authored-by: Kerollmops <clement@meilisearch.com>

* Geosearch - Enhance lat/lng format error messages (#149)

* Update the geosearch error message

* misc: organiser error message in the right specification

Co-authored-by: Guillaume Mourier <guillaume@meilisearch.com>

* Introduces HTTP Verbs changesto be compliant regarding a Rest API (#152)

* Telemetry - Replace `x-meilisearch-client` query parameter by `X-Meilisearch-Client` header (#150)

* Removes x-meilisearch-client, replace it by a header

* Remove capslock

* fix typo (#151)

* Mention telemetry.meilisearch.com (#153)

* update ranking rules error message (#154)

* Misc — Update dump versions compatibility table (#156)

* Update dump table

* Update text/0105-dumps-api.md

* Settings API - Customize the hard limits for `pagination` and `faceting` (#157)

* Introduces specification files

* Update files name

* branch telemetry

* Update open-api.yml

* Update text/0034-telemetry-policies.md

Co-authored-by: Clément Renault <renault.cle@gmail.com>

* update open-api.yml

* Update text/157-faceting-setting-api.md

Co-authored-by: Clément Renault <renault.cle@gmail.com>

* Rename limitedTo to maxTotalHits

* Specify order of returned facet

Co-authored-by: Clément Renault <renault.cle@gmail.com>

* Add dumpCreation task type to OpenAPI.yml

* Tasks filtering params to be in query instead of path on OpenAPI spec

Co-authored-by: Bruno Casali <brunoocasali@gmail.com>
Co-authored-by: cvermand <33010418+bidoubiwa@users.noreply.github.com>
Co-authored-by: Clément Renault <renault.cle@gmail.com>
Co-authored-by: Tommy <68053732+dichotommy@users.noreply.github.com>
Co-authored-by: Many the fish <legendre.maxime.isn@gmail.com>
Co-authored-by: Kerollmops <clement@meilisearch.com>
Co-authored-by: Tamo <tamo@meilisearch.com>
Co-authored-by: ad hoc <postma.marin@protonmail.com>
Co-authored-by: Clémentine Urquizar - curqui <clementine@meilisearch.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
OpenAPI Update OpenAPI specification. Q3:2022 Ready For Review Feature specification must be reviewed. v0.28
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants