Skip to content

Allow unknown fields when unmarshaling types.MFADevice#25437

Merged
rosstimothy merged 1 commit intomasterfrom
tross/fix_mfa_device_unmarshal
May 1, 2023
Merged

Allow unknown fields when unmarshaling types.MFADevice#25437
rosstimothy merged 1 commit intomasterfrom
tross/fix_mfa_device_unmarshal

Conversation

@rosstimothy
Copy link
Copy Markdown
Contributor

@rosstimothy rosstimothy commented May 1, 2023

There was a backward incompatible change introduced in #25238 that was preventing mfa devices from being unmarshalled due to an unknown field:

tctl users reset norbert
ERROR: rpc error: code = Unknown desc = unknown field "credentialRpId" in types.WebauthnDevice

The custom json.Unmarshaler implemented by types.MFADevice did not initialize the protojson decoder to allow unknown fields. This prevented any reads of types.MFADevice from a backend that was upgraded and then downgraded.

There was a backward incompatible change introduce in
#25238 that was preventing mfa devices from being
unmarshalled due to an unknown field:

```
tctl users reset norbert
ERROR: rpc error: code = Unknown desc = unknown field "credentialRpId" in types.WebauthnDevice
```

The custom `json.Unmarshaler` implemented by `types.MFADevice` did
not initialize the protojson decoder to allow unknown fields. This
prevented any reads of `types.MFADevice` from a backend that was
upgraded and then downgraded.
Comment thread lib/auth/usertoken.go
Comment on lines -142 to -145
_, err = s.GetUser(req.Name, false)
if err != nil {
return nil, trace.Wrap(err)
}
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.

s.ResetPassword calls GetUser before proceeding. Removed to reduce redundant/unnecessary backend operations.

@rosstimothy rosstimothy marked this pull request as ready for review May 1, 2023 20:45
@github-actions github-actions Bot requested review from flyinghermit and nklaassen May 1, 2023 20:45
@rosstimothy rosstimothy requested review from r0mant and zmb3 May 1, 2023 20:54
@rosstimothy rosstimothy enabled auto-merge May 1, 2023 20:58
@rosstimothy rosstimothy added this pull request to the merge queue May 1, 2023
Merged via the queue into master with commit b06994e May 1, 2023
@rosstimothy rosstimothy deleted the tross/fix_mfa_device_unmarshal branch May 1, 2023 22:59
@public-teleport-github-review-bot
Copy link
Copy Markdown

@rosstimothy See the table below for backport results.

Branch Result
branch/v11 Create PR
branch/v12 Create PR
branch/v13 Create PR

@codingllama
Copy link
Copy Markdown
Contributor

Thanks for the fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants