Signed attachment urls

[rossta]

Adds ability to generate signed attachment urls by configuring `Refile.secret_token' as security against DoS and generation of unauthorized processing.

[jnicklas]

I've been wanting to do something similar, after @joeljunstrom's suggestion. That being said, this PR has a few problems which have to be adressed before we can consider merging this.

• Using query strings is pretty much a no-go, as support for them in CDNs is pretty sketchy, and at least usually is opt-in. The token needs to be embedded into the URL somehow.
• From what I know, using SHA256 for this is not ideal, since it's vulnerable to extension attacks, which would defeat the purpose of this providing DoS protection. It should be using HMACs.
• The string comparison in Security is not constant time, and so could be vulnerable to timing attacks.
• I feel that the entire Security middleware is over-engineered. Why can't this simply be part of the application? It seems like this should be a fairly simple addition there.
• This really should be audited by someone who actually knows what they're doing ;)

[jnicklas]

That being said, thank you for putting this together. This is definitely something we need and should have.

[jnicklas]

I'm also wondering if this should be the default. It makes it hard/impossible to generate URLs client-side, which is annoying, but it's also quite sensible from a security perspective.

[rossta]

Ok, thanks for the feedback. I can take another pass at this with some additional input:

1. Some possibilities for url schemes:
   a. Add a new fragment: more straightforward, not backwards-compatible, probably necessary to update all app specs: e.g. /:token/:store/:id/:filename
   b. Make it an optional part of existing URL structure for backwards compatibility, ability to omit in specs, though less straightforward to perform path comparison: /:store/:id/:filename.?:token?.:extension
2. Changes to mitigate timing attacks could be include using SHA1 digests to reduce the "linear relationship between the correctness of the string and the time the comparison takes" (reference).

expected = Digest::SHA1.hexdigest(request.path)
actual = Digest::SHA1.hexdigest(params[:token])
expected == actual

Additionally the comparison could happen later in the request, perhaps after retrieving the file from store and processing, at the risk of wasting resources.

[jnicklas]

1. I like option (a) better, though it could be trickier to implement.
2. Yes, from what I understand that's a sensible scheme. There's also this, but it appears that it's quite slow.

I think we want something like:

token = OpenSSL::HMAC.hexdigest(OpenSSL::Digest.new("sha1"), secret, request.path)
expected = Digest::SHA1.hexdigest(token)
actual = Digest::SHA1.hexdigest(params[:token])
expected == actual

Or something like that. Seems like Sha1 is good enough.

[rossta]

Here's take 2 with the suggested changes. To take advantage of the security feature, developers would have to configure the secret token explicitly. It may be worth noting in the README the new URL scheme also means cold cache after upgrading for those running Refile behind CDN in production.

[joeljunstrom]

Until we have a clear use case of the need to generate url's client side maybe this should be enforced (
ie. throw an error if a secret isn't set). Even with the warning non secure defaults should probably be avoided.

I realize this breaks existing installations but I think the feature warrants it.

[jnicklas]

I agree with @joeljunstrom.

Also we should document somewhere what the secret_token should look like. IIRC Rails throws an error if it's too short, which is probably a good thing.

[rossta]

OK, now an informative error is raised when the secret is missing (borrowing from Devise). I can add an additional check for length if necessary. I've added README instructions as well.

[jnicklas]

Would it make sense to default the token to Rails' session token? That way we don't have to add a separate step in the README for it, and most people would by default have a very good, very secure value as their token.

[rossta]

Good idea. I could add an engine initializer for that.

[jnicklas]

Sounds great :)

[rossta]

Added an initializer that works against the TestApp and makes some assumptions about the configuration of the Rails session token in other recent versions of Rails. If necessary, I suppose we could introduce a Travis matrix and some flags to test against different Rails versions.

[jnicklas]

Done!

@rossta thank you so much for an excellent PR, and for hanging in there through all feedback. You did a really fantastic job!

[rossta]

Rock on, thanks @jnicklas! 🎸

[janko]

It's unfortunate that clients can't generate URLs now, which nihilates the flexibility of URLs when your backend and frontend are separate. But I see why this has to be a default, it would really suck if someone could DoS your application so easily. If the token wouldn't depend on the path, then the attacker would just need to find out the token from one image URL, and still have the same power.