Include Secure Setting Names and Keystore Modified Time in Reload API Response #138052
Conversation
…d-secure-settings API response
ebarlas
added
>enhancement
:Security/Security
|
Hi @ebarlas, I've created a changelog YAML for you. |
|
Pinging @elastic/es-security (Team:Security) |
jfreden
left a comment
jfreden
left a comment
There was a problem hiding this comment.
The PR looks great and the code is solid as always! I have a couple of comments.
At a higher level, it was discussed in the issue if the absolute path to the keystore should be included or not. My understanding from the discussion is that you landed on including it but it's not part of the change?
| return ElasticsearchException.readException(this); | ||
| } | ||
|
|
||
| /** |
| try { | ||
| return Files.readAttributes(path, BasicFileAttributes.class).lastModifiedTime().toMillis(); | ||
| } catch (IOException e) { | ||
| return null; // fallback to null if we can't read the time |
There was a problem hiding this comment.
I think we should log a warning here since it's unexpected. We just refreshed the keystore and now we can't get the modified timestamp.
| summary: Include Secure Setting Names and Keystore Modified Time in Reload API Response | ||
| area: Security | ||
| type: enhancement | ||
| issues: [] |
| try { | ||
| assertThat(nodesReloadResponse, notNullValue()); | ||
| final Map<String, NodesReloadSecureSettingsResponse.NodeResponse> nodesMap = nodesReloadResponse.getNodesMap(); | ||
| assertThat(nodesMap.size(), equalTo(cluster().size())); |
There was a problem hiding this comment.
I don't think it matters but this test is disabled when running in fips mode. Since we're reading properties on the keystore we might want to make sure it works in fips mode too?
| public static class NodeResponse extends BaseNodeResponse { | ||
|
|
||
| private Exception reloadException = null; | ||
| private static final TransportVersion KEYSTORE_DETAILS = TransportVersion.fromName("keystore_details_in_reload_response"); |
There was a problem hiding this comment.
nit: might want to use more specific naming since we might want to add more details in the future.
| import java.util.List; | ||
| import java.util.Optional; | ||
|
|
||
| public class NodesReloadSecureSettingsResponseTests extends ESTestCase { |
There was a problem hiding this comment.
There is actually an abstract class that does testing like this AbstractBWCSerializationTestCase that's used throughout the code base. You might want to consider extending that for more test coverage.
3 participants
This change augments the reload_secure_settings API response with the following properties for each node object:
secure_setting_names: JSON array of secure setting names from ES keystorekeystore_last_modified_time: JSON string with ES keystore last-modified date-timeThis change is a response to recurring ES admin confusion about which securing settings are being reloaded from which keystore. See issue #112268.
Curl call to
reload_secure_settingstargeting local ES distribution: