client: move auth and ca on connection level config#420
Merged
Conversation
|
Code coverage summary for 140ec07: ✅ Region coverage 69% passes |
kp-antonio-yang
force-pushed
the
feat/auth-in-endpoint
branch
from
May 11, 2026 09:03
f29a609 to
442ed9d
Compare
kp-antonio-yang
force-pushed
the
feat/mobile-read-conf
branch
from
May 11, 2026 09:22
762bcdd to
bcd8d45
Compare
kp-antonio-yang
force-pushed
the
feat/auth-in-endpoint
branch
6 times, most recently
from
May 12, 2026 02:45
9a59a5f to
d3dddf9
Compare
kp-antonio-yang
marked this pull request as ready for review
May 12, 2026 02:57
kp-antonio-yang
force-pushed
the
feat/mobile-read-conf
branch
from
May 12, 2026 09:56
bcd8d45 to
975f7bb
Compare
12 tasks
kp-antonio-yang
force-pushed
the
feat/mobile-read-conf
branch
from
May 12, 2026 10:01
975f7bb to
c6a6a9f
Compare
kp-antonio-yang
force-pushed
the
feat/auth-in-endpoint
branch
from
May 12, 2026 11:35
d3dddf9 to
d8d07db
Compare
kp-antonio-yang
force-pushed
the
feat/mobile-read-conf
branch
3 times, most recently
from
May 20, 2026 03:15
a457946 to
1abe436
Compare
kp-antonio-yang
marked this pull request as draft
May 20, 2026 05:01
kp-antonio-yang
force-pushed
the
feat/auth-in-endpoint
branch
2 times, most recently
from
May 20, 2026 05:38
c4ba9b4 to
745c99b
Compare
kp-antonio-yang
force-pushed
the
feat/auth-in-endpoint
branch
2 times, most recently
from
May 25, 2026 06:30
65a0822 to
5f6e2b4
Compare
kp-antonio-yang
marked this pull request as ready for review
May 25, 2026 07:03
Comment on lines
+344
to
+353
| let ca_content = if check_cert_header(&self.ca_cert) { | ||
| Some(self.ca_cert.clone()) | ||
| } else { | ||
| // NOTE: we support a path input on desktop | ||
| if cfg!(desktop) { | ||
| std::fs::read_to_string(&self.ca_cert).ok() | ||
| } else { | ||
| None | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
This logic is repeated in a different way below. I think we can keep the load_ca(cert_or_path) -> Result<cert> and put the logic in there
Contributor
Author
There was a problem hiding this comment.
There is a little different between these, so we can not make a load_ca for the Config and also for the ConnectionConfig.
We accept the global config without CA, so the std::fs::read_to_string(&self.ca_cert).ok() and for the connection level the error should raise *ca_cert = std::fs::read_to_string(&mut *ca_cert).map_err(|_| Error::CaFileNotFound)?
Contributor
Author
There was a problem hiding this comment.
More details note added to point out the difference.
kp-antonio-yang
force-pushed
the
feat/auth-in-endpoint
branch
from
May 25, 2026 07:54
5f6e2b4 to
06eba37
Compare
It is possible to have different Auth and CA with servers, we put these in the ConnectionConfig level, also provid a `take_servers` method to load the ca content and normalize it.
The auth and ca are used in connection config level, so the auth and ca functions in config level are removed.
- doc the methods of Config to know the servers, `.len()`, `.is_empty()` and `.take_servers()` - doc normalize flow and auth/cert in the connection level with server and also normalized by `.take_servers()`
kp-antonio-yang
force-pushed
the
feat/auth-in-endpoint
branch
from
May 25, 2026 08:43
06eba37 to
40b6dca
Compare
kp-weiwen-goh
approved these changes
May 26, 2026
Contributor
Author
|
Thanks for reviews. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Description
ClientConnectionConfigConfig::take_serversto normalizeConnectionConfigwith auth and cert informationauthandroot_ca_certofClientConfigMotivation and Context
Currently, auth and ca settings are in global config level in desktop clients, but the mobile client used in connection level. The input of ca_cert can be a content or a path of ca cert file in desktop clients.
It is possible to have different auth and ca with multiple servers in desktop, so we move these in
ConnectionConfig, also provide atake_serversmethod and keepserversprivate to load the ca content and normalize it.This PR is split from #391(the pink red arrow part of overall flow chart), and now it is documented under /doc/connection_setting.md.
How Has This Been Tested?
Follow current test case
Types of changes
Checklist:
main