| Message ID | pull.1667.git.1708212896.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | osxkeychain: bring in line with other credential helpers | expand |
On Sat, Feb 17, 2024 at 6:35 PM Bo Anderson via GitGitGadget <gitgitgadget@gmail.com> wrote: > 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). Although I'm not familiar with the SecItem API nor with the Git keychain API, I gave this series a readthrough and left a few minor comments. Aside from a few very minor style nits, perhaps the only substantive comment was that patch [1/4] could do a slightly better job of protecting against future programmer error, but even that is minor in that it doesn't impact the functionality actually implemented by the patch, thus may not be worth a reroll. Overall, despite not being familiar with the APIs in question, everything I read in the patches made sense and was cleanly implemented. Nicely done.
> 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 credential helper test suite. Great work! Could these tests run as part of macOS CI?
> On 18 Feb 2024, at 20:40, M Hickford <mirth.hickford@gmail.com> wrote: > >> 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 > credential helper test suite. > > Great work! > > 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.
On Sun, 18 Feb 2024 at 23:24, Bo Anderson <mail@boanderson.me> wrote: > > > On 18 Feb 2024, at 20:40, M Hickford <mirth.hickford@gmail.com> wrote: > > > > > > Could these tests run as part of macOS CI? > > Do we do so for any of the other external credential helpers? > We don't. > 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.
On Mon, Mar 04, 2024 at 08:00:00AM +0000, M Hickford wrote: > > 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. I think we suffer from that somewhat already. You cannot run t0303 successfully against credential-store anymore, as of 0ce02e2fec (credential/libsecret: store new attributes, 2023-06-16). There is some prior art in the GIT_TEST_CREDENTIAL_HELPER_TIMEOUT variable, as time is not a concept to every helper (like store, for example). Other new tests like the password-expiry and oauth features could be gated on similar variables. That would help non-CI users testing helpers manually, and then CI jobs could set the appropriate switches for each helper that they cover. 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. -Peff
> From: "Bo Anderson via GitGitGadget" <gitgitgadget@gmail.com> > To: git@vger.kernel.org > Cc: Bo Anderson <mail@boanderson.me> > 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: <pull.1667.git.1708212896.gitgitgadget@gmail.com> (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 > -- > gitgitgadget Hi. Is this patch ready to cook in seen?
M Hickford <mirth.hickford@gmail.com> writes: >> From: "Bo Anderson via GitGitGadget" <gitgitgadget@gmail.com> >> To: git@vger.kernel.org >> Cc: Bo Anderson <mail@boanderson.me> >> 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: <pull.1667.git.1708212896.gitgitgadget@gmail.com> (raw) >> ... >> git-credential-osxkeychain has largely fallen behind other external > Hi. Is this patch ready to cook in seen? A better nudge would be for somebody who uses macOS to resend the patches with their "Tested-by:" trailers added ;-). Thanks.
Hi all, > 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. Bo > On 2 Apr 2024, at 14:21, Robert Coup <robert.coup@koordinates.com> wrote: > > Hi all, > >> 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
Hi Bo, On Tue, 2 Apr 2024 at 14:53, Bo Anderson <mail@boanderson.me> wrote: > > The test script does not interact well with the env filtering. This was the case before this change too. I guess without writing a helper-specific test or having some per-helper-setup thing it's a bit tricky. > 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. And with that, the tests all pass :-) Comparing with master where 7/21 failed. Tested-by: Robert Coup <robert.coup@koordinates.com> Could we document that setup step somewhere? I guess the simplest is probably just to put it in the header of t/t0303-credential-external.sh; maybe along the lines of the patch below. Rob :) diff --git a/t/t0303-credential-external.sh b/t/t0303-credential-external.sh index 095574bfc6..e4e693b233 100755 --- a/t/t0303-credential-external.sh +++ b/t/t0303-credential-external.sh @@ -27,6 +27,13 @@ timeout, you can test that feature with: If your helper requires additional setup before the tests are started, you can set GIT_TEST_CREDENTIAL_HELPER_SETUP to a sequence of shell commands. + +- osxkeychain: + + Because the default macOS user keychain is local to your home + directory, you will need: + + GIT_TEST_CREDENTIAL_HELPER_SETUP="export HOME=$HOME” ' . ./test-lib.sh