zalando / go-keyring

Cross-platform keyring interface for Go
MIT License
881 stars 86 forks source link

writing large passwords in darwin leads to truncated password #84

Closed mfmarche closed 1 year ago

mfmarche commented 2 years ago

Issue appears with "/usr/bin/security -i". When invoking 'add-generic-password', the password value is limited to < 1024 bytes it seems.

mfmarche commented 2 years ago

I will leave this open still (I closed pull request #85 that attempted to resolve it). Since the result of trying a value that is too big leads to strange results (broken expected password values), a solution that returns error on values > x is probably another way to solve this.

fiws commented 2 years ago

The behaviour is pretty weird actually:

i tried to mitigate this in #86

willvedd commented 1 year ago

Can confirm that secrets are getting truncated in v0.2.0 on my mac. In my specific instance, I'm attempting to store a string of 2068 bytes but it gets truncated to 1996 bytes. The truncation is sometimes paired with a encoding/hex: odd length hex string error (see #80) but not always.

Downgrading to v0.1.1 fixes the issue for me.

willvedd commented 1 year ago

Following up – The truncation is due to a limit imposed by the Mac OS. This open source file appears to be where the add-generic-password command is defined. In this file I see this line that appears to set the max secret limit:

/* Maximum length of an input line in interactive mode. */
#define MAX_LINE_LEN 4096

I'm unable to definitely link the 4096 limit to the command, but all evidence points to there being a 4096 byte limit somewhere.

My conclusion is that the hex encoding introduced in v0.2.0 effectively halves the practical secret length limit for this library. At best, this limits the practical applicability of this library. But at worst, it can cause nefarious bugs to pop up since the truncation occurs silently. In our case, we were trying to store a JWT token which only slightly exceeded the limit, but in theory, some JWTs could skirt right underneath that limit and fail intermittently if the payload is large enough.

I plea to the maintainers of this library to:

  1. Rethink the hex encoding to enable storage of longer secrets
  2. Enforce the 4096 byte limit at the time of storage to eliminate instances of truncation
fiws commented 1 year ago
  1. Rethink the hex encoding to enable storage of longer secrets
  2. Enforce the 4096 byte limit at the time of storage to eliminate instances of truncation

i did both of those in my PR #86 You can use my patch by adding replace github.com/zalando/go-keyring v0.2.1 => github.com/fiws/go-keyring v0.0.0-20221010191512-5b6827277d3b to your go.mod

mikkeloscar commented 1 year ago

@fiws Thanks for the PR. This is now merged. I hope it improves the experience.

sergiught commented 1 year ago

@mikkeloscar 👋🏻 Would it be possible to cut a new release with the fix bitte? 🙏🏻

mikkeloscar commented 1 year ago

v0.2.2 is now tagged!