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

question re CREDENTIAL __DO_NOT_USE_ fields in _authentication.py #105

Open
jrobbins-LiveData opened this issue Nov 28, 2021 · 5 comments · May be fixed by #108
Open

question re CREDENTIAL __DO_NOT_USE_ fields in _authentication.py #105

jrobbins-LiveData opened this issue Nov 28, 2021 · 5 comments · May be fixed by #108
Labels

Comments

@jrobbins-LiveData
Copy link

I was wondering why _fields_ AttributeCount and Attribute are prefixed with __DO_NOT_USE_?

I have a potential use for setting an attribute on a Credentials Manager entry (to record the encoding used for a password), and was wondering if this __DO_NOT_USE_ had an back story about what kinds of issues I might be facing in using this feature?

class CREDENTIAL(Structure):

("_DO_NOT_USE_AttributeCount", DWORD),

("__DO_NOT_USE_Attribute", c_void_p),

@jdmarch
Copy link

jdmarch commented Nov 28, 2021

Hi @jrobbins-LiveData -- just a heads-up that it may be a week or two before @itziakos will be available to respond to your question.

@jrobbins-LiveData
Copy link
Author

jrobbins-LiveData commented Nov 28, 2021 via email

@itziakos
Copy link
Member

itziakos commented Dec 6, 2021

@jrobbins-LiveData It looks that the DO_NOT_USE prefix is a very early commit that was added when the ctypes implementation was first introduced. I suspect that it was probably because these keys where not needed at that time. Pywin32 does not have this limitation so we should fix that.

I am designating this report as a bug report. I will need to look into what is necessary to better support the application specific attributes. As far as I can tell it is not enough to just rename the attributes since Pywin32 has a different behaviour (you can probably use Pywin32 for development until we get this fixed).

@jrobbins-LiveData
Copy link
Author

jrobbins-LiveData commented Dec 6, 2021

Hi @itziakos. In working on a keyring Windows issue, we had some discussions about augmenting the interface to the Cred<xyz> functions. Here are some of the factors involved:

  • A ctypes interface, which your project supports, is desirable, to avoid taking any binary dependencies.
  • Access to the Attributes and AttributesCount fields would be helpful.
  • And having a choice about the encoding used for CredentialBlob might be useful.

I wrote a new implementation using ctypes. While it does exactly what I need to address the keyring issue, I can't speak for its general usefulness.

That said, if it, or something like it, would best belong in this project, I'd be happy to work on that with you. I kept the interface upwards compatible (I think?), so hopefully it wouldn't break anyone already using it. That would need to be verifed, of course!

Anyway, please let me know what you think.

@itziakos
Copy link
Member

itziakos commented Dec 7, 2021

Thanks @jrobbins-LiveData

I had a quick look at the implementation in keyring. I will need to think how I can accomodate your usecase while keeping compatibility with the behaviour of pywin32. At the very least it should be easier to implement keyring's specific credential configuration on top of pywin32-ctypes (althought using a private module like _authenticate.py is still a small code smell).

On another note I noticed that in https://github.com/jaraco/keyring/blob/1f8bfb394d67e1f9a632916c444b60cbcebbb896/keyring/backends/Windows/api.py#L105 the code mutates the global windll.advapi32 object. I suggest that a local WinDDL instance is used to avoid mutating global state (which unfortunately a lot of libraries do).

@itziakos itziakos linked a pull request Apr 6, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants