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

[CBOR] Implement a proof of concept for ECDsa COSE key serialization #36002

Merged
merged 9 commits into from
May 13, 2020

Conversation

eiriktsarpalis
Copy link
Member

Implements a proof-of-concept serializer for ECDsa COSE key serialization, as defined in https://www.w3.org/TR/webauthn/#sctn-encoded-credPubKey-examples.

@eiriktsarpalis eiriktsarpalis added this to the 5.0 milestone May 7, 2020
@eiriktsarpalis eiriktsarpalis requested a review from bartonjs May 7, 2020 19:42
@ghost
Copy link

ghost commented May 7, 2020

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq
Notify danmosemsft if you want to be subscribed.

Copy link
Member

@bartonjs bartonjs left a comment

Choose a reason for hiding this comment

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

Apparently my Approve (with feedback) from a couple days ago didn't post.

This box said something like "LGTM, but a few comments to make it more sample-friendly".

throw new FormatException("Unexpected number of elements in the CBOR cose key.");
}

// CTAP2 guarantees order of fields
Copy link
Member

Choose a reason for hiding this comment

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

I was curious to see if you were going to take advantage of this in the reader.

For a public method I'd expect the conformance mode check to be a runtime check => ArgumentException (not just an assert). For internal an assert would make sense, since the only callers should have either validated it themselves or were the ones who created the reader.

To make this Proof of Concept more sample-friendly, I'd switch the access modifier to internal.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was curious to see if you were going to take advantage of this in the reader.

If you mean the field ordering guarantee, I think it's mostly effective in scenaria where you don't support optional fields. In the general case something like this might be preferable.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, to be clear, I approve of taking the dependency; it's part of the niceness of the conformance mode 😄.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I suppose my point is that even assuming CTAP2 conformance mode, you might still need to opt for something like the original approach if you needed to tolerate arbitrary fields (e.g. because of forward compatibility concerns).

Copy link
Member

@krwq krwq left a comment

Choose a reason for hiding this comment

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

LGTM after fixing comments

@eiriktsarpalis eiriktsarpalis merged commit 9386d50 into dotnet:master May 13, 2020
@eiriktsarpalis eiriktsarpalis deleted the feature/cbor branch May 13, 2020 19:16
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants