Skip to content

RFD 155 - Scoped Webauthn Credentials#35185

Merged
Joerger merged 8 commits intomasterfrom
rfd/0155-scoped-webauthn-credentials
Dec 14, 2023
Merged

RFD 155 - Scoped Webauthn Credentials#35185
Joerger merged 8 commits intomasterfrom
rfd/0155-scoped-webauthn-credentials

Conversation

@Joerger
Copy link
Copy Markdown
Contributor

@Joerger Joerger commented Nov 30, 2023

This feature was initially suggested by @jentfoo in this discussion.

I intend to use this feature for admin actions to allow safe reuse of webauthn credentials for specific "bulk" admin actions:

  • Creating a user - 1 MFA for CreateUser and 1 for CreateResetPasswordToken
  • Updating a SAML Idp service provider - 1 MFA CreateSAMLIDP... to check existence, and 1 for UpdateSAMLIDP...
  • Bulk creating of resources with tctl create
  • Creating cloud invites with the button in the WebUI

@github-actions github-actions Bot requested review from bl-nero and kimlisa November 30, 2023 04:18
@github-actions github-actions Bot added rfd Request for Discussion size/md labels Nov 30, 2023
@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 requested review from codingllama, jentfoo, klizhentas, rosstimothy and xinding33 and removed request for bl-nero and kimlisa November 30, 2023 04:18
@Joerger Joerger added the no-changelog Indicates that a PR does not require a changelog entry label Nov 30, 2023
Comment thread rfd/0155-scoped-webauthn-credentials.md Outdated
Comment thread rfd/0155-scoped-webauthn-credentials.md Outdated
Comment thread rfd/0155-scoped-webauthn-credentials.md Outdated
Comment thread rfd/0155-scoped-webauthn-credentials.md Outdated
Comment thread rfd/0155-scoped-webauthn-credentials.md Outdated
Comment thread rfd/0155-scoped-webauthn-credentials.md Outdated
Comment thread rfd/0155-scoped-webauthn-credentials.md
Comment thread rfd/0155-scoped-webauthn-credentials.md Outdated
Comment thread rfd/0155-scoped-webauthn-credentials.md Outdated
Comment thread rfd/0155-scoped-webauthn-credentials.md Outdated
Comment thread rfd/0155-scoped-webauthn-credentials.md Outdated
credentials would prevent the attacker from using the stolen webauthn
credentials freely.

Adding scope also enables us to allow webauthn credentials to be reused within
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.

I appreciate the effort to minimize the scope of an MFA. However I think the tracked reuse complicates this:

  • More writes to the backend as each use needs to be atomically decremented, that atomic nature now becoming very important to this update
  • The client has no guarantees that reuse is exactly as intended anyways, instead this mostly provides an automatic expiration mechanism (based on usage)

As a possible simplification we could have a reuse boolean, which would not always be allowed (similar to how you currently define reuse is not always allowed). This flag would mean only MFA actions which don't allow reuse would require a single update to track. In other cases we would rely on short expiration, but in technically allow unlimited reuse during that period.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I am ok with removing this, I just wanted to show it in case we had strong opinions to have it.

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.

Adding to this, we could have clients tell the server when they are done using a challenge, so it can be cleaned up. We may not even need the reuse bool, as some kinds can't be reused and others (like admin challenges) can use the client signal for the early delete.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm going to omit this from the RFD, but I'll investigate this during implementation as a nice to have.

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.

Looks good, just waiting for resolution in a couple of open threads.

Comment thread rfd/0155-scoped-webauthn-credentials.md Outdated
Comment thread rfd/0155-scoped-webauthn-credentials.md
Comment thread rfd/0155-scoped-webauthn-credentials.md Outdated
Copy link
Copy Markdown
Contributor

@jentfoo jentfoo left a comment

Choose a reason for hiding this comment

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

This looks like a great improvement, thank you for putting it together @Joerger

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.

LGTM.

Comment thread rfd/0155-scoped-webauthn-credentials.md Outdated
Comment thread rfd/0155-scoped-webauthn-credentials.md Outdated
@Joerger
Copy link
Copy Markdown
Contributor Author

Joerger commented Dec 1, 2023

@rosstimothy does this RFD need approval from product, or should I remove those required reviewers?

@Joerger Joerger force-pushed the rfd/0155-scoped-webauthn-credentials branch from c4c9940 to 9284ee3 Compare December 1, 2023 22:21
Comment on lines +78 to +81
+ // Standard webauthn login.
+ SCOPE_LOGIN = 1;
+ // Passwordless webauthn login.
+ SCOPE_PASSWORDLESS_LOGIN = 2;
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.

Curious, why do these need to be distinct scopes?

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.

My 2c: they don't have to be distinct, but I think it's a good distinction. The passwordless challenge is a tad more powerful than the login one, as it covers multiple factors at once. It also only happens in very specific situations (initial user login), whereas the SCOPE_LOGIN equivalent is used for re-authentication (pre RFD 155, that is).

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.

Like I said I was more curious why the two are distinct and not advocating for them to be unified. I'd be happy with a comment elaborating on the differences.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's not exactly necessary, but it comes out naturally in the code, replacing passwordless bool in many methods with just scope, and I see no reason to not have this scope.

Comment thread rfd/0155-scoped-webauthn-credentials.md
Comment thread rfd/0155-scoped-webauthn-credentials.md
Comment thread rfd/0155-scoped-webauthn-credentials.md
Comment thread rfd/0155-scoped-webauthn-credentials.md
Comment thread rfd/0155-scoped-webauthn-credentials.md Outdated
Comment thread rfd/0155-scoped-webauthn-credentials.md
@Joerger Joerger requested a review from rosstimothy December 5, 2023 17:23
Comment thread rfd/0155-scoped-webauthn-credentials.md Outdated
Comment thread rfd/0155-scoped-webauthn-credentials.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog Indicates that a PR does not require a changelog entry rfd Request for Discussion size/md

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants