Store uploader token in xattr so uploader can delete

[jdek]

This may decrease performance by requiring an xattr lookup, but should be isolated to DELETEs. A configuration option for this feature would be added such that a user has to manually enable it, then on startup the service would check if the target directory has xattr support and error out if it was requested but there is no support.

On DELETE, first the global delete tokens would be checked, if they all fail then xattr would be checked. I think the impact for abuse here is less than just fetching the file itself.

Let me know what you think of this feature, I'd be happy to implement it.

[orhun]

Hey! 👋🏼

The idea sounds very interesting, but I have never used xattr myself before. My questions are:

• How much of a performance degradation are we talking about here? (+ is it really worth it?)
• Is there Rust crates already for handling this?

[jdek]

I think it's worth it, the performance impact really depends on the system you're running it on but this method should be many times faster than a database lookup in any case, on my machine it's 800ns slower to read xattrs in addition to reading the file. It's important to keep in mind that a write will only apply on a successful upload and a read only on a potential delete.

Rust is new to me, so I would just be learning it as I go. I found this crate for xattrs which at a cursory look seems okay. No reason this couldn't be changed or improved later though. The only thing which needs to be decided beforehand is the xattr's name and the format for the token, we could decide to save a hash of the token instead if you think it that would be better (though I would want to make this optional, since for me, anyone who has read access to the directory itself also has a token anyway).

[orhun]

Sounds reasonable. We can move forward with the implementation if you'd like, I can provide more opinions once I see how it works!

[tessus]

I really like the idea. However, this means rustypaste can't be run on Windows anymore. Unless Windows supports extended attributes. TBH, I don't care, because I haven't touched Windows since Windows 3.11, but there might be people who are using rustypaste on Win. On the other hand, if you guard this feature behind an OS flag, all should be fine.

[tessus]

@jdek are you looking into this? You can start a PR and I can help you if you get stuck, although I am rather new to Rust too. But orhun is an expert Rust coder in case I get stuck too. ;-)

Just pick a name for the xattr that makes sense e.g. creator_token. We can always change that later.

[jdek]

@tessus has it been two weeks already... I've been a bit busy, I'll get a PR up this week.

[jdek]

I've had a quick look, and have a couple ideas:

• 1. Switch from role-based authentication to a more fine-grained permission system by:
  2. Adding Token struct

     pub struct Token {
     pub token: Option<String>,
     pub token_type: TokenType,
     }

  3. Extending extract_tokens() to use this type & use extensions_mut (or add_data_container?) to attach the token to the request. On a delete route if we have a valid insert token, we auto convert this to a delete token and check the xattr inside the deletion function itself.
• Or I just extract the token from the request inside the insert/delete routes themselves. This would probably be simpler and leave the implementation of how to make more generic token handling to someone who knows more than zero Rust.

In both of these the case for turning off deletes if there are no delete tokens would be removed, as long as there is one insert token the route should be active. I think for deletes the authentication has to be fully moved into the delete handler itself since we cannot know if it's valid until the file is checked. More generic token handling should probably be preferred since another feature I was interested in was the possibility a token being able to access the list endpoint when its globally disabled to list owned files.

[tessus]

I have to think about this.

We might not want that people are automatically allowed to delete the files they uploaded. Additionally an admin would still want to be able to remove other people's files.

I think we need an option that enables this feature: e.g. allow_uploader_to_delete = true|false

If that option is set, the deletion function checks wether the current token is the one stored in the xattr. Does that make sense? Or am I totally off? Maybe orhun also has some input. I am not the owner, so he might want to decide on the design.

[jdek]

My thought wasn't about whether it should be allowed in general (besides, from my understanding, GDPR requires you to provide the user with the ability to delete their own content). I'm more wondering about the mechanism by which the token goes from the header into the main logic. In any case I think expanding the tokens to a more generic permission system would be beneficial since it would allow you to do things such as:

• set a token to only allow oneshot uploads
• set filesize or expiry limits for a token
• let a token list its own files when global list is disabled The things which I'm not sure on are:
  1. What does struct Token look like? something extensible to allow addition of further permissions. If I were to implement this in C I would use something like struct token { char value[32]; uint64_t flags }; and use flags as a bitfield (64 distinct permissions should be more than enough for a paste service).
  2. How to best pass the extracted token to the route (see above with my comment about extensions_mut, I don't know actix so I can't make a good judgement here on best practices)? Also it seems to me that if finer permissions were implemented the actix_web_grants::protect macro would be no longer sufficient and further checks based on the flags would have to be done within the routes themselves.

[tessus]

Let's wait for @orhun's input.

[orhun]

I feel like the discussion here derails a bit from xattr into a better token mechanism. The mindset of this project is to keep everything as simple as possible so let's try not to overcomplicate things 🐻

I would much rather discuss this over a implementation/PoC so feel free to go ahead and hack something together :)

I think we need an option that enables this feature: e.g. allow_uploader_to_delete = true|false

Yes, that sounds reasonable.

[tessus]

@jdek are you still interested in working on this?

[jdek]

@tessus I would love to but am very pressed on time, I can only finish it in the late summer. Apologies. If you want to take it over feel free.

[tessus]

Ok, let's see who gets to it first. ;-)