derive Copy and Clone for UnboundKey

[oconnor663]

OpeningKey and SealingKey intentionally avoid implementing Copy and
Clone, because they're attached to a nonce sequence that should be
unique. UnboundKey isn't attached to a nonce sequence, though, and in
the case of AES it also takes some nontrivial key setup work to
construct from raw bytes. Making it Copy and Clone allows lets callers
avoid repeating that work in cases where a key is reused without a fixed
nonce sequence.

---

Copying keys around is a risky business, which is why this hierarchy of types exists in the first place, and I wouldn't be surprised if there were some objections here. Maybe some folks would prefer to implement Clone but not Copy, for example. I figured it would be easier to start that conversation with some code than to just open an issue, so here's a first draft PR :)

[oconnor663]

Looks like something like this has been discussed before in #727 and #859, but some of that discussion was before LessSafeKey was added to the API, and I'm not sure whether circumstances might've changed.

One thing @briansmith mentioned is that ring wants to abstract over keys contained in keyrings and things. In that world, presumably we'd still like to give callers the option to avoid repeating the work of AES key setup? Where in the type hierarchy would it make sense to this sort of thing?

[codecov]

Codecov Report

Merging #1158 (0e54b85) into main (27200d4) will decrease coverage by 0.03%.
The diff coverage is 84.00%.

@@            Coverage Diff             @@
##             main    #1158      +/-   ##
==========================================
- Coverage   91.97%   91.94%   -0.04%     
==========================================
  Files         106      106              
  Lines       14238    14290      +52     
==========================================
+ Hits        13096    13139      +43     
- Misses       1142     1151       +9     

Impacted Files	Coverage Δ
src/aead/aes.rs	75.00% <0.00%> (-0.72%)	⬇️
src/aead/aes_gcm.rs	98.40% <0.00%> (-0.53%)	⬇️
src/aead/chacha.rs	98.47% <0.00%> (-0.76%)	⬇️
src/aead/gcm.rs	76.78% <33.33%> (-1.06%)	⬇️
src/aead.rs	75.98% <50.00%> (+1.96%)	⬆️
tests/aead_tests.rs	96.87% <97.56%> (+0.10%)	⬆️
src/agreement.rs	81.14% <0.00%> (-4.20%)	⬇️
src/hkdf.rs	81.91% <0.00%> (-1.79%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 27200d4...0e54b85. Read the comment docs.

[briansmith]

I think it might make sense for LessSafeKey to implement Clone. I don't see a need to implement Copy. Like you said, it seems wrong for UnboundKey to be either Clone or Copy since the UnboundKey may be converted into OpeningKey or SealingKey, which make assumptions about the uniqueness of the key w.r.t. the nonce sequence.

[briansmith]

Just to clarify further, UnboundKey exists only for the purpose of eventually converting it to an OpeningKey or SealingKey or LessSafeKey. In the event that it would be converted into an OpeningKey or SealingKey, it would be wrong to clone it, because then the nonce protection mechanism wouldn't work right. So, I think the only thing that would make sense would be to change LessSafeKey to implement Clone. However, I think applications can just use Arc<LessSafeKey> instead, and that would be better for almost all applications.

If somebody thinks that we need to support cloning OpeningKey/SealingKey (or UnboundKey) then I suggest we discuss that in an issue that clearly explains why it is important to support, and more important than our attempts to prevent nonce reuse.

I'm going to close this. If I've overlooked something, please let me know. Thanks!

[oconnor663]

However, I think applications can just use Arc<LessSafeKey> instead, and that would be better for almost all applications.

Is it a downside to require heap allocation in this case? Might not be ideal for libraries planning to support no_std. (Though I'll admit I don't personally plan to support no_std in the use case that led me to file this bug.)

[briansmith]

Is it a downside to require heap allocation in this case? Might not be ideal for libraries planning to support no_std. (Though I'll admit I don't personally plan to support no_std in the use case that led me to file this bug.)

Given that Arc<LessSafeKey> works and would be effectively equivalent to making LessSafeKey clonable, I I think it would be fine to implement Clone for LessSafeKey, especially since semantically it's no different than Arc<LessSafeKey> AFAICT.

[oconnor663]

Cool, I just opened a separate PR for that at #1175.