Compliant SAML Response destination check#31175
Merged
jkakavas merged 4 commits intoelastic:masterfrom Jun 8, 2018
Merged
Conversation
Only validate the Destination element of an incoming SAML Response if the SAML Response is signed. The standard [1] - 3.5.5.2 and [2] - 3.2.2 does mention that the Destination element is optional and should only be verified when the SAML Response is signed. Some Identity Provider implementations are known to not set a Destination XML Attribute in their SAML responses when those are not signed, so this change also aims to enhance interoperability. [1] https://docs.oasis-open.org/security/saml/v2.0/saml-bindings-2.0-os.pdf [2] https://docs.oasis-open.org/security/saml/v2.0/saml-core-2.0-os.pdf
Collaborator
|
Pinging @elastic/es-security |
tvernum
requested changes
Jun 8, 2018
| private void checkResponseDestination(Response response) { | ||
| final String asc = getSpConfiguration().getAscUrl(); | ||
| if (asc.equals(response.getDestination()) == false) { | ||
| if (response.isSigned() && asc.equals(response.getDestination()) == false) { |
Contributor
There was a problem hiding this comment.
I would prefer that we continue to reject present-but-incorrect destinations even for unsigned messages.
So this should be more like
if (asc.equals(response.getDestination()) == false) {
if(response.isSigned() || Strings.hasText(response.getDestination())) {
// ...
tvernum
approved these changes
Jun 8, 2018
dnhatn
added a commit
that referenced
this pull request
Jun 10, 2018
* master: Move default location of dependencies report (#31228) Remove dependencies report task dependencies (#31227) Add recognition of MPL 2.0 (#31226) Fix unknown licenses (#31223) Remove version from license file name for GCS SDK (#31221) Fully encapsulate LocalCheckpointTracker inside of the engine (#31213) [DOCS] Added 'fail_on_unsupported_field' param to MLT. Closes #28008 (#31160) Add licenses for transport-nio (#31218) Remove DocumentFieldMappers#simpleMatchToFullName. (#31041) Allow to trim all ops above a certain seq# with a term lower than X, post backport fix (#31211) Compliant SAML Response destination check (#31175) Remove DocumentFieldMappers#smartNameFieldMapper, as it is no longer needed. (#31018) Remove extraneous references to 'tokenized' in the mapper code. (#31010) Allow to trim all ops above a certain seq# with a term lower than X (#30176) SQL: Make a single JDBC driver jar (#31012) Enhance license detection for various licenses (#31198) [DOCS] Add note about long-lived idle connections (#30990) Move number of language analyzers to analysis-common module (#31143) Default max concurrent search req. numNodes * 5 (#31171) flush job to ensure all results have been written (#31187)
jasontedor
added a commit
to rjernst/elasticsearch
that referenced
this pull request
Jun 10, 2018
…ecker * elastic/master: (309 commits) [test] add fix for rare virtualbox error (elastic#31212) Move default location of dependencies report (elastic#31228) Remove dependencies report task dependencies (elastic#31227) Add recognition of MPL 2.0 (elastic#31226) Fix unknown licenses (elastic#31223) Remove version from license file name for GCS SDK (elastic#31221) Fully encapsulate LocalCheckpointTracker inside of the engine (elastic#31213) [DOCS] Added 'fail_on_unsupported_field' param to MLT. Closes elastic#28008 (elastic#31160) Add licenses for transport-nio (elastic#31218) Remove DocumentFieldMappers#simpleMatchToFullName. (elastic#31041) Allow to trim all ops above a certain seq# with a term lower than X, post backport fix (elastic#31211) Compliant SAML Response destination check (elastic#31175) Remove DocumentFieldMappers#smartNameFieldMapper, as it is no longer needed. (elastic#31018) Remove extraneous references to 'tokenized' in the mapper code. (elastic#31010) Allow to trim all ops above a certain seq# with a term lower than X (elastic#30176) SQL: Make a single JDBC driver jar (elastic#31012) Enhance license detection for various licenses (elastic#31198) [DOCS] Add note about long-lived idle connections (elastic#30990) Move number of language analyzers to analysis-common module (elastic#31143) Default max concurrent search req. numNodes * 5 (elastic#31171) ...
jkakavas
added a commit
that referenced
this pull request
Jun 11, 2018
Make SAML Response Destination check compliant Only validate the Destination element of an incoming SAML Response if Destination is present and the SAML Response is signed. The standard [1] - 3.5.5.2 and [2] - 3.2.2 does mention that the Destination element is optional and should only be verified when the SAML Response is signed. Some Identity Provider implementations are known to not set a Destination XML Attribute in their SAML responses when those are not signed, so this change also aims to enhance interoperability. [1] https://docs.oasis-open.org/security/saml/v2.0/saml-bindings-2.0-os.pdf [2] https://docs.oasis-open.org/security/saml/v2.0/saml-core-2.0-os.pdf
Contributor
|
@jkakavas This may be backported now. |
jkakavas
added a commit
that referenced
this pull request
Jun 14, 2018
Make SAML Response Destination check compliant Only validate the Destination element of an incoming SAML Response if Destination is present and the SAML Response is signed. The standard [1] - 3.5.5.2 and [2] - 3.2.2 does mention that the Destination element is optional and should only be verified when the SAML Response is signed. Some Identity Provider implementations are known to not set a Destination XML Attribute in their SAML responses when those are not signed, so this change also aims to enhance interoperability. [1] https://docs.oasis-open.org/security/saml/v2.0/saml-bindings-2.0-os.pdf [2] https://docs.oasis-open.org/security/saml/v2.0/saml-core-2.0-os.pdf
dnhatn
added a commit
that referenced
this pull request
Jun 14, 2018
* 6.x: SQL: Fix build on Java 10 [Tests] Mutualize fixtures code in BaseHttpFixture (#31210) [TEST] Fix RemoteClusterClientTests#testEnsureWeReconnect [ML] Update test thresholds to account for changes to memory control (#31289) Reenable Checkstyle's unused import rule (#31270) [ML] Check licence when datafeeds use cross cluster search (#31247) Fix non-REST doc snippet [DOC] Extend SQL docs [DOCS] Shortens ML API intros Use quotes in the call invocation (#31249) move security ingest processors to a sub ingest directory (#31306) SQL: Whitelist SQL utility class for better scripting (#30681) Add 5.6.11 version constant. Fix version detection. [Docs] All Rollup docs experimental, agg limitations, clarify DeleteJob (#31299) Add missing release notes. Security: fix token bwc with pre 6.0.0-beta2 (#31254) Fix compilation error in UpdateSettingsIT (#31304) Test: Remove broken yml test feature (#31255) Add unreleased version 6.3.1 [Rollup] Metric config parser must use builder so validation runs (#31159) Removes experimental tag from scripted_metric aggregation (#31298) [DOCS] Removes coming tag from 6.3.0 release notes 6.3 release notes. Add notion of internal index settings (#31286) REST high-level client: add Cluster Health API (#29331) Remove leftover usage of deprecated client API SyncedFlushResponse to implement ToXContentObject (#31155) Add Get Aliases API to the high-level REST client (#28799) HLRest: Add get index templates API (#31161) Log warnings when cluster state publication failed to some nodes (#31233) Fix AntFixture waiting condition (#31272) [TEST] Mute RecoveryIT.testHistoryUUIDIsGenerated Ignore numeric shard count if waiting for ALL (#31265) Update checkstyle to 8.10.1 (#31269) Set analyzer version in PreBuiltAnalyzerProviderFactory (#31202) Revert upgrade to Netty 4.1.25.Final (#31282) Use armored input stream for reading public key (#31229) [DOCS] Added 'fail_on_unsupported_field' param to MLT. Closes #28008 (#31160) Fix Netty 4 Server Transport tests. Again. [DOCS] Fixed typo. [DOCS] Added release highlights for 6.3 (#31256) [DOCS] Mark SQL feature as experimental [DOCS] Updates machine learning custom URL screenshots (#31222) Fix naming conventions check for XPackTestCase Fix security Netty 4 transport tests Fix race in clear scroll (#31259) [DOCS] Clarify audit index settings when remote indexing (#30923) [ML][TEST] Mute tests using rules (#31204) Support RequestedAuthnContext (#31238) Validate xContentType in PutWatchRequest. (#31088) [INGEST] Interrupt the current thread if evaluation grok expressions take too long (#31024) Upgrade to Netty 4.1.25.Final (#31232) Suppress extras FS on caching directory tests Revert "[DOCS] Added 6.3 info & updated the upgrade table. (#30940)" Revert "Fix snippets in upgrade docs" Fix snippets in upgrade docs [DOCS] Added 6.3 info & updated the upgrade table. (#30940) Enable custom credentials for core REST tests (#31235) Move ESIndexLevelReplicationTestCase to test framework (#31243) Encapsulate Translog in Engine (#31220) [DOCS] Adds machine learning 6.3.0 release notes (#31217) Remove all unused imports and fix CRLF (#31207) [TEST] Fix testRecoveryAfterPrimaryPromotion [Docs] Remove mention pattern files in Grok processor (#31170) Use stronger write-once semantics for Azure repository (#30437) Don't swallow exceptions on replication (#31179) Compliant SAML Response destination check (#31175) Move java version checker back to its own jar (#30708) TEST: Retry synced-flush if ongoing ops on primary (#30978) [test] add fix for rare virtualbox error (#31212)
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
6 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.
Only validate the Destination element of an incoming SAML Response
if the SAML Response is signed.
The standard [1] - 3.5.5.2 and [2] - 3.2.2 does mention that the
Destination element is optional and should only be verified when
the SAML Response is signed. Some Identity Provider implementations
are known to not set a Destination XML Attribute in their SAML
responses when those are not signed, so this change also aims to
enhance interoperability.
[1] https://docs.oasis-open.org/security/saml/v2.0/saml-bindings-2.0-os.pdf
[2] https://docs.oasis-open.org/security/saml/v2.0/saml-core-2.0-os.pdf