Feature/add encryption #141
Conversation
mircohacker
commented
Jul 29, 2020
mircohacker
commented
- use nodes crypto library
- encrypt the session after serialization
- test the correct implementation
- closes encrypt cookies #9
There was a problem hiding this comment.
The same IV should never be reused between messages. A static IV is a security vulnerability. This is classified as CWE-329
README.md
Outdated
| @@ -81,6 +81,14 @@ change signature parameters like the algorithm of the signature. | |||
|
|
|||
| A string which will be used as single key if `keys` is not provided. | |||
|
|
|||
| ##### encryptionKey | |||
|
|
|||
| If set the cookie will be encrypted using this key. | |||
There was a problem hiding this comment.
I think this description is misleading, especially if folks are going to try and read the cookie in other programming languages. Looking at the code, the value here is not the encryption key, as it is fed into a hash function and that output is used as the key.
|
are there any other changes i should adress? Currently I am trying to make travis happy... |
mircohacker
force-pushed
the
feature/add-encryption
branch
from
fb5681d
to
a4305ce
Compare
|
I will not be able to review further until I am out of work for the day, so it will be at least 6 hours before I can review again. |
|
Take your time. I tried to create compatibility with as many node versions as possible, but before node v4 there was another now deprecated buffer constructor. Even node 4 is EOL since 2018-04-30 so I figured this should be ok. If you have another opinion let me know then I would add the package https://www.npmjs.com/package/buffer-from for a ponyfill. Is there a timeline for the next release? Currently, I am monkey patching this library to provide this functionality. |
|
Hi @mircohaug thanks for the update. I will try and review when I am off work tonight. I do not want to make any promises on a release date, but I can say that if this change requires dropping Node.js versions, the release date will certainly not be any time soon, as it is important that our Express middlewares from our organization supports the same Node.js versions as the current version of Express does, to provide a consistent experience across our project. |
* use nodes crypto library * encrypt the session after serialisation * initialisation vectors are prepended to the encrypted session * encryption key rotation is implemented * add the current node version to ci configuration * test the correct implementation * closes expressjs#9
mircohacker
force-pushed
the
feature/add-encryption
branch
from
bf8ddad
to
ae47b79
Compare
|
OK, what a ride. The implementation now supports all the required node versions. 🎉 I was not aware, that express officially supports node versions this old. I merged all the previous commits to a single on to declutter the commit history. |
|
@dougwilson any progres on this? |
|
Hi @mircohaug sorry, I am at work currently, so don't have much time, but I did take a bit of a look over it this weekend, and I didn't realize this module is using the There is a tracking issue for encryption at pillarjs/cookies#42 |
|
This is already implemented on the keygrip develop branch which is used by cookies. How can I help to bring this branch into master? |
