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

Change high-level auth to a wrapper around keyed Blake2b #88

Merged
merged 18 commits into from
Oct 7, 2019

Conversation

vlmutolo
Copy link
Contributor

@vlmutolo vlmutolo commented Sep 1, 2019

Let me know if this is what you had in mind for a "wrapper" around keyed Blake2b. I think this is what we discussed, but obviously I'm happy to make changes.

@vlmutolo
Copy link
Contributor Author

vlmutolo commented Sep 1, 2019

This relates to #58. Instead of changing the return type for Blake2b, it was wrapped such that the output digest was replaced with a Tag.

Copy link
Member

@brycx brycx left a comment

Choose a reason for hiding this comment

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

Thanks for your work on this. Apart from some minor details, the only major changes I think we need are:

  1. We want a 32-byte secret key and tag.
  2. I think it would be good to enforce a minimum 32-byte secret key, to avoid misuse.

Please see the comments for details.

src/auth.rs Outdated Show resolved Hide resolved
src/auth.rs Outdated Show resolved Hide resolved
src/auth.rs Outdated Show resolved Hide resolved
src/auth.rs Outdated Show resolved Hide resolved
src/auth.rs Outdated Show resolved Hide resolved
src/auth.rs Outdated Show resolved Hide resolved
src/auth.rs Show resolved Hide resolved
src/auth.rs Show resolved Hide resolved
src/hltypes.rs Outdated Show resolved Hide resolved
src/hltypes.rs Outdated Show resolved Hide resolved
@brycx brycx changed the title Changed high-level auth to a wrapper around keyed Blake2b. Change high-level auth to a wrapper around keyed Blake2b Sep 1, 2019
@brycx brycx added the breaking change A breaking change label Sep 1, 2019
@codecov
Copy link

codecov bot commented Sep 7, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@2b5106c). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #88   +/-   ##
=========================================
  Coverage          ?   95.77%           
=========================================
  Files             ?       36           
  Lines             ?     5612           
  Branches          ?        0           
=========================================
  Hits              ?     5375           
  Misses            ?      237           
  Partials          ?        0

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 2b5106c...250f18f. Read the comment docs.

@codecov
Copy link

codecov bot commented Sep 7, 2019

Codecov Report

Merging #88 into master will increase coverage by 0.01%.
The diff coverage is 97.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #88      +/-   ##
==========================================
+ Coverage   97.31%   97.32%   +0.01%     
==========================================
  Files          46       46              
  Lines        5590     5614      +24     
==========================================
+ Hits         5440     5464      +24     
  Misses        150      150
Impacted Files Coverage Δ
src/hltypes.rs 100% <ø> (ø) ⬆️
src/hazardous/hash/blake2b.rs 93.18% <ø> (+0.25%) ⬆️
src/auth.rs 98.18% <97.14%> (-1.82%) ⬇️
src/hazardous/kdf/hkdf.rs 98.78% <0%> (-1.22%) ⬇️
src/kdf.rs 97.14% <0%> (ø) ⬆️
src/hazardous/stream/chacha20.rs 95.23% <0%> (ø) ⬆️
src/aead.rs 100% <0%> (+1.21%) ⬆️

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 2492076...7b0fd2f. Read the comment docs.

@brycx
Copy link
Member

brycx commented Sep 9, 2019

For future reference: The reason that a 32-byte key and tag have been chosen is due to this being in line with what libsodium recommends in its generic-hash interface, as well as this answer.

@vlmutolo vlmutolo requested a review from brycx September 18, 2019 23:15
src/auth.rs Outdated Show resolved Hide resolved
src/auth.rs Show resolved Hide resolved
src/auth.rs Show resolved Hide resolved
@brycx
Copy link
Member

brycx commented Sep 22, 2019

libs.rs L35-36 also needs to be updated. Currently it says the following:

//! ## Message authentication
//! [`orion::auth`] offers message authentication and verification using HMAC.

This should now state the use of BLAKE2b.

Fixed in 16d0e94

@brycx brycx merged commit fe30f40 into orion-rs:master Oct 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change A breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants