Create manage_connector privilege

[jedrazb]

Changes

Define manage_connector and monitor_connector privileges that will be required role by Connector APIs. Migration of existing logic and endpoints to use this privilege will be done in next steps.

I followed pattern from a previous similar PR: #97941 , few notes:

• The new privilege is not used as of now for any API endpoints. We have a migration strategy to move connectors to system indices and that would require this privilege.
• Since manage_connector is not used as of now, I didn't add this to public docs in: docs/reference/security/authorization/privileges.asciidoc (issue to do it once we start using this for api auth)
• Added this privilege to kibanaSystem, to allow for telemetry collection in the future (see reference)

[github-actions]

Documentation preview:

• ✨ Changed pages

[elasticsearchmachine]

Pinging @elastic/es-security (Team:Security)

[elasticsearchmachine]

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

[kderusso]

lgtm

[kderusso]

FYI this will be in docs.

[jedrazb]

Wouldn't it be required for the docs' tests? I think it makes sense to keep it there, as this console block references an actual privilege API output. Since the privilege is in ES, it will be returned, but I think we can skip documenting the manage_search_connector in the dedicated section.

[ioanatia]

unrelated to your changes, but I always wondered if this comment is still accurate and why couldn't we just assert the contents of the array

[ioanatia]

one side effect is that manage_search_connector will include the read_connector_secret and write_connector_secret, but that is the intention right?

elasticsearch/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/ClusterPrivilegeResolver.java

Lines 368 to 380 in 6e031f5

	public static final NamedClusterPrivilege READ_CONNECTOR_SECRETS = new ActionClusterPrivilege(
	"read_connector_secrets",
	Set.of("cluster:admin/xpack/connector/secret/get")
	);
	public static final NamedClusterPrivilege WRITE_CONNECTOR_SECRETS = new ActionClusterPrivilege(
	"write_connector_secrets",
	Set.of(
	"cluster:admin/xpack/connector/secret/delete",
	"cluster:admin/xpack/connector/secret/post",
	"cluster:admin/xpack/connector/secret/put"
	)
	);

[navarone-feekery]

It is a requirement that the user managing connectors has write_connector_secrets, so that API keys can be stored for the Connector (only relevant if the Connector is native). So that is okay to include.
Regarding read_connector_secrets, I don't think it's a requirement for users to have this. It should only be important for the Connector itself to read the API key.
If a user needs read_connector_secrets for some reason they could manually assign it to whichever role anyway, there is no restriction from doing this.

[jedrazb]

Good spot! I see that it's possible to define excludePatterns in the ActionClusterPrivilege constructor.

     * Constructor for {@link ActionClusterPrivilege} that defines what cluster actions are accessible for the
     * user with this privilege after excluding the action patterns {@code excludedActionPatterns} from the allowed action patterns
     * {@code allowedActionPatterns}

If I read the javadoc correctly it should allow us to match cluster:admin/xpack/connector/* while it's possible to exclude secrets-related privileges.

@navarone-feekery do you think the best way forward is to exclude all secret-related privileges from manage_search_connector to keep status quo? Right now API key generated used by connector service doesn't have this privilege right?

[navarone-feekery]

do you think the best way forward is to exclude all secret-related privileges from manage_search_connector to keep status quo?

I think so. Users don't need read_connector_secrets, and having write_connector_secrets be separate for now is okay. It is already a documented requirement for creating a native connector. It has a slightly different scope and we can always add it later if we expand secrets to do more things (like storing sensitive RCF values).

Right now API key generated used by connector service doesn't have this privilege right?

Yes that's correct. Native connectors use the Enterprise Search system role to fetch the API key from connector secrets, then native connectors use that API key to index documents. The API key that a Connector uses to index docs itself doesn't need access to secrets.

[jfreden]

My understanding is that the new privilege isn't currently used by anything, but implicitly grants read_connector_secrets and write_connector_secrets that grants actions in the ent-search plugin. The purpose is to switch everything that reads from the connector indices to use new actions in the ent-search plugin and that the new manage_search_connector privilege will grant access to those actions. For example getting telemetry data from the connector index for usage in kibana. Is that correct?

Do you think splitting it into more granular privileges might make sense and is there a use case for that? Such as read_connectors, write_connectors and so on?

manage_search_connectors is a little confusing, are there other type of actions or would manage_connectors be a better name?

Thank you!

[jfreden]

If you want this new privilege in serverless it needs to be added here.

You will need to merge this PR before adding it and then either add it to the serverless registry or to the tests “ignore” list, otherwise the serveless ci will fail.

[jedrazb]

Hey @jfreden thank you for reviewing. Answering your questions:

The purpose is to switch everything that reads from the connector indices to use new actions in the ent-search plugin and that the new manage_search_connector privilege will grant access to those actions.

Yes, you got it right :) The story is that: .connector-* hidden indices are planned to be migrated to system indices so we need to define a new privilege to define access to API endpoints that would interact with new hidden indices.

Do you think splitting it into more granular privileges might make sense and is there a use case for that? Such as read_connectors, write_connectors and so on?

All of our current use-cases require to have read and write permissions at the same time, so to me it makes sense to "package" is as a single manage permission. Note the destination index to which the connector would write (sync) data can have other index-level permissions.

but implicitly grants read_connector_secrets and write_connector_secrets that grants actions in the ent-search plugin.

Actually we might want to exclude some secrets-related patterns from that privilege, do I understand correctly that we can use excludePatterns for that, see my comment above.

manage_search_connectors is a little confusing, are there other type of actions or would manage_connectors be a better name?

You are right that manage_connectors perhaps fits better to existing privilege names such as write_connector_secrets and read_connector_secrets. I was tempted to use _search_ to indicate the search org owning this privilege. But I agree that manage_connectors could fit better.

You will need to merge this PR before adding it and then either add it to the serverless registry or to the tests “ignore” list, otherwise the serveless ci will fail.

I will def reach out to you before merging to clarify exact steps 😅

[jfreden]

Thanks @jedrazb!

All of our current use-cases require to have read and write permissions at the same time, so to me it makes sense to "package" is as a single manage permission. Note the destination index to which the connector would write (sync) data can have other index-level permissions.

Sounds like there is a use case for Kibana where they need only read access, so adding a monitor_connector would make sense. @azasypkin might have some more context if needed.

I was tempted to use search to indicate the search org owning this privilege

In general we should avoid naming privileges based on our internal org structure since it might change and it can confuse users.

Actually we might want to exclude some secrets-related patterns from that privilege, do I understand correctly that we can use excludePatterns for that, see my #110128 (comment).

After thinking more about this I think the expectation on the manage privilege from a user is to include the secret rest actions too.

---

I brought this PR up in todays Elasticsearch security team meeting and we have some concerns around reading and writing secrets through the API. In general we should not write or read secrets through the API, but instead use the elasticsearch-keystore and store the secrets in secure settings.

I can see that the read and write secrets are used to store api keys needed by the connectors and it might not be possible to build the functionality you're trying to build using the keystore. Is using the keystore something you considered?

CC: @bytebilly

[jedrazb]

Hi @jfreden thank you for bringing this topic up in the Elasticsearch security team meeting

Sounds like there is a use case for Kibana where they need only read access, so adding a monitor_connector would make sense.

Sure, we can include monitor_connector privilege alongside manage_connector, to allow for read-only access.

After thinking more about this I think the expectation on the manage privilege from a user is to include the secret rest actions too.

This case is tricky. We don't necessarily expect that a user/service that has manage_connector privilege should have access to reading secrets. The "read secret" privilege is only used by Enterprise Search system account to enable native (running in cloud) connectors writing data to non search- prefixed indices.

How strongly are you feeling that manage_connector should include secret (get) rest actions?

I brought this PR up in todays Elasticsearch security team meeting and we have some concerns around reading and writing secrets through the API. In general we should not write or read secrets through the API, but instead use the elasticsearch-keystore and store the secrets in secure settings.

I don't have the full context about this decision. IIRC we followed the pattern established by Fleet (see read_fleet_secrets and write_fleet_secrets). I can share more context from the implementation doc in Slack. I'm not sure if using elasticsearch-keystore was considered. I could tag some folks with more context if this doesn't answer your question. Let me know!

[jedrazb]

Hey @jfreden we introduced monitor_connector privilege with read-only access in af27b4e and Kibana system role will use it for telemetry 9410537.

Let me know if you have any other concerns with this change?

[jfreden]

Thanks for adding that @jedrazb and thanks for the added context!

Even though this change is not related to the secrets, I feel hesitant to approving it until I better understand this part:

I don't have the full context about this decision. IIRC we followed the pattern established by Fleet (see read_fleet_secrets and write_fleet_secrets). I can share more context from the implementation doc in Slack. I'm not sure if using elasticsearch-keystore was considered. I could tag some folks with more context if this doesn't answer your question. Let me know!

The security team is concerned about how the Fleet secrets index was handled and we don't have any recommendation on how this should ideally be built. I'm going to sync with the rest of the team and get back to you as soon as I can, probably early next week. I understand this is time critical and will try to move as fast as possible.

[jfreden]

LGTM!

[azasypkin]

question: do I understand correctly this new privilege will eventually replace existing RoleDescriptor.IndicesPrivileges.builder().indices(".elastic-connectors*").privileges("read") index privilege? If so, do we have any issue tracking this cleanup work to maintain the kibana_system in good and current shape?

[seanstory]

You do understand that correctly. We'll be moving .elastic-connectors* to be system indices, so I'm pretty sure we'll be forced to clean this up.

[azasypkin]

You do understand that correctly. We'll be moving .elastic-connectors* to be system indices,

Okay, thanks for confirming.

so I'm pretty sure we'll be forced to clean this up.

I hope so, but I'm not sure. Let's record this somewhere so we don't forget (context: we're trying really hard to contain what kibana_system is allowed to do).

[jfreden]

Sorry to go back and forth on this but I misunderstood some of the feedback from the rest of the team.

The manage_connector_state name is not aligned with other privileges we have and manage_connector is implicitly expected to include secrets to align with other manage_* privileges.

Adding one of these privileges is a big deal since we can't just change them once they're added for backwards compatibility reasons. To add manage_connector (with secrets management) we need to first align on a security model. We do not feel comfortable with adding a privilege that could result in users storing unencrypted secrets in Elasticsearch.

[jfreden]

LGTM! Thanks for addressing and discussing our concerns!

[azasypkin]

Changes in x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/store/KibanaOwnedReservedRoleDescriptors.java LGTM (not expanding Kibana System privileges as equivalent privileges were already granted).