Skip to content

osxkeychain: bring in line with other credential helpers #1667

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Bo98 wants to merge 4 commits into from
Closed

osxkeychain: bring in line with other credential helpers #1667

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f703131
osxkeychain: replace deprecated SecKeychain API
Bo98 Feb 13, 2024
08284fa
osxkeychain: erase all matching credentials
Bo98 Feb 13, 2024
f7ac228
osxkeychain: erase matching passwords only
Bo98 Feb 13, 2024
f18435b
osxkeychain: store new attributes
Bo98 Feb 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion contrib/credential/osxkeychain/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ CFLAGS = -g -O2 -Wall
-include ../../../config.mak
Copy link

@gitgitgadget gitgitgadget bot Feb 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Eric Sunshine wrote (reply to this):

On Sat, Feb 17, 2024 at 6:35 PM Bo Anderson via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> 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 <mail@boanderson.me>

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...

> diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
> @@ -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;
> +       }
> +       [...]
> +}
> +
> +__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.

> @@ -19,70 +44,135 @@ static void die(const char *err, ...)
> +static CFDictionaryRef create_dictionary(CFAllocatorRef allocator, ...)
> +{
> +       va_list args;
> +       const void *key;
> +       CFMutableDictionaryRef result;
> +
> +       result = CFDictionaryCreateMutable(allocator,
> +                                          0,
> +                                          &kCFTypeDictionaryKeyCallBacks,
> +                                          &kCFTypeDictionaryValueCallBacks);
> +
> +

Style: one blank line is preferred over two

> +       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.

> +static void find_username_in_item(CFDictionaryRef item)
>  {
> +       CFStringRef account_ref;
> +       char *username_buf;
> +       CFIndex buffer_len;
>
> +       account_ref = CFDictionaryGetValue(item, kSecAttrAccount);
> +       if (!account_ref)
> +       {
> +               write_item("username", "", 0);
> +               return;
> +       }

Style: opening brace sticks to the `if` line:

    if !(account_ref) {
        ...
    }

Same comment applies to the `if` below.

> +       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.

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

Style:

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

> +       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.

> +static OSStatus find_internet_password(void)
>  {
> +       CFDictionaryRef attrs;
> +       [...]
>
> +       attrs = CREATE_SEC_ATTRIBUTES(kSecMatchLimit, kSecMatchLimitOne,
> +                                     kSecReturnAttributes, kCFBooleanTrue,
> +                                     kSecReturnData, kCFBooleanTrue,
> +                                     NULL);
> +       result = SecItemCopyMatching(attrs, (CFTypeRef *)&item);
> +       if (result) {
> +               goto out;
> +       }

We omit braces when the body is a single statement:

    if (result)
        goto out;

(Same comment applies to other code in this patch.)

> +       data = CFDictionaryGetValue(item, kSecValueData);
> +       [...]
> +
> +out:
> +       CFRelease(attrs);

Good, `attrs` is released in all cases.

> +static OSStatus add_internet_password(void)
>  {
> +       [...]
> +       attrs = CREATE_SEC_ATTRIBUTES(kSecValueData, password,
> +                                     NULL);
> +       result = SecItemAdd(attrs, NULL);
> +       if (result == errSecDuplicateItem) {
> +               CFDictionaryRef query;
> +               query = CREATE_SEC_ATTRIBUTES(NULL);
> +               result = SecItemUpdate(query, attrs);
> +               CFRelease(query);
> +       }
> +       CFRelease(attrs);
> +       return result;
>  }

Good, `attrs` and `query` are released in all cases.

Copy link

@gitgitgadget gitgitgadget bot Feb 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Bo Anderson wrote (reply to this):

> On 18 Feb 2024, at 06:08, Eric Sunshine <sunshine@sunshineco.com> wrote:
> 
> 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...

Thanks for taking a look!

>> diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
>> @@ -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;
>> +       }
>> +       [...]
>> +}
>> +
>> +__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.

Acknowledged. It is indeed a bit of a nothing change that doesn’t really do much on its own, but when paired with the port variable reorder could potentially make a “minor code cleanup” commit.

>> +       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.
> 

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.

>> +       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.
> 
>> +       /* If we can't get a CString pointer then
>> +        * we need to allocate our own buffer */
> 
> Style:
> 
> /*
>  * Multi-line comments
>  * are formatted like this.
>  */
> 
>> +       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.

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.

Copy link

@gitgitgadget gitgitgadget bot Feb 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Eric Sunshine wrote (reply to this):

On Sun, Feb 18, 2024 at 9:48 AM Bo Anderson <mail@boanderson.me> wrote:
> > On 18 Feb 2024, at 06:08, Eric Sunshine <sunshine@sunshineco.com> wrote:
> >> +    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);
> >
> > 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.

Okay. I had thought that this check was merely protecting against
programmer error, but the described use-case to avoid passing NULL to
SecItem API makes perfect sense. It might be helpful to future readers
to explain this either as a function-level comment (explaining how to
call the function) or as an in-code comment.

> >> +    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.
> >
> >> +    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: [...]
> >
> > 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.

Most likely, anyone working on this code is already familiar with the
CoreFoundation API, thus would understand implicitly that this isn't
leaking. But, yes, a simple comment should be plenty sufficient for
everyone else if you are re-rolling anyhow.


git-credential-osxkeychain: git-credential-osxkeychain.o
$(CC) $(CFLAGS) -o $@ $< $(LDFLAGS) -Wl,-framework -Wl,Security
$(CC) $(CFLAGS) -o $@ $< $(LDFLAGS) \
-framework Security -framework CoreFoundation

git-credential-osxkeychain.o: git-credential-osxkeychain.c
$(CC) -c $(CFLAGS) $<
Expand Down
Loading