Return file-backed service tokens from all nodes #75200

ywangd · 2021-07-10T07:26:43Z

The Get service account credentials API now returns file-backed tokens from all nodes instead of only the local node. For each file-backed service token, we list names of the nodes where this token is found. The response also has a file_tokens._nodes field containing information about the overall request execution (it works the same way as the _nodes field of Nodes info API, etc.) Detailed response sample can be found in #74530 (comment)

This PR also removes the beta label from the API's documentation page.

Resolves: #74530

elasticmachine · 2021-07-10T07:26:46Z

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

…okens

tvernum

Sorry, I didn't get time to review all of this, but here's my feedback-so-far.

...el/src/main/java/org/elasticsearch/client/security/GetServiceAccountCredentialsResponse.java

tvernum · 2021-07-12T07:53:27Z

...el/src/main/java/org/elasticsearch/client/security/GetServiceAccountCredentialsResponse.java

-        return serviceTokenInfos;
+    public List<ServiceTokenInfo> getTokenInfos() {
+        return Stream.concat(fileTokensResponse.getTokenInfos().stream(), indexTokenInfos.stream())
+            .collect(Collectors.toUnmodifiableList());


I don't object, but it feels like you're trying too hard to restructure the response into the API you would like it to be, rather than represent the JSON API as it is.
We don't have to merge the file and index tokens into 1 list.

I merged them because it felt easier to explain in the HLRC doc page. Now I think it's better to have consistency with the server side response. Updated to keep them separate.

x-pack/docs/en/rest-api/security/get-service-credentials.asciidoc

...g/elasticsearch/xpack/core/security/action/service/GetServiceAccountCredentialsResponse.java

tvernum · 2021-07-12T09:08:18Z

...lugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/service/TokenInfo.java


 import java.io.IOException;
+import java.util.Collection;
 import java.util.Map;
 import java.util.Objects;

 public class TokenInfo implements Writeable, ToXContentObject, Comparable<TokenInfo> {

    private final String name;
    private final TokenSource source;


The source seems weird now - file and index are handled so separately in the response objects, that it feels unnecessary to have this field.

The source can be computed based on the new nodeNames field, which has value null for index-based tokens. So I removed this field, but kept the getter to compute it on the fly.

…security/GetServiceAccountCredentialsResponse.java Co-authored-by: Tim Vernum <[email protected]>

Co-authored-by: Tim Vernum <[email protected]>

…okens

ywangd · 2021-07-30T04:46:13Z

@tvernum I updated the PR based on the discussion with the following small change to response field names:

"nodes_credentials": {
  "_nodes": { ... },
  "file_tokens": {
     "token1": { .. },
     ...
   }
}

The intention is to accomodate possible future expansion for node-local credentials other than file-backed tokens.

...vileges-tests/src/javaRestTest/java/org/elasticsearch/xpack/security/operator/Constants.java

…stTest/java/org/elasticsearch/xpack/security/operator/Constants.java

...lasticsearch/xpack/core/security/action/service/GetServiceAccountNodesCredentialsAction.java

...sticsearch/xpack/core/security/action/service/GetServiceAccountCredentialsResponseTests.java

tvernum · 2021-07-30T05:57:15Z

...rg/elasticsearch/xpack/security/action/service/TransportCreateServiceAccountTokenAction.java

+            listener.onFailure(new IllegalStateException("authentication is required"));
+        } else {
+            serviceAccountService.createIndexToken(authentication, request, listener);
+        }


I don't follow why this change is here (to move things from the actions to the service).

I don't have any opinion on it - I'm just confused about where it came from.
I assume it's a consequence of having the Nodes action that needs to query for index & file tokens separately, but I can't quite piece all the bits together.

You understanding is correct. Please let me fill in some more details.

The logic is not really moved from the "action" to the service, but rather moved from IndexServiceAccountTokenStore to the service. Because of the separation between index-token and file-token fetching, the service becomes more aware of the difference. Previously I tried to make the service not aware of the underlying difference between different stores by having just a single CompositeServiceAccountTokenStore. But this is no long suitable because we have to handle token fetching separately.

Now the service knows explicitly about both stores and can hence become a facade for both of them. So I think it is better have the abstraction layer here since it is already aware of it. That's why I decided to let the actions call into service instead of using the underlying store directly. I think in general I prefer this layer separation, but we are (I am) not consistent about it.

Since this action now uses the service and the service already handles HTTP TLS check, it is also moved into the service. One benefit is that I was able to test it in one place and delete a few tests for the actions.

Co-authored-by: Tim Vernum <[email protected]>

The Get service account credentials API now returns file-backed tokens from all nodes instead of only the local node. For each file-backed service token, we list names of the nodes where this token is found. The response for node-local credentials (currently only file-backed tokens) is place inside the "nodes_credentials.file_tokens" field. There is also a nodes_credentials._nodes field containing information about the overall request execution (it works the same way as the _nodes field of Nodes info API, etc.) Detailed response sample can be found in elastic#74530 This PR also removes the beta label from the API's documentation page. Resolves: elastic#74530

The Get service account credentials API now returns file-backed tokens from all nodes instead of only the local node. For each file-backed service token, we list names of the nodes where this token is found. The response for node-local credentials (currently only file-backed tokens) is place inside the "nodes_credentials.file_tokens" field. There is also a nodes_credentials._nodes field containing information about the overall request execution (it works the same way as the _nodes field of Nodes info API, etc.) Detailed response sample can be found in #74530 This PR also removes the beta label from the API's documentation page. Resolves: #74530

The Get service account credentials API now returns file-backed tokens from all nodes instead of only the local node. For each file-backed service token, we list names of the nodes where this token is found. The response for node-local credentials (currently only file-backed tokens) is place inside the "nodes_credentials.file_tokens" field. There is also a nodes_credentials._nodes field containing information about the overall request execution (it works the same way as the _nodes field of Nodes info API, etc.) Detailed response sample can be found in elastic#74530 This PR also removes the beta label from the API's documentation page. Resolves: elastic#74530

ywangd added 5 commits July 8, 2021 16:13

wip

ca4ad2d

fix tests

735b1d7

fix client, yml test and docs

ac35992

fix tests

63e2ab3

tweak

a9585a2

ywangd added >enhancement :Security/Security Security issues without another label v8.0.0 v7.15.0 labels Jul 10, 2021

ywangd requested a review from tvernum July 10, 2021 07:26

elasticmachine added the Team:Security Meta label for security team label Jul 10, 2021

ywangd added 3 commits July 10, 2021 17:35

Merge remote-tracking branch 'origin/master' into es-74530-all-file-t…

68aef9c

…okens

fix yaml tests

9f95cb1

temporarily disable rest compat for the API

c56332e

ywangd force-pushed the es-74530-all-file-tokens branch from 19dbeae to c56332e Compare July 11, 2021 11:40

ywangd mentioned this pull request Jul 11, 2021

Get service account credentials should show file tokens from all nodes #74530

Closed

tvernum reviewed Jul 12, 2021

View reviewed changes

ywangd and others added 5 commits July 13, 2021 22:35

Update client/rest-high-level/src/main/java/org/elasticsearch/client/…

4e074e6

…security/GetServiceAccountCredentialsResponse.java Co-authored-by: Tim Vernum <[email protected]>

Update x-pack/docs/en/rest-api/security/get-service-credentials.asciidoc

64bf0f5

Co-authored-by: Tim Vernum <[email protected]>

address feedback

3a5a718

fix test

155ed0a

Merge remote-tracking branch 'origin/master' into es-74530-all-file-t…

94b7b90

…okens

ywangd force-pushed the es-74530-all-file-tokens branch from e789bb5 to 94b7b90 Compare July 13, 2021 13:14

ywangd added 4 commits July 13, 2021 23:18

checkstyle

9dace76

Merge remote-tracking branch 'origin/master' into es-74530-all-file-t…

8c8f5cd

…okens

Refactor for latest design changes

2425ac9

fix tests

a94b649

ywangd requested a review from tvernum July 30, 2021 04:43

ywangd added 2 commits July 30, 2021 14:48

checkstyle

7855520

more checkstyle

db9b1e4

ywangd commented Jul 30, 2021

View reviewed changes

...vileges-tests/src/javaRestTest/java/org/elasticsearch/xpack/security/operator/Constants.java Outdated Show resolved Hide resolved

Update x-pack/plugin/security/qa/operator-privileges-tests/src/javaRe…

df8a5ae

…stTest/java/org/elasticsearch/xpack/security/operator/Constants.java

tvernum reviewed Jul 30, 2021

View reviewed changes

Apply suggestions from code review

14a013e

Co-authored-by: Tim Vernum <[email protected]>

tvernum approved these changes Jul 30, 2021

View reviewed changes

ywangd merged commit e4f7132 into elastic:master Aug 3, 2021

mark-vieira added v8.0.0-alpha1 and removed v8.0.0 labels Aug 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return file-backed service tokens from all nodes #75200

Return file-backed service tokens from all nodes #75200

ywangd commented Jul 10, 2021

elasticmachine commented Jul 10, 2021

tvernum left a comment

tvernum Jul 12, 2021

ywangd Jul 13, 2021

tvernum Jul 12, 2021

ywangd Jul 13, 2021

ywangd commented Jul 30, 2021

tvernum Jul 30, 2021

ywangd Jul 30, 2021

Return file-backed service tokens from all nodes #75200

Return file-backed service tokens from all nodes #75200

Conversation

ywangd commented Jul 10, 2021

elasticmachine commented Jul 10, 2021

tvernum left a comment

Choose a reason for hiding this comment

tvernum Jul 12, 2021

Choose a reason for hiding this comment

ywangd Jul 13, 2021

Choose a reason for hiding this comment

tvernum Jul 12, 2021

Choose a reason for hiding this comment

ywangd Jul 13, 2021

Choose a reason for hiding this comment

ywangd commented Jul 30, 2021

tvernum Jul 30, 2021

Choose a reason for hiding this comment

ywangd Jul 30, 2021

Choose a reason for hiding this comment