-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Fleet] Add SSL options to fleet server hosts settings #208091
[Fleet] Add SSL options to fleet server hosts settings #208091
Conversation
@elasticmachine merge upstream |
@elasticmachine merge upstream |
…t --include-path /api/status --include-path /api/alerting/rule/ --include-path /api/alerting/rules --include-path /api/actions --include-path /api/security/role --include-path /api/spaces --include-path /api/fleet --include-path /api/dashboards --update'
… src/core/server/integration_tests/ci_checks'
…t --include-path /api/status --include-path /api/alerting/rule/ --include-path /api/alerting/rules --include-path /api/actions --include-path /api/security/role --include-path /api/spaces --include-path /api/fleet --include-path /api/dashboards --update'
…t --include-path /api/status --include-path /api/alerting/rule/ --include-path /api/alerting/rules --include-path /api/actions --include-path /api/security/role --include-path /api/spaces --include-path /api/fleet --include-path /api/dashboards --update'
…t --include-path /api/status --include-path /api/alerting/rule/ --include-path /api/alerting/rules --include-path /api/actions --include-path /api/security/role --include-path /api/spaces --include-path /api/fleet --include-path /api/dashboards --update'
changes: [ | ||
{ | ||
type: 'mappings_addition', | ||
addedMappings: {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: what does empty addedMappings
mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This "empty migration" is needed as I'm using dynamic: false
here. This way new mappings can be added without explicitly declaring them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see, thanks for clarifying!
encryptedSavedObjects.registerType({ | ||
type: FLEET_SERVER_HOST_SAVED_OBJECT_TYPE, | ||
attributesToEncrypt: FLEET_SERVER_HOST_ENCRYPTED_FIELDS, | ||
enforceRandomId: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: does it mean you're making the ssl
property, which was previously unencrypted, encrypted? Or am I missing something? If so, what about existing objects where ssl
property is stored in non-encrypted form? How would that work right now?
The reason why I explicitly added this line is that our POST endpoint supports the creation of an explicit id by the user and having an UUID instead would be a major breaking change.
It'd be great if you could leave a comment in the code explaining this to help future readers.
@@ -1258,6 +1267,10 @@ export const OUTPUT_ENCRYPTED_FIELDS = new Set([ | |||
{ key: 'password', dangerouslyExposeValue: true }, | |||
]); | |||
|
|||
export const FLEET_SERVER_HOST_ENCRYPTED_FIELDS = new Set([ | |||
{ key: 'ssl', dangerouslyExposeValue: true }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Why do we need dangerouslyExposeValue: true
? Do we have to expose this value to our users in unencrypted form? If so, is it a requirement?
If we decide to keep this property, it'd be helpful to include a code comment with justification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well I'am showing the content of the ssl property in the UI to allow the user to see/edit it. This was a request by product. I can add a comment to explain it for sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you retrieve this object? If you have a dedicated endpoint, wouldn't it be possible to use getDecryptedAsInternalUser
internally to retrieve it (https://docs.elastic.dev/kibana-dev-docs/api/encryptedSavedObjects#if1e0a8f1-f453-11ef-ad72-4922219b4f76)?
What I'm trying to say is that dangerouslyExposeValue: true
should be a last-resort solution for cases where you need to retrieve encrypted saved objects using standard SO APIs and are willing to accept the risk of these secrets being leaked accidentally, since ESO will never strip them down.
to allow the user to see/edit it
I see, thanks. That's interesting - usually, existing secrets are never revealed once entered or saved to reduce the risk of leakage (e.g., you typically can't see an API key or password after saving it, since there's no need to view the existing value if you're going to change it). But I guess I'm just missing context and if you have such a product requirement, then I guess that's fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at the use ofgetDecryptedAsInternalUser
but at the moment it's not really feasible as it would mean rewriting the service in a totally different way.
I know that this solution is not ideal, but it's a temporary one and in fact this PR is also adding secrets.ssl
field that has the same capability and can store those values in a secure way by using secrets. The plan is to remove the use of the encrypted SO field once that's enabled. The only reason why is not already available is that we still need to add the secret reading capabilities to fleet server, but it is absolutely a temporary solution.
cc @kpollich
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks for looking into this and clarifying it for me. Having this as a temporary solution sounds reasonable to me.
@elasticmachine merge upstream |
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM from the AppEx Security side - temporarily using dangerouslyExposeValue: true
for a new field sounds tolerable.
@elasticmachine merge upstream |
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
History
cc @criamico |
Starting backport for target branches: 8.18, 8.x, 9.0 https://github.com/elastic/kibana/actions/runs/13630799063 |
💔 All backports failed
Manual backportTo create the backport manually run:
Questions ?Please refer to the Backport tool documentation |
…212918) ## Summary Small follow up of #208091 The editor autocompletion added an incorrect import and so I'm removing it, plus a few comments that should have been removed. Co-authored-by: Elastic Machine <[email protected]>
Fixes #207322
Summary
Show SSL options for fleet server host in Fleet server settings section and in add fleet server host flyout
ssl
property, mirroring what's already existing forlogstash
andkafka
outputsssh.key
andssh.es_key
can additionally be saved as secrets but for now this option is not enabled until fleet server supports it - I used the feature flagenableSSLSecrets
Screenshots
Generated policy:


Checklist