Expose the invalidation_time field in Get/Query ApiKey APIs #102472

jfreden · 2023-11-22T13:28:18Z

This PR exposes invalidation_time in responses from GetApiKey and QueryApiKey API
To address the first half of #92404.

As a follow up to this PR invalidation_time needs to be supported as a valid query value for the QueryApiKey API. There will also be a separate PR to add it to the API docs for GetApiKey and QueryApiKey.

elasticsearchmachine · 2023-11-22T13:29:41Z

Hi @jfreden, I've created a changelog YAML for you.

n1v0lg

Looking great 👍 discussed a few tweaks on Zoom, will re-review afterwards

elasticsearchmachine · 2023-11-23T13:30:10Z

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

jfreden · 2023-11-23T13:37:17Z

@elasticmachine update branch

n1v0lg

LGTM 🚀 Great first PR!

A couple of (not strongly held) nits and smaller tweaks but nothing that needs another round of review.

One easy test coverage boost suggestion:

I'd add an assertion on expiration to this IT here -- that way we also cover the Query API with an integration test.

n1v0lg · 2023-11-23T13:38:32Z

docs/changelog/102472.yaml

@@ -0,0 +1,5 @@
+pr: 102472
+summary: Expose the `invalidation_time` field in Get/Query `ApiKey` APIs
+area: Client


I think Security/Security or Security/Authentication are more appropriate labels since API keys are primarily authentication primitives. If you update the label on the PR, I believe the changelog should get updated automatically.

n1v0lg · 2023-11-23T13:39:51Z

docs/changelog/102472.yaml

@@ -0,0 +1,5 @@
+pr: 102472
+summary: Expose the `invalidation_time` field in Get/Query `ApiKey` APIs


Nit: I'd go with invalidation here as well as in ApiKey::toXContent since that's the user-facing part. I'm added in-line suggestions where I've come across it but may have missed a few parts.

n1v0lg · 2023-11-23T13:41:06Z

...ck/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/apikey/ApiKey.java

@@ -107,6 +108,7 @@ public ApiKey(
        Instant creation,
        Instant expiration,
        boolean invalidated,
+        Instant invalidation,


Nit: would mark @Nullable

n1v0lg · 2023-11-23T13:41:58Z

...ck/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/apikey/ApiKey.java

-            .field("metadata", (metadata == null ? Map.of() : metadata));
+        builder.field("invalidated", invalidated);
+        if (invalidation != null) {
+            builder.field("invalidation_time", invalidation.toEpochMilli());


Suggested change

builder.field("invalidation_time", invalidation.toEpochMilli());

builder.field("invalidation", invalidation.toEpochMilli());

n1v0lg · 2023-11-23T13:42:58Z

...ck/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/apikey/ApiKey.java

@@ -385,6 +419,7 @@ public boolean equals(Object obj) {
        PARSER.declareLong(constructorArg(), new ParseField("creation"));
        PARSER.declareLong(optionalConstructorArg(), new ParseField("expiration"));
        PARSER.declareBoolean(constructorArg(), new ParseField("invalidated"));
+        PARSER.declareLong(optionalConstructorArg(), new ParseField("invalidation_time"));


Suggested change

PARSER.declareLong(optionalConstructorArg(), new ParseField("invalidation_time"));

PARSER.declareLong(optionalConstructorArg(), new ParseField("invalidation"));

n1v0lg · 2023-11-23T13:44:58Z

.../test/java/org/elasticsearch/xpack/core/security/action/apikey/ApiKeySerializationTests.java

+import static org.elasticsearch.xpack.core.security.action.apikey.ApiKeyTests.randomApiKeyInstance;
+import static org.hamcrest.Matchers.nullValue;
+
+public class ApiKeySerializationTests extends AbstractWireSerializingTestCase<ApiKey> {


n1v0lg · 2023-11-23T13:52:09Z

...rc/test/java/org/elasticsearch/xpack/core/security/action/apikey/GetApiKeyResponseTests.java

@@ -192,6 +197,7 @@ public void testToXContent() throws IOException {
                  "creation": 100000,
                  "expiration": 10000000,
                  "invalidated": true,
+                  "invalidation_time": 100000000,


Suggested change

"invalidation_time": 100000000,

"invalidation": 100000000,

n1v0lg · 2023-11-23T13:53:29Z

...ty-trial/src/javaRestTest/java/org/elasticsearch/xpack/security/apikey/GetApiKeysRestIT.java

    private static final String MANAGE_SECURITY_USER = "manage_security_user";

    @Before
    public void createUsers() throws IOException {
        createUser(MANAGE_OWN_API_KEY_USER, END_USER_PASSWORD, List.of("manage_own_api_key_role"));
        createRole("manage_own_api_key_role", Set.of("manage_own_api_key"));
+        createUser(MANAGE_API_KEY_USER, END_USER_PASSWORD, List.of("manage_api_key_role"));


Nit: I don't think we need the new role and user -- either of the existing ones should work here (for sure MANAGE_SECURITY_USER).

n1v0lg · 2023-11-23T13:57:36Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java

@@ -1677,10 +1677,10 @@ private void indexInvalidation(Collection<String> apiKeyIds, ActionListener<Inva
            listener.onFailure(new ElasticsearchSecurityException("No api key ids provided for invalidation"));
        } else {
            BulkRequestBuilder bulkRequestBuilder = client.prepareBulk();
-            final long invalidationTime = clock.instant().toEpochMilli();
+            final long invalidation = clock.instant().toEpochMilli();


Nit: sorry, I forgot to mention this bit: I'd stick with invalidationTime here since it's a different context. Here (i.e., in ApiKeyService), we do refer to the time values with a time suffix, e.g., creationTime, expirationTime.

n1v0lg · 2023-11-23T14:06:31Z

...plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ApiKeyServiceTests.java

@@ -1024,7 +1020,7 @@ public void testValidateApiKey() throws Exception {
        assertFalse(result.isAuthenticated());

        // key is invalidated
-        apiKeyDoc = buildApiKeyDoc(hash, -1, true);
+        apiKeyDoc = buildApiKeyDoc(hash, -1, true, -1);


Could randomize a timestamp here since the key is invalidated (it's not the primary purpose of the test but might as well add that coverage).

This is a follow up PR from #102472. This adds the ability to use `invalidation` timestamp as a valid [query value](https://www.elastic.co/guide/en/elasticsearch/reference/current/security-api-query-api-key.html#security-api-query-api-key-request-body) in the QueryApiKey API.

…102472) This PR exposes `invalidation_time` in responses from [GetApiKey](https://www.elastic.co/guide/en/elasticsearch/reference/current/security-api-get-api-key.html) and [QueryApiKey](https://www.elastic.co/guide/en/elasticsearch/reference/current/security-api-query-api-key.html) API To address the first half of elastic#92404. As a follow up to this PR `invalidation_time` needs to be supported as a valid [query value](https://www.elastic.co/guide/en/elasticsearch/reference/current/security-api-query-api-key.html#security-api-query-api-key-request-body) for the `QueryApiKey` API. There will also be a separate PR to add it to the API docs for [GetApiKey](https://www.elastic.co/guide/en/elasticsearch/reference/current/security-api-get-api-key.html) and [QueryApiKey](https://www.elastic.co/guide/en/elasticsearch/reference/current/security-api-query-api-key.html).

This is a follow up PR from elastic#102472. This adds the ability to use `invalidation` timestamp as a valid [query value](https://www.elastic.co/guide/en/elasticsearch/reference/current/security-api-query-api-key.html#security-api-query-api-key-request-body) in the QueryApiKey API.

jfreden added >enhancement :Security/Client Security in clients (Transport, Rest) labels Nov 22, 2023

elasticsearchmachine added v8.12.0 external-contributor Pull request authored by a developer outside the Elasticsearch team labels Nov 22, 2023

n1v0lg removed the external-contributor Pull request authored by a developer outside the Elasticsearch team label Nov 22, 2023

n1v0lg self-requested a review November 22, 2023 13:32

n1v0lg reviewed Nov 22, 2023

View reviewed changes

jfreden marked this pull request as ready for review November 23, 2023 13:29

jfreden force-pushed the expose-invalidation_time-apikey-apis branch from f7127ba to d23dc60 Compare November 23, 2023 13:29

elasticsearchmachine added the Team:Security Meta label for security team label Nov 23, 2023

n1v0lg approved these changes Nov 23, 2023

View reviewed changes

jfreden added :Security/Security Security issues without another label and removed :Security/Client Security in clients (Transport, Rest) labels Nov 23, 2023

jfreden added 3 commits November 24, 2023 10:19

Update get api_key to return invalidation_time

bb395ea

fixup! Code review comments

0a14038

fixup! Failing CI due to bug in match constant

760cade

jfreden added auto-merge Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) test-update-serverless labels Nov 24, 2023

jfreden force-pushed the expose-invalidation_time-apikey-apis branch from 51c88a8 to 760cade Compare November 24, 2023 09:20

elasticsearchmachine merged commit f3d5a65 into elastic:main Nov 24, 2023
19 checks passed

jfreden deleted the expose-invalidation_time-apikey-apis branch November 24, 2023 11:31

jfreden mentioned this pull request Nov 24, 2023

Add support for invalidation timestamp in QueryApiKey #102590

Merged

jfreden mentioned this pull request Nov 27, 2023

Expose the invalidation_time field in Get/Query ApiKey APIs #92404

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose the invalidation_time field in Get/Query ApiKey APIs #102472

Expose the invalidation_time field in Get/Query ApiKey APIs #102472

jfreden commented Nov 22, 2023

elasticsearchmachine commented Nov 22, 2023

n1v0lg left a comment

elasticsearchmachine commented Nov 23, 2023

jfreden commented Nov 23, 2023

n1v0lg left a comment

n1v0lg Nov 23, 2023

n1v0lg Nov 23, 2023 •

edited

Loading

n1v0lg Nov 23, 2023

n1v0lg Nov 23, 2023

n1v0lg Nov 23, 2023

n1v0lg Nov 23, 2023

n1v0lg Nov 23, 2023

n1v0lg Nov 23, 2023

n1v0lg Nov 23, 2023 •

edited

Loading

n1v0lg Nov 23, 2023

		@@ -0,0 +1,5 @@
		pr: 102472
		summary: Expose the `invalidation_time` field in Get/Query `ApiKey` APIs

	builder.field("invalidation_time", invalidation.toEpochMilli());
	builder.field("invalidation", invalidation.toEpochMilli());

	PARSER.declareLong(optionalConstructorArg(), new ParseField("invalidation_time"));
	PARSER.declareLong(optionalConstructorArg(), new ParseField("invalidation"));

Expose the invalidation_time field in Get/Query ApiKey APIs #102472

Expose the invalidation_time field in Get/Query ApiKey APIs #102472

Conversation

jfreden commented Nov 22, 2023

elasticsearchmachine commented Nov 22, 2023

n1v0lg left a comment

Choose a reason for hiding this comment

elasticsearchmachine commented Nov 23, 2023

jfreden commented Nov 23, 2023

n1v0lg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n1v0lg Nov 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n1v0lg Nov 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n1v0lg Nov 23, 2023 •

edited

Loading

n1v0lg Nov 23, 2023 •

edited

Loading