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

Implement support for PCK signing. #87696

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bruvzg
Copy link
Member

@bruvzg bruvzg commented Jan 29, 2024

  • Adds support for PCK signing. Works in a similar manner to the PCK encryption:
    • SHA256 file hashes added to the PCK directory structure.
    • "require validation" file flags added to the PCK directory structure (hashes of the files w/o flag are not validated to save loading time).
    • PCK directory is signed using provided keypair.
    • If enabled (executable compiled with the public key), it will refuse to load unsigned PCKs.
  • Adds buttons to generate keys from the exporter UI.
  • Improve PCK encryption and signing filters handling to automatically include imported files and remaps.
Screenshot 2024-01-29 at 07 53 33 Screenshot 2024-01-29 at 07 54 06

As with encryption, custom build is required to enable validation (using environment variables a build time):

Screenshot 2024-01-29 at 07 55 29

core/io/pck_packer.h Outdated Show resolved Hide resolved
core/io/pck_packer_compat.inc Outdated Show resolved Hide resolved
@bruvzg bruvzg marked this pull request as ready for review January 31, 2024 16:42
@bruvzg bruvzg requested review from a team as code owners January 31, 2024 16:42
@bruvzg bruvzg force-pushed the _pack_sign branch 3 times, most recently from f415c9c to aa0a137 Compare February 9, 2024 12:43
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks like a pretty thorough implementation. I have yet to test it to see how the flow is like. This will require extensive user documentation so they know how to use it (we're currently also lacking in documentation for the script signing, we have a page about compiling custom templates but no real mention of it in the Exporting section of the docs).

I'd like @Faless to review the crypto related changes.

And overall this requires some discussion with maintainers and users to see how desirable this feature would be. It adds a bit of complexity, so we need to make sure it's something users would actually use.

There's significant demand for better obfuscation / protection of PCKs, so this seems to go in the right direction for this use case, but we need to make sure those users would be happy with this solution.

doc/classes/PCKPacker.xml Show resolved Hide resolved
doc/classes/PCKPacker.xml Outdated Show resolved Hide resolved
editor/export/editor_export_platform.h Outdated Show resolved Hide resolved
editor/export/editor_export_platform.h Outdated Show resolved Hide resolved
@reduz
Copy link
Member

reduz commented Mar 6, 2024

This looks extremely good. In fact, I am not sure if we should also just deprecate the old encryption menu and keep signing with private key only.

@bruvzg
Copy link
Member Author

bruvzg commented Mar 6, 2024

I am not sure if we should also just deprecate the old encryption menu and keep signing with private key only.

I'm not sure if I see how it's related: encryption is used to hide information and signing to prevent modification, it's different stuff for different purpose. ECDSA is not suitable for encryption (and ECIES is not supported by mbedtls), and using the same key as both public key and encryption key is usually considered unsafe.

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.

None yet

4 participants