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

Python: Unable to construct Statement objects #299

Open
woodruffw opened this issue Dec 6, 2023 · 6 comments
Open

Python: Unable to construct Statement objects #299

woodruffw opened this issue Dec 6, 2023 · 6 comments
Assignees

Comments

@woodruffw
Copy link
Contributor

This may be PEBKAC, but filing because I don't see the error 🙂.

I'm trying to use in_toto_attestation (the Python package generated from this repository) to build a Statement. This is what I have so far:

from in_toto_attestation.v1.statement import Statement

stmt = Statement(
    subjects=[
        {
            "name": "lol",
            "digest": {
                "sha256": "01ba4719c80b6fe911b091a7c05124b64eeece964e09c058ef8f9805daca546b"
            },
        }
    ],
    predicate={},
    predicate_type="https://in-toto.io/Statement/v1",
)

however, that fails with the following:

Traceback (most recent call last):
  File "/Users/william/devel/sigstore-python/./test_sign.py", line 14, in <module>
    stmt = Statement(
           ^^^^^^^^^^
  File "/Users/william/devel/sigstore-python/env/lib/python3.12/site-packages/in_toto_attestation/v1/statement.py", line 12, in __init__
    self.pb.subject.extend(subjects)
TypeError: Expected a message object, but got {'name': 'lol', 'digest': {'sha256': '01ba4719c80b6fe911b091a7c05124b64eeece964e09c058ef8f9805daca546b'}}.

it looks to me like the Statement binding is expecting some kind of protobuf object for the inner subjects list member, but it isn't clear to how to construct that object. None of the currently exposed types look right for it.

CC @adityasaky @joshuagl (apologies if you're the wrong people to ping)

@woodruffw
Copy link
Contributor Author

I dug a little further, and this constructed as expected:

stmt = Statement(
    subjects=[
        ResourceDescriptor(
            name="lol",
            digest={
                "sha256": "01ba4719c80b6fe911b091a7c05124b64eeece964e09c058ef8f9805daca546b"
            },
        ).pb,
    ],
    predicate={},
    predicate_type="https://in-toto.io/Statement/v1",
)

(Note the explicit .pb accessor at the end there -- without that, I get the same error).

So it looks like I can construct Statement objects, but doing so is a little cumbersome (and isn't correctly modeled in the current type hints), so I'll leave this open for that.

@adityasaky
Copy link
Member

cc @marcelamelara

@marcelamelara marcelamelara self-assigned this Dec 12, 2023
@marcelamelara
Copy link
Contributor

Apologies for my delayed response. I tend to agree that this level of indirection to properly set the nested protobufs within a Statement is cumbersome, though this was the most straight-forward way we saw to implement validator functions for the structure (as proto definitions don't currently support custom validators).

Seeing how you'd like to use the API and pass in dicts is helpful, and I do think we could convert a list of dicts representing subjects into the appropriate ResourceDescriptor protos within the Statement constructor, rather than expect the list of subjects to contain protos.

@woodruffw is this the sort of change that would work for you?

@woodruffw
Copy link
Contributor Author

@woodruffw is this the sort of change that would work for you?

I think so! It might make sense to align this with #291 -- that will make the underlying protobuf models much more ergonomic and easier to type-check, which will (IMO) eliminate the need to pass in a dict here.

I can open up a draft PR for that in a moment.

@woodruffw
Copy link
Contributor Author

I've opened #315 for the above -- my thinking there is that Statement and ResourceDescriptor could be re-written in terms of the new models there, with the improved type-hinting making it easier to avoid user confusion 🙂

@marcelamelara
Copy link
Contributor

Fair enough! And thank you for the draft PR! I'm all for removing the clunkiness of protobuf in Python where we can.

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

No branches or pull requests

3 participants