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

Add authentication type to authentication response #61130

Closed
ywangd opened this issue Aug 14, 2020 · 6 comments · Fixed by #61247
Closed

Add authentication type to authentication response #61130

ywangd opened this issue Aug 14, 2020 · 6 comments · Fixed by #61247
Assignees
Labels
>enhancement :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) Team:Security Meta label for security team

Comments

@ywangd
Copy link
Member

ywangd commented Aug 14, 2020

The response of GET _security/_authenticate currently does not include authentication type. AuthenticationType is a new addition (#38291) to the Authentication class after the last change (#35648) to the authentication response object. It was unintentional that it did not get added to the authentication response.

We should add it because:

  1. it makes the authentication response more complete
  2. it is the only thing that can tell the difference of a token auth

One might argue that auth type is something that the client already knows so it is not very useful. But from a client side point of view: a connection object can be created with token authentication in one place where the client is aware of the authentication type. But the object can subsequently be passed to somewhere else where it might be a hassle to find out this information from the connection object itself. I’d personally prefer to have the authentication response to cover all necessary information. From server side perspective, I prefer to not assume what clients may or may not know already and always provide complete set of information.

@ywangd ywangd added >enhancement :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) labels Aug 14, 2020
@elasticmachine
Copy link
Collaborator

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

@elasticmachine elasticmachine added the Team:Security Meta label for security team label Aug 14, 2020
@ywangd ywangd self-assigned this Aug 14, 2020
@azasypkin
Copy link
Member

cc @elastic/kibana-security just to track that issue - we rely on the shape of this response in many places and our CI will likely go red as soon as we get ES snapshot with this change.

@ywangd
Copy link
Member Author

ywangd commented Aug 14, 2020

The plan is to add a new top level field authentication_type to the response object. Existing fields will not be touched. It will be something like the follows after the change:

{
  "username": "foo@example.com",
  "roles": [
    "kibana_system"
  ],
  "full_name": null,
  "email": null,
  "metadata": {},
  "enabled": true,
  "authentication_realm": {
    "name": "saml1",
    "type": "saml"
  },
  "lookup_realm": {
    "name": "saml1",
    "type": "saml"
  },
  "authentication_type":  "TOKEN"  // <--- new field here
}

I thought Kibana is lenient for extra fields. But maybe my understanding is wrong.

@azasypkin
Copy link
Member

I thought Kibana is lenient for extra fields. But maybe my understanding is wrong.

We're in the main code, it won't break it in any way. But we still have few places in the tests where we check all the fields. Plus we have Typescript interfaces that describe that response. Nothing to worry about, we just need to update few dev/test-only places.

@ywangd
Copy link
Member Author

ywangd commented Aug 21, 2020

@azasypkin Thanks for the insights. The change is now merged to master and backported for v7.10.

@azasypkin
Copy link
Member

@azasypkin Thanks for the insights. The change is now merged to master and backported for v7.10.

Thanks @ywangd ! I'll be on PTO next week, so pinging @elastic/kibana-security in case our CI starts failing (it likely will) next week when new ES snapshot is promoted. I'm mostly concerned with our tests that have the check like this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) Team:Security Meta label for security team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants