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

Add conformance tests #18

Closed
wants to merge 2 commits into from
Closed

Conversation

qmuntal
Copy link
Member

@qmuntal qmuntal commented Mar 28, 2022

This PR checks in several conformance samples from https://github.com/cose-wg/Examples and test that go-cose can sign and verify them.

Additionally, if the sample allows it, our CBOR output is compared against the sample output.

This new test suite deprecates all tests contained in sign_verify_cose_wg_examples_test.go, which were difficult to run and only did partial validations.

qmuntal added 2 commits March 28, 2022 10:31
Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>
Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>
@qmuntal qmuntal force-pushed the dev/qmuntal/examples branch from 4922315 to 8df89d9 Compare March 28, 2022 08:31
Copy link
Contributor

@thomas-fossati thomas-fossati left a comment

Choose a reason for hiding this comment

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

Just noting that there's an opportunity here to synchronise with the work that we are doing in GlueCOSE, specifically:

  1. Reshaping the JSON test vectors according to the gluecose schema
  2. Adapting the testing driver to the logics described in README.md

@qmuntal
Copy link
Member Author

qmuntal commented Mar 28, 2022

Just noting that there's an opportunity here to synchronise with the work that we are doing in GlueCOSE, specifically:

  1. Reshaping the JSON test vectors according to the gluecose schema
  2. Adapting the testing driver to the logics described in README.md

Agree. The gluecose/test-vectors design would make it easier for use to compare our CBOR output against the test cases, as it restrict the random number generators to zero readers.

My impression is that gluecose/test-vectors is still in early WIP, so I'm hesitant to use their test cases for now. I don't think it will be difficult to migrate once it settles down.

@thomas-fossati
Copy link
Contributor

Agree. The gluecose/test-vectors design would make it easier for use to compare our CBOR output against the test cases, as it restrict the random number generators to zero readers.

👍

My impression is that gluecose/test-vectors is still in early WIP, so I'm hesitant to use their test cases for now. I don't think it will be difficult to migrate once it settles down.

I am currently working on expanding the gluecose test suite, so if you want we could take the tests that you've selected, translate them, and drop them into gluecose/test-vectors from where go-cose can pick.

@qmuntal
Copy link
Member Author

qmuntal commented Mar 28, 2022

I am currently working on expanding the gluecose test suite, so if you want we could take the tests that you've selected, translate them, and drop them into gluecose/test-vectors from where go-cose can pick.

Sure! Can you translate them? There is no need to keep the input content, just that we are testing the same.
If so I will adapt my tests to gluecose/test-vectors format.

@thomas-fossati
Copy link
Contributor

I am currently working on expanding the gluecose test suite, so if you want we could take the tests that you've selected, translate them, and drop them into gluecose/test-vectors from where go-cose can pick.

Sure! Can you translate them? There is no need to keep the input content, just that we are testing the same. If so I will adapt my tests to gluecose/test-vectors format.

Cool, I'll take a stab at them and let you know as soon as I've something ready for review.

@thomas-fossati
Copy link
Contributor

I'll take a stab at them and let you know as soon as I've something ready for review.

A first Sign1 batch is here: gluecose/test-vectors#4

@SteveLasker
Copy link
Contributor

Thank you @qmuntal, this is awesome to see.
What is the current state? Is this ready for merging, with incremental changes as you and @thomas-fossati consolidate the gluecose tests?

@qmuntal
Copy link
Member Author

qmuntal commented Mar 29, 2022

This PR will be abandoned once #19 is merged

@thomas-fossati
Copy link
Contributor

Thank you @qmuntal, this is awesome to see.
What is the current state? Is this ready for merging, with incremental changes as you and @thomas-fossati consolidate the gluecose tests?

After we merge gluecose/test-vectors#4 (which is scoped to Sign1) I will extend the CDDL schema to support Sign and add another batch of related tests.
Let me know if we need to get more Sign1 coverage with algorithms other than ECDSA.

@qmuntal
Copy link
Member Author

qmuntal commented Mar 29, 2022

Thank you @qmuntal, this is awesome to see.
What is the current state? Is this ready for merging, with incremental changes as you and @thomas-fossati consolidate the gluecose tests?

After we merge gluecose/test-vectors#4 (which is scoped to Sign1) I will extend the CDDL schema to support Sign and add another batch of related tests. Let me know if we need to get more Sign1 coverage with algorithms other than ECDSA.

Great! I'm missing RSA-PSS with SHA256 and some verification failures

@thomas-fossati
Copy link
Contributor

I'm missing RSA-PSS with SHA256

👍

and some verification failures

Do you have anything specific failure case in mind?

@qmuntal
Copy link
Member Author

qmuntal commented Mar 29, 2022

I'm missing RSA-PSS with SHA256

👍

and some verification failures

Do you have anything specific failure case in mind?

Not really. Probably @shizhMSFT does.

@thomas-fossati
Copy link
Contributor

Do you have anything specific failure case in mind?

Not really. Probably @shizhMSFT does.

OK, thanks. I'll sync-up with Shiwei and ping you when I have something ready for you to consume.

@SteveLasker
Copy link
Contributor

Thanks folks. Should we flip this to a draft WIP PR?

@qmuntal
Copy link
Member Author

qmuntal commented Mar 30, 2022

Thanks folks. Should we flip this to a draft WIP PR?

This PR doesn't make sense anymore. Closing it!

@qmuntal qmuntal closed this Mar 30, 2022
@thomas-fossati
Copy link
Contributor

thomas-fossati commented Mar 30, 2022

Great! I'm missing RSA-PSS with SHA256

@qmuntal Could you please try out gluecose/test-vectors#5 ?

and some verification failures

I was busy coding other stuff today, I'll work on these tomorrow.

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