[match] Added s3_skip_encryption parameter #21018

mbogh · 2023-01-30T09:43:27Z

Furthermore fixed a bug in parameters added to for_storage_mode

Checklist

I've run bundle exec rspec from the root directory to see all new and existing tests pass
I've followed the fastlane code style and run bundle exec rubocop -a to ensure the code style is valid
I see several green ci/circleci builds in the "All checks have passed" section of my PR (connect CircleCI to GitHub if not)
I've read the Contribution Guidelines
I've updated the documentation if necessary.

Motivation and Context

We plan to store all certificates and provisioning profiles in S3 with custom tailored IAM Roles for each team/project/app that can be assumed via OIDC and it would be complicated and not so secure, to have to share a encryption key as well across GitHub, GitLab and other.

Description

Added a single parameter to disable/skip encryption if you are using S3 storage mode. I have had to add everywhere Encryption.for_storage_mode(... is invoked.

In order to test this, I created two S3 buckets on my AWS account and ran:
bundle exec fastlane match import --skip_certificate_matching true -y appstore -b XYZ with s3_skip_encryption(true) and with s3_skip_encryption(false).
Went into the AWS Console and browsed the S3 buckets, downloaded the imported files and could verify that the encrypted ones were in fact still encrypted and the unencrypted ones plain .cer, .p12.

Then I tested it from a lane:

sync_code_signing(
  storage_mode: "s3",
  readonly: true,
  type: "appstore",
  app_identifier: "com.example.fake",
  s3_region: "my-region",
  s3_access_key: "ACCESSKEY!!!!",
  s3_secret_access_key: "mySecretAccessKey",
  s3_bucket: "fastlane-match-unencrypted-bucket",
  s3_skip_encryption: true
)

Testing Steps

Create two S3 buckets
Obtain access key for your account
Configure Matchfile with

storage_mode("s3")
s3_region("your-region")
s3_access_key("XYZ")
s3_secret_access_key("xyz")
s3_bucket("your-bucket")
s3_skip_encryption(true)

bundle exec fastlane match import --skip_certificate_matching true -y appstore -b XYZ
Toggle s3_skip_encryption and change bucket
bundle exec fastlane match import --skip_certificate_matching true -y appstore -b XYZ
Go to https://console.aws.amazon.com and browse your buckets.
Download files and examine.

Furthermore fixed a bug in parameters added to for_storage_mode

google-cla · 2023-01-30T09:43:31Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

mbogh · 2023-03-01T23:12:59Z

Is there any way bump this 🥹

match/lib/match/options.rb

getaaron · 2023-03-14T01:51:38Z

match/lib/match/nuke.rb

@@ -55,6 +55,7 @@ def run(params, type: nil)
        s3_secret_access_key: params[:s3_secret_access_key].to_s,
        s3_bucket: params[:s3_bucket].to_s,
        s3_object_prefix: params[:s3_object_prefix].to_s,
+        s3_skip_encryption: params[:s3_skip_encryption],
        gitlab_project: params[:gitlab_project],
        team_id: params[:team_id] || Spaceship::ConnectAPI.client.portal_team_id
      })


I kinda dislike how the current design of for_storage_mode forces all these unrelated params to be littered across all the call sites... what do you think about refactoring it so we just pass in the whole params object plus any overrides and nil replacements? Then this entire block of code could be replaced with something like

self.storage = Storage .from_params(params) .replace_if_nil({team_id: Spaceship::ConnectAPI.client.portal_team_id})

Fine with me, I can give it a go. It was kinda more changes than I thought would be needed since I had to hand pick all params.

Stay tuned :)

Okay, so the params object in this case is a FastlaneCore::Configuration and that does not play nicely with the existing tests... I will see if it something I can work around.

@getaaron I gave it a swing, found that it made the most sense to cherry pick the params going into the Storage classes themselves since they are more specific.

Please give me your thoughts.

getaaron · 2023-03-14T01:52:34Z

Added a few questions, also

Furthermore fixed a bug in parameters added to for_storage_mode

what's the bug? I didn't notice it 😅

Co-authored-by: Aaron Brager <getaaron@gmail.com>

mbogh · 2023-03-14T07:08:55Z

Added a few questions, also

Furthermore fixed a bug in parameters added to for_storage_mode

what's the bug? I didn't notice it 😅

So the s3_bucket was never sent in to the Encryption class and therefore it wouldn't be stored in the keychain, at least that is how read the code.

params[:keychain_name] = params[:s3_bucket]

mbogh · 2023-04-13T05:56:06Z

@getaaron any chance of a second look at the changes?

mbogh · 2023-05-22T19:03:07Z

Anything I can do to get this PR rolling?

giovannidegani · 2023-05-22T19:46:33Z

Would be great to get this one in

mbogh · 2023-08-07T08:32:53Z

Possible to get some eyes on this?

getaaron · 2023-08-27T19:18:16Z

@mbogh I merged master in to resolve some merge conflicts. I also had to update the importer_spec since it was asserting GitStorage would receive params that it no longer does. Could you take a look and let me know if it's good to merge?

@darbyfrey Could you double-check the GitLab-related changes? For this part, I don't think there should be any behavior change, it's just refactoring to avoid sending unnecessary params.

darbyfrey · 2023-08-30T13:21:45Z

Nice improvements @mbogh!

The GitLab storage mode changes look good. I've tested them locally and everything works as expected.

getaaron · 2023-09-03T19:11:01Z

@mbogh I think this is good to go except a few tests are still failing

mbogh · 2023-09-04T08:08:21Z

I will get it to green ❤️

mbogh · 2023-09-04T10:16:52Z

@getaaron all green again

getaaron · 2023-09-04T12:53:18Z

@mbogh thanks very much for adding this feature to fastlane!

mbogh · 2023-09-04T21:17:38Z

Thanks for your work on Fastlane @getaaron 🤗

MichalAlgor · 2023-09-18T12:48:46Z

Hi, @mbogh @getaaron
After updating from version 2.214.0 to 2.215.0 match stopped working on Gitlab CI, the error printed is

Could not find option 'job_token' in the list of available options: type, additional_cert_types, readonly, generate_apple_certs, skip_provisioning_profiles, app_identifier, api_key_path, api_key, username, team_id, team_name, storage_mode, git_url, git_branch, git_full_name, git_user_email, shallow_clone, clone_branch_directly, git_basic_authorization, git_bearer_authorization, git_private_key, google_cloud_bucket_name, google_cloud_keys_file, google_cloud_project_id, skip_google_cloud_account_confirmation, s3_region, s3_access_key, s3_secret_access_key, s3_bucket, s3_object_prefix, s3_skip_encryption, gitlab_project, gitlab_host, keychain_name, keychain_password, force, force_for_new_devices, include_mac_in_profiles, include_all_certificates, force_for_new_certificates, skip_confirmation, safe_remove_certs, skip_docs, platform, derive_catalyst_app_identifier, template_name, profile_name, fail_on_name_taken, skip_certificate_matching, output_path, skip_set_partition_list, verbose

I believe this has something to do with this change, but I have no Ruby experience to find the problem

EDIT:
My suspicion is that the issue lies in match/lib/match/storage.rb line 64 job_token: params[:job_token],.

getaaron · 2023-09-18T13:29:01Z

@MichalAlgor thanks for the report — @darbyfrey said he can take a look today and we'll do a release shortly after

darbyfrey · 2023-09-18T17:12:42Z

@MichalAlgor apologies for the regression! I've got a fix ready in #21520.

MichalAlgor · 2023-09-18T17:15:46Z

@darbyfrey no worries, glad I could help, thanks for quick fix 🙂

rogerluan · 2023-11-11T04:53:28Z

match/lib/match/storage.rb

@@ -39,7 +89,8 @@ def register_backend(type: nil, storage_class: nil, &configurator)
        end
      end

-      def for_mode(storage_mode, params)
+      def from_params(params)


It seems like this change wasn't made backwards compatible 😬😬
This was brought to my attention in fastlane/docs#1225

Is there anything I can do to help?

I don't think this is really a public API. The case in the docs (in the other PR) is a pretty niche use case / workaround for missing match functionality so I think this PR is probably fine. If anything we could add a test for the use case in the docs so we'd notice if we need to update them.

Yeah fair enough. Not sure if we have any guidelines on what to consider important to keep backwards compatible and what not to, so that's to blame haha
Whenever I personally make changes or review code I try to keep all APIs backwards compatible, but that may be exclusive to me and not really the team's preferences

I'll kick off a discussion around this in Slack so perhaps we can better define this :)

Thanks for the input guys!

Added s3_skip_encryption parameter to match

d7c7871

Furthermore fixed a bug in parameters added to for_storage_mode

Fixed tests for now.

0ea04c4

mbogh marked this pull request as draft January 30, 2023 11:07

mbogh added 2 commits February 7, 2023 13:29

Added tests for encryption for_storage_mode

001b67d

Forgot rubocop

7af388b

mbogh marked this pull request as ready for review February 7, 2023 12:42

mbogh changed the title ~~Draft: Added s3_skip_encryption parameter to match~~ Added s3_skip_encryption parameter to match Feb 7, 2023

mbogh changed the title ~~Added s3_skip_encryption parameter to match~~ [match] Added s3_skip_encryption parameter Feb 7, 2023

getaaron reviewed Mar 14, 2023

View reviewed changes

match/lib/match/options.rb Outdated Show resolved Hide resolved

getaaron reviewed Mar 14, 2023

View reviewed changes

Added warning to option

52507e5

Co-authored-by: Aaron Brager <getaaron@gmail.com>

for_mode to from_params

fd31159

mbogh requested a review from getaaron March 22, 2023 12:37

getaaron added 6 commits August 27, 2023 13:36

Merge branch 'master' into match-skip-encryption

f0f1e84

Fix GitLabSecureFiles config params

fd8a503

disable ClassLength rubocop on match/options.rb

b82a1a9

Remove no longer needed fields from importer spec assertion

a24fbc7

Remove trailing comma

2179f5e

Removed one more param from importer_spec

63b23ac

mbogh added 2 commits September 4, 2023 11:53

A bit of clean up should make the tests go green

4813d3d

One comma

6f7b56d

getaaron approved these changes Sep 4, 2023

View reviewed changes

getaaron merged commit 6627c28 into fastlane:master Sep 4, 2023
10 checks passed

mbogh deleted the match-skip-encryption branch September 4, 2023 21:17

joshdholtz mentioned this pull request Sep 16, 2023

Version bump to 2.215.0 #21517

Merged

darbyfrey mentioned this pull request Sep 18, 2023

[match] Fixes missing job-token error in GitLab storage mode #21520

Merged

6 tasks

arc-v2 mentioned this pull request Nov 9, 2023

fix Match::Storage docs to reflect latest API changes fastlane/docs#1225

Merged

rogerluan reviewed Nov 11, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[match] Added s3_skip_encryption parameter #21018

[match] Added s3_skip_encryption parameter #21018

mbogh commented Jan 30, 2023 •

edited

google-cla bot commented Jan 30, 2023

mbogh commented Mar 1, 2023

getaaron Mar 14, 2023 •

edited

mbogh Mar 14, 2023

mbogh Mar 14, 2023

mbogh Mar 14, 2023

getaaron commented Mar 14, 2023

mbogh commented Mar 14, 2023

mbogh commented Apr 13, 2023

mbogh commented May 22, 2023

giovannidegani commented May 22, 2023

mbogh commented Aug 7, 2023

getaaron commented Aug 27, 2023

darbyfrey commented Aug 30, 2023

getaaron commented Sep 3, 2023

mbogh commented Sep 4, 2023

mbogh commented Sep 4, 2023

getaaron commented Sep 4, 2023

mbogh commented Sep 4, 2023

MichalAlgor commented Sep 18, 2023 •

edited

getaaron commented Sep 18, 2023

darbyfrey commented Sep 18, 2023

MichalAlgor commented Sep 18, 2023

rogerluan Nov 11, 2023

mbogh Nov 11, 2023

getaaron Nov 11, 2023

rogerluan Nov 11, 2023

[match] Added s3_skip_encryption parameter #21018

[match] Added s3_skip_encryption parameter #21018

Conversation

mbogh commented Jan 30, 2023 • edited

Checklist

Motivation and Context

Description

Testing Steps

google-cla bot commented Jan 30, 2023

mbogh commented Mar 1, 2023

getaaron Mar 14, 2023 • edited

Choose a reason for hiding this comment

mbogh Mar 14, 2023

Choose a reason for hiding this comment

mbogh Mar 14, 2023

Choose a reason for hiding this comment

mbogh Mar 14, 2023

Choose a reason for hiding this comment

getaaron commented Mar 14, 2023

mbogh commented Mar 14, 2023

mbogh commented Apr 13, 2023

mbogh commented May 22, 2023

giovannidegani commented May 22, 2023

mbogh commented Aug 7, 2023

getaaron commented Aug 27, 2023

darbyfrey commented Aug 30, 2023

getaaron commented Sep 3, 2023

mbogh commented Sep 4, 2023

mbogh commented Sep 4, 2023

getaaron commented Sep 4, 2023

mbogh commented Sep 4, 2023

MichalAlgor commented Sep 18, 2023 • edited

getaaron commented Sep 18, 2023

darbyfrey commented Sep 18, 2023

MichalAlgor commented Sep 18, 2023

rogerluan Nov 11, 2023

Choose a reason for hiding this comment

mbogh Nov 11, 2023

Choose a reason for hiding this comment

getaaron Nov 11, 2023

Choose a reason for hiding this comment

rogerluan Nov 11, 2023

Choose a reason for hiding this comment

mbogh commented Jan 30, 2023 •

edited

getaaron Mar 14, 2023 •

edited

MichalAlgor commented Sep 18, 2023 •

edited