splone / splonebox-client

A client for the splonebox
http://splonebox.io
GNU Lesser General Public License v3.0
6 stars 2 forks source link

Implement crypto stack #14

Closed stze closed 8 years ago

stze commented 8 years ago

This PR introduces the client crypto implementation. Please review.

bontric commented 8 years ago

Styling looks good, documentation looks great. You can also annotate the return type of functions:

def crypto_random_mod(number: int) -> int:   

(I didn't do this everywhere yet, but I will update wherever it's missing)

The tests are failing since there is no public key in the repository

FileNotFoundError: [Errno 2] No such file or directory: '.keys/server-long-term.pub'

I guess a temporary key should be created before the tests and removed afterwards.

We could implement a mechanism to set the location of the key when creating the plugin (using an optional parameter) and maybe also allowing the key to be added in the code. Something like this:

def __init__(self,
                    plugin_id: str,
                    name: str,
                    desc: str,
                    author: str, 
                    licence: str,
                    debug=False,
                    key_location='.keys',
                    server_longtermpk=None):  

I can take care of that but I'd like to hear your opinion on this :wink:

Everything else :+1:

stze commented 8 years ago

I guess a temporary key should be created before the tests and removed afterwards.

We could implement a mechanism to set the location of the key when creating the plugin (using an optional parameter) and maybe also allowing the key to be added in the code. Something like this:

def __init__(self,
                    plugin_id: str,
                    name: str,
                    desc: str,
                    author: str, 
                    licence: str,
                    debug=False,
                    key_location='.keys',
                    server_longtermpk=None):  

I can take care of that but I'd like to hear your opinion on this :wink:

would be great if you can implement the "key-initialization" mechanism and add it to this branch. you can generate a valid keypair with sb-makekey tool from splonebox-core repository. thx for reviewing :wink:

mkind commented 8 years ago

Please verify that the python crypto stack properly destroys sensitive information in memory.

some proposals found in the internet :)

stze commented 8 years ago

@bontric is this PR in progress? otherwise i would assign this to @mkind

mkind commented 8 years ago

It might be a good idea to write tests for the crypto stack before pushing to master, isn't it?

bontric commented 8 years ago

@stze I'm on it. Had to fix the tests first, now I have to write tests for the crypto stack ( :+1: @mkind) @mkind

Please verify that the python crypto stack properly destroys sensitive information in memory.

will take a look at that

bontric commented 8 years ago

Okay, I added the tests for the crypto class and added some minor tweaks. @stze i think it'd be good if you take a look at the tests and see if there is something missing :wink:

Oh, and to run the tests, navigate to the projects root folder and type:

$ export PYTHONPATH=. && python test/run_tests.py

(also make sure that .keys/server-long-term.pub exists, I'll implement a better mechanism for this soon)

mkind commented 8 years ago

also make sure that .keys/server-long-term.pub exists

yes, this is a problem also in the core testing. maybe it is possible to create a dummy key directory during tests.

stze commented 8 years ago

@stze I'm on it

great :+1:

stze commented 8 years ago

yes, this is a problem also in the core testing. maybe it is possible to create a dummy key directory during tests.

in my opinion this is not a problem. it's a "install or init" prerequisite to put the key in .keys before testing. i'm not sure if it's a good idea to create dummy keys.

stze commented 8 years ago

@bontric would be great if you can update the nonce generate and verify mechanisms. first, client nonces should all be odd, so check after the random nonce generation if the random number is odd, otherwise add +1. additionally you need to update the nonce update function to always add +2 (otherwise the nonce is not always odd). second, received server nonces should all be even, so check if they are even, otherwise return failure. :yellow_heart:

here is the server commit, client is vice versa: https://github.com/splone/splonebox-core/pull/29/commits/9a450738d0d94382611e0606549820c0b40958d6

bontric commented 8 years ago

yes, this is a problem also in the core testing. maybe it is possible to create a dummy key directory during tests. in my opinion this is not a problem. it's a "install or init" prerequisite to put the key in .keys before testing. i'm not sure if it's a good idea to create dummy keys.

@stze, @mkind It's not a problem. There is no need to create a file containing the key for testing. I've already done temporary key creation for tests in test_crypto since I need a private/public key pair to en/de-crypt messages. The keys are passed by argument or overwritten manually. See here

@bontric would be great if you can update the nonce generate and verify mechanisms. first, client nonces should all be odd, so check after the random nonce generation if the random number is odd, otherwise add +1. additionally you need to update the nonce update function to always add +2 (otherwise the nonce is not always odd). second, received server nonces should all be even, so check if they are even, otherwise return failure. :yellow_heart:

I'll look into it

stze commented 8 years ago

@bontric the crypto_nonce_update function is incorrect. the nonce needs to be incremented always by 2, because the nonce needs to be odd.

stze commented 8 years ago

@bontric the crypto_nonce_update function is incorrect. the nonce needs to be incremented always by 2, because the nonce needs to be odd.

i've fixed it