Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide locally mounted secure settings implementation. #93392

Merged

Conversation

grcevski
Copy link
Contributor

This PR adds a different SecureSettings implementation which can fetch secrets from a locally mounted sub-directory under the Elasticsearch configuration directory.

Currently not used and in future should be extended to support reloading the secrets file if they are changed on disk.

@grcevski grcevski added >enhancement :Core/Infra/Core Core issues without another label Team:Core/Infra Meta label for core/infra team v8.7.0 labels Jan 31, 2023
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@elasticsearchmachine
Copy link
Collaborator

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

Copy link
Contributor

@williamrandolph williamrandolph left a comment

Choose a reason for hiding this comment

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

I have two small suggestions but otherwise I think the code is good to go!

* externally mounted local directory. It looks for the folder called 'secrets'
* under the config directory. All secure settings should be supplied in a single
* file called 'secrets.json' which sits inside the 'secrets' directory.
* <p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: it would be handy to have a sample settings.json (with dummy key-value pairs) in the javadoc.

}

@Override
public InputStream getFile(String setting) throws GeneralSecurityException {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be worth throwing an exception here? At least we should probably add a comment explaining that this implementation doesn't support files as setting value sources.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I'll actually add an implementation with ByteArrayInputStream, just like StatelessSecureSettings.

@grcevski grcevski merged commit c1b0bf6 into elastic:main Feb 6, 2023
@grcevski grcevski deleted the secure_settings/locally_mounted_secrets branch February 6, 2023 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Core Core issues without another label >enhancement Team:Core/Infra Meta label for core/infra team v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants