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.

Sign up for GitHub

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

"take-2": new Windows backend, ctypes-based, separate class which shards via Attributes #555

Closed
wants to merge 1 commit into from
Closed

"take-2": new Windows backend, ctypes-based, separate class which shards via Attributes #555

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
110 changes: 74 additions & 36 deletions keyring/backends/Windows.py → keyring/backends/Windows/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,26 +1,13 @@
import logging

from ..util import properties
from ..backend import KeyringBackend
from ..credentials import SimpleCredential
from ..errors import PasswordDeleteError, ExceptionRaisedContext
from ...util import properties
from ...backend import KeyringBackend
from ...credentials import SimpleCredential
from ...errors import PasswordDeleteError, ExceptionRaisedContext


with ExceptionRaisedContext() as missing_deps:
try:
# prefer pywin32-ctypes
from win32ctypes.pywin32 import pywintypes
from win32ctypes.pywin32 import win32cred

# force demand import to raise ImportError
win32cred.__name__
except ImportError:
# fallback to pywin32
import pywintypes
import win32cred

# force demand import to raise ImportError
win32cred.__name__
from . import api as win32cred

log = logging.getLogger(__name__)

Expand Down Expand Up @@ -48,7 +35,12 @@ def value(self):
"""
Attempt to decode the credential blob as UTF-16 then UTF-8.
"""
# If the credential blob was already decoded, e.g.
# by CredReadFromAttributes, simply return it.
cred = self['CredentialBlob']
if isinstance(cred, str):
return cred

try:
return cred.decode('utf-16')
except UnicodeDecodeError:
Expand All @@ -65,7 +57,7 @@ class WinVaultKeyring(KeyringBackend):
WinVaultKeyring stores encrypted passwords using the Windows Credential
Manager.

Requires pywin32
Requires Windows

This backend does some gymnastics to simulate multi-user support,
which WinVault doesn't support natively. See
Expand All @@ -87,7 +79,7 @@ def priority(cls):
If available, the preferred backend on Windows.
"""
if missing_deps:
raise RuntimeError("Requires Windows and pywin32")
raise RuntimeError("Requires Windows")
return 5

@staticmethod
Expand All @@ -109,26 +101,32 @@ def _get_password(self, target):
res = win32cred.CredRead(
Type=win32cred.CRED_TYPE_GENERIC, TargetName=target
)
except pywintypes.error as e:
if e.winerror == 1168 and e.funcname == 'CredRead': # not found
except OSError as e:
if e.winerror == 1168: # not found
return None
raise

return DecodingCredential(res)

def set_password(self, service, username, password):
def set_password(self, service, username, password, encoding='utf-16-le'):
existing_pw = self._get_password(service)
if existing_pw:
# resave the existing password using a compound target
existing_username = existing_pw['UserName']
target = self._compound_name(existing_username, service)
self._set_password(
target,
existing_username,
existing_pw.value,
)
self._set_password(service, username, str(password))

def _set_password(self, target, username, password):
# resave the existing password using a compound target
# Fixes part of https://github.com/jaraco/keyring/issues/545,
# but get_credentials also needs to be fixed to search in the same
# order as get_password.
if existing_username != username:
target = self._compound_name(existing_username, service)
self._set_password(
target,
existing_username,
existing_pw.value,
encoding=encoding,
)
self._set_password(service, username, str(password), encoding=encoding)
Comment on lines 113 to +127
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused by this comment. It's saying that by re-saving the existing password using a compound target, it fixes part of 545, yet re-saving the existing password was already part of the prior change. Moreover, "get_credentials also needs to be fixed" feels like a misplaced TODO. Additionally, the comment doesn't say anything about what the code is meant to do, only has a reference to a reported issue.

By reading the issue, I now think the comment is only about the "if existing_username" check. The problem with putting the comment here is it becomes interleaved with the other comment, making it difficult to tell what comment goes with which parts of code. Since this change adds a whole new branch of code, it may be time to consider a refactor.

At the very least, I'd move the "resave" comment back to where it was (at the beginning of the if existing_pw branch and then move the rest of the comment into the if existing_username branch and rephrase to indicate its essential purpose, something like, "Only set the compound target when usernames match. Ref #545."

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.


def _set_password(self, target, username, password, encoding):
credential = dict(
Type=win32cred.CRED_TYPE_GENERIC,
TargetName=target,
Expand All @@ -137,7 +135,7 @@ def _set_password(self, target, username, password):
Comment="Stored using python-keyring",
Persist=self.persist,
)
win32cred.CredWrite(credential, 0)
win32cred.CredWrite(credential, 0, encoding=encoding)

def delete_password(self, service, username):
compound = self._compound_name(username, service)
Expand All @@ -153,11 +151,13 @@ def delete_password(self, service, username):
def _delete_password(self, target):
try:
win32cred.CredDelete(Type=win32cred.CRED_TYPE_GENERIC, TargetName=target)
except pywintypes.error as e:
if e.winerror == 1168 and e.funcname == 'CredDelete': # not found
except OSError as e:
if e.winerror == 1168: # not found
return
raise

# TODO https://github.com/jaraco/keyring/issues/545
# check non-compound_name first?
def get_credential(self, service, username):
res = None
# get the credentials associated with the provided username
Expand All @@ -169,3 +169,41 @@ def get_credential(self, service, username):
if not res:
return None
return SimpleCredential(res['UserName'], res.value)


class WinVaultAttributesKeyring(WinVaultKeyring):
def _get_password(self, target):
try:
res = win32cred.CredRead(
Type=win32cred.CRED_TYPE_GENERIC, TargetName=target
)
except OSError as e:
if e.winerror == 1168: # not found
return None
raise

# check if possibly a python-keyring sharded password
if res['CredentialBlobSize'] == 0:
res = win32cred.CredReadFromAttributes(
Type=win32cred.CRED_TYPE_GENERIC, TargetName=target, Credential=res
)

return DecodingCredential(res)

def _set_password(self, target, username, password, encoding):
credential = dict(
Type=win32cred.CRED_TYPE_GENERIC,
TargetName=target,
UserName=username,
CredentialBlob=password,
Comment="Stored using python-keyring",
Persist=self.persist,
)
try:
win32cred.CredWrite(credential, 0, encoding=encoding)
except OSError as e:
if e.winerror == 1783: # The stub received bad data.
# This means that the encoded password is too big to store
# in the CredentialBlob field. So try to store it sharded
# across up to 64 Attributes records (256 bytes each)
win32cred.CredWriteToAttributes(credential, 0)
Loading