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

Fix NTLM auth state when iterationCount is more than 2 #945

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open
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
4 changes: 4 additions & 0 deletions CHANGELOG.yaml
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
master:
fixed bugs:
- >-
GH-945 Fixed a bug where NTLM requests were failing for `iterationCount`
more that 2
chores:
- GH-943 Added `nyc` and `codecov` for code coverage checks

Expand Down
27 changes: 19 additions & 8 deletions lib/authorizer/ntlm.js
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,10 @@ module.exports = {
* @param {AuthHandlerInterface~authPreHookCallback} done
*/
pre: function (auth, done) {
!auth.get(STATE) && auth.set(STATE, STATES.INITIALIZED);
if (!auth.get(STATE)) {
auth.set(STATE, STATES.INITIALIZED);
auth.set(NTLM_HEADER, undefined);
}

done(null, true);
},
Expand All @@ -154,10 +157,18 @@ module.exports = {
challengeMessage, // type 2
authenticateMessage, // type 3
ntlmType2Header,
parsedParameters;
parsedParameters,

// resets the state and NTLM header and exits replay loop
resetStateAndStop = function (err) {
auth.set(STATE, STATES.INITIALIZED);
auth.set(NTLM_HEADER, undefined);

return done(err || null, true);
};

if (response.code !== 401 && response.code !== 403) {
return done(null, true);
return resetStateAndStop();
}

// we try to extract domain from username if not specified.
Expand All @@ -172,7 +183,7 @@ module.exports = {
// Nothing to do if the server does not ask us for auth in the first place.
if (!(response.headers.has(WWW_AUTHENTICATE, NTLM) ||
response.headers.has(WWW_AUTHENTICATE, NEGOTIATE))) {
return done(null, true);
return resetStateAndStop();
}

// Create a type 1 message to send to the server
Expand Down Expand Up @@ -202,13 +213,13 @@ module.exports = {
});

if (!ntlmType2Header) {
return done(new Error('ntlm: server did not send NTLM type 2 message'));
return resetStateAndStop(new Error('ntlm: server did not send NTLM type 2 message'));
}

challengeMessage = ntlmUtil.parseType2Message(ntlmType2Header.valueOf(), _.noop);

if (!challengeMessage) {
return done(new Error('ntlm: server did not correctly process authentication request'));
return resetStateAndStop(new Error('ntlm: server did not correctly process authentication request'));
}

authenticateMessage = ntlmUtil.createType3Message(challengeMessage, {
Expand All @@ -227,11 +238,11 @@ module.exports = {
}
else if (state === STATES.T3_MSG_CREATED) {
// Means we have tried to authenticate, so we should stop here without worrying about anything
return done(null, true);
return resetStateAndStop();
}

// We are in an undefined state
return done(null, true);
return resetStateAndStop();
},

/**
Expand Down
8 changes: 7 additions & 1 deletion lib/runner/extensions/item.command.js
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,8 @@ module.exports = {

var request = result.request,
response = result.response,
cookies = result.cookies;
cookies = result.cookies,
auth = result.item.getAuth();

if ((stopOnError || stopOnFailure) && requestError) {
this.triggers.item(null, coords, item); // @todo - should this trigger receive error?
Expand All @@ -211,6 +212,11 @@ module.exports = {
// set cookies for this transaction
cookies && (ctxTemplate.cookies = cookies);

// update the item auth to prevent loss of auth state
// @note this is a hack because the auth architecture is messy, and there is some confusion
// between `originalItem` and `item`.
auth && (item.auth = auth);

// the context template also has a test object to store assertions
ctxTemplate.tests = {}; // @todo remove

Expand Down
5 changes: 4 additions & 1 deletion test/fixtures/server.js
Original file line number Diff line number Diff line change
Expand Up @@ -537,6 +537,8 @@ function createNTLMServer (options) {
domain = options.domain || '',
workstation = options.workstation || '',

challenged = false,

type1Message = ntlmUtils.createType1Message({
domain,
workstation
Expand All @@ -559,14 +561,15 @@ function createNTLMServer (options) {
// sure that runtime can handle it.
'www-authenticate': [type2Message, 'Negotiate']
});
challenged = true;

options.debug && console.info('401: got type1 message');
}

// successful auth
// @note we don't check if the username and password are correct
// because I don't know how.
else if (authHeaders && authHeaders.startsWith(type3Message.slice(0, 100))) {
else if (challenged && authHeaders && authHeaders.startsWith(type3Message.slice(0, 100))) {
res.writeHead(200);

options.debug && console.info('200: got type3 message');
Expand Down
Loading