Contrib.social

Change master key to API and search key for improved security

Open on Drupal.org →
Created on 6 February 2024, 9 months ago
Updated 28 February 2024, 9 months ago

Problem/Motivation

As laid out in ✨ Allow usage of API with dropped privileges (not only master key) Needs work , only using a master key is insecure.
The linked ticket seems to have a finished solution that takes care of full key management from the backend without having to deal directly with the Meilisearch instance (via HTTP requests), which is a big bonus (not only) for newcomers.
However, it adds a lot of functionality that goes beyond the basic intention of the search_api_meilisearch core module.
The code and workflow developed in the linked ticket shall therefore be outsourced to a submodule (and a note added that it is strongly recommended for production setups to either use the submodule or manually configure search & API keys per index and use them with the default module)

Proposed resolution

  • Add a second key field "Meilisearch search key"
    Help text: The Meilisearch search key is used on the frontend for actual queries when visitors trigger searches. Enter a key that only has permissions for the search action on this index. See https://www.meilisearch.com/docs/reference/api/keys#actions for details.
    Adding the master key here is strongly discouraged but can be used for a quick development setup only.
  • Rename the current Meilisearch master key to a field named "Meilisearch indexing key"
    Help text: The Meilisearch indexing key is used on the backend for data management operations, that is: adding documents to an index, emptying/reindexing, etc.
    "actions": ["documents.get", "documents.add", "documents.delete", "settings.update", "settings.get", "tasks.get", "stats.get", "version" ] need to be allowed for this to work.
    Adding the master key here is strongly discouraged but can be used for a quick development setup only.
  • An upgrade scenario has to be laid out, as we currently only have one (master) key field. One solution (that does not break existing setups) is to copy the existing key to both fields. Even if it wasn't the master key (which should not be a case to consider, as this was never implied by the form - and just misused by me :-) ), this should work, as it must have had sufficient permissions to do both indexing and searching. A warning should be issued to users that this has only be done for compatibility: On the module's front page, in the drush updb/update.php step, and whenever the two keys are the same in the backend.
    Info text: Your configured master key of the module's previous version has been migrated to both the search and indexing key at <link to config page> to avoid breaking your current functionality. This however is not a recommended setup. Please create two separate keys with their respective permissions and update the configuration.

User interface changes

The existing one master key field needs to be split in two. Help texts and a warning when both are the same should be added.

Discussion

Open for suggestions/changes. This is just how I thought it would make sense!
In particular, I don't know if the listed actions are indeed enough, someone more knowledgeable with the actual implementation in code would be better suited for defining that list.
I added the hint for using the master key as I still feel the steps for quickly checking out whether Meilisearch makes sense for a given setup should be kept low. It adds potential for insecure setups, which I don't like at all. One might as well add a hint for the key management submodule for those that don't understand what is needed here. Might be better?

✨ Feature request
Status

Needs work

Version

2.0

Component

Code

Created by

🇦🇹Austria tgoeg

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @tgoeg
  • Assigned to timotej-pl
  • Comment 9 months ago →
    🇸🇮Slovenia timotej-pl
  • Comment 9 months ago →
    🇸🇮Slovenia timotej-pl

    timotej-pl → changed the visibility of the branch 3419416-change-master-key to hidden.

  • Comment 9 months ago →
    🇸🇮Slovenia timotej-pl

    timotej-pl → changed the visibility of the branch 3419416-change-master-key to active.

  • Merge request !49Resolve #3419416 "Change master key" → (Open) created by timotej-pl
  • Pipeline finished with Failed
    9 months ago
    Total: 271s
    #100335
  • Status changed to Needs review 9 months ago
  • Comment 9 months ago →
    🇸🇮Slovenia timotej-pl

    Upon discussion with the maintainer, it was proposed to refactor the logic in how connection to the API is handled in API service. With introduction of different API keys it's easier to not store Client objects and its variables inside the service, so client object is now created when needed and API service methods require Client object as parameter.

  • Pipeline finished with Failed
    9 months ago
    Total: 274s
    #101389
  • Status changed to Needs work 9 months ago
  • Comment 9 months ago →
    🇸🇮Slovenia timotej-pl
  • Pipeline finished with Failed
    9 months ago
    Total: 271s
    #101631
  • Comment 9 months ago →
    🇸🇮Slovenia timotej-pl

    While working on resolving tests I found couple of things that change the need for specific permissions on each key.
    For Index key to work with all functionalities it needs to have permissions:
    "documents.get",
    "documents.add",
    "documents.delete",
    "settings.update",
    "settings.get",
    "tasks.get",
    "stats.get",
    "version",
    "indexes.create",
    "indexes.get",
    "indexes.update",
    "indexes.delete",
    "indexes.swap"
    Search key needs to have permissions:
    "version",
    "search",
    "indexes.get"

  • Pipeline finished with Failed
    9 months ago
    Total: 273s
    #105851
contrib.social Blog FAQ Discussions
Production build 0.71.5 2024