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

Add realm information for Authenticate API #35648

Merged
merged 15 commits into from
Nov 27, 2018
Merged

Add realm information for Authenticate API #35648

merged 15 commits into from
Nov 27, 2018

Conversation

jkakavas
Copy link
Member

@jkakavas jkakavas commented Nov 16, 2018 •
edited
Loading

  • Adds the authentication realm and lookup realm name and type in the response for the _authenticate API
  • Doesn't touch the User object ( neither in the client nor on the server side ) and adds the realm information on the response objects only.
  • The authentication realm is set as the lookup realm too (instead of setting the lookup realm to null or empty ) when no lookup realm is used.

@jkakavas jkakavas added >enhancement WIP :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) labels Nov 16, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security

tvernum
tvernum reviewed Nov 19, 2018
View reviewed changes
Copy link
Contributor

@tvernum tvernum left a comment

Choose a reason for hiding this comment

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

I left a couple of comments

...re/src/main/java/org/elasticsearch/xpack/core/security/action/user/AuthenticateResponse.java Outdated Show resolved Hide resolved
...ck/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/Authentication.java Outdated Show resolved Hide resolved
@albertzaharovits
Copy link
Contributor

Left some notes, it does look more complicated than at first sight!

@jkakavas jkakavas removed the WIP label Nov 20, 2018
@jkakavas jkakavas changed the title [WIP] Add realm information for Authenticate API Add realm information for Authenticate API Nov 20, 2018
@jkakavas
Copy link
Member Author

Thanks for the feedback @albertzaharovits , @tvernum . I addressed your comments and removed the WIP, this is ready for a final round

@elastic elastic deleted a comment Nov 20, 2018
@elastic elastic deleted a comment Nov 20, 2018
albertzaharovits
albertzaharovits reviewed Nov 21, 2018
View reviewed changes
...re/src/main/java/org/elasticsearch/xpack/core/security/action/user/AuthenticateResponse.java Outdated
}

@Override
public void writeTo(StreamOutput out) throws IOException {
super.writeTo(out);
User.writeTo(user, out);
if (out.getVersion().before(Version.V_6_6_0)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you have to keep Version.V_7_0_0 and change on backport. I don't know how CI passed, it should've failed in the bwc tests. To not lose any extra time I think you can try to push to both branches simultaneously, Tom Cruise style. Squash locally and run the precommit tests on 6.x, then merge on master, and do a fast cherry-pick and push origin on the 6.x . 🤞

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. I'll address this now and take advantage of CI split and the fact that Tim won't wake up for another 6 hours :)

albertzaharovits
albertzaharovits approved these changes Nov 21, 2018
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

@jkakavas
Copy link
Member Author

jkakavas commented Nov 27, 2018 •
edited
Loading

I addressed @tvernum 's suggestion to add the name and type in a nested object. I left out node name from the realm information on purpose as I'm not sure it adds value but I have no strong objections in adding that too.

tvernum
tvernum approved these changes Nov 27, 2018
View reviewed changes
Copy link
Contributor

@tvernum tvernum left a comment

Choose a reason for hiding this comment

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

LGTM

...st-high-level/src/test/java/org/elasticsearch/client/security/AuthenticateResponseTests.java Outdated
final String authenticationRealmName = randomAlphaOfLength(5);
final String authenticationRealmType = randomFrom("file", "native", "ldap", "ad", "saml", "kerberos");
final String lookupRealmName = randomAlphaOfLength(5);
final String lookupRealmType = randomFrom("file", "native", "ldap", "ad", "saml", "kerberos");
Copy link
Contributor

Choose a reason for hiding this comment

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

It's totally insignificant, but the actual AD realm type is active_directory.

@jkakavas
Copy link
Member Author 

REPRODUCE WITH ./gradlew :client:rest-high-level:integTestRunner -Dtests.seed=F4446ECC07BEB9D0 -Dtests.class=org.elasticsearch.client.TasksIT -Dtests.method="testGetInvalidTask" -Dtests.security.manager=true -Dtests.locale=lt -Dtests.timezone=Europe/Prague -Dcompiler.java=11 -Druntime.java=8

this does not reproduce locally.

@elasticmachine run the gradle build tests 1

@jkakavas jkakavas merged commit 580b5ba into elastic:master Nov 27, 2018
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Nov 28, 2018
@jasontedor
Merge branch 'master' into xpack-license
6037c8b 
* master:
  DOCS Audit event attributes in new format (elastic#35510)
  Scripting: Actually add joda time back to whitelist (elastic#35965)
  [DOCS] fix HLRC ILM doc misreferenced tag
  Add realm information for Authenticate API (elastic#35648)
  [ILM] add HLRC docs to remove-policy-from-index (elastic#35759)
  [Rollup] Update serialization version after backport
  [Rollup] Add more diagnostic stats to job (elastic#35471)
  Build: Fix gradle build for Mac OS (elastic#35968)
  Adds deprecation logging to ScriptDocValues#getValues. (elastic#34279)
  [Monitoring] Make Exporters Async (elastic#35765)
  [ILM] reduce time restriction on IndexLifecycleExplainResponse (elastic#35954)
  Remove use of AbstractComponent in xpack (elastic#35394)
  Deprecate types in search and multi search templates. (elastic#35669)
  Remove fromXContent from IndexUpgradeInfoResponse (elastic#35934)
@jkakavas jkakavas mentioned this pull request Nov 28, 2018
Closed
jkakavas added a commit that referenced this pull request Nov 29, 2018
@jkakavas
Add realm information for Authenticate API (#35648)
b515ec7 
- Add the authentication realm and lookup realm name
  and type in the response for the _authenticate API
- The authentication realm is set as the lookup realm
  too (instead of setting the lookup realm to null or
  empty ) when no lookup realm is used.
jkakavas added a commit that referenced this pull request Nov 29, 2018
@jkakavas
Update version for authenticate API responses
89c5151 
This commits changes the serialization version from V_7_0_0 to
v_6_6_0 for the authenticate API response now that the work to add
the realm info in the response has been backported to 6.x in
b515ec7

Relates #35648
This was referenced Nov 30, 2018
Merged
Closed
@codebrain codebrain mentioned this pull request May 8, 2019
Merged
@ywangd ywangd mentioned this pull request Aug 14, 2020
Closed
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
>enhancement :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) v6.6.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jkakavas @elasticmachine @albertzaharovits @tvernum @colings86