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

[Security] Check auth scheme case insensitively #31490

Merged
merged 2 commits into from
Jun 22, 2018
Merged

[Security] Check auth scheme case insensitively #31490

merged 2 commits into from
Jun 22, 2018

Conversation

bizybot
Copy link
Contributor

@bizybot bizybot commented Jun 21, 2018 •
edited
Loading

According to RFC 7617, the Basic authentication scheme name should not be case sensitive.
Case insensitive comparisons are also applicable for bearer tokens where Bearer authentication scheme is used as per RFC 6750 and RFC 7235

Some Http clients may send authentication scheme names in different case types for eg. Basic
basic, BASIC, BEARER etc., so the lack of case-insensitive check is an issue when these clients try to authenticate with elasticsearch. This commit adds case-insensitive checks for Basic and Bearer authentication schemes.

Closes #31486

[AuthcHeader] Check auth scheme case insensitively
a230346 
According to [RFC 7617](https://tools.ietf.org/html/rfc7617),
the authorization scheme should not be case sensitive
Applicable to bearer token where "Bearer " auth scheme is used.
Ref:  [RFC 6750](https://tools.ietf.org/html/rfc6750)
[RFC 7235](https://tools.ietf.org/html/rfc7235#page-10)
This is problem for some of HTTP clients using values like
`BASIC`, `basic`, `BEARER` etc.

This commit modifes the check to be case insensitive.

Closes elastic#31486
@bizybot bizybot added >bug v7.0.0 :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) v6.4.0 labels Jun 21, 2018
@bizybot bizybot requested review from tvernum and jaymode June 21, 2018 01:04
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security

@jaymode jaymode changed the title [AuthcHeader] Check auth scheme case insensitively [Security] Check auth scheme case insensitively Jun 21, 2018
@jaymode
Copy link
Member

jaymode commented Jun 21, 2018

@bizybot we need a better description for this PR.

the authorization scheme

this should be the 'Basic' authentication scheme.

Applicable to bearer tokens

This is only a partial thought. We need complete thoughts like Case insensitive comparisons are also applicable for bearer tokens where the 'Bearer' authentication scheme is used per RFC 6750 and RFC 7235.

This is problem for some of HTTP clients using values like
BASIC, basic, BEARER etc.

There is a lot of ambiguity here IMO. Maybe: Clients may send values in various casing, such as Basic, basic, BEARER, Bearer, etc, so the lack of a case insensitive check is an issue when these clients try to authenticate with elasticsearch.

jaymode
jaymode reviewed Jun 21, 2018
View reviewed changes
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/TokenService.java Outdated
&& header.length() > "Bearer ".length()) {
return header.substring("Bearer ".length());
return header.substring("Bearer ".length()).trim();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the trimming per the RFC? If so, we should add a test for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added it as it was referring to RFC 2617 but other than that I could not find explicit mention of this as a requirement so I will remove it.

jaymode
jaymode approved these changes Jun 21, 2018
View reviewed changes
Copy link
Member

@jaymode jaymode left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Please make sure the title and commit message are correct when merging.

@bizybot bizybot merged commit 724438a into elastic:master Jun 22, 2018
@bizybot bizybot deleted the fix/31486 branch June 22, 2018 00:15
dnhatn added a commit that referenced this pull request Jun 23, 2018
@dnhatn
Merge branch 'master' into ccr
08ee9b6 
* master:
  Add get field mappings to High Level REST API Client (#31423)
  [DOCS] Updates Watcher examples for code testing (#31152)
  TEST: Add bwc recovery tests with synced-flush index
  [DOCS] Move sql to docs (#31474)
  [DOCS] Move monitoring to docs folder (#31477)
  Core: Combine doExecute methods in TransportAction (#31517)
  IndexShard should not return null stats (#31528)
  fix repository update with the same settings but different type (#31458)
  Fix Mockito trying to mock IOException that isn't thrown by method (#31433) (#31527)
  Node selector per client rather than per request (#31471)
  Core: Combine messageRecieved methods in TransportRequestHandler (#31519)
  Upgrade to Lucene 7.4.0. (#31529)
  [ML] Add ML filter update API (#31437)
  Allow multiple unicast host providers (#31509)
  Avoid deprecation warning when running the ML datafeed extractor. (#31463)
  REST high-level client: add simulate pipeline API (#31158)
  Get Mapping API to honour allow_no_indices and ignore_unavailable (#31507)
  [PkiRealm] Invalidate cache on role mappings change (#31510)
  [Security] Check auth scheme case insensitively (#31490)
  In NumberFieldType equals and hashCode, make sure that NumberType is taken into account. (#31514)
  [DOCS] Fix REST tests in SQL docs
  [DOCS] Add code snippet testing in more ML APIs (#31339)
  Core: Remove ThreadPool from base TransportAction (#31492)
  [DOCS] Remove fixed file from build.gradle
  Rename createNewTranslog to fileBasedRecovery (#31508)
  Test: Skip assertion on windows
  [DOCS] Creates field and document level security overview (#30937)
  [DOCS] Significantly improve SQL docs
  [DOCS] Move migration APIs to docs (#31473)
  Core: Convert TransportAction.execute uses to client calls (#31487)
  Return transport addresses from UnicastHostsProvider (#31426)
  Ensure local addresses aren't null (#31440)
  Remove unused generic type for client execute method (#31444)
  Introduce http and tcp server channels (#31446)
bizybot added a commit that referenced this pull request Jul 9, 2018
@bizybot
[Security] Check auth scheme case insensitively (#31490)
942d537 
According to RFC 7617, the Basic authentication scheme name
should not be case sensitive.
Case insensitive comparisons are also applicable for the bearer
tokens where Bearer authentication scheme is used as per
RFC 6750 and RFC 7235

Some Http clients may send authentication scheme names in
different case types for eg. Basic, basic, BASIC, BEARER etc.,
so the lack of case-insensitive check is an issue when these
clients try to authenticate with elasticsearch.

This commit adds case-insensitive checks for Basic and Bearer
authentication schemes.

Closes #31486
dnhatn added a commit that referenced this pull request Jul 12, 2018
@dnhatn
Merge branch '6.x' into ccr-6.x
4363363 
* 6.x:
  Force execution of fetch tasks (#31974)
  [TEST] Mute SlackMessageTests.testTemplateRender
  Docs: Explain closing the high level client
  [test] disable java packaging tests for suse
  XContentTests : Insert random fields at random positions (#30867)
  Add Get Snapshots High Level REST API (#31980)
  Fix unreachable error condition in AmazonS3Fixture (#32005)
  [6.x][ML] Ensure immutability of MlMetadata (#31994)
  Add Expected Reciprocal Rank metric (#31891)
  SQL: Add support for single parameter text manipulating functions (#31874)
  muted tests due to #31940
  Work around reported problem in eclipse (#31960)
  Move build integration tests out of :buildSrc project (#31961)
  [Test] Reactive 3rd party tests on CI (#31919)
  Fix assertIngestDocument wrongfully passing (#31913) (#31951)
  SQL: Support for escape sequences (#31884)
  SQL: HAVING clause should accept only aggregates (#31872)
  Docs: fix typo in datehistogram (#31972)
  Switch url repository rest tests to new style requests (#31944)
  Switch reindex tests to new style requests (#31941)
  Switch test framework to new style requests (#31939)
  Docs: Added note about cloud service to installation and getting started
  [DOCS] Removes alternative docker pull example (#31934)
  ingest: date_index_name processor template resolution (#31841)
  Test: fix null failure in watcher test (#31968)
  Watcher: Slack message empty text (#31596)
  Switch low level rest tests to new style Requests (#31938)
  Switch high level rest tests to new style requests (#31937)
  HLREST: Bundle the x-pack protocol project (#31904)
  [ML] Mute test failing due to Java 11 date time format parsing bug (#31899)
  Increase logging level for testStressMaybeFlush
  rolling upgrade should use a replica to prevent relocations while running a scroll
  [test] port archive distribution packaging tests (#31314)
  HLRest: Move xPackInfo() to xPack().info() (#31905)
  Increase logging level for :qa:rolling-upgrade
  Backport: Add template config for Beat state to X-Pack Monitoring (#31809) (#31893)
  Fix building AD URL from domain name (#31849)
  Fix broken NaN check in MovingFunctions#stdDev() (#31888)
  Change trappy float comparison (#31889)
  Add opaque_id to audit logging (#31878)
  add support for is_write_index in put-alias body parsing (#31674)
  Ingest: Enable Templated Fieldnames in Rename (#31690) (#31896)
  Ingest: Add ignore_missing option to RemoveProc (#31693) (#31892)
  [Docs] Fix typo in the Rollup API Quick Reference (#31855)
  Watcher: Add ssl.trust email account setting (#31684)
  [PkiRealm] Invalidate cache on role mappings change (#31510)
  [Security] Check auth scheme case insensitively (#31490)
  HLREST: Add x-pack-info API (#31870)
  Remove link to oss-MSI (#31844)
  Painless: Restructure Definition/Whitelist (#31879)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaymode jaymode jaymode approved these changes

@tvernum tvernum Awaiting requested review from tvernum

Assignees
No one assigned
Labels
>bug :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.
4 participants
@bizybot @elasticmachine @jaymode @colings86