Peer Profiles should be protected by password, Configurations encrypted #23

New Issue

sarah · 2018-05-29T21:05:49Z

sarah commented

2018-05-29 21:05:49 +00:00

No description provided.

angus commented

2018-06-13 01:26:22 +00:00

Are you referring to the loading of the profiles to be encrypted/pw protected, forming the peer to peer connections should be password protected, or both?

sarah commented

2018-06-13 01:30:59 +00:00

Profiles, I've updated the title to clarify.

angus commented

2018-06-13 20:35:12 +00:00

I can work on this. Plan is to implement in the cwtch_peer.go package where the objects are being saved and loaded from files. Let me know if you have other thoughts.

sarah commented

2018-06-13 20:39:12 +00:00

Awesome!

angus commented

2018-06-17 15:38:17 +00:00

This is ready to go. I can't seem to make branches via the web app here: if thats a privileges thing, or if I'm being an idiot, please let me know. This is my first time collaborating on Git and programming in Go so be gentle :)

The password is created by the user when a new profile is created: it is encrypted, then stored in the Peer object. It is then used to save the profile to disk. It is never stored itself, consequently can never be recovered.
When a user wishes to load a file, the password is requested from the user and encrypted, then this encrypted variable used to decrypt the file and the encrypted password added to the profile in memory.

Known issue here: password is currently stored in memory as an MD5. We may want to change that algorithm to something more secure at a later date.

Changes to three files:
cwtch_peer:

Added encrypt and decrypt functions using AES and GCM, nonce prepended. Based on this: https://www.thepolyglotdeveloper.com/2018/02/encrypt-decrypt-data-golang-application-crypto-packages/
Altered NewCwtchPeer and LoadCwtchPeer to accept a password argument
Added password variable in Peer struct to store the encrypted password during the session
Altered Save function to remove the password variable from a Peer object copy before saving

main.go:

Altered QUIT condition to only run Save if profilefile not blank (fixed bug on quitting when no profile set)
Added password fetching logic to NEW PROFILE and LOAD PROFILE conditions

app.go:

Created new function CreateHash, referenced by main.go, to MD5 hash the password for passing to the Peer objects. The program structure seems to not have separate functions in main.go so this function was placed here.
Added password arguments to the SetProfile and NewProfile functions

This is ready to go. I can't seem to make branches via the web app here: if thats a privileges thing, or if I'm being an idiot, please let me know. This is my first time collaborating on Git and programming in Go so be gentle :) The password is created by the user when a new profile is created: it is encrypted, then stored in the Peer object. It is then used to save the profile to disk. It is never stored itself, consequently can never be recovered. When a user wishes to load a file, the password is requested from the user and encrypted, then this encrypted variable used to decrypt the file and the encrypted password added to the profile in memory. Known issue here: password is currently stored in memory as an MD5. We may want to change that algorithm to something more secure at a later date. Changes to three files: cwtch_peer: 1. Added encrypt and decrypt functions using AES and GCM, nonce prepended. Based on this: https://www.thepolyglotdeveloper.com/2018/02/encrypt-decrypt-data-golang-application-crypto-packages/ 2. Altered NewCwtchPeer and LoadCwtchPeer to accept a password argument 3. Added password variable in Peer struct to store the encrypted password during the session 4. Altered Save function to remove the password variable from a Peer object copy before saving main.go: 1. Altered QUIT condition to only run Save if profilefile not blank (fixed bug on quitting when no profile set) 2. Added password fetching logic to NEW PROFILE and LOAD PROFILE conditions app.go: 1. Created new function CreateHash, referenced by main.go, to MD5 hash the password for passing to the Peer objects. The program structure seems to not have separate functions in main.go so this function was placed here. 2. Added password arguments to the SetProfile and NewProfile functions

sarah commented

2018-06-17 16:42:29 +00:00

Awesome! I've updated permissions you should be able to push a branch and submit a pull request now. Let me know if you are still having issues.

dan commented

2018-06-17 16:42:54 +00:00

So yeah the web interface doesn't support branch managment which is fine.

The usual work flow is you hit the 'fork' button in the web interface on cwtch.im/cwtch and that will get you your own repo, angus/cwtch

then on the command line or in your IDE or favourite git management tool after checking out your repo you create a branch there and push to your repo and then in the web interface create a pull request on cwtch.im/cwtch

it would look like

'fork'
git pull gogs@git.openprivacy.ca:angus/cwtch.git
git checkout -b profile-password
do the work
git add ... the work
git commit
git push -u origin profile-password (the -u is setting the upstream tracking branch for the first time, after that you can just git push)
create the PR in the web interface

(usually what i eventually do is git remote add cwtch https://git.openprivacy.ca/cwtch.im/cwtch.git so I can pull updates from it like git pull cwtch)

however I am going to guess you did a git clone of cwtch.im/cwtch directly? so, slightly modified since I assume you have the work either commited to master locally or not commited but done

'fork'
git remote add angus gogs@git.openprivacy.ca:angus/cwtch.git
git checkout -b profile-password (the -b forces branch creation. could be done in two steps as git branch profile-password followed by git checkout profile-password)
[if you havent commited, you can now git add the files and git commit. if you already had it's fine, that history will be here
`git push -u origin profile-password
then create a PR on cwtch.im/cwtch from your branch

Hope that helps!

Also before pushing you can run testing/tests.sh to run all the unit tests, and if you have tor setup to be controllable by apps, you can also run go test in testing to run the integration test
Then mention it in the PR so we know stuff didnt break :)

So yeah the web interface doesn't support branch managment which is fine. The usual work flow is you hit the 'fork' button in the web interface on cwtch.im/cwtch and that will get you your own repo, angus/cwtch then on the command line or in your IDE or favourite git management tool after checking out your repo you create a branch there and push to your repo and then in the web interface create a pull request on cwtch.im/cwtch it would look like - 'fork' - `git pull gogs@git.openprivacy.ca:angus/cwtch.git` - `git checkout -b profile-password` - do the work - `git add ` ... the work - `git commit` - `git push -u origin profile-password` (the -u is setting the upstream tracking branch for the first time, after that you can just `git push`) - create the PR in the web interface (usually what i eventually do is `git remote add cwtch https://git.openprivacy.ca/cwtch.im/cwtch.git` so I can pull updates from it like `git pull cwtch`) however I am going to guess you did a git clone of cwtch.im/cwtch directly? so, slightly modified since I assume you have the work either commited to master locally or not commited but done - 'fork' - `git remote add angus gogs@git.openprivacy.ca:angus/cwtch.git` - `git checkout -b profile-password` (the -b forces branch creation. could be done in two steps as `git branch profile-password` followed by `git checkout profile-password`) - [if you havent commited, you can now `git add ` the files and `git commit`. if you already had it's fine, that history will be here - `git push -u origin profile-password - then create a PR on cwtch.im/cwtch from your branch Hope that helps! Also before pushing you can run testing/tests.sh to run all the unit tests, and if you have tor setup to be controllable by apps, you can also run `go test` in testing to run the integration test Then mention it in the PR so we know stuff didnt break :)

angus commented

2018-06-17 18:11:34 +00:00

Awesome, thank you. Mostly set up now. Just realized I haven't fixed up main.go in TUI so I'll need to do that...

angus commented

2018-06-18 02:52:29 +00:00

Tests all passed. Will pull request once I can figure out what I've done with my git.

sarah commented

2018-06-18 19:52:43 +00:00

Some thoughts on this:

MD5 is not appropriate to use as a key derivation function. Use an actual key derivation primitive: https://godoc.org/golang.org/x/crypto/pbkdf2
I'm not sure what benefit we get from introducing AES/GCM vs. using Nacl secret box primitive which we already use elsewhere. I'd like to keep the number of distinct cryptographic primitives as small as possible.

Some thoughts on this: * MD5 is not appropriate to use as a key derivation function. Use an actual key derivation primitive: https://godoc.org/golang.org/x/crypto/pbkdf2 * I'm not sure what benefit we get from introducing AES/GCM vs. using Nacl secret box primitive which we already use elsewhere. I'd like to keep the number of distinct cryptographic primitives as small as possible.

angus commented

2018-06-18 23:11:00 +00:00

The sole reason is inexperience on my part so I was leveraging work I could find elsewhere. Apologies. I'll look at the Nacl and will use the key derivation primitive you've listed above, as well as look into the theory behind both. Thanks for your patience and help with this.

If there is specific reading you'd recommend I look into, please let me know.

The sole reason is inexperience on my part so I was leveraging work I could find elsewhere. Apologies. I'll look at the Nacl and will use the key derivation primitive you've listed above, as well as look into the theory behind both. Thanks for your patience and help with this. If there is specific reading you'd recommend I look into, please let me know.

sarah commented

2018-06-18 23:22:39 +00:00

No worries! I should have been more specific about the design requirements when I created the issue.

angus commented

2018-06-23 00:29:22 +00:00

Thanks again for your help. The Group package was the one package I hadn't reviewed... derp. I've adapted the logic and library from there and have brought in the PBKDF2 library.

So all implemented now with one design question: how would you like the passwords managed? I had previously been passing them through and not storing them anywhere, however incorporating PBKDF2 and salts I'll need to hard code the salt to do the same. If the salt is to be randomly generated however, I'll need to store the salts somewhere for decryption, and consequently encrypt the salt file. Do you have a preference? I feel the former is probably adequate considering the password will remain in memory rather than storing on disk but let me know (or if I'm overlooking something again)

Thanks again for your help. The Group package was the one package I hadn't reviewed... derp. I've adapted the logic and library from there and have brought in the PBKDF2 library. So all implemented now with one design question: how would you like the passwords managed? I had previously been passing them through and not storing them anywhere, however incorporating PBKDF2 and salts I'll need to hard code the salt to do the same. If the salt is to be randomly generated however, I'll need to store the salts somewhere for decryption, and consequently encrypt the salt file. Do you have a preference? I feel the former is probably adequate considering the password will remain in memory rather than storing on disk but let me know (or if I'm overlooking something again)

sarah commented

2018-06-23 02:44:02 +00:00

I think randomly generating the salt and then storing it pre-pended to the profile file (or in a separate file) is the best approach here. Hardcoded salts leave the door open to wider attack vectors - where as per-peer random salts decrease the scope.