-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Only proxy whitelisted request headers to ES server upstream #6896
Changes from all commits
9b268cf
1ed0a25
b102e26
b635b2c
e56e98f
ad5d772
5cca6f7
5f34d68
836c740
81631ff
f9f4b79
7f75efb
2149f84
eeedc54
c5ce30f
3045117
89dfbee
65ddf96
028db3f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,76 @@ | ||
import expect from 'expect.js'; | ||
import mapUri from '../map_uri'; | ||
import sinon from 'sinon'; | ||
|
||
describe('plugins/elasticsearch', function () { | ||
describe('lib/map_uri', function () { | ||
|
||
let request; | ||
|
||
beforeEach(function () { | ||
request = { | ||
path: '/elasticsearch/some/path', | ||
headers: { | ||
cookie: 'some_cookie_string', | ||
'accept-encoding': 'gzip, deflate', | ||
origin: 'https://localhost:5601', | ||
'content-type': 'application/json', | ||
'x-my-custom-header': '42', | ||
accept: 'application/json, text/plain, */*', | ||
authorization: '2343d322eda344390fdw42' | ||
} | ||
}; | ||
}); | ||
|
||
it('only sends the whitelisted request headers', function () { | ||
|
||
const get = sinon.stub() | ||
.withArgs('elasticsearch.url').returns('http://foobar:9200') | ||
.withArgs('elasticsearch.requestHeadersWhitelist').returns(['x-my-custom-HEADER', 'Authorization']); | ||
const config = function () { return { get: get }; }; | ||
const server = { | ||
config: config | ||
}; | ||
|
||
mapUri(server)(request, function (err, upstreamUri, upstreamHeaders) { | ||
expect(err).to.be(null); | ||
expect(upstreamHeaders).to.have.property('authorization'); | ||
expect(upstreamHeaders).to.have.property('x-my-custom-header'); | ||
expect(Object.keys(upstreamHeaders).length).to.be(2); | ||
}); | ||
}); | ||
|
||
it('sends no headers if whitelist is set to []', function () { | ||
|
||
const get = sinon.stub() | ||
.withArgs('elasticsearch.url').returns('http://foobar:9200') | ||
.withArgs('elasticsearch.requestHeadersWhitelist').returns([]); | ||
const config = function () { return { get: get }; }; | ||
const server = { | ||
config: config | ||
}; | ||
|
||
mapUri(server)(request, function (err, upstreamUri, upstreamHeaders) { | ||
expect(err).to.be(null); | ||
expect(Object.keys(upstreamHeaders).length).to.be(0); | ||
}); | ||
}); | ||
|
||
it('sends no headers if whitelist is set to no value', function () { | ||
|
||
const get = sinon.stub() | ||
.withArgs('elasticsearch.url').returns('http://foobar:9200') | ||
.withArgs('elasticsearch.requestHeadersWhitelist').returns([ null ]); // This is how Joi returns it | ||
const config = function () { return { get: get }; }; | ||
const server = { | ||
config: config | ||
}; | ||
|
||
mapUri(server)(request, function (err, upstreamUri, upstreamHeaders) { | ||
expect(err).to.be(null); | ||
expect(Object.keys(upstreamHeaders).length).to.be(0); | ||
}); | ||
}); | ||
|
||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,10 +16,12 @@ function createProxy(server, method, route, config) { | |
handler: { | ||
proxy: { | ||
mapUri: mapUri(server), | ||
passThrough: true, | ||
agent: createAgent(server), | ||
xforward: true, | ||
timeout: server.config().get('elasticsearch.requestTimeout') | ||
timeout: server.config().get('elasticsearch.requestTimeout'), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we probably want to keep What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm torn on this one. It's common for proxies to add I think, for now, we should err on the side of not including it. We can always add it in later if it is requested (either explicitly or via a config option) but it would be harder to take away later if we add it now and folks start relying on it somehow. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with the sentiment, but in this case, It seems like a different issue than this PR is addressing, so why don't we keep There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressing this in a separate issue/PR sounds good to me. |
||
onResponse: function (err, responseFromUpstream, request, reply) { | ||
reply(err, responseFromUpstream); | ||
} | ||
} | ||
}, | ||
}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -78,8 +78,8 @@ module.exports = function (server) { | |
server.expose('ElasticsearchClientLogging', ElasticsearchClientLogging); | ||
server.expose('client', client); | ||
server.expose('createClient', createClient); | ||
server.expose('callWithRequestFactory', callWithRequest); | ||
server.expose('callWithRequest', callWithRequest(noAuthClient)); | ||
server.expose('callWithRequestFactory', _.partial(callWithRequest, server)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @lukasolson Hey, I know you originally introduced BTW, I noticed that no code is actually using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This refactor looks good to me. It is used in the security plugin in xpack. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @lukasolson! |
||
server.expose('callWithRequest', callWithRequest(server, noAuthClient)); | ||
server.expose('errors', elasticsearch.errors); | ||
|
||
return client; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
import _ from 'lodash'; | ||
|
||
export default function (originalHeaders, headersToKeep) { | ||
|
||
const normalizeHeader = function (header) { | ||
if (!header) { | ||
return ''; | ||
} | ||
header = header.toString(); | ||
return header.trim().toLowerCase(); | ||
}; | ||
|
||
// Normalize list of headers we want to allow in upstream request | ||
const headersToKeepNormalized = headersToKeep.map(normalizeHeader); | ||
|
||
// Normalize original headers in request | ||
const originalHeadersNormalized = _.mapKeys(originalHeaders, function (headerValue, headerName) { | ||
return normalizeHeader(headerName); | ||
}); | ||
|
||
return _.pick(originalHeaders, headersToKeepNormalized); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does
.items().single()
do here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It allows the user to define a single item array as a bare string. So they can define the setting as:
instead of having to say:
.single()
will convertx-my-custom-header-1
to[ x-my-custom-header-1 ]
because it knows the value of this setting must ultimately be an array.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, thanks.