Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[match] Added s3_skip_encryption parameter #21018

Merged
merged 14 commits into from
Sep 4, 2023
Merged

[match] Added s3_skip_encryption parameter #21018

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
d7c7871
Added s3_skip_encryption parameter to match
mbogh Jan 23, 2023
0ea04c4
Fixed tests for now.
mbogh Jan 30, 2023
001b67d
Added tests for encryption for_storage_mode
mbogh Feb 7, 2023
7af388b
Forgot rubocop
mbogh Feb 7, 2023
52507e5
Added warning to option
mbogh Mar 14, 2023
fd31159
for_mode to from_params
mbogh Mar 14, 2023
f0f1e84
Merge branch 'master' into match-skip-encryption
getaaron Aug 27, 2023
fd8a503
Fix GitLabSecureFiles config params
getaaron Aug 27, 2023
b82a1a9
disable ClassLength rubocop on match/options.rb
getaaron Aug 27, 2023
a24fbc7
Remove no longer needed fields from importer spec assertion
getaaron Aug 27, 2023
2179f5e
Remove trailing comma
getaaron Aug 27, 2023
63b23ac
Removed one more param from importer_spec
getaaron Aug 27, 2023
4813d3d
A bit of clean up should make the tests go green
mbogh Sep 4, 2023
6f7b56d
One comma
mbogh Sep 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions match/lib/match/change_password.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ def self.update(params: nil)

encryption = Encryption.for_storage_mode(params[:storage_mode], {
git_url: params[:git_url],
s3_bucket: params[:s3_bucket],
s3_skip_encryption: params[:s3_skip_encryption],
working_directory: storage.working_directory
})
encryption.decrypt_files
Expand Down
2 changes: 2 additions & 0 deletions match/lib/match/commands_generator.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,8 @@ def run

encryption = Encryption.for_storage_mode(params[:storage_mode], {
git_url: params[:git_url],
s3_bucket: params[:s3_bucket],
s3_skip_encryption: params[:s3_skip_encryption],
working_directory: storage.working_directory
})
encryption.decrypt_files if encryption
Expand Down
2 changes: 1 addition & 1 deletion match/lib/match/encryption.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ def backends
},
"s3" => lambda { |params|
params[:keychain_name] = params[:s3_bucket]
return Encryption::OpenSSL.configure(params)
return params[:s3_skip_encryption] ? nil : Encryption::OpenSSL.configure(params)
},
"gitlab_secure_files" => lambda { |params|
return nil
Expand Down
3 changes: 3 additions & 0 deletions match/lib/match/importer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ def import_cert(params, cert_path: nil, p12_path: nil, profile_path: nil)
s3_access_key: params[:s3_access_key],
s3_secret_access_key: params[:s3_secret_access_key],
s3_object_prefix: params[:s3_object_prefix],
s3_skip_encryption: params[:s3_skip_encryption],
gitlab_project: params[:gitlab_project],
readonly: params[:readonly],
username: params[:username],
Expand All @@ -50,6 +51,8 @@ def import_cert(params, cert_path: nil, p12_path: nil, profile_path: nil)
# Encryption
encryption = Encryption.for_storage_mode(params[:storage_mode], {
git_url: params[:git_url],
s3_bucket: params[:s3_bucket],
s3_skip_encryption: params[:s3_skip_encryption],
working_directory: storage.working_directory
})
encryption.decrypt_files if encryption
Expand Down
2 changes: 2 additions & 0 deletions match/lib/match/migrate.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ def migrate(params)

encryption = Encryption.for_storage_mode(params[:storage_mode], {
git_url: params[:git_url],
s3_bucket: params[:s3_bucket],
s3_skip_encryption: params[:s3_skip_encryption],
working_directory: git_storage.working_directory
})
encryption.decrypt_files if encryption
Expand Down
3 changes: 3 additions & 0 deletions match/lib/match/nuke.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
})
Copy link
Collaborator

@getaaron getaaron Mar 14, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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})

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Expand All @@ -63,6 +64,8 @@ def run(params, type: nil)
# After the download was complete
self.encryption = Encryption.for_storage_mode(params[:storage_mode], {
git_url: params[:git_url],
s3_bucket: params[:s3_bucket],
s3_skip_encryption: params[:s3_skip_encryption],
working_directory: storage.working_directory
})
self.encryption.decrypt_files if self.encryption
Expand Down
5 changes: 5 additions & 0 deletions match/lib/match/options.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,11 @@ def self.available_options
env_name: "MATCH_S3_OBJECT_PREFIX",
description: "Prefix to be used on all objects uploaded to S3",
optional: true),
FastlaneCore::ConfigItem.new(key: :s3_skip_encryption,
env_name: "MATCH_S3_SKIP_ENCRYPTION",
description: "Skip encryption of all objects uploaded to S3",
mbogh marked this conversation as resolved.
Show resolved Hide resolved
type: Boolean,
default_value: false),

# Storage: GitLab Secure Files
FastlaneCore::ConfigItem.new(key: :gitlab_project,
Expand Down
3 changes: 3 additions & 0 deletions match/lib/match/runner.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ def run(params)
s3_secret_access_key: params[:s3_secret_access_key],
s3_bucket: params[:s3_bucket],
s3_object_prefix: params[:s3_object_prefix],
s3_skip_encryption: params[:s3_skip_encryption],
gitlab_project: params[:gitlab_project],
readonly: params[:readonly],
username: params[:readonly] ? nil : params[:username], # only pass username if not readonly
Expand All @@ -67,6 +68,8 @@ def run(params)
# Init the encryption only after the `storage.download` was called to have the right working directory
encryption = Encryption.for_storage_mode(params[:storage_mode], {
git_url: params[:git_url],
s3_bucket: params[:s3_bucket],
s3_skip_encryption: params[:s3_skip_encryption],
working_directory: storage.working_directory
})
encryption.decrypt_files if encryption
Expand Down
2 changes: 2 additions & 0 deletions match/spec/commands_generator_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,8 @@ def expect_githelper_clone_with(git_url, shallow_clone, git_branch)

expect(Match::Encryption).to receive(:for_storage_mode).with("git", {
git_url: git_url,
s3_bucket: nil,
s3_skip_encryption: false,
working_directory: fake_working_directory
}).and_return(fake_encryption)

Expand Down
78 changes: 78 additions & 0 deletions match/spec/encryption_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
describe Match::Encryption do
describe "for_storage_mode" do
it "returns nil if storage mode is google_cloud" do
storage_mode = "google_cloud"

encryption = Match::Encryption.for_storage_mode(storage_mode, {
git_url: "",
s3_bucket: "",
s3_skip_encryption: false,
working_directory: ""
})

expect(encryption).to be_nil
end

it "returns nil if storage mode is gitlab_secure_files" do
storage_mode = "gitlab_secure_files"

encryption = Match::Encryption.for_storage_mode(storage_mode, {
git_url: "",
s3_bucket: "",
s3_skip_encryption: false,
working_directory: ""
})

expect(encryption).to be_nil
end

it "returns nil if storage mode is s3 and skip encryption is true" do
storage_mode = "s3"

encryption = Match::Encryption.for_storage_mode(storage_mode, {
git_url: "",
s3_bucket: "my-bucket",
s3_skip_encryption: true,
working_directory: ""
})

expect(encryption).to be_nil
end

it "should return OpenSSL object for storage mode git" do
storage_mode = "git"
git_url = "git@github.com:you/your_repo.git"
working_directory = "my-workding-directory"

encryption = Match::Encryption.for_storage_mode(storage_mode, {
git_url: git_url,
s3_bucket: "",
s3_skip_encryption: false,
working_directory: working_directory
})

expect(encryption).to_not(be_nil)
expect(encryption).to be_kind_of(Match::Encryption::OpenSSL)
expect(encryption.keychain_name).to be(git_url)
expect(encryption.working_directory).to be(working_directory)
end

it "should return OpenSSL object for storage mode s3 and skip encryption is false" do
storage_mode = "s3"
s3_bucket = "my-bucket"
working_directory = "my-workding-directory"

encryption = Match::Encryption.for_storage_mode(storage_mode, {
git_url: "",
s3_bucket: s3_bucket,
s3_skip_encryption: false,
working_directory: working_directory
})

expect(encryption).to_not(be_nil)
expect(encryption).to be_kind_of(Match::Encryption::OpenSSL)
expect(encryption.keychain_name).to be(s3_bucket)
expect(encryption.working_directory).to be(working_directory)
end
end
end
1 change: 1 addition & 0 deletions match/spec/importer_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ def setup_fake_storage(repo_dir, config)
s3_access_key: nil,
s3_secret_access_key: nil,
s3_object_prefix: nil,
s3_skip_encryption: false,
gitlab_project: nil,
readonly: false,
username: config[:username],
Expand Down
1 change: 1 addition & 0 deletions match/spec/nuke_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
s3_secret_access_key: "",
s3_bucket: "",
s3_object_prefix: "",
s3_skip_encryption: false,
gitlab_project: nil,
team_id: nil
).and_return(fake_storage)
Expand Down
4 changes: 4 additions & 0 deletions match/spec/runner_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@
s3_secret_access_key: nil,
s3_bucket: nil,
s3_object_prefix: nil,
s3_skip_encryption: false,
gitlab_project: nil,
readonly: false,
username: values[:username],
Expand Down Expand Up @@ -153,6 +154,7 @@
s3_secret_access_key: nil,
s3_bucket: nil,
s3_object_prefix: nil,
s3_skip_encryption: false,
gitlab_project: nil,
readonly: false,
username: values[:username],
Expand Down Expand Up @@ -243,6 +245,7 @@
s3_secret_access_key: nil,
s3_bucket: nil,
s3_object_prefix: nil,
s3_skip_encryption: false,
gitlab_project: nil,
readonly: false,
username: values[:username],
Expand Down Expand Up @@ -315,6 +318,7 @@
s3_secret_access_key: nil,
s3_bucket: nil,
s3_object_prefix: nil,
s3_skip_encryption: false,
gitlab_project: nil,
readonly: false,
username: values[:username],
Expand Down