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

Feature/update to draft 03 #34

Merged
merged 8 commits into from
Oct 28, 2023
Merged

Feature/update to draft 03 #34

merged 8 commits into from
Oct 28, 2023

Conversation

TobTheRock
Copy link
Contributor

@TobTheRock TobTheRock commented Sep 2, 2023

@TobTheRock TobTheRock requested a review from hoodie September 2, 2023 15:47
[
SFRAME_LABEL,
SFRAME_HKDF_KEY_EXPAND_INFO,
&key_id.to_be_bytes(),
Copy link
Contributor

Choose a reason for hiding this comment

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

is this length constant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is actually. We need to allocate here however (and can't return an iterator), since openssl expects a slice for:https://docs.rs/openssl/latest/openssl/pkey_ctx/struct.PkeyCtx.html#method.set_hkdf_salt.

Creating an array on the stack makes the function quite complex:

    if #[cfg(feature = "openssl")] {
        pub const SFRAME_HKDF_SUB_ENC_EXPAND_INFO: &[u8] = b"enc";
        pub const SFRAME_HDKF_SUB_AUTH_EXPAND_INFO: &[u8] = b"auth";

        pub fn get_hkdf_aead_label(tag_len: usize) -> [u8; AEAD_LABEL_SIZE] {
            let mut label = [0; AEAD_LABEL_SIZE];
            label[..SFRAME_HDKF_SUB_AEAD_LABEL.len()].copy_from_slice(SFRAME_HDKF_SUB_AEAD_LABEL);
            // for current platforms there is no issue casting from usize to u64
            label[SFRAME_HDKF_SUB_AEAD_LABEL.len()..].copy_from_slice(&(tag_len as u64).to_be_bytes());
            label

        }
        const SFRAME_HDKF_SUB_AEAD_LABEL: &[u8] = b"SFrame 1.0 AES CTR AEAD ";
        const U64_BYTES:usize = (u64::BITS as usize)/8;
        const AEAD_LABEL_SIZE : usize = SFRAME_HDKF_SUB_AEAD_LABEL.len() + U64_BYTES;
    }

At least that was the best I could come up with.
I think the extra complexity is not really worth it, since the key derivation is not performance critical, so I would rather keep it as is.


// For the current test vectors different labels than specified were used
// see https://github.com/sframe-wg/sframe/issues/137
cfg_if::cfg_if! {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@TobTheRock TobTheRock Oct 16, 2023

Choose a reason for hiding this comment

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

Unfortunately this is still nightly: rust-lang/rust#115585
I will keep it in mind though ;)

let ct_len = &(encrypted.len() as u64).to_be_bytes();
let tag_len = &(self.auth_tag_len as u64).to_be_bytes();

for buf in [aad_len, ct_len, tag_len, nonce, aad, encrypted] {
Copy link
Contributor

Choose a reason for hiding this comment

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

loop needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I double checked, and there is no other way than to allocate another buffer then. Openssl requires continous slice. However encrypted can be quite large as it holds the entire encrypted frame. Thus I would refrain from another allocation and just keep the loop.

P.s. I also tried an intermediate solution only copying
aad_len, ct_len, tag_len, nonce, aad into a new buffer to reduce the amount of function calls. However I could see no real difference in the benchmarks at least on my system. At some point we might want to come back to testing WASM performance, not sure how it behaves there. But that is whole topic for it self ...

for buf in [&aad_len, &ct_len, nonce, aad, encrypted] {
let aad_len = &(aad.len() as u64).to_be_bytes();
let ct_len = &(encrypted.len() as u64).to_be_bytes();
let tag_len = &(self.auth_tag_len as u64).to_be_bytes();
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a .as_bytes() equivalent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately this is not possible as it not agnostic on which system you are compiling (Big Endian vs. Little Endian).

So what to_be_bytes() does is to call self.to_be().to_ne_bytes()
where

        #[inline]
        pub const fn to_ne_bytes(self) -> [u8; mem::size_of::<Self>()] {
            // SAFETY: integers are plain old datatypes so we can always transmute them to
            // arrays of bytes
            unsafe { mem::transmute(self) }
        }

        pub const fn to_be(self) -> Self { // or not to be?
            #[cfg(target_endian = "big")]
            {
                self
            }
            #[cfg(not(target_endian = "big"))]
            {
                self.swap_bytes()
            }
        }

As you see, the original value is implicitly copied, where the copy is then consumed as there might need to be some byte swaping. Due to the consuming it is a to operation.

Tobias Waurick and others added 5 commits October 21, 2023 17:22
With the newest draft now also the key id/auth tag length is used during the key derivation
to create distinct salts. Also for AES CTR mode ciphers the tag length is also taken into
account when computing the tag

BREAKING CHANGE:
The latest [changes in the draft](https://author-tools.ietf.org/diff?doc_1=draft-ietf-sframe-enc-01&doc_2=draft-ietf-sframe-enc-03) regarding the key derivation and tag computation, make theimplementation incompatible with previous versions
so the name matches the one in the spec

BREAKING CHANGE:

Instead of `SframeError.KeyExpansion` use `SframeError.KeyDerivation`
upstream bug was closed
@TobTheRock TobTheRock force-pushed the feature/update-to-draft-03 branch from 3de3423 to 35c86bd Compare October 21, 2023 21:38
@TobTheRock TobTheRock force-pushed the feature/update-to-draft-03 branch from 35c86bd to e4c8a6e Compare October 22, 2023 15:11
@TobTheRock TobTheRock requested a review from hoodie October 22, 2023 15:31
@TobTheRock TobTheRock merged commit 3ff961e into main Oct 28, 2023
@TobTheRock TobTheRock deleted the feature/update-to-draft-03 branch October 28, 2023 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants