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.

Sign up for GitHub

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

Jump to bottom

[v9] Disallow malformed U2F facets #12208

Merged
merged 8 commits into from
May 3, 2022
Merged

[v9] Disallow malformed U2F facets #12208

merged 8 commits into from
May 3, 2022

Conversation

xacrimon
Copy link
Contributor

U2F has a specified format for facets and an algorithm to determine if they are valid or not. During an audit it was found that no place in our stack does proper facet validation for configuration. This PR hardens Teleport by rejecting invalid facets during validation of the AuthPreferenceV2 resource.

@github-actions github-actions bot requested review from jakule and rosstimothy April 25, 2022 14:50
zmb3
zmb3 reviewed Apr 25, 2022
View reviewed changes
Copy link
Collaborator

@zmb3 zmb3 left a comment

Choose a reason for hiding this comment

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

Do we even need this code if we remove the last bits of U2F support?

(You'd still probably want this fix in v8/v9, but maybe in master we fix it by just removing what remains of U2F.

api/types/authentication.go Outdated Show resolved Hide resolved
jakule
jakule reviewed Apr 25, 2022
View reviewed changes
api/types/authentication_authpreference_test.go Outdated Show resolved Hide resolved
api/types/authentication_authpreference_test.go Show resolved Hide resolved
api/types/authentication.go Outdated Show resolved Hide resolved
api/types/authentication.go Show resolved Hide resolved
@xacrimon
Copy link
Contributor Author

xacrimon commented Apr 25, 2022 •
edited
Loading

Do we even need this code if we remove the last bits of U2F support?

(You'd still probably want this fix in v8/v9, but maybe in master we fix it by just removing what remains of U2F.

@zmb3 Unfortunately we do need this code now. Sunsetting U2F fully is scheduled for v11 due to compatibility reasons. So we still need to push this to master for v10.

@xacrimon xacrimon requested review from zmb3 and jakule April 25, 2022 16:09
@codingllama
Copy link
Contributor

codingllama commented Apr 25, 2022 •
edited
Loading

Thanks for taking care of this issue, @xacrimon.

Do we even need this code if we remove the last bits of U2F support?
(You'd still probably want this fix in v8/v9, but maybe in master we fix it by just removing what remains of U2F.

@zmb3 Unfortunately we do need this code now. Sunsetting U2F fully is scheduled for v11 due to compatibility reasons. So we still need to push this to master for v10.

Hey folks. I've kept the U2F configuration knobs around for v10 so we won't break teleport.yaml/cluster_auth_preference on updates, but the U2F subsystem never truly kicks in during v10, instead we bump it to Webauthn and issue a warning. Facets don't have an effect in master/v10+.

I suggest we do this directly on branch/v9 and backport from there.

References:

@xacrimon xacrimon changed the base branch from master to branch/v9 April 25, 2022 17:58
@xacrimon
Copy link
Contributor Author

@zmb3 @codingllama Thanks for the explanation! I've changed the base of this branch to branch/v9 and cherrypicked back the previous changes so that this will merge correctly to v9 now. Then I can proceed to backport to v8 etc.

@xacrimon xacrimon changed the title Disallow malformed U2F facets [v9] Disallow malformed U2F facets Apr 25, 2022
zmb3
zmb3 reviewed Apr 25, 2022
View reviewed changes
api/types/authentication.go Outdated Show resolved Hide resolved
zmb3
zmb3 reviewed Apr 25, 2022
View reviewed changes
api/types/authentication.go Outdated
// NOTE: We allow naked AppID's without a scheme for compatibility with WebAuthn
// as we rely on that behaviour to derive WebAuthn configs on-the-fly.
// This is only valid if all facets are also naked however.
appIDUrl, err := url.Parse(u.AppID)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Acronyms are typically all caps in Go identifiers. How about appIDURL or just u?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to appIDURL.

xacrimon and others added 2 commits April 26, 2022 13:23
@xacrimon @zmb3
Update api/types/authentication.go
635234e 
Co-authored-by: Zac Bergquist <zmb3@users.noreply.github.com>
@xacrimon
update name
f36a701
@xacrimon xacrimon requested a review from zmb3 April 26, 2022 11:25
@xacrimon xacrimon enabled auto-merge (squash) April 29, 2022 15:55
@xacrimon xacrimon merged commit 59fcffa into branch/v9 May 3, 2022
This was referenced May 19, 2022
Merged
Merged
codingllama added a commit that referenced this pull request May 23, 2022
@codingllama
Remove non-https facets from documentation (#12776) (#12785)
3ca1a2c 
Non-https facets aren't allowed since #12208.

* Remove non-https facets from documentation
* Add localhost non-https facets as an example
@webvictim webvictim mentioned this pull request Jun 8, 2022
Closed
@zmb3 zmb3 deleted the joel/TEL-Q122-8 branch April 26, 2023 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zmb3 zmb3 zmb3 approved these changes

@jakule jakule jakule approved these changes

@rosstimothy rosstimothy rosstimothy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@xacrimon @codingllama @zmb3 @jakule @rosstimothy