-
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
Warn users when security is not configured #78545
Warn users when security is not configured #78545
Conversation
8b84db0
to
4b09d12
Compare
4b09d12
to
26d5b9d
Compare
Pinging @elastic/kibana-security (Team:Security) |
@elastic/kibana-security the actual alert text is still being discussed, but the technical implementation is ready to be looked at. |
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.
Looking great so far! Some nits and comments below.
Edit: I know we haven't finalized the wording, but when I'm using the Basic license with Security disabled, the warning really doesn't make sense to me as a user. The "Learn more" button drops me on a landing page talking about the Stack's security features, but it doesn't give me a concise call to action. What should I do as a user?
Maybe in the X-pack version of the message we should link to this page instead: https://www.elastic.co/guide/en/elasticsearch/reference/current/get-started-enable-security.html
export const insecureClusterAlertText = (onDismiss: () => void) => | ||
((e) => { | ||
render( | ||
<div data-test-subj="insecureClusterAlertText"> | ||
<EuiText> | ||
<FormattedMessage | ||
id="xpack.security.checkup.insecureClusterMessage" | ||
defaultMessage="Our free security features can protect against unauthorized access." | ||
/> | ||
</EuiText> | ||
<EuiSpacer /> | ||
<EuiFlexGroup> | ||
<EuiFlexItem grow={false}> | ||
<EuiButton | ||
color="primary" | ||
fill | ||
href="https://www.elastic.co/what-is/elastic-stack-security" | ||
> | ||
Learn more | ||
</EuiButton> | ||
</EuiFlexItem> | ||
<EuiFlexItem grow={false}> | ||
<EuiButton onClick={onDismiss}>Dismiss</EuiButton> | ||
</EuiFlexItem> | ||
</EuiFlexGroup> | ||
</div>, | ||
e | ||
); |
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 know the wording isn't finalized, but looking at the toast:
IMO it looks a bit better if we change the text size and button size to 'small', and if we space the buttons evenly:
(click for diff)
@@ -19,16 +19,17 @@ export const insecureClusterAlertText = (onDismiss: () => void) =>
((e) => {
render(
<div data-test-subj="insecureClusterAlertText">
- <EuiText>
+ <EuiText size="s">
<FormattedMessage
id="xpack.security.checkup.insecureClusterMessage"
defaultMessage="Our free security features can protect against unauthorized access."
/>
</EuiText>
<EuiSpacer />
- <EuiFlexGroup>
+ <EuiFlexGroup justifyContent="spaceBetween">
<EuiFlexItem grow={false}>
<EuiButton
+ size="s"
color="primary"
fill
href="https://www.elastic.co/what-is/elastic-stack-security"
@@ -37,7 +38,9 @@ export const insecureClusterAlertText = (onDismiss: () => void) =>
</EuiButton>
</EuiFlexItem>
<EuiFlexItem grow={false}>
- <EuiButton onClick={onDismiss}>Dismiss</EuiButton>
+ <EuiButton size="s" onClick={onDismiss}>
+ Dismiss
+ </EuiButton>
</EuiFlexItem>
</EuiFlexGroup>
</div>,
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 like your proposal. Consider it done!
</EuiButton> | ||
</EuiFlexItem> | ||
<EuiFlexItem grow={false}> | ||
<EuiButton onClick={onDismiss}>Dismiss</EuiButton> |
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 know the wording isn't finalized, but this button doesn't just dismiss the toast, it hides the toast permanently. Maybe we need to change this to "Ignore" or "Don't remind me again" (though that's a bit verbose). Interested to hear what @gchaps has to say.
(Also: I know this is out of scope for the MVP, but it would be better if we could give the user an option to "remind me later" too. If I was an admin and I wanted to get this out of the way, I'd click that instead.)
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.
For the button name, I thought of "Skip", "No thanks", "Got it", and "Close". Given all that, I prefer Dismiss. we really don't want users to ignore this message. Or, can a toast have a Close icon in the upper right?
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.
Given all that, I prefer Dismiss. we really don't want users to ignore this message.
Right, but I don't think we're adequately communicating to users that if they click Dismiss, they'll never see the message again.
Or, can a toast have a Close icon in the upper right?
There is a close icon in the upper right of every toast -- currently if you click it for this toast, it dismisses the toast until the next page load. Unlike the button, which permanently hides the toast.
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.
import { ConfigType } from '../config'; | ||
import { defaultAlertText, defaultAlertTitle } from './components'; | ||
|
||
const STORAGE_KEY = 'insecureClusterWarningVisibility'; |
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.
Perhaps we should make a tenant-specific key, similar to:
kibana/x-pack/plugins/security/public/session/session_timeout.tsx
Lines 129 to 130 in 574205d
const key = `${this.tenant}/session_provider`; | |
sessionStorage.setItem(key, sessionInfo.provider.name); |
via:
kibana/x-pack/plugins/security/public/plugin.tsx
Lines 76 to 80 in 574205d
const tenant = core.http.basePath.serverBasePath; | |
const sessionExpired = new SessionExpired(logoutUrl, tenant); | |
http.intercept(new UnauthorizedResponseHttpInterceptor(sessionExpired, anonymousPaths)); | |
this.sessionTimeout = new SessionTimeout(notifications, sessionExpired, http, tenant); |
// 10 days is reasonably long enough to call "forever" for a page load. | ||
// Can't go too much longer than this. See https://github.com/elastic/kibana/issues/64264#issuecomment-618400354 |
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.
👍
private getPersistedVisibilityPreference() { | ||
return (this.storage.getItem(STORAGE_KEY) ?? 'show') !== 'hide'; | ||
} | ||
|
||
private setPersistedVisibilityPreference(visible: boolean) { | ||
this.storage.setItem(STORAGE_KEY, visible ? 'show' : 'hide'); | ||
} |
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.
Perhaps we should change this to persist JSON instead of a raw string, to support future enhancements (such as a "remind me later" dismiss option).
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.
Yeah I was thinking about this too. You also questioning this is reason enough for me!
} | ||
|
||
export class SecurityCheckupService { | ||
private securityOssStart?: SecurityOSSPluginStart; |
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 plugin dependency is required, so it doesn't need to be be optional here (and below in the onDismiss
function too).
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.
Yep - we do expect the plugin to be available, but marking securityOssStart
as potentially undefined is more correct since it is only set during the setup
phase, which should always be called, but there's nothing forcing this to happen (unlike a constructor, which can't not be called)
router.get( | ||
{ | ||
path: '/internal/security_oss/display_insecure_cluster_alert', | ||
validate: false, | ||
options: { | ||
authRequired: 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.
I'm concerned about information disclosure here. We're exposing a route that doesn't require authentication, which will tell any user whether this cluster has (non-system, non-sample) data in it.
Reading InsecureClusterService
, it appears we only attempt to hit this API if we aren't on an anonymous route -- do we need authRequired: false
here, or can we get rid of it?
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.
Good call, I don't think we need authRequired: false
here. I think this was a holdover from a previous implementation.
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.
FWIW I'm not so concerned about information disclosure here. If security is enabled, then this will always return false
, so you won't know if the cluster contains data by hitting this endpoint.
On the other hand, if security is disabled, then anyone can enter Kibana without credentials and use Dev Tools or Index Management to see what's inside the cluster.
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.
Good point!
…-insecure-clusters
++ Completely agree, and I like your suggestion |
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.
One nit, but docker config changes LGTM
@jportner ready for next round of reviews (aside from final copy). As discussed offline, this adds a checkbox to toggle whether or not we should remember the user's preference to hide the alert, instead of always persisting it |
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.
Looks great! My last nit:
When the "Don't show again" box is unchecked, if you dismiss the toast, opening a new tab shows the message again. I could see that getting annoying pretty quickly.
It would be nice to make this option dismiss the toast for the duration of the session.
However, It appears that sessionStorage
wouldn't help us here. We could potentially store a value in the server-side session, or on the client side resort to something like broadcast-channel
. Both of these sound like overkill to me for an MVP, though. /ramble
Agree on all counts. Let's see what kind of feedback we get before architecting something overly complex. |
I know we are still iterating on text, but since the technical approach looks good to reviewers, I will plan on merging by EOD today so that we aren't trying to fight CI on FF day. Any changes to text can be easily made following FF as a bugfix IMO. Anything more substantial than text changes can be evaluated for inclusion. |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]@kbn/optimizer bundle module count
distributable file count
page load bundle size
History
To update your PR or re-run it, just comment with: |
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> # Conflicts: # .github/CODEOWNERS # scripts/functional_tests.js # tasks/function_test_groups.js
* master: (85 commits) Refactor attribute service (elastic#78414) [APM] Add default message to alerts. (elastic#78930) [Discover] Modify columns and sort when switching index pattern (elastic#74336) Document ts project references setup (elastic#78586) build all ts refs in single kbn:bootstrap (elastic#79438) [TSVB] Allow string fields on value count aggregation (elastic#79267) [SECURITY SOLUTION] Investigate EQL signal in timeline (elastic#79049) [Fleet] Add loading spinner to page headers (elastic#79568) [Security Solution][Resolver] Resolver query panel load more (elastic#79160) Add type row to monitor detail page. (elastic#79556) Remove license refresh from setup (elastic#79518) [docker] add reporting fonts (elastic#74806) [SECURITY_SOLUTION][ENDPOINT] Add info about trusted apps to the page subtitle + create flyout (elastic#79558) Trim Hash value before validating it (elastic#79545) Warn users when security is not configured (elastic#78545) update copy styling (elastic#79313) Update dependency @elastic/charts to v23.1.1 (elastic#78459) Introduce geo-threshold alerts (elastic#76285) elastic#76920 Show base breadcrumb when there is an error booting the app (elastic#79571) remove react-intl from kibana and keep it inside only i18n package (elastic#78956) ...
Summary
Running Kibana or Elaticsearch without security enabled is a potentially dangerous configuration. This is especially true when either of these systems are exposed to the public Internet.
In an effort to educate users, we display an alert when security is not enabled, and offer a call to action so that users can learn more about enabling our free security features.
Showing the alert
We perform a series of checks to determine if the alert should be displayed:
.
, or that isn't the Kibana Sample Data indices.The alert will only be displayed if all of the preceding checks are satisfied.
Disabling the alert via
kibana.yml
If you are securing access to Kibana/Elasticseach through other means, -or- you wish to intentionally run without security, then you can disable the alert via
kibana.yml
:Disabling the alert via third-party plugin
If you are a third-party plugin developer, you can disable this check by consuming the
securityOSS
plugin'ssetup
contract:Resolves #73929