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.

Sign up for GitHub

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

Jump to bottom

[WebAuthn] CBOR encoded extensions not passed along during assertions #2557

Merged
merged 1 commit into from Jul 20, 2022
Merged

[WebAuthn] CBOR encoded extensions not passed along during assertions #2557

merged 1 commit into from Jul 20, 2022

Conversation

pascoej
Copy link
Member

@pascoej pascoej commented Jul 19, 2022 •
edited by webkit-commit-queue

c93cca1

[WebAuthn] CBOR encoded extensions not passed along during assertions
https://bugs.webkit.org/show_bug.cgi?id=242913
rdar://96912101

Reviewed by Chris Dumez.

* Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:
(+[_WKWebAuthenticationPanel convertToCoreRequestOptionsWithOptions:]):
* Source/WTF/wtf/cocoa/SpanCocoa.h:
(WTF::asUInt8Span):
* Source/WebCore/Modules/webauthn/AuthenticationExtensionsClientInputs.cpp:
(WebCore::AuthenticationExtensionsClientInputs::fromCBOR):
* Source/WebCore/Modules/webauthn/AuthenticationExtensionsClientInputs.h:
* Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:
(+[_WKWebAuthenticationPanel convertToCoreCreationOptionsWithOptions:]):
(+[_WKWebAuthenticationPanel convertToCoreRequestOptionsWithOptions:]):
Pass along CBOR encoded extension to ASC, use span to avoid copy. Rest of callsites to be fixed in
https://bugs.webkit.org/show_bug.cgi?id=242919.

Canonical link: https://commits.webkit.org/252626@main

@pascoej pascoej requested a review from cdumez as a code owner July 19, 2022 19:54
@pascoej pascoej self-assigned this Jul 19, 2022
@pascoej pascoej added WebKit Misc. For miscellaneous bugs in the WebKit framework (and not JavaScriptCore or WebCore). WebKit Nightly Build labels Jul 19, 2022
cdumez
cdumez reviewed Jul 19, 2022
View reviewed changes
Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm Outdated
@@ -1017,7 +1017,10 @@ - (void)makeCredentialWithClientDataHash:(NSData *)clientDataHash options:(_WKPu
result.userVerificationString = toString(userVerification(options.userVerification));
if (auto attachment = authenticatorAttachment(options.authenticatorAttachment))
result.authenticatorAttachmentString = toString(*attachment);
result.extensions = authenticationExtensionsClientInputs(options.extensions);
if (options.extensionsCBOR)
result.extensions = WebCore::AuthenticationExtensionsClientInputs::fromCBOR(vectorFromNSData(options.extensionsCBOR));
Copy link
Contributor

@cdumez cdumez Jul 19, 2022 •
edited

Choose a reason for hiding this comment

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

It is a bit unfortunate to copy the bytes from a NSData to a Vector, just to decode it.

Ideally, AuthenticationExtensionsClientInputs::fromCBOR() would take in a Span<const uint8_t> so we could pass in data without copying it. It is cheap to construct a Span from a Vector or a NSData because it doesn't involve copying.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ooh, that's really good to know. Updating CBORReader with this.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should check (as a follow-up patch) if there are other cases where we lack this optimization.

Copy link
Member Author

Choose a reason for hiding this comment

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

https://bugs.webkit.org/show_bug.cgi?id=242919

@pascoej pascoej force-pushed the eng/WebAuthn-CBOR-encoded-extensions-not-passed-along-during-assertions branch from bf66a2f to f0292e0 Compare July 19, 2022 22:35
@pascoej pascoej requested a review from cdumez July 19, 2022 22:36
cdumez
cdumez reviewed Jul 19, 2022
View reviewed changes
Source/WTF/wtf/cocoa/SpanCocoa.h Outdated
@@ -41,6 +41,12 @@ inline Span<const std::byte> asBytes(const RetainPtr<NSData>& data)
return asBytes(data.get());
}

inline Span<const uint8_t> spanFromNSData(NSData* data)
Copy link
Contributor

@cdumez cdumez Jul 19, 2022 •
edited

Choose a reason for hiding this comment

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

Any chance we could use asBytes() from SpanCocoa.h?

Copy link
Member Author

@pascoej pascoej Jul 19, 2022 •
edited

Choose a reason for hiding this comment

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

I tried this, converting everything to use std::byte instead of uint8_t turns out to be pretty involved as Span<const uint8_t> and Span<const std::byte> don't seem to be easily interchangeable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, maybe we can name it asUInt8Span() to be a little more consistent with asBytes().

@pascoej pascoej force-pushed the eng/WebAuthn-CBOR-encoded-extensions-not-passed-along-during-assertions branch from f0292e0 to ceb5066 Compare July 19, 2022 23:11
@pascoej pascoej requested a review from cdumez July 19, 2022 23:12
cdumez
cdumez approved these changes Jul 19, 2022
View reviewed changes
Copy link
Contributor

@cdumez cdumez left a comment

Choose a reason for hiding this comment

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

Looks good.

@pascoej
Copy link
Member Author

pascoej commented Jul 19, 2022

Thanks for the review and patience.

@pascoej pascoej added the merge-queue Applied to send a pull request to merge-queue label Jul 19, 2022
@pascoej
[WebAuthn] CBOR encoded extensions not passed along during assertions
c93cca1 
https://bugs.webkit.org/show_bug.cgi?id=242913
rdar://96912101

Reviewed by Chris Dumez.

* Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:
(+[_WKWebAuthenticationPanel convertToCoreRequestOptionsWithOptions:]):
* Source/WTF/wtf/cocoa/SpanCocoa.h:
(WTF::asUInt8Span):
* Source/WebCore/Modules/webauthn/AuthenticationExtensionsClientInputs.cpp:
(WebCore::AuthenticationExtensionsClientInputs::fromCBOR):
* Source/WebCore/Modules/webauthn/AuthenticationExtensionsClientInputs.h:
* Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:
(+[_WKWebAuthenticationPanel convertToCoreCreationOptionsWithOptions:]):
(+[_WKWebAuthenticationPanel convertToCoreRequestOptionsWithOptions:]):
Pass along CBOR encoded extension to ASC, use span to avoid copy. Rest of callsites to be fixed in
https://bugs.webkit.org/show_bug.cgi?id=242919.

Canonical link: https://commits.webkit.org/252626@main
@webkit-early-warning-system webkit-early-warning-system force-pushed the eng/WebAuthn-CBOR-encoded-extensions-not-passed-along-during-assertions branch from ceb5066 to c93cca1 Compare July 20, 2022 00:50
@webkit-commit-queue
Copy link
Collaborator

Committed 252626@main (c93cca1): https://commits.webkit.org/252626@main

Reviewed commits have been landed. Closing PR #2557 and removing active labels.

@webkit-early-warning-system webkit-early-warning-system merged commit c93cca1 into WebKit:main Jul 20, 2022
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jul 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cdumez cdumez cdumez approved these changes

@brentfulgham brentfulgham Awaiting requested review from brentfulgham

Assignees

@pascoej pascoej

Labels
WebKit Misc. For miscellaneous bugs in the WebKit framework (and not JavaScriptCore or WebCore).
Projects
None yet
Milestone
No milestone
5 participants
@pascoej @webkit-commit-queue @cdumez @brentfulgham @webkit-early-warning-system