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

update symmetric to use naci instead #18

Merged
merged 11 commits into from Mar 20, 2014

Conversation

cvasilak
Copy link
Contributor

done for AGIOS-172

@@ -30,7 +30,7 @@ pod "AeroGear-Crypto", '0.2.0'
## Project Status
The following services are currently provided:

* A [Symmetric encryption](http://en.wikipedia.org/wiki/Symmetric-key_algorithm) interface
* A [Symmetric encryption](http://nacl.cr.yp.to/secretbox.html) interface
* An [Asymmetric encryption interface](http://nacl.cr.yp.to/box.html)
* Password based key generation using [PBKDF2](http://en.wikipedia.org/wiki/PBKDF2)
* Generation of Cryptographically secure [random numbers](http://en.wikipedia.org/wiki/Cryptographically_secure_pseudorandom_number_generator).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/Cryptographically/cryptographically/

@corinnekrych
Copy link
Contributor

+1

@corinnekrych
Copy link
Contributor

Actually trying to use this version in AeroGear-iOS (replacing AGCryptoBox by AGSecretBox ) I run into issue in [1] when trying to initialize a crypto box with a KDF derived key because its length (160bytes does not comply with [2])

[1] https://github.com/aerogear/aerogear-ios/blob/master/AeroGear-iOS/security/AGPassphraseEncryptionServices.m#L32
[2] https://github.com/cvasilak/aerogear-crypto-ios/blob/cc-naci/crypto-sdk/AGSecretBox.m#L26

@abstractj
Copy link
Contributor

@cvasilak the PR looks, however the test with @corinnekrych's scenario is failing to me. Please take a look:

    it(@"should accept KDF keys", ^{

        AGPBKDF2 *pbkdf2 = [[AGPBKDF2 alloc] init];
        NSString *PASSWORD = @"My Bonnie lies over the ocean, my Bonnie lies over the sea";
        NSData *salt = [AGRandomGenerator randomBytes];
        NSData *key = [pbkdf2 deriveKey:PASSWORD salt:salt];

        NSData *nonce = [AGUtil hexStringToBytes:BOX_NONCE];
        NSData *message = [AGUtil hexStringToBytes:BOX_MESSAGE];

        secretBox = [[AGSecretBox alloc] initWithKey:key];

        NSData *cipherText = [secretBox encrypt:message nonce:nonce];

        //Create a new box to test end to end symmetric encryption
        AGSecretBox *pandora = [[AGSecretBox alloc] initWithKey:key];
        NSData *plainText = [pandora decrypt:cipherText nonce:nonce];
        [[[AGUtil hexString:plainText] should] equal:BOX_MESSAGE];
    });

I was concerned about SecretBox in C, so I did the same experiment with Kalium (abstractj/kalium@2a07397) and seems to be working correctly.

@corinnekrych
Copy link
Contributor

Using @abstractj test case on @cvasilak brnahc and removing key length check:
https://github.com/cvasilak/aerogear-crypto-ios/blob/cc-naci/crypto-sdk/AGSecretBox.m#L38
and removing statuscode return assert:
https://github.com/cvasilak/aerogear-crypto-ios/blob/cc-naci/crypto-sdk/AGSecretBox.m#L50
I manage to have a success test.

I wonder if the status code == 0 really means a failure....

@cvasilak
Copy link
Contributor Author

the error here seems to be the incorrect key length specified when initialising SecretBox, making the assertion to fail. It expects 32 bytes but the default AGPBKDF2 class generates 160 bytes.

@abstractj the DERIVED_KEY_LENGTH used in Pbkdf2 is 32 bytes (256 bits). Replacing here with that size, makes the test run. Unfortunately, I can't recall why the use of 160 bytes as the minimum derived key.

I think this should be replaced on our side with the correct 32, wdyth?

@abstractj
Copy link
Contributor

@cvasilak aloha, I think is the way to go. Once the SecretBox will expect bytes, instead of bits. Thanks for the explanation

@corinnekrych
Copy link
Contributor

+1
works like a charm.
As easy as changing the size :)

@matzew
Copy link
Contributor

matzew commented Mar 20, 2014

Gave @cvasilak branch a shot -> 👍

@matzew
Copy link
Contributor

matzew commented Mar 20, 2014

Oh, should we change all the 5.0 references, like:
https://github.com/cvasilak/aerogear-crypto-ios/blob/cc-naci/crypto-sdk.xcodeproj/project.pbxproj#L515

to 7.0 ?

@cvasilak
Copy link
Contributor Author

thanks @matzew done

@abstractj abstractj merged commit 1b073df into aerogear-attic:master Mar 20, 2014
@abstractj
Copy link
Contributor

@cvasilak landed, thank you

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