Allow skipping verification of addresses already verified by OIDC providers

[Saancreed]

Preflight checklist

• I could not find a solution in the existing issues, docs, nor discussions.
• I agree to follow this project's Code of Conduct.
• I have read and am following this repository's Contribution Guidelines.
• I have joined the Ory Community Slack.
• I am signed up to the Ory Security Patch Newsletter.

Ory Network Project

No response

Context and scope

In (fairly common) scenarios in which Kratos is configured to allow registration using both password and OIDC methods, identity schema contains a verifiable address (often email) and having a verified address is required to sign in with password, the inability to mark an address as verified during OIDC registration leads to several pain points:

• User still gets an email asking them to complete verification flow, even if they intend to always sign in with OIDC where the verified address requirement is ignored anyway
• User cannot just set a password and immediately use it to sign in, they do need to complete verification in this case
• When Kratos is considered to be the source of truth with regard to user's verified addresses for domain-specific purposes other than signing in (e.g. checking if we can send sensitive information to some email), we can't simply use addresses from OIDC providers even if we trust providers themselves if they user didn't complete verification

See related issue: #1057

The plan is to allow Kratos administrator to configure which addresses are trusted to be already verified for each known OIDC provider.

Goals and non-goals

Goals:

• Configurable per OIDC provider instructions in which cases the verification can be skipped and the address can be assumed to be already verified
• Little to no assumptions made about the type or count of such addresses; the administrator should be free to mark multiple addresses as verified if some provider returns claims with multiple user emails or phone numbers, or if new address type is introduced in the future

Non-goals:

• Skipping verification of addresses coming from self-service registration flows using methods other than OIDC one
• Changing behavior for OIDC registrations where the user changes their address after callback (which can happen when they are redirected back to registration form due to missing traits that could not be derived from OIDC claims)

The design

We allow Jsonnet mapper to produce additional result returned via verified_addresses key. If present, it should be an array of objects each with two string properties: via describing the type of an address (corresponding directly to VerifiableAddressType) and value being the value of the address itself (phone number or email). After the identity is created and passes validation, any verifiable address matching one of addresses in verified_addresses is automatically marked as verified and no verification flow is started.

To simplify comparison of email addresses without complicating Jsonnet snippets, any value in an object with via equal to "email" is normalized by trimming spaces and converting to lower case after evaluating the snippet but before performing the compariton itself, matching normalization rules of verifiable addresses with email type.

APIs

Client APIs are unaffected, the only contract that changes is the extension of result returned by Jsonnet OIDC claim mappers.

Data storage

No response

Code and pseudo-code

A sample work-in-progress implementation can be found here: https://github.com/leancodepl/kratos/tree/feat/oidc-preverified-addresses

Then, an example Jsonnet snippet taking advantage of this feature could look like this:

local claims = {
  email_verified: false,
} + std.extVar('claims');

{
  identity: {
    traits: {
      [if 'email' in claims && claims.email_verified then 'email' else null]: claims.email,
      given_name: claims.given_name,
      family_name: claims.family_name,
    },
  },
  verified_addresses: std.prune([
    if 'email' in claims && claims.email_verified then { via: 'email', value: claims.email },
  ]),
}

Degree of constraint

No response

Alternatives considered

• Configuring this via kratos.yaml config file, but expressing similar logic in YAML could be clunky while simplifying the feature to "here, look if this, let's say, email_verified claim is true" makes it less obvious which email will be considered trusted and what will happen if user modified their traits before completing registration while also being less extensible.

[Sytten]

I like this rfc @Saancreed a lot I think this is more flexible than a claim check (like I proposed in the past #1057 (comment)). We do however need to help users make sure the email can be trusted.

• Google has the email_verified claim so its easy (https://developers.google.com/identity/openid-connect/openid-connect#an-id-tokens-payload)
• Microsoft is known to have had account take over issues (https://learn.microsoft.com/en-us/azure/active-directory/develop/migrate-off-email-claim-authorization), they added the optional xms_edov
• Github only gives the public email if set and I am unsure if it needs to be verified, you need to make an additional API call to get all emails (https://stackoverflow.com/questions/35373995/github-user-email-is-null-despite-useremail-scope)
• Apple gives the email_verified so its easy (https://developer.apple.com/documentation/sign_in_with_apple/sign_in_with_apple_rest_api/authenticating_users_with_sign_in_with_apple)
• Facebook provides contradictory information on the subject, some documentation seems to indicate you wont receive an email if it not verified but other documentation suggests you should always reverify the email (https://stackoverflow.com/questions/26672839/facebook-login-and-email-verification)
• Twitter requires an additional call to be made, but will only return the email if it is verified (https://developer.twitter.com/en/docs/twitter-api/v1/accounts-and-users/manage-account-settings/api-reference/get-account-verify_credentials)
  So I think for the feature to be complete we need a configuration key to allow kratos to fetch more data prior to running the jsonnet. I am unsure what exactly right now but I will keep thinking about it.

[Saancreed]

I like this rfc @Saancreed a lot I think this is more flexible than a claim check (like I proposed in the past #1057 (comment)).

Thanks! I've seen your suggestion and addressed it "Alternatives considered" section. It would indeed be simpler to configure like this but it makes a bit too many implicit assumptions for my tastes which I'm afraid wouldn't be very clear to the users. And while Kratos will attach email_verified itself for some providers, if you tried to add a custom integration with anything that doesn't provide the claim but returns only verified addresses, configuring that becomes tricky. Hopefully my take on this which should be flexible enough to handle various corner cases in the Jsonnet mapper itself isn't too complicated.

We do however need to help users make sure the email can be trusted.

I have to admit, the only providers I had in mind while designing this were Google and Apple and those cases are already covered with no additional work required. But to address some other providers you mentioned:

Facebook provides contradictory information on the subject, some documentation seems to indicate you wont receive an email if it not verified but other documentation suggests you should always reverify the email

Kratos seems to go with the former.

Twitter requires an additional call to be made, but will only return the email if it is verified

Actually, so does Facebook! Kratos already does additional calls for some providers before executing Jsonnet mappers, e.g. provider for Facebook retrieves claims using a separate HTTP call to Facebook's Graph API.

But in any case, that would be some additional provider-specific code which could (should?) be handled separately from this feature and implemented in another PR.

[Sytten]

Agreed it's a good design, lets try to get some input from ory people.

[aeneasr]

Great stuff, this makes a lot of sense IMO. I think we could put the verified_addresses field under the identity key as it relates to the identity? :)

local claims = {
  email_verified: false,
} + std.extVar('claims');

{
  identity: {
    traits: {
      [if 'email' in claims && claims.email_verified then 'email' else null]: claims.email,
      given_name: claims.given_name,
      family_name: claims.family_name,
    },
  },
  verified_addresses: std.prune([
    if 'email' in claims && claims.email_verified then { via: 'email', value: claims.email },
  ]),
}

[Saancreed]

Sure, that would also work (except your code is currently the same as mine). I assumed having this returned "separately" would be better because of the distinction that values under identity are, in one way of another, ones we persist to the database while verified_addresses is more transient, used only while processing the registration and discarded quickly afterwards. I didn't want to suggest that it would create additional verifiable addresses for the user other than ones extracted from identity and matching the schema but the choice was done fairly lightly.

[meysam81]

verified_addresses: std.prune([
if 'email' in claims && claims.email_verified then { via: 'email', value: claims.email },
]),

I realized that the hierarchy in this jsonnet is not correct.

As I see it in the following code, the verified_addresses need to be a child element of identity:

https://github.com/monta-app/kratos/blob/bc9a3b0d9df7ccbb50af198a0e279c51ec9893ad/selfservice/strategy/oidc/strategy_registration.go#L52

Consequently, your jsonnet would turn out to be the following:

local claims = {
  email_verified: false,
} + std.extVar('claims');

{
  identity: {
    traits: {
      [if 'email' in claims && claims.email_verified then 'email' else null]: claims.email,
      given_name: claims.given_name,
      family_name: claims.family_name,
    },

    verified_addresses: std.prune([
      if 'email' in claims && claims.email_verified then { via: 'email', value: claims.email },
    ]),

  },
}