Cipher X-CS-Auth token and X-CS-User, rename Headers and refactor

[icedfiend]

I've implemented 2 things:

First, made the genRandomString actually generate a cryptographically strong random string using crypto core library.

Second, the returned token on the header X-CS-Auth is now ciphered when sent to the client and deciphered when returned for session checking.

Might be a bit overkill though...

I'm planning on adding the option to customize the headers sent to the user if that makes any sense (changing X-CS-Auth for any other arbitrary header name, or hiding the X-CS-User header for example)

Great project :)

[deitch]

@IGZalejandropenedo this is interesting. Thanks for contributing.

A number of questions:

1. getRandomString - definitely makes a ton of sense. The original even had a comment not to rely on it too heavily.
2. cipher the token - why? What does it buy us? We are already sha1 one-way hashing the info so what does the cipher buy us? We already use the sessionKey inside the clear text fed to the hash function, so we are covered there. The only parts I think are weak are (a) lack of a salt and (b) sha1 is a weaker algorithm. But what does additional symmetric aes192 encryption buy us?
3. formatting - all good, as is updating the tests.
4. customer header - part of me is hesitant, since being consistent is good practice and makes debugging easier, especially if you use it across multiple services (I could see this doing some interesting SSO stuff), but who am I to argue with giving users choice?

Again, thanks for being part of this.

[icedfiend]

About the cipher, that's why I mentined it might be a bit overkill, but it seemed reasonable in some circumstances where someone might not want to expose any details of the user on the client more than necessary.

About the header customization, you need to consistently implement it across all your servers, but it's just a measure to further hide the implementation details of the server.

Anyway, feel free to take whatever seems reasonable, I'm more than glad to help :)

[deitch]

Interesting; I always specifically wanted to expose it. But I do understand why now.

Rather than always cipher it, can you make it an option in cs.init()? Say, {encryptHeader: true}, but the default is false. As long as it is a user option, I am quite comfortable with it, and can merge the request.

And add your name and GitHub user link to package.json under contributors (should probably bump the version number too); credit goes where it is due...

[icedfiend]

I will give a try when I manage to get some spare time.

Also, I will try to reflect some undocumented functionality, like clearing the session (for example, after a logout).

Also the validate and validatePass code seems to be the same on the sessionManager. Is there any sense on having both?

2013/11/24 Avi Deitcher notifications@github.com

Interesting; I always specifically wanted to expose it. But I do understand why now.

Rather than always cipher it, can you make it an option in cs.init()? Say, {encryptHeader: true}, but the default is false. As long as it is a user option, I am quite comfortable with it, and can merge the request.

And add your name and GitHub user link to package.json under contributors(should probably bump the version number too); credit goes where it is due...

— Reply to this email directly or view it on GitHubhttps://github.com/deitch/cansecurity/pull/5#issuecomment-29154148 .

[deitch]

validatePass was legacy, if you look at older versions. I kept it around for backwards-compatiiblity.

[deitch]

@IGZalejandropenedo did you get anywhere?

[icedfiend]

Sorry, I've been quite busy last 2 weeks with work and visits at home.

I made the changes and pushed it to the fork. Check it out and tell me if it's all good :)

[deitch]

Ouch. It's been 5 days since you commented? So sorry it took me so long to get back to you. Looking now...

[deitch]

So far, reviewing the code, looks pretty good. But you didn't:

1. Update the README to include the encryption of X-CS-Auth value as an option, how to invoke it, and how it is encrypted (the algorithm)
2. Update the README to show that X-CS-Auth is configurable, and who to configure it.
3. Update package.json to bump version number and add @IGZalejandropenedo as a contributor - credit where it is due!

Can you add those three comments, I will merge, run the full set of tests, and push back in.

Truly, great work!

[icedfiend]

Sorry for taking so long on this, I have no real excuse.

On the good side, I've pulled myself up to finish what I proposed and a few extra things:

• Extrated some constants (like header names) to a constants module to make it easier to share.
• Main headers (X-CS-Auth and X-CS-User) are now configurable.
• Encrypted X-CS-Auth header as well.
• Updated README.md to reflect the changes.
• Made a full refactor of the sessionManager module for the sake of readability. The public API is the same, just made it a bit more reader friendly. (Hope not to hurt anyone's feelings )

This raised a question and a problem:

• Does the X-CS-User header serves any other porpuse beyond exposing the user object on the browser and the use of getUser method? As useful as this might seem on the browser, it could be a security risk. Just a thought.
• The way NodeJS module cache works when requireing is leading the test-authtoken-encrypt test fail when executed with all the rest of the tests modules. Stand-alone, the tests pass successfully. The Cansecurity instance the encryptation test is receiving seems to have been already initialized and shared among all tests and have the encryptation disabled. I'm working on fixing this.

Feel free to comment on anything that might look extrange and again, sorry for the delay.

[deitch]

Ugh, I just realized I missed the notification on this. I'll get to it this week.

[deitch]

There was so much involved in all of this, I'm still trying to get my head around all of the changes. As far as I can see from this stream (thank God someone invented github), all of the original changes were good and valid. I am going to start with your original commits, add them and push, and then look at the additional changes.

[icedfiend]

Yhea, I think I went a little bit too far with the refactor :)

The only thing that I haven't had the time to solve is what I mentioned about test-authtoken-encrypt working stand-alone but not with the rest of the tests due to the way nodejs caches modules.

Next week I'm on vacation so I'll give it another try (vacations is the only time I have to work on any side project hehe)

[deitch]

Yeah, I think you did! I cannot believe you actually reformatted all of that for readability. In principle, if it passes all tests, I don't mind, but I always review the code manually to be sure, and it takes some time to do that. Hence it has been sitting in my inbox for almost a week.

[icedfiend]

Well, while implementing the the encryptation for the tokens, I found the code a bit difficult to follow at some points, and I just put in practice what we do at work...

"try to leave every code you work on a little bit cleaner than when you started"

[deitch]

I run into that all the time... and about half the time discover it is a stylistic difference. I never take offense - all personal preference, unless it is egregious - but reading through style changes is forever.

[deitch]

I am almost done reviewing the code...

[deitch]

Having gone through it all, I have one thing I don't understand.

In https://github.com/IGZalejandropenedo/cansecurity/commit/94e61e35b682e6405908550e8d8ef3ad58be57b7

You comment out some of the tests in test-locationsession.js. Why?

[icedfiend]

Ugh... Probably an error I was having when doing the ecryptation code and I totally forgot about it... I'll get to it this friday and fix the tests.

I'm sorry

[deitch]

I am reviewing now - there are some merge conflicts, which will be a pain to resolve - but once I do, uncommenting those should be easy.

[deitch]

Whew. I managed to remove the localsession changes, had to fix a few things, some strange merge conflicts, all that is left is the errors on authtoken-encrypted...

[icedfiend]

Great, sorry for all the hassle... If you manage to find a workaround for it let me know, otherwise I'll get to it this friday El 06/04/2014 12:07, "Avi Deitcher" notifications@github.com escribió:

Whew. I managed to remove the localsession changes, had to fix a few things, some strange merge conflicts, all that is left is the errors on authtoken-encrypted...

Reply to this email directly or view it on GitHubhttps://github.com/deitch/cansecurity/pull/5#issuecomment-39663697 .

[deitch]

No worries. In resolving conflicts, some of your new formatting went by the wayside; just a question of preference (I find some styles easier to work with), but I didn't remove it just to do so.

Can you explain the problem with the authtoken-encrypted test?

[icedfiend]

The problem is that when passing the test along with the rest, the instance of cansecurity I receive is one without encryptation enabled. Passed stand alone I get the right instance with encryptation enabled.

That probably has to do with the way nodejs caches moduled and requires El 06/04/2014 12:17, "Avi Deitcher" notifications@github.com escribió:

No worries. In resolving conflicts, some of your new formatting went by the wayside; just a question of preference (I find some styles easier to work with), but I didn't remove it just to do so.

Can you explain the problem with the authtoken-encrypted test?

Reply to this email directly or view it on GitHubhttps://github.com/deitch/cansecurity/pull/5#issuecomment-39663891 .

[deitch]

No, I get the same problem even when I run it standalone. It is something else. I think the test may be valid but the enciphering is failing.

[icedfiend]

That's weird... They were passing stand-alone when I pushed them

I'll take a look asap, I'm on the train right now hehe El 06/04/2014 12:33, "Avi Deitcher" notifications@github.com escribió:

No, I get the same problem even when I run it standalone. It is something else. I think the test may be valid but the enciphering is failing.

Reply to this email directly or view it on GitHubhttps://github.com/deitch/cansecurity/pull/5#issuecomment-39664154 .

[deitch]

Oh, that is funny. I just figured it out. It isn't the caching at all. Curious?

[deitch]

Truth is, it is a problem across all tests, but did not materialize until you ran it separately. Bug in the tests, but you tripped it up.

[icedfiend]

Yes, indeed. I replicated the function you use to initialize everything on the tests and enabled the encrytion on that one... El 06/04/2014 12:42, "Avi Deitcher" notifications@github.com escribió:

Oh, that is funny. I just figured it out. It isn't the caching at all. Curious?

Reply to this email directly or view it on GitHubhttps://github.com/deitch/cansecurity/pull/5#issuecomment-39664275 .

[deitch]

So here it is. You can rerun cs.init() and it will reinitialize, no problem.

But each test file has the following (or similar):

var cansec = require('cansecurity').init();
// later
before(function()) {
  app.use(cansec.validate());
}

in the test var section wrapping the entire file, and so gets called for every single file, in alphabetical order, before running the tests, even if you don't actually use that test file. The functional init() for cansec, then, becomes whichever file is last.

The correct thing to do is to apply the init() inside the before() clause, and thus make it apply only to that test suite:

var cansec = require('cansecurity');
// later
before(function()) {
  cansec = cansec.init();
  app.use(cansec.validate());
}

[deitch]

I just fixed it on my laptop, it works beautifully. Pushing in a minute or two.

[icedfiend]

Great! How come I didn't notice that...

Thanks for the fix :) El 06/04/2014 12:52, "Avi Deitcher" notifications@github.com escribió:

I just fixed it on my laptop, it works beautifully. Pushing in a minute or two.

Reply to this email directly or view it on GitHubhttps://github.com/deitch/cansecurity/pull/5#issuecomment-39664466 .

[deitch]

Same reason I didn't. It didn't matter until now.

[deitch]

All in, as of https://github.com/deitch/cansecurity/commit/6aee4f0da6128001e1ada458731753f01f5b9509

[icedfiend]

Great :)

Actually I had a similar problem in the tests of the project I'm working on and that is using cansecurity. But as I thought it was the cache I didn't get to relate it with the init function... El 06/04/2014 12:58, "Avi Deitcher" notifications@github.com escribió:

All in, as of 6aee4f0https://github.com/deitch/cansecurity/commit/6aee4f0da6128001e1ada458731753f01f5b9509

Reply to this email directly or view it on GitHubhttps://github.com/deitch/cansecurity/pull/5#issuecomment-39664567 .