KBKDF: add CounterLocation.MiddleFixed #7489

jeanpaulgalea · 2022-08-09T09:31:54Z

Hi,

Would you consider adding support for CounterLocation.MiddleFixed in KBKDFHMAC and KBKDFCMAC?

Currently the counter bytes can only be added at the beginning (CounterLocation.BeforeFixed) or end (CounterLocation.AfterFixed) of the input. This patch adds support for CounterLocation.MiddleFixed, which along with blocation=, allows the user to specify a particular byte offset where the counter bytes are to be added to input.

blocation is shorthand for "break location".

The MR is based off an internal patch that we make use of. It breaks backward compatibility due to the introduction of the new blocation= argument. If it's something worth your while, happy to discuss alternative implementations (to remain backward compatible) and help with adding tests and updating documentation.

Thank you

reaperhulk · 2022-08-09T18:00:49Z

I wouldn't be opposed to supporting middle fixed. However, I'd like to look at ways to make the API backwards compatible and we should also expand the tests to use the middle_fixed vectors (they're currently skipped).

jeanpaulgalea · 2022-08-09T18:25:42Z

Cool, thanks.

I did try the following earlier today. It should retain backwards compatibility, but it's a little crude... perhaps there are better ways of doing this in Python?

from enum import Enum
from functools import partial

def f(pos: int):
    return pos


class CounterLocation(Enum):
    BeforeFixed = 0
    AfterFixed = -1
    MiddleFixed = partial(f)

    def __call__(self, pos: int = None):
        return self.value(pos)


def _check(location: CounterLocation):
    if location == CounterLocation.BeforeFixed:
        print("before")
    elif location == CounterLocation.AfterFixed:
        print("after")
    else:
        print(location)


location = CounterLocation.BeforeFixed
_check(location)

location = CounterLocation.AfterFixed
_check(location)

location = CounterLocation.MiddleFixed(12)
_check(location)

reaperhulk · 2022-08-09T21:55:02Z

That would definitely work but it's probably more discoverable to add blocation (is that the name in the KBKDF documentation?) as an optional kwarg to the end. The kwarg approach requires users to either use kwargs-only calling convention or explicitly pass a backend (None being the default value though) before the blocation though.

alex · 2022-08-09T21:57:44Z

Part of me loves the hack to get a rust-style enum with values, but its probably a bit too magic :-)

…

On Tue, Aug 9, 2022 at 5:55 PM Paul Kehrer ***@***.***> wrote: That would definitely work but it's probably more discoverable to add blocation (is that the name in the KBKDF documentation?) as an optional kwarg to the end. The kwarg approach requires users to either use kwargs-only calling convention or explicitly pass a backend (None being the default value though) before the blocation though. — Reply to this email directly, view it on GitHub <#7489 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAAGBEZDRTVXPPRHY37KKDVYLHUBANCNFSM56ACQI3A> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

-- All that is necessary for evil to succeed is for good people to do nothing.

jeanpaulgalea · 2022-08-10T09:10:13Z

Updated to use an optional kwarg at the end: e98c7aa

jeanpaulgalea · 2022-08-10T09:20:13Z

NIST SP 800-108 doesn't specify a name for the counter location (or I missed it). It simply states that the length and order is arbitrary and defined by the protocol where the KDF is used (page 12):

The length for each data field and an order shall be defined unambiguously. For example,
the length and the order may be defined as a part of a KDF specification or by the protocol
where the KDF is used. In each of the following sections, a specific order for the feedback
value, the counter, the Label, the separation indicator 0x00, the Context, and [L]2 is used,
assuming that each of them is represented with a specific length. This Recommendation
specifies several families of KDFs. Alternative orders for the input data fields may be used
for different KDFs.

However, NIST's own test vectors use breakLocation:
https://github.com/usnistgov/ACVP-Server/blob/master/gen-val/json-files/KDF-1.0/internalProjection.json

Perhaps you'd rather use that name? But snake case? i.e. break_location ?

jeanpaulgalea · 2022-08-10T11:31:53Z

Added tests in b013a7b and 9b3652d. Could use your help to verify they're actually running and passing, though.

Also attempted to update documentation in a9cbef4.

alex · 2022-08-10T11:41:46Z

break_location seems reasonable to me.

jeanpaulgalea · 2022-08-10T11:52:49Z

src/cryptography/hazmat/primitives/kdf/kbkdf.py

+                "blocation is ignored when location is not"
+                " CounterLocation.MiddleFixed"
+            )
+


Is it necessary to check that blocation is within 0..len(fixed or label+context) ?

In its present state, it's allowed to set blocation=-1 or =1000. This works fine with fixed[blocation:] and fixed[:blocation] further below, but not sure how pedantic we should be.

Also not sure if you prefer to require location=BeforeFixed instead of allowing location=MiddleFixed && blocation=0, and similarly location=AfterFixed instead of location=MiddleFixed && blocation=len(fixed or label+context).

My personal preference is to do none of these length checks and leave it up to the user, but obviously it's your call.

reaperhulk · 2022-08-11T01:42:18Z

src/cryptography/hazmat/primitives/kdf/kbkdf.py

@@ -51,6 +53,21 @@ def __init__(
        if not isinstance(location, CounterLocation):
            raise TypeError("location must be of type CounterLocation")

+        if break_location is None and location == CounterLocation.MiddleFixed:


Suggested change

if break_location is None and location == CounterLocation.MiddleFixed:

if break_location is None and location is CounterLocation.MiddleFixed:

Thanks, updated.

reaperhulk

I haven't completely reviewed this yet, but we should add a changelog entry as well.

docs/hazmat/primitives/key-derivation-functions.rst

- Test CounterLocation.MiddleFixed and blocation=

jeanpaulgalea · 2022-08-11T06:52:15Z

I haven't completely reviewed this yet, but we should add a changelog entry as well.

Thanks, changelog entry added 🚀

docs/hazmat/primitives/key-derivation-functions.rst

alex · 2022-08-11T12:01:19Z

src/cryptography/hazmat/primitives/kdf/kbkdf.py

+                h.update(fixed)
+                h.update(counter)
+            else:
+                h.update(fixed[: self._break_location])


I think we should validate break location, if it's negative or > len(fixed) dumb things happen.

OK, will do.

Updated. Quite a bit had to change, might be best to review the MR in its entirety again... 😢

alex · 2022-08-11T12:02:27Z

tests/hazmat/primitives/utils.py

+        params["fixedinputdata"] = params.pop("databeforectrdata")
+        params["fixedinputdata"] += params.pop("dataafterctrdata")


Suggested change

params["fixedinputdata"] = params.pop("databeforectrdata")

params["fixedinputdata"] += params.pop("dataafterctrdata")

params["fixedinputdata"] = params.pop("databeforectrdata") + params.pop("dataafterctrdata")

FWIW, I tried this approach, but black reformats it into:

- params["fixedinputdata"] = params.pop("databeforectrdata") - params["fixedinputdata"] += params.pop("dataafterctrdata") + params["fixedinputdata"] = params.pop( + "databeforectrdata" + ) + params.pop("dataafterctrdata")

Which I found a little less clear.

Would you still prefer this approach?

Yeah, I think I still prefer that, even if it's a bit stupid :-)

src/cryptography/hazmat/primitives/kdf/kbkdf.py

jeanpaulgalea · 2022-08-11T13:25:23Z

src/cryptography/hazmat/primitives/kdf/kbkdf.py

@@ -114,17 +135,27 @@ def derive(self, key_material: bytes, prf_output_size: int) -> bytes:
        if rounds > pow(2, len(r_bin) * 8) - 1:
            raise ValueError("There are too many iterations.")

+        fixed = self._generate_fixed_input()


This should be fine to move outside the loop, the fixed input doesn't change across iterations, except for the counter value.

- Update tests to assert exception is raised when break_location < 0 or > len(fixed) - When asserting for "break_location is ignored when MiddleFixed", use break_location=0 instead of =10, to ensure we don't raise because of break_location > len(fixed) - Assert that the right error messages are returned to the user.

jeanpaulgalea · 2022-08-11T14:37:05Z

Should be ready for another round of review 🙏

Will refrain from pushing any further changes, unless specifically requested.

Cheers

reaperhulk · 2022-08-14T16:17:27Z

This looks basically ready, but if we could make break_location a kwarg-only argument that would make life simpler in the distant future when we look to remove the backend arg. I don't recall when kwarg only syntax became available, but I suppose CI can test it for us 😂. Would you mind giving that a shot (it should just be backend=None, *, break_location=None in the type definition + the docs of course)?

jeanpaulgalea · 2022-08-15T07:52:16Z

Thanks for the feedback!

TIL about kwarg-only arguments and it's been in Python since 3.0 apparently! Initially I thought you meant to simply move break_location to the end of the argument list.

Anyway, PR updated 🚀

jeanpaulgalea · 2022-08-15T12:39:03Z

Thanks @alex and @reaperhulk!

Wish more upstreams were this easy to work with 😄

alex · 2022-08-15T12:49:09Z

Thanks for your work and being patient with all the revisions we asked for!

…

On Mon, Aug 15, 2022 at 8:39 AM Jean Paul Galea ***@***.***> wrote: Thanks @alex <https://github.com/alex> and @reaperhulk <https://github.com/reaperhulk>! Wish more upstreams were this easy to work with 😄 — Reply to this email directly, view it on GitHub <#7489 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAAGBBSACBIIVW2XZNB3C3VZI27DANCNFSM56ACQI3A> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- All that is necessary for evil to succeed is for good people to do nothing.

jeanpaulgalea commented Aug 10, 2022

View reviewed changes

jeanpaulgalea changed the title ~~WIP: KBKDF: add CounterLocation.MiddleFixed~~ KBKDF: add CounterLocation.MiddleFixed Aug 10, 2022

reaperhulk reviewed Aug 11, 2022

View reviewed changes

docs/hazmat/primitives/key-derivation-functions.rst Show resolved Hide resolved

jeanpaulgalea added 2 commits August 11, 2022 08:28

KBKDF: add CounterLocation.MiddleFixed

4288ba4

tests_kbkdf: add new tests

726a9ac

- Test CounterLocation.MiddleFixed and blocation=

alex reviewed Aug 11, 2022

View reviewed changes

jeanpaulgalea added 5 commits August 11, 2022 14:26

test_kbkdf_vectors: make use of MIDDLE_FIXED

c49a9da

docs: document KBKDF CounterLocation.MiddleFixed

8f0f8f9

s/blocation/break_location/g

94241e4

CHANGELOG: document KBKDF{HMAC,CMAC} CounterLocation.MiddleFixed #7489

bef83c8

kbkdf: fixed is static across n iterations, only the counter changes

fb3ff34

jeanpaulgalea commented Aug 11, 2022

View reviewed changes

src/cryptography/hazmat/primitives/kdf/kbkdf.py Show resolved Hide resolved

jeanpaulgalea commented Aug 11, 2022

View reviewed changes

src/cryptography/hazmat/primitives/kdf/kbkdf.py Show resolved Hide resolved

jeanpaulgalea commented Aug 11, 2022

View reviewed changes

jeanpaulgalea added 2 commits August 15, 2022 09:33

kbkdf: keyword-only break_location

ba4ea8c

docs: document kw-only KBKDF{HMAC,CMAC}

8b0737f

alex approved these changes Aug 15, 2022

View reviewed changes

alex merged commit da1a30b into pyca:main Aug 15, 2022

jeanpaulgalea deleted the jeanpaul/blocation branch August 15, 2022 12:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KBKDF: add CounterLocation.MiddleFixed #7489

KBKDF: add CounterLocation.MiddleFixed #7489

jeanpaulgalea commented Aug 9, 2022

reaperhulk commented Aug 9, 2022

jeanpaulgalea commented Aug 9, 2022

reaperhulk commented Aug 9, 2022

alex commented Aug 9, 2022 via email

jeanpaulgalea commented Aug 10, 2022

jeanpaulgalea commented Aug 10, 2022

jeanpaulgalea commented Aug 10, 2022

alex commented Aug 10, 2022

jeanpaulgalea Aug 10, 2022

reaperhulk Aug 11, 2022

jeanpaulgalea Aug 11, 2022

reaperhulk left a comment

jeanpaulgalea commented Aug 11, 2022

alex Aug 11, 2022

jeanpaulgalea Aug 11, 2022

jeanpaulgalea Aug 11, 2022

alex Aug 11, 2022

jeanpaulgalea Aug 11, 2022

alex Aug 11, 2022

jeanpaulgalea Aug 11, 2022

jeanpaulgalea Aug 11, 2022

jeanpaulgalea commented Aug 11, 2022

reaperhulk commented Aug 14, 2022

jeanpaulgalea commented Aug 15, 2022

jeanpaulgalea commented Aug 15, 2022

alex commented Aug 15, 2022 via email

	if break_location is None and location == CounterLocation.MiddleFixed:
	if break_location is None and location is CounterLocation.MiddleFixed:

		params["fixedinputdata"] = params.pop("databeforectrdata")
		params["fixedinputdata"] += params.pop("dataafterctrdata")

KBKDF: add CounterLocation.MiddleFixed #7489

KBKDF: add CounterLocation.MiddleFixed #7489

Conversation

jeanpaulgalea commented Aug 9, 2022

reaperhulk commented Aug 9, 2022

jeanpaulgalea commented Aug 9, 2022

reaperhulk commented Aug 9, 2022

alex commented Aug 9, 2022 via email

jeanpaulgalea commented Aug 10, 2022

jeanpaulgalea commented Aug 10, 2022

jeanpaulgalea commented Aug 10, 2022

alex commented Aug 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reaperhulk left a comment

Choose a reason for hiding this comment

jeanpaulgalea commented Aug 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeanpaulgalea commented Aug 11, 2022

reaperhulk commented Aug 14, 2022

jeanpaulgalea commented Aug 15, 2022

jeanpaulgalea commented Aug 15, 2022

alex commented Aug 15, 2022 via email