[Kerberos] Remove realm from principal name#31928
Merged
bizybot merged 5 commits intoelastic:feature/kerberosfrom Jul 12, 2018
Merged
[Kerberos] Remove realm from principal name#31928bizybot merged 5 commits intoelastic:feature/kerberosfrom
bizybot merged 5 commits intoelastic:feature/kerberosfrom
Conversation
This commit adds setting for stripping realm name from the Kerberos principal name. The principal names in Kerberos are in the form `primary/instance@realm`. Since we will be supporting user lookups and depending on the scenario we may want to strip the REALM part and use the username for lookup or role mapping. This change adds new setting with default value `false`. Also modified tests to randomly use this setting during Kerberos testing.
bizybot
added
>feature
:Security/Authentication
Collaborator
|
Pinging @elastic/es-security |
jaymode
reviewed
Jul 10, 2018
Member
jaymode
left a comment
jaymode
left a comment
There was a problem hiding this comment.
I left some minor comments about naming
| public static final Setting<Boolean> SETTING_KRB_DEBUG_ENABLE = | ||
| Setting.boolSetting("krb.debug", Boolean.FALSE, Property.NodeScope); | ||
| public static final Setting<Boolean> SETTING_STRIP_REALM_NAME = | ||
| Setting.boolSetting("strip.realm_name", Boolean.FALSE, Property.NodeScope); |
Member
There was a problem hiding this comment.
what about include_realm_name with a default of true? or remove_realm_name with a default of false? @tvernum thoughts?
Contributor
Author
There was a problem hiding this comment.
changed this to remove_realm_name with a default of false. Will change if Tim suggests something else. Thank you.
| * @param principalName user principal name | ||
| * @return result string after stripping realm name | ||
| */ | ||
| private String stripRealmName(final String principalName) { |
Member
There was a problem hiding this comment.
this should probably be named something like maybeRemoveRealmName
| } else { | ||
| if (validTicket) { | ||
| final User expectedUser = new User(username, roles.toArray(new String[roles.size()]), null, null, null, true); | ||
| final User expectedUser = new User(stripRealmName(username), roles.toArray(new String[roles.size()]), null, null, null, |
Member
There was a problem hiding this comment.
nit: its much cleaner if you just move the whole new User(... statement to a new line
Contributor
Author
There was a problem hiding this comment.
Yes, changes it. Thank you.
bizybot
changed the title
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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 commit adds support for removing realm name
from the Kerberos principal name. The principal names in
Kerberos are in the form
primary/instance@realm.Since we will be supporting user lookups and depending on the
scenario we may want to remove the REALM part and use the username
for lookup or role mapping.
This change adds a new setting with the default value
falsetocontrol removing of realm name.
Modified tests to randomly use this setting during testing.