Skip to content

Fix webauthnwin c types size#31407

Merged
tobiaszheller merged 1 commit intomasterfrom
tobiaszheller/webauthn-fix-missing-icon
Sep 5, 2023
Merged

Fix webauthnwin c types size#31407
tobiaszheller merged 1 commit intomasterfrom
tobiaszheller/webauthn-fix-missing-icon

Conversation

@tobiaszheller
Copy link
Copy Markdown
Contributor

@tobiaszheller tobiaszheller commented Sep 4, 2023 •
edited by codingllama
Loading

#29750 removed unused icon fields from webauthwin internal C types. Unfortunately it causes panic because now types have wrong size.

Manual tests on windows machine confirm issue and fix.

Fixes: #31333

Changelog: Fix WebAuthn Windows registration breakage

@github-actions github-actions Bot requested a review from Tener September 4, 2023 13:14
codingllama
codingllama approved these changes Sep 4, 2023
View reviewed changes
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.

Many thanks. I completely missed the structs had to be aligned.

Any ideas on how we could test to avoid future size/align regressions?

Comment thread lib/auth/webauthnwin/ctypes.go Outdated
Comment thread lib/auth/webauthnwin/ctypes.go Outdated
Tener
Tener approved these changes Sep 4, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@Tener Tener left a comment

Choose a reason for hiding this comment

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

Tricky one. Having this tested would be lovely, for sure.

@tobiaszheller
Copy link
Copy Markdown
Contributor Author

tobiaszheller commented Sep 4, 2023

Any ideas on how we could test to avoid future size/align regressions?

Do you have some ideas? I have tried using unsafe.Sizeof(webauthnRPEntityInformation{}) however it won't for example catch change from uint32 to uint8 (due to Go padding and alignment). @Tener @codingllama

zmb3
zmb3 reviewed Sep 4, 2023
View reviewed changes
Comment thread lib/auth/webauthnwin/ctypes.go Outdated
Copy link
Copy Markdown
Collaborator

@zmb3 zmb3 Sep 4, 2023

Choose a reason for hiding this comment

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

Hmm, if the size of the struct matters, shouldn't we be more explicit? A *uint16 can have different sizes on different platforms, no?

Copy link
Copy Markdown
Contributor Author

@tobiaszheller tobiaszheller Sep 4, 2023

Choose a reason for hiding this comment

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

i believe we don't support 32bit tsh at all for windows

@codingllama
Copy link
Copy Markdown
Contributor

codingllama commented Sep 4, 2023

Manually verified the fix as well, looking good. FYI, I'm happy to merge this and do whatever testing/refinements we see fit as a follow up.

@tobiaszheller
Copy link
Copy Markdown
Contributor Author

tobiaszheller commented Sep 5, 2023

Manually verified the fix as well, looking good. FYI, I'm happy to merge this and do whatever testing/refinements we see fit as a follow up.

Yep, let's merge it.

@tobiaszheller tobiaszheller force-pushed the tobiaszheller/webauthn-fix-missing-icon branch from d3046b5 to ea2126e Compare September 5, 2023 06:45
@tobiaszheller tobiaszheller force-pushed the tobiaszheller/webauthn-fix-missing-icon branch from ea2126e to 8384d81 Compare September 5, 2023 08:03
@tobiaszheller tobiaszheller added this pull request to the merge queue Sep 5, 2023
Merged via the queue into master with commit 4c1c8e5 Sep 5, 2023
@tobiaszheller tobiaszheller deleted the tobiaszheller/webauthn-fix-missing-icon branch September 5, 2023 09:03
@public-teleport-github-review-bot
Copy link
Copy Markdown

public-teleport-github-review-bot Bot commented Sep 5, 2023

@tobiaszheller See the table below for backport results.

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

This was referenced Sep 5, 2023
Merged
Merged
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zmb3 zmb3 zmb3 left review comments

@Tener Tener Tener approved these changes

@codingllama codingllama codingllama approved these changes

Assignees

No one assigned

Labels

merge-for-v14 size/sm

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

tsh mfa add --type=WEBAUTHN panics on Windows

4 participants

@tobiaszheller @codingllama @Tener @zmb3