Skip to content
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

Error when storing large token in keyring on macOS #1255

Open
Gerrit-K opened this issue Jan 20, 2025 · 15 comments
Open

Error when storing large token in keyring on macOS #1255

Gerrit-K opened this issue Jan 20, 2025 · 15 comments
Labels
bug Something isn't working

Comments

@Gerrit-K
Copy link

Gerrit-K commented Jan 20, 2025

Describe the issue

kubelogin repeatedly fails with:

error: get-token: could not write the token cache: keyring write kubelogin/tokencache/8f08accac[...]: data passed to Set was too big

To reproduce

Not sure how to reproduce this, as it's probably caused by a large token from our IdP. We do have a lot of groups that are mapped in the token, which significantly increases the token size.

Your environment

  • OS: macOS Sequoia 15.2
  • kubelogin version: v1.32.0
  • kubectl version: v1.30.6
  • OpenID Connect provider: Keycloak

Additional context

I verified that this was not happening in version 1.31.1, so I'm relatively confident this is related to #973. The description of the PR states that

The code prefers the OS keyring, if supported. Falls back to file based cache

However, with every kubectl command, I noticed consecutive failures and wasn't able to obtain a token* to interact with the cluster. There was no fallback to the file-based cache. For a reason I don't understand yet, the new option --no-keyring also doesn't seem to be available (and neither is --force-keyring), so I can't work around this by any other means than downgrading to 1.31.1:

$ kubectl oidc-login --help | grep keyring
$ kubectl oidc-login --version
kubelogin version 1.32.0

*in fact, I did get a token (which I could verify with the verbose logging), but kubelogin wasn't able to persist it

@Gerrit-K Gerrit-K added the bug Something isn't working label Jan 20, 2025
@mhanc
Copy link

mhanc commented Jan 20, 2025

I can confirm the issue to be related to kubelogin version 1.32.0. Downgrading to 1.31.1 fixes the issue as a workaround.

Not working:

$ kubectl oidc-login --version
kubelogin version 1.32.0

Error message:

error: get-token: could not write the token cache: keyring write kubelogin/tokencache/4b405edc3945a844f98861a1fb987a4297184c85f01d6bf4ebef54fe15b5cb70: data passed to Set was too big

Working version:

kubectl oidc-login --version
kubelogin version 1.31.1

@jamesrwhite
Copy link
Contributor

jamesrwhite commented Jan 20, 2025

I'm also seeing this issue since upgrading from 1.31.0 to 1.32.0.

❯ kubelogin version
kubelogin version v1.32.0 (go1.23.5 darwin_arm64)

I was able to workaround it by setting the --token-cache-storage=disk option.

Our IdP is AWS Cognito and token length is around ~1,300 characters.

@bbrala
Copy link

bbrala commented Jan 21, 2025

Same same :) using keycloak/microsoft entraid

@kelveden
Copy link

kelveden commented Jan 21, 2025

Same issue. I can confirm that jamesrwhite's workaround works for me - i.e. add the --token-cache-storage=disk option to the options for get-token in my kube config file.

Technical info:

  • kubelogin version: 1.32.0
  • Hardware: Apple M2 Pro
  • OS: Sonoma 14.6.1

So, it looks like possibly an integration issue with the OS keyring on MacOS perhaps?

@jamesrwhite
Copy link
Contributor

In the underlying library (go-keyring) it claims the limit for the keychain is around 3,000 bytes (source) but if you look at how it's actually implemented it's the combination of the service, username and password that you pass to Set() that cannot exceed 4096 bytes (source).

Worth noting this is also once those values have been base64 encoded and shell escaped.

A potential fix could be to calculate the length in the same way before calling Set() in the logic that selects which storage to use or alternatively to catch the ErrSetDataTooBig error here and fallback to using disk storage.

Catching the error and falling back to disk storage seems simpler and less likely to break if the underlying library changes it's implementation in my opinion.

I'd be happy to open a PR for this.

@minimax75
Copy link

minimax75 commented Jan 22, 2025

Hi,
I have the same issue on a Windows-WSL machine. I have no keyring installend and the fallback to disk is not happening.
--token-cache-storage=auto should automatically fallback to disk, if keyring is not available.

As already mentioned --token-cache-storage=disk fix the problem, but yould force us to update all our kubeconfigs

OS: Win11
kubelogin version: v1.32.0
kubectl version: v1.32.1
Ubuntu: 24.04 on WSL
OpenID Connect provider: EntraID

WSL-Version: 2.3.26.0
Kernelversion: 5.15.167.4-1
WSLg-Version: 1.0.65
MSRDC-Version: 1.2.5620
Direct3D-Version: 1.611.1-81528511
DXCore-Version: 10.0.26100.1-240331-1435.ge-release
Windows-Version: 10.0.22631.4751

@int128
Copy link
Owner

int128 commented Jan 25, 2025

v1.32.1 is available with the fix of #1257.

I think it is difficult to cover all edge cases in the fallback mode. I tested the keyring on Linux and it causes an error if dbus is not running.

I'd like to change the default mode to --token-cache-storage=disk. Any opinions?

@jamesrwhite
Copy link
Contributor

@int128 thanks for merging and releasing my hotfix, I can confirm I'm no longer getting the same issue with v1.32.1 🥳

@bbrala
Copy link

bbrala commented Jan 27, 2025

<3

@anders-elastisys
Copy link

anders-elastisys commented Jan 28, 2025

I'd like to change the default mode to --token-cache-storage=disk. Any opinions?

I had a use case where I was running oidc-login from a container which became problematic with the new keyring changes as it required dbus to be running inside the container causing this error:

error: get-token: could not write the token cache: keyring write kubelogin/tokencache/...: dbus: couldn't determine address of session bus
Unable to connect to the server: getting credentials: exec: executable kubectl failed with exit code 1

Setting the --token-cache-storage=disk option in kubeconfigs or downgrading kubelogin to v1.31.1 solved the issue, so if the old behavior could be the default for kubelogin would be nice, or kubelogin falling back to disk if needed.

@ergonab
Copy link

ergonab commented Jan 28, 2025

I'd like to change the default mode to --token-cache-storage=disk. Any opinions?

It does make sense to use the OS keyring when available, since it is arguably more secure than just a file somewhere on disk. Having a fallback to disk when all else fails is good enough IMHO. If there are other errors that need to be exempted then those should be addressed separately, just like in #1257.

@int128
Copy link
Owner

int128 commented Jan 28, 2025

I think the fallback mode will cause the complicated problem.

It needs to handle a case if both keyring and disk have the credentials. For example,

  1. It stores the credentials to keyring.
  2. It loads the credentials from keyring.
  3. It possibly stores the credentials to the disk, for example, when the provider returns a large ID token.
  4. It loads the credentials from keyring but it is expired. It always requests re-login.

It is possible to try to load the credentials from keyring and then disk, but I think it brings the other problems:

  • It causes a flaky behavior if the credentials size is approximately 3,000 bytes.
  • Credentials may be written to the disk. For the security, it is nice to enforce the keyring usage to prevent it.

@ergonab
Copy link

ergonab commented Jan 28, 2025

The decision could also be offloaded to the user. The problem with the original behaviour in 1.32.0 was that you just got an error from the Go library without any indication what went wrong. If the keyring was the default, the user could be given the choice to switch to disk storage, if an error with the keyring occurs. Then, being made aware of the security implications, the user could opt-in to storing it in a less secure place (on disk) by passing the --token-cache-storage=disk flag. But the error message needs to indicate this and explain to the user what to do.

Of course this is suboptimal long-term when the Go library is (hopefully) fixed to support longer tokens, but at least the user knows what happens with his token data and we tried the more secure option first.

@SISheogorath
Copy link

SISheogorath commented Jan 30, 2025

If the token is too large, it maybe makes sense to switch to envelope encryption?

Generate a secret, encrypt the token with it, and store the key in the keyring. The key size is easy to control.


Another idea, that might sound a bit silly but should be possible: Technically speaking only the signature of the token is security relevant. So storing just the signature of the token in the keyring is enough for security.

However when people use access tokens instead of login tokens, there is no requirement for a JWT and therefore one can't just split the signature away.


All in all, I want to emphasise: It's good we have these problems now, and we should solve them without offloading them to users, because offloading security to users is the worst security choice.

@int128
Copy link
Owner

int128 commented Jan 31, 2025

That's right. I think we will be able to enforce keyring usage by default when the implementation is enough mature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

10 participants