API Key id part of transport request body #63221
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
albertzaharovits
added
>non-issue
:Security/Audit
|
Pinging @elastic/es-security (:Security/Audit) |
jkakavas
approved these changes
Oct 5, 2020
| @@ -45,13 +49,19 @@ public CreateApiKeyRequest() {} | |||
| * @param expiration to specify expiration for the API key | |||
| */ | |||
| public CreateApiKeyRequest(String name, @Nullable List<RoleDescriptor> roleDescriptors, @Nullable TimeValue expiration) { | |||
| this.id = UUIDs.base64UUID(); | |||
There was a problem hiding this comment.
nit: I would add a comment as it is not obvious why we would generate the id ( in the same way it would be autogenerated as the doc id ) here
There was a problem hiding this comment.
Yes, I'll do that, I wanted to do that, thanks.
ywangd
reviewed
Oct 5, 2020
| @@ -96,11 +97,22 @@ public void testSerialization() throws IOException { | |||
| } | |||
| request.setRoleDescriptors(descriptorList); | |||
|
|
|||
| boolean testV710Bwc = true;// randomBoolean(); | |||
There was a problem hiding this comment.
Aaarg, it should be randomBoolean . Thanks for catching Yang!
|
@elasticmachine update branch |
albertzaharovits
commented
Oct 5, 2020
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java
Outdated
Show resolved
Hide resolved
…ecurity/authc/ApiKeyService.java
|
Can we discuss this please? I think it has wider implications than have been accounted for. |
albertzaharovits
added a commit
that referenced
this pull request
Dec 17, 2020
This adds a new method to the AuditTrail that intercepts the responses of transport-level actions. This new method is unlike all the other existing audit methods because it is called after the action has been run (so that it has access to the response). After careful deliberation, the new method is called for the responses of actions that are intercepted by the `SecurityActionFilter` only, and not by the transport filter. In order to facilitate the "linking" of the new audit event with the other existing events, the audit method receives the requestId as well as the authentication as arguments (in addition to the request itself and the response). This is labeled non-issue because it is only the foundation upon which later features that actually print out (some) responses can be built upon. Related #63221
albertzaharovits
added a commit
to albertzaharovits/elasticsearch
that referenced
this pull request
Dec 17, 2020
This adds a new method to the AuditTrail that intercepts the responses of transport-level actions. This new method is unlike all the other existing audit methods because it is called after the action has been run (so that it has access to the response). After careful deliberation, the new method is called for the responses of actions that are intercepted by the `SecurityActionFilter` only, and not by the transport filter. In order to facilitate the "linking" of the new audit event with the other existing events, the audit method receives the requestId as well as the authentication as arguments (in addition to the request itself and the response). This is labeled non-issue because it is only the foundation upon which later features that actually print out (some) responses can be built upon. Related elastic#63221
albertzaharovits
added a commit
that referenced
this pull request
Dec 17, 2020
This adds a new method to the AuditTrail that intercepts the responses of transport-level actions. This new method is unlike all the other existing audit methods because it is called after the action has been run (so that it has access to the response). After careful deliberation, the new method is called for the responses of actions that are intercepted by the `SecurityActionFilter` only, and not by the transport filter. In order to facilitate the "linking" of the new audit event with the other existing events, the audit method receives the requestId as well as the authentication as arguments (in addition to the request itself and the response). This is labeled non-issue because it is only the foundation upon which later features that actually print out (some) responses can be built upon. Related #63221
ywangd
added a commit
to ywangd/elasticsearch
that referenced
this pull request
Jul 12, 2022
The API key ID generation is handled by the Request class since elastic#63221. This makes it possible to audit it when creating or granting API keys. This PR makes the necessary changes for it to happen. Relates: elastic#63221
ywangd
added a commit
that referenced
this pull request
Jul 12, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When auditing API key creation, it is useful to show the
idof the key together with its other parameters (such asnameandexpiration, etc). Because auditing shows request bodies of security transport actions (see #62916), theidmust be part of the request body for it to be audited.Because the API Key id is the doc id of the key doc in the
.securityindex, this change moves theidgeneration fromelasticsearch/server/src/main/java/org/elasticsearch/action/index/IndexRequest.java
Line 608 in ce649d0