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

feat(share): make sharelink token length configurable #47265

Merged
merged 1 commit into from
Sep 28, 2024

Conversation

ernolf
Copy link
Contributor

@ernolf ernolf commented Aug 15, 2024

Summary

The share-token length is currently fixed at 15 characters as a constant.
This length allows for $52^{15}$ (54,960,434,128,018,667,122,720,768) possible token variations using CHAR_HUMAN_READABLE:

public const CHAR_HUMAN_READABLE = 'abcdefgijkmnopqrstwxyzABCDEFGHJKLMNPQRSTWXYZ23456789';

This level of variation can be considered Military Grade High Entropy and is often much more than sufficient for many use cases. Reducing the token length requires direct code modifications.

Changes

This Pull Request introduces the ability to configure the token length for the Share API.
Administrators can set the token length dynamically through the database in a range from 6 up to 32:

occ config:app:set --value=8 -- core shareapi_token_length

This example sets the token length to 8 characters.
If no changes are made, the default value of 15 will continue to be used, ensuring backward compatibility.

Even at the minimum length of 6, there are still $52^6$ (19,770,609,664) possible variations.

Security Consideration

Reducing the token length increases the chance of token collisions, as it drastically reduces the number of possible combinations. It’s important to note that when a variable token length is used, this does not automatically mean, that the security decreases, since the total number of possible variations increases significantly. This is because, for shorter tokens, we must sum the possible variations for each length from 6 up to the maximum configured length to get the number of theoretically possible variations.
I have been using a token length of 8 for a long time now, although there are still 53,459,728,531,456 different possible variations. Absolutely sufficient for my use case where music producers share music samples and the shares have a very short ttl..
Shorter token lengths can be very helpful in cases where a lot of work is done with short-lived shares.
The admin is therefore responsible for assessing which security requirements he attaches to the token length. Therefore, this PR makes it possible not only to decrease- but to increase the token length up to a maximum of 32 as well, which corresponds to 8,167,835,760,036,914,488,254,418,108,462,708,901,695,678,621,570,564,096 possible variations, which is absolutely an increase in security.


EDIT:
Added the values:

* "max" for the maximum token length (32) *)
* "min" for the minimum token length (6) *)

  • '0' and any other non-numerical value (like "default") for the default token length (15)

This ensures that the token length is "idiot-proof," defaulting to a safe (the default) value not only when the input is empty but also when any invalid value is provided. This makes it more robust and prevents potential errors.

*) simplified with ee24fdd


EDIT 2:

I had mistakenly assumed A-Za-z0-9 instead of CHAR_HUMAN_READABLE


Todo

Consideration must be given to how this should be documented.

@ernolf ernolf force-pushed the ernolf/configurable_sharetoken_length branch from 4c65ee9 to 4b10b32 Compare August 15, 2024 19:12
@ernolf ernolf requested review from come-nc and kesselb August 15, 2024 19:18
Copy link
Member

@juliushaertl juliushaertl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good to me, but would leave deciding that to the security people 👍

@skjnldsv
Copy link
Member

occ config:app:set --value=8 -- core shareapi_token_length

Also, as a side note, we tend to avoid those kind of config setters. They're very hidden and not very discoverable.
Either they go into the config.php or we create a UI for it :)

@ernolf
Copy link
Contributor Author

ernolf commented Aug 18, 2024

Either they go into the config.php or we create a UI for it :)

I mainly wanted to create this possibility first. I was aware that there would be many objections. For me personally, it would be sufficient if this was handled as a "hidden feature". If it is generally accepted, I would of course be happy to write the integration in the UI within the scope of my skills.

Copy link
Contributor

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

lib/private/Share/Helper.php Outdated Show resolved Hide resolved
lib/private/Share/Helper.php Outdated Show resolved Hide resolved
- ensure unique share token with dynamic length adjustment

Signed-off-by: ernolf <raphael.gradenwitz@googlemail.com>
@nickvergessen nickvergessen force-pushed the ernolf/configurable_sharetoken_length branch from 8145b53 to b4ea146 Compare September 28, 2024 06:06
@nickvergessen nickvergessen merged commit c470ef0 into master Sep 28, 2024
174 checks passed
@nickvergessen nickvergessen deleted the ernolf/configurable_sharetoken_length branch September 28, 2024 06:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make configurable length of shared link token
5 participants