osxkeychain: bring in line with other credential helpers

The SecKeychain API was deprecated in macOS 10.10, nearly 10 years ago. The replacement SecItem API however is available as far back as macOS 10.6.

While supporting older macOS was perhaps prevously a concern, git-credential-osxkeychain already requires a minimum of macOS 10.7 since 5747c8072b (contrib/credential: avoid fixed-size buffer in osxkeychain, 2023-05-01) so using the newer API should not regress the range of macOS versions supported.

Adapting to use the newer SecItem API also happens to fix two test failures in osxkeychain:

8 - helper (osxkeychain) overwrites on store
9 - helper (osxkeychain) can forget host

The new API is compatible with credentials saved with the older API.

Signed-off-by Bo Anderson <[email protected]>

contrib/credential/osxkeychain/git-credential-osxkeychain.c +4 -1

@@ -3,14 +3,39 @@
-__attribute__((format (printf, 1, 2)))
+#define ENCODING kCFStringEncodingUTF8
+static CFStringRef protocol; /* Stores constant strings - not memory managed */
+static CFStringRef host;
[...]
+

contrib/credential/osxkeychain/git-credential-osxkeychain.c +4 -0

+static OSStatus add_internet_password(void)
 {
+       [...]
+       attrs = CREATE_SEC_ATTRIBUTES(kSecValueData, password,
+                                     NULL);

d208bfdfef (credential: new attribute password_expiry_utc, 2023-02-18) and a5c76569e7 (credential: new attribute oauth_refresh_token, 2023-04-21) introduced new credential attributes but support was missing from git-credential-osxkeychain.

[...]

Signed-off-by Bo Anderson <[email protected]>

---

contrib/credential/osxkeychain/git-credential-osxkeychain.c +5 -1

@@ -6,10 +6,12 @@
 static CFStringRef host;
+static CFNumberRef port;
 static CFStringRef path;
-static CFNumberRef port;

@@ -17,6 +19,10 @@ static void clear_credential(void)
+       if (port) {
+               CFRelease(port);
+               port = NULL;
+       }

git-credential-osxkeychain has largely fallen behind other external credential helpers in the features it supports, and hasn't received any functional changes since 2013. [...]

osxkeychain also made use of macOS APIs that had been deprecated since 2014. Replacement API was able to be used without regressing the minimum supported macOS established in 5747c8072b (contrib/credential: avoid fixed-size buffer in osxkeychain, 2023-05-01).

I haven't studied the SecItem API, so I can't comment on the meat of the patch, but I can make a few generic observations...

Quoted 16 lines

contrib/credential/osxkeychain/git-credential-osxkeychain.c +12 -1

@@ -3,14 +3,39 @@
-__attribute__((format (printf, 1, 2)))
+#define ENCODING kCFStringEncodingUTF8
+static CFStringRef protocol; /* Stores constant strings - not memory managed */
+static CFStringRef host;
[...]
+
+static void clear_credential(void)
+{
+       if (host) {
+               CFRelease(host);
+               host = NULL;
+       }
+       [...]
+}

contrib/credential/osxkeychain/git-credential-osxkeychain.c

Quoted 2 lines

contrib/credential/osxkeychain/git-credential-osxkeychain.c +2 -0

+
+__attribute__((format (printf, 1, 2), __noreturn__))

The addition of __noreturn__ to the __attribute__ seems unrelated to the stated purpose of this patch. As such, it typically would be placed in its own patch. If it really is too minor for a separate patch, mentioning it in the commit message as a "While at it..." would be helpful.

contrib/credential/osxkeychain/git-credential-osxkeychain.c static void die(const char *err, ...) · line 78

Quoted 8 lines

contrib/credential/osxkeychain/git-credential-osxkeychain.c +8 -0

+       va_start(args, allocator);
+       while ((key = va_arg(args, const void *)) != NULL) {
+               const void *value;
+               value = va_arg(args, const void *);
+               if (value)
+                       CFDictionarySetValue(result, key, value);
+       }
+       va_end(args);

A couple related comments...

If va_arg() ever returns NULL for value, the next iteration of the loop will call va_arg() again, but calling va_arg() again after it has already returned NULL is likely undefined behavior. At minimum, I would have expected this to be written as:

while (...) {
    ...
    if (!value)
        break;
    CFDictionarySetValue(...);
}

However, isn't it a programmer error if va_arg() returns NULL for value? If so, I'd think we'd want to scream loudly about that rather than silently ignoring it. So, perhaps something like this:

while (...) {
    ...
    if (!value) {
        fprintf(stderr, "BUG: ...");
        abort();
    }
    CFDictionarySetValue(...);
}

Or, perhaps just call the existing die() function in this file with a suitable "BUG ..." message.

contrib/credential/osxkeychain/git-credential-osxkeychain.c static void die(const char *err, ...) · line 120

Quoted 6 lines

contrib/credential/osxkeychain/git-credential-osxkeychain.c +5 -0

+       username_buf = (char *)CFStringGetCStringPtr(account_ref, ENCODING);
+       if (username_buf)
+       {
+               write_item("username", username_buf, strlen(username_buf));
            return;
+       }

According to the documentation for CFStringGetCStringPtr(), the returned C-string is not newly-allocated, so the caller does not have to free it. Therefore, can username_buf be declared const char * rather than char * to make it clear to readers that nothing is being leaked here? Same comment about the (char *) cast.

contrib/credential/osxkeychain/git-credential-osxkeychain.c static void die(const char *err, ...) · line 123

Quoted 2 lines

contrib/credential/osxkeychain/git-credential-osxkeychain.c +2 -0

+       /* If we can't get a CString pointer then
+        * we need to allocate our own buffer */

Style:

/*
* Multi-line comments
* are formatted like this.
*/

contrib/credential/osxkeychain/git-credential-osxkeychain.c static void die(const char *err, ...) · line 133

Quoted 10 lines

contrib/credential/osxkeychain/git-credential-osxkeychain.c +10 -0

+       buffer_len = CFStringGetMaximumSizeForEncoding(
+                       CFStringGetLength(account_ref), ENCODING) + 1;
+       username_buf = xmalloc(buffer_len);
+       if (CFStringGetCString(account_ref,
+                               username_buf,
+                               buffer_len,
+                               ENCODING)) {
+               write_item("username", username_buf, buffer_len - 1);
+       }
+       free(username_buf);

Okay, this explains why username_buf is declared char * rather than const char *. Typically, when we have a situation in which a value may or may not need freeing, we use a to_free variable like this:

const char *username_buf;
char *to_free = NULL;
...
username_buf = (const char *)CFStringGetCStringPtr(...);

if (username_buf) {
    ...
    return;
}
...
username_buf = to_free = xmalloc(buffer_len);
if (CFStringGetCString(...))
    ...

free(to_free);

But that may be overkill for this simple case, and what you have here may be "good enough" for anyone already familiar with the API and who knows that the return after CFStringGetCStringPtr() isn't leaking.

Quoted 10 lines · Eric Sunshine

contrib/credential/osxkeychain/git-credential-osxkeychain.c static void die(const char *err, ...) · line 78

Quoted 8 lines

contrib/credential/osxkeychain/git-credential-osxkeychain.c +8 -0

+    va_start(args, allocator);
+    while ((key = va_arg(args, const void *)) != NULL) {
+        const void *value;
+        value = va_arg(args, const void *);
+        if (value)
+            CFDictionarySetValue(result, key, value);
+    }
+    va_end(args);

Quoted 3 lines

However, isn't it a programmer error if va_arg() returns NULL for value? If so, I'd think we'd want to scream loudly about that rather than silently ignoring it. So, perhaps something like this: [...]

In this case it’s by design to accept and check for NULL values as it greatly simplifies the code. Inputs to the credential helpers have various optional fields, such as port and path. It is programmer error to pass NULL to the SecItem API (runtime crash) so in order to simplify having to check each individual field in all of the callers (and probably ditch varargs since you can’t really do dynamic varargs), I check the value here instead. That means you can do something like:

create_dictionary(kCFAllocatorDefault,
kSecAttrServer, host,
kSecAttrPath, path, \
kSecAttrPort, port,
NULL)

And it will only include the key-value pairs that have non-NULL values.

It would indeed be programmer error to not pass key-value pairs, though it is equally programmer error to not have a terminating NULL.

Quoted 24 lines

contrib/credential/osxkeychain/git-credential-osxkeychain.c static void die(const char *err, ...) · line 120

Quoted 6 lines

contrib/credential/osxkeychain/git-credential-osxkeychain.c +5 -0

+    username_buf = (char *)CFStringGetCStringPtr(account_ref, ENCODING);
+    if (username_buf)
+    {
+        write_item("username", username_buf, strlen(username_buf));
      return;
+    }

According to the documentation for CFStringGetCStringPtr(), the returned C-string is not newly-allocated, so the caller does not have to free it. Therefore, can username_buf be declared const char * rather than char * to make it clear to readers that nothing is being leaked here? Same comment about the (char *) cast.

contrib/credential/osxkeychain/git-credential-osxkeychain.c static void die(const char *err, ...) · line 133

Quoted 10 lines

contrib/credential/osxkeychain/git-credential-osxkeychain.c +10 -0

+    buffer_len = CFStringGetMaximumSizeForEncoding(
+            CFStringGetLength(account_ref), ENCODING) + 1;
+    username_buf = xmalloc(buffer_len);
+    if (CFStringGetCString(account_ref,
+                username_buf,
+                buffer_len,
+                ENCODING)) {
+        write_item("username", username_buf, buffer_len - 1);
+    }
+    free(username_buf);

Quoted 8 lines

Okay, this explains why username_buf is declared char * rather than const char *. Typically, when we have a situation in which a value may or may not need freeing, we use a to_free variable like
this: [...]

But that may be overkill for this simple case, and what you have here may be "good enough" for anyone already familiar with the API and who knows that the return after CFStringGetCStringPtr() isn't leaking.

Would it make sense to just have a comment paired with the CFStringGetCStringPtr return explaining why it doesn’t need to be freed there? I’m OK with the to_free variable however if that’s clearer. Idea in my mind was pairing it based on xmalloc but I can see why pairing based on variable is clearer.

git-credential-osxkeychain has largely fallen behind other external credential helpers in the features it supports, and hasn't received any functional changes since 2013. As it stood, osxkeychain failed seven tests in the external credential helper test suite:

not ok 8 - helper (osxkeychain) overwrites on store
not ok 9 - helper (osxkeychain) can forget host
not ok 11 - helper (osxkeychain) does not erase a password distinct from input not ok 15 - helper (osxkeychain) erases all matching credentials not ok 18 - helper (osxkeychain) gets password_expiry_utc
not ok 19 - helper (osxkeychain) overwrites when password_expiry_utc changes not ok 21 - helper (osxkeychain) gets oauth_refresh_token

After this set of patches, osxkeychain passes all tests in the external

Quoted 14 lines

git-credential-osxkeychain has largely fallen behind other external credential helpers in the features it supports, and hasn't received any functional changes since 2013. As it stood, osxkeychain failed seven tests in the external credential helper test suite:

not ok 8 - helper (osxkeychain) overwrites on store
not ok 9 - helper (osxkeychain) can forget host
not ok 11 - helper (osxkeychain) does not erase a password distinct from input not ok 15 - helper (osxkeychain) erases all matching credentials not ok 18 - helper (osxkeychain) gets password_expiry_utc
not ok 19 - helper (osxkeychain) overwrites when password_expiry_utc changes not ok 21 - helper (osxkeychain) gets oauth_refresh_token

Quoted 1 line

After this set of patches, osxkeychain passes all tests in the external

credential helper test suite.

Great work!

Could these tests run as part of macOS CI?

Quoted 5 lines · M Hickford

Could these tests run as part of macOS CI?

Do we do so for any of the other external credential helpers?

It definitely makes sense in principle. Though the concern perhaps will be that any new features added to the credential helpers and thus its test suite would need adding to each credential helper simultaneously to avoid failing CI. Ideally we would do exactly that, though that requires knowledge on each of the keystore APIs used in each of the credential helpers.

Quoted 6 lines

It definitely makes sense in principle. Though the concern perhaps will be that any new features added to the credential helpers and thus its test suite would need adding to each credential helper simultaneously to avoid failing CI. Ideally we would do exactly that, though that requires knowledge on each of the keystore APIs used in each of the credential helpers.

Good point.

From: "Bo Anderson via GitGitGadget" <[email protected]>
To: [email protected]
Cc: Bo Anderson <[email protected]>
Subject: [PATCH 0/4] osxkeychain: bring in line with other credential helpers
Date: Sat, 17 Feb 2024 23:34:52 +0000 [thread overview]
Message-ID: <[email protected]> (raw)

git-credential-osxkeychain has largely fallen behind other external credential helpers in the features it supports, and hasn't received any functional changes since 2013. As it stood, osxkeychain failed seven tests in the external credential helper test suite:

not ok 8 - helper (osxkeychain) overwrites on store
not ok 9 - helper (osxkeychain) can forget host
not ok 11 - helper (osxkeychain) does not erase a password distinct from input not ok 15 - helper (osxkeychain) erases all matching credentials not ok 18 - helper (osxkeychain) gets password_expiry_utc
not ok 19 - helper (osxkeychain) overwrites when password_expiry_utc changes not ok 21 - helper (osxkeychain) gets oauth_refresh_token

osxkeychain also made use of macOS APIs that had been deprecated since 2014. Replacement API was able to be used without regressing the minimum supported macOS established in 5747c8072b (contrib/credential: avoid fixed-size buffer in osxkeychain, 2023-05-01).

After this set of patches, osxkeychain passes all tests in the external credential helper test suite.

Bo Anderson (4):
osxkeychain: replace deprecated SecKeychain API
osxkeychain: erase all matching credentials
osxkeychain: erase matching passwords only
osxkeychain: store new attributes

 contrib/credential/osxkeychain/Makefile       |   3 +-
 .../osxkeychain/git-credential-osxkeychain.c  | 376 ++++++++++++++----
 2 files changed, 310 insertions(+), 69 deletions(-)

base-commit: 3e0d3cd5c7def4808247caf168e17f2bbf47892b
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1667%2FBo98%2Fosxkeychain-update-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1667/Bo98/osxkeychain-update-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1667

Signature · 1 line

gitgitgadget

Quoted 8 lines

From: "Bo Anderson via GitGitGadget" <[email protected]>
To: [email protected]
Cc: Bo Anderson <[email protected]>
Subject: [PATCH 0/4] osxkeychain: bring in line with other credential helpers
Date: Sat, 17 Feb 2024 23:34:52 +0000 [thread overview]
Message-ID: <[email protected]> (raw)
...
git-credential-osxkeychain has largely fallen behind other external

Hi. Is this patch ready to cook in seen?

All that said, I'd be surprised if testing osxkeychain in the CI environment worked. Back when I worked on it in 2011, I found that I had to actually run the tests in a local terminal; even a remote ssh login could not access the keychain. It's possible that things have changed since then, though, or perhaps I was imply ignorant of how to configure things correctly.

Hi all,

Quoted 6 lines

All that said, I'd be surprised if testing osxkeychain in the CI environment worked. Back when I worked on it in 2011, I found that I had to actually run the tests in a local terminal; even a remote ssh login could not access the keychain. It's possible that things have changed since then, though, or perhaps I was imply ignorant of how to configure things correctly.

I have gotten keychain working in Github Actions before: there's some helpers for it, but you can also basically do it manually via the steps from [1]. Basically anyone who needs to do Apple code-signing in CI has to make it work.

@Bo, how are you actually testing this manually? Following these steps:

$ make
$ (cd contrib/credential/osxkeychain && make)
$ ln -s contrib/credential/osxkeychain/git-credential-osxkeychain .
$ cd t
$ make GIT_TEST_CREDENTIAL_HELPER=osxkeychain t0303-credential-external.sh

I get 'A keychain cannot be found to store "store-user".' in a popup dialog when #2 runs; then similar for other tests in 0303. For #14 I get a slight alternative with "A keychain cannot be found". There's a "Reset To Defaults" button, but that wipes everything. AFAIK I have a relatively normal setup, with a login keychain as default. macOS 14.3.1; arm64.

$ security list-keychains
   "/Users/rc/Library/Keychains/login.keychain-db"
   "/Library/Keychains/System.keychain"
$ security default-keychain
   "/Users/rc/Library/Keychains/login.keychain-db"
$ security unlock-keychain
password to unlock default: ...

I don't see any settings or code for setting which keychain the credential helper uses, so I guess it's the default one?

Cheers,

Rob :)

[1] https://docs.github.com/en/actions/deployment/deploying-xcode-applications/installing-an-apple-certificate-on-macos-runners-for-xcode-development

The test script does not interact well with the env filtering. This was the case before this change too.

To interact with your default keychain, you will need:

GIT_TEST_CREDENTIAL_HELPER_SETUP="export HOME=$HOME”

This is because the default macOS user keychain is local to your home directory - that’s why it’s giving errors about not finding any.