zalando / go-keyring

Cross-platform keyring interface for Go
MIT License
811 stars 82 forks source link

Added support for prompt unlock broadcast rather than just unicast #87

Closed c-reeder closed 1 year ago

c-reeder commented 1 year ago

Currently, when waiting for a prompt, it will block indefinitely unless the prompt sends back a unicast Completed signal, however that is not the standard.

Though several do, such as gnome keyring, we cannot assume that all implementations of the Secret Service API will do this.

For instance, KeepassXC is a popular password management tool that also implements the Secret Service and it uses a broadcast (destination field is null) message to signal back that a prompt has been completed or dismissed instead of a unicast message (direct to the original sender of the prompt). The discussion here is relevant: https://github.com/keepassxreboot/keepassxc/issues/7759

The interesting points referenced there are this documentation section, specifically citing:

When the message bus receives a signal, if the DESTINATION field is absent, it is considered to be a broadcast signal, and is sent to all applications with message matching rules that match the message. Most signal messages are broadcasts, and no other message types currently defined in this specification may be broadcast.

Unicast signal messages (those with a DESTINATION field) are not commonly used, but they are treated like any unicast message: they are delivered to the specified receipient, regardless of its match rules. One use for unicast signals is to avoid a race condition in which a signal is emitted before the intended recipient can call the section called “org.freedesktop.DBus.AddMatch” to receive that signal: if the signal is sent directly to that recipient using a unicast message, it does not need to add a match rule at all, and there is no race condition. Another use for unicast signals, on message buses whose security policy prevents eavesdropping, is to send sensitive information which should only be visible to one recipient.

Thus, the preferred behavior appears to be leaving support for unicast messages, but also ensuring to receive broadcast ones, which is what I've added support for here.

Very open to discussion on the exact implementation on this and happy to discuss any ideas about specifics. 🙂

However, you'll note that it's a pretty isolated case and doesn't touch much of the code.

szuecs commented 1 year ago

@c-reeder freebsd seems to break. You can try to fix it, no idea what is wrong with it.

You could also break out the func to make FreeBSD at compile time to have old behavior and rest the new behavior. It works based on file name for example: foo_freebsd.go and foo.go

voidus commented 1 year ago

I'm having a bit of a tough time understanding the github actions output, and the last successful run doesn't have logs anymore, but it looks like initializing the freebsd VM failed? Could we maybe re-run it, that sounds like flakyness, right?

szuecs commented 1 year ago

I restarted it now the second time, let's see what returns

voidus commented 1 year ago

The pipeline still fails because of broken test setup. It's failing on master since January as well.

szuecs commented 1 year ago

:+1:

szuecs commented 1 year ago

@voidus I tried to outline how to fix it, but I also don't mind ignoring the error. One day we should fix it for freebsd users

mikkeloscar commented 1 year ago

:+1: