Hello again,
Here's v2 of the SecKeychain -> SecItem patch set. Major changes
include dropping patch 1, swapping patches 3 and 4, and adding another
query_macos_keychain() helper function with slightly more robust error
handling. Minor changes include formatting more optimised for 4-space
tabs, combining variable declarations and definitions where reasonable,
and rewording commit messages.
Take care,
Seth McDonald.
Seth McDonald (3):
Segregate macOS-specific keychain code
Add no-fail Core Foundation wrappers
Replace use of deprecated SecKeychain API
src/common.h | 9 ++++
src/drv_imap.c | 132 ++++++++++++++++++++++++++++++++++++++++---------
src/util.c | 32 ++++++++++++
3 files changed, 151 insertions(+), 22 deletions(-)
Range-diff against v1:
1: 73eee7269825 ! 1: c2b614c8b739 Remove excessive if nesting
@@ Metadata
Author: Seth McDonald <[email protected]>
## Commit message ##
- Remove excessive if nesting
+ Segregate macOS-specific keychain code
- ensure_user() and ensure_password() attempt to obtain the user and
- password, respectively, if not already known, before returning it.
This
- has resulted in the function definitions being almost entirely
contained
- within a single if block, executed when the result is not already
known.
- Such control flow is not the most readable, particularly for longer
- functions, due to the added indentation.
+ It is useful to segregate OS-specific code where appropriate for better
+ readability and easier debugging. Logic regarding the macOS keychain
is
+ macOS-specific (obviously), and was intermixed with the
+ ensure_password() function.
- Reorder these function definitions to instead return early if the
- user/password is already known. This saves space by removing the
- unnecessary indentation and increases readability by immediately
- communicating the intent to do nothing if not needed.
+ Move the logic relating to searching the macOS keychain into a
+ self-contained search_macos_keychain() function, adding a clearer
+ separation between macOS-specific and OS-agnostic code in the
+ ensure_password() function (since the macOS-specific code is just a
+ single function call).
## src/drv_imap.c ##
-@@ src/drv_imap.c: cred_from_cmd( const char *cred, const char *cmd, const
char *srv_name )
- static const char *
- ensure_user( imap_server_conf_t *srvc )
- {
-- if (!srvc->user) {
-- if (srvc->user_cmd) {
-- srvc->user = cred_from_cmd( "UserCmd", srvc->user_cmd,
srvc->name );
-- } else {
-- error( "Skipping account %s, no user\n", srvc->name );
-- }
-- }
-+ if (srvc->user)
-+ return srvc->user;
-+ if (srvc->user_cmd)
-+ srvc->user = cred_from_cmd( "UserCmd", srvc->user_cmd,
srvc->name );
-+ else
-+ error( "Skipping account %s, no user\n", srvc->name );
+@@ src/drv_imap.c: ensure_user( imap_server_conf_t *srvc )
return srvc->user;
}
++#ifdef HAVE_MACOS_KEYCHAIN
++/*
++ * Searches the macOS file-based keychain for an appropriate password. If
++ * found, returns a newly allocated copy of the password (which should
later be
++ * freed). Otherwise, returns NULL.
++ *
++ * A password is deemed appropriate if its protocol is IMAP, and its
server name
++ * and account name match the given host and user, respectively. If
multiple
++ * such passwords exist, the first found by the Security framework is
returned.
++ */
++static char *
++search_macos_keychain( const char *host, const char *user )
++{
++ void *password_data;
++ UInt32 password_length;
++
++ OSStatus ret = SecKeychainFindInternetPassword(
++ NULL, // keychainOrArray
++ strlen( host ), host,
++ 0, NULL, // securityDomain
++ strlen( user ), user,
++ 0, NULL, // path
++ 0, // port - we could use it, but it seems pointless
++ kSecProtocolTypeIMAP,
++ kSecAuthenticationTypeDefault,
++ &password_length, &password_data,
++ NULL ); // itemRef
++ if (ret != errSecSuccess) {
++ CFStringRef errmsg = SecCopyErrorMessageString( ret, NULL );
++ error( "Looking up Keychain failed: %s\n",
++ CFStringGetCStringPtr( errmsg, kCFStringEncodingUTF8 ) );
++ CFRelease( errmsg );
++ return NULL;
++ }
++
++ char *password_copy = nfstrndup( password_data, password_length );
++ SecKeychainItemFreeContent( NULL, password_data );
++ return password_copy;
++}
++#endif /* HAVE_MACOS_KEYCHAIN */
++
static const char *
ensure_password( imap_server_conf_t *srvc )
{
-- if (!srvc->pass) {
-- if (srvc->pass_cmd) {
-- srvc->pass = cred_from_cmd( "PassCmd", srvc->pass_cmd,
srvc->name );
-+ if (srvc->pass)
-+ return srvc->pass;
-+ if (srvc->pass_cmd) {
-+ srvc->pass = cred_from_cmd( "PassCmd", srvc->pass_cmd,
srvc->name );
+ if (!srvc->pass) {
+ if (srvc->pass_cmd) {
+ srvc->pass = cred_from_cmd( "PassCmd", srvc->pass_cmd,
srvc->name );
#ifdef HAVE_MACOS_KEYCHAIN
-- } else if (srvc->use_keychain) {
+ } else if (srvc->use_keychain) {
- void *password_data;
- UInt32 password_length;
- OSStatus ret = SecKeychainFindInternetPassword(
@@ src/drv_imap.c: cred_from_cmd( const char *cred, const char *cmd, const
char *sr
- }
- srvc->pass = nfstrndup( password_data, password_length
);
- SecKeychainItemFreeContent( NULL, password_data );
--#endif /* HAVE_MACOS_KEYCHAIN */
-- } else {
-- flushn();
-- char prompt[80];
-- sprintf( prompt, "Password (%s): ", srvc->name );
-- char *pass = getpass( prompt );
-- if (!pass) {
-- perror( "getpass" );
-- exit( 1 );
-- }
-- if (!*pass) {
-- error( "Skipping account %s, no password\n",
srvc->name );
-- return NULL;
-- }
-- /* getpass() returns a pointer to a static buffer. Make
a copy for long term storage. */
-- srvc->pass = nfstrdup( pass );
-+ } else if (srvc->use_keychain) {
-+ void *password_data;
-+ UInt32 password_length;
-+ OSStatus ret = SecKeychainFindInternetPassword(
-+ NULL, // keychainOrArray
-+ strlen( srvc->sconf.host ), srvc->sconf.host,
-+ 0, NULL, // securityDomain
-+ strlen( srvc->user ), srvc->user,
-+ 0, NULL, // path
-+ 0, // port - we could use it, but it seems
pointless
-+ kSecProtocolTypeIMAP,
-+ kSecAuthenticationTypeDefault,
-+ &password_length, &password_data,
-+ NULL ); // itemRef
-+ if (ret != errSecSuccess) {
-+ CFStringRef errmsg = SecCopyErrorMessageString( ret,
NULL );
-+ error( "Looking up Keychain failed: %s\n",
-+ CFStringGetCStringPtr( errmsg,
kCFStringEncodingUTF8 ) );
-+ CFRelease( errmsg );
-+ return NULL;
- }
-+ srvc->pass = nfstrndup( password_data, password_length );
-+ SecKeychainItemFreeContent( NULL, password_data );
-+#endif /* HAVE_MACOS_KEYCHAIN */
-+ } else {
-+ flushn();
-+ char prompt[80];
-+ sprintf( prompt, "Password (%s): ", srvc->name );
-+ char *pass = getpass( prompt );
-+ if (!pass) {
-+ perror( "getpass" );
-+ exit( 1 );
-+ }
-+ if (!*pass) {
-+ error( "Skipping account %s, no password\n", srvc->name
);
-+ return NULL;
-+ }
-+ /* getpass() returns a pointer to a static buffer. Make a copy
for long term storage. */
-+ srvc->pass = nfstrdup( pass );
- }
- return srvc->pass;
- }
++ srvc->pass = search_macos_keychain( srvc->sconf.host,
srvc->user );
+ #endif /* HAVE_MACOS_KEYCHAIN */
+ } else {
+ flushn();
2: 0bb1a1c2ad9f < -: ------------ Segregate macOS-specific keychain code
4: 7d9c0b63d82d ! 2: 46015e2af9fb Add no-fail Core Foundation wrappers
@@ src/common.h: int ATTR_PRINTFLIKE(3, 4) nfsnprintf( char *buf, int blen,
const c
## src/drv_imap.c ##
@@ src/drv_imap.c: search_macos_keychain( const char *host, const char
*user )
- CFIndex password_length;
- OSStatus ret;
-
-- CFStringRef account = CFStringCreateWithCString(
-- kCFAllocatorDefault, user, kCFStringEncodingUTF8 );
-- assert( account );
--
-- CFStringRef protocol = CFSTR( "imap" );
-- assert( protocol );
--
-- CFStringRef server = CFStringCreateWithCString(
-- kCFAllocatorDefault, host, kCFStringEncodingUTF8 );
-- assert( server );
-+ CFStringRef account = nfstrtocf( user );
-+ CFStringRef protocol = nfstrtocf( "imap" );
-+ CFStringRef server = nfstrtocf( host );
-
- /* Same index = key-value pair */
- CFTypeRef query_keys[] = {
-@@ src/drv_imap.c: search_macos_keychain( const char *host, const char
*user )
- CFRelease( account );
+ NULL ); // itemRef
if (ret != errSecSuccess) {
CFStringRef errmsg = SecCopyErrorMessageString( ret, NULL );
- error( "Looking up Keychain failed: %s\n",
- CFStringGetCStringPtr( errmsg, kCFStringEncodingUTF8 ) );
+- CFRelease( errmsg );
+ if (errmsg) {
+ char *errstr = nfcftostr( errmsg );
+ error( "Looking up Keychain failed: %s\n", errstr );
+ free( errstr );
++ CFRelease( errmsg );
+ } else {
-+ error( "Looking up Keychain failed: Unknown error\n");
++ error( "Looking up Keychain failed: Unknown error\n" );
+ }
- CFRelease( errmsg );
return NULL;
}
+
## src/util.c ##
@@ src/util.c: nfasprintf( char **str, const char *fmt, ... )
@@ src/util.c: nfasprintf( char **str, const char *fmt, ... )
+char *
+nfcftostr( CFStringRef str )
+{
-+ CFIndex len = CFStringGetLength( str ); /* Length in UTF-16 */
++ CFIndex len = CFStringGetLength( str ); // Length in UTF-16
+ size_t max_size = len * sizeof(UInt16) + sizeof(char);
+ char *ret = nfmalloc( max_size );
+ CFStringGetCString( str, ret, max_size, kCFStringEncodingUTF8 );
3: 3f19b9dca42d ! 3: be6a5d51655e Remove use of deprecated SecKeychain API
@@ Metadata
Author: Seth McDonald <[email protected]>
## Commit message ##
- Remove use of deprecated SecKeychain API
+ Replace use of deprecated SecKeychain API
isync's macOS keychain functionality interfaces with Apple's Security
framework via the SecKeychain API. The SecItem API was introduced in
macOS 10.6 and became the preferred method using the keychain on macOS,
officially deprecating the SecKeychain API in macOS 10.10. So the
- codebase is currently using an API deprecated since 2014.
-
- This can be seen by building isync on macOS. Running `make` on macOS
- 26.4 with gcc 15.2.0 gives relevant compilation warnings:
-
- drv_imap.c: In function 'search_macos_keychain':
- drv_imap.c:2352:9: warning: 'SecKeychainFindInternetPassword'
is deprecated: SecKeychain is deprecated [-Wdeprecated-declarations]
- 2352 | ret = SecKeychainFindInternetPassword(
- | ^~~
- In file included from /.../SecImportExport.h:41,
- from /.../Security.h:35,
- from drv_imap.c:22:
- /.../SecKeychain.h:592:10: note: declared here
- 592 | OSStatus SecKeychainFindInternetPassword(...)
- | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
- drv_imap.c:2371:9: warning: 'SecKeychainItemFreeContent' is
deprecated: SecKeychain is deprecated [-Wdeprecated-declarations]
- 2371 | SecKeychainItemFreeContent( NULL,
password_data );
- | ^~~~~~~~~~~~~~~~~~~~~~~~~~
- In file included from /.../Security.h:79:
- /.../SecKeychainItem.h:220:10: note: declared here
- 220 | OSStatus SecKeychainItemFreeContent(...)
- | ^~~~~~~~~~~~~~~~~~~~~~~~~~
+ codebase was using an API deprecated since 2014.
Rewrite the search_macos_keychain() function to interface with the
- SecItem API. The function retains the same behaviour with regard to
- the success or failure of searching the keychain.
-
- The SecItem API requires the creation of multiple objects to specify
the
- search query, each of which may fail for whatever reason (most likely
- OOM). For now, use assertions to ensure no such object creation fails.
+ SecItem API. Also add the query_macos_keychain() function to simplify
+ the error handling. The function retains mostly the same behaviour with
+ regard to the success or failure of finding the password.
## src/drv_imap.c ##
+@@ src/drv_imap.c: ensure_user( imap_server_conf_t *srvc )
+ }
+
+ #ifdef HAVE_MACOS_KEYCHAIN
++/*
++ * Queries the macOS keychain for items matching the paramters specified
by the
++ * given keys and vals. If matches exist, returns the corresponding
reference.
++ * Otherwise, returns NULL.
++ *
++ * The query paramters are a dictionary whose key-value pairs are
specified by
++ * each pair of values located at the same index in the given keys and
vals
++ * arrays. So key 'keys[0]' has value 'vals[0]', key 'keys[1]' has value
++ * 'vals[1]', etc., up until key 'keys[size - 1]' and value 'vals[size -
1]'.
++ *
++ * The given size must be nonnegative and no more than the length of keys
or
++ * vals. Both keys and vals must be non-NULL if size is positive.
++ */
++static CFTypeRef
++query_macos_keychain( CFTypeRef *keys, CFTypeRef *vals, CFIndex size )
++{
++ assert( size >= 0 );
++
++ CFDictionaryRef query = CFDictionaryCreate(
++ kCFAllocatorDefault,
++ keys,
++ vals,
++ size,
++ &kCFTypeDictionaryKeyCallBacks,
++ &kCFTypeDictionaryValueCallBacks );
++ if (!query) {
++ error( "Looking up Keychain failed: Query creation failed\n" );
++ return NULL;
++ }
++
++ CFTypeRef item;
++ OSStatus ret = SecItemCopyMatching( query, &item );
++ CFRelease( query );
++ if (ret == errSecSuccess)
++ return item;
++ if (ret == errSecItemNotFound) {
++ // Ask user to add password to keychain if not found
++ error(
++ "Looking up Keychain failed: "
++ "Please add your password to the macOS keychain; "
++ "instructions can be found in the mbsync man page.\n" );
++ } else {
++ // Error is not because of user
++ CFStringRef errmsg = SecCopyErrorMessageString( ret, NULL );
++ if (errmsg) {
++ char *errstr = nfcftostr( errmsg );
++ error( "Looking up Keychain failed: %s\n", errstr );
++ free( errstr );
++ CFRelease( errmsg );
++ } else {
++ error( "Looking up Keychain failed: Unknown error\n" );
++ }
++ }
++ return NULL;
++}
++
+ /*
+ * Searches the macOS file-based keychain for an appropriate password. If
+ * found, returns a newly allocated copy of the password (which should
later be
@@ src/drv_imap.c: ensure_user( imap_server_conf_t *srvc )
static char *
search_macos_keychain( const char *host, const char *user )
{
- void *password_data;
-+ CFDictionaryRef query;
-+ CFDataRef password_data;
-+ const char *password_buffer;
- char *password_copy;
- UInt32 password_length;
-+ CFIndex password_length;
- OSStatus ret;
++ CFStringRef account = nfstrtocf( user );
++ CFStringRef protocol = nfstrtocf( "imap" ); // Also "imps" for IMAPS
++ CFStringRef server = nfstrtocf( host );
-- ret = SecKeychainFindInternetPassword(
+- OSStatus ret = SecKeychainFindInternetPassword(
- NULL, // keychainOrArray
- strlen( host ), host,
- 0, NULL, // securityDomain
@@ src/drv_imap.c: ensure_user( imap_server_conf_t *srvc )
- kSecAuthenticationTypeDefault,
- &password_length, &password_data,
- NULL ); // itemRef
-+ CFStringRef account = CFStringCreateWithCString(
-+ kCFAllocatorDefault, user, kCFStringEncodingUTF8 );
-+ assert( account );
-+
-+ CFStringRef protocol = CFSTR( "imap" );
-+ assert( protocol );
-+
-+ CFStringRef server = CFStringCreateWithCString(
-+ kCFAllocatorDefault, host, kCFStringEncodingUTF8 );
-+ assert( server );
-+
-+ /* Same index = key-value pair */
+- if (ret != errSecSuccess) {
+- CFStringRef errmsg = SecCopyErrorMessageString( ret, NULL );
+- if (errmsg) {
+- char *errstr = nfcftostr( errmsg );
+- error( "Looking up Keychain failed: %s\n", errstr );
+- free( errstr );
+- CFRelease( errmsg );
+- } else {
+- error( "Looking up Keychain failed: Unknown error\n" );
+- }
++ // Same index = key-value pair
+ CFTypeRef query_keys[] = {
+ kSecClass,
+ kSecAttrAccount,
-+ /* Also kSecAttrPort */
++ // Also kSecAttrPort
+ kSecAttrProtocol,
+ kSecAttrServer,
+ kSecReturnData
@@ src/drv_imap.c: ensure_user( imap_server_conf_t *srvc )
+ };
+
+ static_assert( as(query_keys), "empty query" );
-+ static_assert(
-+ as(query_keys) == as(query_vals), "query size mismatch" );
++ static_assert( as(query_keys) == as(query_vals), "query size mismatch"
);
+
-+ query = CFDictionaryCreate(
-+ kCFAllocatorDefault,
-+ query_keys,
-+ query_vals,
-+ as(query_keys),
-+ &kCFTypeDictionaryKeyCallBacks,
-+ &kCFTypeDictionaryValueCallBacks );
-+ assert( query );
-+
-+ ret = SecItemCopyMatching( query, (CFTypeRef *)&password_data );
-+ CFRelease( query );
++ CFDataRef password = query_macos_keychain(
++ query_keys, query_vals, as(query_keys) );
+ CFRelease( server );
+ CFRelease( protocol );
+ CFRelease( account );
- if (ret != errSecSuccess) {
- CFStringRef errmsg = SecCopyErrorMessageString( ret, NULL );
- error( "Looking up Keychain failed: %s\n",
- CFStringGetCStringPtr( errmsg, kCFStringEncodingUTF8 ) );
- CFRelease( errmsg );
++ if (!password)
return NULL;
- }
-- password_copy = nfstrndup( password_data, password_length );
+- }
+
++ const char *password_data = (const char *)CFDataGetBytePtr( password );
++ CFIndex password_length = CFDataGetLength( password );
+ char *password_copy = nfstrndup( password_data, password_length );
- SecKeychainItemFreeContent( NULL, password_data );
-+
-+ password_buffer = (const char *)CFDataGetBytePtr( password_data );
-+ password_length = CFDataGetLength( password_data );
-+ password_copy = nfstrndup( password_buffer, password_length );
-+ CFRelease( password_data );
++ CFRelease( password );
return password_copy;
}
#endif /* HAVE_MACOS_KEYCHAIN */
base-commit: 9aaac66286910f547ec3068d3fd72afb4fe716bf
--
2.50.1 (Apple Git-155)
_______________________________________________
isync-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/isync-devel
