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.

Sign up for GitHub

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

Jump to bottom

Add Python validation APIs and unit tests #227

Merged
merged 5 commits into from
Aug 1, 2023
Merged

Add Python validation APIs and unit tests #227

merged 5 commits into from
Aug 1, 2023

Conversation

marcelamelara
Copy link
Contributor

This PR introduces basic wrapper APIs for the Python language bindings for the Statement layer. I also adds basic tests and documentation for testing the Python language bindings.

Fixes #176

@marcelamelara marcelamelara requested a review from a team as a code owner May 24, 2023 21:38
@marcelamelara marcelamelara marked this pull request as draft May 24, 2023 22:23
@marcelamelara marcelamelara marked this pull request as ready for review May 25, 2023 21:21
@adityasaky adityasaky mentioned this pull request May 30, 2023
Merged
@marcelamelara
Copy link
Contributor Author

marcelamelara commented Jun 1, 2023 •
edited
Loading

@adityasaky Given the discussion in in-toto/in-toto#570 about Signable, I'm thinking that's functionality that the Statement wrapper class in this PR can possibly expose. I have two ideas for addressing this: 1) The statement class provides a signable_bytes API as a replacement for the existing Signable API, or 2) /in-toto uses the serialized protobuf message for signing, though this may break verifiers. Thoughts?

TomHennen
TomHennen requested changes Jun 2, 2023
View reviewed changes
python/in_toto_attestation/v1/resource_descriptor.py Outdated Show resolved Hide resolved
python/in_toto_attestation/v1/statement.py Show resolved Hide resolved
python/in_toto_attestation/v1/statement.py Outdated Show resolved Hide resolved
python/in_toto_attestation/v1/statement.py Outdated Show resolved Hide resolved
python/in_toto_attestation/v1/statement.py Outdated Show resolved Hide resolved
tests/python/test_resource_descriptor.py Outdated Show resolved Hide resolved
tests/python/test_resource_descriptor.py Outdated Show resolved Hide resolved
tests/python/test_statement.py Outdated Show resolved Hide resolved
@adityasaky
Copy link
Member

I have two ideas for addressing this: 1) The statement class provides a signable_bytes API as a replacement for the existing Signable API, or 2) /in-toto uses the serialized protobuf message for signing, though this may break verifiers. Thoughts?

I think option 1 probably makes sense for compatibility. FWIW, we should test across implementations as soon as we implement statements. cc @PradyumnaKrishna who's beginning to look at cross implementation compatibility for in-toto

@adityasaky
Copy link
Member

I suggest we proceed with this PR without supporting anything specific re signing. If in-toto/in-toto implements an intermediate class for the Statement that builds on what's provided here, it can use the existing Signable class for consistency.

TomHennen
TomHennen approved these changes Jun 12, 2023
View reviewed changes
Copy link
Contributor

@TomHennen TomHennen left a comment

Choose a reason for hiding this comment

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

Thanks Marcela!

@marcelamelara marcelamelara requested review from adityasaky and a team June 13, 2023 23:42
@adityasaky
Copy link
Member

LGTM!

joshuagl
joshuagl previously requested changes Jun 27, 2023
View reviewed changes
Copy link
Contributor

@joshuagl joshuagl left a comment

Choose a reason for hiding this comment

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

Could we add type annotations to the public methods (especially constructors)? 🙏

docs/testing.md Outdated Show resolved Hide resolved
python/in_toto_attestation/v1/resource_descriptor.py Outdated Show resolved Hide resolved
python/in_toto_attestation/v1/statement.py Outdated Show resolved Hide resolved
python/in_toto_attestation/v1/statement.py Show resolved Hide resolved
@marcelamelara marcelamelara requested a review from joshuagl July 1, 2023 00:03
@marcelamelara marcelamelara mentioned this pull request Jul 5, 2023
Closed
@marcelamelara marcelamelara requested a review from a team July 11, 2023 15:15
adityasaky
adityasaky reviewed Jul 14, 2023
View reviewed changes
Copy link
Member

@adityasaky adityasaky left a comment

Choose a reason for hiding this comment

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

Couple of thoughts I'd missed before, apologies.

tests/python/test_resource_descriptor.py Outdated
@@ -0,0 +1,59 @@
'''
Copy link
Member

Choose a reason for hiding this comment

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

Can this go in a tests/ directory in /python? So all the Python code is contained in the python dir, similar to how the Go tests are in the go dir with the corresponding packages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, this sounds good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not related to this PR, but should we also move the examples/go dir into the go/ directory at some point? I've been thinking (in general) about where to house any existing and future examples for in-toto attestation use cases.

python/in_toto_attestation/v1/resource_descriptor.py Show resolved Hide resolved
@marcelamelara
Copy link
Contributor Author

marcelamelara commented Jul 31, 2023 •
edited
Loading

@joshuagl Can you please re-review to see if I've addressed your previous comments? There are a few (unopened) PRs that depend on this PR being merged.

@marcelamelara
Add Python wrapper APIs for statement layer
600fb0b 
Signed-off-by: Marcela Melara <marcela.melara@intel.com>
@marcelamelara
Add Python unit tests
66fefe7 
Signed-off-by: Marcela Melara <marcela.melara@intel.com>
@marcelamelara
Document Python testing
f05f387 
Signed-off-by: Marcela Melara <marcela.melara@intel.com>
@marcelamelara
Address review comments
bc49835 
* Make parameters explicit in constructors; use in tests
* Add type annotations in functions
* Update constructors based on in-toto#257 and in-toto#263

Signed-off-by: Marcela Melara <marcela.melara@intel.com>
@marcelamelara
Move python tests into /python
95592b1 
Signed-off-by: Marcela Melara <marcela.melara@intel.com>
@marcelamelara marcelamelara mentioned this pull request Aug 1, 2023
Merged
pxp928
pxp928 approved these changes Aug 1, 2023
View reviewed changes
Copy link
Member

@pxp928 pxp928 left a comment

Choose a reason for hiding this comment

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

LGTM!

@marcelamelara marcelamelara dismissed joshuagl’s stale review August 1, 2023 21:13

Requested changes have been addressed.

@marcelamelara marcelamelara merged commit 5944353 into in-toto:main Aug 1, 2023
@marcelamelara marcelamelara deleted the add-python-apis branch August 30, 2024 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adityasaky adityasaky adityasaky left review comments

@pxp928 pxp928 pxp928 approved these changes

@TomHennen TomHennen TomHennen approved these changes

@joshuagl joshuagl Awaiting requested review from joshuagl

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create test suite for Python bindings
5 participants
@marcelamelara @adityasaky @TomHennen @joshuagl @pxp928