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

Support for passkey registration #2885

Merged
merged 22 commits into from
Oct 30, 2023
Merged

Support for passkey registration #2885

merged 22 commits into from
Oct 30, 2023

Conversation

kspearrin
Copy link
Member

@kspearrin kspearrin commented Apr 26, 2023 •
edited by trmartin4
Loading

Type of change

- [ ] Bug fix
- [ ] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

This PR introduces the server-side code for registration of passkeys for use in logging in to Bitwarden clients. The introduction of the feature is hidden behind the passwordless-login feature flag.

The corresponding client PR can be found here: bitwarden/clients#5396.

📓 This foundational code will be built upon with ongoing work that the Auth team is doing.

Code changes

  • file.ext: Description of what was changed and why

Before you submit

  • Please check for formatting errors (dotnet format --verify-no-changes) (required)
  • If making database changes - make sure you also update Entity Framework queries and/or migrations
  • Please add unit tests where it makes sense to do so (encouraged but not required)
  • If this change requires a documentation update - notify the documentation team
  • If this change has particular deployment requirements - notify the DevOps team

kspearrin and others added 15 commits April 26, 2023 11:12
@kspearrin
support for fido2 auth
d91e22e
@kspearrin
stub out registration implementations
9dc24de
@kspearrin
stub out assertion steps and token issuance
f33570e
@kspearrin
verify token
bd01206
@kspearrin
webauthn tokenable
0abba03
@kspearrin
remove duplicate expiration set
6d4aeff
@kspearrin
Merge branch 'master' into fido2
d63588a
@kspearrin
revert sqlproj changes
f2c6b03
@kspearrin
update sqlproj target framework
26da1b2
@kspearrin
update new validator signature
ec0c93f
@kspearrin
Merge branch 'master' into fido2
94073fa
@kspearrin
Merge branch 'master' into fido2
07683f1
@coroiu
[PM-2014] Passkey registration (#2915)
cd5635f 
* [PM-2014] chore: rename `IWebAuthnRespository` to `IWebAuthnCredentialRepository`

* [PM-2014] fix: add missing service registration

* [PM-2014] feat: add user verification when fetching options

* [PM-2014] feat: create migration script for mssql

* [PM-2014] chore: append to todo comment

* [PM-2014] feat: add support for creation token

* [PM-2014] feat: implement credential saving

* [PM-2014] chore: add resident key TODO comment

* [PM-2014] feat: implement passkey listing

* [PM-2014] feat: implement deletion without user verification

* [PM-2014] feat: add user verification to delete

* [PM-2014] feat: implement passkey limit

* [PM-2014] chore: clean up todo comments

* [PM-2014] fix: add missing sql scripts

Missed staging them when commiting

* [PM-2014] feat: include options response model in swagger docs

* [PM-2014] chore: move properties after ctor

* [PM-2014] feat: use `Guid` directly as input paramter

* [PM-2014] feat: use nullable guid in token

* [PM-2014] chore: add new-line

* [PM-2014] feat: add support for feature flag

* [PM-2014] feat: start adding controller tests

* [PM-2014] feat: add user verification test

* [PM-2014] feat: add controller tests for token interaction

* [PM-2014] feat: add tokenable tests

* [PM-2014] chore: clean up commented premium check

* [PM-2014] feat: add user service test for credential limit

* [PM-2014] fix: run `dotnet format`

* [PM-2014] chore: remove trailing comma

* [PM-2014] chore: add `Async` suffix

* [PM-2014] chore: move delay to constant

* [PM-2014] chore: change `default` to `null`

* [PM-2014] chore: remove autogenerated weirdness

* [PM-2014] fix: lint
@kspearrin
Merge branch 'master' into fido2
e3fcc5e
@coroiu
Merge branch 'master' into fido2
38f48e2
@bitwarden-bot
Copy link

bitwarden-bot commented Aug 24, 2023 •
edited
Loading

Logo
Checkmarx One – Scan Summary & Details – 6b095899-7cf1-411b-9dc5-5da61260b170

New Issues

Severity Issue Source File / Package Checkmarx Insight
MEDIUM CSRF /src/Api/Auth/Controllers/WebAuthnController.cs: 79 Attack Vector
MEDIUM CSRF /src/Api/Auth/Controllers/WebAuthnController.cs: 62 Attack Vector
MEDIUM CSRF /src/Api/Auth/Controllers/WebAuthnController.cs: 37 Attack Vector
LOW Heap_Inspection /src/Core/Constants.cs: 43 Attack Vector
LOW Log_Forging /src/Api/Auth/Controllers/WebAuthnController.cs: 62 Attack Vector
LOW Log_Forging /src/Api/Auth/Controllers/WebAuthnController.cs: 62 Attack Vector
LOW Use_Of_Hardcoded_Password /src/Core/Constants.cs: 43 Attack Vector

@trmartin4 trmartin4 marked this pull request as ready for review September 20, 2023 16:46
@trmartin4 trmartin4 requested a review from a team as a code owner September 20, 2023 16:46
trmartin4 and others added 2 commits September 21, 2023 09:01
@trmartin4 @coroiu
Added check for PasswordlessLogin feature flag on new controller and …
84f7fc8 
…methods. (#3284)

* Added check for PasswordlessLogin feature flag on new controller and methods.

* fix: build error from missing constructor argument

---------

Co-authored-by: Andreas Coroiu <[email protected]>
@coroiu
[PM-4171] Update DB to support PRF (#3321)
d11fc46 
* [PM-4171] feat: update database to support PRF

* [PM-4171] feat: rename `DescriptorId` to `CredentialId`

* [PM-4171] feat: add PRF felds to domain object

* [PM-4171] feat: add `SupportsPrf` column

* [PM-4171] fix: add missing comma

* [PM-4171] fix: add comma
@kspearrin kspearrin requested a review from a team as a code owner October 10, 2023 11:19
@ike-kottlowski
[PM-3263] fix identity server tests for passkey registration (#3331)
3bda447 
* Added WebAuthnRepo to EF DI

* updated config to match current grant types
@trmartin4 trmartin4 changed the title support for fido2 auth Support for passkey registration Oct 18, 2023
ike-kottlowski
ike-kottlowski previously approved these changes Oct 26, 2023
View reviewed changes
Copy link
Contributor

@ike-kottlowski ike-kottlowski 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 to me. Pulled it down and tested it locally. Everything seems to work well.

Only thing is I wasn't able to delete the test account unless the passkeys were removed. I won't block the merge, but I'll add it to JIRA so it doesn't get lost.

@eliykat eliykat requested a review from jlf0dev October 30, 2023 04:55
@eliykat
Copy link
Member

eliykat commented Oct 30, 2023

@jlf0dev given that this is largely an auth PR, could you please review for @bitwarden/tech-leads?

💭 could the UserService methods be put in auth owned code rather than adding to the unowned, cross-domain UserService?

jlf0dev
jlf0dev approved these changes Oct 30, 2023
View reviewed changes
Copy link
Member

@jlf0dev jlf0dev 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!
@eliykat I just discussed this with @coroiu last week. The User Service methods are going to be refactored out in the iteration he is currently working on.

@trmartin4 trmartin4 merged commit 44c559c into master Oct 30, 2023
43 checks passed
@trmartin4 trmartin4 deleted the fido2 branch October 30, 2023 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ike-kottlowski ike-kottlowski ike-kottlowski approved these changes

@jlf0dev jlf0dev jlf0dev approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@kspearrin @bitwarden-bot @eliykat @jlf0dev @ike-kottlowski @trmartin4 @coroiu