-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
Create manage_connector
privilege
#110128
base: main
Are you sure you want to change the base?
Conversation
Documentation preview: |
Pinging @elastic/es-security (Team:Security) |
Pinging @elastic/ent-search-eng (Team:SearchOrg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@@ -92,6 +92,7 @@ A successful call returns an object with "cluster", "index", and "remote_cluster | |||
"manage_rollup", | |||
"manage_saml", | |||
"manage_search_application", | |||
"manage_search_connector", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI this will be in docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
# This is fragile - it needs to be updated every time we add a new cluster/index privilege | ||
# I would much prefer we could just check that specific entries are in the array, but we don't have | ||
# an assertion for that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -174,6 +174,7 @@ public class ClusterPrivilegeResolver { | |||
); | |||
|
|||
private static final Set<String> MANAGE_SEARCH_APPLICATION_PATTERN = Set.of("cluster:admin/xpack/application/search_application/*"); | |||
private static final Set<String> MANAGE_SEARCH_CONNECTOR_PATTERN = Set.of("cluster:admin/xpack/connector/*"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one side effect is that manage_search_connector
will include the read_connector_secret
and write_connector_secret
, but that is the intention right?
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" | |
) | |
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
My understanding is that the new privilege isn't currently used by anything, but implicitly grants Do you think splitting it into more granular privileges might make sense and is there a use case for that? Such as
Thank you! |
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. |
Hey @jfreden thank you for reviewing. Answering your questions:
Yes, you got it right :) The story is that:
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
Actually we might want to exclude some secrets-related patterns from that privilege, do I understand correctly that we can use
You are right that
I will def reach out to you before merging to clarify exact steps 😅 |
manage_search_connector
privilegemanage_connector
privilege
… secrets patterns from this privilege
Thanks @jedrazb!
Sounds like there is a use case for Kibana where they need only read access, so adding a
In general we should avoid naming privileges based on our internal org structure since it might change and it can confuse users.
After thinking more about this I think the expectation on the 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 |
Hi @jfreden thank you for bringing this topic up in the Elasticsearch security team meeting
Sure, we can include
This case is tricky. We don't necessarily expect that a user/service that has How strongly are you feeling that
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! |
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:
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. |
Since privilege names are often namespaced and used with globs, we want to ensure that if there's a future privilege like `manage_connector_secrets`, that it is not implicitly included in this new privileg's <name>*. By extending the privilege name to include "_state", we better namespace this distinct from any "_secrets" namespace.
Changes
Define
manage_connector
andmonitor_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:
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)