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

Do not be case-sensitive when checking http headers #2277

Merged
merged 4 commits into from
Dec 27, 2024
Merged

Do not be case-sensitive when checking http headers #2277

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
89a7f0e
Do not be case-sensitive when checking http headers
williamlardier Dec 23, 2024
5aadd36
Do not trust the x forwarded for header if not from a trusted proxy
williamlardier Dec 26, 2024
753fea6
Remove old TODO on getClientIp and remove duplicate logic
williamlardier Dec 26, 2024
ad12532
Bump Arsenal version
williamlardier Dec 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 6 additions & 7 deletions lib/policyEvaluator/requestUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,6 @@ export interface S3Config {
}
}

// TODO
// I'm not sure about this behavior.
// Should it returns string | string[] | undefined or string ?
/**
* getClientIp - Gets the client IP from the request
* @param request - http request object
Expand All @@ -20,8 +17,7 @@ export interface S3Config {
export function getClientIp(request: IncomingMessage, s3config?: S3Config): string {
const requestConfig = s3config?.requests;
const remoteAddress = request.socket.remoteAddress;
// TODO What to do if clientIp === undefined ?
const clientIp = (requestConfig ? remoteAddress : request.headers['x-forwarded-for'] || remoteAddress)?.toString() ?? '';
const clientIp = remoteAddress?.toString() ?? '';
if (requestConfig) {
const { trustedProxyCIDRs, extractClientIPFromHeader } = requestConfig;
/**
Expand All @@ -30,11 +26,14 @@ export function getClientIp(request: IncomingMessage, s3config?: S3Config): stri
* which header to be used to extract client IP
*/
if (ipCheck.ipMatchCidrList(trustedProxyCIDRs, clientIp)) {
const ipFromHeader = request.headers[extractClientIPFromHeader]?.toString();
// Request headers in nodejs are lower-cased, so we should not
// be case-sentive when looking for the header, as http headers
// are case-insensitive.
const ipFromHeader = request.headers[extractClientIPFromHeader.toLowerCase()]?.toString();
Copy link
Contributor

@francoisferrand francoisferrand Dec 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

looking a few lines above, we already retrieve the x-forwarded-for header:

const clientIp = (requestConfig ? remoteAddress : request.headers['x-forwarded-for'] || remoteAddress)?.toString() ?? '';

this seems inconsistent, please take the chance to review and clean this...
Esp. if requestConfig is not provided, we kind of bindly trust the x-forwarded-for header : is this really expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do we really need the extra conversion to lowercase, can't we just fix the 'X-Forwarded-For' where it is defined instead?

We could do it but the headers are case-insensitive anyway, so the current code is not following the standard and we might have more issues in the future...

Copy link
Contributor Author

@williamlardier williamlardier Dec 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

looking a few lines above, we already retrieve the x-forwarded-for header:
const clientIp = (requestConfig ? remoteAddress : request.headers['x-forwarded-for'] || remoteAddress)?.toString() ?? '';
this seems inconsistent, please take the chance to review and clean this...
Esp. if requestConfig is not provided, we kind of bindly trust the x-forwarded-for header : is this really expected?

I am not aware of any deployment today that does not set the requestConfig, but this header retrieval was introduced in https://scality.atlassian.net/browse/ARSN-57, so here: https://github.com/scality/Arsenal/pull/1693/files#diff-ca8ab692737b69e1ab977a7a6104be671972410cafadea45b119e8259070ad1bR11

If the question is whether we should trust the header blindly or not: as long as the proxy is not trusted we should not trust the header. I will remove this logic to be on the safe side.

Previously we were not using IP conditions so it was only about displaying the info in logs, hence the previous change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

extractClientIPFromHeader probably does not work if not equal to x-forwarded-for

Please elaborate what you mean by "does not work": it's a string field, that can be any header: e.g., we can have a custom header set by nginx and use it.

Copy link
Contributor

Choose a reason for hiding this comment

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

extractClientIPFromHeader probably does not work if not equal to x-forwarded-for

Please elaborate what you mean by "does not work": it's a string field, that can be any header: e.g., we can have a custom header set by nginx and use it.

"does not work" was too quick, I missed the condition on requestConfig

if (ipFromHeader && ipFromHeader.trim().length) {
return ipFromHeader.split(',')[0].trim();
}
}
}
return clientIp?.toString() ?? '';
return clientIp;
}
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"engines": {
"node": ">=16"
},
"version": "7.10.63",
"version": "7.10.64",
"description": "Common utilities for the S3 project components",
"main": "build/index.js",
"repository": {
Expand Down
27 changes: 24 additions & 3 deletions tests/unit/policyEvaluator/requestUtils.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,27 @@ describe('requestUtils.getClientIp', () => {
assert.strictEqual(result, testClientIp1);
});

it('should return client Ip address in the proxy case when the header has uppercases', () => {
const request = new DummyRequest({
headers: {
'x-forwarded-for': [testClientIp1, testProxyIp].join(','),
},
url: '/',
parsedHost: 'localhost',
socket: {
remoteAddress: testProxyIp,
},
});
const result = requestUtils.getClientIp(request, {
requests: {
viaProxy: true,
trustedProxyCIDRs: ['192.168.100.0/22'],
extractClientIPFromHeader: 'X-Forwarded-For',
},
});
assert.strictEqual(result, testClientIp1);
});

it('should return client Ip address from socket info if the request is not forwarded from proxies', () => {
const request = new DummyRequest({
headers: {},
Expand Down Expand Up @@ -56,8 +77,8 @@ describe('requestUtils.getClientIp', () => {
assert.strictEqual(result, testClientIp2);
});

it('should return client Ip address from header if the request comes via proxies and ' +
'no request config is available', () => {
it('should not return client Ip address from header if the request comes via proxies and ' +
'no request config is available as the proxy is not trusted', () => {
const request = new DummyRequest({
headers: {
'x-forwarded-for': testClientIp1,
Expand All @@ -69,7 +90,7 @@ describe('requestUtils.getClientIp', () => {
},
});
const result = requestUtils.getClientIp(request, configWithoutProxy);
assert.strictEqual(result, testClientIp1);
assert.strictEqual(result, testProxyIp);
});

it('should return client Ip address from socket info if the request comes via proxies and ' +
Expand Down
Loading