Hdk encrypt/decrypt functions

[StaticallyTypedAnxiety]

PR summary

In this Pr we implement and refactor crypto functions in the hdk

testing/benchmarking notes

( if any manual testing or benchmarking was/should be done, add notes and/or screenshots here )

followups

( any new tickets/concerns that were discovered or created during this work but aren't in scope for review here )

changelog

Please check one of the following, relating to the CHANGELOG-UNRELEASED.md

• this is a code change that effects some consumer (e.g. zome developers) of holochain core so it is added to the CHANGELOG-UNRELEASED.md (linked above), with the format - summary of change [PR#1234](https://github.com/holochain/holochain-rust/pull/1234)
• this is not a code change, or doesn't effect anyone outside holochain core development

[StaticallyTypedAnxiety]

More refactors need to be done to DRY up these functions

[zippy]

Minor fixes to missed renames in comments. Looks great.

[neonphog]

I think we need the output to be a parameter, so that the implementer can decide if the data need to be protected or not.

something like:

pub fn decrypt(&mut self, cipher: &mut SecBuf, result: &mut SecBuf) -> HcResult<()> {

let res = SecBuf::with_secure(cipher.len() + ABYTES);
store.decrypt(&mut cipher, &mut res)?;

For symmetry, we could also make the output of encrypt be a param, but that's not strictly needed, since the encrypted data shouldn't need to be protected.

[StaticallyTypedAnxiety]

The reason I implemented it this way was because we have a few unsafe spots deep in the encryption code and thought it would be better to reduce the area of mutability by just returning the new encrypted data. That way that way they have a new cleaner block of data that minimizes the side effects

[neonphog]

I don't understand this reasoning. The unsafe code is executed in both cases, either by us or by the user of this api. We need to make sure that is solid through whatever testing is needed, but is also not related to my comment above.

The current code does not allow the user to memory protect the results of the decryption. If they are decrypting sensitive data from somewhere, they may need to use with_secure instead of with_insecure which the current api does not allow.

[StaticallyTypedAnxiety]

I guess the comment was related to the output type itself, I would still like to output a new piece of data just to be a bit closer to determinism but i guess it is okay since rust compiler will always take care of shared mutability.

How do you feel about an enum just to make it more rust style?

decrypt(&mut self, cipher,CryptoOptions::Secure) -> HcResult

and crypto options could be Secure and InSecure?

[neonphog]

I swear I'm not trying to be argumentative, lol : ) But, I don't see how the determinism is different in either case, and I don't like the dissonance of implementing the HDK api in a different manner than the underlying crypto api.

I feel like I'm missing some context for why this is a problem. Is this specifically because of the unsafe copy in the write function: std::ptr::copy(data.as_ptr(), (**b).as_mut_ptr().add(offset), data.len());?

I misspoke earlier when I said that would happen in either case. You are correct, it doesn't get called when creating a new SecBuf. That code was originally a for-loop copy of the bytes, but that can't take advantage of memory block size optimizations in the processor, the ptr copy is far more efficient, and there is an explicit bonds check above it, so there shouldn't be any real unsafety (though if you think of additional unit tests needed, please feel free to add them : )

Ultimately, though, even rust std code uses mem copy techniques at its lowest levels, if there's an efficiency reason to do it, I don't feel like we should fear diving in.

I'm just making things up at this point... please let me know if I'm totally off the mark. Thanks!

[StaticallyTypedAnxiety]

I have made the changes though and even though I have my opinions on rust and unsafety, you have worked on this side of the code more and I trust your expertise!

[lucksus]

Great! This looks good basically.
Request changes though that @neonphog pointed to, as well as having the documentation reflect the state of the implementation..