godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.12k stars 69 forks source link

Don't put sensitive credentials such as Android keystore properties in `export_presets.cfg` #1156

Closed andy-noisyduck closed 1 year ago

andy-noisyduck commented 4 years ago

I had a feeling I've come across this before, but I couldn't find any proposals....

Describe the project you are working on: Mobile game (Android)

Describe the problem or limitation you are having in your project: Godot stores export settings including which resources to bundle, app icons, export capabilities in the export_presets.cfg. These settings are a core part of development, and we normally want to include these in source control. Unfortunately Godot also stores the keystore credentials in the same export file. This means we either have to commit credentials to our repo, or we have to manually recreate the presets between developers, and somehow sync changes. Both are really bad options.

Describe the feature / enhancement and how it helps to overcome the problem or limitation: It would be better to have keystore credentials in their own config file alongside exports_presets.cfg. Something like exports_credentials.cfg or similar. This file could then be local to each developer and not need committing. It also makes it much easier to apply specific keys for CI/CD if being used.

Another alternative would be to allow any properties of exports_presets.cfg to be overriden locally, such as a exports_presets.local file with the same format as the presets file. This might be more appropriate if further settings plan to be stored in the presets file too - paths would be a reasonable use case here as they can often be machine specific.

If this enhancement will not be used often, can it be worked around with a few lines of script?: I can't think of a nice way around it.

Is there a reason why this should be core and not an add-on in the asset library?: Android exporting is a core feature of Godot. Anyone exporting to android with a team of more than 1 will end up hitting this issue.

Calinou commented 4 years ago

This will be addressed by https://github.com/godotengine/godot/pull/35930 once it's merged.

Calinou commented 1 year ago

Renamed to also cover Windows and macOS codesigning credentials.

In the meantime, the current recommendation is to add export_presets.cfg to .gitignore, or avoiding specifying any credentials in it. You can specify credentials at build-time using sed if running a script or using a CI platform to export the project in production.

and-rad commented 1 year ago

I'm about to pick this up where @Calinou left off. These are the properties that I consider sensitive information (Names in parentheses are the key names stored in the config file):

All:

Not quite sure about these:

Is remote deploy supposed to be used for pushing to production machines? If not, I can see value in having these properties in the git repository if it's a larger place with internal test servers that every team member should be able to deploy to for testing, but I don't know if that is an intended scenario here.

Windows:

Android:

iOS:

UWP:

Not sure about these:

macOS:

These exist in the config file, but I couldn't see them in the export UI:

The plan is to create a separate export_credentials.cfg file that stores these and should not be committed to git. You will also be able to set each of these properties by environment variables. If an environment variable is present, it will always take precedence over the export_credentials.cfg file.

Calinou commented 1 year ago

Is remote deploy supposed to be used for pushing to production machines? If not, I can see value in having these properties in the git repository if it's a larger place with internal test servers that every team member should be able to deploy to for testing, but I don't know if that is an intended scenario here.

To my knowledge, remote deploy always deploys debug export templates, so it's not meant for deploying to production. (Deploying release export templates could be added to make performance testing more representative of real-world scenarios, but it'll disable live scene editing and other debug client features.)

and-rad commented 1 year ago

If that's the case, I'm inclined to leave the remote deploy properties where they are. I can see good reasons for committing them to VCS and for moving them out, so I think I'll stick with how it is now. We can still change it later if people prefer it the other way.

What about UWP's Identity Product GUID and Identity Publisher GUID? I have no experience with UWP, but from my limited research it seems that these are public knowledge.

and-rad commented 1 year ago

Should values that are stored in environment variables show up in the export UI? I'm thinking probably no because it might be a little too confusing. On the other hand, if the UI only displays the values that are stored in the .cfg files, (a) people have to remember that they set up environment variables and should make any changes there and (b) the UI would effectively lie to them, claiming that an export option has value X (defined in .cfg) when in reality it is Y (defined in env var).

~The reason why I still lean towards keeping environment vars out of the UI is that I think the majority of users won't use them anyway.~ I think I changed my mind on this. I thought about it some more and it might be a cleaner and overall better solution to read from environment variables too when populating the export UI.

What may be a useful addition, though, is a button somewhere in the export UI that lets you copy all environment variable names in a given preset to the clipboard.