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

kwallet support #66

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

SoMuchForSubtlety
Copy link

This PR replaces #52 from @bearsh

the kwallet (kde) backend communicates through dbus with the kwalletd.
it is modeled after the qtkeychain implementation [1].
like qtkeychain, the keyring backend tries to get the kwallet's
'networkWallet' if this fails the 'secret service' is used as before.
kwallet is tried first as the 'secrets service' may exists on systems
running kde but it's very unlikely kwallet is running on non kde
systems.

[1] https://github.com/frankosterfeld/qtkeychain

Signed-off-by: Martin Gysel <me@bearsh.org>
@SoMuchForSubtlety SoMuchForSubtlety marked this pull request as ready for review November 7, 2021 12:41
Signed-off-by: SoMuchForSubtlety <jakob@ahrer.dev>
Signed-off-by: SoMuchForSubtlety <jakob@ahrer.dev>
Signed-off-by: SoMuchForSubtlety <jakob@ahrer.dev>
@szuecs
Copy link
Member

szuecs commented Nov 8, 2021

I see multiple possible issues with this change:

  • public interface was changed quite a lot
  • the kwallet build will be done on all platforms as far as I understand

I let @mikkeloscar decide if it's great or not.

@SoMuchForSubtlety
Copy link
Author

  • public interface was changed quite a lot

I had to change the signature of SecretService.Delete for it to satisfy the Keyring interface. It would probably be better to create a wrapper type?

Same for the errors, in order to make them accessible in the kwallet and secret_service packages they need to be in a separate package, but this could also be solved with wrapper types. (although I don't think declaring the errors in a different package is a breaking change)

@syncjuncture
Copy link

syncjuncture commented Feb 6, 2022

Any progress? @mikkeloscar @SoMuchForSubtlety. Looks like 99designs/keyring supports KWallet.

@technodrome
Copy link

Is this still being actively developed?

@SoMuchForSubtlety
Copy link
Author

Is this still being actively developed?

I would need feedback from someone from the zalando team to continue.

I ended up only using go-keyring for macOS and falling back to 99designs/keyring for everything else.

@szuecs
Copy link
Member

szuecs commented Apr 6, 2022

Maybe suggest the change for the SecretService.Delete wrapper type?
For me this sounds ok if we can hold the public interface.
Error types I am not too much concerned about.

I hope this helps.

@mikkeloscar
Copy link
Member

I have lost track of this one, but will have a look tomorrow and give some feedback. Sorry for letting you hanging like this @SoMuchForSubtlety

@mikkeloscar
Copy link
Member

I have now had a look and I think it looks quite good. I believe it's ok to break the public interface for the SecretService interface as we only really aim to offer stable support for the public keyring API.

It would be good if you could provide a small guide for the steps needed to migrate in case anyone depends on the SecretService interface.

Also the PR needs a rebase :)

@technodrome
Copy link

Nice! Looking forward to trying this out.

@technodrome
Copy link

Any news when this is going to be merged?

@sm1999
Copy link

sm1999 commented Aug 23, 2022

Kwallet recently added support for Secret Service API.

@mrueg
Copy link
Contributor

mrueg commented Aug 25, 2022

Kwallet recently added support for Secret Service API.

Looks like this does not work as "plain" Algorithm is not supported.
Trying to use it, I get: Algorithm plain is not supported. (only dh-ietf1024-sha256-aes128-cbc-pkcs7 is supported)
See: https://invent.kde.org/frameworks/kwallet/-/blob/master/src/runtime/kwalletd/kwalletfreedesktopservice.cpp#L265

Probably either kwallet should add support for plain, or maybe

err := s.object.Call(serviceInterface+".OpenSession", 0, "plain", dbus.MakeVariant("")).Store(&disregard, &sessionPath)
could be changed to use other algorithms as well.

edit: fwiw I've created an issue with kwallet to support plain as well: https://invent.kde.org/frameworks/kwallet/-/issues/3

@SuperSandro2000
Copy link

FYI kwallet since plasma frameworks 5.97.0 supports the secrets api https://kde.org/announcements/frameworks/5/5.97.0/#:~:text=Introduce%20Secret%20Service%20API

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants