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.

Sign up for GitHub

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

Jump to bottom

Deprecate the behaviour of implicitly disabling file/native realm #69320

Merged
merged 14 commits into from
Mar 15, 2021
Merged

Deprecate the behaviour of implicitly disabling file/native realm #69320

merged 14 commits into from
Mar 15, 2021

Conversation

ywangd
Copy link
Member

@ywangd ywangd commented Feb 22, 2021

As a precursor for #50892, this PR deprecate the behaviour of file and/or native realm being implicitly disabled when there are other explicitly configured realms.

With this change, the recommend way of disabling file/native realm is to explicitly set enabled to false, e.g.:

xpack.security.authc.realms.file.default_file.enabled: false

This PR ensures that a warning is generated whenever file and/or native realm is implicitly disabled.

This change also brings a question about the order parameter. Currently, the order parameter is mandatory in 8.0 and gets a warning message if it is missing in 7.x. However, it makes sense to not specify the order parameter if the realm is disabled. So I also updated the order parameter related code to do just that.

Relates: #50892

@ywangd ywangd added >deprecation :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) v7.13.0 labels Feb 22, 2021
@elasticmachine elasticmachine added the Team:Security Meta label for security team label Feb 22, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security (Team:Security)

@ywangd
Copy link
Member Author

ywangd commented Feb 22, 2021

Please note this PR is for 7.x only because there is a separate PR (#69096) for always enabling file/native realm. Also because of the code difference caused by the order parameter deprecation, it does not worth the effort trying to add this to 8.0 and then "backport" to 7.x since the "backport" is likely to be just a manual rewrite.

albertzaharovits
albertzaharovits approved these changes Feb 25, 2021
View reviewed changes
Copy link
Contributor

@albertzaharovits albertzaharovits left a comment

Choose a reason for hiding this comment

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

LGTM with two small nits.

...gin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecks.java Outdated Show resolved Hide resolved
...gin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecks.java Outdated Show resolved Hide resolved
@albertzaharovits
Copy link
Contributor

PS: Do not forget to add the breaking notice in the #50892 PR.

@jkakavas jkakavas mentioned this pull request Mar 8, 2021
Closed
tvernum
tvernum reviewed Mar 10, 2021
View reviewed changes
...gin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecks.java Outdated Show resolved Hide resolved
...gin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecks.java Outdated
return null;
}
// If all configured realms are disabled, this equals to no realm is configured. The implicitly behaviour in this case
// is to add file and native realms. So we are good here.
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we good?
What will this config do in 8.0?

xpack.security.authc.realms:
  native.native_realm:
    enabled: false
  ldap.corp_ldap:
    enabled: false

In 7.x it will result in both a file and native realm enabled (I think)
I believe the proposal is that in 8.x it will result in just a file realm.

Should we issue a deprecation warning (because the behaviour will change) in this case too?
(A separate PR is fine though)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. Yes this is indeed the proposal in the other PR #69096. I updated this PR to issue warning for this scenario as well. I'd like to mention that this change also means that the cluster will not automatically fallback to file/native realms when:

  • The License is downgraded and
  • The file or native realm is explicitly disabled

I don't know whether users rely on this "feature". I personally feel it is not something we should cater for because implicitly auto-enabling realms on the fly could be a security issue and also explicit behaviours are what we are after with these changes. All in all, if a realm is explicity disabled, I prefer it remain disabled at all times.

...gin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecks.java Outdated

final String details = String.format(
Locale.ROOT,
"Found implicitly disabled native %s: [%s]. %s disabled because there are other explicitly configured realms." +
Copy link
Contributor

Choose a reason for hiding this comment

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

"native" here means "file or native" right?
Do we need a better word?

Copy link
Member Author

Choose a reason for hiding this comment

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

I struggled with this and could not find a better word. I used native because that is what used in the Realms class. I agree it is confusing. If possible, I would rename "native realm" to something like "index realm", but I think it would be a cognitive challenge for users. I also thought about the word "builtin" but all realms are kinda "builtin", i.e. part of default distribution. The word "internal" is also used for all realms. How about "basic"?

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed all places to use the term "basic".

@ywangd ywangd requested a review from tvernum March 14, 2021 12:23
ywangd and others added 2 commits March 15, 2021 15:51
@ywangd @tvernum
Update x-pack/plugin/deprecation/src/main/java/org/elasticsearch/xpac…
f1bb799 
…k/deprecation/NodeDeprecationChecks.java

Co-authored-by: Tim Vernum <[email protected]>
@ywangd
final tweak
3a3c261
@ywangd ywangd merged commit 1e4732e into elastic:7.x Mar 15, 2021
@ywangd ywangd mentioned this pull request Mar 15, 2021
Merged
ywangd added a commit that referenced this pull request Apr 19, 2021
@ywangd
[Test] Handle warning when all realms are disabled (#70392)
3ceb34e 
When all configured realms are disabled, the new warning added by #69320 needs to be handled.

Resolves: #70378
@jrodewig jrodewig mentioned this pull request Sep 28, 2021
Merged
elasticsearchmachine pushed a commit that referenced this pull request Sep 29, 2021
@jrodewig @ywangd
[DOCS] Always enable file and native realms unless explicitly disabled (
4544ab2 
#78405)

* [DOCS] Always enable file and native realms by default

Adds an 8.0 breaking change for PR #69096.

The copy is based on the 7.13 deprecation notice added with PR #69320.

* reword

* Update docs/reference/migration/migrate_8_0/security.asciidoc

Co-authored-by: Yang Wang <[email protected]>

* Update docs/reference/migration/migrate_8_0/security.asciidoc

Co-authored-by: Yang Wang <[email protected]>

Co-authored-by: Yang Wang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@albertzaharovits albertzaharovits albertzaharovits approved these changes

@tvernum tvernum tvernum approved these changes

Assignees
No one assigned
Labels
>deprecation :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) Team:Security Meta label for security team v7.13.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ywangd @elasticmachine @albertzaharovits @tvernum