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

SAML: Process only verified signed data #30420

Merged
merged 2 commits into from
May 10, 2018
Merged

SAML: Process only verified signed data #30420

merged 2 commits into from
May 10, 2018
+12 −14

Conversation

jkakavas
Copy link
Member

@jkakavas jkakavas commented May 7, 2018

As conformance to best practices, this changes ensures that if a
SAML Response is signed, we verify the signature before processing
it any further. We were only checking the InResponseTo and
Destination attributes before potential signature validation but
there was no reason to do that up front either.

@jkakavas
Process only signed data
f9af929 
As conformance to best practices, this changes ensures that if a
SAML Response is signed, we verify the signature before processing
it any further. We were only checking the InResponseTo and
Destination attributes before potential signature validation but
there was no reason to do that up front either.
@jkakavas jkakavas added >non-issue review v7.0.0 :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) v6.4.0 v6.3.1 labels May 7, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security

@jkakavas jkakavas requested a review from tvernum May 7, 2018 12:34
tvernum
tvernum approved these changes May 9, 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

@tvernum
Copy link
Contributor

tvernum commented May 9, 2018

The reason I chose to validate the issuer first is that an incorrect issuer is very likely to cause a signature failure, but would produce a clearer message (particularly in the case where we have multiple SAML realms, and the non-matching realm can quickly & clearly skip messages from other issuers)

I'm happy to change it on your recommendations, but there was method to my madness.

@jkakavas
Copy link
Member Author

jkakavas commented May 9, 2018

You mean Destination, right? The Issuer was checked after the Response Signature was validated.

The useful error message argument still stands for InRepsonseTo and Destination checking might offer more helpful messages in a multiple saml realm setup, but
a) I don't think this is a setup that we'll be seeing much.
b) I think it's better to future-proof our approach from a security perspective, even if it incurs a minor hit to ease of troubleshooting.

@tvernum
Copy link
Contributor

tvernum commented May 9, 2018

You mean Destination, right? The Issuer was checked after the Response Signature was validated.

I guess I do. It was a while ago :)
I don't especially care - but the premise was if things are totally misconfigured and we're getting the wrong messages, provide a useful error message.

@jkakavas jkakavas merged commit 4b319d7 into elastic:master May 10, 2018
dnhatn added a commit that referenced this pull request May 10, 2018
@dnhatn
Merge branch 'master' into ccr
a5be414 
* master:
  Upgrade to Lucene-7.4-snapshot-6705632810 (#30519)
  add version compatibility from 6.4.0 after backport, see #30319 (#30390)
  Security: Simplify security index listeners (#30466)
  Add proper longitude validation in geo_polygon_query (#30497)
  Remove Discovery.AckListener.onTimeout() (#30514)
  Build: move generated-resources to build (#30366)
  Reindex: Fold "with all deps" project into reindex (#30154)
  Isolate REST client single host tests (#30504)
  Solve Gradle deprecation warnings around shadowJar (#30483)
  SAML: Process only signed data (#30420)
  Remove BWC repository test (#30500)
  Build: Remove xpack specific run task (#30487)
  AwaitsFix IntegTestZipClientYamlTestSuiteIT#indices.split tests
  LLClient: Add setJsonEntity (#30447)
  Expose CommonStatsFlags directly in IndicesStatsRequest. (#30163)
  Silence IndexUpgradeIT test failures. (#30430)
  Bump Gradle heap to 1792m (#30484)
  [docs] add warning for read-write indices in force merge documentation (#28869)
  Avoid deadlocks in cache (#30461)
  Test: remove hardcoded list of unconfigured ciphers (#30367)
  mute SplitIndexIT due to #30416
  Docs: Test examples that recreate lang analyzers  (#29535)
  BulkProcessor to retry based on status code (#29329)
  Add GET Repository High Level REST API (#30362)
  add a comment explaining the need for RetryOnReplicaException on missing mappings
  Add `coordinating_only` node selector (#30313)
  Stop forking groovyc (#30471)
  Avoid setting connection request timeout (#30384)
  Use date format in `date_range` mapping before fallback to default (#29310)
  Watcher: Increase HttpClient parallel sent requests (#30130)

# Conflicts:
#	x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/LocalStateCompositeXPackPlugin.java
martijnvg added a commit to martijnvg/elasticsearch that referenced this pull request May 14, 2018
@martijnvg
Merge remote-tracking branch 'es/ccr' into ccr_security
6f969f5 
* es/ccr: (78 commits)
  Upgrade to Lucene-7.4-snapshot-6705632810 (elastic#30519)
  add version compatibility from 6.4.0 after backport, see elastic#30319 (elastic#30390)
  Security: Simplify security index listeners (elastic#30466)
  Add proper longitude validation in geo_polygon_query (elastic#30497)
  Remove Discovery.AckListener.onTimeout() (elastic#30514)
  Build: move generated-resources to build (elastic#30366)
  Reindex: Fold "with all deps" project into reindex (elastic#30154)
  Isolate REST client single host tests (elastic#30504)
  Solve Gradle deprecation warnings around shadowJar (elastic#30483)
  SAML: Process only signed data (elastic#30420)
  Remove BWC repository test (elastic#30500)
  Build: Remove xpack specific run task (elastic#30487)
  AwaitsFix IntegTestZipClientYamlTestSuiteIT#indices.split tests
  Enable soft-deletes in v6.4
  LLClient: Add setJsonEntity (elastic#30447)
  [CCR] Read changes from Lucene instead of translog (elastic#30120)
  Expose CommonStatsFlags directly in IndicesStatsRequest. (elastic#30163)
  Silence IndexUpgradeIT test failures. (elastic#30430)
  Bump Gradle heap to 1792m (elastic#30484)
  [docs] add warning for read-write indices in force merge documentation (elastic#28869)
  ...
jkakavas added a commit to jkakavas/elasticsearch that referenced this pull request May 16, 2018
@jkakavas
SAML: Process only signed data (elastic#30420)
84e3f54 
As conformance to best practices, this changes ensures that if a
SAML Response is signed, we verify the signature before processing
it any further. We were only checking the InResponseTo and
Destination attributes before potential signature validation but
there was no reason to do that up front either.
jkakavas added a commit that referenced this pull request May 16, 2018
@jkakavas
SAML: Process only signed data (#30420)
75ecf58 
As conformance to best practices, this changes ensures that if a
SAML Response is signed, we verify the signature before processing
it any further. We were only checking the InResponseTo and
Destination attributes before potential signature validation but
there was no reason to do that up front either.
@jpountz
Copy link
Contributor

jpountz commented Jun 13, 2018

@jkakavas I'm not seeing this PR on the 6.3 branch, is this PR mislabeled? I removed the 6.3.1 label for now.

@jpountz jpountz removed the v6.3.1 label Jun 13, 2018
@jkakavas jkakavas deleted the saml-best-practice branch September 14, 2018 06:57
@colings86 colings86 removed the v7.0.0 label Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tvernum tvernum tvernum approved these changes

Assignees
No one assigned
Labels
>non-issue :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) v6.4.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jkakavas @elasticmachine @tvernum @jpountz @colings86 @bleskes