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.

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

feat: Add Webauthn support #9194

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from
Draft

Conversation

RicardoE105
Copy link
Contributor

@RicardoE105 RicardoE105 commented Apr 23, 2024

Summary

This feature came out of Hackmation in the Berlin Mitte IRL. Some folks in the community and internally are interested in getting this feature production-ready. In an attempt to make it happen, and track progress, you can see below the to-do.

TO-DO

  • Save security keys in the database, instead of in memory. See the suggested model here.
  • When user is logged and security keys are enabled add user id to cookie, so we can return the challenge for the user's registered keys.
  • Return security keys feature flag in /settings endpoint. Currently we only return whether MFA is enabled or not (referring to TOTP). We need to make more generic. e.g: { "mfa": { "securityKeys": { "enabled": true }, "totp": { "enabled": true } } }.
  • Return whether the user has security keys or TOTP enabled in the GET /login endpoint. We will use this data to build the MFA section in the user's profile.
  • Update MFA endpoints to differentiate between TOTP and security keys.
  • Prompt for security keys when user's attempts to change password.
  • Make all actions in MFA section for each MFA method to work. e.g disable or set as default.
  • Add tests to new endpoints
  • Add e2e tests to test security keys flow if possible.
  • Nice to have - Allow for passkeys as well as it basically uses the same mechanism as security keys.
  • Nice to have - Allow owner via settings to enforce members to enable a MFA method at any time. This could be an awesome feature for companies that need to meet security compliances.

To be determined:

  1. Currently, the instance owner can disable or enable MFA (referring to TOTP) in the instance via env variables. Do we want to deprecate this behavior and always make it available in the instance? Whatever we decide here, we need to do the same with Security keys.
  2. Should make the MFA logic generic so we could easily extend to other MFA methods like SMS, Push notifications or Code in email easily? is it worth it considering the before mentioned methods are less secure than TOTP and security keys.
  3. Should we allow multiple MFA methods to be enabled at the same time? Github for example, allows you to do this, which is very convenient when your browser does not support Webauthn (very unlikely I think), but defeats the whole purpose of having security keys in the first place, as the attacker could use a less secure MFA method to gain access to your account.

Review / Merge checklist

  • PR title and summary are descriptive. Remember, the title automatically goes into the changelog. Use (no-changelog) otherwise. (conventions)
  • Docs updated or follow-up ticket created.
  • Tests included.

    A bug is not considered fixed, unless a test is added to prevent it from happening again.
    A feature is not complete without tests.

RicardoE105 and others added 9 commits April 16, 2024 10:12
…n-io/n8n into hackmation-milorado-irl-mitte

* 'hackmation-milorado-irl-mitte' of https://github.com/n8n-io/n8n:
  add store methods to do security keys CRUD operations
  thow error when security key already registered
  add possibility to update security keys name and prevent existing keys from registeing
  add endpoints to retrieve/delete security keys

# Conflicts:
#	packages/editor-ui/src/views/SettingsPersonalView.vue
@RicardoE105 RicardoE105 changed the title Add Webauthn support feat: Add Webauthn support Apr 23, 2024
@n8n-assistant n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team ui Enhancement in /editor-ui or /design-system labels Apr 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team ui Enhancement in /editor-ui or /design-system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants