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

Additional test vectors for the RSA implicit rejection #541

Merged
merged 6 commits into from
Jan 16, 2025

Conversation

tomato42
Copy link
Member

@tomato42 tomato42 commented Jan 15, 2025

More vectors and align them with the draft-irtf-cfrg-rsa-guidance


This change is Reviewable

@tomato42 tomato42 requested a review from t184256 January 16, 2025 12:08
@t184256
Copy link
Collaborator

t184256 commented Jan 16, 2025

unit_tests/test_tlslite_utils_rsakey.py line 3204 at r6 (raw file):

        plaintext = a2b_hex(remove_whitespace("""
732f025d1adea74649b4

using multiline literals seems unnecessary for such short values

@t184256
Copy link
Collaborator

t184256 commented Jan 16, 2025

unit_tests/test_tlslite_utils_rsakey.py line 3087 at r6 (raw file):

        # 9 byte long value
        ciphertext = a2b_hex(remove_whitespace("""
00001ec97ac981dfd9dcc7a7389fdfa9d361141dac80c23a060410d472c16094

I don't see this one in https://github.com/tomato42/marvin-ietf/blob/d911d9a31654aa619a57bb0a970a2185e763b29b/draft-irtf-cfrg-rsa-guidance.xml, is that OK? same for several more; they're not new in this PR

@t184256
Copy link
Collaborator

t184256 commented Jan 16, 2025

unit_tests/test_tlslite_utils_rsakey.py line 1822 at r6 (raw file):

        dec = self.priv_key._raw_private_key_op_bytes(ciphertext)
        self.assertEqual(dec[0:2], b'\x00\x02')
        self.assertTrue(all(i != 0 for i in dec[2:-1]))

nit: all(i != 0 for i in N) looks overly verbose to me given it could be an all(N), but that's a matter of preference

Copy link
Collaborator

@t184256 t184256 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed all commit messages.
Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @tomato42)

Copy link
Member Author

@tomato42 tomato42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @t184256)


unit_tests/test_tlslite_utils_rsakey.py line 1822 at r6 (raw file):

Previously, t184256 (Alexander Sosedkin) wrote…

nit: all(i != 0 for i in N) looks overly verbose to me given it could be an all(N), but that's a matter of preference

while yes, I wanted it to be understandable to people with just cursory understanding of Python


unit_tests/test_tlslite_utils_rsakey.py line 3087 at r6 (raw file):

Previously, t184256 (Alexander Sosedkin) wrote…

I don't see this one in https://github.com/tomato42/marvin-ietf/blob/d911d9a31654aa619a57bb0a970a2185e763b29b/draft-irtf-cfrg-rsa-guidance.xml, is that OK? same for several more; they're not new in this PR

yes, it's ok, I don't think it's strictly necessary or brings much above a single null byte at the beginning; for the I-D I've selected a subset that should catch most of issues


unit_tests/test_tlslite_utils_rsakey.py line 3204 at r6 (raw file):

Previously, t184256 (Alexander Sosedkin) wrote…

using multiline literals seems unnecessary for such short values

while true, I wanted them to be easily diffable with the xml source of the I-D and then later, the RFC

Copy link
Collaborator

@t184256 t184256 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 2 files reviewed, all discussions resolved

@tomato42 tomato42 merged commit ef3b623 into master Jan 16, 2025
111 of 112 checks passed
@tomato42 tomato42 deleted the rsa-test-vectors branch January 16, 2025 13:20
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