2
\$\begingroup\$

I am new to Rust. I am making this to help me learn better. This code has two functions: ::make() and ::get_payload(). ::make() creates a signed web token to be stored on the client. The client sends that token on every request that requires authentication, and the clients information or 'claims' are verified by the signature with ::get_payload(). If ::get_payload() returns Value::Null the token is not valid (or expired).

I need 3 types of feedback.

  1. Are there any security holes or issues with what you can see here?
  2. Is this Rust written well, and how can I improve it?
  3. Am I missing some important error handling?

extern crate base64;
extern crate ring;
extern crate chrono;
extern crate serde_json;

use base64::{encode, decode};
use ring::{digest, hmac};
use chrono::prelude::Utc;
use std::str;
use serde_json::Value;

static RWT_SECRET: &str = "some secret"; // this should be from an environment variable

pub fn make(payload: &str) -> String {
    let mut token = encode(&Utc::now().to_string());
    token.push_str(".");

    let payload_base64 = &encode(payload);
    token.push_str(payload_base64);

    let key = hmac::SigningKey::new(&digest::SHA256, RWT_SECRET.as_bytes());
    let signature = &encode(&hmac::sign(&key, token.as_bytes()));
    token.push_str(".");
    token.push_str(signature);

    token
}

pub fn get_payload(token: &str) -> Value {
    let parts: Vec<&str> = token.split(".").collect();
    if parts.len() != 3 {
        return Value::Null;
    }

    // todo: check timestamp for expiration

    let signed = parts[..2].join(".").to_string();
    let key = hmac::SigningKey::new(&digest::SHA256, RWT_SECRET.as_bytes());
    let signature = &encode(&hmac::sign(&key, signed.as_bytes()));

    if signature.to_string() == parts[2] {
        let utf8 = &decode(parts[1]).unwrap();

        let json = str::from_utf8(utf8).unwrap();
        let payload: Value = serde_json::from_str(json).unwrap();

        return payload;
    }

    Value::Null
}
\$\endgroup\$
  • \$\begingroup\$ Using nom may be a useful alternative approach. \$\endgroup\$ – CAD97 Sep 7 '17 at 23:36
3
\$\begingroup\$
  1. Run clippy to be automatically told about things like:

    warning: single-character string constant used as pattern
      --> src/main.rs:30:40
       |
    30 |     let parts: Vec<&str> = token.split(".").collect();
       |                            ------------^^^- help: try using a char instead: `token.split('.')`
       |
       = note: #[warn(single_char_pattern)] on by default
       = help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#single_char_pattern
    
    warning: this creates an owned instance just for comparison
      --> src/main.rs:41:8
       |
    41 |     if signature.to_string() == parts[2] {
       |        ^^^^^^^^^^^^^^^^^^^^^ help: try `signature`
       |
       = note: #[warn(cmp_owned)] on by default
       = help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#cmp_owned
    
  2. Tests, tests, tests, TESTS! Who even knows if this code works? Who knows if the changes I made broken anything? I sure don't know, but all the provided tests passed!

  3. Encoding and decoding have highly related parts, so I'd introduce a struct to bundle them together; this allows extracting the key and signing bits which are reused across both. It also makes it easier to start to use an environment variable.

  4. There's a lot of allocation that doesn't seem needed. For example, you can Base-64 encode into an existing String. Splitting the string on . and then joining it back together when verifying the token also seems inefficient.

  5. Encoding into a string introduces some duplication with the details around the encoding, so I extracted that to a new type as well.

  6. I chose to take some generics to avoid having to call as_bytes and friends everywhere.

  7. Construct the key once when the object is created, then you can reuse it. This also makes it easier to change how the key is constructed.

  8. When collecting, you usually don't need to specify the type inside the collection. Instead, do Vec<_>.

  9. Returning Value::Null conflates an error case with a valid value. Use Result instead.

  10. The API is unbalanced; encoding takes a string, but decoding returns a JSON Value. These should be symmetrical.

  11. The practice of putting & before a result value (let foo = &some_function();) is not usual and it's very distracting. Usually, you'd just take the reference when you need it.

  12. Libraries should not unwrap things unless it's the author of the libraries mistake. Panicking in a library should be very rare and highly documented. Usually, you should return an error. If you do panic, use expect to give yourself a hint what went wrong.

  13. Since you have a Vec<u8>, you can convert it to a String, no need to make a &str. Even better though, you can parse JSON from a slice, no need to explicitly check for UTF-8.

  14. There's no need to introduce a temporary variable for the result of serde_json::from_str, just return it directly.

extern crate base64;
extern crate ring;
extern crate chrono;
extern crate serde_json;

use base64::{encode, decode};
use ring::{digest, hmac};
use chrono::prelude::Utc;
use std::str;
use serde_json::Value;

static RWT_SECRET: &str = "some secret"; // this should be from an environment variable

#[derive(Debug, Default)]
struct Encoder {
    buffer: String,
}

impl Encoder {
    fn new() -> Self {
        Self::default()
    }

    fn encode<D>(&mut self, data: D)
    where
        D: AsRef<[u8]>,
    {
        base64::encode_config_buf(data.as_ref(), base64::STANDARD, &mut self.buffer);
    }

    fn finish_part(&mut self) {
        self.buffer.push_str(".");
    }
}

impl AsRef<[u8]> for Encoder {
    fn as_ref(&self) -> &[u8] {
        self.buffer.as_bytes()
    }
}

impl From<Encoder> for String {
    fn from(me: Encoder) -> String {
        me.buffer
    }
}

pub struct Thang {
    key: hmac::SigningKey,
}

impl Thang {
    fn new() -> Self {
        Thang { key: hmac::SigningKey::new(&digest::SHA256, RWT_SECRET.as_bytes()) }
    }

    fn sign<D>(&self, data: D) -> hmac::Signature
    where
        D: AsRef<[u8]>,
    {
        hmac::sign(&self.key, data.as_ref())
    }

    fn encoded_signature<D>(&self, data: D) -> String
    where
        D: AsRef<[u8]>,
    {
        encode(&hmac::sign(&self.key, data.as_ref()))
    }

    pub fn make(&self, payload: &str) -> String {
        let mut token = Encoder::new();

        let time = Utc::now().to_string();
        token.encode(time);
        token.finish_part();
        token.encode(payload);

        let signature = self.sign(&token);

        token.finish_part();
        token.encode(signature);

        token.into()
    }

    pub fn get_payload(&self, token: &str) -> Result<Value, Error> {
        let first_dot = token.find('.').ok_or(Error)?;
        let second_dot = first_dot + 1 + token[(first_dot + 1)..].find('.').ok_or(Error)?;

        // let header = &token[..first_dot]; // TODO: check header
        let payload = &token[(first_dot + 1)..second_dot];
        let header_and_payload = &token[..second_dot];
        let actual_signature = &token[(second_dot + 1)..];

        let expected_signature = self.encoded_signature(header_and_payload);

        if expected_signature == actual_signature {
            let payload = decode(payload).map_err(|_| Error)?;
            return serde_json::from_slice(&payload).map_err(|_| Error);
        }

        Err(Error)
    }
}

#[derive(Debug)]
pub struct Error; // TODO: implement std::error::Error, maybe promote to an enum

fn main() {
    let x = Thang::new().make(r#"{"hello": true}"#);
    println!("{}", x);
    let x = Thang::new().get_payload(&x);
    println!("{:?}", x);
}

Are there any security holes or issues with what you can see here?

I'm not qualified to make any such judgements. I will note that you've hardcoded the signature type, which doesn't mesh with real JWT.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.