-
Notifications
You must be signed in to change notification settings - Fork 192
osxkeychain: store: add atyp attribute #367
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.
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #367 +/- ##
==========================================
+ Coverage 51.28% 52.08% +0.79%
==========================================
Files 13 13
Lines 661 672 +11
==========================================
+ Hits 339 350 +11
Misses 278 278
Partials 44 44 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| // | ||
| // In order to keep compatibility with older versions, we need to store | ||
| // credentials with this attribute set. | ||
| item.SetAuthenticationType("dflt") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a follow-up, it'd be great to add this constant and the ones I hardcoded in #361 to keybase/go-keychain.
I started doing that, but it's lower priority than getting this fixed here before we release the next version of Docker Desktop with a creds store that doesn't support downgrades.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hum yes indeed I looked at previous implem https://github.com/docker/docker-credential-helpers/pull/282/files#diff-13e5d0d1bee45d43439783edc43138e1d9584d2ec29cab4969460384db72ec42L28 and we set it to dflt as well.
I thought dflt was the default SecAuthenticationType in keybase/go-keychain so agree to set this upstream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! I forgot to add a link to keychain_get in my comment. Let me update it before I merge.
Prior to v0.9.0, the osxkeychain creds helper was adding the `atyp` attribute (ie. authentication type) to its credentials. It was also specifying this attribute when querying the keychain for credentials. Since v0.9.0, we don't set this attribute anymore. So, if a credential is stored with v0.9.0+ and then queried with a v0.8.2 helper, the atyp attribute will be missing and the credential won't be found. Signed-off-by: Albin Kerouanton <[email protected]>
79117ca to
e7bd395
Compare
- What I did
Prior to v0.9.0, the osxkeychain creds helper was adding the
atypattribute (ie. authentication type) to its credentials. It was also specifying this attribute when querying the keychain for credentials.Since v0.9.0, we don't set this attribute anymore. So, if a credential is stored with v0.9.0+ and then queried with a v0.8.2 helper, the atyp attribute will be missing and the credential won't be found.
- How to verify it
- Description for the changelog