Skip to content
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,16 @@
fleet.delete_secret:
id: $id

---
"Create Fleet Secret with multiple values":
- do:
fleet.post_secret:
body: '{"value": ["test secret 1", "test secret 2"]}'
- set: { id: id }
- do:
fleet.delete_secret:
id: $id

---
"Create secret fails for unprivileged user":
- skip:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,27 @@
fleet.delete_secret:
id: $id

---
"Get Fleet Secret with multiple values":
- do:
fleet.post_secret:
body: '{"value": ["test secret 1", "test secret 2"]}'
- set: { id: id }
# search node needs to be available for fleet.get_secret to work in stateless.
# The `.fleet-secrets` index is created on demand, and its search replica starts out unassigned,
# so wait_for_no_uninitialized_shards can miss it.
- do:
cluster.health:
wait_for_active_shards: all
- do:
fleet.get_secret:
id: $id
- match: { id: $id }
- match: { value: ["test secret 1", "test secret 2"] }
- do:
fleet.delete_secret:
id: $id

---
"Get non existent Fleet Secret":
- do:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,15 @@
public class GetSecretResponse extends ActionResponse implements ToXContentObject {

private final String id;
private final String value;
private final Object value;

public GetSecretResponse(StreamInput in) throws IOException {
super(in);
id = in.readString();
value = in.readString();
this.id = in.readString();
this.value = in.readString();
Copy link

Copilot AI Mar 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The StreamInput deserialization for 'value' only handles a single String. When a multi-value secret is returned, the stream reading should be updated to properly deserialize a String array.

Suggested change
this.value = in.readString();
if (in.readBoolean()) {
this.value = in.readString();
} else {
this.value = in.readStringArray();
}

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@jen-huang jen-huang Mar 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried this but it makes tests fail with error about unexpected byte

I also tried this.value = in.readGenericValue(); and tests also fail with errors like:
unexpected error expanding serialized delayed writeable
tried to read: 119 bytes but only 9 remaining

(tests can be run with ./gradlew ":x-pack:plugin:fleet:test" --tests "org.elasticsearch.xpack.fleet.action.GetSecretResponseTests.*")

I tried similar for PostSecretResponse as well with the same errors.

but it seems that this constructor is not used by REST endpoints, so it may be safe to leave readString() only here?

}

public GetSecretResponse(String id, String value) {
public GetSecretResponse(String id, Object value) {
this.id = id;
this.value = value;
}
Expand All @@ -40,15 +40,24 @@ public String id() {
@Override
public void writeTo(StreamOutput out) throws IOException {
out.writeString(id);
out.writeString(value);
if (value instanceof String) {
out.writeString((String) value);
} else if (value instanceof String[]) {
out.writeStringArray((String[]) value);
}
}

@Override
public XContentBuilder toXContent(XContentBuilder builder, ToXContent.Params params) throws IOException {
builder.startObject();
builder.field("id", id);
builder.field("value", value);
return builder.endObject();
if (value instanceof String) {
builder.field("value", (String) value);
} else if (value instanceof String[]) {
builder.field("value", (String[]) value);
}
builder.endObject();
return builder;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import org.elasticsearch.xcontent.ParseField;
import org.elasticsearch.xcontent.XContentBuilder;
import org.elasticsearch.xcontent.XContentParser;
import org.elasticsearch.xcontent.XContentParser.Token;

import java.io.IOException;
import java.util.Objects;
Expand All @@ -27,26 +28,30 @@ public class PostSecretRequest extends ActionRequest {
public static final ConstructingObjectParser<PostSecretRequest, Void> PARSER = new ConstructingObjectParser<>(
"post_secret_request",
args -> {
return new PostSecretRequest((String) args[0]);
return new PostSecretRequest(args[0]);
}
);

static {
PARSER.declareField(
ConstructingObjectParser.optionalConstructorArg(),
(p, c) -> p.text(),
VALUE_FIELD,
ObjectParser.ValueType.STRING
);
PARSER.declareField(ConstructingObjectParser.optionalConstructorArg(), (p, c) -> {
Token token = p.currentToken();
if (token == XContentParser.Token.VALUE_STRING) {
return p.text();
} else if (token == XContentParser.Token.START_ARRAY) {
return p.list().stream().map(s -> (String) s).toArray(String[]::new);
} else {
throw new IllegalArgumentException("Unexpected token: " + token);
}
}, VALUE_FIELD, ObjectParser.ValueType.STRING_ARRAY);
}

public static PostSecretRequest fromXContent(XContentParser parser) throws IOException {
return PARSER.parse(parser, null);
}

private final String value;
private final Object value;

public PostSecretRequest(String value) {
public PostSecretRequest(Object value) {
this.value = value;
}

Expand All @@ -55,21 +60,29 @@ public PostSecretRequest(StreamInput in) throws IOException {
this.value = in.readString();
}

public String value() {
public Object value() {
return value;
}

public XContentBuilder toXContent(XContentBuilder builder) throws IOException {
builder.startObject();
builder.field(VALUE_FIELD.getPreferredName(), this.value);
if (value instanceof String) {
builder.field(VALUE_FIELD.getPreferredName(), (String) value);
} else if (value instanceof String[]) {
builder.field(VALUE_FIELD.getPreferredName(), (String[]) value);
}
builder.endObject();
return builder;
}

@Override
public void writeTo(StreamOutput out) throws IOException {
super.writeTo(out);
out.writeString(value);
if (value instanceof String) {
out.writeString((String) value);
} else if (value instanceof String[]) {
out.writeStringArray((String[]) value);
}
}

@Override
Expand All @@ -80,6 +93,12 @@ public ActionRequestValidationException validate() {
return exception;
}

if ((this.value instanceof String == false) && (this.value instanceof String[] == false)) {
ActionRequestValidationException exception = new ActionRequestValidationException();
exception.addValidationError("value must be a string or an array of strings");
return exception;
}

return null;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
import org.elasticsearch.tasks.Task;
import org.elasticsearch.transport.TransportService;

import java.util.List;

import static org.elasticsearch.xpack.core.ClientHelper.FLEET_ORIGIN;
import static org.elasticsearch.xpack.fleet.Fleet.FLEET_SECRETS_INDEX_NAME;

Expand All @@ -36,7 +38,19 @@ protected void doExecute(Task task, GetSecretRequest request, ActionListener<Get
delegate.onFailure(new ResourceNotFoundException("No secret with id [" + request.id() + "]"));
return;
}
delegate.onResponse(new GetSecretResponse(getResponse.getId(), getResponse.getSource().get("value").toString()));
Object value = getResponse.getSource().get("value");
if (value instanceof String) {
delegate.onResponse(new GetSecretResponse(getResponse.getId(), value.toString()));
} else if (value instanceof List) {
List<?> valueList = (List<?>) value;
if (valueList.stream().allMatch(item -> item instanceof String)) {
delegate.onResponse(new GetSecretResponse(getResponse.getId(), valueList.toArray(new String[0])));
} else {
delegate.onFailure(new IllegalArgumentException("Unexpected value type in list for id [" + request.id() + "]"));
}
} else {
delegate.onFailure(new IllegalArgumentException("Unexpected value type for id [" + request.id() + "]"));
}
}));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,26 @@ public void testValidateRequest() {
assertNull(e);
}

public void testValidateRequestWithMultiValue() {
String[] secrets = { "secret1", "secret2" };
PostSecretRequest req = new PostSecretRequest(secrets);
ActionRequestValidationException e = req.validate();
assertNull(e);
}

public void testValidateRequestWithoutValue() {
PostSecretRequest req = new PostSecretRequest((String) null);
ActionRequestValidationException e = req.validate();
assertNotNull(e);
assertThat(e.validationErrors().size(), equalTo(1));
assertThat(e.validationErrors().get(0), containsString("value is missing"));
}

public void testValidateRequestWithInvalidValue() {
PostSecretRequest req = new PostSecretRequest(123);
ActionRequestValidationException e = req.validate();
assertNotNull(e);
assertThat(e.validationErrors().size(), equalTo(1));
assertThat(e.validationErrors().get(0), containsString("value must be a string or an array of strings"));
}
}