Remove SinglePromptSecureEnclaveValet from tvOS targets by diogot · Pull Request #284 · square/Valet

diogot · 2022-08-12T17:59:05Z

LAContext is not available in tvOS, but for some reason it was compiling in older Xcode versions.

CLAassistant · 2022-08-12T17:59:09Z

All committers have signed the CLA.

NickEntin · 2022-08-12T22:04:01Z

Sources/Valet/SinglePromptSecureEnclaveValet.swift

-        keychainQuery[kSecUseAuthenticationContext as String] = localAuthenticationContext
-        return keychainQuery
+        #if os(tvOS)
+            // tvOS doesn't support LAContext


I'm assuming omitting the authentication context won't change any behavior. Presumably this never worked on tvOS, but I don't know if the presence of the key affects behavior. cc @dfed

So I think we need to make the entire SinglePromptSecureEnclaveValet unavailable on tvOS.

The approach taken here will make this type behave like the SecureEnclaveValet, which isn't what we want.

The more I think about this, the more I think we make this a breaking change and roll a major version. We may also need a way to let folk on tvOS using this type migrate to SecureEnclaveValet, but given that no one has told us this wasn't working I kinda doubt anyone tried to use it that way... so maybe we don't build that until someone asks for it.

Taking the route of making SinglePromptSecureEnclaveValet unavailable on tvOS will also force us to solve #270, which is kinda a nice bonus.

I agree, I'll update the PR to make SinglePromptSecureEnclaveValet unavailable on tvOS.

codecov-commenter · 2022-08-12T22:05:50Z

Codecov Report

Merging #284 (ac8235f) into master (53f25b8) will decrease coverage by 0.10%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #284      +/-   ##
==========================================
- Coverage   86.81%   86.71%   -0.11%     
==========================================
  Files          16       16              
  Lines         986      986              
==========================================
- Hits          856      855       -1     
- Misses        130      131       +1

Impacted Files	Coverage Δ
Sources/Valet/SinglePromptSecureEnclaveValet.swift	`75.75% <ø> (ø)`
Sources/Valet/Valet.swift	`94.05% <0.00%> (-0.50%)`	⬇️

dfed · 2022-08-15T14:19:05Z

This is a good start! To get CI passing you'll likely need to update the availability checks in Tests/ValetIntegrationTests/SinglePromptSecureEnclaveIntegrationTests.Swift

diogot · 2022-08-15T15:25:10Z

Just marking as SinglePromptSecureEnclaveValet as @available(tvOS, unavailable) is not enough to compile the project with Xcode 13.4.1, we still need the other checks as the first version.
So to make it compile with 13.4.1 and 14 I might need to keep all the checks.

Signed-off-by: Diogo Tridapalli <diogot@users.noreply.github.com>

diogot · 2022-08-16T19:24:19Z

New approach, remove the classes from tvOS using #if !os(tvOS).
Not it compiles fine in Xcode 13 and 14.
All tests pass with Xcode 14, but a few in SecureEnclaveIntegrationTests fail in Xcode 13 and I don't know why.
I may need some help to fix that.

diogot · 2022-08-16T19:35:56Z

They are failing with KeychainError status: errSecAuthFailed

dfed · 2022-08-17T14:22:29Z

New approach, remove the classes from tvOS using #if !os(tvOS).
Not it compiles fine in Xcode 13 and 14.
All tests pass with Xcode 14, but a few in SecureEnclaveIntegrationTests fail in Xcode 13 and I don't know why.
I may need some help to fix that.

Which tests are failing for you? Are these tests failing for you on master as well, or is the failure new to your branch? Getting the test setup right locally can be relatively tricky given the code-signing requirements. Are you running these tests on a simulator, iOS device, or Mac? What OS version is the test target running on?

Good news is CI is passing, but there are some tests we don't run in CI because CI can't properly test some keychain interactions without some manual intervention 😬

dfed

This is looking really good! Some suggestions below. Let's figure out these test failures you are seeing locally, but I think we're darned close.

Sources/Valet/SinglePromptSecureEnclaveValet.swift

Tests/ValetIntegrationTests/SinglePromptSecureEnclaveIntegrationTests.swift

dfed · 2022-08-17T14:14:53Z

Tests/ValetIntegrationTests/SinglePromptSecureEnclaveIntegrationTests.swift

 {
    static let identifier = Identifier(nonEmpty: "valet_testing")!

-    @available(tvOS 11.0, *)


so good to be able to get rid of these!

dfed · 2022-08-17T14:15:32Z

Tests/ValetIntegrationTests/SinglePromptSecureEnclaveIntegrationTests.swift

-        guard #available(tvOS 11.0, *) else {
-            return
-        }


"Fun" fact: Marking a test as @available does not prevent that test from being run on unavailable OS versions 😬, hence the need for all of these guards

I figure that out, hehe

Tests/ValetIntegrationTests/SinglePromptSecureEnclaveIntegrationTests.swift

Tests/ValetObjectiveCBridgeTests/VALSinglePromptSecureEnclaveValetTests.m

dfed · 2022-08-17T14:17:07Z

Tests/ValetObjectiveCBridgeTests/VALSinglePromptSecureEnclaveValetTests.m

-    if (@available(tvOS 11.0, *)) {
-        VALSinglePromptSecureEnclaveValet *const valet = [VALSinglePromptSecureEnclaveValet valetWithIdentifier:self.identifier accessControl:VALSecureEnclaveAccessControlDevicePasscode];
-        XCTAssertEqual(valet.accessControl, VALSecureEnclaveAccessControlDevicePasscode);
-        XCTAssertEqual([valet class], [VALSinglePromptSecureEnclaveValet class]);
-    }
+    VALSinglePromptSecureEnclaveValet *const valet = [VALSinglePromptSecureEnclaveValet valetWithIdentifier:self.identifier accessControl:VALSecureEnclaveAccessControlDevicePasscode];
+    XCTAssertEqual(valet.accessControl, VALSecureEnclaveAccessControlDevicePasscode);
+    XCTAssertEqual([valet class], [VALSinglePromptSecureEnclaveValet class]);


Holy moly this is nicer!

Tests/ValetTests/SinglePromptSecureEnclaveTests.swift

Co-authored-by: Dan Federman <dfed@me.com> Signed-off-by: Diogo Tridapalli <diogot@users.noreply.github.com>

diogot · 2022-08-17T14:35:59Z

The tests are also failing in master, tvOS and iOS 15.5 simulators :

test_secureEnclaveValetsWithEqualConfiguration_canAccessSameData
test_secureEnclaveValetsWithDifferingAccessControl_canNotAccessSameData
test_migrateObjectsFromValet_migratesSuccessfullyToSecureEnclave
test_migrateObjectsFromValet_migratesSuccessfullyAfterCanAccessKeychainCalls

dfed · 2022-08-17T15:09:45Z

The tests are also failing in master, tvOS and iOS 15.5 simulators :

test_secureEnclaveValetsWithEqualConfiguration_canAccessSameData

test_secureEnclaveValetsWithDifferingAccessControl_canNotAccessSameData

test_migrateObjectsFromValet_migratesSuccessfullyToSecureEnclave

test_migrateObjectsFromValet_migratesSuccessfullyAfterCanAccessKeychainCalls

That makes sense. I think this method needs to be updated to include later OS versions:

Valet/Tests/ValetIntegrationTests/ValetIntegrationTests.swift

Lines 39 to 52 in 53f25b8

    
           func testEnvironmentSupportsWhenPasscodeSet() -> Bool { 
        
               if let simulatorVersionInfo = ProcessInfo.processInfo.environment["SIMULATOR_VERSION_INFO"], 
        
                   simulatorVersionInfo.contains("iOS 13") || simulatorVersionInfo.contains("tvOS 13") 
        
               { 
        
                   // iOS and tvOS 13 simulators fail to store items in a Valet that has a 
        
                   // kSecAttrAccessibleWhenPasscodeSetThisDeviceOnly flag. The documentation for this flag says: 
        
                   // "No items can be stored in this class on devices without a passcode". I currently do not 
        
                   // understand why prior simulators work with this flag, given that no simulators have a passcode. 
        
                   return false 
        
               } else { 
        
                   return true 
        
               } 
        
           }

Note that these tests are failing on my iOS 15 simulator as well. The good news is it works on device. I'll put up a PR to fix this. In the interim, I think this PR is good to merge!

diogot · 2022-08-17T15:26:22Z

Thanks!!

diogot mentioned this pull request Aug 12, 2022

tvOS target doesn't compile with Xcode 14.0 beta #283

Closed

NickEntin approved these changes Aug 12, 2022

View reviewed changes

NickEntin reviewed Aug 12, 2022

View reviewed changes

diogot force-pushed the xcode-14-tvos-fix branch from 7a22eea to ac8235f Compare August 15, 2022 11:26

dfed changed the title ~~Remove LAContext references from tvOS targets~~ Remove SinglePromptSecureEnclaveValet from tvOS targets Aug 15, 2022

diogot added 2 commits August 16, 2022 15:46

Make SinglePromptSecureEnclaveValet unavailable in tvOS

75fe4ab

Signed-off-by: Diogo Tridapalli <diogot@users.noreply.github.com>

Remove SinglePromptSecureEnclave tests from tvOS

f615eeb

Signed-off-by: Diogo Tridapalli <diogot@users.noreply.github.com>

diogot force-pushed the xcode-14-tvos-fix branch from ac8235f to f615eeb Compare August 16, 2022 18:46

dfed approved these changes Aug 17, 2022

View reviewed changes

Apply suggestions from code review

4c533de

Co-authored-by: Dan Federman <dfed@me.com> Signed-off-by: Diogo Tridapalli <diogot@users.noreply.github.com>

diogot force-pushed the xcode-14-tvos-fix branch from 529e2c0 to 4c533de Compare August 17, 2022 14:30

dfed merged commit 2f2ce52 into square:master Aug 17, 2022

dfed mentioned this pull request Aug 17, 2022

Unit tests fail on tvOS 14 #270

Closed

diogot deleted the xcode-14-tvos-fix branch August 17, 2022 15:26

dfed mentioned this pull request Feb 24, 2023

Deprecation warnings when targeting iOS 14+ #294

Closed

Conversation

diogot commented Aug 12, 2022

Uh oh!

CLAassistant commented Aug 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NickEntin Aug 12, 2022

Choose a reason for hiding this comment

Uh oh!

dfed Aug 13, 2022

Choose a reason for hiding this comment

Uh oh!

diogot Aug 15, 2022

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Aug 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dfed commented Aug 15, 2022

Uh oh!

diogot commented Aug 15, 2022

Uh oh!

diogot commented Aug 16, 2022

Uh oh!

diogot commented Aug 16, 2022

Uh oh!

dfed commented Aug 17, 2022

Uh oh!

dfed left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dfed Aug 17, 2022

Choose a reason for hiding this comment

Uh oh!

dfed Aug 17, 2022

Choose a reason for hiding this comment

Uh oh!

diogot Aug 17, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dfed Aug 17, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

diogot commented Aug 17, 2022

Uh oh!

dfed commented Aug 17, 2022

Uh oh!

diogot commented Aug 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

CLAassistant commented Aug 12, 2022 •

edited

Loading

codecov-commenter commented Aug 12, 2022 •

edited

Loading