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

Remove securityOss plugin #113946

Merged

Conversation

jportner
Copy link
Contributor

@jportner jportner commented Oct 5, 2021

Resolves #104152.

Note: we don't ship the OSS distribution anymore, but the security plugin can be disabled in 7.16 (it won't be disable-able in 8.0+).
If someone is using the Default distribution of 7.16 and they have the security plugin disabled, then they won't see the Security Checkup message when they open Kibana for the first time. This edge case is an acceptable tradeoff to eliminating an entire plugin that we don't need anymore.

This PR also deprecates the security.showInsecureClusterWarning: false setting, renaming it to xpack.security.showInsecureClusterWarning: false. This setting does not appear in our documentation.

Testing

Existing functionality was refactored, here are the steps to test and make sure it still works as expected.

1. Insecure cluster toast

Kibana shows the toast when all three of these conditions are true:

  • The warning is enabled in kibana.yml (default),
  • Security is disabled, and
  • Kibana has some data in the cluster that is not sample data

image

To test this:

  1. Start Elasticsearch with security disabled: yarn es snapshot -E xpack.security.enabled=false
  2. Start Kibana
  3. Navigate to Dev Tools and add this staging data:
    POST foo/_doc
    { "bar": "baz" }
    
  4. Reload the page

You should see the toast now.
You can dismiss the toast, or you can disable the warning in kibana.yml using either security.showInsecureClusterWarning: false or xpack.security.showInsecureClusterWarning: false.
(I deprecated the former in favor of the latter)

2. Share Public URL

When anonymous access is enabled with another form of authentication, using the Share menu on a supported app (such as Dashboard) should show a switch for the Public URL. Enabling that will add an auth provider hint for the anonymous auth provider.

image

To test this:

  1. Configure Kibana with anonymous access:
    xpack.security.authc.providers:
      basic.basic1:
        order: 0
      anonymous.anonymous1:
        order: 1
        credentials:
          username: "elastic"
          password: "changeme"
  2. Start Elasticsearch and Kibana, and add some sample data
  3. View a Dashboard, click Share, and verify that the Public URL switch is displayed

If you configure Kibana without the anonymous auth provider, or with only the anonymous auth provider, the switch should not be displayed.

State includes access URL parameters and whether or not anonymous access
is enabled. Moved the server and client side code, and updated the share
plugin consumer accordingly. Got rid of a bit of dead code in the
process.
@jportner jportner added v8.0.0 auto-backport Deprecated - use backport:version if exact versions are needed v7.16.0 labels Oct 5, 2021
The securityOss plugin provided an insecure cluster service that the
security plugin augmented using its own security checkup service. I
combined the two into the latter. That was the last dependency on the
securityOss plugin, so this allowed me to completely remove the plugin.
@jportner jportner force-pushed the issue-104152-remove-security_oss-plugin branch from 123cc98 to 126d1fb Compare October 5, 2021 21:25
Copy link
Contributor Author

@jportner jportner left a comment

Choose a reason for hiding this comment

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

Author's notes for reviewers.

The insecure cluster warning could be shown in two different scenarios:
1. The Kibana security plugin is not available
2. Kibana security is available, but Elasticsearch security is disabled

Scenario 1 is no longer possible with this PR. Scenario 2 never had a
functional test suite because up to this point we did not have a way to
run the functional test server in that manner.

This commit removes the tests for Scenario 1, and we'll need to add new
tests for Scenario 2 in the 8.0 release when our functional testing
tooling supports it.
@jportner
Copy link
Contributor Author

jportner commented Oct 6, 2021

I had to remove the functional tests as described in bc8ac43:

The insecure cluster warning could be shown in two different scenarios:

  1. The Kibana security plugin is not available
  2. Kibana security is available, but Elasticsearch security is disabled

Scenario 1 is no longer possible with this PR. Scenario 2 never had a
functional test suite because up to this point we did not have a way to
run the functional test server in that manner.

This commit removes the tests for Scenario 1, and we'll need to add new
tests for Scenario 2 in the 8.0 release when our functional testing
tooling supports it.

I'll create an issue for adding the new functional tests, and update this comment with a link.

Edit: tracking this in #114049.

@jportner jportner marked this pull request as ready for review October 6, 2021 02:39
@jportner jportner requested review from a team as code owners October 6, 2021 02:39
@jportner jportner requested a review from vadimkibana October 6, 2021 02:43
@jportner jportner added release_note:skip Skip the PR/issue when compiling release notes release_note:deprecation and removed release_note:skip Skip the PR/issue when compiling release notes release_note:deprecation labels Oct 6, 2021
Copy link
Member

@legrego legrego left a comment

Choose a reason for hiding this comment

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

Tested locally, everything seems to be working as expected. I just have a couple of minor comments below, but otherwise looking good!

@jportner jportner requested a review from legrego October 6, 2021 18:41
Copy link
Member

@legrego legrego left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the edits!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
security 457 459 +2
securityOss 11 - -11
total -9

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
securityOss 9 - -9

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
securityOss 3 - -3

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
security 49.4KB 51.3KB +1.9KB
securityOss 6.2KB - -6.2KB
share 53.8KB 54.0KB +227.0B
total -4.1KB
Unknown metric groups

API count

id before after diff
securityOss 12 - -12
share 137 143 +6
total -6

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@jportner jportner enabled auto-merge (squash) October 7, 2021 14:56
Copy link
Contributor

@vadimkibana vadimkibana left a comment

Choose a reason for hiding this comment

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

AppServices code changes LGTM.

@jportner jportner merged commit 64f37e7 into elastic:master Oct 7, 2021
@kibanamachine
Copy link
Contributor

💔 Backport failed

The backport operation could not be completed due to the following error:
The PR #113946 is not merged

The backport PRs will be merged automatically after passing CI.

To backport manually run:
node scripts/backport --pr 113946

@jportner jportner deleted the issue-104152-remove-security_oss-plugin branch October 7, 2021 16:01
jportner added a commit to jportner/kibana that referenced this pull request Oct 7, 2021
# Conflicts:
#	.eslintrc.js
#	.github/CODEOWNERS
#	api_docs/security_oss.mdx
#	packages/kbn-optimizer/limits.yml
#	scripts/functional_tests.js
#	x-pack/plugins/translations/translations/ja-JP.json
#	x-pack/plugins/translations/translations/zh-CN.json
jportner added a commit that referenced this pull request Oct 7, 2021
* Remove securityOss plugin (#113946)

# Conflicts:
#	.eslintrc.js
#	.github/CODEOWNERS
#	api_docs/security_oss.mdx
#	packages/kbn-optimizer/limits.yml
#	scripts/functional_tests.js
#	x-pack/plugins/translations/translations/ja-JP.json
#	x-pack/plugins/translations/translations/zh-CN.json

* Add unintentionally removed translations

* Fix jest test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes v7.16.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move and refactor x-pack plugin code for Security and Spaces
6 participants