[Valet 4.0] Allow kSecAttrService to be a customer-friendly string on Mac by dfed · Pull Request #154 · square/Valet

dfed · 2018-10-02T06:44:31Z

Addresses #140. Note that I haven't added unit tests or README documentation for these cases yet, so this PR is WIP. Before I commit to writing unit tests and README docs, I'm curious what the reaction is to adding this API, and I'd also like feedback on how we can improve the API documentation to make it clear what is going on here. Note that by explicitly setting the kSecAttrService, each Valet instance is no longer guaranteed to be a sandbox, unless the kSecAttrService is unique to the organization. Since this API is dangerous, I've only added it to the macOS target.

cc @abey, @antons, and @gregcotten

dfed · 2018-10-02T06:45:17Z

Note that I'll probably want to tackle #123 before this merges.

gregcotten · 2019-02-01T00:47:59Z

This all looks good to me!

dfed · 2019-05-25T04:42:01Z

I've updated the README to include information on this new API. Note that getting this API to compile will require dropping Xcode 9 support. I'm not yet willing to do that, so this PR will stay open until we have more we'd want to roll into a breaking change.

fdiaz · 2020-01-16T02:04:58Z

README.md

-The identifier you choose for your Valet is used to create a sandbox for the data your Valet writes to the keychain. Two Valets of the same type created via the same initializer, accessibility value, and identifier will be able to read and write the same key:value pairs; Valets with different identifiers each have their own sandbox. Choose an identifier that describes the kind of data your Valet will protect. You do not need to include your application name or bundleIdentifier in your Valet’s identifier.
+The identifier you choose for your Valet is used to create a sandbox for the data your Valet writes to the keychain. Two Valets of the same type created via the same initializer, accessibility value, and identifier will be able to read and write the same key:value pairs; Valets with different identifiers each have their own sandbox. Choose an identifier that describes the kind of data your Valet will protect. You do not need to include your application name or bundle identifier in your Valet’s identifier.
+
+#### Choosing a User-friendly Identifier on macOS


nit: Should we have some type ⚠️ or an asterisk in the title to make sure it's explicit that this breaks Valet sandbox?

fdiaz · 2020-01-16T03:25:36Z

README.md

+
+```swift
+let mySecureEnclaveValet = Valet.valet(withExplicitlySet: Identifier(nonEmpty: "Druidia")!, accessibility: .whenUnlocked)
+```


nit: Let's add an Obj-C example

fdiaz · 2020-01-16T03:32:12Z

Sources/Valet/SecureEnclave.swift

+        #endif
        case .standard:
            noPromptValet = .valet(with: identifier, accessibility: .whenPasscodeSetThisDeviceOnly)
+            #if os(macOS)


nit: This is not correctly indented

fdiaz · 2020-01-16T03:33:26Z

README.md

+let mySecureEnclaveValet = Valet.valet(withExplicitlySet: Identifier(nonEmpty: "Druidia")!, accessibility: .whenUnlocked)
+```
+
+Mac apps signed with a developer ID may see their Valet’s identifier [shown to their users](https://github.com/square/Valet/issues/140). While it is possible to explicitly set a user-friendly identifier, note that doing so bypasses this project’s guarantee that one Valet type will not have access to one another type’s key:value pairs. To maintain this guarantee, ensure that each Valet’s identifier is globally unique.


I think providing an example of what types of Valets will be sharing the same key:value pairs would be useful here.

Literally any with the same identifier, hence the suggestion to make the identifier globally unique.

fdiaz · 2020-01-16T03:36:37Z

Sources/Valet/Valet.swift

+    /// - parameter identifier: A non-empty string that must correspond with the value for keychain-access-groups in your Entitlements file. Must be unique relative to other Valet identifiers.
+    /// - parameter accessibility: The desired accessibility for the Valet.
+    /// - returns: A Valet (synchronized with iCloud) that reads/writes keychain elements that can be shared across applications written by the same development team.
+    public class func iCloudSharedAccessGroupValet(withExplicitlySet identifier: Identifier, accessibility: CloudAccessibility) -> Valet {


I'm guessing all of these are class and not static for Obj-c interoperability?

I think I'm using class because of a decision I made years ago where I thought it was nicer? I'll do a separate PR to use static, since we're not using subclassing here.

fdiaz · 2020-01-16T03:47:19Z

Sources/Valet/Valet.swift

+        }
+        return findOrCreate(explicitlySet: identifier, configuration: .iCloud(accessibility), sharedAccessGroup: true)
+    }
+    #endif


Just for my understanding: all of this is also for Obj-C interoperability given structs are not exposed?

exactly. Identifier is not Objective-C compatible. Given that we're making breaking changes, I'm open to changing this up if you've got better ideas 🙂

fdiaz · 2020-01-16T03:49:28Z

Sources/Valet/Valet.swift

+    class func permutations(withExplictlySet identifier: Identifier, shared: Bool = false) -> [Valet] {
+        return Accessibility.allValues().map { accessibility in
+            let valet: Valet = shared ? .sharedAccessGroupValet(withExplicitlySet: identifier, accessibility: accessibility) : .valet(withExplicitlySet: identifier, accessibility: accessibility)
+            return valet


I'm somewhat surprised Swift can't infer this?

nit, but do you think making this closure be explicitly

accessibility -> Valet

so this can be a one-liner makes this nicer?

yeah let's make it a one-liner. It looks like in the year since this PR started I made the other methods (above) one-liners 😅

fdiaz · 2020-01-16T03:50:06Z

Sources/Valet/Valet.swift

+        }
+    }
+
+    class func iCloudPermutations(withExplictlySet identifier: Identifier, shared: Bool = false) -> [Valet] {


I think you might be missing marking this explicitly as internal on this one and on the top

This is all in internal extension Valet 🙂

fdiaz · 2020-01-16T03:52:08Z

Tests/ValetIntegrationTests/MacTests.swift

        // This is not be the case upon setting a breakpoint and inspecting before the valet.setString(, forKey:) call above.
    }

+    func test_withExplicitlySet_identifierHasExplicitlySetIdentifier() {


nit: identifierHasExplicitlySetIdentifier -> assignsExplicitIdentifier

fdiaz · 2020-01-16T03:53:23Z

Tests/ValetIntegrationTests/MacTests.swift

+
+        Valet.iCloudPermutations(withExplictlySet: explicitlySetIdentifier, shared: false).forEach {
+            XCTAssertEqual($0.keychainQuery?[kSecAttrService as String], explicitlySetIdentifier.description)
+        }


This is (probably?) a nit, but should these be 2 tests? We're testing the same thing but on different contexts.

I mean, this could be 4 tests. But I don't gain a lot by splitting this up?

I'd indeed do 4 tests here 😁

We mostly gain the ability that when a test fails, we immediately know what assert is causing it (since we'd have 1 per test)

I'm a fan of that. I think using XCTest doesn't play that well with this though since you need to write a whole new test for that, but I also think this is fine though 🤷‍♂

I think another solve for that is writing good descriptions for the failure case. In this case, I have 4 classes of tests, each with Accessibility.allValues().count asserts. That's... a lot of tests to write. And if we ever add a new accessibility value, it'll get worse.

Let me touch this up by adding some failure strings.

Fwiw, when one of these tests failed earlier in CI, I looked up the line number of the failure. That worked well enough 🤷‍♂

codecov-io · 2020-01-16T06:41:49Z

Codecov Report

Merging #154 into develop--4.0 will increase coverage by 0.19%.
The diff coverage is 71.42%.

@@               Coverage Diff                @@
##           develop--4.0     #154      +/-   ##
================================================
+ Coverage         75.37%   75.57%   +0.19%     
================================================
  Files                14       14              
  Lines              1206     1269      +63     
================================================
+ Hits                909      959      +50     
- Misses              297      310      +13

Impacted Files	Coverage Δ
Sources/Valet/Internal/Keychain.swift	`89.19% <100%> (+1.04%)`	⬆️
Sources/Valet/Valet.swift	`75.3% <100%> (-4.14%)`	⬇️
Sources/Valet/SecureEnclaveValet.swift	`52.99% <50%> (ø)`	⬆️
Sources/Valet/SecureEnclave.swift	`62.16% <50%> (ø)`	⬆️
Sources/Valet/SinglePromptSecureEnclaveValet.swift	`54.74% <50%> (ø)`	⬆️
Sources/Valet/Internal/Service.swift	`100% <0%> (+2.29%)`	⬆️
Sources/Valet/Internal/SecItem.swift	`83.66% <0%> (+3.26%)`	⬆️

…iguration, accessibility, and sharedAccessGroup to prevent returning the wrong Valet

… access groups

dfed · 2020-01-17T02:19:55Z

The lower test coverage mostly comes from the objc-compatibility layer not being tested. I currently have no test coverage on the compatibility layer. I need to fix that, but that's for another PR.

dfed requested review from EricMuller22, NickEntin and gfontenot October 2, 2018 06:44

dfed force-pushed the dfed--mac-friendly-service branch from fadccad to 1a1d23c Compare October 2, 2018 20:11

dfed force-pushed the dfed--mac-friendly-service branch from 1a1d23c to 64979d4 Compare May 25, 2019 04:06

dfed changed the base branch from master to dfed--simplify-ci-v4 December 22, 2019 08:45

dfed changed the title ~~[WIP] Allow kSecAttrService to be a customer-friendly string on Mac~~ [WIP][Valet 4.0] Allow kSecAttrService to be a customer-friendly string on Mac Dec 22, 2019

dfed force-pushed the dfed--mac-friendly-service branch from 0c9efbe to b03a958 Compare December 23, 2019 00:08

dfed changed the title ~~[WIP][Valet 4.0] Allow kSecAttrService to be a customer-friendly string on Mac~~ [Valet 4.0] Allow kSecAttrService to be a customer-friendly string on Mac Dec 23, 2019

dfed force-pushed the dfed--simplify-ci-v4 branch from 921aa52 to f087d9e Compare December 23, 2019 00:14

dfed force-pushed the dfed--mac-friendly-service branch from 51caa23 to f009f7b Compare December 23, 2019 06:23

dfed force-pushed the dfed--simplify-ci-v4 branch from f087d9e to e9848e0 Compare December 23, 2019 20:07

dfed force-pushed the dfed--mac-friendly-service branch 2 times, most recently from d00a634 to 100df92 Compare December 24, 2019 03:28

dfed changed the base branch from dfed--simplify-ci-v4 to develop--4.0 December 24, 2019 03:28

dfed force-pushed the dfed--mac-friendly-service branch from 100df92 to ebf1e11 Compare December 24, 2019 03:30

dfed and others added 8 commits December 24, 2019 14:06

Allow kSecAttrService to be a customer-friendly string on Mac

66f929f

Fix tests

f77050e

Add a section to the README on choosing the best identifier on a Mac

0507ef7

Update README documentation on choosing a user-friendly identifier

42ea7e8

Start working on tests

6fc4de7

Add tests

f2d2f5a

better README

0cc99f9

Add ObjC compat layer for new initializers

0bb4fa1

dfed force-pushed the dfed--mac-friendly-service branch from ebf1e11 to 0bb4fa1 Compare December 24, 2019 22:06

dfed added 2 commits December 29, 2019 19:56

Merge branch 'develop--4.0' into dfed--mac-friendly-service

ccf184a

Merge branch 'develop--4.0' into dfed--mac-friendly-service

2c3655b

Merge conflict

6dc319b

dfed requested a review from fdiaz January 13, 2020 19:57

dfed added 3 commits January 14, 2020 15:36

Merge branch 'develop--4.0' into dfed--mac-friendly-service

e620d33

Merge branch 'develop--4.0' into dfed--mac-friendly-service

577ffb8

Merge branch 'develop--4.0' into dfed--mac-friendly-service

81eb4ac

fdiaz approved these changes Jan 16, 2020

View reviewed changes

dfed added 14 commits January 15, 2020 20:50

indentation

3bfa061

Use SeeAlso

d42814b

Modernize doc comments

84c4b70

Add Warning

ed45a2c

Remove comment which elucidates little

c5f8a10

one-line it

c422a91

Add objc example

1772731

Better test name

d180af5

Add ⚠️

6605668

Remove duplicative test line

7b7c482

testEnvironmentIsSigned

1427989

Remove all objects between tests

259983f

Merge branch 'develop--4.0' into dfed--mac-friendly-service

1d032f8

Fix merge issue

7f6bbe7

dfed added 6 commits January 15, 2020 22:57

findOrCreate(explicitlySet must use a key that combines service, conf…

08da1d1

…iguration, accessibility, and sharedAccessGroup to prevent returning the wrong Valet

Utilize testEnvironmentIsSigned before using shared keychains

33688e2

Use explicitlySetSharedAccessGroupIdentifier when dealing with shared…

6b4781a

… access groups

removeAllObjects() to avoid collisions in tests

23733cc

More tests

fc14a7e

Better test failure logs

610acbf

dfed merged commit d23922e into develop--4.0 Jan 17, 2020

dfed deleted the dfed--mac-friendly-service branch January 17, 2020 02:20

Conversation

dfed commented Oct 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dfed commented Oct 2, 2018

Uh oh!

gregcotten commented Feb 1, 2019

Uh oh!

dfed commented May 25, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Jan 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dfed commented Jan 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dfed commented Oct 2, 2018 •

edited

Loading

codecov-io commented Jan 16, 2020 •

edited

Loading