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

Propose text to close #63 #171

Merged
merged 5 commits into from Mar 6, 2020
Merged

Propose text to close #63 #171

merged 5 commits into from Mar 6, 2020

Conversation

cabo
Copy link
Collaborator

@cabo cabo commented Feb 26, 2020

This text tries to summarize the mailing list discussion, stating facts about what approaches can be used under which circumstances.

duplicate keys. This requires the application to handle (check
against) duplicate keys, even if the application rules are identical
to the generic data model rules.
* lose some entries with duplicate keys, e.g. by only delivering the
Copy link
Contributor

@laurencelundblade laurencelundblade Feb 26, 2020

Choose a reason for hiding this comment

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

Clearly it is right for generic decoders to error out on the first duplicate.

Is it OK for a generic decoder to silently ignore duplicates? If this is considered OK designers and implementors will rely on this behavior. I think it is not OK because we probably can't write a rule about which ones (first, last, middle, random) will be ignored and have it widely and reliably implemented. So I think this third option should be removed.

they do need to validate key uniqueness. These generic decoders can
only be used in situations where the encoder can be relied upon to
always provide valid maps; this is not possible if the encoder can
be under control of an attacker.
Copy link
Contributor

@laurencelundblade laurencelundblade Feb 26, 2020

Choose a reason for hiding this comment

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

The encoder should always be considered under control of the attacker. There should be no discussion of a situation where an encoder can be relied upon to provide valid maps.

duplicate keys. This requires the application to handle (check
against) duplicate keys, even if the application rules are identical
to the generic data model rules.
* lose some entries with duplicate keys, e.g. by only delivering the
Copy link
Contributor

@laurencelundblade laurencelundblade Feb 26, 2020

Choose a reason for hiding this comment

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

Do we really want to exclude decoders that can neither detect duplicates nor pass duplicates on? That will force encoder implementors in some environments to not use the common map / list constructs in those environments.

mcr
mcr approved these changes Feb 26, 2020
@x448
Copy link
Contributor

@x448 x448 commented Feb 26, 2020

Just an FYI, fxamacker/cbor provides a generic decoder with two choices (so far) for duplicate map keys. I'm hoping changes to 7049bis doesn't turn these options into non-compliance, the DupMapKeyEnforcedAPF feature is already being used by oasislabs/oasis-core.

fxamacker/cbor provides options for fast detection and rejection of duplicate map keys based on applying a Go-specific data model to CBOR's extended generic data model in order to determine duplicate vs distinct map keys. Detection relies on whether the CBOR map key would be a duplicate "key" when decoded and applied to the user-provided Go map or struct.

DupMapKeyQuiet turns off detection of duplicate map keys. It tries to use a "keep fastest" method by choosing either "keep first" or "keep last" depending on the Go data type.

DupMapKeyEnforcedAPF enforces detection and rejection of duplidate map keys. Decoding stops immediately and returns DupMapKeyError when the first duplicate key is detected. The error includes the duplicate map key and the index number.

APF suffix means "Allow Partial Fill" so the destination map or struct can contain some decoded values at the time of error. It is the caller's responsibility to respond to the DupMapKeyError by discarding the partially filled result if that's required by their protocol.

@laurencelundblade
Copy link
Contributor

@laurencelundblade laurencelundblade commented Feb 27, 2020

@x448 You are in compliance with the most recent text. DupMapKeyEnforcedAPF corresponds to the first bullet and DupMapKeyQuiet to the last. The text implies that use of the last option should only be in attacker-free environments.

@x448
Copy link
Contributor

@x448 x448 commented Feb 27, 2020

@laurencelundblade Thanks! I appreciate you taking time to let me know. BTW, we thanked you in the release notes for fxamacker/cbor v2.1.0 (Feb 17) -- it added CBOR tags support resulting in lots of questions (e.g. time tags) that week.

paulehoffman and others added 2 commits Mar 6, 2020
Co-Authored-By: Laurence Lundblade <laurencelundblade@users.noreply.github.com>
@paulehoffman paulehoffman merged commit d696036 into master Mar 6, 2020
0 of 2 checks passed
cabo
Copy link
Collaborator

@cabo cabo commented on 3b8699c Mar 7, 2020

Choose a reason for hiding this comment

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

I actually would prefer "or" over "and".

@cabo cabo deleted the map-keys-63 branch Mar 28, 2020
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.

None yet

5 participants