AtherEnergy / rumqtt

Pure rust mqtt cilent
The Unlicense
203 stars 72 forks source link

Panic with invalid key file #176

Open david-mcgillicuddy-moixa opened 4 years ago

david-mcgillicuddy-moixa commented 4 years ago

Hi, when given an invalid client key file, rumqtt panics with 'index out of bounds: the len is 0 but the index is 0'.

The stacktrace (included as an attachment) points to this line: https://github.com/AtherEnergy/rumqtt/blob/master/src/client/network.rs#L124 as the culprit.

The error happens during the code:

    let mqtt_options = MqttOptions::new(
        config.client_id.clone(),
        config.endpoint.clone(),
        config.mqtt_port.unwrap_or(DEFAULT_MQTTS_PORT),
    )
    .set_ca(config.ca.clone())
    .set_client_auth(config.cert.clone(), "dummy_key".to_owned().into_bytes());

    MqttClient::start(mqtt_options)

and if the dummy key bytes are replaced with the real key bytes there is no issue and everything works. I would have expected MqttClient::start (and NetworkStreamBuilder::create_stream) to instead return an error which says something about an invalid key instead of panicking.

EDIT: that unwrap in create_stream doesn't look great to me either, from looking at the source of pemfile::rsa_private_keys, it will return an Err if there's invalid base64 after "-----BEGIN PRIVATE KEY-----" in the file: https://github.com/ctz/rustls/blob/master/rustls/src/pemfile.rs

EDIT 2: I should also say that I'm very happy to put together a PR to fix this.

rumqtt_panic_backtrace.txt

tekjar commented 4 years ago

EDIT 2: I should also say that I'm very happy to put together a PR to fix this.

Please do :). I've been meaning to do an audit on all the unwraps but that never happened

david-mcgillicuddy-moixa commented 4 years ago

I can't promise an audit on all the unwraps but I will certainly do the ones that are causing issue for me in that function

TheBestJohn commented 4 years ago

EDIT 2: I should also say that I'm very happy to put together a PR to fix this.

Please do :). I've been meaning to do an audit on all the unwraps but that never happened

unwraps look few. The only one aside from the cert/ca/key is publish.pkid.unwrap() in mqttstate as well as Runtime::new().unwrap(); and connection_tx.try_send(Ok(())).unwrap(); in client/connection. The rest are in examples

andresv commented 4 years ago

It would be nice if rumqtt could also support pkcs8 private keys: https://github.com/ctz/rustls/blob/master/rustls/src/pemfile.rs#L69.

This caused panic for me.