-
Notifications
You must be signed in to change notification settings - Fork 252
Add Atomic API based Passkey Registration and Authentication Support #987
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request introduces foundational support for WebAuthn/Passkey authentication in the backend, including credential registration and authentication flows. The implementation refactors the user credentials storage model from an array to a map-based structure to support multiple credential types (password, pin, secret, passkey) and adds comprehensive WebAuthn service layer with session management.
Key Changes:
- Refactored user credential storage from
[]CredentialtoCredentials(map[string][]Credential) to support multiple credential types - Implemented complete WebAuthn service with registration/authentication flows
- Added database schema for WebAuthn session storage with proper deployment isolation
- Created new API endpoints for WebAuthn operations
Reviewed changes
Copilot reviewed 43 out of 44 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
backend/dbscripts/runtimedb/*.sql |
Added WEBAUTHN_SESSION table with deployment isolation support |
backend/internal/user/model.go |
Refactored Credential structure and added Credentials map type |
backend/internal/user/constants.go |
Added credential type constants and validation methods |
backend/internal/user/service.go |
Updated credential handling to use map-based structure |
backend/internal/user/handler.go |
Enhanced handler to hash credentials before service calls |
backend/internal/authn/webauthn/*.go |
New WebAuthn service implementation with session management |
backend/internal/authn/handler.go |
Added WebAuthn API handlers |
backend/internal/authn/init.go |
Registered WebAuthn routes and service |
backend/go.mod |
Added WebAuthn library dependencies |
| Various test files | Updated mocks and added unit tests |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #987 +/- ##
==========================================
- Coverage 87.14% 87.07% -0.07%
==========================================
Files 526 533 +7
Lines 35024 36002 +978
Branches 1611 1611
==========================================
+ Hits 30522 31350 +828
- Misses 2882 3013 +131
- Partials 1620 1639 +19
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
7628df1 to
0a7670e
Compare
0a7670e to
dc015bc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 43 out of 44 changed files in this pull request and generated no new comments.
45e65a5 to
fa27491
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 43 out of 44 changed files in this pull request and generated no new comments.
fa27491 to
f60e349
Compare
f60e349 to
1ed49a2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 43 out of 44 changed files in this pull request and generated no new comments.
5ad8c38 to
f5c5c6f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 43 out of 44 changed files in this pull request and generated no new comments.
2971d35 to
acb9d60
Compare
| AuthenticatorGithub = "GithubOAuthAuthenticator" | ||
| AuthenticatorOAuth = "OAuthAuthenticator" | ||
| AuthenticatorOIDC = "OIDCAuthenticator" | ||
| AuthenticatorWebAuthn = "WebAuthnAuthenticator" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to think again whether to go ahead with the webauthn term or passkeys. Webauthn is okay for the internal implementations as it is the core technology that we're using. But passkeys is more user friendly and should be used for user facing interfaces such as develop app UI, token assurance, authenticator list, etc.
Since this is a new implementation, we should think whether to mixup both terms or stick to one. Another related question is whether we're going to support any other webauthn authentication options except for FIDO2 in future. My answer is no. Hence we can stick to passkeys.
@darshanasbg WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a side note, passkeys is not webauthn or fido. Passkeys is the modern term for FIDO2 + Webauthn. So in technical terms it should be either FIDO2 or WebAuthn.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed to passkey as discussed offline
|
|
||
| // WebAuthnConfig holds the WebAuthn configuration details. | ||
| type WebAuthnConfig struct { | ||
| AllowedOrigins []string `yaml:"allowed_origins" json:"allowed_origins"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we also add default configs corresponding to the cors allowed origins to the deployment.yaml file?
6ecc7a8 to
4d86a12
Compare
4d86a12 to
3bde4e1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 40 out of 41 changed files in this pull request and generated no new comments.
3bde4e1 to
31bcde0
Compare
31bcde0 to
3a4265f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 40 out of 41 changed files in this pull request and generated no new comments.
8558aa9 to
4ba3d6b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 41 out of 42 changed files in this pull request and generated no new comments.
| port: | ||
| default: "8090" | ||
|
|
||
| paths: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move this to api/authentication.yaml and remove this file as the API is implemented
|
|
||
| // webauthnUserInterface defines the interface for WebAuthn user operations. | ||
| // This interface abstracts the passkey.User interface to reduce direct dependencies. | ||
| type webauthnUserInterface interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we move this to models.go?
| jsonKeyExpires = "expires" | ||
| jsonKeyUserVerification = "user_verification" | ||
| jsonKeyExtensions = "extensions" | ||
| jsonKeyCredParams = "cred_params" // nolint:gosec // This is a JSON key, not a credential |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's verify this lint disable comment is needed. If not shall we remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to add this since it gave a gosec warning
| allowed_origins: | ||
| - "https://localhost:3000" | ||
|
|
||
| passkey: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If cors origins are also there, let's add this to default.json as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cors origins are not in the default.json
|
|
||
| passkey: | ||
| allowed_origins: | ||
| - "https://localhost:8090" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can only keep "https://localhost:8090". Let's remove other too from the list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
|
||
| logger.Debug("Passkey credential creation options generated successfully", | ||
| log.String("userID", log.MaskString(req.UserID)), | ||
| log.Int("excludeCredentialsCount", len(credentials))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this log field correct? excludeCredentialsCount
| } | ||
|
|
||
| // Retrieve user's existing passkey credentials from database | ||
| credentials, err := w.getPasskeyCredentialsFromDB(req.UserID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: We need to verify whether WebAuthn user needs to have credentials for initialization phase. I feel like it's not a good solution to retrieve credentials prior and return in response or set in memory unless absolutely necessary.
Let's proceed with current implementation and revisit this later
|
|
||
| // Verify the credential creation response using WebAuthn library service | ||
| logger.Debug("Calling FinishRegistration with session data", | ||
| log.String("sessionChallenge", sessionData.Challenge), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's avoid logging unnecessary details even in debug logs. Also shall we merge the two debug logs?
I feel like second log is unnecessary
| credential, err := webAuthnLibService.FinishRegistration(webAuthnUserAdapter, *sessionData, parsedCredential) | ||
| if err != nil { | ||
| logger.Error("Failed to verify and create credential", log.String("error", err.Error())) | ||
| return nil, &ErrorInvalidAttestationResponse |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the correct error? If so let's avoid logging client errors
|
|
||
| logger.Debug("Passkey credential verified and created", | ||
| log.String("credentialID", credentialID), | ||
| log.String("aaguid", base64.StdEncoding.EncodeToString(credential.Authenticator.AAGUID))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's avoid logging unnecessary details even in debug logs. Some of these fields maybe sensitive data. We shouldn't log sensitive stuff/ PIIs even in debug logs
| // Retrieve user by userID to verify user exists | ||
| userInfo, svcErr := w.userService.GetUser(userID) | ||
| if svcErr != nil { | ||
| if svcErr.Type == serviceerror.ClientErrorType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we check for user not found error specifically? This could be any other client error as well
|
|
||
| logger.Debug("Passkey authentication options generated successfully", | ||
| log.String("userID", log.MaskString(userID)), | ||
| log.Int("allowedCredentialsCount", len(credentials))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this log field correct? allowedCredentialsCount
| parsedResponse, err := parseAssertionResponse(credentialID, credentialType, | ||
| clientDataJSON, authenticatorData, signature, userHandle) | ||
| if err != nil { | ||
| logger.Error("Failed to parse assertion response", log.String("error", err.Error())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we avoid logging client errors? You can have deb ug logs.
Check other places as well
| ) | ||
|
|
||
| // sessionStoreInterfaceMock is a mock implementation of sessionStoreInterface. | ||
| type sessionStoreInterfaceMock struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we use mockery generated mocks instead of custom ones?
4ba3d6b to
4e383a7
Compare
This pull request introduces foundational support for WebAuthn-based authentication and registration in the backend. The changes include database schema updates, new handler methods for WebAuthn flows, updates to authentication constants, and enhancements to the test mocks. Additionally, several dependencies are updated or added, including the core WebAuthn library.
WebAuthn Feature Implementation
authenticationHandlerto support starting and finishing WebAuthn registration and authentication, enabling the backend to process WebAuthn flows.AuthenticatorWebAuthnconstant to the list of supported authenticators.Database Schema Changes
WEBAUTHN_SESSIONtable and supporting indexes to both Postgres and SQLite schema files for storing WebAuthn session data. [1] [2]Testing and Mocking Enhancements
AuthenticationServiceInterfaceMockwith methods for starting and finishing WebAuthn authentication and registration, supporting improved test coverage for the new flows. [1] [2]Dependency Updates
github.com/go-webauthn/webauthnand related packages, and updated several existing dependencies to newer versions ingo.modto support WebAuthn and related functionalities.