-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Support metadata on API keys #70292
Support metadata on API keys #70292
Conversation
Pinging @elastic/es-security (Team:Security) |
client/rest-high-level/src/main/java/org/elasticsearch/client/security/CreateApiKeyRequest.java
Outdated
Show resolved
Hide resolved
client/rest-high-level/src/main/java/org/elasticsearch/client/security/support/ApiKey.java
Outdated
Show resolved
Hide resolved
...est-high-level/src/test/java/org/elasticsearch/client/security/CreateApiKeyRequestTests.java
Outdated
Show resolved
Hide resolved
@Nullable | ||
private final Map<String, Object> metadata; |
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.
Why Nullable
?
Does a null
metadata mean something different to empty?
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.
They should be functionally equivalent. The general approach I took is to let variables represent the underlying data exactly and only present them consistently when it is sent to end user. In this case, ApiKey
's metadata field can be null
or empty map depending on the underlying data (the security index), where the metadata field can be null
. When the API key information is returned to end user, the value of the metadata field will be consolidated to be Map.of()
for both null
and empty map (as shown in ApiKey#toXContent
). That said, I am happy to change it to be an empty map, i.e. consolidation in its constructor.
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.
That's not our typical strategy. Usually we convert null
to empty in the constructor so that objects that are semantically equivalent are also structurally equivalent.
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.
What you said makes sense. I was influenced by the existing expiration
field which can be null
when there is no expiration. But after some more thoughts, I see it is different from the metadata
field because it does not have a meaningful default value other than null
, while metadata has. I also agree that "objects that are semantically equivalent are also structurally equivalent" and that should be a rule to stick to. Thanks.
...gin/core/src/main/java/org/elasticsearch/xpack/core/security/action/CreateApiKeyRequest.java
Show resolved
Hide resolved
List<CreateApiKeyResponse> responses = new ArrayList<>(); | ||
for (int i = 0; i < noOfApiKeys; i++) { | ||
final RoleDescriptor descriptor = new RoleDescriptor("role", clusterPrivileges, null, null); | ||
Client client = client().filterWithHeader(headers); | ||
final Map<String, Object> metadata = ApiKeyTests.randomMetadata(); |
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.
This seems weird. Everything else in this method is provided by the caller, but the metadata is randomly generated and returned.
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.
I thought about letting callers pass it in, but then realised the callers were just going to pass ApiKeyTests.randomMetadata()
anyway because there is no place where the metadata needs to be more specific. So instead of changing every call sites, I just added it here as an easy change.
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 feels like we accumulating debt - we don't really know what we want our test-object-creation strategy to be, so we have a weird mix.
I don't know what the answer is, but I think this is a sign we're doing something wrong.
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.
we don't really know what we want our test-object-creation strategy to be
I think this is true for the metadata
field, at least for now. But it has a reasonable explanation. The metadata currently really does not do much other than being stored literally and retrieved literally. Therefore a simple random is sufficient for existing tests. We will need a better strategy when the metadata is searchable. When that happens, we will need to craft the metadata more specifically because we need to search for specific things. I think this can wait till we start working on the metadata search part.
The existing createApiKeys
methods are also aligned with the above reasoning. We currently have five of them:
createApiKeys(int noOfApiKeys, TimeValue expiration)
createApiKeys(String user, int noOfApiKeys, TimeValue expiration, String... clusterPrivileges)
createApiKeys(String owningUser, String authenticatingUser, int noOfApiKeys, TimeValue expiration, String... clusterPrivileges)
createApiKeys(Map<String, String> headers, int noOfApiKeys, TimeValue expiration, String... clusterPrivileges)
createApiKeys(Map<String, String> headers, int noOfApiKeys, String namePrefix, TimeValue expiration, String... clusterPrivileges)
They are listed roughly in the order of delegation (or increasing complexity), i.e. (1) calls (2), (2) call (3), etc. For the more generic variation, say (1), it basically randomly chooses most of the attributes for an API key, while the more specific variation, (5), gets to specify many of the attributes. It is also where the metadata
is currently randomised because the existing tests do not care about the exact content of metadata. When we start working on the metadata search, I imagine we can add a 6th variation of createApiKeys
so it takes the metadata as an argument, i.e.:
createApiKeys(Map<String, String> headers, int noOfApiKeys, String namePrefix, TimeValue expiration, Map<String, Object> metadata, String... clusterPrivileges)
and current variation (5) will delegate its call to (6).
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.
I think we have a smell - the ApiKeyIntegTests are a bit weirdly stuck together, and would benefit from having some structured thinking about what they're trying to achieve and how best to do that.
I think that's a separate issue though.
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java
Show resolved
Hide resolved
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
This PR adds metadata support for API keys. Metadata are of type Map<String, Object> and can be optionally provided at API key creation time. It is returned as part of GetApiKey response. It is also stored as part of the authentication object to transfer throw the wire. Note that it is not yet searchable and not exposed to any ingest processors. They will be handled by separate PRs.
This PR adds metadata support for API keys. Metadata are of type Map<String, Object> and can be optionally provided at API key creation time. It is returned as part of GetApiKey response. It is also stored as part of the authentication object to transfer throw the wire. Note that it is not yet searchable and not exposed to any ingest processors. They will be handled by separate PRs. Because flattened field type is not available prior to 7.3. This backport also adds a prior SystemIndexDescriptor for the security index so it works in a mixed cluster with nodes older than 7.3. Co-authored-by: Tim Vernum <[email protected]>
This PR adds the missing doc update to the grant api key rest api page for the new API key metadata field added by elastic#70292 Relates: elastic#48182
This PR adds the missing doc update to the grant api key rest api page for the new API key metadata field added by elastic#70292 Relates: elastic#48182
This PR adds the missing doc update to the grant api key rest api page for the new API key metadata field added by #70292 Relates: #48182 Co-authored-by: Yang Wang <[email protected]>
This PR adds metadata support for API keys. Metadata are of type
Map<String, Object>
and can be optionally provided at API key creation time. It is returned as part of GetApiKey response. It is also stored as part of the authentication object. Refer to this comment for detailed feature list.Also updated docs and HLRC along with its tests and docs.
NOTE: Storing the API key metadata in the authentication object makes it possible to be exposed in ingestion processors, e.g.
SetSecurityUserProcessor
. But this PR intentionaly leaves it out because:Doc preview links: https://elasticsearch_70292.docs-preview.app.elstc.co/diff
Resolves: #48182