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

[BIOMAGE-1824] Missing clusters issue #339

Merged
merged 6 commits into from
Apr 27, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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: 10 additions & 3 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
"express-openapi-validator": "^4.10.8",
"github-download-directory": "^2.0.0",
"ioredis": "^4.17.3",
"ipaddr.js": "^2.0.1",
"js-yaml": "^3.13.1",
"json-merger": "^1.1.7",
"jsonwebtoken": "^8.5.1",
Expand Down
42 changes: 8 additions & 34 deletions src/utils/authMiddlewares.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ const jwt = require('jsonwebtoken');
const jwtExpress = require('express-jwt');
const jwkToPem = require('jwk-to-pem');
const util = require('util');
const dns = require('dns').promises;

const config = require('../config');

Expand All @@ -18,6 +17,7 @@ const { CacheMissError } = require('../cache/cache-utils');
const { UnauthorizedError, UnauthenticatedError } = require('./responses');
const ProjectsService = require('../api/route-services/projects');
const AccessService = require('../api/route-services/access');
const { isReqFromLocalhost, isReqFromCluster } = require('./isReqFrom');

const accessService = new AccessService();
const projectService = new ProjectsService();
Expand Down Expand Up @@ -82,32 +82,7 @@ const authenticationMiddlewareExpress = async (app) => {
});
};


// eslint-disable-next-line no-useless-escape
const INTERNAL_DOMAINS_REGEX = new RegExp('((\.compute\.internal)|(\.svc\.local))$');

const checkAuthExpiredMiddleware = (req, res, next) => {
const isReqFromLocalhost = async () => {
const ip = req.connection.remoteAddress;
const host = req.get('host');

if (ip === '127.0.0.1' || ip === '::ffff:127.0.0.1' || ip === '::1' || host.indexOf('localhost') !== -1) {
return true;
}

throw new Error('ip address is not localhost');
};

const isReqFromCluster = async () => {
const domains = await dns.reverse(req.ip);

if (!domains.some((domain) => INTERNAL_DOMAINS_REGEX.test(domain))) {
throw new Error('ip address does not come from internal sources');
}

return true;
};

if (!req.user) {
return next();
}
Expand All @@ -125,8 +100,9 @@ const checkAuthExpiredMiddleware = (req, res, next) => {
return next(new UnauthenticatedError('token has expired'));
}


// check if we should ignore expired jwt token for this path and request type
const longTimeoutEndpoints = [{ urlMatcher: /^\/v1\/experiments\/.{32}\/cellSets$/, method: 'PATCH' }];
const longTimeoutEndpoints = [{ urlMatcher: /experiments\/.{32}\/cellSets$/, method: 'PATCH' }];
const isEndpointIgnored = longTimeoutEndpoints.some(
({ urlMatcher, method }) => (
req.method.toLowerCase() === method.toLowerCase() && urlMatcher.test(req.url)
Expand All @@ -135,15 +111,13 @@ const checkAuthExpiredMiddleware = (req, res, next) => {

// if endpoint is not in ignore list, the JWT is too old, send an error accordingly
if (!isEndpointIgnored) {
return next(new UnauthenticatedError('token has expired'));
return next(new UnauthenticatedError('token has expired for non-ignored endpoint'));
}

promiseAny([isReqFromCluster(), isReqFromLocalhost()])
.then(() => {
next();
})
.catch(() => {
next(new UnauthenticatedError('token has expired'));
promiseAny([isReqFromCluster(req), isReqFromLocalhost(req)])
.then(() => next())
.catch((e) => {
next(new UnauthenticatedError(`invalid request origin ${e}`));
});

return null;
Expand Down
41 changes: 41 additions & 0 deletions src/utils/isReqFrom.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
const dns = require('dns').promises;
const ipaddr = require('ipaddr.js');

// eslint-disable-next-line no-useless-escape
const INTERNAL_DOMAINS_REGEX = new RegExp('((\.compute\.internal)|(\.svc\.local))$');

const isReqFromLocalhost = async (req) => {
const ip = req.connection.remoteAddress;
const host = req.get('host');

if (ip === '127.0.0.1' || ip === '::ffff:127.0.0.1' || ip === '::1' || host.indexOf('localhost') !== -1) {
return true;
}

throw new Error('ip address is not localhost');
};

const isReqFromCluster = async (req) => {
let remoteAddress = req.ip;
const addr = ipaddr.parse(req.ip);
// req.ip returns IPv4 addresses mapped to IPv6, e.g.:
// 127.0.0.1 (IPv4) -> ::ffff:127.0.0.1 (IPv6)
// dns.reverse is not capable of dealing with them,
// it either uses IPv4 or IPv6, so we need to map those
// IPs back to IPv4 before.
if (addr.kind() === 'ipv6' && addr.isIPv4MappedAddress()) {
remoteAddress = addr.toIPv4Address().toString();
}

const domains = await dns.reverse(remoteAddress);
if (domains.some((domain) => INTERNAL_DOMAINS_REGEX.test(domain))) {
return true;
}

throw new Error('ip address does not come from internal sources');
};

module.exports = {
isReqFromCluster,
isReqFromLocalhost,
};
18 changes: 18 additions & 0 deletions tests/utils/authMiddlewares.test.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
const AWSMock = require('aws-sdk-mock');

const {
checkAuthExpiredMiddleware,
expressAuthorizationMiddleware,
authorize,
} = require('../../src/utils/authMiddlewares');
Expand Down Expand Up @@ -52,6 +54,22 @@ describe('Tests for authorization/authentication middlewares', () => {
expect(next).toBeCalledWith();
});

it('checkAuth accepts expired tokens for patch cellsets', async () => {
mockDynamoGetItem(data);

const req = {
params: { experimentId: fake.EXPERIMENT_ID },
user: fake.USER,
url: `/v1/experiments/${fake.EXPERIMENT_ID}/cellSets`,
method: 'PATCH',
ip: '::ffff:127.0.0.1',
};
const next = jest.fn();

const ret = checkAuthExpiredMiddleware(req, {}, next);
expect(ret).toBe(null);
});

it('Express middleware can reject incorrect users', async () => {
mockDynamoGetItem({});

Expand Down
77 changes: 77 additions & 0 deletions tests/utils/isReqFrom.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@


jest.mock('dns', () => ({
promises: {
reverse: jest.fn((ip) => {
if (ip === '127.0.0.1') return [];
return ['ip-192-168-905-123.region.compute.internal'];
}),
},
}));

const {
isReqFromCluster,
isReqFromLocalhost,
} = require('../../src/utils/isReqFrom');
const fake = require('../test-utils/constants');

describe('Tests for isRequestFromCluster and isRequestFromLocalhost', () => {
Copy link
Member

Choose a reason for hiding this comment

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

you could have also one describe('isRequestFromCluster') and another describe('isRequestFromLocalhost') (in the same .test.js file).

It's not a PR-blocking thing though, but just leaving this as another option.

it('isReqFromLocalhost returns true for localhost', async () => {
const req = {
params: { experimentId: fake.EXPERIMENT_ID },
user: fake.USER,
url: `/v1/experiments/${fake.EXPERIMENT_ID}/cellSets`,
method: 'PATCH',
connection: {
address: '::ffff:127.0.0.1',
},
get: () => 'localhost',
};

expect(await isReqFromLocalhost(req)).toEqual(true);
});

it('isRequestFromLocalhost throws with cluster addr', async () => {
const req = {
params: { experimentId: fake.EXPERIMENT_ID },
user: fake.USER,
url: `/v1/experiments/${fake.EXPERIMENT_ID}/cellSets`,
method: 'PATCH',
ip: '::ffff:192.0.0.1',
connection: {
address: '::ffff:192.0.0.1',
},
get: () => 'whatever.internal.compute',
};

await expect(isReqFromLocalhost(req))
.rejects
.toThrow();
});

it('isReqFromCluster returns true for cluster addr', async () => {
const req = {
params: { experimentId: fake.EXPERIMENT_ID },
user: fake.USER,
url: `/v1/experiments/${fake.EXPERIMENT_ID}/cellSets`,
method: 'PATCH',
ip: '::ffff:192.0.0.1',
};

expect(await isReqFromCluster(req)).toEqual(true);
});

it('isRequestFromCluster throws with localhost', async () => {
const req = {
params: { experimentId: fake.EXPERIMENT_ID },
user: fake.USER,
url: `/v1/experiments/${fake.EXPERIMENT_ID}/cellSets`,
method: 'PATCH',
ip: '::ffff:127.0.0.1',
};

await expect(isReqFromCluster(req))
.rejects
.toThrow();
});
});