Enhance parsing of StatusCode in SAML Responses #38628
Merged
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
<Status> elements in a failed response might contain two nested <StatusCode> elements. We currently only parse the first one in order to create a message that we attach to the Exception we return and log. However this is generic and only gives out informarion about whether the SAML IDP believes it's an error with the request or if it couldn't handle the request for other reasons. The encapsulated StatusCode has a more interesting error message that potentially gives out the actual error as in Invalid nameid policy, authentication failure etc. This change ensures that we print that information also, and removes Message and Details fields from the message when these are not part of the Status element (which quite often is the case)
jkakavas
added
>enhancement
v6.2.5
:Security/Authentication
|
Pinging @elastic/es-security |
|
That list of version labels doesn't look right:
|
tvernum
requested changes
Feb 11, 2019
| StatusCode subLevel = firstLevel.getStatusCode(); | ||
| StringBuilder sb = new StringBuilder(); | ||
| if (StatusCode.REQUESTER.equals(firstLevel.getValue())) { | ||
| sb.append("The request could not be granted due to an error in the Elastic Stack side ("); |
There was a problem hiding this comment.
I don't think this gives an accurately picture. In reality it's the IdP thinks the Requester did something wrong.
The IdP's assessment might be true (or in the case of ADFS, it's probably not) but in either case if we say ther eis an error in the Elastic Stack side, it implies that we know about a problem, which we don't.
I fear it will lead to users hunting for error messages, or expecting us to explain the error, when we don't know what the problem is (if we did, we wouldn't have made the request).
Suggested change
| sb.append("The request could not be granted due to an error in the Elastic Stack side ("); | |
| sb.append("The SAML IdP did not grant the request. It indicated that the Elastic Stack side sent something invalid ("); |
| sb.append("The request could not be granted because the SAML IDP doesn't support SAML 2.0 ("); | ||
| } else { | ||
| sb.append("The request could not be granted, the SAML IDP responded with a non-standard Status code ("); | ||
|
|
There was a problem hiding this comment.
Nit:
unneccessary
new
line
tvernum
approved these changes
Feb 11, 2019
|
This needs to be labelled for |
jkakavas
added a commit
that referenced
this pull request
Feb 11, 2019
* Enhance parsing of StatusCode in SAML Responses <Status> elements in a failed response might contain two nested <StatusCode> elements. We currently only parse the first one in order to create a message that we attach to the Exception we return and log. However this is generic and only gives out informarion about whether the SAML IDP believes it's an error with the request or if it couldn't handle the request for other reasons. The encapsulated StatusCode has a more interesting error message that potentially gives out the actual error as in Invalid nameid policy, authentication failure etc. This change ensures that we print that information also, and removes Message and Details fields from the message when these are not part of the Status element (which quite often is the case)
jkakavas
added a commit
that referenced
this pull request
Feb 12, 2019
* Enhance parsing of StatusCode in SAML Responses <Status> elements in a failed response might contain two nested <StatusCode> elements. We currently only parse the first one in order to create a message that we attach to the Exception we return and log. However this is generic and only gives out informarion about whether the SAML IDP believes it's an error with the request or if it couldn't handle the request for other reasons. The encapsulated StatusCode has a more interesting error message that potentially gives out the actual error as in Invalid nameid policy, authentication failure etc. This change ensures that we print that information also, and removes Message and Details fields from the message when these are not part of the Status element (which quite often is the case)
jkakavas
added a commit
that referenced
this pull request
Feb 12, 2019
* Enhance parsing of StatusCode in SAML Responses <Status> elements in a failed response might contain two nested <StatusCode> elements. We currently only parse the first one in order to create a message that we attach to the Exception we return and log. However this is generic and only gives out informarion about whether the SAML IDP believes it's an error with the request or if it couldn't handle the request for other reasons. The encapsulated StatusCode has a more interesting error message that potentially gives out the actual error as in Invalid nameid policy, authentication failure etc. This change ensures that we print that information also, and removes Message and Details fields from the message when these are not part of the Status element (which quite often is the case)
jkakavas
added a commit
that referenced
this pull request
Feb 12, 2019
* Enhance parsing of StatusCode in SAML Responses <Status> elements in a failed response might contain two nested <StatusCode> elements. We currently only parse the first one in order to create a message that we attach to the Exception we return and log. However this is generic and only gives out informarion about whether the SAML IDP believes it's an error with the request or if it couldn't handle the request for other reasons. The encapsulated StatusCode has a more interesting error message that potentially gives out the actual error as in Invalid nameid policy, authentication failure etc. This change ensures that we print that information also, and removes Message and Details fields from the message when these are not part of the Status element (which quite often is the case)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
<Status>elements in a failed response might contain two nested<StatusCode>elements. We currently only parse the first one inorder to create a message that we attach to the Exception we return
and log. However this is generic and only gives out information
about whether the SAML IDP believes it's an error with the
request or if it couldn't handle the request for other reasons. The
encapsulated
<StatusCode>has a more interesting error message thatpotentially gives out the actual error as in Invalid NameID policy,
authentication failure etc.
This change ensures that we print the nested information also, and
removes Message and Details fields from the message when these
are not part of the Status element (which quite often is the case)