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

Document SAML APIs #45105

Open
wants to merge 19 commits into
base: master
from

Conversation

@jkakavas
Copy link
Contributor

commented Aug 1, 2019

This change adds documentation for the SAML APIs in Elasticsearch
and adds simple instructions on how these APIs can be used to
authenticate a user with SAML by a custom web application other
than Kibana.

Resolves: #40352

jkakavas added 2 commits Aug 1, 2019
Document SAML APIs
This change adds documentation for the SAML APIs in Elasticsearch
and adds simple instructions on how these APIs can be used to
authenticate a user with SAML by a custom web application other
than Kibana.

Resolves: #40352
@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Aug 1, 2019

@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Aug 1, 2019

@jkakavas

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

@elasticmachine update branch

elasticmachine and others added 3 commits Aug 1, 2019
@jkakavas

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

@elasticmachine test this please

@jkakavas

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

java.lang.AssertionError: Failure at [painless/10_basic:109]: watch_record.state didn't match expected value:
            watch_record.state: expected String [executed] but was String [not_executed_already_queued]

@elasticmachine run elasticsearch-ci/2

Base64 encoded XML document.

`ids`::
(array) A json array with all the valid SAML Request Ids that the caller of

This comment has been minimized.

Copy link
@lcawl

lcawl Aug 2, 2019

Contributor
Suggested change
(array) A json array with all the valid SAML Request Ids that the caller of
(Required, array) A json array with all the valid SAML Request Ids that the caller of
`username`::
(string) The authenticated user's name.
`expires_in`::
The amount of time (in seconds) left until the token expires.

This comment has been minimized.

Copy link
@lcawl

lcawl Aug 2, 2019

Contributor

I wasn't sure what data type to put for this property. Integer?

@lcawl

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

I altered the API reference content to align with our new template (https://github.com/elastic/docs/blob/master/shared/api-ref-ex.asciidoc).

jkakavas and others added 2 commits Aug 5, 2019
Apply suggestions from code review
Co-Authored-By: Lisa Cawley <lcawley@elastic.co>
@jkakavas

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

@elasticmachine update branch

@tvernum
Copy link
Contributor

left a comment

I didn't quite get through all this, but I left some comments.


The SAML response can be a response to a SAML authentication request that was
previously created using the
<<security-api-saml-prepare-authentication, SAML prepare authentication API>>.

This comment has been minimized.

Copy link
@tvernum

tvernum Aug 6, 2019

Contributor

I find this sentence hard to parse (too many request / response terms, I think).
I don't have a suggestion right now, but since it's the first sentence in the body, it's probably worth trying to make it clear.

This comment has been minimized.

Copy link
@jkakavas

jkakavas Aug 16, 2019

Author Contributor

I have no good alternatives either :/ The fact that we have two pairs of requests/responses we need to talk about here, doesn't help. I broke down the sentence a bit hoping this makes it clearer but I'm open to suggestions

This comment has been minimized.

Copy link
@tvernum

tvernum Sep 12, 2019

Contributor

I think that the issue might be that we call the content from the IdP a response.
It could be clearer to call it either a SAML message (or payload, or content, or something of that nature) or a <Response>.
We might get somewhere if we talk about it as a SAML message and then have a separate sentence/paragraph that states that the SAML message must be a (base 64 encoded) XML document with a root element of <Response>.

}
--------------------------------------------------
// CONSOLE
// TEST[skip:handled in IT]

This comment has been minimized.

Copy link
@tvernum

tvernum Aug 6, 2019

Contributor

The IT doesn't validate that the example in the docs is correct.
But, I don't know how we can test this well since it needs a token which needs a valid SAML authc response ...

@jkakavas jkakavas requested review from tvernum and lcawl Aug 16, 2019

@jkakavas

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

ci1 hit #45605

@elasticmachine run elasticsearch-ci/1

[[security-api-saml-authenticate]]
=== SAML authenticate API

Submits a SAML response message to {es} for consumption when using a custom web application other than {kib}.

This comment has been minimized.

Copy link
@lcawl

lcawl Aug 28, 2019

Contributor

The current sentence seems to imply that this API is only used when you're not using Kibana, whereas the final paragraph in the description indicates the API is also used under the covers by Kibana. I think we should simplify the introductory statement to avoid confusion.

Suggested change
Submits a SAML response message to {es} for consumption when using a custom web application other than {kib}.
Submits a SAML response message to {es}.

This comment has been minimized.

Copy link
@jkakavas

jkakavas Aug 30, 2019

Author Contributor

we want something really high up to be a warning for users that if they want to configure SAML , they should not come here but go to our SAML Guide. They should only need to care about this API if they have a custom web application that they integrate with Elasticsearch and want to use SAML.

I'm open to ideas on how to write this to be obvious :)

This comment has been minimized.

Copy link
@lcawl

lcawl Aug 30, 2019

Contributor

Maybe something like this?:

Suggested change
Submits a SAML response message to {es} for consumption when using a custom web application other than {kib}.
Submits a SAML response message to {es}.
TIP: If you have a custom web application, you can use this API to authenticate against {es} with SAML. It is also used internally by {kib}; see
{stack-ov}/saml-kibana.html[Configuring SAML in {kib}].

This comment has been minimized.

Copy link
@jkakavas

jkakavas Sep 2, 2019

Author Contributor

you can use this API to authenticate against

It's unfortunately a little more complicated than that. Both when kibana uses the APIs and in the case of a custom web app, the caller of the API would need to call both the prepare and the authenticate API in sequence , in order to authenticate.

It is also used internally by {kib}; see {stack-ov}/saml-kibana.html[Configuring SAML in {kib}].

This also sounds like the kibana docs where we link them to , explain this API , or how kibana uses it, which is not the case.

We don't want to document this API as a further detail on how Kibana works with Elasticsearch in order to provide SAML authentication for our users, but explicitly for custom web applications that want to take advantage of our SAML implementation in Elasticsearch. As such, it's a little more than a TIP here, it is the actual purpose of this API doc.

This comment has been minimized.

Copy link
@lcawl

lcawl Sep 3, 2019

Contributor

Okay! What about something like this:

Suggested change
Submits a SAML response message to {es} for consumption when using a custom web application other than {kib}.
Submits a SAML response message to {es}. This API is intended for use by custom web applications.

This comment has been minimized.

Copy link
@jkakavas

jkakavas Sep 4, 2019

Author Contributor

I used this but in a NOTE: in the current version. I don't know if it's too much shouting but one of the concerns we have had wrt documenting this APIs is that users will get mixed up and think they need to use this for SAML Authentication with our stack. So I'd rather err on the side of being too explicit about it :)


[[security-api-saml-authenticate-desc]]
==== {api-description-title}

This comment has been minimized.

Copy link
@lcawl

lcawl Aug 28, 2019

Contributor

If we simplify the introductory sentence, I think we can move the other details to the Description. For example, you could move the following sentence to the beginning of the description:

"{kib} uses this API to provide SAML based authentication. You can also use it, however, in custom web applications or clients. For more information about configuring SAML for use in Kibana, see the {stack-ov}/saml-guide.html[SAML guide]."

This comment has been minimized.

Copy link
@jkakavas

jkakavas Aug 30, 2019

Author Contributor

see above. I'm not sure if this is straightforward enough for users.

@jkakavas jkakavas requested a review from lcawl Aug 30, 2019

@colings86 colings86 added v7.5.0 and removed v7.4.0 labels Aug 30, 2019


The SAML response message that is submitted can be:

* a response to a SAML authentication request that was previously created using the

This comment has been minimized.

Copy link
@lcawl

lcawl Sep 3, 2019

Contributor

Maybe it's easier to parse if it's split into two sentences. For example, something like this:

Suggested change
* a response to a SAML authentication request that was previously created using the
* A response to a SAML authentication request. The response must be generated by the

This comment has been minimized.

Copy link
@jkakavas

jkakavas Sep 4, 2019

Author Contributor

It's actually the authentication request that was generated, not the response. This does get too complicated :(

@jkakavas jkakavas requested a review from lcawl Sep 4, 2019

@jkakavas

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2019

@tvernum this is by no means urgent ,just doing a cleanup round in my PRs. This is ready for review


The SAML response can be a response to a SAML authentication request that was
previously created using the
<<security-api-saml-prepare-authentication, SAML prepare authentication API>>.

This comment has been minimized.

Copy link
@tvernum

tvernum Sep 12, 2019

Contributor

I think that the issue might be that we call the content from the IdP a response.
It could be clearer to call it either a SAML message (or payload, or content, or something of that nature) or a <Response>.
We might get somewhere if we talk about it as a SAML message and then have a separate sentence/paragraph that states that the SAML message must be a (base 64 encoded) XML document with a root element of <Response>.

@jkakavas jkakavas requested a review from tvernum Sep 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.