fix: handle peer names with special chars #2169
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #2169 /- ##
==========================================
Coverage 60.24% 60.26% 0.01%
==========================================
Files 152 152
Lines 30900 30898 -2
==========================================
Hits 18617 18621 4
Misses 12283 12277 -6
... and 8 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
|
Are peer names used anywhere else besides the TLS cert? Only worry with silently replacing the bad characters with _ is if the user actually needs to use the peer name somewhere else, but now it's not what they expect. |
| @@ -185,7 185,7 @@ pub async fn run_dkg(root_task_group: &TaskGroup, servers: usize) -> anyhow::Res | |||
| async fn create_tls(id: usize, sender: Sender<String>) -> anyhow::Result<()> { | |||
| // set env vars | |||
| let bin_dir = env::var("FM_BIN_DIR")?; | |||
| let server_name = format!("Server-{id}"); | |||
| let server_name = format!("Server {id}!"); | |||
There was a problem hiding this comment.
I wonder if we should just
info!(peer-id=peer_id, out-dir=out_dir", "creating TLS certs created for peer in {out_dir}");
kind of thing, instead of interpolating strings for logging.
There was a problem hiding this comment.
Isn't this being used as a cmd line arg, not logging?
There was a problem hiding this comment.
I don't understand that nit either, looks good to me, just test code running the dkg.
They are only replaced when used as a cert, otherwise they will be kept original everywhere else. We just use the peer name in the TLS cert arbitrarily, it could be anything. |
@elsirion Certs are pinned to the P2P endpoint in config gen, shouldn't that be enough? |
Fixes #2074
@SumantxD took a shot at fixing it, using your string replacement code