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

Include enterpriseAttestation in getClientClientCapabilities #2051

Closed
wants to merge 2 commits into from

Conversation

timcappalli
Copy link
Member

@timcappalli timcappalli commented Mar 27, 2024

  • Adds enterpriseAttestation to getClientClientCapabilities enum
  • Adds blurb to "enterprise" definition that clients should include it

Resolves #1742


Preview | Diff

@timcappalli
Copy link
Member Author

@msft-bob looks like you're not in the repo so I can't assign review to you, but flagging for your feedback via comment

Copy link
Member

@emlun emlun left a comment

Choose a reason for hiding this comment

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

This probably needs to take into account not only whether the client can support enterprise attestation, but also whether enterprise attestation is allowed for the calling RP.

There's also the possible complication that it might be allowed by the client, but not by the authenticator if it is configured for vendor-facilitated enterprise attestation instead of platform-managed enterprise attestation. Do we need to care about that here? @ve7jtb

@timcappalli
Copy link
Member Author

This probably needs to take into account not only whether the client can support enterprise attestation, but also whether enterprise attestation is allowed for the calling RP.

@emlun the intent was to only convey whether the WebAuthn client supported it, not whether it was allowed for a given origin/RP.

@emlun
Copy link
Member

emlun commented Apr 2, 2024

Ah, ok. In that case it's probably worth mentioning in the {{ClientCapability/enterpriseAttestation}} definition that the presence of this capability is no guarantee that the client's/authenticator's policy allows EA for the calling RP.

@timcappalli
Copy link
Member Author

Ah, ok. In that case it's probably worth mentioning in the {{ClientCapability/enterpriseAttestation}} definition that the presence of this capability is no guarantee that the client's/authenticator's policy allows EA for the calling RP.

@emlun addressed in 51b4b40

Copy link
Member

@emlun emlun left a comment

Choose a reason for hiding this comment

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

LGTM!

@nsatragno
Copy link
Member

The type error if an attestation type is not supported should not be a reason to merge this. Browsers should not error out on unknown values to begin with, and we should not patch non compliant behaviour with more feature detection.

However, when an RP requires Enterprise Attestation, it probably doesn't make any sense to continue the ceremony when it has no chance to succeed -- so I can see value in this capability. My only concern is there will be a gap until we get the browsers updated where EA will be supported, but this capability will not. Are we risking those RPs assuming EA is not supported when it is during that gap?

@nadalin nadalin added the stat:Blocked Prerequisites are not yet satisfied label May 1, 2024
@timcappalli
Copy link
Member Author

Closing for the time being.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stat:Blocked Prerequisites are not yet satisfied
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should enterprise attestation support be flagged explicitly?
6 participants