Encrypt/decrypt command #1127

fortmarek · 2020-03-21T16:27:51Z

Short description 📝

This PR is part of signing feature described here. In this PR I want to tackle two steps: encryption and decryption, to be exact implementing two commands: tuist signing encrypt and tuist signing decrypt

Solution 📦

The basic logic for these commands is actually quite trivial: find master.key in Tuist/Signing directory and encrypt all the files in that directory and its subdirectories. The same process but vice versa is applied for decryption.

I also needed to add some additional logic to support subcommands which SPM's parser does not out of the box.

Implementation 👩‍💻👨‍💻

When implementing this, I first used Apple's swift-crypto but unfortunately that requires macOS Catalina. I resorted to using CryptoSwift as a well-known alternative. We thought about using openssl and that'd create unnecessary overhead IMHO.

Encrypt command
Decrypt command
Tests

ollieatkinson

When running locally I was not sure if it worked, I did not get any feedback. Can you add some logging?

I agree with your implementation of CryptoSwift however OpenSSL would have not created overhead, we would likely either rely on the c-library or to shell out to the command line both of which are fairly trivial for what we need to do.

It's really nice to see where Tuist is going with these capabilities, it's removing some of the complexity we have to deal with. At the moment we use fastlane and match to manage the certificates, we can take some inspiration from those tools about furthering the capabilities of this.

Great work @fortmarek

ollieatkinson · 2020-03-23T20:20:29Z

Sources/TuistKit/Commands/CommandRegistry.swift

        let arguments = Array(processArguments().dropFirst())
        guard let argumentName = arguments.first else { return nil }
        let subparser = try parser.parse([argumentName]).subparser(parser)
        if let command = commands.first(where: { type(of: $0).command == subparser }) {
            return try command.parse(with: parser, arguments: arguments)
        }
-        return try parser.parse(arguments)
+        return (try parser.parse(arguments), parser)


parser is an instance on CommandRegistry, I'm not sure what the benefit of returning it here is?

SigningCommand returns a custom parser to make it easier to parse subcommands, I'll try to revisit it if we can make it work without creating a new one

Unfortunately, I could not do it -> the reason is because here I need to trigger a command which is found by subparser. subparser can be only one word, thus I need a custom parser that is tuist signing and then I can successfully find the subcommand.

I am not happy with this extra logic, but we should start using the new Swift argument parser sooner than later which supports subcommands out of the box, so maybe perfecting the mechanism might be lost work, so I'd prefer keeping it as it is.

ollieatkinson · 2020-03-23T20:23:22Z

Sources/TuistKit/Commands/CommandRegistry.swift

+        let allCommands = commands + commands.flatMap { $0.subcommands }
+        if let command = allCommands.first(where: { type(of: $0).command == subparser }) {


Why do we need to ask all commands for their subcommand?

I did not want to check here just for SigningCommand, though it would be possible

ollieatkinson · 2020-03-23T20:24:31Z

Sources/TuistKit/Commands/DecryptCommand.swift

+        pathArgument = subParser.add(option: "--path",
+                                     shortName: "-p",
+                                     kind: String.self,
+                                     usage: "The path to the folder where the template will be generated (Default: Current directory).",


This usage text is incorrect, how about:

The path to the folder containing the encrypted certificates

Copy & pasting programming

Busted 🚓 👮

Sources/TuistKit/Commands/EncryptCommand.swift

ollieatkinson · 2020-03-23T20:26:26Z

Sources/TuistSigning/SigningCipher.swift

+    var description: String {
+        switch self {
+        case .failedToEncrypt:
+            return "Encryption failed"


Can we provide more information to the end-user about why the encryption failed? This might help when debugging issues related to signing.

Agree. We should make it easier for users to understand what's happening and for us to help them debug the issues.

I have tried my best to describe what might have gone wrong

ollieatkinson · 2020-03-23T20:28:49Z

Sources/TuistSigning/SigningCipher.swift

+        let blockSize = 16
+        var iv = Data(repeating: 0, count: blockSize)
+        let result = iv.withUnsafeMutableBytes { bytes -> Int32 in
+            guard let baseAddress = bytes.baseAddress else { return 0 }


It would be better to crash/throw here then to generate a zero iv

Agreed 👍

ollieatkinson · 2020-03-23T20:31:55Z

Sources/TuistSigning/SigningCipher.swift

+            guard let baseAddress = bytes.baseAddress else { return 0 }
+            return SecRandomCopyBytes(kSecRandomDefault, blockSize, baseAddress)
+        }
+        guard result == errSecSuccess else { throw SigningCipherError.failedToEncrypt }


This is where you could pass the result, so we can print it out to the end-user. The error code might be good enough, but KeychainAccess went one step further and provided a description for each message - I will leave it up to you to consider.

I am trying to parse the error and if that fails, I print out the error code

ollieatkinson · 2020-03-23T20:35:05Z

Sources/TuistSigning/SigningCipher.swift

+            }
+
+        try zip(decipheredKeys, signingKeyFiles).forEach {
+            try $0.write(to: $1.url)


It might be a good idea to introduce some logging so that the user gets some feedback as the work for encrypt/decrypt is going on. You can introduce debug messages for more detailed logs which only get printed when --verbose is passed to any of the commands - this will help users of Tuist to debug what is wrong.

You're definitely right, I have not got used to using it just yet, but I definitely should since it might be crucial for us and for the end user!

ollieatkinson · 2020-03-23T20:43:34Z

Sources/TuistSigning/SigningCipher.swift

+
+    /// - Returns: Master key data
+    private func masterKey(from signingDirectory: AbsolutePath) throws -> Data {
+        let masterKeyFile = signingDirectory.appending(component: "master.key")


is it worth creating a constant for this, since it's used in two places and is critical to the algorithm

ollieatkinson · 2020-03-23T20:56:29Z

fixtures/ios_app_with_signing/Project.swift

+                                 bundleId: "io.tuist.SignApp",
+                                 infoPlist: "Info.plist",
+                                 sources: "App/**",
+                                 dependencies: []))


This project does not generate by default, there's an extra )

$ /usr/bin/xcrun swiftc --driver-mode=swift -suppress-warnings -I /Users/oliver/Library/Developer/Xcode/DerivedData/tuist-edomxougefoacwakknnztphpgmtm/Build/Products/Debug -L /Users/oliver/Library/Developer/Xcode/DerivedData/tuist-edomxougefoacwakknnztphpgmtm/Build/Products/Debug -F /Users/oliver/Library/Developer/Xcode/DerivedData/tuist-edomxougefoacwakknnztphpgmtm/Build/Products/Debug -lProjectDescription /Users/oliver/Development/Tuist/fixtures/ios_app_with_signing/Project.swift --tuist-dump /Users/oliver/Development/Tuist/fixtures/ios_app_with_signing/Project.swift:11:51: error: expected ',' separator dependencies: [])) ^ , /Users/oliver/Development/Tuist/fixtures/ios_app_with_signing/Project.swift:11:51: error: expected expression in container literal dependencies: [])) ^

Oops, forgot to check, good catch 👍 This should have a fixture test sooner than later to catch these as soon as possible

pepicrft

What are you eating lately @fortmarek 😜, you are full-speed shipping lots of great features nicely implemented. One scenario that popped in my mind and that it'd be great to handle is the following:

As a user, I generate and place the certificates and profiles under Tuist/Signing, and when I run encrypt, there's no master.key. I think in that case we should generate a random one and use that to encrypt the files.

Also, the other scenario that I'm thinking about is. What if I want to "vendor" some of the profiles and certificates that are already in my system. Where are the profiles? How do I take the certificate from my keychain? We could provide the following command:

tuist signing vendor

The command would run only if the user is running Tuist in a interactive shell and would do the following:

Get the list of signing certificates and ask the user for which ones they'd like to vendor.
Get the list of profiles and ask the user for which ones they'd like to vendor.
Place them under Tuist/Signing and encrypt them.
🤯

Sources/TuistKit/Commands/SigningCommand.swift

pepicrft · 2020-03-24T09:41:38Z

Sources/TuistKit/Commands/DecryptCommand.swift

+        pathArgument = subParser.add(option: "--path",
+                                     shortName: "-p",
+                                     kind: String.self,
+                                     usage: "The path to the folder where the template will be generated (Default: Current directory).",


Copy & pasting programming

pepicrft · 2020-03-24T09:43:23Z

Sources/TuistScaffold/TemplatesDirectoryLocator.swift

@@ -15,7 +15,7 @@ public final class TemplatesDirectoryLocator: TemplatesDirectoryLocating {
    private let rootDirectoryLocator: RootDirectoryLocating

    /// Default constructor.
-    public init(rootDirectoryLocator: RootDirectoryLocating = RootDirectoryLocator.shared) {


What about removing .shared from RootDirectoryLocator. In hindsight, I think it was a good idea introducing that there because that class holds state, and that might introduce side effects to both the code and the tests.

pepicrft · 2020-03-24T09:43:47Z

Sources/TuistSigning/SigningCipher.swift

+    var description: String {
+        switch self {
+        case .failedToEncrypt:
+            return "Encryption failed"


Agree. We should make it easier for users to understand what's happening and for us to help them debug the issues.

pepicrft · 2020-03-24T09:44:01Z

Sources/TuistSigning/SigningCipher.swift

+        case let .masterKeyNotFound(masterKeyPath):
+            return "Could not find master.key at \(masterKeyPath.pathString)"
+        case let .rootDirectoryNotFound(fromPath):
+            return "Could not find root directory from \(fromPath.pathString)"


What is root directory?

Changed to signingDirectory as that makes more sense in this context

pepicrft · 2020-03-24T09:47:36Z

Sources/TuistSigning/SigningCipher.swift

+        let signingDirectory = rootDirectory.appending(components: Constants.tuistDirectoryName, Constants.signingDirectoryName)
+        let masterKey = try self.masterKey(from: signingDirectory)
+        // Find all files in `signingDirectory` with the exception of "master.key"
+        let signingKeyFiles = FileHandler.shared.glob(signingDirectory, glob: "**/*")


I'd start by only encrypting/decrypting files under Signing. We want to be conventional about the structure of the directory and how the certificates and profiles are named and structure. If we follow the following convention there's no need to support subdirectories recursively:

Signing/ Debug.cert.enc Release.cert.enc MyApp.Debug.mobileprovision.enc MyWatchApp.Debug.mobileprovision.enc master.key

Actually... after giving it another thought, I think we master.key can be under Tuist. That way we can use the key in the future to encrypt/decrypt other things. Something along the lines of:

master.key Signing/ Debug.cert.enc Release.cert.enc MyApp.Debug.mobileprovision.enc MyWatchApp.Debug.mobileprovision.enc

Extract master.key and stopped parsing the subdirectories, your points make sense to me 👍

fortmarek · 2020-03-24T21:34:42Z

Thanks @ollie and @pepibumur for the comprehensive reviews! I hope I have resolved most of the points you have raised.

As for the feature suggestions of autogeneration of master key and tuist vendor (which sounds super exciting!) I have added them to the list here and we can discuss them there/in a different PR if that's OK with you.

I am stoked about what's next, the next PR for signing should be the most interesting one since it's where the logic of implementing signing comes in 👀

codecov · 2020-03-30T17:33:17Z

Codecov Report

Merging #1127 into master will not change coverage by %.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1127   +/-   ##
=======================================
  Coverage   76.88%   76.88%           
=======================================
  Files         273      273           
  Lines        9620     9620           
=======================================
  Hits         7396     7396           
  Misses       2224     2224

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 58f8107...58f8107. Read the comment docs.

pepicrft · 2020-03-31T18:10:01Z

Sources/TuistKit/Commands/SigningCommand.swift

+    // MARK: - Attributes
+
+    static let command = "signing"
+    static let overview = "Signing command"


Should we expand this a bit?:

A set of commands for signing-related operations.

Good thinking, will do 👍

pepicrft · 2020-03-31T18:10:44Z

Sources/TuistSigning/SigningCipher.swift

+    case masterKeyNotFound(AbsolutePath)
+    case signingDirectoryNotFound(AbsolutePath)
+
+    var type: ErrorType { .abort }


I'd implement all the cases. That way, if someone adds a new case in the future, they'll forced to think about the type.

This just declares what the ErrorType of SigningCipher is, I suppose a switch does not make sense in this case.

Right, but as a developers we need to think about what type of error we are throwing. If we don't use a switch clause and rather always return the same value, when I add a new value I won't be prompted to think about the type of error of the new case.

var type: ErrorType { switch self { ... } }

Oh sorry, you're definitely right, I kind of did not realize it makes sense to have different ErrorType for individual error cases 👍

pepicrft · 2020-03-31T18:12:36Z

Sources/TuistSigning/SigningCipher.swift

+        case let .failedToDecrypt(reason):
+            return "Could not decrypt data: \(reason)"
+        case let .ivGenerationFailed(reason):
+            return "Generation of IV failed with error: \(reason)"


Can users do something to mitigate this issue? If so, what about including those steps here?

This error should never occur - and honestly, if it does, I am not sure how I'd fix it 😞

That's fine! If this error is very unlikely, we don't need to put a lot of effort into figuring out how to handle it. If it turns out to be more likely, then it's worth the investigation. Thanks for clarifying it!

pepicrft · 2020-03-31T18:13:53Z

Sources/TuistSigning/SigningCipher.swift

+            let rootDirectory = rootDirectoryLocator.locate(from: path)
+        else { throw SigningCipherError.signingDirectoryNotFound(path) }
+        let signingDirectory = rootDirectory.appending(components: Constants.tuistDirectoryName, Constants.signingDirectoryName)
+        let masterKey = try self.masterKey(from: rootDirectory)


I'd require the key to be at /Tuist/master.key

pepicrft · 2020-03-31T18:18:19Z

@fortmarek I think this PR is in a good shape for a first iteration. There are some bits that I'd try to polish before making it official:

If master.key doesn't exist, we should create a random key automatically and let the users know.
I'd extend the init template to generate a random key when running tuist init.
I'd add the unencrypted files under the Tuist/Signing directory to the .gitignore file generated when running tuist init. I'd also add the master.key to the .gitignore.
What about deleting unencrypted files after encrypting them? I'd default to that and give developers the option to specify whether they don't want those files to be deleted: tuist signing encrypt --keep-files. I think it's good to default to deletion for safety reasons.
On CI we should read the master key from a environment variable, TUIST_MASTER_KEY

fortmarek · 2020-03-31T18:33:34Z

@pepibumur Thanks for your points! I agree that it needs some polish, before advertising I'd like to tackle how to use files in Signing directory in your Xcode project after which it will be a killer feature!

I agree with most of your points. Currently, the files are unencrypted in-place - that means we don't create new ones (so no files can be deleted either) - but I can see now that this might not be the ideal behavior since you could very easily mistakenly push unencrypted files to the remote repo. Would it make sense to encrypt files with .encrypted extension? That way everything in Signing directory can be in .gitignore with the exception of files with the aforementioned .encrypted extension.

But I'd still like to make the appropriate changes you suggested later - I think there might be more things that will need to be changed when I start working on integration with tuist generate anyway (but we'll see).

Marek Fořt added 15 commits March 5, 2020 21:02

Add Tuist signing fixture.

5d95571

Add SigningCipher.

064f55a

Decipher signing files.

df5d14b

Add warning if could not encrypt a file.

7512361

Change swift-crypto to CryptoSwift.

7cf75f3

Add SigningCipherTests.

d7f683d

Lower macOS version.

ca2cf49

Support subcommands for SigningCommand.

53a3585

Add run code for SigningCommand.

9b4de08

Merge remote-tracking branch 'origin/master' into signing

74ab15d

Merge remote-tracking branch 'origin/master' into signing

07aac1b

Add path arguments.

ed55321

Simplify fixture project for signing.

8ddd0e0

Run swiftformat.

68de668

Add documentation.

ca38803

fortmarek changed the title ~~Ciphering~~ Encrypt/decrypt command Mar 21, 2020

fortmarek requested review from pepicrft and ollieatkinson March 21, 2020 16:28

Marek Fořt added 3 commits March 21, 2020 19:13

Remove RootDirectoryLocator as shared instance.

7d51574

Run swiftformat.

858a9e8

Add TuistCoreTesting as a dependency of TuistSigningTests.

c6254b0

fortmarek self-assigned this Mar 22, 2020

ollieatkinson requested changes Mar 23, 2020

View reviewed changes

ollieatkinson reviewed Mar 23, 2020

View reviewed changes

pepicrft reviewed Mar 24, 2020

View reviewed changes

Error logging, change master.key location.

31e7ada

fortmarek mentioned this pull request Mar 24, 2020

WIP: Signing #1066

Closed

10 tasks

Marek Fořt added 3 commits March 24, 2020 22:12

Fix GeneratorPathsTests.

3859f0f

Merge branch 'signing' into ciphering

b7c07a8

Better error description.

e25e62c

Marek Fořt added 2 commits March 24, 2020 22:25

Print error on ivGenerationFailed.

899fc07

Merge remote-tracking branch 'origin/master' into ciphering

cb273f9

Marek Fořt added 3 commits March 25, 2020 17:39

Fix scaffold parse function signature.

49105e2

Edit changelog.

093981b

Fix scaffold command test.

2758aab

fortmarek requested review from pepicrft and ollieatkinson March 25, 2020 21:52

Merge branch 'master' into ciphering

8a4649c

pepicrft reviewed Mar 31, 2020

View reviewed changes

pepicrft approved these changes Mar 31, 2020

View reviewed changes

Marek Fořt added 4 commits March 31, 2020 21:28

Change master.key location.

16f320c

Add switch to error type.

07401e6

Expand SigningCommand overview.

e060788

Merge remote-tracking branch 'origin/master' into ciphering

58f8107

fortmarek merged commit 65c7f26 into master Mar 31, 2020

fortmarek mentioned this pull request Apr 8, 2020

Signing install #1186

Merged

4 tasks

natanrolnik deleted the ciphering branch June 17, 2020 16:31

		let allCommands = commands + commands.flatMap { $0.subcommands }
		if let command = allCommands.first(where: { type(of: $0).command == subparser }) {

Encrypt/decrypt command #1127

Encrypt/decrypt command #1127

Conversation

fortmarek commented Mar 21, 2020

Short description 📝

Solution 📦

Implementation 👩‍💻👨‍💻

ollieatkinson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fortmarek Mar 24, 2020 • edited Loading

Choose a reason for hiding this comment

pepicrft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fortmarek commented Mar 24, 2020

codecov bot commented Mar 30, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pepicrft commented Mar 31, 2020 • edited Loading

fortmarek commented Mar 31, 2020

fortmarek Mar 24, 2020 •

edited

Loading

codecov bot commented Mar 30, 2020 •

edited

Loading

pepicrft commented Mar 31, 2020 •

edited

Loading