Nodes reload request store password as SecureString #31261

albertzaharovits · 2018-06-12T08:11:13Z

This improvement was suggested by @jaymode in a comment in another review.

The node request, NodesReloadSecureSettingsRequest, broadcasting the password required to locally decrypt the keystore on each node, stores the password in a SecureString request field and not a plain String field.

Relates: #29135
CC @elastic/es-security

elasticmachine · 2018-06-12T08:11:15Z

Pinging @elastic/es-core-infra

albertzaharovits · 2018-06-12T08:42:43Z

...ava/org/elasticsearch/action/admin/cluster/node/reload/NodesReloadSecureSettingsRequest.java

+        final byte[] bytes = Arrays.copyOfRange(byteBuffer.array(), byteBuffer.position(), byteBuffer.limit());
+        Arrays.fill(byteBuffer.array(), (byte) 0); // clear sensitive data
+        return bytes;
+    }


Copied from:

elasticsearch/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/support/CharArrays.java

Line 43 in 113c191

public static byte[] toUtf8Bytes(char[] chars) {

Alternatively, we could pull the CharArrays utils class out of x-pack. Yet this is the only use case in server. Moreover, CharArrays brings a methods that is narrowed to the security scope, namely constantTimeEquals.
Another alternative would be to have the utf8BytesToChars and toUtf8Bytes as static members of SecureString since right now uses are tied to SecureString.

All in all, I think copying them here for this sole use is the best alternative.

Another alternative would be to have the utf8BytesToChars and toUtf8Bytes as static members of SecureString since right now uses are tied to SecureString.

I actually think this might be the best option but we should do this separately in a followup.

albertzaharovits · 2018-06-12T08:49:22Z

...ava/org/elasticsearch/action/admin/cluster/node/reload/NodesReloadSecureSettingsRequest.java

+        final CharBuffer charBuffer = StandardCharsets.UTF_8.decode(byteBuffer);
+        final char[] chars = Arrays.copyOfRange(charBuffer.array(), charBuffer.position(), charBuffer.limit());
+        Arrays.fill(charBuffer.array(), (char) 0); // clear sensitive data
+        return chars;


Copied from:

elasticsearch/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/support/CharArrays.java

Line 18 in 113c191

public static char[] utf8BytesToChars(byte[] utf8Bytes) {

See previous comment .

Some point worth raising here. In the original implementation (CharArray#utf8BytesToChars) the decoded CharBuffer is not cleared. It is charBuffer.clear() but should be Arrays.fill(charBuffer.array(), (char) 0);.
@jaymode Should I raise a PR to fix this?

I think it is worth raising a separate PR to fix this and move the methods as I said before. I also think that this method needs to handle some other cases such as my suggestion above about the hasArray() and other items.

jaymode

I left some ideas for improvements. Also I wonder if it is possible to clear the secureSettingsPassword after the request has been sent to all the nodes?

jaymode · 2018-06-13T14:55:22Z

...ava/org/elasticsearch/action/admin/cluster/node/reload/NodesReloadSecureSettingsRequest.java

+        final byte[] bytes = Arrays.copyOfRange(byteBuffer.array(), byteBuffer.position(), byteBuffer.limit());
+        Arrays.fill(byteBuffer.array(), (byte) 0); // clear sensitive data
+        return bytes;
+    }


Another alternative would be to have the utf8BytesToChars and toUtf8Bytes as static members of SecureString since right now uses are tied to SecureString.

I actually think this might be the best option but we should do this separately in a followup.

jaymode · 2018-06-13T15:12:19Z

...ava/org/elasticsearch/action/admin/cluster/node/reload/NodesReloadSecureSettingsRequest.java

+    private static byte[] toUtf8Bytes(char[] chars) {
+        final CharBuffer charBuffer = CharBuffer.wrap(chars);
+        final ByteBuffer byteBuffer = StandardCharsets.UTF_8.encode(charBuffer);
+        final byte[] bytes = Arrays.copyOfRange(byteBuffer.array(), byteBuffer.position(), byteBuffer.limit());


This code is dangerous so I think we should correct it. The char buffer is guaranteed to be a HeapCharBuffer by the javadocs; however there is no such guarantee about the ByteBuffer returned by StandardCharsets.UTF_8.encode(charBuffer). We need to account for whether the bytebuffer has an array that can be accessed or not.

final byte[] bytes; if (byteBuffer.hasArray()) { // there is no guarantee that the byte buffers backing array is the right size so we need to make a copy bytes = Arrays.copyOfRange(byteBuffer.array(), byteBuffer.position(), byteBuffer.limit()); Arrays.fill(byteBuffer.array(), (byte) 0); // clear sensitive data } else { final int length = byteBuffer.limit() - byteBuffer.position(); bytes = new byte[length]; byteBuffer.get(bytes); // if the buffer is not read only we can reset and fill with 0's if (byteBuffer.isReadOnly() == false) { byteBuffer.clear(); // reset for (int i = 0; i < byteBuffer.limit(); i++) { byteBuffer.put((byte) 0); } } }

Thanks for the detailed snippet.

jaymode · 2018-06-13T15:18:57Z

...ava/org/elasticsearch/action/admin/cluster/node/reload/NodesReloadSecureSettingsRequest.java

+        final CharBuffer charBuffer = StandardCharsets.UTF_8.decode(byteBuffer);
+        final char[] chars = Arrays.copyOfRange(charBuffer.array(), charBuffer.position(), charBuffer.limit());
+        Arrays.fill(charBuffer.array(), (char) 0); // clear sensitive data
+        return chars;


I think it is worth raising a separate PR to fix this and move the methods as I said before. I also think that this method needs to handle some other cases such as my suggestion above about the hasArray() and other items.

albertzaharovits · 2018-06-14T17:18:36Z

Thank you for the feedback @jaymode .
I will open a follow-up PR to move the NodesReloadSecureSettingsRequest#charsToUtf8Bytes and NodeesReloadSecureSettingsRequest#utf8BytesToChars to SecureString after the feature branch is merged in.

There are two points worth bringing up after addressing the feedback, which I have pointed in the code.

albertzaharovits · 2018-06-14T17:21:21Z

.../elasticsearch/action/admin/cluster/node/reload/NodesReloadSecureSettingsRequestBuilder.java

+        }
+        return this;
+    }
+


1/2: Since right now the keystore only uses the empty password, the previous implementation was sloppy and actually read the password as a REST parameter. This is terrible and has been corrected. It is now read from the request body.

We're going to need to figure out a way to filter this in the security auditing. In x-pack we introduced the concept of a RestRequestFilter. This concept might need to be promoted to core for this API.

albertzaharovits · 2018-06-14T17:23:43Z

...elasticsearch/action/admin/cluster/node/reload/TransportNodesReloadSecureSettingsAction.java

@@ -82,15 +83,15 @@ protected NodeRequest newNodeRequest(String nodeId, NodesReloadSecureSettingsReq
    protected NodesReloadSecureSettingsResponse.NodeResponse nodeOperation(NodeRequest nodeReloadRequest) {
        final NodesReloadSecureSettingsRequest request = nodeReloadRequest.request;
        KeyStoreWrapper keystore = null;
-        try {
+        try (final SecureString secureSettingsPassword = request.secureSettingsPassword()) {


2/2 (1): The password is cleared on each NodeRequest.

albertzaharovits · 2018-06-14T17:39:38Z

...rc/main/java/org/elasticsearch/rest/action/admin/cluster/RestReloadSecureSettingsAction.java

@@ -68,7 +72,8 @@ public RestResponse buildResponse(NodesReloadSecureSettingsResponse response, XC
                        builder.field("cluster_name", response.getClusterName().value());
                        response.toXContent(builder, channel.request());
                        builder.endObject();
-
+                        // clear password for the original request
+                        nodesRequest.secureSettingsPassword().close();


2/2 (2): The password is also cleared on the original NodesRequest.
The way everything fits together is as follows: The NodeSRequest has the password which has been read from the REST body. The password is stored as a SecureString. It is passed as a reference to all the constructed NodeRequests.

public static class NodeRequest extends BaseNodeRequest { NodesReloadSecureSettingsRequest request; public NodeRequest() { } NodeRequest(String nodeId, NodesReloadSecureSettingsRequest request) { super(nodeId); this.request = request; } @Override public void readFrom(StreamInput in) throws IOException { super.readFrom(in); request = new NodesReloadSecureSettingsRequest(); request.readFrom(in); } @Override public void writeTo(StreamOutput out) throws IOException { super.writeTo(out); request.writeTo(out); } }

ALL these requests are serialized (including the one for the local node, doing the broadcast) and their password is not cleared. The deserialized NodeRequests will have their password cleared: https://github.com/elastic/elasticsearch/pull/31261/files#r195506031 .
The original password (read from the REST body) which was passed as a reference to all the broadcasted requests, which have been serialized, is cleared here.

All goes down in flames if the password in the NodeRequest for the local node is not copied (through serialize-deserialize) AND it is cleared before other requests are serialized.
Since ALL NodeRequests are serialized this should not happen, right?

I believe everything you've said is correct

jaymode

LGTM

jaymode · 2018-06-14T18:08:27Z

...rc/main/java/org/elasticsearch/rest/action/admin/cluster/RestReloadSecureSettingsAction.java

@@ -68,7 +72,8 @@ public RestResponse buildResponse(NodesReloadSecureSettingsResponse response, XC
                        builder.field("cluster_name", response.getClusterName().value());
                        response.toXContent(builder, channel.request());
                        builder.endObject();
-
+                        // clear password for the original request
+                        nodesRequest.secureSettingsPassword().close();


I believe everything you've said is correct

jaymode · 2018-06-14T18:11:14Z

.../elasticsearch/action/admin/cluster/node/reload/NodesReloadSecureSettingsRequestBuilder.java

+        }
+        return this;
+    }
+


We're going to need to figure out a way to filter this in the security auditing. In x-pack we introduced the concept of a RestRequestFilter. This concept might need to be promoted to core for this API.

bellatrix1001 · 2018-11-09T15:57:01Z

@jaymode How does one actually set this password via POST? Docs have no reference to secure_settings_password. Thanks!!

jaymode · 2018-11-09T17:56:43Z

Password has been removed in a separate issue since we didn't have full support for it, so you cannot set it.

Secure String in Nodes reload request

9e71fa8

albertzaharovits added >non-issue review :Core/Infra/Settings Settings infrastructure and APIs labels Jun 12, 2018

albertzaharovits self-assigned this Jun 12, 2018

albertzaharovits requested a review from jaymode June 12, 2018 08:11

albertzaharovits commented Jun 12, 2018

View reviewed changes

Merge branch 'reload-secure-store-action' into secure-string-request

0172407

jaymode requested changes Jun 13, 2018

View reviewed changes

albertzaharovits added 4 commits June 14, 2018 17:21

Clear password on node action handler

51675f8

Encode and decode arrays

bccff0e

Clear last password

072bbbf

Rename toUtf8Bytes -> charsToUtf8Bytes

28a3305

albertzaharovits commented Jun 14, 2018

View reviewed changes

jaymode approved these changes Jun 14, 2018

View reviewed changes

Merge branch 'reload-secure-store-action' into secure-string-request

f67f1ae

albertzaharovits merged commit d9835d5 into elastic:reload-secure-store-action Jun 16, 2018

albertzaharovits deleted the secure-string-request branch June 16, 2018 19:26

albertzaharovits added a commit that referenced this pull request Jun 16, 2018

Fallout from #31261 in tests

e1a69dc

albertzaharovits mentioned this pull request Jun 17, 2018

Reload secure settings for plugins #31383

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nodes reload request store password as SecureString #31261

Nodes reload request store password as SecureString #31261

albertzaharovits commented Jun 12, 2018

elasticmachine commented Jun 12, 2018

albertzaharovits Jun 12, 2018 •

edited

Loading

jaymode Jun 13, 2018

albertzaharovits Jun 12, 2018

jaymode Jun 13, 2018

jaymode left a comment

jaymode Jun 13, 2018

jaymode Jun 13, 2018

albertzaharovits Jun 14, 2018

jaymode Jun 13, 2018

albertzaharovits commented Jun 14, 2018

albertzaharovits Jun 14, 2018

jaymode Jun 14, 2018

albertzaharovits Jun 14, 2018

albertzaharovits Jun 14, 2018

jaymode Jun 14, 2018

jaymode left a comment

jaymode Jun 14, 2018

jaymode Jun 14, 2018

bellatrix1001 commented Nov 9, 2018

jaymode commented Nov 9, 2018

Nodes reload request store password as SecureString #31261

Nodes reload request store password as SecureString #31261

Conversation

albertzaharovits commented Jun 12, 2018

elasticmachine commented Jun 12, 2018

albertzaharovits Jun 12, 2018 • 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

jaymode left a 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

albertzaharovits commented Jun 14, 2018

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

jaymode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bellatrix1001 commented Nov 9, 2018

jaymode commented Nov 9, 2018

albertzaharovits Jun 12, 2018 •

edited

Loading