[PM-4167] Add PRF attestation flow during passkey registration #3339

coroiu · 2023-10-12T12:35:41Z

Type of change

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

Objective

Allow users to create and register all necessary keys to allow vault decryption using a passkey

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

bitwarden-bot · 2023-10-12T12:51:56Z

Checkmarx One – Scan Summary & Details – 9891f15f-17c6-4140-96fe-da20c4919e19

New Issues

Severity	Issue	Source File / Package	Checkmarx Insight
	Privacy_Violation	/src/Core/Services/Implementations/UserService.cs: 789	Attack Vector
	Privacy_Violation	/src/Core/Services/Implementations/UserService.cs: 789	Attack Vector

Fixed Issues

Severity	Issue	Source File / Package
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 368
	CSRF	/src/Api/Controllers/AccountsController.cs: 254
	Healthcheck Not Set	/docker-compose.yml: 4
	Healthcheck Not Set	/docker-compose.yml: 11
	Healthcheck Not Set	/docker-compose.override.yml: 4
	Healthcheck Not Set	/docker-compose.yml: 26
	Host Namespace is Shared	/docker-compose.override.yml: 4
	Host Namespace is Shared	/docker-compose.yml: 26
	Host Namespace is Shared	/docker-compose.yml: 4
	Host Namespace is Shared	/docker-compose.yml: 11
	Memory Not Limited	/docker-compose.yml: 4
	Memory Not Limited	/docker-compose.yml: 11
	Memory Not Limited	/docker-compose.yml: 26
	Memory Not Limited	/docker-compose.override.yml: 4
	Networks Not Set	/docker-compose.yml: 11
	Networks Not Set	/docker-compose.override.yml: 4
	Networks Not Set	/docker-compose.yml: 26
	Networks Not Set	/docker-compose.yml: 4
	Privacy_Violation	/src/Core/Auth/UserFeatures/UserMasterPassword/SetInitialMasterPasswordCommand.cs: 59
	Privacy_Violation	/src/Core/Auth/UserFeatures/UserMasterPassword/SetInitialMasterPasswordCommand.cs: 59
	Security Opt Not Set	/docker-compose.yml: 4
	Security Opt Not Set	/docker-compose.yml: 11
	Security Opt Not Set	/docker-compose.override.yml: 4
	Security Opt Not Set	/docker-compose.yml: 26
	Container Capabilities Unrestricted	/docker-compose.yml: 26
	Container Capabilities Unrestricted	/docker-compose.override.yml: 4
	Container Capabilities Unrestricted	/docker-compose.yml: 4
	Container Capabilities Unrestricted	/docker-compose.yml: 11
	Cpus Not Limited	/docker-compose.override.yml: 4
	Cpus Not Limited	/docker-compose.yml: 11
	Cpus Not Limited	/docker-compose.yml: 4
	Cpus Not Limited	/docker-compose.yml: 26
	Log_Forging	/src/Billing/Controllers/PayPalController.cs: 64
	Log_Forging	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 197
	Log_Forging	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 211
	Log_Forging	/src/Api/Controllers/AccountsController.cs: 110
	Log_Forging	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 211
	Log_Forging	/src/Api/Controllers/AccountsController.cs: 110
	Log_Forging	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 197
	Log_Forging	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 211
	Log_Forging	/src/Api/Controllers/AccountsController.cs: 110
	Log_Forging	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 197
	Log_Forging	/src/Api/AdminConsole/Controllers/PoliciesController.cs: 91
	Log_Forging	/src/Api/AdminConsole/Controllers/PoliciesController.cs: 91
	Log_Forging	/src/Api/AdminConsole/Controllers/PoliciesController.cs: 91
	Use_Of_Hardcoded_Password	/test/Core.Test/Auth/UserFeatures/UserMasterPassword/SetInitialMasterPasswordCommandTests.cs: 61

…port

withinfocus · 2023-11-01T15:37:12Z

src/Core/Services/IUserService.cs

@@ -28,7 +28,7 @@ public interface IUserService
    Task<bool> DeleteWebAuthnKeyAsync(User user, int id);
    Task<bool> CompleteWebAuthRegistrationAsync(User user, int value, string name, AuthenticatorAttestationRawResponse attestationResponse);
    Task<CredentialCreateOptions> StartWebAuthnLoginRegistrationAsync(User user);
-    Task<bool> CompleteWebAuthLoginRegistrationAsync(User user, string name, CredentialCreateOptions options, AuthenticatorAttestationRawResponse attestationResponse);
+    Task<bool> CompleteWebAuthLoginRegistrationAsync(User user, string name, bool supportsPrf, string encryptedUserKey, string encryptedPublicKey, string encryptedPrivateKey, CredentialCreateOptions options, AuthenticatorAttestationRawResponse attestationResponse);


🎨 Could these new parameters move to the end and be optional? For non-PRF default assumptions.

You could also consider making a data model for all these parameters and build it from the response model.

Fixed! I'll look into the model approach in my refactor task, where I'll convert these functions to commands!

jlf0dev

Looks good, I'll wait to approve until @ike-kottlowski gets a chance as well 👍

jlf0dev · 2023-11-01T18:34:00Z

src/Core/Auth/Entities/WebAuthnCredential.cs

+        if (SupportsPrf && EncryptedUserKey != null && EncryptedPrivateKey != null && EncryptedPublicKey != null)
+        {
+            return WebAuthnPrfStatus.Enabled;
+        }
+        else if (SupportsPrf)
+        {
+            return WebAuthnPrfStatus.Supported;
+        }
+
+        return WebAuthnPrfStatus.Unsupported;


nit (non-blocking): just a tiny suggestion for readability

Suggested change

if (SupportsPrf && EncryptedUserKey != null && EncryptedPrivateKey != null && EncryptedPublicKey != null)

{

return WebAuthnPrfStatus.Enabled;

}

else if (SupportsPrf)

{

return WebAuthnPrfStatus.Supported;

}

return WebAuthnPrfStatus.Unsupported;

if (!SupportsPrf)

{

return WebAuthnPrfStatus.Unsupported;

}

if (EncryptedUserKey != null && EncryptedPrivateKey != null && EncryptedPublicKey != null)

{

return WebAuthnPrfStatus.Enabled;

}

return WebAuthnPrfStatus.Supported;

jlf0dev · 2023-11-01T18:36:42Z

src/Core/Services/IUserService.cs

@@ -28,7 +28,7 @@ public interface IUserService
    Task<bool> DeleteWebAuthnKeyAsync(User user, int id);
    Task<bool> CompleteWebAuthRegistrationAsync(User user, int value, string name, AuthenticatorAttestationRawResponse attestationResponse);
    Task<CredentialCreateOptions> StartWebAuthnLoginRegistrationAsync(User user);
-    Task<bool> CompleteWebAuthLoginRegistrationAsync(User user, string name, CredentialCreateOptions options, AuthenticatorAttestationRawResponse attestationResponse);
+    Task<bool> CompleteWebAuthLoginRegistrationAsync(User user, string name, bool supportsPrf, string encryptedUserKey, string encryptedPublicKey, string encryptedPrivateKey, CredentialCreateOptions options, AuthenticatorAttestationRawResponse attestationResponse);


You could also consider making a data model for all these parameters and build it from the response model.

ike-kottlowski

Overall looks good. No notes.

coroiu changed the base branch from master to fido2 October 12, 2023 12:35

coroiu force-pushed the PM-4167-add-prf-attestation-flow-during-passkey-registration-server branch from 3ba6c73 to 4f0e15b Compare October 12, 2023 12:42

coroiu force-pushed the PM-4167-add-prf-attestation-flow-during-passkey-registration-server branch 2 times, most recently from 696638e to 083b342 Compare October 16, 2023 12:26

Base automatically changed from fido2 to master October 30, 2023 13:40

coroiu force-pushed the PM-4167-add-prf-attestation-flow-during-passkey-registration-server branch from 083b342 to 6c31877 Compare November 1, 2023 08:36

coroiu added 7 commits November 1, 2023 10:52

[PM-4167] feat: add support for SupportsPrf

e09c791

[PM-4167] feat: add prfStatus property

2e8722a

[PM-4167] feat: add support for storing PRF keys

3e04edd

[PM-4167] fix: allow credentials to be created without encryption sup…

73606a2

…port

[PM-4167] fix: broken test

0b679d2

[PM-4167] chore: remove whitespace

1d908fe

[PM-4167] fix: controller test

5e589ae

coroiu force-pushed the PM-4167-add-prf-attestation-flow-during-passkey-registration-server branch from 6c31877 to 5e589ae Compare November 1, 2023 09:52

coroiu marked this pull request as ready for review November 1, 2023 09:53

coroiu requested review from a team as code owners November 1, 2023 09:53

coroiu requested a review from andrebispo5 November 1, 2023 09:53

ike-kottlowski self-requested a review November 1, 2023 14:40

jlf0dev self-requested a review November 1, 2023 15:11

withinfocus reviewed Nov 1, 2023

View reviewed changes

withinfocus requested a review from a team November 1, 2023 15:37

jlf0dev reviewed Nov 1, 2023

View reviewed changes

ike-kottlowski previously approved these changes Nov 1, 2023

View reviewed changes

[PM-4167] chore: improve readability of GetPrfStatus

ee3fad2

jlf0dev previously approved these changes Nov 7, 2023

View reviewed changes

[PM-4167] fix: make prf optional

71e67e5

coroiu dismissed stale reviews from jlf0dev and ike-kottlowski via 71e67e5 November 7, 2023 14:56

coroiu requested a review from jlf0dev November 7, 2023 14:57

coroiu added 2 commits November 7, 2023 16:02

[PM-4167] fix: commit missing controller change

52ac09c

[PM-4167] fix: tests

378b51d

jlf0dev approved these changes Nov 7, 2023

View reviewed changes

coroiu merged commit e401fc0 into master Nov 7, 2023
40 checks passed

coroiu deleted the PM-4167-add-prf-attestation-flow-during-passkey-registration-server branch November 7, 2023 15:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PM-4167] Add PRF attestation flow during passkey registration #3339

[PM-4167] Add PRF attestation flow during passkey registration #3339

coroiu commented Oct 12, 2023 •

edited

Loading

bitwarden-bot commented Oct 12, 2023 •

edited

Loading

withinfocus Nov 1, 2023

jlf0dev Nov 1, 2023

coroiu Nov 7, 2023

jlf0dev left a comment

jlf0dev Nov 1, 2023

coroiu Nov 7, 2023

jlf0dev Nov 1, 2023

ike-kottlowski left a comment

[PM-4167] Add PRF attestation flow during passkey registration #3339

[PM-4167] Add PRF attestation flow during passkey registration #3339

Conversation

coroiu commented Oct 12, 2023 • edited Loading

Type of change

Objective

Before you submit

bitwarden-bot commented Oct 12, 2023 • edited Loading

New Issues

Fixed Issues

withinfocus Nov 1, 2023

Choose a reason for hiding this comment

jlf0dev Nov 1, 2023

Choose a reason for hiding this comment

coroiu Nov 7, 2023

Choose a reason for hiding this comment

jlf0dev left a comment

Choose a reason for hiding this comment

jlf0dev Nov 1, 2023

Choose a reason for hiding this comment

coroiu Nov 7, 2023

Choose a reason for hiding this comment

jlf0dev Nov 1, 2023

Choose a reason for hiding this comment

ike-kottlowski left a comment

Choose a reason for hiding this comment

coroiu commented Oct 12, 2023 •

edited

Loading

bitwarden-bot commented Oct 12, 2023 •

edited

Loading