[MIR] python-keyring 0.5.1

Bug #686257 reported by Matthias Klose
18
This bug affects 1 person
Affects Status Importance Assigned to Milestone
NULL Project
Invalid
Undecided
Unassigned
python-keyring (Debian)
Fix Released
Unknown
python-keyring (Ubuntu)
Fix Released
High
Unassigned

Bug Description

Binary package hint: python-keyring

MIR needed (dependency of python-launchpadlib)

Tags: python27

Related branches

Revision history for this message
Matthias Klose (doko) wrote :

[please reassign if appropriate; I would like to see this addressed before switching the python default]

Changed in python-keyring (Ubuntu):
assignee: nobody → Canonical Desktop Team (canonical-desktop-team)
importance: Undecided → High
milestone: none → natty-alpha-2
status: New → Confirmed
Matthias Klose (doko)
tags: added: python27
Revision history for this message
Martin Pitt (pitti) wrote :

I'll check the package for main requirements.

Changed in python-keyring (Ubuntu):
assignee: Canonical Desktop Team (canonical-desktop-team) → Martin Pitt (pitti)
status: Confirmed → Triaged
Revision history for this message
Martin Pitt (pitti) wrote :

- No bug report in Debian; very calm Debian maintenance, there's a much newer upstream version 0.5 which didn't get packaged.
- Two non-critical bug reports in Ubuntu, one is fixed upstream in 0.5.
- Version 0.5 indeed looks a lot better, as it removes a lot of code duplication and uses more existing libraries (like libgnome-keyring). This version should be packaged first.
- Relatively small package, most of which is glue code.
- The main problem that I see here is that it's handling a lot of passwords, and doesn't use any kind of mlock()-like protection anywhere. So passwords are copied around a lot and get easily written to disk unencrypted, once this gets into swap.
- No i18n or usability issues, it's a backend library.
- Not actively maintained in Ubuntu.

Aside from this, it needs to be investigated what launchpadlib now does with this module. Previously it stored its cookie files on disk in ~/.launchpadlib.., and it seems this change will not only break the existing credentials files, but might also cause trouble with using launchpadlib on servers, where no native keyring servers are available. python-keyring has its own native implementation using python-crypto (Recommends:, already in main), but I haven't reviewed this for security. Perhaps Kees can take a look at this?

Revision history for this message
Martin Pitt (pitti) wrote :

@launchpadlib developers. NEWS for 1.8 now says

- Store authorization tokens in the Gnome keyring or KDE wallet, when
  available. The credentials_file parameter of Launchpad.login_with() is now
  ignored.

This sounds like deliberately breaking backwards compatibility with existing setups, which rely on existing credentials files. Can we at least keep backwards compatibility for existing credentials files, and the credentials_file parameter for a while? What happens if you use the new launchpadlib on servers where there are no native keyring services, and stuff runs from cron?

Changed in python-keyring (Ubuntu):
assignee: Martin Pitt (pitti) → nobody
Revision history for this message
Benji York (benji) wrote :

The login_with class method and the closely related get_token_and_login
are only to be used by interactive applications (not cron scripts).

The get_token_and_login docstring explains:

    This is a convenience method which will open up the user's preferred
    web browser and thus should not be used by most applications.
    Applications should, instead, use Credentials.get_request_token() to
    obtain the authorization URL and
    Credentials.exchange_request_token_for_access_token() to obtain the
    actual OAuth access token.

Revision history for this message
Benji York (benji) wrote :

To put a little more meat on the bones of my previous message:

Non-interactive apps will continue to work as they did previously (using
the technique mentioned in the quoted docstring, which was included in
the previous version) and interactive apps (that use login_with or
get_token_and_login) will prompt the user on the first use after
upgrading launchpadlib.

The retired argument (credentials_file) is of the login_with class
method, which would never have been a good idea for non-interactive apps
because of the aforementioned web browser invocation.

We interacted with Kees heavily during the integration of launchpadlib
with python-keyring, but he hasn't reviewed the python-keyring library
itself (to my knowledge). What kind of follow up with him should be
done and should we do it?

Revision history for this message
Martin Pitt (pitti) wrote :

Erm, we make heavy use of login_with() in noninteractive scripts that run from cron. Until recently this was "the" official method to log into launchpad. So claiming "Non-interactive apps will continue to work as they did previously" is certainly wrong, since it currently forces the creation of a keyring and usage of a password (this is also a nuisance in interactive apps, FWIW).

Instead of breaking existing API, could the new keyring method perhaps get its own, and the existing login_with() and credentials_file would keep working as before?

(Sorry, at this point we should probably open a separate bug)

Revision history for this message
Martin Pitt (pitti) wrote :

I filed bug 686690 about the API breakage.

Revision history for this message
Martin Pitt (pitti) wrote :

This also breaks CD builds; as this MIR is far from trivial, I reverted natty to 1.6.2 for now, so that we can discuss bug 686690 without being "under the gun".

Changed in python-keyring (Ubuntu):
milestone: natty-alpha-2 → none
Changed in launchpadlib:
status: New → Invalid
affects: launchpadlib → null
Barry Warsaw (barry)
summary: - MIR needed (dependency of python-launchpadlib)
+ upgrade to python-keyring 0.5 (and MIR)
Changed in python-keyring (Ubuntu):
assignee: nobody → Barry Warsaw (barry)
milestone: none → natty-alpha-3
Barry Warsaw (barry)
Changed in python-keyring (Ubuntu):
assignee: Barry Warsaw (barry) → nobody
milestone: natty-alpha-3 → none
Revision history for this message
Leonard Richardson (leonardr) wrote : Re: upgrade to python-keyring 0.5 (and MIR)

Kees, when you have some time can we get your opinion on the python-keyring module? Specifically this concern noted by Martin:

The main problem that I see here is that it's handling a lot of passwords, and doesn't use any kind of mlock()-like protection anywhere. So passwords are copied around a lot and get easily written to disk unencrypted, once this gets into swap.

Feel free to ping benji for a walk through the code.

Changed in null:
assignee: nobody → Kees Cook (kees)
Changed in null:
assignee: Kees Cook (kees) → nobody
Revision history for this message
Martin Pitt (pitti) wrote :

Note that my NACK from above referred to the broken and outdated 0.2 version only. I haven't checked 0.5 yet (as it isn't even packaged), and I didn't look into the upstream code whether there are any improvements with memory locking.

Revision history for this message
Martin Pitt (pitti) wrote :

To be clear, while I don't like 0.5 (or python password handling in general), I'd still be okay with having it in main, for launchpadlib (especially as it still supports unencrypted credentials files as well).

Barry Warsaw (barry)
Changed in python-keyring (Ubuntu):
assignee: nobody → Barry Warsaw (barry)
milestone: none → natty-alpha-3
Revision history for this message
Barry Warsaw (barry) wrote : Re: upgrade to python-keyring 0.5.1 (and MIR)
summary: - upgrade to python-keyring 0.5 (and MIR)
+ upgrade to python-keyring 0.5.1 (and MIR)
Revision history for this message
Barry Warsaw (barry) wrote :

Note that the package has changed significantly since 0.2. Specifically, it no longer contains extension modules for gnome-keyring and kde_kwallet. In that case, I think it makes the most sense to get rid of the python-keyring-gnome and python-keyring-kwallet binary packages, since they no longer have anything to install.

Since radical surgery is needed anyway, I think it also makes sense to upgrade the package to dh_python2 and fix other issues with the packaging. This will make backporting problematic, but Leonard says in IRC that YAGNI.

Barry Warsaw (barry)
Changed in python-keyring (Ubuntu):
status: Triaged → In Progress
Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package python-keyring - 0.5.1-0ubuntu1

---------------
python-keyring (0.5.1-0ubuntu1) natty; urgency=low

  * New upstream release. (LP: #686257)
  * debian/control
      - The upstream package no longer builds extension modules for
        keyring-gnome or keyring-kwallet, so those binary packages are deleted.
      - Specify Python 2.6 as the minimum requirement and use
        X-Python-Version instead of XS-Python-Version (dh_python2).
      - Update Vcs-* headers to match what is given on the Cheeseshop page.
      - Update Standards-Version and Depends.
      - Suggests is no longer relevant, so it's removed.
      - Trim Build-Depends to only what's needed now.
  * debian/rules
      - Update to dh_python2 build system and simplify.
  * debian/python-keyring{,-gnome,-kwallet}.install
      - Removed as there is now only one binary package.
  * debian/copyright
      - Include my name since the packaging has changed quite a bit.
 -- Barry Warsaw <email address hidden> Wed, 02 Feb 2011 15:17:50 -0500

Changed in python-keyring (Ubuntu):
status: In Progress → Fix Released
Revision history for this message
Martin Pitt (pitti) wrote :

Reopening as the MIR still needs review/approval.

Changed in python-keyring (Ubuntu):
status: Fix Released → New
Revision history for this message
Barry Warsaw (barry) wrote :

Leonard or Francis, can you please verify that the python-keyring MIR is still required for launchpadlib. Thanks!

Revision history for this message
Leonard Richardson (leonardr) wrote :

Yes, python-keyring is a hard dependency of launchpadlib 1.9.x.

Changed in python-keyring (Debian):
status: Unknown → New
Michael Terry (mterry)
Changed in python-keyring (Ubuntu):
assignee: Barry Warsaw (barry) → Kees Cook (kees)
Revision history for this message
Kees Cook (kees) wrote :

This looks fine. +1

Changed in python-keyring (Ubuntu):
assignee: Kees Cook (kees) → nobody
status: New → In Progress
Revision history for this message
Barry Warsaw (barry) wrote :

MIR approval team: what more do you need from us? I think this package meets all the requirements for main inclusion, and it's definitely required for launchpadlib 1.9.7, which is also ready for upload. We have sign-off from Kees.

summary: - upgrade to python-keyring 0.5.1 (and MIR)
+ [MIR] python-keyring 0.5.1
Barry Warsaw (barry)
Changed in python-keyring (Ubuntu):
status: In Progress → Fix Released
Revision history for this message
Martin Pitt (pitti) wrote :

Was approved, but not yet promoted, setting back to Fix committed. Promotion will happen as soon as the new launchpadlib will be uploaded, as previously there is nothing that "holds" the package in main.

Changed in python-keyring (Ubuntu):
status: Fix Released → Fix Committed
Revision history for this message
Martin Pitt (pitti) wrote :

Ah, saw on -release that new launchpadlib got uploaded. Promoted p-keyring to main.

Changed in python-keyring (Ubuntu):
status: Fix Committed → Fix Released
Changed in python-keyring (Debian):
status: New → Fix Committed
Changed in python-keyring (Debian):
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.