Improve credentials manager selection

[dmach]

Implemented new keyctl credentials manager.
Order credential managers by priority and name.
Credential manager with the highest prio is a default now.
Remove unwanted python-keyrings backends.
Made clear which credentials managers are (in)secure.
Briefly tested the code on python2.

[marcus-h]

On 2021-12-09 07:31:10 -0800, Daniel Mach wrote: This PR is still incomplete. Goals: - [x] Implement kernel keyring password backend

Nice!:) But see below.

- [ ] Set as default for new installations

Hmm there is no default anymore. The user has to choose a credentials manager during the setup of an apiurl.

- [ ] Warn user if insecure backend is used

Hmm that's not possible/trivial because, for instance, a user can configure _any_ python-keyring backend (in theory, we do not have a "fixed" number of potential credential managers).

Unresolved issues: - [ ] py2 support (Popen doesn't support `encoding`) - [ ] when removing password from the keyring (`keyctl purge user osc_https://api.opensuse.org`), osc asks for username again

What about using the python-keyring-keyutils module [1]? It also ships a "special" osc keyring backend [2] (so that osc does not always ask for a password). Its usage wrt. to osc is described here [3]. The corresponding pythonXY-keyring-keyutils package is already available in tumbleweed. [1] https://github.com/marcus-h/python-keyring-keyutils [2] https://github.com/marcus-h/python-keyring-keyutils/blob/master/keyutils/osc.py [3] https://github.com/marcus-h/python-keyring-keyutils/blob/master/README_osc.md

[dmach]

@marcus-h I'll definitely take a look.

BTW, haven't you considered splitting your lib in the following way?

• Submit the generic code to python-keyring.
• Move the osc specific code to osc.

Is python2 supported in your library?
Plain super() indicates that it may not be supported.
I was told that py2 support in osc is probably not going away for a while.

[marcus-h]

On 2021-12-10 01:13:49 -0800, Daniel Mach wrote: @marcus-h I'll definitely take a look.

Cool! I'm always interested in feedback, objections, bug reports, PRs etc.:)

BTW, haven't you considered splitting your lib in the following way? - Submit the generic code to python-keyring. - Move the osc specific code to osc.

I haven't thought about this yet but if there's interested in it, I can certainly move the code.

Is python2 supported in your library? Plain `super()` indicates that it may not be supported.

No. Also, the C extension is written with python3 in mind (it won't compile on python2).

I was told that py2 support in osc is probably not going away for a while.

My personal take on this is the following: - let's not actively break python2 support in osc (unless there is a really good reason) - it is perfectly fine if a new feature relies on python3

[pep8speaks]

Hello @dmach! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-03-28 07:50:18 UTC

[dmach]

Force-pushed brand new code based on python-keyring-keyutils.

The credentials manager selection looks like this now:

the apiurl 'https://example.com' does not exist in the config file. Please enter
your credentials for this apiurl.

Username: USER
Password: 

NUM NAME              DESCRIPTION
1   Kernel keyring    Store password in user session keyring in kernel keyring [secure, in-memory, per-session]
2   KWallet           Store password in KWallet [secure, persistent]
3   Transient         Do not store the password and always ask for it [secure, in-memory]
4   Obfuscated config Store the password in obfuscated form in the osc config file [insecure, persistent]
5   Config            Store the password in plain text in the osc config file [insecure, persistent]
Select credentials manager [default=1]: 

[marcus-h]

I just had a very very quick glance at it: I really like it:)

On 2022-03-24 13:43:00 +0000, Daniel Mach wrote: diff --git a/osc/credentials.py b/osc/credentials.py index d2ce493b..a0f789df 100644 --- a/osc/credentials.py +++ b/osc/credentials.py @@ -195,16 +195,21 @@ def delete_password(self, url, user):

<SNIP>

def get_credentials_manager_descriptors(): - if has_keyring_support(): - backend_list = keyring.backend.get_all_keyring() - else: - backend_list = [] descriptors = [] - for backend in backend_list: - descriptors.append(KeyringCredentialsDescriptor(backend)) + + if has_keyring_support(): + for backend in keyring.backend.get_all_keyring(): + qualified_backend_name = qualified_name(backend) + data = SUPPORTED_KEYRING_BACKENDS.get(qualified_backend_name, None) + if not data: + continue

Hmm IMHO we should also offer the other backends (except "useless" ones like fail, null etc.). We could give them a lower priority, though. What do you think?

…

+ descriptor = KeyringCredentialsDescriptor(backend, data["name"], data["description"]) + descriptors.append(descriptor)

[dmach]

Hmm IMHO we should also offer the other backends (except "useless" ones like fail, null etc.). We could give them a lower priority, though. What do you think?

Could you be more specific about which backends do you want enabled?

I've reviewed classes python-keyring provides and added keyring.backends.SecretService.Keyring to the list.

These are skipped on my system and I believe they should remain that way:

keyring.backends.fail.Keyring
keyutils.backend.KeyutilsKeyringBackend
keyring.backends.chainer.ChainerBackend

Do we really want to support keyring.backends.kwallet.DBusKeyringKWallet4?

Did I miss anything?

[marcus-h]

Hmm github ignored my mail again... here's the forwarded message:

----- Forwarded message from Marcus Hüwe <suse-tux@gmx.de> -----

Date: Mon, 28 Mar 2022 10:33:16 +0200
From: Marcus Hüwe <suse-tux@gmx.de>
To: reply+AAIEUTH5A4XPNL3BSFET3QWAJ2NG5EVBNHHEAHPWNE@reply.github.com
Subject: Re: [openSUSE/osc] Improve credentials manager selection (PR #983)

On 2022-03-28 00:46:54 -0700, Daniel Mach wrote:
> > Hmm IMHO we should also offer the other backends (except "useless" ones like fail, null etc.). We could give them a lower priority, though. What do you think?
> 
> Could you be more specific about which backends do you want enabled?
> 
IMHO, all except the ones that we explicitly want to disable (like the
ones you listed below). For instance, there could be third party
keyrings that are not part of the official python-keyring package.

> I've reviewed classes python-keyring provides and added `keyring.backends.SecretService.Keyring` to the list.
> 
> These are skipped on my system and I believe they should remain that way:
> ```
> keyring.backends.fail.Keyring
> keyutils.backend.KeyutilsKeyringBackend
> keyring.backends.chainer.ChainerBackend
> ```
> 
I'm fine with skipping those.

> Do we really want to support `keyring.backends.kwallet.DBusKeyringKWallet4`?
> 
Hmm I would rather say s/support/offer/ - eventually, it is up to the
user what they wants to use.

----- End forwarded message -----

[lethliel]

Ok.. This works and is already discussed.. I will merge this now..
Also that this is done in several commits is fine.

[marcus-h]

On 2022-03-30 01:21:38 -0700, Marco Strigl wrote: Ok.. This works and is already discussed.. I will merge this now..

Hmm I really fail to see why we limit ourselves to SUPPORTED_KEYRING_BACKENDS instead of offering all available backends (except the nonsense ones...) :/ Strictly speaking, this is again a (small) regression.