Skip to content
This repository has been archived by the owner on Jan 17, 2020. It is now read-only.

Panic with invalid key file #176

Open
david-mcgillicuddy-moixa opened this issue Oct 2, 2019 · 4 comments · May be fixed by #177
Open

Panic with invalid key file #176

david-mcgillicuddy-moixa opened this issue Oct 2, 2019 · 4 comments · May be fixed by #177

Comments

@david-mcgillicuddy-moixa
Copy link

david-mcgillicuddy-moixa commented Oct 2, 2019

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
Copy link

tekjar commented Oct 3, 2019

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
Copy link
Author

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
Copy link
Contributor

TheBestJohn commented Oct 3, 2019

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
Copy link

andresv commented Jan 16, 2020

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants