Slight odd post, let me know if this isn't appropriate.
I wrote a small rust web project as a replacement of imgix at my work (context: imgix is not appropriate for our use case and costs us too much)
I am trying to introduce rust at my work but i have never written any rust. I sat down and took 4 hours to write this project without ever using rust before or having any prior knowledge of rust.
So i am not sure if this is idiomatic rust. I come from a background of OCaml, Haskell, Scala, Clojure and JS.
here is the project GitHub - shrynx/vulpix: An image processing service.
It's overall less than 200 lines (including imports and white space)
Not looking for a thorough review but just a quick glance if it makes sense.
PS: Honestly using rust for the first time, even without knowing anything was such a breathe of fresh air.
Very well designed language, amazing tooling, ide integration just works, cargo is dead simple build tool and the super friendly compiler error messages. i feel i am already turning into a fanboy
The main file is pretty clean; the only thing I don't understand is the Once/call_once mechanism. You don't reference that value anywhere else, so you could literally just call magick_wand_genesis() and not use Once at all.
In image_processing.rs, there are a couple of red flags:
&String parameters. Passing owned containers that have a borrowed counterpart (e.g. String or Vec) by reference is harmful. A &String isn't any more useful than a &str (since you can't mutate it!), and a &String can always be converted to a &str but the converse is not true. Hence, accepting a &String forces the caller to allocate a new String if they don't already have one, even if they had a type that was convertible to a &str. (This useless allocation is exactly what you are forced to do in test_validate_signature().)
This:
format!("image/{}", format.to_string())
should just be
format!("image/{}", format)
It's unnecessary to allocate a string just to splice it into a format string; chances are if something is ToString, it's already Display so you can just format it directly.
"".to_string() is more simply String::new().
You shouldn't use raw string replacement for URL manipulation (specifically, I mean the code in validate_signature()). Use a proper URL parser/formatter library.
If there is no secret key and no expiration date, then validation succeeds. That's probably a huge security hole and not what was intended.
There are a couple really weird pieces of code, e.g. if &format!("{:x}", md5_digest) == encrypted_key – you could just compare the formatted String directly with the key.
In errors.rs:
the get_error_message() function is doubly non-idiomatic. First, in Rust, one doesn't write "getters". get_xyz() shouldn't be used as a method name unless you have a very good reason. Second, this method should just be the ToString::to_string() impl, which is transitively implemented if your type is Display, which it should be. Use the thiserror library to remove From and Display/Error boilerplate from your error types.
In params.rs: the enum variants should have CamelCase names, not ALLCAPS. It should be Png and Jpeg, not PNG and JPEG.
There's a function called validate_signature that takes an encryption_key (instead of a verifying_key) and uses md5...? Maybe you meant to use a Message Authentication Code (MAC) like HMAC-SHA256? Cryptographically, this looks at the very least extremely jumbled up, and in the worst case horribly broken. I think what you call:
secret_salt is the mac_key
encrypted_key is the received_mac
md5_digest should be a recomputed_mac
Also you're using == to compare the received MAC with the re-computed one, yielding a timing side-channel, which is like the 101 of CTF challenges.
Maybe you should pay a cryptographer to review this custom authentication scheme or use something well established instead.
Also it seems like it's parsing parameters from the query string (in validate_signature), when axum could be doing that for you, no?
Thank you for such a comprehensive review, much appreciated.
I don't understand is the Once /call_once mechanism
yeah i don't either, directly taken from magick-rust but yeah i feel that can be removed as the example wraps it in a function.
You shouldn't use raw string replacement for URL manipulation.
i use extracted query params for rest of stuff but need the query string to create md5 hash (with the salt). That's how imgix works, and i want to keep it compatible so at work i can use it as a drop in replacement (at least initially).
Their api needs query param s to be md5 of secret_salt/image_path?foo=1&bar=2
It's pretty shit and not that secure honestly. after first compatible version i am going to change it.
This is their doc Securing Assets | imgix Documentation if your curious
if &format!("{:x}", md5_digest) == encrypted_key – you could just compare the formatted String directly with the key.
The md5 returns a Digest type, which i couldn't compare to String, i'll give another look.
If there is no secret key and no expiration date, then validation succeeds. That's probably a huge security hole and not what was intended.
yeah those will be non optional params when productionised. i just made them optional so i could test other features without calculation hash every time i change width or something
That's a good suggestion. i mentioned in previous comment that the current version is to maintain compatibility with imgix which is sadly not that secure.
The next version i could definitely use this suggestion. thanks !
That's not what I meant. I wrote "compare the formatted String directly" – that sounds pretty clear to me, remove the leading &, there's no need to take a reference.
That's fine, just don't do it using .replace(format!("?q="), ""). That's going to break as soon as the externally-supplied parameter itself contains ?q= or something equally maliciously-crafted.
