Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add requirement for sha256 on upload #12

Merged
merged 1 commit into from
May 22, 2024
Merged

Conversation

hzrd149
Copy link
Owner

@hzrd149 hzrd149 commented May 18, 2024

Replace the requirement for the size tag with an x tag containing the sha256 hash of the blob being uploaded

This is much more secure and removes the possibility of an attacker using another users "upload authorization" event with a padded blob

@hzrd149 hzrd149 added the bug Something isn't working label May 18, 2024
@hzrd149 hzrd149 self-assigned this May 18, 2024
@Giszmo
Copy link

Giszmo commented May 18, 2024

The hash also allows you to skip the upload altogether if the file is already known. On the other hand, why drop the size? You might have authorization to upload files of 1MB but not of 2MB, so by advertising the size, the server could reject it at this point?

Why does the content contain the verb "upload" or "delete"? Wouldn't it be cleaner to contain the filename, only?

@VnUgE
Copy link

VnUgE commented May 18, 2024

I think it's fair to assume the HTTP request would have content-length alongside the event headers. The sha provides data integrity so length would just be checked alongside. An http 100-continue could serve this purpose better.

I really like the idea of providing content integrity in the signed message. I was going to open an issue last week but I wrongly assumed you already considered that and had a reason not to.

Although does the the server really need to care about data integrity since we assume all clients verify blob integrity during a download anyway, or do we trust servers to verify integrity?

@franzaps
Copy link

Besides better security, including a hash also has the upside of skipping existing uploads, +1 @Giszmo

If it's fast enough on most devices for average uploads, I'd say yes.

Q: regarding size, were Blossom servers rejecting outsized files based on the auth event declared size or via request headers?

@hzrd149
Copy link
Owner Author

hzrd149 commented May 18, 2024

regarding size, were Blossom servers rejecting outsized files based on the auth event declared size or via request headers?

Yes, at least my initial implementation. although as I mentioned before it was only there as a really simple security measure to make sure the uploaded blob matched the users auth event

Although does the the server really need to care about data integrity since we assume all clients verify blob integrity during a download anyway, or do we trust servers to verify integrity?

The clients can verify the hash on download, but its also good to verify it in the other direction.
It can protect against man in the middle attacks. and also prevents an attacker or malicious server from re-using the users upload auth to upload other blobs (of the same size) to other servers

Why does the content contain the verb "upload" or "delete"? Wouldn't it be cleaner to contain the filename, only?

The content of the auth can be anything and should be a human readable explanation of what the auth event is intended to be used for.
Its mostly for convenience so the user knows what they are signing

@flox1an
Copy link

flox1an commented May 19, 2024

Looks good. https://bouquet.slidestr.net/ is now already adding the x tag (with client-sdk 0.8). Also added an optional check to https://media-server.slidestr.net/

Having the hash before the upload allows us to improve the upload performance as well. Currently we store the file in a temp location, calculate the hash on the server and then upload to a blob storage. This leads to significant delay for larger files. When the hash is known beforehand the file can be directly uploaded to the final location in an optimistic way. We would only need to revert the storage when the hash does not match.

@hzrd149
Copy link
Owner Author

hzrd149 commented May 20, 2024

If there are no objections to this Ill merge it tomorrow

@hzrd149 hzrd149 merged commit 6f782ed into master May 22, 2024
@hzrd149 hzrd149 deleted the add-requirement-for-sha256 branch May 22, 2024 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants