Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Enterprise Search] Add .connector-secrets system index and GET/POST requests #103683

Merged
merged 27 commits into from
Jan 25, 2024

Conversation

navarone-feekery
Copy link
Contributor

@navarone-feekery navarone-feekery commented Dec 22, 2023

Context

The Search team is currently implementing BYOI (Bring Your Own Index) for Connectors. This feature will allow users to create search indices for Connectors without having search- prefix the index name.
Native Connectors are run using the Enterprise Search system account, which has access to search indices because it can access all indices that follow the pattern search-*. To continue allowing Native Connectors to function after removing the prefix requirement, we need to switch to using API keys to manage index access. That brings us to this PR.

The goal of this PR is to have a place where Native Connectors can store API keys (secrets storage). A connector secret can be stored from either Kibana or the Connectors CLI. Kibana would only have write access to this index so it cannot expose secrets in the UI. Enterprise Search (which runs the Connectors CLI on cloud) will have both read and write access.

These changes follow the implementation design of Fleet's secrets storage (example PR). The implementation is largely identical, with a change in namespace.

Changes

  • Introduce new internal system index called .connector-secrets. This will be used to store the API keys that Native Connectors will use.
  • Add GET and POST requests for connector secrets
  • Give the Kibana system account write access to .connector-secrets
  • Give the Enterprise Search account read and write access to .connector-secrets

Not included

  • To reduce PR size, DELETE requests will be added in a further PR

How to test this PR

The API endpoints can be tested in Kibana Dev Tools:

  1. Run ES from source with ./gradlew run.

  2. Once ES is running, change the default password for the elastic user from "password" to "changeme" to match Kibana's:

    curl -k -u elastic-admin:elastic-password -H 'Content-Type: application/json' \
    http://localhost:9200/_security/user/elastic/_password -d'{"password": "changeme"}'
    curl -k -u elastic:changeme -H 'Content-Type: application/json' \
     http://localhost:9200/_security/user/kibana_system/_password -d'{"password": "changeme"}'
    
  3. Run Kibana with yarn start.

  4. Log into Kibana and go to /app/dev_tools#/console.

  5. Create a secret:

    POST /_connector/secret/
    {
      "value": "test123"
    }
    
  6. This should succeed and return the id of the created secret doc, e.g.:

    {
      "id": "1234"
    }
    
  7. Copy the id and try to get the secret:

    GET /_connector/secret/1234
    
  8. This should succeed and return the secret id and value:

    {
      "id": "1234",
      "value": "test123"
    }
    
  9. Try to get a different secret using a random value. It should fail with 404 and return an error.

    GET /_connector/secret/notrealid
    

@navarone-feekery navarone-feekery changed the title [WIP][Enterprise Search] Add system index for Connector secrets and GET request [WIP][Enterprise Search] Add .connector-secrets system index and GET request Dec 22, 2023
@navarone-feekery navarone-feekery changed the title [WIP][Enterprise Search] Add .connector-secrets system index and GET request [WIP][Enterprise Search] Add .connector-secrets system index and GET/POST requests Jan 9, 2024
@navarone-feekery navarone-feekery changed the title [WIP][Enterprise Search] Add .connector-secrets system index and GET/POST requests [Enterprise Search] Add .connector-secrets system index and GET/POST requests Jan 9, 2024
@navarone-feekery navarone-feekery marked this pull request as ready for review January 9, 2024 15:20
@navarone-feekery navarone-feekery requested review from a team as code owners January 9, 2024 15:20
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/ent-search-eng (Team:Enterprise Search)

@@ -38,6 +38,13 @@ dependencies {
module ':modules:search-business-rules'
}

testClusters.configureEach {
testDistribution = 'DEFAULT'
Copy link
Contributor

Choose a reason for hiding this comment

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

@breskeby we should think about making it simpler to setup clusters that require security w/o having to use the default distribution. I suspect the vast majority of tests that use the default distro is for this reason alone.

Otherwise these changes LGTM.

Copy link
Member

@kderusso kderusso left a comment

Choose a reason for hiding this comment

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

This looks great! A couple of questions and notes.

The only other thing that I'd request before merging is adding unit tests extending AbstractBWCSerializationTestCase for your request and response objects. After that I'm ready to 👍. Thank you!

Copy link
Contributor

@timgrein timgrein left a comment

Choose a reason for hiding this comment

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

Really nice work and congrats on your (I think) first PR in ES 👏 And thank you for the detailed PR description!

Left some minor comments, which are non-blocking and having one question: Do you plan to add the Backwards compatibility tests, index service tests and other unit tests, too?

Copy link
Member

@jedrazb jedrazb left a comment

Choose a reason for hiding this comment

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

Great work! 🚀 Left some minor comments

Copy link
Contributor

@timgrein timgrein left a comment

Choose a reason for hiding this comment

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

One additional question around endpoint naming 🙏

Copy link
Member

Choose a reason for hiding this comment

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

Not for this PR, but we should consider creating separate directories for each Ent Search feature - this is becoming crowded

Copy link
Member

Choose a reason for hiding this comment

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

++

@navarone-feekery
Copy link
Contributor Author

@timgrein I've added BWC and index service tests. Let me know if there are more tests I should add.
@carlosdelest I've removed the unnecessary index tests now that we rely on origin settings + privileges.

Copy link
Member

@carlosdelest carlosdelest left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for iterating on this! 🚀

@navarone-feekery navarone-feekery merged commit b4345d9 into elastic:main Jan 25, 2024
15 checks passed
@navarone-feekery navarone-feekery deleted the connector-secrets branch January 25, 2024 12:56
navarone-feekery added a commit to navarone-feekery/elasticsearch that referenced this pull request Jan 25, 2024
navarone-feekery added a commit that referenced this pull request Jan 25, 2024
@navarone-feekery navarone-feekery restored the connector-secrets branch January 25, 2024 13:36
henningandersen pushed a commit to henningandersen/elasticsearch that referenced this pull request Jan 25, 2024
…T requests (elastic#103683)

- Introduce new internal system index called .connector-secrets
- Add GET and POST requests for connector secrets
- Create read_connector_secrets and write_connector_secrets role permissions
henningandersen pushed a commit to henningandersen/elasticsearch that referenced this pull request Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:EnterpriseSearch/Application Enterprise Search >non-issue Team:Enterprise Search Meta label for Enterprise Search team v8.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants