Add optional secret for authenticating clients #1
Conversation
There was a problem hiding this comment.
Thanks for implementing this! It looks really great and quite cleanly implemented. I just have a couple minor comments.
I feel like passing a secret in this case would just be a very minor layer of authentication, since security is not particularly critical for this server, so passing it via CLI is probably fine + convenient (despite the issues of having passwords as command-line arguments).
However, I do want to have the protocol be correct if we add this feature. And in this case I believe the encryption does not add additional benefits, since an onlooker would still be able to access an encrypted secret that they could replay with the same nonce. I have two differing suggestions for how to adjust, and I'm open to either security model:
- Send the secret in plaintext. I believe this is as secure as the current approach but would be simpler / has less code and dependencies.
- Extend the protocol to have the server challenge with a random nonce first. This is more complex but is actually secure against TCP snooping (in that bystanders cannot learn how to connect to the server).
Also if we are assuming that someone can snoop on TCP connections, then it may be worth sending the authentication during Accept messages as well, otherwise a third party could intercept someone / accept the connection if they see the UUID.
README.md
Outdated
| The client and server can optionally use secrets to protect the server from | ||
| being used by others. The secret is sent only by the client as encrypted base64 | ||
| by using itself as a key with a randomly generated nonce. No additional traffic | ||
| is encrypted; only the secret. |
There was a problem hiding this comment.
What benefit does encrypting the secret provide here? It seems like any client could replay the same encrypted secret + nonce combination, so this doesn't provide any security benefit beyond a plaintext exchange of secrets.
There was a problem hiding this comment.
Absolutely, I forgot to consider replay attacks. I'll add the server challenge to make encryption actually work properly, shouldn't be too hard to do.
src/auth.rs
Outdated
| if sec_bytes.len() > KEY_LEN { | ||
| bail!("secret must be 32 bytes or fewer"); | ||
| } |
There was a problem hiding this comment.
Is it possible to just pass the secret through a SHA-2 digest, as the seed of an PRNG, or some other one-way key generation approach that removes the 32 byte restriction? That might be a slightly nicer interface.
src/client.rs
Outdated
| @@ -26,14 +27,23 @@ pub struct Client { | |||
|
|
|||
| impl Client { | |||
| /// Create a new client. | |||
| pub async fn new(local_port: u16, to: &str, port: u16) -> Result<Self> { | |||
| pub async fn new(local_port: u16, to: &str, port: u16, secret: Option<String>) -> Result<Self> { | |||
There was a problem hiding this comment.
Could be Option<&str> for consistency.
src/server.rs
Outdated
| Some(ClientMessage::Hello((port, secret))) => { | ||
| if let Err(e) = self.authenticate(secret) { | ||
| send_json(&mut stream, ServerMessage::ClientError(e.into())).await?; | ||
| return Ok(()); |
There was a problem hiding this comment.
Could this be given a more descriptive name like Unauthenticated or Invalid rather than ClientError?
src/server.rs
Outdated
| (Some(key), Some(cln_sec)) => { | ||
| if auth::secrets_match(&key, &cln_sec).is_err() { | ||
| warn!("incorrect secret provided"); | ||
| Err("incorrect secret".into()) |
There was a problem hiding this comment.
.into() is a no-op for these Err variants
src/shared.rs
Outdated
| @@ -13,7 +13,7 @@ pub const CONTROL_PORT: u16 = 7835; | |||
| #[derive(Serialize, Deserialize)] | |||
| pub enum ClientMessage { | |||
| /// Initial client message specifying a port to forward. | |||
| Hello(u16), | |||
| Hello((u16, Option<String>)), | |||
There was a problem hiding this comment.
This can just be a standard Hello(u16, Option<String>) variant, doesn't need the nested tuple!
Cargo.toml
Outdated
| @@ -25,3 +25,6 @@ tokio = { version = "1.17.0", features = ["full"] } | |||
| tracing = "0.1.32" | |||
| tracing-subscriber = "0.3.10" | |||
| uuid = { version = "0.8.2", features = ["serde", "v4"] } | |||
| chacha20poly1305 = { version = "0.9.0" } | |||
| rand = { version = "0.8.5" } | |||
| base64 = { version = "0.13.0" } | |||
|
Thank you for the contribution! I've changed the code fairly substantially to adjust the protocol and:
Now it is only 400 lines of code instead of 600 lines of code. |
No description provided.