Always add file and native realms unless explicitly disabled #69096
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ywangd
added
>enhancement
:Security/Authentication
Collaborator
|
Pinging @elastic/es-security (Team:Security) |
Contributor
albertzaharovits
left a comment
albertzaharovits
left a comment
There was a problem hiding this comment.
Mostly LGTM, but I had 3 nits to help with readability of the existing complicated code.
One suggestion I have is to add the file realm at the beginning, not at the end. This to make the future "rescue tool" more robust and also because I believe the special file realm in cloud is like this.
I wonder if after this change, as a follow-up, we can disable APIs that put and delete users if the native realm is explicitly disabled. I think we should.
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/Realms.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/RealmConfig.java
Show resolved
Hide resolved
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/Realms.java
Outdated
Show resolved
Hide resolved
Contributor
|
PS I think it should only go in 8, there is some risk that forgotten users pop-up after an upgrade. |
albertzaharovits
approved these changes
Apr 8, 2021
Member
Author
|
@tvernum Albert approved the PR. Do you need to take another look? |
tvernum
approved these changes
Apr 12, 2021
| them explicitly, `file` and `native` realms will be added automatically to the | ||
| beginning of the realm chain in that order. To opt-out from the automatic behaviour, | ||
| you can explicitly configure the `file` and `native` realms with the `order` | ||
| and `enabled` settings. |
Contributor
There was a problem hiding this comment.
I'd like us to include an example here (or somewhere else in the docs), like:
This sample enables the file realm and an LDAP realm, but disables the native realm:
xpack.security.authc.realms: file.file_realm: order: 0 ldap.corporate_ldap order: 1 url: 'url_to_ldap_directory' ... native.native_realm: enabled: false
Member
Author
There was a problem hiding this comment.
I updated the existing sample to disable the native realm as an example. Thanks!
nik9000
added a commit
to nik9000/elasticsearch
that referenced
this pull request
Apr 13, 2021
We recently upgrade to gradle 7.0 (elastic#69096) which turned on module inference by default. I'm sure that's lovely, but its broken eclipse. We should probably support modules one day, but that day ain't today. This turns off module inference for eclipse so it can continue compiling things just like it did yesterday. Closes elastic#71648
nik9000
added a commit
that referenced
this pull request
Apr 13, 2021
We recently upgrade to gradle 7.0 (#69096) which turned on module inference by default. I'm sure that's lovely, but its broken eclipse. We should probably support modules one day, but that day ain't today. This turns off module inference for eclipse so it can continue compiling things just like it did yesterday. Closes #71648
breskeby
pushed a commit
to breskeby/elasticsearch
that referenced
this pull request
Apr 14, 2021
We recently upgrade to gradle 7.0 (elastic#69096) which turned on module inference by default. I'm sure that's lovely, but its broken eclipse. We should probably support modules one day, but that day ain't today. This turns off module inference for eclipse so it can continue compiling things just like it did yesterday. Closes elastic#71648
breskeby
added a commit
that referenced
this pull request
Apr 14, 2021
- Update gradle wrapper to gradle 7.0 - Remove deprecated usages to make build 7.0 compatible - Fix excludes in docs snippet tasks (See gradle/gradle#16160 for details) - Fix deprecation warnings in 7.0 - Add explicit dependencies that have been missed - Make extract native licenses tasks output dir more explicit - Use a snapshot of the ospackage plugin that includes a fix for 7.0 already - fix test runtime classpath setup in repository-hdfs - Make task dependency explicit to fix further deprecation warnings - Remove manual check for http repo usages that has been deprecated in gradle 7.0 - Update spock to latest 2.0 milestone required for groovy 3 - Disable module inference in Eclipse (#71649) We recently upgrade to gradle 7.0 (#69096) which turned on module inference by default. I'm sure that's lovely, but its broken eclipse. We should probably support modules one day, but that day ain't today. This turns off module inference for eclipse so it can continue compiling things just like it did yesterday. Co-authored-by: Nik Everett <[email protected]>
elasticsearchmachine
pushed a commit
that referenced
this pull request
Sep 29, 2021
#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]>
ywangd
added a commit
to ywangd/elasticsearch
that referenced
this pull request
Jun 24, 2022
All enabled realms must have unique names. This PR tightened the logic for ensuring realm name uniqueness. Previously the unique name check does not cover realms that are automatically enabled. Relates: elastic#69096
elasticsearchmachine
pushed a commit
that referenced
this pull request
Jun 24, 2022
All enabled realms must have unique names. This PR tightened the logic for ensuring realm name uniqueness. Previously the unique name check does not cover realms that are automatically enabled. Relates: #69096
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR makes sure that the file and native realms are always added to the realm chain unless explicitly disabled.
Currently, they are only impliciltly added when:
A side effect (intended?) is that file and native realm cannot be truely disabled at all time because the above two rules always apply regardless whether the realms are disabled or not.
This PR makes the behaviour more explicit. If the file or native realm is explicitly disabled, it will be disabled at all time. If they are not explicitly disabled, they will always be added to the realm chain. Two scenarios are possible:
PS: I labelled this as
v8.0.0only because it is a breaking change.TODO:
Resolves: #50892