[AINFRA-1533] Adopt git-conceal in this repo
#14979
Conversation
AliSoftware
force-pushed
the
AINFRA-882-adopt-git-crypt
branch
5 times, most recently
from
dc6517c to
703d297
Compare
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
AliSoftware
force-pushed
the
AINFRA-882-adopt-git-crypt
branch
4 times, most recently
from
be5c217 to
0509840
Compare
AliSoftware
added
the
category: tooling
AliSoftware
changed the title
| echo "--- :closed_lock_with_key: Installing Secrets" | ||
| bundle exec fastlane run configure_apply | ||
|
|
There was a problem hiding this comment.
Having the repo git-crypt-unlocked during the branch dance that is done internally by comment_with_manifest_diff caused Your local changes to the following files would be overwritten by checkoutissues, especially if the git-crypt'd files listed in .gitattributes on the HEAD branch are not the same as the ones in the BASE branch1
Since we don't need any secret in practice to generate the manifest and call process{variant}Manifest, the solution is simple: just don't bother unlocking the repo's secrets for that task.
A better long-term solution to make comment_with_manifest_diff more resilient to situations like this would be to make it use git worktree instead of switching branches in-place:
- Generate base manifest:
git worktree add $TMP_DIR_FOR_BASE $BASE_BRANCH && cd $TMP_DIR_FOR_BASEthen run./gradlew process{variant}Manifestthere - Generate head manifest:
cd $CHECKOUT_DIR && rm $TMP_DIR_FOR_BASE && git worktree prunethen run./gradlew process{variant}Manifestthere
That way each checkout is done in independent folders, eliminating the risk of conflicts during the branch dance.
Footnotes
-
like will be the case during that transition to
git-crypt, or when we'll add a new secret file, especially if that secret file previously existed unencrypted in the BASE branch as an example file for external contributors I think? ↩
.buildkite/shared-pipeline-vars
Outdated
| export CI_TOOLKIT="automattic/a8c-ci-toolkit#5.4.0" | ||
| # "git-crypt-unlock" branch / https://github.com/Automattic/a8c-ci-toolkit-buildkite-plugin/pull/195 | ||
| export CI_TOOLKIT="automattic/a8c-ci-toolkit#0a3f10921096cee57c18ac5667fc64c1aaad4a7d" |
There was a problem hiding this comment.
🎗️ TODO: Revert back to pointing to a tag version once Automattic/a8c-ci-toolkit-buildkite-plugin#195 is merged and we have an official new version of the ci-toolkit
AliSoftware
added
the
status: do not merge
Generated by 🚫 Danger |
wzieba
left a comment
wzieba
left a comment
There was a problem hiding this comment.
ℹ️ The real migration won't require devs to clone the repo from scratch; this is just for the testing steps of this PR
Hm, I don't know why exactly, but ~/woocommerce-android/.configure-files/firebase.secrets.json is not respected in .gitignore on this branch.
So, as a result, I could by mistake include the firebase.secrets.json in my git commit. To reproduce:
gco trunkbundle exec fastlane run configure_applygco AINFRA-882-adopt-git-cryptgit status -u
This is a very good and important point, thanks for raising it! This PR indeed removed those files from |
There was a problem hiding this comment.
💭 I was wondering if there could be a way to be sure, in reviews, that this has in fact been encrypted or not specially given files like this are binary files. Then I've noticed all git-crypt encrypted files start with GITCRYPT so perhaps this could be a simple way to check for that in an automated way?
There was a problem hiding this comment.
You mean like I do here? 😛
Indeed maybe we can write a Dangermattic plugin to detect which file in the PR are encrypted and add an inline comment on the file if so as an extra information? Is that what you meant?
As for manually testing locally if a file is properly encrypted before pushing a commit to the remote, one can use git-crypt status -e <file> directly to check that (or, alternatively, print the raw content of the file with git show :<file>—e.g. git show :secrets.properties, with leading :colon—and confirm that row content is some binary garbage starting with\0GITCRYPT\0`.
There was a problem hiding this comment.
You mean like I do here? 😛
Ha, yes, I realized that function was doing that after I posted the comment 😂
Indeed maybe we can write a Dangermattic plugin to detect which file in the PR are encrypted and add an inline comment on the file if so as an extra information? Is that what you meant?
Yeah, though at the same time I find it a bit difficult to do that in a systemic way that will be useful (as we don't add secrets that often)...
And without a clear pattern on the secret files, it's also not clear how to generalize a check without knowing all files in advance (defeating the purpose of warning when that new file has been added without encryption, there's still some value for updates...) 🤔
As for manually testing locally if a file is properly encrypted before pushing a commit to the remote, one can use
git-crypt status -e <file>directly to check that (or, alternatively, print the raw content of the file withgit show :<file>—e.g. git show :secrets.properties, with leading:colon—and confirm that row content is some binary garbage starting with\0GITCRYPT\0`.
👍
There was a problem hiding this comment.
💭 Again thinking out loud, not sure if it's worth the trouble: what do you think of grouping encrypted / secret files in the same folder and making a convention out of this for all projects (well, kinda similar to what we had before but making it more obvious)?
The main advantage is added clarity and creating a pattern where we can build things on top (e.g. validation to make sure everything under that folder is encrypted). Of course, it wouldn't completely prevent mistakes, but it would be a way to keep things clear.
There was a problem hiding this comment.
Yeah, I thought about that.
I think that for some particular secret files, they do need to be at specific locations in the project structure for the compilation to work (e.g. WooCommerce/google-services.json, secrets.properties). So we won't have much choice on those, it's enforced by Gradle expecting them there.
For others (upload.jks, debug.keystore, google-upload-credentials.json…), their location might be arbitrary and ok to move somewhere else. In that case maybe we can move them to some .secret-files/ folder just to group them indeed.
The only trick is that if they were already existing in the repo a particular location on disk before that PR, and we move them elsewhere as part of this PR, we'd then need to add their old location back to .gitignore to avoid the case that @wzieba found about above of someone switching to a branch in which those were in the old location, then switching to this branch while having the leftover of the decrypted file from old branch at the old location if they didn't run git clean in-between to remove untracked files… and then risk of commiting those old files and thus leaking secrets accidentally.
I still see the appeal of having secret files grouped nicely in a dedicated folder rather than keeping them at the root of the repo still. But I'm not sure moving them is worth the potential risk (or having to keep legacy entries in .gitignore just for that risk) 🤔 WDYT?
There was a problem hiding this comment.
But I'm not sure moving them is worth the potential risk
👍 I'm also leaning towards that. It's also annoying that they can't be in the same place, which would be the ideal scenario and perhaps would make the change worth it.
The `google-services.json-example` file is automatically copied by `WooCommerce/build.gradle` if the `google-service.json` file is detected as being encrypted. So the instructions I initially added to suggest to copy the file manually are not relevant and not needed after all. h/t @wzieba #14979 (comment)
Addressed in 1754ce8 by re-adding |
So that we decrypt things as early as possible in the CI jobs, now that `git-crypt` doesn't need Ruby to be installed first like `configure_apply` did. h/t @iangmaia — #14979 (comment)
|
Version |
AliSoftware
changed the title
AliSoftware
force-pushed
the
AINFRA-882-adopt-git-crypt
branch
from
847462c to
7edb5cc
Compare
AliSoftware
changed the title
git-conceal in this repo
AliSoftware
changed the title
git-conceal in this repogit-conceal in this repo
AliSoftware
force-pushed
the
AINFRA-882-adopt-git-crypt
branch
from
cd6a059 to
a8a3738
Compare
AliSoftware
force-pushed
the
AINFRA-882-adopt-git-crypt
branch
from
a8a3738 to
279e2b7
Compare
AliSoftware
force-pushed
the
AINFRA-882-adopt-git-crypt
branch
from
279e2b7 to
6365dc5
Compare
There was a problem hiding this comment.
🎗️ TODO: Point back to stable version once Automattic/a8c-ci-toolkit-buildkite-plugin#195 has been merged and released
|
@wzieba I have now updated the PR to use my new Can you thus give it another review? |
Wrapper from `ci-toolkit` that downloads `git-conceal` if it isn't yet available on the machine, then run `git-conceal unlock env:…`
AliSoftware
force-pushed
the
AINFRA-882-adopt-git-crypt
branch
from
6365dc5 to
248c9f4
Compare
AliSoftware
force-pushed
the
AINFRA-882-adopt-git-crypt
branch
from
84837c5 to
64221f8
Compare
|
Sorry @AliSoftware , I didn't make it to review this today. I'll prioritize it first on Monday. |
No worries. Btw, in practice after discussing the timing during my 1:1 with Tanner and given the EOY rush and my upcoming long AFK, I am not even sure I'm going to merge this before January. i.e.
I am still interested in your review of course, especially since I've started to work on draft PRs to do similar changes to other Android repos so if you have feedback on the code changes / on the approach in the Gradle code to fit that change, it'd be easier to get them before I start applying the same approach to my other draft PRs… but it's also not critically urgent because none will be merged before January anyway. PS: I'm also interested in having WCAndroid devs playing with this PR and experimenting with the tool—while staying in this PR—so that they can validate, as users, that it's easy to use and works well for them. Though that won't allow them to test it in real situations (like validate it still works when switching branches) until I actually merge it in January—when I'll be more available and around for any needed support—but at least it'd give early user-side feedback too. |
Closes: AINFRA-1533
What
This PR migrates the repo to use
git-concealinstead ofconfigure_apply, so that the usage of secrets in the repo gets easier and more transparent for developers.How
.configureand.configure-files/*.encfiles.configureas encrypted files in the repo instead, by also adding their path as entries in the.gitattributeswithfilter=git-conceal diff=git-conceal—so they get encrypted bygit-concealon push.gradlefiles andFastfileto account for new file paths.gradlecode to account for the fact that now the files are present (but encrypted) after cloning the repo—as opposed to being absent when we didn't runconfigure_applyin the old way.bundle exec fastlane run configure_applyin.buildkite/*commands—and calls toconfigure_apply(force: true)inFastfile—with calls togit-conceal-unlock, and bumpci-toolkitplugin to point to Addgit-conceal-unlockwrapper Automattic/a8c-ci-toolkit-buildkite-plugin#195 where this helper livesNote
ℹ️ About the git-conceal-helper helper from ci-toolkit
The way this currently works is that:
git-concealGitHub repo builds new binaries for macOS, Linux and Windows when we do a new release, and attach them as assets to the GitHub Releasegit-concealGitHub repo also provides aninstall.shscript to make it easy to installgit-concealfor your current platform (the script takes care of detecting if you're on a Linux, macOS or Windows agent, downloads the right asset from the latest GitHub release for that platform,chmod +xit and install it)git-conceal-helperwrapper from oura8c-ci-toolkitplugin is then just a small bootstrap/helper script that checks ifgit-concealis installed in the CI agent, if so callsgit-conceal unlock "env:${1:-GIT_CONCEAL_SECRET_KEY}"and exit; otherwise calls theinstall.shscript ofgit-concealto install it first before callinggit-conceal unlockThe way this might work in the future:
git-concealin most of our self-hosted CI agents and custom AMIs to avoid having to download it on each job… even if in practice it takes less than 1s to download and unlock the repoci-toolkitthat does the "tests if installed already and install it if not" logic, because not all our CI agents are custom agents (e.g. we sometimes use thedefaultqueue which uses EC2 instances with the official Buildkite AMI, not customized by us), so that would still be useful for those.Merge timing
Important
Do not merge this PR until (1) we have all the documentation / FAQ ready for all devs to follow in FG and (2) we have validated similar PRs in other repos—especially ones using macOS/Windows and Tumblr CI agents—work too..
Test Steps
General check
WooCommerce/google-services.jsonWooCommerce/upload.jksdebug.keystorefirebase.secrets.jsongoogle-upload-credentials.jsonsecrets.propertiessentry.properties*.gradlefiles to reflect the new setup are covered by this PR (path updates to.propertiesor keystore files, code updates around logic that was based on if a secret file exists or not, …)Follow the
README.mdinstructions as an Automatticianpbpaste | base64 -d | git-conceal unlock -at the root of your new clone's directorysecrets.propertiesin clear nowgoogle-services.jsonis taken into account, etc)Note
ℹ️ The real migration won't require devs to clone the repo from scratch; this is just for the testing steps of this PR
Above I suggest to test this scenario in a fresh clone of the repo rather than your everyday working copy, especially to avoid risking to leave your git repo in a state that is setup for
git-concealwhile you were just testing and the PR is not merged yet.In practice, once the PR is merged in
trunk, devs won't need to clone the repo fresh to migrate togit-concealthough.They will just have to run
pbpaste | base64 -d | git-conceal unlock -on their existing working copy once, and probably never run agit-concealcommand again afterwards (except maybegit-conceal statusif they want to validate a new secret file they're about to add is properly gonna be encrypted as they expect).At some point I'd still highly recommend for them also get rid of the old files that were installed by their previous setup with
configure(rm -rf ~/.configure/woocommerce-androidandgit clean -dxi -e .DS_Store -e .idea -e local.properties), to avoid confusion and risking relying on obsolete files.Follow the
README.mdinstructions as an external contributorsecrets.propertiesshow as encrypted garbagedefaults.propertiesto add the relevant values forwp.oauth.*WooCommerce/google-services.jsonencrypted file withWooCommerce/google-services.json-example