Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Mandatory map fields not enforced? #84

Closed
yaronf opened this issue Jan 17, 2021 · 9 comments · Fixed by #86
Closed

Mandatory map fields not enforced? #84

yaronf opened this issue Jan 17, 2021 · 9 comments · Fixed by #86
Assignees
Labels
bug Something isn't working cbor validation
Milestone

Comments

@yaronf
Copy link

yaronf commented Jan 17, 2021

I have this CBOR (diag notation):

{1: 65535, 2: h'1122334455', 3: 6, }

It validates successfully with this CDDL:

var_header = {
        K_KEY_PROVIDER: uint,
        K_KEY_ID: bstr,
        ? K_KEY_VERSION: uint,
        ? K_AUX_DATA: bstr,
        ? K_NONCE : bstr,
        ? K_AUTH_TAG : bstr,
        ? K_AAD : bstr,
        *uint => any ; extensions
}

K_RESERVED = 0
K_KEY_PROVIDER = 1
K_KEY_ID = 2
K_KEY_VERSION = 3
K_AUX_DATA = 4
K_NONCE = 5
K_AUTH_TAG = 6
K_AAD = 7
        ; extend here

According to @cabo my CDDL is incorrect because it translates to textual, rather than integer, map keys, e.g. "K_NONCE" and not 5. My file is accepted because of the "extensions" line. However it should have failed validation anyway, because the first two fields (K_KEY_PROVIDER and K_KEY_ID) are mandatory in the schema, but missing from the CBOR file.

@yaronf
Copy link
Author

yaronf commented Jan 17, 2021

Closing: not a bug. I corrected the CDDL to:

var_header = {
	K_KEY_PROVIDER => uint,
	K_KEY_ID => bstr,
	? K_KEY_VERSION => uint,
	? K_AUX_DATA => bstr,
	? K_NONCE  => bstr,
	? K_AUTH_TAG  => bstr,
	? K_AAD  => bstr,
	*uint => any ; extensions
}

K_RESERVED = 0
K_KEY_PROVIDER = 1
K_KEY_ID = 2
K_KEY_VERSION = 3
K_AUX_DATA = 4
K_NONCE = 5
K_AUTH_TAG = 6
K_AAD = 7
	; extend here

And everything works fine, including detection of mandatory fields.

@yaronf yaronf closed this as completed Jan 17, 2021
@cabo
Copy link

cabo commented Jan 17, 2021

The initial instance still should not validate with the initial CDDL, as the mandatory members are not present:

    K_KEY_PROVIDER: uint,
    K_KEY_ID: bstr,

(The rest of the behavior is fine, as Yaron mentioned.)

@cabo
Copy link

cabo commented Jan 17, 2021

(hmm, can't reopen the issue.)

@anweiss anweiss reopened this Jan 25, 2021
@anweiss
Copy link
Owner

anweiss commented Jan 25, 2021

Per #84 (comment), this is indeed still a bug. Will fix.

@anweiss anweiss self-assigned this Jan 25, 2021
@anweiss anweiss added bug Something isn't working validation cbor labels Jan 25, 2021
@anweiss anweiss added this to the v1.0.0 milestone Jan 26, 2021
@yaronf
Copy link
Author

yaronf commented Jan 27, 2021

Sorry, you didn't include any test cases in the PR, and I'm unable to generate any cases where the behavior is changed for the CBOR above and various variants. Specifically, the mandatory members are actually present in the CBOR at the top of this issue. Could you please include an example?

@anweiss anweiss reopened this Jan 27, 2021
@anweiss
Copy link
Owner

anweiss commented Jan 27, 2021

@yaronf my apologies. Merging the PR auto closed the issue. This will be introduced into v0.8.7 of the CLI. Will update the docs and release notes.

var_header = {
        K_KEY_PROVIDER: uint,
        K_KEY_ID: bstr,
        ? K_KEY_VERSION: uint,
        ? K_AUX_DATA: bstr,
        ? K_NONCE : bstr,
        ? K_AUTH_TAG : bstr,
        ? K_AAD : bstr,
        *uint => any ; extensions
}

K_RESERVED = 0
K_KEY_PROVIDER = 1
K_KEY_ID = 2
K_KEY_VERSION = 3
K_AUX_DATA = 4
K_NONCE = 5
K_AUTH_TAG = 6
K_AAD = 7

should now validate the following:

{"K_KEY_PROVIDER": 65535, "K_KEY_ID": h'1122334455', "K_KEY_VERSION": 6, }

and this will now be denoted as invalid:

{1: 65535, 2: h'1122334455', 3: 6, }

The colon shortcut notation validates the map keys based on their text representation. When used with the double arrow notation, the keys are resolved to the type rules. Prior to merging #86, the CBOR validation implementation in the tool was not properly interpreting the colon shortcut notation.

@yaronf
Copy link
Author

yaronf commented Jan 27, 2021

Yes, thank you. Agree. Though I had to read the RFC and two of its appendices quite thoroughly to get myself convinced. (Also IMHO this "syntactic sugar" is more pain than it's worth.)

@anweiss
Copy link
Owner

anweiss commented Jan 27, 2021

lol, yea I can't tell you how many times I've read through the RFC since I started this project a few years ago and I STILL misinterpret something.

@cabo
Copy link

cabo commented Jan 27, 2021 via email

Repository owner locked and limited conversation to collaborators Oct 6, 2021
@anweiss anweiss closed this as completed Oct 6, 2021

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
bug Something isn't working cbor validation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants