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

Fix #143 Allow the combination of X509Data and KeyValue when they represent the same public key #169

Merged
merged 1 commit into from
Mar 14, 2021

Conversation

lscorcia
Copy link
Contributor

Hi, this is a shot at implementing the suggested logic fix for issue #143. Basically the code now extracts the key parameters for both key representations and compares them to decide whether they are the same key or not.

If the match is positive, then the code goes on as if there were no ambiguity. If the match is negative, the ignore_ambiguous_key_info parameter lets the user decide whether to validate using the X509 data only.

Please bear in mind that this is my first python code ever, so I apologize in advance for any stupid mistake I may have made. I have tested it only using RSA signatures. Also some minor code duplication is introduced and it probably could use some refactoring, but I have to say I don't know python well enough to improve this code. Please review it carefully.

Thanks in advance for your work and your time!

@kislyuk
Copy link
Member

kislyuk commented Mar 14, 2021

Thank you for your work here @lscorcia. This looks like a very good start, and I appreciate your contribution. Before I can release this, it will require a test case and a refactoring to reduce duplication as you mentioned. I am merging this now since I would rather have this in the main branch and work on it there.

Please let me know if you can contribute a test case or test data with a positive (valid signature) and negative (mismatch) cases.

@kislyuk kislyuk merged commit d16ba95 into XML-Security:develop Mar 14, 2021
@lscorcia lscorcia deleted the issue-143 branch March 14, 2021 23:20
kislyuk added a commit that referenced this pull request May 14, 2021
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.

2 participants