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

Expose Sha256 algorithm to lua api #14259

Closed
wants to merge 1 commit into from

Conversation

chmodsayshello
Copy link
Contributor

@chmodsayshello chmodsayshello commented Jan 14, 2024

This is a pull request allowing the sha225 algorithm that already is in minetest's code to be used by mods, which are currently limited to just sha1. This allows mods to hash a given set of data in a more secure way if needed.

This PR Ready for Review.

How to test

  1. Clone my branch and compile minetest
  2. Call minetest.sha256 as a mod, and log the result
  3. Use an external tool/webpage like this to verify the hash is correct.

@Zughy Zughy added Feature ✨ PRs that add or enhance a feature Roadmap: Needs approval The change is not part of the current roadmap and needs to be approved by coredevs beforehand. @ Script API and removed @ Script API labels Jan 14, 2024
@wsor4035
Copy link
Contributor

just a thought as a modder/thinking of the api, as these options are added, potentially more in the future, it seems this would pollute to a degree the minetest api namespace, maybe it would be better to have something like minetest.encode(method, ...) similar to minetest.compress? with the previous said, of course minetest.sha1 will have to exist for backwards compat, however that could be depreciated and aliased to a new function.

alternatively:
minetest.encode.TYPE

@appgurueu
Copy link
Contributor

just a thought as a modder/thinking of the api, as these options are added, potentially more in the future, it seems this would pollute to a degree the minetest api namespace, maybe it would be better to have something like minetest.encode(method, ...) similar to minetest.compress? with the previous said, of course minetest.sha1 will have to exist for backwards compat, however that could be depreciated and aliased to a new function.

alternatively: minetest.encode.TYPE

The latter is the better suggestion, IMO.

  • It means minetest.hash.this_does_not_exist(...) will result in an attempt to call a nil value (otherwise we'd have to implement erroring on invalid values ourselves).
  • Modders can also check for the existence of a certain method conveniently - just minetest.hash.method (otherwise we'd have to add some feature flags or similar).
  • Editor tooling will also understand minetest.hash.blah better than minetest.hash("blah", ...).
  • It also makes the intent (we're choosing an algorithm here, not passing some string data) clearer, IMO.

(By the way, it should be minetest.hash or similar. These are "secure hashing" algorithms.)

@sfan5
Copy link
Member

sfan5 commented Jan 14, 2024

IMO there's no reason or usecase for sha224 and we shouldn't expose it. If we have leftover code we should rather go and remove it.

@wsor4035
Copy link
Contributor

The latter is the better suggestion, IMO.

  • It means minetest.hash.this_does_not_exist(...) will result in an attempt to call a nil value (otherwise we'd have to implement erroring on invalid values ourselves).
  • Modders can also check for the existence of a certain method conveniently - just minetest.hash.method (otherwise we'd have to add some feature flags or similar).
  • Editor tooling will also understand minetest.hash.blah better than minetest.hash("blah", ...).
  • It also makes the intent (we're choosing an algorithm here, not passing some string data) clearer, IMO.

(By the way, it should be minetest.hash or similar. These are "secure hashing" algorithms.)

while i agree, mainly suggested it that way to keep consistency with minetest.compress

@chmodsayshello
Copy link
Contributor Author

chmodsayshello commented Jan 17, 2024

I'll depend on #14265 as it's likely to be merged before this (that one doesn't add anything, was created by a core dev etc, while this still has no roadmap approval), and that way I'll be able to update the PR for it early...

@sfan5 sfan5 added Concept approved Approved by a core dev: PRs welcomed! Rebase needed The PR needs to be rebased by its author. and removed Roadmap: Needs approval The change is not part of the current roadmap and needs to be approved by coredevs beforehand. labels Jan 17, 2024
@Zughy Zughy added Action / change needed Code still needs changes (PR) / more information requested (Issues) and removed Rebase needed The PR needs to be rebased by its author. labels Jan 19, 2024
@Zughy
Copy link
Member

Zughy commented Jan 19, 2024

@chmodsayshello the PR you mentioned has been merged, feel free to proceed as discussed :)

@Zughy Zughy added Supported by core dev Not on the roadmap, yet some core dev decided to take care of this PR and removed Concept approved Approved by a core dev: PRs welcomed! labels Jan 21, 2024
@chmodsayshello chmodsayshello changed the title Expose Sha224 & Sha256 algorithms to lua api Expose Sha256 algorithm to lua api Jan 27, 2024
const bool hex = !lua_isboolean(L, 2) || !readParam<bool>(L, 2);

std::string data_sha256;
data_sha256.assign((char *) SHA256((const unsigned char*) data, size, NULL), SHA256_DIGEST_LENGTH);
Copy link
Member

Choose a reason for hiding this comment

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

If md is NULL, the digest is placed
in a static array. Note: setting md to NULL is not thread safe.

@@ -130,7 +133,6 @@ class ModApiUtil : public ModApiBase

// urlencode(value)
static int l_urlencode(lua_State *L);

Copy link
Member

Choose a reason for hiding this comment

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

please keep this space

@Zughy Zughy added the Adoption needed The pull request needs someone to adopt it. Adoption welcomed! label Feb 21, 2024
@Zughy Zughy closed this Feb 21, 2024
@Zughy Zughy removed the Adoption needed The pull request needs someone to adopt it. Adoption welcomed! label Feb 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Action / change needed Code still needs changes (PR) / more information requested (Issues) Feature ✨ PRs that add or enhance a feature @ Script API Supported by core dev Not on the roadmap, yet some core dev decided to take care of this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants