Skip to content

Handle API key type in Get/Query API key responses#96014

Merged
elasticsearchmachine merged 8 commits intoelastic:mainfrom
ywangd:rcs-get-special-api-keys
May 15, 2023
Merged

Handle API key type in Get/Query API key responses#96014
elasticsearchmachine merged 8 commits intoelastic:mainfrom
ywangd:rcs-get-special-api-keys

Conversation

@ywangd
Copy link
Member

@ywangd ywangd commented May 11, 2023

This PR makes the Get/Query API key APIs to return the new API key type information in their responses. For cross-cluster type API keys, the limited-by field is not shown because they do not use the limited-by model as existing API keys.

Relates: #95714

This PR makes the Get/Query API key APIs to return the new API key type
information in their responses. For cross-cluster type API keys, the
limited-by field is not shown because they do not use the limited-by
model as existing API keys.

Relates: elastic#95714
@ywangd ywangd added >non-issue :Security/Security Security issues without another label v8.9.0 labels May 11, 2023
Comment on lines +244 to +246
if (TcpTransport.isUntrustedRemoteClusterEnabled()) {
builder.field("type", type.value());
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This PR has some unfortunate complexity in tests due to this conditional serialization logic with featureFlag checking. A simple solution could be adding assumeTrue(featureFlag) for all those tests. But it felt disabling a bit too much. So I took a more finer controlled approach.

Luckily Doc tests do not actually assert Get/Query responses and Yaml tests only assert selective fields. So they need no change in this PR.

this.name = in.readString();
}
this.id = in.readString();
if (in.getTransportVersion().onOrAfter(TRANSPORT_VERSION_ADVANCED_REMOTE_CLUSTER_SECURITY_CCR)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

We may need a new TransportVersion for this given the recent changes to versioning. I am reaching out to the experts for confirmation.

Copy link
Member Author

Choose a reason for hiding this comment

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

A new TransportVersion is added f3c58f7

@ywangd ywangd marked this pull request as ready for review May 11, 2023 05:13
@ywangd ywangd requested a review from n1v0lg May 11, 2023 05:13
@elasticsearchmachine elasticsearchmachine added the Team:Security Meta label for security team label May 11, 2023
@elasticsearchmachine
Copy link
Collaborator

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

@ywangd
Copy link
Member Author

ywangd commented May 12, 2023

@elasticmachine run elasticsearch-ci/part-1-fips

Copy link
Contributor

@n1v0lg n1v0lg left a comment

Choose a reason for hiding this comment

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

LGTM!

As a side note, if we ever consider filtering by type, we may be able to add validation that with_limited_by is not used with type=CROSS_CLUSTER since that's arguably an invalid request. It's not really possible to do this for general Get and Query requests though since those can apply to a mix or REST and CROSS_CLUSTER API keys.

@ywangd
Copy link
Member Author

ywangd commented May 14, 2023

As a side note, if we ever consider filtering by type, we may be able to add validation that with_limited_by is not used with type=CROSS_CLUSTER since that's arguably an invalid request.

Thanks for this idea but I prefer we don't do it. In my opinion, not showing limited_by in the response (effectively null) is a reasonable behaviour for cross_cluster API keys. It means these API keys do not have limited_by roles. As an analogy, we don't error for REST API keys that have no assigned roles. For historical reasons, we show them as empty. But it is not more correct than not showing them at all. It also feels user unfriendly to throw error when the query filters on particular field (type), value (cross_cluster) and requests another particular field (limited_by). It feels better that query parameters are simply additive and not blocking each other. The fields in a search request seems to be a parallel here: we don't error on unknown fields, they are just ignored in response. Technically it is also challenging to implement because there are many ways the type field can be queried and the query field can appear in arbitrary nested boolean clauses.

@ywangd
Copy link
Member Author

ywangd commented May 14, 2023

@elasticmachine update branch

@ywangd ywangd added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label May 14, 2023
@ywangd
Copy link
Member Author

ywangd commented May 15, 2023

@elasticmachine run elasticsearch-ci/part-1

@ywangd
Copy link
Member Author

ywangd commented May 15, 2023

Failure is not related. I raised #96084

@ywangd
Copy link
Member Author

ywangd commented May 15, 2023

@elasticmachine run elasticsearch-ci/part-1

@elasticsearchmachine elasticsearchmachine merged commit f01f07f into elastic:main May 15, 2023
@ywangd ywangd deleted the rcs-get-special-api-keys branch May 15, 2023 02:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) >non-issue :Security/Security Security issues without another label Team:Security Meta label for security team v8.9.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants