diff --git a/TODO.md b/TODO.md index b2bb723..e4bf515 100644 --- a/TODO.md +++ b/TODO.md @@ -18,6 +18,22 @@ https://git.eeqj.de/sneak/secret/milestone/12 # Completed Steps +- 2026-10-04: A failed `secret unlocker add keychain` or + `secret unlocker add secure-enclave` no longer leaves its keychain item or + Secure Enclave key behind (https://git.eeqj.de/sneak/secret/issues/89). + `CreateSecureEnclaveUnlocker` gets the long-term key before it creates the + Secure Enclave key, so that a wrong passphrase creates none, and deletes the + key again if encrypting with it or writing the unlocker then fails. + `macse.CreateKey` finds the new key's hash right after `sc_auth` creates + it, and fails with an error naming the key's label if it cannot; it deletes + the key again if getting its public key then fails. The Objective-C was only + read, never compiled or run, and so was `macse_darwin.go`, which is cgo only. + `CreateKeychainUnlocker` writes all of the unlocker's files, the metadata + among them, before it stores the item in the keychain, and deletes the item + again if moving the unlocker into place then fails. A failure to delete is + reported along with the first error. The tests of this run only on macOS: + the Secure Enclave one in a build with cgo on a Mac with a Secure Enclave, + the keychain one in a build with cgo. - 2026-10-04: What a command killed part-way left under a `.tmp-` name (https://git.eeqj.de/sneak/secret/issues/75), the temporary directories of `secret.TempDirFor` and the temporary files of diff --git a/internal/macse/macse_darwin.go b/internal/macse/macse_darwin.go index 04cc57b..130d33c 100644 --- a/internal/macse/macse_darwin.go +++ b/internal/macse/macse_darwin.go @@ -15,6 +15,7 @@ package macse import "C" import ( + "errors" "fmt" "unsafe" ) @@ -39,10 +40,9 @@ const ( // CreateKey creates a new P-256 non-exportable key in the Secure Enclave via sc_auth. // Returns the uncompressed public key bytes (65 bytes) and the identity hash -// (for deletion). +// (for deletion). If getting the public key fails, CreateKey deletes the key +// again; a failure to delete is returned along with the first error. func CreateKey(label string) (publicKey []byte, hash string, err error) { - pubKeyBuf := make([]C.uint8_t, p256UncompressedKeySize) - pubKeyLen := C.int(p256UncompressedKeySize) var hashBuf [hashBufferSize]C.char var errBuf [errorBufferSize]C.char @@ -50,7 +50,6 @@ func CreateKey(label string) (publicKey []byte, hash string, err error) { defer C.free(unsafe.Pointer(cLabel)) //nolint:nlreturn // CGo free pattern result := C.se_create_key(cLabel, - &pubKeyBuf[0], &pubKeyLen, &hashBuf[0], C.int(hashBufferSize), &errBuf[0], C.int(errorBufferSize)) @@ -58,9 +57,29 @@ func CreateKey(label string) (publicKey []byte, hash string, err error) { return nil, "", fmt.Errorf("secure enclave: %s", C.GoString(&errBuf[0])) } + h := C.GoString(&hashBuf[0]) + + pubKeyBuf := make([]C.uint8_t, p256UncompressedKeySize) + pubKeyLen := C.int(p256UncompressedKeySize) + + result = C.se_copy_public_key(cLabel, + &pubKeyBuf[0], &pubKeyLen, + &errBuf[0], C.int(errorBufferSize)) + + if result != 0 { + err = fmt.Errorf("secure enclave: %s", C.GoString(&errBuf[0])) + + deleteErr := DeleteKey(h) + if deleteErr != nil { + err = errors.Join(err, + fmt.Errorf("failed to delete key %s: %w", label, deleteErr)) + } + + return nil, "", err + } + //nolint:nlreturn // CGo result extraction pk := C.GoBytes(unsafe.Pointer(&pubKeyBuf[0]), pubKeyLen) - h := C.GoString(&hashBuf[0]) return pk, h, nil } diff --git a/internal/macse/secure_enclave.h b/internal/macse/secure_enclave.h index a828101..c70c7dd 100644 --- a/internal/macse/secure_enclave.h +++ b/internal/macse/secure_enclave.h @@ -5,20 +5,30 @@ #include -// se_create_key creates a new P-256 key in the Secure Enclave via sc_auth. +// se_create_key creates a new P-256 key in the Secure Enclave via sc_auth and +// finds its identity hash. If the hash cannot be found, the key exists but +// se_create_key fails, with an error naming the label. // label: unique identifier for the CTK identity (UTF-8 C string) -// pub_key_out: output buffer for the uncompressed public key (65 bytes for P-256) -// pub_key_len: on input, size of pub_key_out; on output, actual size written // hash_out: output buffer for the identity hash (for deletion) // hash_out_len: size of hash_out buffer // error_out: output buffer for error message // error_out_len: size of error_out buffer // Returns 0 on success, -1 on failure. int se_create_key(const char *label, - uint8_t *pub_key_out, int *pub_key_len, char *hash_out, int hash_out_len, char *error_out, int error_out_len); +// se_copy_public_key copies the public key of a CTK identity. +// label: label of the CTK identity +// pub_key_out: output buffer for the uncompressed public key (65 bytes for P-256) +// pub_key_len: on input, size of pub_key_out; on output, actual size written +// error_out: output buffer for error message +// error_out_len: size of error_out buffer +// Returns 0 on success, -1 on failure. +int se_copy_public_key(const char *label, + uint8_t *pub_key_out, int *pub_key_len, + char *error_out, int error_out_len); + // se_encrypt encrypts data using the SE-backed public key (ECIES). // label: label of the CTK identity whose public key to use // plaintext: data to encrypt diff --git a/internal/macse/secure_enclave.m b/internal/macse/secure_enclave.m index 180754c..50651ed 100644 --- a/internal/macse/secure_enclave.m +++ b/internal/macse/secure_enclave.m @@ -47,7 +47,6 @@ static SecKeyRef lookup_ctk_private_key(const char *label, char *error_out, int } int se_create_key(const char *label, - uint8_t *pub_key_out, int *pub_key_len, char *hash_out, int hash_out_len, char *error_out, int error_out_len) { @autoreleasepool { @@ -87,7 +86,56 @@ int se_create_key(const char *label, return -1; } - // Retrieve the public key from the created identity + // Get the identity hash, which deleting the key needs, by parsing + // sc_auth list output + hash_out[0] = '\0'; + NSTask *listTask = [[NSTask alloc] init]; + listTask.executableURL = [NSURL fileURLWithPath:@"/usr/sbin/sc_auth"]; + listTask.arguments = @[@"list-ctk-identities"]; + + NSPipe *listPipe = [NSPipe pipe]; + listTask.standardOutput = listPipe; + listTask.standardError = [NSPipe pipe]; + + if ([listTask launchAndReturnError:&nsError]) { + [listTask waitUntilExit]; + NSData *listData = [listPipe.fileHandleForReading readDataToEndOfFile]; + NSString *listStr = [[NSString alloc] initWithData:listData + encoding:NSUTF8StringEncoding]; + + for (NSString *line in [listStr componentsSeparatedByString:@"\n"]) { + if ([line containsString:labelStr]) { + NSMutableArray *tokens = [NSMutableArray array]; + for (NSString *part in [line componentsSeparatedByCharactersInSet: + [NSCharacterSet whitespaceCharacterSet]]) { + if (part.length > 0) { + [tokens addObject:part]; + } + } + if (tokens.count > 1) { + snprintf(hash_out, hash_out_len, "%s", [tokens[1] UTF8String]); + } + break; + } + } + } + + if (hash_out[0] == '\0') { + NSString *msg = [NSString stringWithFormat: + @"created key '%s' but found no hash for it in sc_auth list-ctk-identities", + label]; + snprintf_error(error_out, error_out_len, msg); + return -1; + } + + return 0; + } +} + +int se_copy_public_key(const char *label, + uint8_t *pub_key_out, int *pub_key_len, + char *error_out, int error_out_len) { + @autoreleasepool { SecKeyRef privateKey = lookup_ctk_private_key(label, error_out, error_out_len); if (!privateKey) { return -1; @@ -126,39 +174,6 @@ int se_create_key(const char *label, *pub_key_len = (int)length; CFRelease(pubKeyData); - // Get the identity hash by parsing sc_auth list output - hash_out[0] = '\0'; - NSTask *listTask = [[NSTask alloc] init]; - listTask.executableURL = [NSURL fileURLWithPath:@"/usr/sbin/sc_auth"]; - listTask.arguments = @[@"list-ctk-identities"]; - - NSPipe *listPipe = [NSPipe pipe]; - listTask.standardOutput = listPipe; - listTask.standardError = [NSPipe pipe]; - - if ([listTask launchAndReturnError:&nsError]) { - [listTask waitUntilExit]; - NSData *listData = [listPipe.fileHandleForReading readDataToEndOfFile]; - NSString *listStr = [[NSString alloc] initWithData:listData - encoding:NSUTF8StringEncoding]; - - for (NSString *line in [listStr componentsSeparatedByString:@"\n"]) { - if ([line containsString:labelStr]) { - NSMutableArray *tokens = [NSMutableArray array]; - for (NSString *part in [line componentsSeparatedByCharactersInSet: - [NSCharacterSet whitespaceCharacterSet]]) { - if (part.length > 0) { - [tokens addObject:part]; - } - } - if (tokens.count > 1) { - snprintf(hash_out, hash_out_len, "%s", [tokens[1] UTF8String]); - } - break; - } - } - } - return 0; } } diff --git a/internal/secret/atomic_test.go b/internal/secret/atomic_test.go index cf9792c..a976205 100644 --- a/internal/secret/atomic_test.go +++ b/internal/secret/atomic_test.go @@ -8,6 +8,7 @@ import ( "testing" "filippo.io/age" + "git.eeqj.de/sneak/secret/internal/macse" "git.eeqj.de/sneak/secret/internal/secret" "git.eeqj.de/sneak/secret/internal/vault" "github.com/awnumar/memguard" @@ -899,3 +900,42 @@ func TestWriteDirRefusesExistingDir(t *testing.T) { }) } } + +// TestSecureEnclaveUnlockerFailureDeletesKey makes moving a new Secure +// Enclave unlocker into place fail after its Secure Enclave key is created: +// the key must be deleted again. Skipped when the add fails before that, as +// it does everywhere but in a macOS build with cgo on a Mac with a Secure +// Enclave. +func TestSecureEnclaveUnlockerFailureDeletesKey(t *testing.T) { + t.Parallel() + + mnemonic := testMnemonicBuffer(t) + base := afero.NewMemMapFs() + _, err := vault.CreateVault(base, testVaultStateDir, testVaultName, mnemonic) + require.NoError(t, err) + + // The unlocker's directory is named se-