-
Notifications
You must be signed in to change notification settings - Fork 29
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
Provide catch-able exceptions for 2 dpapi errors #108
Conversation
cf84d33
to
c98bcba
Compare
msal_extensions/persistence.py
Outdated
except OSError as exception: | ||
raise PersistenceDecryptionError( | ||
err_no=getattr(exception, "winerror", None), # Exists in Python 3 on Windows | ||
message="Decryption failed: {}. You may have to delete the file.".format(exception), # pylint: disable=consider-using-f-string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in .NET we delete it ourselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for bringing up this intriguing topic.
The current "you may have to (manually) delete the file" advice was indeed influenced by the actual workaround that we gave to those few Azure CLI's end users who ran into this issue. So, when/if those end users took our advice, they implicitly consented and they knew what they were doing.
But it feels uncomfortable for our implementation to do this silently. The deletion is an irrevocable and potentially destructive operation.
Azure CLI team and us have not quite figured out why end users would run into this issue in the first place. Although the following speculation may or may not be the case for Azure CLI, but, if a downstream app would happen to use same filename for plaintext data and encrypted data, it will then be theoretically possible that a plaintext file predates the decryption attempt. In such case, a smooth recovery plan would be for the app developer to gracefully convert the plaintext data into encrypted data:
encrypted_persistence = FilePersistenceWithDataProtection("my_data.bin")
try:
valuable_data = encrypted_persistence.load()
except PersistenceDecryptionError:
valuable_data = FilePersistence("my_data.bin").load()
encrypted_persistence.save(valuable_data)
Or, if the app developer would still prefer, they can catch the PersistenceDecryptionError
and do the file removal there. The decision is in their hands.
I incline to keep this PR's current behavior i.e. NOT automatically delete the file. We may, however, somehow reword the advice to "App developer or end user may choose to delete the file, or app developer could recover by blah blah blah". The "blah blah blah" part will be lengthy and the audience is the app developer. It is probably fine, as we anticipate the app developer to (eventually) catch this exception and translate it into a more user-friendly message to their end users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UPDATE: Now the exception message contains a link to this newly-created wiki page https://github.com/AzureAD/microsoft-authentication-extensions-for-python/wiki/PersistenceDecryptionError
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Azure CLI guarantees the encrypted file and unencrypted file do not have the same name.
file_extensions = {True: '.bin', False: '.json'}
...
location += file_extensions[encrypt]
-2146892987: "The requested operation cannot be completed. " | ||
"The computer must be trusted for delegation and " | ||
"the current user account must be configured to allow delegation. " | ||
"See also https://docs.microsoft.com/en-us/windows/security/threat-protection/security-policy-settings/enable-computer-and-user-accounts-to-be-trusted-for-delegation", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think common users can understand this enigma. They are still non-actionable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think common users can understand this enigma. They are still non-actionable.
That is arguably true. (TBH, that link was a TL;DR for me. LOL.) But my thoughts were:
- Bogdan found this valuable information that might be useful to those who are determined enough to follow that route, so I would keep it here rather than waste it.
- These paragraphs are for the "inner exception"
OSError
. The "outer exception"PersistenceEncryptionError
andPersistenceDecryptionError
both end with a slightly-more-actionable sentence. - Eventually, Azure CLI may still want to catch the
PersistenceEncryptionError
andPersistenceDecryptionError
, and convert them into an end-user-friendly sentence.
@@ -39,6 +39,15 @@ def raw(self): | |||
_MEMCPY(blob_buffer, pb_data, cb_data) | |||
return blob_buffer.raw | |||
|
|||
_err_description = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have some knowledge on WinError 0 (Azure/azure-cli#20278)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know. The "error 0" is particularly strange. The zero is supposed to mean success.
Regardless, like I admitted here, these error description are still more like troubleshooting trace. I should probably change this PR's title to "Provide catch-able exceptions for all dpapi errors". Eventually Azure CLI might still need to catch them and then tell end user "just turn off encryption by configuring ~/.azure/..." (such a very actionable sentence can not be provided by this MSAL EX package, anyway.)
a977289
to
a9a7f6a
Compare
b184f54
to
1e39f06
Compare
Add underlying OSError info into Persistence exceptions Refer to the PersistenceDecryptionError wiki page Update msal_extensions/windows.py Co-authored-by: Jiashuo Li <[email protected]> Ignore consider-using-f-string, once and for all
1e39f06
to
effe778
Compare
This PR provides user-friendly exception messages for real-world observation: Azure/azure-cli#20231, Azure/azure-cli#20759, and Azure/azure-cli#21010 . The default error messages is not (and can not) be specific to Azure CLI, but Azure CLI can choose to catch the 2 new exceptions
PersistenceEncryptionError
andPersistenceDecryptionError
, and then emit their own error messages.CC: @jiasli