Skip to content

Identity.verify_digest should only return True on successful verification, otherwise throw according Exceptions #459

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

Closed
2 tasks done
NindoK opened this issue Jul 11, 2024 · 4 comments · Fixed by #468
Closed
2 tasks done
Labels
c-bug Category: Something isn't working s-confirmed Status: This is a confirmed issue

Comments

@NindoK
Copy link
Contributor

NindoK commented Jul 11, 2024

Prerequisites

  • I checked the documentation and found no answer to my problem
  • I checked the existing issues and made sure there are no similar bug reports

Category

Bug (unexpected behavior)

Expected Behavior

If we have an Envelope with a wrong signature e.g. sigWrong and we call verify, it should return False.

envelope = Envelope(signature="sigWrong")

envelope.verify() #Should be False

Observed Behavior

If we have an Envelope with a wrong signature e.g. sigWrong and we call verify, it throws an error

envelope = Envelope(signature="sigWrong")

envelope.verify() #Needs to be catched

Screenshot 2024-07-11 at 12 28 14

To Reproduce

No response

Version

v0.14.0

Environment Details (Optional)

No response

Failure Logs (Optional)

No response

Additional Information (Optional)

No response

@NindoK
Copy link
Contributor Author

NindoK commented Jul 11, 2024

cc @qati

@Dacksus
Copy link
Contributor

Dacksus commented Jul 17, 2024

You spotted indeed inconsistent behavior. The example you're describing raises an error because no sender address is specified. If you create a complete and valid Envelope, but provide a wrong signature, Envelope.verify() would not throw an exception and actually return False.

I would argue that the latter is the actually wrong behavior and following security common practices verify should always throw an exception if not successful. The reasoning behind this is that there is no scenario which could come to a successful outcome, if the signature was not valid and in every case the developer would need to gracefully wind off the current process and return corresponding information to the user and/or communication partners. Which is the exact scope of exception handling.

Thoughts or objections @Archento @jrriehl @ejfitzgerald?

@ejfitzgerald
Copy link
Member

Agreed @Dacksus my feeling is that this function should always throw.

If python had a way to signal the return value should always be read then it might be different.

I would refactor this so all error cases cause exceptions to be thrown

@NindoK
Copy link
Contributor Author

NindoK commented Jul 17, 2024

Sorry my examples were a bit reductive and I have only highlighted the signature part, considering that all the other fields were put as such to generate a valid Envelope.

Actually, with a valid Envelope and the signature that I have provided the code should just return false since the signature by itself is compromised hence not valid.

@Dacksus Dacksus changed the title verify_digest shouldn't throw an error, but just return False if the signature is wrong. Identity.verify_digest should only return True on successful verification, otherwise throw according Exceptions Jul 18, 2024
@Dacksus Dacksus added c-bug Category: Something isn't working s-confirmed Status: This is a confirmed issue labels Jul 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-bug Category: Something isn't working s-confirmed Status: This is a confirmed issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants