Browser Build "Validate Secrets" Improvements

[dnzxy]

This PR is the first of probably a set of PRs over the next couple of weeks (and months?) that aim to improve Loop's browser build's validation and error handling. It further aims to provide more descriptive error messages for common errors that first time builder's come across, such as:

• Incorrectly setting up the Match-Secrets repository (with the current implementation if that repository is incorrectly set up the error will point toward GH_PAT issues)
• Spell checking for Apple Developer related secrets (TEAMID, FASTLANE_KEY_ID, FASTLANE_ISSUER_ID)

Contents:

• Adds check for Match-Secrets existence
• Adds TEAMID case sensitivity check
• Adds FASTLANE_KEY_ID case sensitivity check
• Adds FASTLANE_KEY_ID length check
• Adds FASTLANE_ISSUER_ID format check (adhering to UUID).
• Later additions:
• Adds check for Match-Secrets (existence and visibility == private) and can automatically create it if non-existent
• Adds check for MATCH_PASSWORD to avoid bad decrypt errors when users change their secret between builds

As always, open for feedback and input. Thanks to @billybooth and @bjorkert for being great sparring partners to trade ideas back and forth with.

[billybooth]

This looks really good to me! I don't see any reason to hold it up. The changes are all surgical and will definitely save additional support cycles.

As you say, further changes can be made in subsequent PRs.

[marionbarker]

Summary: This was able to identify problems with 4 of the 6 secrets and provide easy to read messages

TEAMID - an error in TEAMID (other than in number of characters) cannot be identified until the build fails
MATCH_PASSWORD - not identified until Create Certificates. (Can this be found as part of Verify Secrets?).

Testing Details:

I created a branch at my LoopWorkspace repo and merged this file. (branch name dev_plus_pr77).

All 6 of my secrets are valid and stored safely.
Test 1: run validate secrets - all passed:

• https://github.com/marionbarker/LoopWorkspace/actions/runs/5802769895

Test 2: Change all 6 to be incorrect. Note - issuer-id - I replaced a digit with a Q which made it invalid

• https://github.com/marionbarker/LoopWorkspace/actions/runs/5802835902
  • GH_PAT secret is set but invalid or lacking appropriate privileges on the marionbarker/Match-Secrets repository. Verify that it is set correctly and try again.
  • FASTLANE_ISSUER_ID is set but invalid. Verify that it is set correctly (adhering to UUID format) and try again.

Test 3: Modify GH_PAT and FASTLANE_ISSUER_ID to be correct (note TEAMID is still incorrect).

• https://github.com/marionbarker/LoopWorkspace/actions/runs/5802885435
  • The FASTLANE_KEY secret is set but invalid. Verify that it is set correctly and try again.

Test 4: Modify FASTLANE_KEY to be correct content but bad format - same error message

• https://github.com/marionbarker/LoopWorkspace/actions/runs/5802912472

Test 5: Modify FASTLANE_KEY to be correct content and format

• https://github.com/marionbarker/LoopWorkspace/actions/runs/5802935244
  • Unable to create a valid authorization token for the App Store Connect API. Verify that the FASTLANE_ISSUER_ID, FASTLANE_KEY_ID, and FASTLANE_KEY secrets are set correctly and try again.

Test 6: update FASTLANE_KEY_ID (Note that TEAMID is still wrong)

• https://github.com/marionbarker/LoopWorkspace/actions/runs/5802964170
• This passed even though TEAMID is incorrect and MATCH_PASSWORD is incorrect

Test 6 B: Only Four of Six secrets are OK, move on to Add Identifiers

• That also succeeded - check identifiers and delete the "bad" ones with wrong TEAMID
• Learned something - all identifiers (other than Loop) must be deleted FIRST and then and only then can the Loop identifier with the bad TEAMID be deleted

Test 7: Update to correct TEAMID and move on to Create Certificates (MATCH_PASSWORD is still incorrect)

• https://github.com/marionbarker/LoopWorkspace/actions/runs/5803248710
  • now I get the Invalid password passed via 'MATCH_PASSWORD'
  • but I have to go looking for it

[billybooth]

Thanks for this @marionbarker. The fastfile's validate routine could be updated to include a fastlane match, which will catch the bad MATCH_PASSWORD up front.

Edit: Perhaps that should remain out of scope for this PR though, since the next related objective is to extract error messages/exit codes from fastlane's output, and that change would be required to give a good, descriptive annotation and identify the bad secret here.

[billybooth]

@billybooth said:

Edit: Perhaps that should remain out of scope for this PR though, since the next related objective is to extract error messages/exit codes from fastlane's output, and that change would be required to give a good, descriptive annotation and identify the bad secret here.

As I think this over, a separate match validation routine could be added to the fastfile, which would separate the validation of the App Store Connect (i.e., FASTLANE) secrets from the match (i.e., MATCH_PASSWORD and GH_PAT) secrets without any further work on extracting/parsing known fastlane errors.

[dnzxy]

The fastfile's validate routine could be updated to include a fastlane match, which will catch the bad MATCH_PASSWORD up front.

Just want to note: The bad decrypt error only happens for repeat builders that have stored certificates in the Match-Secrets repository. For first time builders this is irrelevant.

@marionbarker additional testing, if you want to do that, would be to remove permissions for GH_PAT and to rename/remove Match-Secrets.

Also a learning I bad while implementing these validations (which is quite obvious when you think about it, still): The GH_PAT needs repo access to read a private repository and verify it. Hence, the Match-Secrets check is the last in the conditional chain. With lacking permission, the validation will point to GH_PAT first and once permissions are correctly set and there are still issues with the matching repo the validation will show that error.

[marionbarker]

Suggestions

1. I think the spelling for Match-Secrets should be checked first
2. If possible, check that Match-Secrets is private (not public)

Continued testing:

• renamed my real match-secrets and created a new one
• remove workflow and repo scope from GH_PAT

Test 1: New repo is not spelled correctly and is public and GH_PAT has no scopes selected.

• https://github.com/marionbarker/LoopWorkspace/actions/runs/5810577180
  • GH_PAT secret is set but invalid or lacking appropriate privileges on the marionbarker/Match-Secrets repository. Verify that it is set correctly and try again.

Test 2: Now spell the (public) Match-Secrets correctly

• https://github.com/marionbarker/LoopWorkspace/actions/runs/5810616678
  • This succeeded - but Match-Secrets should NOT be public
  • Is there a way to check this?

Test 3: deleted the public repo and renamed my private one back to the correct name, GH_PAT still has no scope selected

• https://github.com/marionbarker/LoopWorkspace/actions/runs/5810683360
  • GH_PAT secret is set but invalid or lacking appropriate privileges on the marionbarker/Match-Secrets repository. Verify that it is set correctly and try again.

[dnzxy]

Thank you for testing and your ideas @marionbarker

Suggestion 1: Not possible if the repo is private (which it should). If the GH_PAT lacks permissions (which we check), then we cannot list private repos in the repo query that is used for the Match-Secrets spelling check. That’s why the order is as it currently is - token existence first, then token permissions, then spelling for Match-Secrets.

Suggestion 2: I can look into it, maybe it can be done with a bit of a workaround where we make a query without proving a token and if Match-Secrets is not listed we query it with the token and it should be listed then - that way we could probably verify that it’s indeed private. I‘ll have to fiddle around a bit and try that; it seems a bit hacks though.

[marionbarker]

If Match-Secrets is public - you'll be able to see it and should tell them to make it private.
If you can't see it, then you can't check spelling without a valid token.

[marionbarker]

Testing continued:
With appropriate GH_PAT with workflow (and repo) scope.
Rename Match-Secrets repository (private).

• https://github.com/marionbarker/LoopWorkspace/actions/runs/5811371526

this shows me this error message:

GH_PAT secret is set but Match-Secrets repository is missing or configured incorrectly. Verify that the repository was created and that its name is spelled correctly (uppercase 'M' for 'Match', uppercase 'S' and plural for 'Secrets', dash character '-' between the words as separator).

[dnzxy]

If Match-Secrets is public - you'll be able to see it and should tell them to make it private.
If you can't see it, then you can't check spelling without a valid token.

Was going to test something along those lines, yes, but want to make sure it doesn’t give errors when it shouldn’t. I‘ll test around a bit 😊

[marionbarker]

next test: TEAMID is 9 characters and includes a lower case letter:

• https://github.com/marionbarker/LoopWorkspace/actions/runs/5811784394
  • TEAMID secret is set but has wrong length. Verify that it is set correctly and try again.

Change it to be 10 characters but still with a lower case letter

• https://github.com/marionbarker/LoopWorkspace/actions/runs/5811805172
  • TEAMID is set but invalid. Verify that it is set correctly (only uppercase letters and numbers) and try again.

[dnzxy]

So while working on this PR this afternoon, I had an idea and I want to just put it here to get some feedback, so pinging you guys – @marionbarker @billybooth @bjorkert @bjornoleh @ps2 .

One suggestion was to check whether the potential Looper has created the Match-Secrets repository as a private repository, and I thought: Why not create it for them on the first run of validate secrets in a potential setup infrastructure step?

That would

• Minimize the configuration for the build by one step
• Eliminate one potential source for error, i.e., misspelled Match-Secrets repo name

Building on this, is there a good way to check for a "correct" MATCH_PASSWORD secret? Well, technically yes.

We would have to run a fastlane match that accesses the repo and if there's a mismatch for the passphrase, we will run into a bad decrypt error (addition of this error to LoopDocs is pending).

So how about we do this:

• Check for Match-Secrets existence (we can adapt the alive branch check that I built as part of PR71 for that)
• If it does not exist (initial run), create it via gh repo create Match-Secrets --private
• If it exists, check for any commits to it (those would be by the fastlane runner) via

gh api \
     --method GET \
     -H "Authorization: token $GITHUB_TOKEN" \
     -H "Accept: application/vnd.github.v3+json" \
      /repos/${{ github.repository_owner }}/Match-Secrets/commits \

• If it has commits, perform a fastlane match --readonly which does not alter or update any existing certificates or profiles but simply reads them; to do that it will need access to the correct passphrase, i.e., our MATCH_PASSWORD secret.
• If it does not have any commits, this will result in the following output gh: Git Repository is empty. (HTTP 409). We can skip match in this case, as there will be no certs encoded with the passphrase in the repo at this point.
• If fastlane match --readonly fails (possibly check for bad decrypt), add a corresponding error to the annotated output.

What do you guys think? Yay or nay?

[billybooth]

@dnzxy said

What do you guys think? Yay or nay?

I think this is clearly a yay, since it represents a significant reduction in the initial setup for new builders. If you wanted, you could even create the MATCH_PASSWORD secret for the user via Create GitHub Secret Action, but in that case the user wouldn't know the match password (since we don't want to print it in a public repository's actions history) and then presumably wouldn't be able to use the Match-Secrets repository for building other apps in addition to Loop. Best leave that notion behind and focus on the creation of the Match-Secrets repository for the user if it doesn't exist.

In any case, I think this is a good justification for moving the GH_PAT permissions/scope check out of the build loop workflow and into the validate secrets workflow (or else, duplicating it here if that's simpler). Adding a readonly fastlane match is also a great opportunity for capturing/processing fastlane's output to extract a specific error detail (bad decrypt) with much smaller surface area. I am interested in doing some work to support that if you'd like an assist, and I could probably work on it/discuss it this week.

All in all though, hard "yay" from me, since (in general), you have all of the pieces and permissions required to create the Match-Secrets repository with a repo-scoped classic PAT. It is worth considering what the implications are for a modern, fine-grained PAT, but I think anyone who has a specific reason to justify using one of those is capable of creating the Match-Secrets repo in advance and granting the required permissions.

[marionbarker]

Regarding fine-grained token - no expiration date is not an option. The nominal Loop builder should use classic token and select no expiration.

[dnzxy]

I've pushed various changes that incorporate most of the suggestions and test feedback I have gotten here and through private chats. Thanks to @billybooth for reworking the validate_secrets lane and suggesting some great re-ordering of validations.

This can/should be re-tested, happy to get feedback :)

[marionbarker]

We have tested this PR extensively following the merge yesterday of PR #71.
@billybooth, @dnzxy and @marionbarker agree this is ready for final review by @ps2.

Test 1. "Good SECRETS - existing builder", build action successful:

• Configure as a current user who will sync their main branch of their fork and run Build Loop following the next release
• Comments: GH_PAT set with repo scope, alive branch does not exist
  • If there had not been changes to Identifiers since last release, this user would only need Build Loop
    • This test is ignoring the changes to identifiers required by libre addition and rename of widget
    • It is only considering the validation improvements associated with this PR
  • Build Loop succeeds with:
    • alive branch automatically created
    • Annotations associated with the successful Build Loop log indicates that automatic build is not configured, and provides information

Test 2. "Good SECRETS - new builder", all actions successful:

• Configure as a new builder, (new instructions in LoopDocs will indicate Match-Secrets is automatically generated)
• Comments: fresh fork from LoopKit/LoopWorkspace, GH_PAT has repo, workflow scope, add the 6 SECRETS and run the 4 actions (with associated steps)
  1. Validate secrets
     • most secrets verified, some checked only for format
     • private Match-Secrets automatically created
  2. Add Identifiers
  3. Create Certificates
  4. Build Loop
     • alive branch automatically created

Many Tests for "Bad" SECRETS:

• Modifed each of the six SECRETS and GH_PAT permissions extensively to cause an action to fail; and then viewed messages in Annotations
• Based on these tests, the following changes were made today to this PR
  • Add a few more failure messages as Annotations when an action fails
  • Provide instructions (with URL where appropriate) for how to fix an error

[marionbarker]

With my default branch set to dev_plus_pr77, (dev with PR 77 merged), the time for the first weekly automatic check for updates ran successfully: Build Loop log

That branch was already up to date, so the build action was skipped. It ended with a success after 1 min 14 sec.

One thing that may not have been mentioned about this PR (#77) is that the action log has a very clean layout that is much easier to interpret.

[ps2]

Looks good! Thanks all!