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

Forward soft argument to validate_signature #664

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

neanias
Copy link

@neanias neanias commented Jun 12, 2023

When calling the validate_document method chain, the soft parameter is passed down from one method to another except in the hand off from validate_document_with_cert to validate_signature.

When calling the `validate_document` method chain, the `soft` parameter
is passed down from one method to another except in the hand off from
`validate_document_with_cert` to `validate_signature`.
@johnnyshields
Copy link
Collaborator

@neanias two questions:

  1. Just checking Chesterton's Fence here, why do you think it might have been true in the first place?
  2. What is your used case to have this pass in the soft arg?

@neanias
Copy link
Author

neanias commented Jul 8, 2024

For 1. I would think it was true in the first place because validation may have always wanted to be soft at this stage to avoid raising an error? It's a bit unusual to me in reading the git log that when the soft parameter was introduced for this method it was to allow surpressing exceptions, but there's no way to stop surpressing exceptions. I'm not clear if the tests are covering the unhappy path as a result since it doesn't check if passing soft = false raises an error.

For 2. it's been over a year since I opened this so I can't exactly remember why I opened the PR in the first place. I think I was trying to understand why my code wasn't throwing any errors when I passed in soft = false as per the docs.

@neanias
Copy link
Author

neanias commented Jul 8, 2024

The soft = true param was added in this commit: d0e117a

@johnnyshields
Copy link
Collaborator

OK. @pitbulk I think it's reasonable to merge this.

@pitbulk
Copy link
Collaborator

pitbulk commented Jul 9, 2024

I remember I checked it time ago when was registered, but for some reason I did not merge it.

As "validate signature" is a critical piece, I will need more time to review this PR and check why it was implemented in that way.

@johnnyshields
Copy link
Collaborator

johnnyshields commented Jul 9, 2024

This change seems important for security.

The previous logic made signature validation always a soft validation. However, if one's system is relying on errors to be raised, then currently failed signature validation won't raise an error, and one will permit a failed signature validation when one should actually reject it! 😱

I can't imaginee a case where a user wants hard errors, but only wants signature validation to be soft.

Conversely, users using soft validations won't be affected at all.

@neanias
Copy link
Author

neanias commented Jul 9, 2024

Also realised that I didn't add tests to check that the right hard error is raised when soft is disabled. I'll add one now

@pitbulk
Copy link
Collaborator

pitbulk commented Jul 9, 2024

If you check the use of validate_document_with_cert in the code, it is already used with soft parameter with true value, and the error is added as in the rest of validations.

@neanias
Copy link
Author

neanias commented Jul 9, 2024

I see. So does this method need to accept the soft flag at all then?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants