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

Store sensitive export options in dedicated credentials file #76165

Merged
merged 1 commit into from
May 10, 2023

Conversation

and-rad
Copy link
Contributor

@and-rad and-rad commented Apr 17, 2023

Closes godotengine/godot-proposals#1156

This PR splits sensitive information from the export_presets.cfg file off into a new export_credentials.cfg file. This effectively allows users to commit export presets to version control without making confidential information public.

The change is transparent and seamless for the most part. Old export presets are loaded fine and all the credentials are preserved*. The next time any export presets are changed, the new format is being used automatically. For a list of properties that are considered confidential, see the linked proposal.

*: The only exception to this is the script encryption key. Users will have to re-enter it the next time they open the export presets UI.

I included two minimal projects to highlight the change. Before Credentials contains the old file layout and can be used to test the conversion from before this PR to after. With Credentials shows how export presets are stored with this PR merged.

Before Credentials.zip
With Credentials.zip

@and-rad and-rad requested review from a team as code owners April 17, 2023 11:16
Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

Since the rest of the values can be read from env. variables, script_encryption_key should be as well (in the editor_export_platform.cpp, there are two String script_key = p_preset->get_script_encryption_key().to_lower(); lines where it's used).

platform/windows/export/export_plugin.cpp Outdated Show resolved Hide resolved
@and-rad
Copy link
Contributor Author

and-rad commented Apr 17, 2023

Since the rest of the values can be read from env. variables, script_encryption_key should be as well (in the editor_export_platform.cpp, there are two String script_key = p_preset->get_script_encryption_key().to_lower(); lines where it's used).

No problem, but I have to look into how to do this cleanly since the script encryption key is handled differently and by different classes than the rest of the properties here.

Edit: In particular, I'm concerned with where to store the environment variable name. Every export platform has its own encryption key, so there needs to be a dedicated environment variable for each platform. That makes it platform-dependent code, but EditorExportPreset is platform-independent. Likewise, storing the environment variable in EditorExportPreset is a no for the same reason. Nevermind, I was being dumb. I'll look into this, I think I know a pretty straightforward way to do it.

@and-rad and-rad requested a review from a team as a code owner April 18, 2023 13:57
@and-rad
Copy link
Contributor Author

and-rad commented Apr 18, 2023

I implemented the requested changes and extended it so that the script encryption key can also be set by environment variables. I haven't updated the example projects yet, will do that ASAP. Example projects have been updated.

@and-rad and-rad force-pushed the safe-credentials branch 3 times, most recently from a3ae30c to 5902532 Compare April 18, 2023 19:13
Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

Needs rebase after #74644

Newly added codesign/provisioning_profile in macOS exporter should be added to credentials as well (local file path, so not critical).

platform/android/export/export_plugin.cpp Outdated Show resolved Hide resolved
@and-rad
Copy link
Contributor Author

and-rad commented Apr 26, 2023

Rebased and fixed the issues pointed out by @m4gr3d . There's still the question of whether it is necessary to have an environment variable for the script encryption key per platform. I'm happy to go either way, but I'd need someone with more authority to make a final decision.

@and-rad and-rad force-pushed the safe-credentials branch from c235830 to 8893bd0 Compare May 2, 2023 12:57
@and-rad
Copy link
Contributor Author

and-rad commented May 2, 2023

Rebased and changed script encryption key environment variable to be the same for all platforms.

core/object/object.h Outdated Show resolved Hide resolved
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.

This looks really solid. Great work!

@akien-mga akien-mga modified the milestones: 4.x, 4.1 May 8, 2023
@and-rad and-rad force-pushed the safe-credentials branch from db530f8 to 4256322 Compare May 9, 2023 12:57
@and-rad and-rad force-pushed the safe-credentials branch from 4256322 to fab160c Compare May 10, 2023 09:40
@and-rad and-rad requested a review from a team as a code owner May 10, 2023 09:40
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 great!

@akien-mga akien-mga merged commit 74e5ad5 into godotengine:master May 10, 2023
@akien-mga
Copy link
Member

Thanks!

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.

Don't put sensitive credentials such as Android keystore properties in export_presets.cfg
6 participants