Support Preemptive Authentication with RestClient #21336

pickypg · 2016-11-04T14:14:13Z

This adds the necessary AuthCache needed to support preemptive authentication. By adding every host to the cache, the automatically added RequestAuthCache interceptor will add credentials on the first pass rather than waiting to do it after each anonymous request is rejected (thus always sending everything twice when basic auth is required).

It's easy to test with/without this using a nodejs proxy. package.json:

{
  "name": "proxy-listener",
  "version": "1.0.0",
  "description": "Proxy Listener will print METHOD URL [AUTHORIZATION CONTENT-LENGTH]",
  "main": "proxy.js",
  "dependencies": {
    "http": "^0.0.0",
    "http-proxy": "^1.15.2"
  },
  "devDependencies": {},
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "author": "",
  "license": "ISC"
}

Using npm, install necessary dependencies

npm install

Then run the proxy to intercept comms:

node proxy.js

proxy.js:

var http = require('http'),
    httpProxy = require('http-proxy');
    
// Create a new instance of HttProxy to use in your server 
var proxy = httpProxy.createProxyServer({});
 
// Create a regular http server and proxy its handler 
http.createServer(function (req, res) {
  console.log(req.method + ' ' + req.url + ' [' + req.headers['authorization'] + ', ' + req.headers['content-length'] + ']');

  proxy.web(req, res, { target: 'http://localhost:9250' });
}).listen(9550);

console.log('Listening at 9550');

/cc @javanna

pickypg · 2016-11-04T14:21:53Z

client/rest/src/main/java/org/elasticsearch/client/RestClient.java

        Set<HttpHost> httpHosts = new HashSet<>();
        for (HttpHost host : hosts) {
            Objects.requireNonNull(host, "host cannot be null");
            httpHosts.add(host);
+            authCache.put(host, new BasicScheme());


This should probably be volatile and work in the same way as hosts. Built, then set.

I wanted to avoid adding an unnecessary volatile variable; BasicAuthCache is thread safe, but clearing it technically means that an async request could be sent that then does not use preemptive authorization while the new hosts are being set.

volatile reads are cheap on x86; IIRC mapping to a no-op. I'd prefer volatile; but there is still the issue that there will be a small amount of time when hosts and the auth cache are not in sync (between setting both).

I do not think we should hardcode the scheme. This makes the assumption that every host uses preemptive basic auth if basic auth credentials are provided. Some requests may not need preemptive authentication. For example, requests could be made with anonymous access, while others from the same client may require authentication. In that scenario you don't want/need preemptive auth but we are forcing it.

I think this should be opt-in like the default behavior of HttpClient. We can do this by providing the ability to specify a callback in the builder that when given a HttpHost it will provide the appropriate scheme.

I checked with the other HTTP-based ES clients, and they all do it this way. So I think it's best if we match their behavior.

For anyone that does want to opt-out, it's very simple to do with the existing client builder by invoking:

httpClientBuilder.disableAuthCaching();

The better approach for users that expect the current behavior is to only supply credentials when they want to use them. This will give the general user the more common approach of using credentials when they're supplied rather than doubling the number of requests for the average auth-based user.

but there is still the issue that there will be a small amount of time when hosts and the auth cache are not in sync (between setting both).

I'll make them a volatile Tuple that get updated together. That way there's no opportunity for a dirty read.

jaymode · 2016-11-04T14:32:44Z

client/rest/src/main/java/org/elasticsearch/client/RestClient.java

        Set<HttpHost> httpHosts = new HashSet<>();
        for (HttpHost host : hosts) {
            Objects.requireNonNull(host, "host cannot be null");
            httpHosts.add(host);
+            authCache.put(host, new BasicScheme());


volatile reads are cheap on x86; IIRC mapping to a no-op. I'd prefer volatile; but there is still the issue that there will be a small amount of time when hosts and the auth cache are not in sync (between setting both).

jaymode · 2016-11-04T14:58:20Z

client/rest/src/main/java/org/elasticsearch/client/RestClient.java

        Set<HttpHost> httpHosts = new HashSet<>();
        for (HttpHost host : hosts) {
            Objects.requireNonNull(host, "host cannot be null");
            httpHosts.add(host);
+            authCache.put(host, new BasicScheme());


I do not think we should hardcode the scheme. This makes the assumption that every host uses preemptive basic auth if basic auth credentials are provided. Some requests may not need preemptive authentication. For example, requests could be made with anonymous access, while others from the same client may require authentication. In that scenario you don't want/need preemptive auth but we are forcing it.

I think this should be opt-in like the default behavior of HttpClient. We can do this by providing the ability to specify a callback in the builder that when given a HttpHost it will provide the appropriate scheme.

pickypg · 2017-01-23T07:42:38Z

@nik9000 I've updated as I mentioned before. @jaymode FYI, I've changed it to be volatile-friendly now.

pickypg · 2017-01-23T21:07:58Z

please test this

jaymode · 2017-01-24T12:52:46Z

client/rest/src/main/java/org/elasticsearch/client/RestClient.java

@@ -122,11 +126,13 @@ public synchronized void setHosts(HttpHost... hosts) {
            throw new IllegalArgumentException("hosts must not be null nor empty");
        }
        Set<HttpHost> httpHosts = new HashSet<>();
+        AuthCache authCache = new BasicAuthCache();


Hmm, I think this might not be desired behavior. On every sniff the auth cache is invalidated completely. I think it would be better to add the hosts to the set. Then we should do a set difference to find the hosts that have been added and a set difference to find the hosts that have been removed. The added hosts get a new entry in the authcache and the removed ones get removed from the cache

If we update the AuthCache rather than replace it, then it will potentially impact an ongoing request. I checked out the BasicAuthCache and BasicScheme code and it looks pretty lightweight?

You're right. We do not need to do this

This adds the necessary `AuthCache` needed to support preemptive authorization. By adding every host to the cache, the automatically added `RequestAuthCache` interceptor will add credentials on the first pass rather than waiting to do it after _each_ anonymous request is rejected (thus always sending everything twice when basic auth is required). Note: This currently has no tests because I wanted to make sure that my approach was sound. I did test it locally and it worked as expected.

This adds the necessary `AuthCache` needed to support preemptive authorization. By adding every host to the cache, the automatically added `RequestAuthCache` interceptor will add credentials on the first pass rather than waiting to do it after _each_ anonymous request is rejected (thus always sending everything twice when basic auth is required).

pickypg · 2017-01-24T16:45:35Z

5.3: bc5d37b

pickypg added >enhancement v6.0.0-alpha1 :Clients/Java Low Level REST Client Minimal dependencies Java Client for Elasticsearch v5.1.1 labels Nov 4, 2016

pickypg commented Nov 4, 2016

View reviewed changes

pickypg changed the title ~~Support Preemptive Authorization with RestClient~~ Support Preemptive Authentication with RestClient Nov 4, 2016

jaymode requested changes Nov 4, 2016

View reviewed changes

pickypg added v5.2.0 and removed v5.1.1 labels Nov 29, 2016

pickypg removed the v5.2.0 label Jan 3, 2017

nik9000 mentioned this pull request Jan 10, 2017

Reindex-from-remote should use preemptive authorization if username and password are configured #22328

Closed

pickypg force-pushed the feature/use-preemptive-auth branch from 2efc089 to 56a5e38 Compare January 23, 2017 07:38

pickypg added the v5.3.0 label Jan 23, 2017

pickypg force-pushed the feature/use-preemptive-auth branch from d265585 to 7b70f01 Compare January 24, 2017 06:15

jaymode requested changes Jan 24, 2017

View reviewed changes

jaymode approved these changes Jan 24, 2017

View reviewed changes

pickypg and others added 7 commits January 24, 2017 09:30

Update docs to include how to disable preemptive authorization

d269925

authorization -> authentication

ca283fe

Use tuple for thread safety

aac1f39

Add preemptive auth integration tests

389bd6f

Fix line length after adding final

02e96ff

Update reindex tests with new parameter for mocked method

e7075ac

pickypg force-pushed the feature/use-preemptive-auth branch from 7b70f01 to e7075ac Compare January 24, 2017 14:50

pickypg merged commit f0f75b1 into elastic:master Jan 24, 2017

pickypg deleted the feature/use-preemptive-auth branch January 24, 2017 16:34

tvernum mentioned this pull request May 14, 2018

Preserve REST client auth despite 401 response #30558

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Support Preemptive Authentication with RestClient #21336

Support Preemptive Authentication with RestClient #21336

Uh oh!

pickypg commented Nov 4, 2016 •

edited

Loading

Uh oh!

pickypg Nov 4, 2016

Uh oh!

jaymode Nov 4, 2016

Uh oh!

jaymode Nov 4, 2016

Uh oh!

pickypg Nov 29, 2016

Uh oh!

pickypg Nov 29, 2016

Uh oh!

jaymode Nov 4, 2016

Uh oh!

jaymode Nov 4, 2016

Uh oh!

pickypg commented Jan 23, 2017

Uh oh!

pickypg commented Jan 23, 2017

Uh oh!

jaymode Jan 24, 2017

Uh oh!

pickypg Jan 24, 2017

Uh oh!

jaymode Jan 24, 2017

Uh oh!

pickypg commented Jan 24, 2017

Uh oh!

Uh oh!

Support Preemptive Authentication with RestClient #21336

Support Preemptive Authentication with RestClient #21336

Uh oh!

Conversation

pickypg commented Nov 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pickypg commented Jan 23, 2017

Uh oh!

pickypg commented Jan 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pickypg commented Jan 24, 2017

Uh oh!

Uh oh!

pickypg commented Nov 4, 2016 •

edited

Loading