KBKDF: add CounterLocation.MiddleFixed

[jeanpaulgalea]

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]

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]

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]

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]

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 https://github.com/pyca/cryptography/pull/7489#issuecomment-1209931932, 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]

Updated to use an optional kwarg at the end: https://github.com/pyca/cryptography/pull/7489/commits/e98c7aaf66638d963ce80ed8c72d2b1316489856

[jeanpaulgalea]

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]

Added tests in https://github.com/pyca/cryptography/pull/7489/commits/b013a7b6a5ef2d7acf67d1fb88e4416e34cd5b3c and https://github.com/pyca/cryptography/pull/7489/commits/9b3652da6a3e5a3cdc9128288696e89a22d96f6a. Could use your help to verify they're actually running and passing, though.

Also attempted to update documentation in https://github.com/pyca/cryptography/pull/7489/commits/a9cbef48c5079e1979581e60f6661cb7024aeddb.

[alex]

break_location seems reasonable to me.

[jeanpaulgalea]

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

Thanks, changelog entry added :rocket:

[jeanpaulgalea]

Should be ready for another round of review :pray:

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

Cheers

[reaperhulk]

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]

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 :rocket:

[jeanpaulgalea]

Thanks @alex and @reaperhulk!

Wish more upstreams were this easy to work with :smile:

[alex]

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 https://github.com/pyca/cryptography/pull/7489#issuecomment-1214964844, 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.