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

Preserve REST client auth despite 401 response #30558

Merged
merged 5 commits into from May 17, 2018

Conversation

Projects
None yet
7 participants
@tvernum
Copy link
Contributor

commented May 14, 2018

The default behaviour for Apache HTTP client is to mimic the standard
browser behaviour of clearing the authentication cache (for a given
host) if that host responds with 401.
This behaviour is appropriate in a interactive browser environment
where the user is given the opportunity to provide alternative
credentials, but it is not the preferred behaviour for the ES REST
client.

X-Pack may respond with a 401 status if a request is made before the
node/cluster has recovered sufficient state to know how to handle the
provided authentication credentials - for example the security index
need to be recovered before we can authenticate native users.
In these cases the correct behaviour is to retry with the same
credentials (rather than discarding those credentials).

Preserve REST client auth despite 401 response
The default behaviour for Apache HTTP client is to mimic the standard
browser behaviour of clearing the authentication cache (for a given
host) if that host responds with 401.
This behaviour is appropriate in a interactive browser environment
where the user is given the opportunity to provide alternative
credentials, but it is not the preferred behaviour for the ES REST
client.

X-Pack may respond with a 401 status if a request is made before the
node/cluster has recovered sufficient state to know how to handle the
provided authentication credentials - for example the security index
need to be recovered before we can authenticate native users.
In these cases the correct behaviour is to retry with the same
credentials (rather than discarding those credentials).
@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented May 14, 2018

@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented May 14, 2018

@tvernum

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2018

Relates: #21336

@@ -204,7 +204,8 @@ private CloseableHttpAsyncClient createHttpClient() {
HttpAsyncClientBuilder httpClientBuilder = HttpAsyncClientBuilder.create().setDefaultRequestConfig(requestConfigBuilder.build())
//default settings for connection pooling may be too constraining
.setMaxConnPerRoute(DEFAULT_MAX_CONN_PER_ROUTE).setMaxConnTotal(DEFAULT_MAX_CONN_TOTAL)
.setSSLContext(SSLContext.getDefault());
.setSSLContext(SSLContext.getDefault())
.setTargetAuthenticationStrategy(new PersistentCredentialsAuthenticationStrategy());

This comment has been minimized.

Copy link
@javanna

javanna May 14, 2018

Member

is this good as a default in every case? Thinking of installation without x-pack installed and a different authentication method (e.g proxy or something along those lines)

This comment has been minimized.

Copy link
@tvernum

tvernum May 14, 2018

Author Contributor

I think it is, but I'm keen to hear other opinions.

Given that we default to preemptive authentication it seems right to default to preemptive authentication that actually works.

If the server rejects the supplied credentials, then you're going to need some custom behaviour to provide alternative credentials - which probably means a custom auth strategy, although I think it could be possible to use a custom credentials provider and rely on the clear-cache behaviour.

Note the http client has separate strategies for proxy auth and target auth, though a reverse proxy would look like the target to the client.

@jaymode
Copy link
Member

left a comment

I left two minor comments, OTT LGTM

* The default handler in Apache HTTP client mimics standard browser behaviour of clearing authentication
* credentials if it receives a 401 response from the server. While this can be useful for browser, it is
* rarely the desired behaviour with the Elasticsearch REST API.
* When an Elasticsearch node starts up with X-Pack enabled, the standard behaviour is to respond with a

This comment has been minimized.

Copy link
@jaymode

jaymode May 14, 2018

Member

I'd personally leave x-pack out of the javadocs, even without x-pack I think this is the right default since there needs to be a way to get the proper credentials for a host and for the rest client this needs to be provided by the user as part of configuration.

* what users exist.
* The desired behaviour under these circumstances is for the Rest client to retry with the same credentials.
*/
class PersistentCredentialsAuthenticationStrategy extends TargetAuthenticationStrategy {

This comment has been minimized.

Copy link
@jaymode

jaymode May 14, 2018

Member

make it final?

@tvernum

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2018

@javanna Do you want to discuss this further? I'm happy to talk it through when our timezones overlap.

@javanna

This comment has been minimized.

Copy link
Member

commented May 17, 2018

I am good thanks @tvernum ! Mine was just a question, I have no concerns around your change, thanks for taking care of this.

@tvernum tvernum merged commit 8a89306 into elastic:master May 17, 2018

3 checks passed

CLA Commit author is a member of Elasticsearch
Details
elasticsearch-ci Build finished.
Details
elasticsearch-ci/packaging-sample Build finished.
Details

@tvernum tvernum added backport pending and removed review labels May 17, 2018

martijnvg added a commit that referenced this pull request May 17, 2018

Merge remote-tracking branch 'es/master' into ccr
* es/master: (74 commits)
  Preserve REST client auth despite 401 response (#30558)
  [test] packaging: add windows boxes (#30402)
  Make xpack modules instead of a meta plugin (#30589)
  Mute ShrinkIndexIT
  [ML] DeleteExpiredDataAction should use client with origin (#30646)
  Reindex: Fixed typo in assertion failure message (#30619)
  [DOCS] Fixes list of unconverted snippets in build.gradle
  [DOCS] Reorganizes RBAC documentation
  SQL: Remove dependency for server's version from JDBC driver (#30631)
  Test: increase search logging for LicensingTests
  Adjust serialization version in IndicesOptions
  [TEST] Fix compilation
  Remove version argument in RangeFieldType (#30411)
  Remove unused DirectoryUtils class. (#30582)
  Mitigate date histogram slowdowns with non-fixed timezones. (#30534)
  Add a MovingFunction pipeline aggregation, deprecate MovingAvg agg (#29594)
  Removes AwaitsFix on IndicesOptionsTests
  Template upgrades should happen in a system context (#30621)
  Fix bug in BucketMetrics path traversal (#30632)
  Fixes IndiceOptionsTests to serialise correctly (#30644)
  ...

martijnvg added a commit to martijnvg/elasticsearch that referenced this pull request May 17, 2018

Merge remote-tracking branch 'es/ccr' into ccr_create_and_follow
* es/ccr: (75 commits)
  Preserve REST client auth despite 401 response (elastic#30558)
  [test] packaging: add windows boxes (elastic#30402)
  Make xpack modules instead of a meta plugin (elastic#30589)
  Mute ShrinkIndexIT
  [ML] DeleteExpiredDataAction should use client with origin (elastic#30646)
  Reindex: Fixed typo in assertion failure message (elastic#30619)
  [DOCS] Fixes list of unconverted snippets in build.gradle
  [DOCS] Reorganizes RBAC documentation
  SQL: Remove dependency for server's version from JDBC driver (elastic#30631)
  Test: increase search logging for LicensingTests
  Adjust serialization version in IndicesOptions
  [TEST] Fix compilation
  Remove version argument in RangeFieldType (elastic#30411)
  Remove unused DirectoryUtils class. (elastic#30582)
  Mitigate date histogram slowdowns with non-fixed timezones. (elastic#30534)
  Add a MovingFunction pipeline aggregation, deprecate MovingAvg agg (elastic#29594)
  Removes AwaitsFix on IndicesOptionsTests
  Template upgrades should happen in a system context (elastic#30621)
  Fix bug in BucketMetrics path traversal (elastic#30632)
  Fixes IndiceOptionsTests to serialise correctly (elastic#30644)
  ...

jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request May 17, 2018

Merge remote-tracking branch 'elastic/master' into default-copy-setti…
…ngs-to-true

* elastic/master: (25 commits)
  [DOCS] Replace X-Pack terms with attributes
  [ML] Clean left behind model state docs (elastic#30659)
  Correct typos
  filters agg docs duplicated 'bucket' word removal (elastic#30677)
  top_hits doc example description update (elastic#30676)
  [Docs] Replace InetSocketTransportAddress with TransportAdress (elastic#30673)
  [TEST] Account for increase in ML C++ memory usage (elastic#30675)
  User proper write-once semantics for GCS repository (elastic#30438)
  Remove bogus file accidentally added
  Add detailed assert message to IndexAuditUpgradeIT (elastic#30669)
  Adjust fast forward for token expiration test  (elastic#30668)
  Improve explanation in rescore (elastic#30629)
  Deprecate `nGram` and `edgeNGram` names for ngram filters (elastic#30209)
  Watcher: Fix watch history template for dynamic slack attachments (elastic#30172)
  Fix _cluster/state to always return cluster_uuid (elastic#30656)
  [Tests] Add debug information to CorruptedFileIT
  Preserve REST client auth despite 401 response (elastic#30558)
  [test] packaging: add windows boxes (elastic#30402)
  Make xpack modules instead of a meta plugin (elastic#30589)
  Mute ShrinkIndexIT
  ...

tvernum added a commit that referenced this pull request May 18, 2018

Preserve REST client auth despite 401 response (#30558)
The default behaviour for Apache HTTP client is to mimic the standard
browser behaviour of clearing the authentication cache (for a given
host) if that host responds with 401.
This behaviour is appropriate in a interactive browser environment
where the user is given the opportunity to provide alternative
credentials, but it is not the preferred behaviour for the ES REST
client.

X-Pack may respond with a 401 status if a request is made before the
node/cluster has recovered sufficient state to know how to handle the
provided authentication credentials - for example the security index
need to be recovered before we can authenticate native users.
In these cases the correct behaviour is to retry with the same
credentials (rather than discarding those credentials).

dnhatn added a commit that referenced this pull request May 19, 2018

Merge branch '6.x' into ccr-6.x
* 6.x:
  Mute testCorruptFileThenSnapshotAndRestore
  Plugins: Remove meta plugins (#30670)
  Upgrade to Lucene-7.4.0-snapshot-59f2b7aec2 (#30726)
  Docs: Add uptasticsearch to list of clients (#30738)
  [TEST] Reduce forecast overflow to disk test memory limit (#30727)
  [DOCS] Removes redundant index.asciidoc files (#30707)
  [DOCS] Moves X-Pack configurationg pages in table of contents (#30702)
  [ML][TEST] Fix bucket count assertion in ModelPlotsIT (#30717)
  [ML][TEST] Make AutodetectMemoryLimitIT less fragile (#30716)
  [Build] Add test admin when starting gradle run with trial license and
  [ML] provide tmp storage for forecasting and possibly any ml native jobs #30399
  Tests: Fail if test watches could not be triggered (#30392)
  Watcher: Prevent duplicate watch triggering during upgrade (#30643)
  [ML] add version information in case of crash of native ML process (#30674)
  Add detailed assert message to IndexAuditUpgradeIT (#30669)
  Preserve REST client auth despite 401 response (#30558)
  Make TransportClusterStateAction abide to our style (#30697)
  [DOCS] Fixes edit URLs for stack overview (#30583)
  [DOCS] Add missing callout in IndicesClientDocumentationIT
  Backport get settings API changes to 6.x (#30494)
  Silence sleep based watcher test
  [DOCS] Replace X-Pack terms with attributes
  Improve explanation in rescore (#30629)
  [test] packaging: add windows boxes (#30402)
  [ML] Clean left behind model state docs (#30659)
  filters agg docs duplicated 'bucket' word removal (#30677)
  top_hits doc example description update (#30676)
  MovingFunction Pipeline agg backport to 6.x (#30658)
  [Docs] Replace InetSocketTransportAddress with TransportAdress (#30673)
  [TEST] Account for increase in ML C++ memory usage (#30675)
  User proper write-once semantics for GCS repository (#30438)
  Deprecate `nGram` and `edgeNGram` names for ngram filters (#30209)
  Watcher: Fix watch history template for dynamic slack attachments (#30172)
  Fix _cluster/state to always return cluster_uuid (#30656)

ywelsch added a commit to ywelsch/elasticsearch that referenced this pull request May 23, 2018

Preserve REST client auth despite 401 response (elastic#30558)
The default behaviour for Apache HTTP client is to mimic the standard
browser behaviour of clearing the authentication cache (for a given
host) if that host responds with 401.
This behaviour is appropriate in a interactive browser environment
where the user is given the opportunity to provide alternative
credentials, but it is not the preferred behaviour for the ES REST
client.

X-Pack may respond with a 401 status if a request is made before the
node/cluster has recovered sufficient state to know how to handle the
provided authentication credentials - for example the security index
need to be recovered before we can authenticate native users.
In these cases the correct behaviour is to retry with the same
credentials (rather than discarding those credentials).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.