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

Switch from cookies to bearer tokens #288

Closed

Conversation

@michielbdejong
Copy link
Contributor

michielbdejong commented Jun 9, 2014

This PR takes the auth token from the couchdb cookie and exposes it as a bearer
token at the HAPI level:

  • When couchdb sets a cookie, it is stripped, and the authToken is added to
    the JSON body.
  • When a request comes in, the 'Bearer ...' token is read from the Authorization
    header, and converted to a cookie for couchdb.

This happens at the hapi level.
The solution is based on hapijs/hapi#1637
and https://github.com/spumko/hapi/blob/master/docs/Reference.md

TODO:

  • create a couchdb mock in the test framework (added test/unit/api-plugin-test.js)
  • use this to create a test for the mapProxyPath function
  • use this to create a test for the addCorsAndBearerToken method
  • add tests for the extractToken function
  • fix tests that rely on callback being called
  • fix clashing of integration test suites that both start couchdb on port 6006
@michielbdejong
Copy link
Contributor Author

michielbdejong commented Jun 9, 2014

I could use some advice about

@michielbdejong
Copy link
Contributor Author

michielbdejong commented Jun 10, 2014

btw, the way I implemented this now, no OAuth dialog is necessary. The app just gets to see the user's real password and then calls hoodie.account.signIn the normal way. Much simpler! :)

@Acconut
Copy link
Contributor

Acconut commented Jun 10, 2014

Will this be an integrated protection against CSRF?

@svnlto
Copy link
Member

svnlto commented Jun 10, 2014

@Acconut there is a separate hapi module for CSRF protection https://github.com/spumko/crumb

@Acconut
Copy link
Contributor

Acconut commented Jun 10, 2014

@svnlto Yes, but you're only vulnerable to CSRF if you use cookies. And if this PR removes all cookies between the browser and hoodie-server, we won't be vulnerable by default so no protection is needed.
Or am I missing something?

@svnlto
Copy link
Member

svnlto commented Jun 10, 2014

@Acconut correct, but this makes me think that we should add CSRF protection by default? /cc @janl @caolan

@Acconut
Copy link
Contributor

Acconut commented Jun 10, 2014

@svnlto The question is, if this PR removes cookie usage. :) But if not we should totally do! 👍

@michielbdejong
Copy link
Contributor Author

michielbdejong commented Jun 10, 2014

@Acconut yes, it tries to strip all outgoing as well as incoming cookies, but I'm not sure whether it does so successfully, there may be edge cases I forgot. If there are no bugs in the cookie-stripping then your point about CSRF being solved by that sounds plausible. But IANASE. :)

And this PR introduces a new attack surface: the hoodie.account.authToken variable which will be available in memory, and persisted to config (localStorage?), so that also requires some extra pairs of eyes.

It's not a trivial change. But it makes new Hoodie('https://a.url.of.my.own.choice.com/'); work, which is I think an important part of the hoodie philosophy.

@Acconut
Copy link
Contributor

Acconut commented Jun 10, 2014

@michielbdejong It's still possible for XSS attacks to get the token but before your PR, using cookies, XSS could also make own request, so I see this as an improvement 👍.

@janl
Copy link
Member

janl commented Jun 10, 2014

Let’s keep the XSS/CSRF question out of this PR as the scope and intent are entirely different.

@michielbdejong
Copy link
Contributor Author

michielbdejong commented Jul 2, 2014

It seems same-origin signup may be broken; i get a 409 and it displays "undefined: undefined" in the dialog. Will investigate further once I fix my Dockerfile for this branch.

@michielbdejong
Copy link
Contributor Author

michielbdejong commented Jul 3, 2014

The code is working now, but before this gets merged I could use some help with:

  • fix tests that rely on callback being called
  • fix clashing of integration test suites that both start couchdb on port 6006
  • testing whether same-origin signup still works correctly

would someone be available to pair on that, maybe?

Also, Michiel Leenaars from nlnet pointed out I could offer to do a guest post on the Hoodie blog about "per-user Hoodie" once it's merged - for my part I think that would be a great idea! Let me know if that would work for you guys as well.

@janl
Copy link
Member

janl commented Jul 3, 2014

@michielbdejong great progress. I’m meaning to look into this, but realistically, it won’t be before two weeks from now, so maybe someone can beat me to it :)

Also, yes, we would totally blog about this, if you want to supply a blog draft, let us know :)

@Acconut
Copy link
Contributor

Acconut commented Jul 3, 2014

@michielbdejong I would enjoy helping with this PR but I'm currently busy and I need some time to get used to your changes but I'll try to contribute.

resp.headers = res.headers;

resp.headers['Access-Control-Allow-Origin'] = request.headers.origin || '*';
resp.headers['Access-Control-Allow-Headers'] = 'Authorization, Content-Length, Content-Type, If-Match, If-None-Match, Origin, X-Requested-With';

This comment has been minimized.

Copy link
@gr2m

gr2m Jul 14, 2014

Member

can you please make Access-Control-Allow-Headers dynamic, based on the request coming in, just like you do for Access-Control-Allow-Origin? Otherwise plugins with hooks depending on own headers won't work with CORS

This comment has been minimized.

Copy link
@michielbdejong

michielbdejong Jul 14, 2014

Author Contributor

good idea! although I guess that will only work if the headers are already sent in the OPTIONS request

This comment has been minimized.

Copy link
@janl

janl Sep 3, 2014

Member

this still seems unresolved?

This comment has been minimized.

Copy link
@gr2m

gr2m Sep 3, 2014

Member

I guess that will only work if the headers are already sent in the OPTIONS request

Shouldn't be a problem. When browsers need custom headers for CORS requests, they send the headers in the OPTIONS request as far as I know

This comment has been minimized.

Copy link
@michielbdejong

michielbdejong Sep 4, 2014

Author Contributor

ah ok, I could make it add all headers that are on the request, plus all headers from the Access-Control-Request-Headers request header. OK, I'll add that functionality. Anything else?

This comment has been minimized.

Copy link
@gr2m

gr2m Sep 4, 2014

Member

It looks good to me, I gave it another test with the latest version of your hoodie-server/hoodie.js pull requests.

Just one thought. Is there a reason to call the token "bearer token" and "auth token" at different places? Maybe it would reduce potential confusion to call it bearer token all over? Or do I miss something?

This comment has been minimized.

Copy link
@michielbdejong

michielbdejong Sep 4, 2014

Author Contributor

agreed, I'll make that change as well.

hoodie_server.start(config, done);
});

// TODO: I guess we should kill the server once we are done with the tests

This comment has been minimized.

Copy link
@janl

janl Jul 14, 2014

Member

This is currently a limitation of the tests, don’t worry about it too much at this point. We’ll fix it with the other tests when we get to it. The important bit is to use a port number that is different from the ones used in the other integration tests.

});

describe('addCorseAndBearerToken', function () {
//FIXME: this test passes even if the callback is not called

This comment has been minimized.

Copy link
@janl

This comment has been minimized.

Copy link
@michielbdejong

michielbdejong Jul 14, 2014

Author Contributor

thanks! that's what i was looking for :)

});
});

//FIXME: this test passes even if the callback is not called
});
});

//FIXME: this test passes even if the callback is not called
});
});

//FIXME: this test passes even if the callback is not called
});
});

//FIXME: this test passes even if the callback is not called
});
});

//FIXME: this test passes even if the callback is not called
@michielbdejong
Copy link
Contributor Author

michielbdejong commented Jul 22, 2014

If someone wants to pair on this in Berlin, I'm here until Monday evening

@gr2m
Copy link
Member

gr2m commented Sep 16, 2014

@michielbdejong I just rebased it on master, there were a few conflicts. I know get an error when starting up, you also get it when running npm test. I pushed the rebased branch to https://github.com/hoodiehq/hoodie-server/tree/bearer-token-support-rebased

Any chance you could help me get this fixed? I want to get this merged, everything else is set from my side

@michielbdejong
Copy link
Contributor Author

michielbdejong commented Sep 16, 2014

i'll have a look after lunch!

@gr2m
Copy link
Member

gr2m commented Sep 16, 2014

@svnlto helped out me to get hoodie-server running again, it was a hapi problem. But I know see some of the addCorseAndBearerToken errors fail

@gr2m
Copy link
Member

gr2m commented Sep 16, 2014

I fear I broke something with your hoodie-server work, the response to POST /_api/_session has no bearerToken property. If you could have a look at it, that'd be great

@michielbdejong
Copy link
Contributor Author

michielbdejong commented Sep 16, 2014

OK, should be easy to fix. I lost some time getting couchdb running again on my devbox, but I'll continue tomorrow.

@michielbdejong michielbdejong force-pushed the michielbdejong:bearer-token-support branch from e86d489 to 45beb1c Sep 17, 2014
michielbdejong added a commit to michielbdejong/hoodie-server that referenced this pull request Sep 17, 2014
This PR takes auth token from the couchdb cookie and exposes it as a bearer
token at the HAPI level:
* When couchdb sets a cookie, it is stripped, and the authToken is added to
    the JSON body.
* When a request comes in, the 'Bearer ...' token is read from the Authorization
    header, and converted to a cookie for couchdb.

This happens at the hapi level.
The solution is based on hapijs/hapi#1637
and https://github.com/spumko/hapi/blob/master/docs/Reference.md

TODO:
* [ ] create a couchdb mock in the test framework
* [ ] use this to create a test for the `mapProxyPath` function
* [ ] use this to create a test for the `addCorsAndBearerToken` method
* [ ] add tests for the `extractToken` function

add basic tests for api plugin

tests for mapProxyPath

add test for extractToken

first draft of addCorsAndBearerToken tests

use done function in tests

use port number that is different from the one used in other integration tests

fix hoodiehq#288 (comment)

fix hoodiehq#288 (comment)

jshint and typo

fix merge errors from rebase

try to fix https://travis-ci.org/hoodiehq/hoodie.js/builds/34471428

fix typo in test
@michielbdejong
Copy link
Contributor Author

michielbdejong commented Sep 17, 2014

The latest changes from my branch are missing in your rebase, for instance the addAllowedHeader function.

I just rebased my branch on master, can you maybe test with my one?

I'm having a hard time getting hoodie-server and hoodie.js working on my machine, I have been trying for hours, but it just fails to install in different ways.

I'm going to start from scratch using the Ubuntu install instructions from hood.ie, instead of using the two git repos as a starting point.

@gr2m
Copy link
Member

gr2m commented Sep 17, 2014

@michielbdejong Try to remove all karma packages from hoodie.js package json. Then it should run the npm install command at least. Sorry for the trouble, got bit a lot by this myself in the past. It's fixed in the new version that is currently in the works, but we have to get this merged into the old one.

Another problem I've found: the bearer token gets sent after you sign out. It gets removed form localStorage, but is still set on hoodie.account.bearerToken

@michielbdejong
Copy link
Contributor Author

michielbdejong commented Sep 18, 2014

@gr2m ok, that should be fixed in michielbdejong/hoodie.js@893f999. Is everything else working now, if you use my rebase?

@gr2m
Copy link
Member

gr2m commented Sep 18, 2014

Unfortunately not, when running the integration tests, the server crashes repeatedly with this error:

Debug: hapi, internal, implementation, error 
    SyntaxError: Uncaught error: Unexpected end of input
    at Object.parse (native)
    at /Users/gregor/JavaScripts/hood.ie/hoodie-server/lib/server/plugins/api/index.js:51:19
    at finish (/Users/gregor/JavaScripts/hood.ie/hoodie-server/node_modules/nipple/lib/index.js:202:20)
    at wrapped (/Users/gregor/JavaScripts/hood.ie/hoodie-server/node_modules/nipple/node_modules/hoek/lib/index.js:560:20)
    at onReaderFinish (/Users/gregor/JavaScripts/hood.ie/hoodie-server/node_modules/nipple/lib/index.js:255:16)
    at g (events.js:180:16)
    at EventEmitter.emit (events.js:117:20)
    at finishMaybe (_stream_writable.js:360:12)
    at endWritable (_stream_writable.js:367:3)
    at Writable.end (_stream_writable.js:345:5)

Have you seen that? Maybe we can pair on this later today or tomorrow?

To setup the integration tests locally, checkout https://github.com/hoodiehq/hoodie-integration-test/, do npm install, change port numbers in hosts.json file to point to your hoodie app that you test with (make sure admin password is set to "12345"), make sure latest selenium is running, and then run grunt intern:tests.

I also found a problem in hoodie.js that I fixed (https://gist.github.com/c109b87f27dacdd90a11), can you add me as collaborator to your fork?

@gr2m
Copy link
Member

gr2m commented Sep 18, 2014

@michielbdejong Here is a script you can run against your hoodie test app using the bearer branches of hoodie-server and hoodie.js

// make sure localStorage is empty. If not, clear an reload page
var username = 'user' + Date.now();
var password = 'secret';
window.events = [];

[
  'signin',
  'signup',
  'signout',
  'changeusername',
  'changepassword',
  'passwordreset',
  'reauthenticated',
  'error:unauthenticated',
  'error:passwordreset'
].forEach(function(eventName) {
  hoodie.account.on(eventName, function() {
    window.events.push(eventName);
  });
});

hoodie.account.signIn('foo', 'bar').fail(function() {
  console.log('✓ sign in error message');
  hoodie.account.signUp(username, password).done(function() {
    console.log('✓ sign up');
    var oldPassword = password;
    password += '2';
    hoodie.account.changePassword(oldPassword, password).done(function() {
      console.log('✓ change password');
      username += '2';
      hoodie.account.changeUsername(password, username).done(function() {
        console.log('✓ change username');
        hoodie.account.signOut().done(function() {
          console.log('✓ sign out');
          hoodie.account.signIn(username, password).done(function() {
            console.log('✓ sign in');
            hoodie.account.destroy().done(function() {
              console.log('✓ destroy');
              var allEvents = events.join(',');
              var allEventsExpected = 'signup,changepassword,changeusername,signout,signin,signout';

              console.log('checking for events with 2s timeout to catch unwanted error:unauthenticated events');
              setTimeout(function() {
                if (allEvents === allEventsExpected) {
                  console.log('✓ events');
                } else {
                  console.log('Events failed. Expected ' + allEventsExpected + ' but was ' + allEvents);
                }
              }, 2000);
            });
          });
        });
      });
    });
  });
});

When running this code in latest Firefox stable on Mac, I see the hapi error above. I don't see it in Chrome. But even if wrap the line with a try / catch and fallback data to {}, the script above does not succeed, there is an error:unauthenticated triggered somewhere around the account.destroy call

Hope that helps.

@gr2m
Copy link
Member

gr2m commented Sep 19, 2014

I've git pushed -d'd to the hoodie.js pull request, which resolves the problem with the error:unauthenticated event. Looking into this hapi error now

gr2m added a commit to michielbdejong/hoodie-server that referenced this pull request Sep 19, 2014
@gr2m
Copy link
Member

gr2m commented Sep 19, 2014

Okay, all integration tests pass now, but I had to hack the hapi exception mentioned above, see a69586a

I'd like someone review this before we merge.

michielbdejong added a commit to michielbdejong/hoodie.js that referenced this pull request Sep 23, 2014
This PR does two simple things:
* look for an 'authToken' field in the sign-in response, and
    store this as hoodie.account.authToken
* send an 'Authorization' header with 'Bearer ' + that token in
    each request to the hoodie-server.

This PR goes together with a PR on the hoodie-server repo, which
at the HAPI level, will:
* strip the couchdb cookie and add it into the JSON response of
    signIn requests.
* read the Authorization header to look for a token of the form
    'Bearer ...', and create a cookie header from the '...', to
    send to the couchdb layer.

set authToken from config at page load

fix hoodiehq/hoodie-server#288 (comment)
@gr2m gr2m force-pushed the michielbdejong:bearer-token-support branch from 84a402f to bf4f7c2 Sep 23, 2014
michielbdejong added a commit to michielbdejong/hoodie-server that referenced this pull request Sep 23, 2014
This PR takes auth token from the couchdb cookie and exposes it as a bearer
token at the HAPI level:
* When couchdb sets a cookie, it is stripped, and the authToken is added to
    the JSON body.
* When a request comes in, the 'Bearer ...' token is read from the Authorization
    header, and converted to a cookie for couchdb.

This happens at the hapi level.
The solution is based on hapijs/hapi#1637
and https://github.com/spumko/hapi/blob/master/docs/Reference.md

TODO:
* [ ] create a couchdb mock in the test framework
* [ ] use this to create a test for the `mapProxyPath` function
* [ ] use this to create a test for the `addCorsAndBearerToken` method
* [ ] add tests for the `extractToken` function

add basic tests for api plugin

tests for mapProxyPath

add test for extractToken

first draft of addCorsAndBearerToken tests

use done function in tests

use port number that is different from the one used in other integration tests

fix hoodiehq#288 (comment)

fix hoodiehq#288 (comment)

jshint and typo

fix merge errors from rebase

try to fix https://travis-ci.org/hoodiehq/hoodie.js/builds/34471428

fix typo in test
gr2m added a commit to michielbdejong/hoodie-server that referenced this pull request Sep 23, 2014
michielbdejong and others added 3 commits Jun 9, 2014
Switch from cookies to bearer tokens
This PR takes auth token from the couchdb cookie and exposes it as a bearer
token at the HAPI level:
* When couchdb sets a cookie, it is stripped, and the authToken is added to
    the JSON body.
* When a request comes in, the 'Bearer ...' token is read from the Authorization
    header, and converted to a cookie for couchdb.

This happens at the hapi level.
The solution is based on hapijs/hapi#1637
and https://github.com/spumko/hapi/blob/master/docs/Reference.md

BREAKING CHANGE:

Hoodie is not using cookies for authentication anymore and relies
on hoodie.js@^1.0.0
@gr2m gr2m force-pushed the michielbdejong:bearer-token-support branch from bf4f7c2 to 1f50385 Sep 23, 2014
gr2m added a commit that referenced this pull request Sep 23, 2014
@gr2m
Copy link
Member

gr2m commented Sep 23, 2014

@michielbdejong are you around by chance? We have 4 of the unit tests failing now and are quite puzzled

image

@michielbdejong
Copy link
Contributor Author

michielbdejong commented Sep 24, 2014

Sorry, was out of town for a few days. Will be on irc!

@gr2m
Copy link
Member

gr2m commented Sep 24, 2014

any update? Sorry for pushing, I just need to coordinate with @boennemann when we both have time to finish the merge and release the new versions, and I'll be gone Friday - Monday. Would be great to get it done by tomorrow evening.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.