return/branca

Authenticated and encrypted API tokens written in Rust. A secure JWT alternative.

branca's Issues

Let builder be created by generating random key

In extension to #29, perhaps it would be beneficial to add a method to the builder, that didn't require a key to be passed, but generated it randomly instead.

What do you think @return?

branca decode does not handle binary data

1. Take another branca implementation (eg. pybranca)
2. Encode binary data

f = Branca(fromstring(key))
ct = f.encode(b"\x80")

3. Pass ct do decode in this implementation

-> it will return b'\xef\xbf\xbd'

The spec allows binary data:

..... serialized by MessagePack or Protocol Buffers. from branca.io

Add error for an overflowing operation on TTL and timestamp

I think it would be best to return an error, instead of panic, when ttl + timestamp would overflow. Otherwise, someone might construct tokens with the timestamp maximum value to have the validating side panic on each parsing (if validating side sets non-zero ttl).

Modify README and doc examples to use randomly generated keys

Re. discussion in tuupola/branca-spec#37, I believe it would be a good idea to modify the current examples.

Instead of using a static string for this, I suggest they simply show keys with random generation.

Branca builder incorrectly sets timestamp to non-zero on new()

The Branca builder sets the timestamp to the current system time when calling new():

branca/src/lib.rs

Line 201 in 94725c9

let timestamp = SystemTime::now()

But encode() then checks if it is non-zero. If it is zero, it uses the current system time to encode the token. If it is not zero, it uses the timestamp field of the Branca struct:

branca/src/lib.rs

Line 278 in 94725c9

if timestamp == 0 {

This means, if an instance of Branca is created at one point, and some given time later is used to encode a token, the token would use the timestamp of when the instance was created, not when the token was created.

Security issue: Panic on invalid base62-encoded tokens

Documentation for decode(), which also implicitly covers Branca::decode(), states that:

If the input is not in Base62 format, it returns a BrancaError::InvalidBase62Token Result.

Prior to v0.10.0 this was not the case, instead a panic would occur:

branca/src/lib.rs

Line 403 in 2fd4908

let decoded_data = b62_decode(BASE62, &data).expect("Base62 token is invalid.");

This could leave any validating instance vulnerable to potential DoS, when parsing untrusted data and unexpected panics could occur.

This behavior was corrected in 7da3274:

branca/src/lib.rs

Line 427 in 289cf60

let decoded_data = match b62_decode(BASE62, data) {

Update orion to 0.16

Orion was recently updated to 0.16.0. I ran a quick test with the updated dependency, where all test pass locally. It will however increase MSRV to 1.51.

Branca TTL field is never used

The ttl field of the Branca struct is never used. Instead, decode() takes a separate TTL. Maybe the ttl field should be removed?

to_string() overflow

Hi, I use actix_web to write an api, when I try to call the to_string() method of an branca::errors::Error my worker get stack overflow.

What I have try :

#[derive(Debug, Display)]
pub enum ApiError {
    InternalError(String),
}

#[derive(Debug, Deserialize, Serialize)]
pub struct ErrorResponse {
    errors: Vec<String>,
}

impl ResponseError for ApiError {
    fn error_response(&self) -> HttpResponse {
        match self {
            _ => HttpResponse::InternalServerError().json::<ErrorResponse>(self.into())
        }
    }
}

impl From<&String> for ErrorResponse {
    fn from(error: &String) -> Self {
        ErrorResponse {
            errors: vec![error.into()],
        }
    }
}

impl From<Vec<String>> for ErrorResponse {
    fn from(errors: Vec<String>) -> Self {
        ErrorResponse { errors }
    }
}

impl From<&ApiError> for ErrorResponse {
    fn from(error: &ApiError) -> Self {
        ErrorResponse {
            errors: vec![error.to_string()],
        }
    }
}

impl From<DieselError> for ApiError {
    fn from(error: DieselError) -> ApiError {
        ApiError::InternalError(error.to_string())
    }
}

impl From<BrancaError> for ApiError {
    fn from(error: BrancaError) -> ApiError {
        ApiError::InternalError(error.to_string())
    }
}

If I use description it works :

impl From<BrancaError> for ApiError {
    fn from(error: BrancaError) -> ApiError {
        ApiError::InternalError(error.description().to_string())
    }
}

It's an error on my side ? I'm new to rust, still learning.
See my error :

[2020-11-12T21:59:57Z INFO  actix_server::builder] Starting "actix-web-service-127.0.0.1:8080" service on 127.0.0.1:8080
thread 'actix-rt:worker:0' has overflowed its stack
fatal runtime error: stack overflow
[1]    1143042 abort (core dumped)  cargo run --release

Edit :
If I change the implementation of display error I manage to get it working, I don't know if it's a good way to do that but it's working for me :

impl fmt::Display for Error {
    fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
        match *self {
            Error::InvalidBase62Token => write!(fmt, "Base62 token is invalid."),
            Error::InvalidTokenVersion => write!(fmt, "Token version is invalid."),
            Error::BadNonceLength => write!(fmt, "Bad nonce length."),
            Error::BadKeyLength => write!(fmt, "Bad key length."),
            Error::ExpiredToken => write!(fmt, "This token has expired."),
            Error::DecryptFailed => write!(fmt, "Decryption failed."),
            Error::EncryptFailed => write!(fmt, "Encryption failed."),
        }
         // write!(fmt, "{}", self.to_string()) <== this call to self.to_string() seems to throw an stackoverflow
    }
}

I'm not sure of how to do a clean disply error impl but I have see something similar in the chrono crate :
https://github.com/chronotope/chrono/blob/main/src/format/mod.rs (line 358) :

impl fmt::Display for ParseError {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        match self.0 {
            ParseErrorKind::OutOfRange => write!(f, "input is out of range"),
            ParseErrorKind::Impossible => write!(f, "no possible date and time matching input"),
            ParseErrorKind::NotEnough => write!(f, "input is not enough for unique date and time"),
            ParseErrorKind::Invalid => write!(f, "input contains invalid characters"),
            ParseErrorKind::TooShort => write!(f, "premature end of input"),
            ParseErrorKind::TooLong => write!(f, "trailing input"),
            ParseErrorKind::BadFormat => write!(f, "bad or unsupported format string"),
        }
    }
}

Enable Dependabot to keep dependencies up to date

Dependabot can be set up to check for outdated dependencies, or dependencies with reported security issues (I think I was wrong on this on. For Dependabot with Rust, we'd still need #18 for security issues), on a daily basis. It'll then create PRs if any such are detected.

Dependabot is added quite easily to the CI with a dependabot.yml file in the .github folder.

The file should include something like this:

version: 2
updates:
  - package-ecosystem: "cargo"
    directory: "/" # Location of package manifests
    schedule:
      interval: "daily"

@return I tried myself, but it seems only a repository owner is able to set this up.

Impossible to know if expired token is valid or not

Because the token expiration check is performed before authentication and decryption, it's impossible to know if an expired token is valid or invalid.

An attacker may modify the timestamp, in the header, to make the token expire but the user would never know that it had been modified.

Do not consume self on builder getters

key(), nonce(), ttl() and timestamp() all consume self. This means the builders instance needs to be cloned if a user wants to continue using it after calling any of the above getters. This seems unintentional. I suggest they all take &self instead of self.

pub fn nonce(&self) -> &[u8] {
        self.nonce
}

Consider using generic parameters

So encode could take AsRef<[u8]> instead of &[u8]. Originally suggested by @J-F-Liu in #10 (comment)