-
-
Notifications
You must be signed in to change notification settings - Fork 928
fix: pre-initialize mTLS flag from stored alias to handle TLS session resumption in Wear OS onboarding #6663
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
Draft
smhc
wants to merge
1
commit into
home-assistant:main
Choose a base branch
from
smhc:fix/wear-mtls-tls-session-resumption
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+64
−0
Draft
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, if I'm not wrong it means that if I onboard a new server that is not using mTLS but one of my server has mTLS I'm going to be forced to pick a cert for the wear where I might have none. It shouldn't be an issue to give a "fake" cert but still a pretty bad experience.
We could try to store the host information indeed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had assumed the cert picker in the UI was optional even if the cert was considered "mandatory". I must admit I didn't check or I was remembering an old implementation.
It might want it to be optional anyway, in the event the server specifies it is optional. Although I believe at the moment it only presents this screen if it's mandatory (and if you don't hit this bug).
We could alternatively set a flag stating that it is "probably/optionally required" and let the user decide whether to provide one or not.
I understand this will only be the case if the server you are currently logged into in the phone app requires mTLS. Note prior to this change it wouldn't work at all in this scenario if the server you are onboarding the watch to also requires mTLS (most common case). You get a "could not register watch" toast and no further info.
I can add the host matching - but am a bit concerned about matching domain suffixes, http vs https and so on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is your concern?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just meant matching myhainstance vs mhainstance.lan vs http://127.0.01 vs https://localhost etc, it's not a simple strcmp. I guess the certificate CNAME has to match anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have some utils already for that in the repo like
android/common/src/main/kotlin/io/homeassistant/companion/android/util/UrlUtil.kt
Line 107 in 9bc151c
android/common/src/main/kotlin/io/homeassistant/companion/android/util/UrlUtil.kt
Line 186 in 9bc151c