Skip to content

Scoped Webauthn Challenges#36588

Closed
Joerger wants to merge 7 commits intomasterfrom
joerger/scoped-webauthn-credentials
Closed

Scoped Webauthn Challenges#36588
Joerger wants to merge 7 commits intomasterfrom
joerger/scoped-webauthn-credentials

Conversation

@Joerger
Copy link
Copy Markdown
Contributor

@Joerger Joerger commented Jan 11, 2024

Implements scoped Webauthn challenges as described in #35185.

Follow up PRs:

  • Add audit events and product metrics
  • Prompt for reusable MFA when needed to avoid redundant mfa prompts (tctl users add, tctl create muliple-resources.yaml, etc.`)

changelog: Add scope to WebAuthn challenges

@Joerger Joerger force-pushed the joerger/scoped-webauthn-credentials branch 3 times, most recently from fce5a27 to f247bb3 Compare January 11, 2024 21:06
@Joerger Joerger marked this pull request as ready for review January 11, 2024 21:09
@github-actions github-actions Bot added size/md tsh tsh - Teleport's command line tool for logging into nodes running Teleport. ui labels Jan 11, 2024
@github-actions
Copy link
Copy Markdown
Contributor

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

@Joerger Joerger force-pushed the joerger/scoped-webauthn-credentials branch 5 times, most recently from abe66f2 to d4799d1 Compare January 12, 2024 04:22
@Joerger Joerger force-pushed the joerger/scoped-webauthn-credentials branch from d4799d1 to 9a0668c Compare January 12, 2024 04:27
Copy link
Copy Markdown
Contributor

@mdwn mdwn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, admittedly this review was a bit daunting to me. It looks pretty straightforward as a replacement of more "basic" MFA with scoped MFA. I can't speak to the validity of the scopes, but otherwise LGTM.

Comment thread lib/authz/permissions.go
return nil
}

if err := a.authorizeAdminAction(ctx, authContext); err != nil {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: You could replace this if statement and the subsequent return with just

return trace.Wrap(a.authorizeAdminAction(ctx, authContext)

Copy link
Copy Markdown
Contributor

@codingllama codingllama left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the first few commits, more to follow.

I would move proto definitions (and a possible "refactor" of SessionData) to a separate PR. Nailing definitions is a good part of the battle.

Comment thread api/proto/teleport/legacy/types/webauthn/webauthn.proto
Comment thread api/proto/teleport/legacy/types/webauthn/webauthn.proto
Comment thread api/proto/teleport/legacy/types/webauthn/webauthn.proto
Comment thread api/proto/teleport/legacy/types/webauthn/webauthn.proto
Comment thread api/proto/teleport/legacy/types/webauthn/webauthn.proto
Comment thread api/proto/teleport/legacy/types/webauthn/webauthn.proto
Comment thread lib/auth/auth.go
Comment thread lib/auth/auth.go Outdated
Comment thread lib/auth/webauthn/login.go
Comment thread lib/auth/webauthn/login.go
Comment thread api/proto/teleport/legacy/types/webauthn/webauthn.proto
Copy link
Copy Markdown
Contributor

@codingllama codingllama left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more comments.

I can say with certainty now that this PR is too big. Please split into smaller, self-focused parts.

A few split ideas:

  1. Proto definitions
  2. (Possible) Make SessionData a storage-only type
  3. Add scopes lib/auth/webauthn (minimal code changes elsewhere)
  4. Add scopes to other important methods (server-side)
  5. Add scopes to other important methods (client-side)
  6. Teleport binary changes (if significant/not part of the above)
  7. Web UI changes

Comment thread lib/auth/webauthn/login.go
Comment thread lib/auth/webauthn/login_test.go
Comment thread lib/auth/webauthn/login_test.go
Comment thread lib/auth/webauthn/login_test.go
Comment thread lib/auth/webauthn/login_test.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/md tsh tsh - Teleport's command line tool for logging into nodes running Teleport. ui

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants