Skip to content

Commit

Permalink
new+test: allow Basic authentication over HTTPS only.
Browse files Browse the repository at this point in the history 
* this is not a recommended authentication scheme, as every request will
  incur a heavy password-validation cost as we have to run bcrypt every
  single time.
* we also will only offer this if HTTPS is in use. it's possible to fake
  out the server into thinking this is true, but the user does this at
  their own peril.
  • Loading branch information
@issa-tseng
issa-tseng committed Apr 5, 2018
1 parent 9b375ab commit 471782b
Show file tree
Hide file tree
Showing 4 changed files with 159 additions and 22 deletions.
60 changes: 48 additions & 12 deletions lib/http/middleware.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ const { isBlank } = require('../util/util');
const { isTrue } = require('../util/http');
const Problem = require('../util/problem');
const Option = require('../util/option');
const { getOrReject } = require('../util/promise');
const { verifyPassword } = require('../util/crypto');


// Strips a /v# prefix off the request path and exposes on the request object
Expand All @@ -20,20 +22,52 @@ const versionParser = (request, response, next) => {
// appropriate credentials. If the given credentials don't match a session, aborts
// with a 401. If no credentials are given, injects an empty session.
// TODO: probably a better home for this.
const sessionParser = ({ Session, Auth }) => (request, response, next) => {
// TODO: repetitive, but detangling it doesn't make it clearer.
const sessionParser = ({ Session, User, Auth }) => (request, response, next) => {
const authHeader = request.get('Authorization');
if (!isBlank(authHeader) && authHeader.startsWith('Bearer ')) {
if ((request.auth != null) && (request.auth.session.isDefined())) return next(Problem.user.authenticationFailed());
// Standard Bearer token auth.
if ((request.auth != null) && request.auth.isAuthenticated()) return next(Problem.user.authenticationFailed());

Session.getByBearerToken(authHeader.slice(7)).point()
.then((session) => {
if (!session.isDefined()) return next(Problem.user.authenticationFailed());

request.auth = new Auth({ session });
request.auth = new Auth({ _session: session });
next();
});
} else if (!isBlank(authHeader) && authHeader.startsWith('Basic ')) {
// Allow Basic auth over HTTPS only.
if ((request.auth != null) && request.auth.isAuthenticated()) return next(Problem.user.authenticationFailed());

// fail the request unless we are under HTTPS.
// this logic does mean that if we are not under nginx it is possible to fool the server.
// but it is the user's prerogative to undertake this bypass, so their security is in their hands.
if ((request.protocol !== 'https') && (request.get('x-forwarded-proto') !== 'https'))
return next(Problem.user.httpsOnly());

// we have to use a regex rather than .split(':') in case the password contains :s.
const plainCredentials = Buffer.from(authHeader.slice(6), 'base64').toString('utf8');
const match = /^([^:]+):(.+)$/.exec(plainCredentials);
if (match == null) return next(Problem.user.authenticationFailed());
const [ , email, password ] = match;

// actually do our verification.
// TODO: email existence timing attack on whether bcrypt runs or not.
User.getByEmail(email).point()
.then(getOrReject(Problem.user.authenticationFailed()))
.then((user) => verifyPassword(password, user.password).point()
.then((verified) => {
if (verified == true) {
request.auth = new Auth({ _actor: user.actor });
next();
} else {
next(Problem.user.authenticationFailed());
}
}))
.catch(next);
} else {
request.auth = new Auth({ session: Option.none() });
request.auth = new Auth();
next();
}
};
Expand All @@ -51,16 +85,18 @@ const sessionParser = ({ Session, Auth }) => (request, response, next) => {
const fieldKeyParser = ({ Session, Auth }) => (request, response, next) => {
const match = /^\/key\/([a-z0-9!$]{64})\//i.exec(request.url);
if (match == null) return next();
if ((request.auth != null) && (request.auth.session.isDefined())) return next(Problem.user.authenticationFailed());
if ((request.auth != null) && (request.auth.isAuthenticated())) return next(Problem.user.authenticationFailed());

Session.getByBearerToken(match[1]).point().then((session) => {
if (!session.isDefined()) return next(Problem.user.authenticationFailed());
if (session.get().actor.type !== 'field_key') return next(Problem.user.authenticationFailed());
Session.getByBearerToken(match[1]).point()
.then(getOrReject(Problem.user.authenticationFailed()))
.then((session) => {
if (session.actor.type !== 'field_key') return next(Problem.user.authenticationFailed());

request.auth = new Auth({ session });
request.url = request.url.slice('/key/'.length + match[1].length);
next();
});
request.auth = new Auth({ _session: session });
request.url = request.url.slice('/key/'.length + match[1].length);
next();
})
.catch(next);
};

// simply determines if we have been fed a true-ish value for X-Extended-Metadata.
Expand Down
14 changes: 10 additions & 4 deletions lib/model/instance/auth.js
View file
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
const Instance = require('./instance');
const { reject } = require('../../util/promise');
const Problem = require('../../util/problem');
const Option = require('../../util/option');

module.exports = Instance(({ Grant }) => class {
module.exports = Instance(({ Auth, Grant }) => class {
// See: Grant::can for behaviour. TODO: also see it for todo notes.
// If no actor is present for this session, will check against anonymous rights.
can(verb, actee) {
return Grant.can(this.session.map((session) => session.actor), verb, actee);
return Grant.can(this.actor(), verb, actee);
}

// See: Actor#canOrReject for behaviour.
Expand All @@ -16,7 +17,12 @@ module.exports = Instance(({ Grant }) => class {
((result === true) ? true : reject(Problem.user.insufficientRights())));
}

// simple helper since this is often required:
actor() { return this.session.map((session) => session.actor); }
session() { return Option.of(this._session); }
actor() {
const bySession = this.session().map((session) => session.actor);
return bySession.isDefined() ? bySession : Option.of(this._actor);
}

isAuthenticated() { return (this._session != null) || (this._actor != null); }
});

2 changes: 2 additions & 0 deletions lib/util/problem.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ const problems = {
// no detail information for security reasons.
authenticationFailed: problem(401.2, () => 'Could not authenticate with the provided credentials.'),

httpsOnly: problem(401.3, () => 'This authentication method is only available over HTTPS'),

// TODO: should have details but not sure what yet.
insufficientRights: problem(403.1, () => 'The authenticated actor does not have rights to perform that action.'),

Expand Down
105 changes: 99 additions & 6 deletions test/unit/http/middleware.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ const middleware = require(appRoot + '/lib/http/middleware');
const Problem = require(appRoot + '/lib/util/problem');
const Option = require(appRoot + '/lib/util/option');
const { ExplicitPromise } = require(appRoot + '/lib/util/promise');
const { hashPassword } = require(appRoot + '/lib/util/crypto');

describe('middleware', () => {
describe('versionParser', () => {
Expand Down Expand Up @@ -56,19 +57,36 @@ describe('middleware', () => {
? Option.of('session')
: Option.none()))
});
const mockUser = (expectedEmail, password) => ({
getByEmail: (email) => ExplicitPromise.of(Promise.resolve((email === expectedEmail)
? Option.of({ password, actor: 'actor' })
: Option.none()))
});

it('should set no-session if no Authorization header is provided', (done) => {
it('should set no auth if no Authorization header is provided', (done) => {
const request = createRequest();
sessionParser({ Auth, Session: mockSession() })(request, null, () => {
request.auth.session.should.equal(Option.none());
should.not.exist(request.auth._session);
should.not.exist(request.auth._actor);
done();
});
});

it('should set no-session if Authorization mode is no Bearer', (done) => {
const request = createRequest({ headers: { Authorization: 'Basic aabbccddeeff123' } });
it('should set no auth if Authorization mode is not Bearer or Basic', (done) => {
const request = createRequest({ headers: { Authorization: 'Digest aabbccddeeff123' } });
sessionParser({ Auth, Session: mockSession() })(request, null, () => {
request.auth.session.should.equal(Option.none());
should.not.exist(request.auth._session);
should.not.exist(request.auth._actor);
done();
});
});

it('should fail the request if Bearer auth is attempted with a successful auth present', (done) => {
const request = createRequest({ headers: { Authorization: 'Bearer aabbccddeeff123' } });
request.auth = { isAuthenticated: () => true };
sessionParser({ Auth, Session: mockSession() })(request, null, (failure) => {
failure.isProblem.should.equal(true);
failure.problemCode.should.equal(401.2);
done();
});
});
Expand All @@ -85,10 +103,85 @@ describe('middleware', () => {
it('should set the appropriate session if a valid Bearer token is given', (done) => {
const request = createRequest({ headers: { Authorization: 'Bearer alohomora' } });
sessionParser({ Auth, Session: mockSession('alohomora') })(request, null, () => {
request.auth.session.should.eql(Option.of('session'));
request.auth._session.should.eql(Option.of('session'));
done();
});
});

it('should reject non-https Basic auth requests', (done) => {
const request = createRequest({ headers: { Authorization: 'Basic abracadabra', } });
sessionParser({ Auth, User: mockUser('alice@opendatakit.org') })(request, null, (failure) => {
failure.isProblem.should.equal(true);
failure.problemCode.should.equal(401.3);
done();
});
});

it('should fail the request if an improperly-formatted Basic auth is given', (done) => {
const encodedCredentials = Buffer.from('alice@opendatakit.org:', 'utf8').toString('base64');
const request = createRequest({ headers: {
Authorization: `Basic ${encodedCredentials}`,
'X-Forwarded-Proto': 'https'
} });
sessionParser({ Auth, User: mockUser('alice@opendatakit.org') })(request, null, (failure) => {
failure.isProblem.should.equal(true);
failure.problemCode.should.equal(401.2);
done();
});
});

it('should fail the request if Basic auth is attempted with a successful auth present', (done) => {
const request = createRequest({ headers: {
Authorization: `Basic abcdef1234567890`,
'X-Forwarded-Proto': 'https'
} });
request.auth = { isAuthenticated: () => true };
sessionParser({ Auth, User: mockUser('alice@opendatakit.org') })(request, null, (failure) => {
failure.isProblem.should.equal(true);
failure.problemCode.should.equal(401.2);
done();
});
});

it('should fail the request if the Basic auth user cannot be found', (done) => {
const encodedCredentials = Buffer.from('bob@opendatakit.org:bob', 'utf8').toString('base64');
const request = createRequest({ headers: {
Authorization: `Basic ${encodedCredentials}`,
'X-Forwarded-Proto': 'https'
} });
sessionParser({ Auth, User: mockUser('alice@opendatakit.org') })(request, null, (failure) => {
failure.isProblem.should.equal(true);
failure.problemCode.should.equal(401.2);
done();
});
});

it('should fail the request if the Basic auth credentials are not right', (done) => {
const encodedCredentials = Buffer.from('alice@opendatakit.org:password', 'utf8').toString('base64');
const request = createRequest({ headers: {
Authorization: `Basic ${encodedCredentials}`,
'X-Forwarded-Proto': 'https'
} });
sessionParser({ Auth, User: mockUser('alice@opendatakit.org', 'willnevermatch') })(request, null, (failure) => {
failure.isProblem.should.equal(true);
failure.problemCode.should.equal(401.2);
done();
});
});

it('should set the appropriate session if valid Basic auth credentials are given @slow', (done) => {
hashPassword('alice').point().then((hashed) => {
const encodedCredentials = Buffer.from('alice@opendatakit.org:alice', 'utf8').toString('base64');
const request = createRequest({ headers: {
Authorization: `Basic ${encodedCredentials}`,
'X-Forwarded-Proto': 'https'
} });
sessionParser({ Auth, User: mockUser('alice@opendatakit.org', hashed) })(request, null, () => {
request.auth._actor.should.equal('actor');
done();
});
});
});
});
});

0 comments on commit 471782b

Please sign in to comment.