Client pin features #127

kaczmarczyck · 2020-07-02T18:13:31Z

This PR adds new subcommands from CTAP 2.1 in clientPin.

CTAP 2.1 introduces an interface for PIN protocol versions, since a v2 is currently being drafted. I decided to move all clientPin code for the existing PIN protocol v1 to a separate module. For better code sharing with a potential v2, we might introduce the common interface later when needed. I decided to not proactively refactor v1 into that interface, and wait until it is stable.

This has the added benefit of moving some complexity out of ctap/mod.rs which will have to accommodate more commands as they are introduced.

We will need to do more testing of changes for 2.1 in the wild when platforms and relying parties support it.

This PR contains some unused functions, this is intentional. They will later be used by the "Minimum PIN Length Extension" to allow some relying parties to see the minimum PIN length required by this authenticator, and include it in its attestation during MakeCredential. This feature establishes trust with relying parties with strict PIN policies.

Other open TODOs are (1) enforcing PIN length in code points and (2) places where the specification is ambiguous and discussed in the FIDO Alliance (you might lack permissions).

Tests pass
Appropriate changes to README are included in PR

src/ctap/command.rs

src/ctap/data_formats.rs

src/ctap/pin_protocol_v1.rs

src/ctap/storage.rs

src/ctap/data_formats.rs

src/ctap/storage.rs

src/ctap/response.rs

src/ctap/storage.rs

src/ctap/pin_protocol_v1.rs

kaczmarczyck · 2020-07-08T09:24:25Z

This is a CTAP2.1 feature, tracked in #106 .

ia0

LGTM, letting others approve.

src/ctap/pin_protocol_v1.rs

Cargo.toml

…lues

README.md

gendx

I took a closer look to the pin_protocol_v1. I didn't look into all the details of the protocol, but I have a few nits regarding Rust code. Other than that looks good!

src/ctap/pin_protocol_v1.rs

kaczmarczyck · 2020-07-27T20:10:44Z

Thanks @gendx for the review and the Rust comments.

Concerning reviewing for details of the protocol, the old comments are thoroughly tested and mostly taken from mod.rs, so those should be good. The new commands, on the other hand, or not even in the publicly visible specification, so we don't really have any real life applications for testing. They are hidden behind the ctap2_1 feature flag for now for that reason.

src/ctap/pin_protocol_v1.rs

gendx

Just a few remaining nits, but otherwise it looks good :)

src/ctap/pin_protocol_v1.rs

src/ctap/data_formats.rs

gendx

Apart from a few nits that had unfortunately slipped through, and rebasing/merging with master, this looks good to me.

src/ctap/pin_protocol_v1.rs

kaczmarczyck added 6 commits July 2, 2020 19:14

new error codes and enum cleanups

a4becf9

new client pin subcommands

63aef3b

refactors the client PIN implementation into a new module

9ff988d

adding tests to pin_protocol_v1

033f544

adds new client Pin subcommand minPinLength implementation

26595db

adds permissions and adapts clientPin 2.1 subcommands

216a6a0

kaczmarczyck requested review from ia0 and gendx July 2, 2020 18:13

googlebot added the cla: yes label Jul 2, 2020

ia0 reviewed Jul 3, 2020

View reviewed changes

adds clarifications, improvements and tests

3b66155

ia0 reviewed Jul 8, 2020

View reviewed changes

gendx reviewed Jul 8, 2020

View reviewed changes

src/ctap/storage.rs Outdated Show resolved Hide resolved

gendx reviewed Jul 8, 2020

View reviewed changes

src/ctap/storage.rs Show resolved Hide resolved

gendx reviewed Jul 8, 2020

View reviewed changes

src/ctap/pin_protocol_v1.rs Outdated Show resolved Hide resolved

gendx reviewed Jul 8, 2020

View reviewed changes

src/ctap/pin_protocol_v1.rs Outdated Show resolved Hide resolved

gendx reviewed Jul 8, 2020

View reviewed changes

src/ctap/pin_protocol_v1.rs Outdated Show resolved Hide resolved

kaczmarczyck mentioned this pull request Jul 8, 2020

#cfg in macros #128

Open

adds code style improvements, including a new enum for permissions

04278d9

ia0 reviewed Jul 8, 2020

View reviewed changes

src/ctap/pin_protocol_v1.rs Outdated Show resolved Hide resolved

src/ctap/pin_protocol_v1.rs Outdated Show resolved Hide resolved

kaczmarczyck added 2 commits July 8, 2020 17:59

use the enum-iterator crate for better testing of enums

131f876

improved documentation for the PinPermission enum

25b6756

kaczmarczyck requested a review from gendx July 9, 2020 07:09

ia0 previously approved these changes Jul 9, 2020

View reviewed changes

ia0 reviewed Jul 9, 2020

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

moves enum-iterator dependency to dev and updates binary reference va…

950d90f

…lues

kaczmarczyck dismissed ia0’s stale review via 950d90f July 9, 2020 09:26

ia0 previously approved these changes Jul 9, 2020

View reviewed changes

updates reproducible binary hashes and sizes

cc0e2bb

kaczmarczyck requested a review from gendx July 10, 2020 08:08

gendx reviewed Jul 20, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

gendx requested changes Jul 20, 2020

View reviewed changes

improves documentation to address comments

a398c40

kaczmarczyck requested a review from gendx July 27, 2020 20:19

Merge branch 'master' into client-pin-features

4e4ed12

gendx requested changes Jul 30, 2020

View reviewed changes

improved code consistency and documentation

d5fefa2

kaczmarczyck requested a review from gendx August 6, 2020 06:46

gendx reviewed Aug 7, 2020

View reviewed changes

src/ctap/pin_protocol_v1.rs Outdated Show resolved Hide resolved

gendx reviewed Aug 7, 2020

View reviewed changes

src/ctap/pin_protocol_v1.rs Outdated Show resolved Hide resolved

src/ctap/pin_protocol_v1.rs Outdated Show resolved Hide resolved

src/ctap/pin_protocol_v1.rs Show resolved Hide resolved

gendx reviewed Aug 7, 2020

View reviewed changes

src/ctap/pin_protocol_v1.rs Outdated Show resolved Hide resolved

improved testing in pin_protocol_v1.rs

0aabf82

kaczmarczyck requested a review from gendx August 13, 2020 03:40

gendx reviewed Aug 17, 2020

View reviewed changes

src/ctap/pin_protocol_v1.rs Show resolved Hide resolved

src/ctap/pin_protocol_v1.rs Outdated Show resolved Hide resolved

src/ctap/pin_protocol_v1.rs Show resolved Hide resolved

src/ctap/pin_protocol_v1.rs Show resolved Hide resolved

src/ctap/data_formats.rs Outdated Show resolved Hide resolved

unifying the use instructions to another standard

bbcff48

kaczmarczyck requested a review from gendx August 17, 2020 15:38

gendx reviewed Aug 19, 2020

View reviewed changes

src/ctap/pin_protocol_v1.rs Outdated Show resolved Hide resolved

src/ctap/pin_protocol_v1.rs Show resolved Hide resolved

src/ctap/pin_protocol_v1.rs Outdated Show resolved Hide resolved

kaczmarczyck and others added 2 commits August 19, 2020 19:24

improved documentation

77b21e9

Merge branch 'master' into client-pin-features

fe57be2

gendx reviewed Aug 20, 2020

View reviewed changes

src/ctap/pin_protocol_v1.rs Outdated Show resolved Hide resolved

makes tests more readable

9259102

gendx previously approved these changes Aug 20, 2020

View reviewed changes

updates reproducible references

6902115

kaczmarczyck dismissed gendx’s stale review via 6902115 August 20, 2020 16:02

kaczmarczyck requested a review from gendx August 20, 2020 16:13

gendx approved these changes Aug 20, 2020

View reviewed changes

kaczmarczyck merged commit 74c773d into google:master Aug 20, 2020

kaczmarczyck deleted the client-pin-features branch August 20, 2020 18:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Client pin features #127

Client pin features #127

kaczmarczyck commented Jul 2, 2020

kaczmarczyck commented Jul 8, 2020

ia0 left a comment

gendx left a comment

kaczmarczyck commented Jul 27, 2020

gendx left a comment

gendx left a comment

Client pin features #127

Client pin features #127

Conversation

kaczmarczyck commented Jul 2, 2020

kaczmarczyck commented Jul 8, 2020

ia0 left a comment

Choose a reason for hiding this comment

gendx left a comment

Choose a reason for hiding this comment

kaczmarczyck commented Jul 27, 2020

gendx left a comment

Choose a reason for hiding this comment

gendx left a comment

Choose a reason for hiding this comment