Patch 1.2.1: App Secrets

[esreli]

This PR is the second attempt to resolve https://github.com/Esri/data-collection-ios/issues/229 once and for all. This PR builds off of the first attempt.

The solution

This solution leverages apple's swift gyb to generate a static AppSecrets.swift file at build time which is output to the project's derived data directory; this is done using a custom build rule. The output swift file is compiled alongside all other swift files in the compile sources build phase. This solution requires the app's client ID and license key to be written to a secret file (the instructions are located in the README.md).

[rolson]

Clever solution.

However, If I'm reading this correctly, this solution requires copying apple's script and putting in this repo. To me that raises a few red flags. One, that we are duplicating code (apple's). Two, licensing of that would need to go through legal.

From what I know, the goal is to include sensitive keys without committing them to git - what about putting the file that contains the keys in the .gitignore?

[esreli]

@rolson this is some good feedback, thanks.

One, that we are duplicating code (apple's).

Good point. Perhaps we can consider a solution that gitignores gyb and uses some sort of git-hook that downloads the script at a certain moment in git flow. Thoughts?

Two, licensing of that would need to go through legal.

Ah yes, of course. Well, Swift and the swift repo is licensed under Apache License 2.0. We should check with legal but I think a strong case can be made for using it.

From what I know, the goal is to include sensitive keys without committing them to git - what about putting the file that contains the keys in the .gitignore?

I considered this solution as well. I wanted to avoid this route because to me it feels like an inelegant solution simply because it breaks the Xcode project. Alternatively, the solution proposed in this PR doesn't break the Xcode project but instead provides a build error if no .secrets file is provided. To me, this feels more explicit and complete.

[rolson]

Good point. Perhaps we can consider a solution that gitignores gyb and uses some sort of git-hook that downloads the script at a certain moment in git flow. Thoughts?

Not sure. Duplicating this may not be a problem, although it always feels wrong.

I considered this solution as well. I wanted to avoid this route because to me it feels like an inelegant solution simply because it breaks the Xcode project. Alternatively, the solution proposed in this PR doesn't break the Xcode project but instead provides a build error if no .secrets file is provided. To me, this feels more explicit and complete.

Makes sense. I think an argument can be made that while the project will be broken in the other case, this solution might require a bit more complexity. But in the end those are things you guys can weigh and decide.

[nixta]

Good feedback/discussion @rolson and @esreli.

While I understand Ryan's reticence to replicate the gyb.py file in our repo, I think that's OK for now. We could eventually implement a solution to download gby.py when the repo is first built, following in the Samples app's footsteps where a build phase script downloads resources that are too large to store in Git.

My remaining question is have we considered what to do if Python is not installed? Would the Xcode build system not even work if it wasn't?

[esreli]

My remaining question is have we considered what to do if Python is not installed? Would the Xcode build system not even work if it wasn't?

I simulated this by modifying gyb to access a resource unavailable to the system. The result is that the build step fails, failing the entire project build, providing this error:

Command RuleScriptExecution failed with a nonzero exit code

[nixta]

Oh, well, I assumed the build would fail. I meant more how should we guide the user forward. Or is that simply to ask them to ensure that Python is installed?

Does it have to be a specific version of Python installed a specific way? Would brew install python work, for example? Or should it be some other method to install?

[esreli]

Does it have to be a specific version of Python installed a specific way?

Yes, gyb looks for the python 2.7 interpreter to perform the the rest of the script. The user needs to have python2.7 in their path.

Would brew install python work, for example?

I'm not sure we can depend on the user having homebrew installed. If they do, I think the brew command should specify python 2.7. though I don't think we should coach them into using homebrew.

brew install python@2

python2.7 ships with macOS by default. Someone would have to remove python2.7 or modify the path to the interpreter all together. Maybe i'm off on this one but I think either of these scenarios is unlikely. If they've done that, they'll know how to add python2.7 back in.

Perhaps we consider offering a more verbose warning or error message?

# Secrets
SECRETS=${PROJECT_DIR}/.secrets
if [ -f "$SECRETS" ]; then
    source "${SECRETS}"
else 
    echo "$SECRETS does not exist."
    exit 1
fi
# Client ID
if [ -z "$ARCGIS_CLIENT_ID" ]
then
      echo ".secrets is missing \$ARCGIS_CLIENT_ID, this will affect your app."
fi
# License Key
if [ -z "$ARCGIS_LICENSE_KEY" ]
then
      echo ".secrets is missing \$ARCGIS_LICENSE_KEY, this will affect your app."
fi
# Python
if [[ -z $(which python2.7) ]]
then
    echo "Python2.7 interpreter not found in in the user's path, exiting."
    exit 1
fi
# Execute GYB
ARCGIS_CLIENT_ID=$ARCGIS_CLIENT_ID \
ARCGIS_LICENSE_KEY=$ARCGIS_LICENSE_KEY \
"${PROJECT_DIR}/scripts/gyb" --line-directive '' -o "${DERIVED_FILE_DIR}/${INPUT_FILE_BASE}" "${INPUT_FILE_PATH}"

[nixta]

Python If we know that python test is the right test, we could include it, but I'd be OK if we didn't. As you say, the user would have had to explicitly remove the default macOS Python install, so I think that just documenting this requirement in the readme would be sufficient.

If for some reason they cannot install Python though, can we document that they should just add an AppSecrets.swift file manually and remove the build phase?

Secrets If we're trying to define and demonstrate best practices for embedding API Keys in an app, and we're already using GYB, should we obfuscate the keys as suggested by NSHipster? Otherwise, we're only solving half the problem and arguably, for any closed source projects, the less important half. We don't necessarily need to go to this length for this PR, but we should figure it out for our own internal use and eventually it would be good to see published in our apps.

ReadMe Lastly, now that I've wrapped my head a bit more fully around what's happening, I would suggest that we need to explain more explicitly how this system works and why it's important to go to these lengths. Happy to work on some copy with you. We can create a separate README that goes into it, and just allude to it in the main README. Something like "The project includes a mechanism to inject sensitive license keys and credentials without committing them to source control, and to obstruct efforts to extract them from the compiled binary. See this page for more information."

[esreli]

Python I did a bit more digging here. I remembered that Python 2.7 has passed sunset starting January 1, 2020. Given 2.7 is the macOS default, I wanted to see how macOS plans to handle the sunset and what is learned is the OS has deprecated the language version as of 10.15.

Scripting language runtimes such as Python, Ruby, and Perl are included in macOS for compatibility with legacy software. Future versions of macOS won’t include scripting language runtimes by default, and might require you to install additional packages. If your software depends on scripting languages, it’s recommended that you bundle the runtime within the app. (49764202)

Use of Python 2.7 isn’t recommended as this version is included in macOS for compatibility with legacy software. Future versions of macOS won’t include Python 2.7. Instead, it’s recommended that you run python3 from within Terminal. (51097165)

So there are a few truths here.

1. Apple Swift depends on gyb.
2. MacOS is deprecating python 2.7 by default and it's expected to not ship with macOS 11.
3. It's likely macOS 11 Big Sur won't ship with python whatsoever
4. I can't find a discussion that explicitly calls to deprecate python2.7, I looked through apple/swift and apple/swift-evolution.
5. If you look at the gyb.py git history you'll see quite an overwhelming effort to support gyb.py for python 2 and 3.
6. The python 2to3 module can perform the work to update a python2.x script to python3.x, here is the diff output of gyb.py:

gyb.py diff

```sh RefactoringTool: Refactored gyb.py --- gyb.py (original) +++ gyb.py (refactored) @@ -2,7 +2,7 @@ # GYB: Generate Your Boilerplate (improved names welcome; at least # this one's short). See -h output for instructions -from __future__ import print_function + import os import re @@ -13,15 +13,15 @@ try: - from cStringIO import StringIO + from io import StringIO except ImportError: from io import StringIO try: - basestring + str except NameError: - basestring = str + str = str def get_line_starts(s): @@ -243,7 +243,7 @@ # pull out the one matched key (ignoring internal patterns starting # with _) ((kind, text), ) = ( - (kind, text) for (kind, text) in m.groupdict().items() + (kind, text) for (kind, text) in list(m.groupdict().items()) if text is not None and kind[0] != '_') if kind in ('literal', 'symbol'): @@ -731,7 +731,7 @@ # If we got a result, the code was an expression, so append # its value if result is not None \ - or (isinstance(result, basestring) and result != ''): + or (isinstance(result, str) and result != ''): from numbers import Number, Integral result_string = None if isinstance(result, Number) and not isinstance(result, Integral): ```

So, we're going to have to negotiate a future where gyb continues to be relied upon in the Swift ecosystem but is not included in macOS by default.

So what are the implications of this future as it relates to this solution? In order to implement this solution we will have to require python3, provide installation instructions and migrate gyb.py to python3. @nixta i'm really curious for your thoughts here. Is this too much to ask?

[nixta]

Thanks for doing all that research, @esreli. Nice work!

In order to implement this solution we will have to require python3, provide installation instructions and migrate gyb.py to python3.

This seems to be getting into rabbit hole territory.

I would rather see that we develop some best practices for not embedding secrets in repos and for obfuscating secrets in binaries, and write a blog post about them. At that point we could better understand the impact of attaching this mechanism to an existing project (I will hazard a guess that we'll learn some stuff as we write the blog post) and can re-evaluate.

Right now, requiring that Python be explicitly installed for everyone, as will apparently be the case in a few months, seems an unnecessary impediment to getting these apps up and running.

[esreli]

Right now, requiring that Python be explicitly installed for everyone, as will apparently be the case in a few months, seems an unnecessary impediment to getting these apps up and running.

As nice as it would be for this problem to finally be solved, I agree with you. Looks like it's back to the drawing board here.