in-toto / attestation

in-toto Attestation Framework
Other
239 stars 66 forks source link

Python: Unable to construct `Statement` objects #299

Open woodruffw opened 11 months ago

woodruffw commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

cc @marcelamelara

marcelamelara commented 10 months ago

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 commented 10 months ago

@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 commented 10 months ago

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 commented 10 months ago

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