in-toto / attestation

in-toto Attestation Framework
Other
234 stars 60 forks source link

Add 'attester.id' field (or similar) to statement layer. #137

Closed TomHennen closed 1 year ago

TomHennen commented 1 year ago

130 discussed the option of adding this field before v1, but the conversation is getting interesting so it's better to have it's own issue.

In this comment @MarkLodato says

For "Decide adding an 'attestor id' field to the statement layer (per above)." I suggest we drop it. The current model generally works fine and prevents ambiguity.

This came up again in slsa-framework/slsa#622, since I'm proposing dropping builder.id from the SLSA Provenance predicate. If we moved it to the Statement layer, this field would be more narrowly focused to hopefully avoid confusion. Maybe something like onBehalfOf to make it clear that it's only needed to differentiate who the attester is?

The alternative is to move it to DSSE, but that might be more work and with a different timeline.

My main worry with it being in the Statement layer is that, since it will almost always be empty except for some very particular use cases, implementations will forget it exists and just ignore it (or otherwise handle it incorrectly). Whereas if it's in the envelope layer, implementations are more forced to deal with it.

We seem to be converging on some need for this data, if SLSA drops builder.id then we probably definitely want someplace this data can be encoded.

TomHennen commented 1 year ago

@MarkLodato if we were to add attestor.id or onBehalfOf my inclination is to make it a required field. That way it won't always be empty and people can rely on it for some purposes.

MarkLodato commented 1 year ago

@TomHennen I'm inclined to make it optional, steer people away from it, and require implementations to either disallow it or handle it properly. Maybe something like:

onBehalfOf (string, optional)

If non-empty, indicates that the signer generated the attestation on behalf of the given party. Consumers SHOULD consider the attester to be the pair (signer, onBehalfOf). To reduce the chance of mistakes, producers SHOULD NOT use the same signer identity for attestations with both empty and non-empty onBehalfOf, and consumers SHOULD NOT accept signer identities that allow both empty and non-empty onBehalfOf. If implementations do not fully support onBehalfOf, they SHOULD reject any non-empty value. The format of the string is dependent on the signer identity.

My concern with making it required is the same as for builder.id (https://github.com/slsa-framework/slsa/issues/622), namely that it's error prone and confusing to always set it when it's almost always in agreement with the signer identity.

(By the way, the spec uses "attester" rather than "attestor".)

TomHennen commented 1 year ago

and require implementations to either disallow it or handle it properly

Isn't the issue that the producer of the attestation and the consumer are likely different implementations? Some implementations would just wind up failing.

I do agree that there's subtly & complexity here, but it feels like it's just getting pushed around? The suggested text doesn't exactly lend itself to an obvious implementation.

(changed title to reflect the correct spelling :smile:)

FYI @AdamZWu who has indicated an interest in this topic.

AdamZWu commented 1 year ago

Just a nit:

It seems attestor was the right word: "a person who attests to the genuineness of a document or signature by adding their own signature";

vs. attester means "One who testifies, especially in court".

https://www.thefreedictionary.com/attestor#:~:text=attester

AdamZWu commented 1 year ago

My thoughts are that:

  1. attestor.id is an essential statement field if we are to embrace separation of concerns between DSSE and in-toto layers;
  2. attestor.id has a more general purpose, and "on-behalf-of" is a subset of that purpose.

DSSE, as its name indicates, intends to be slim and very simple. To me its elegance comes from the simple goal of ensuring the integrity of the payload. If we add more concerns such as "who produced the payload", "who signed the envelope on whose behalf", "was that a legit delegation" etc., I am afraid DSSE will lose simplicity and elegance.

I think the attestor.id is like the "letter head", which is what we see first when we open up a (physical) envelope. It serves the purpose of announcing the origin of the letter content. The origin can be the same as the signer of the envelope, or it can be different (delegated), and the "letter head" is capable of expressing both cases.

MarkLodato commented 1 year ago

I'm OK with attestor. I think the two spellings are synonymous, but BinAuthz uses "attestor" so we might as well use that for consistency.

I'm also OK putting it in Statement instead of DSSE.

But I'm still not convinced that it makes sense as a required field. The letterhead is a good analogy. In your analogy, where the envelope is what provides the authenticity, you should not trust the letterhead! It's just a red herring! Isn't the only legitimate use case where the envelope identity and statement identity don't match is the on-behalf-of use case?

TomHennen commented 1 year ago

Is there a downside to requiring it? Do we have to place any requirements on how it should be used to validate signatures or the contents of things at this layer?

AdamZWu commented 1 year ago

One benefit I can think of is debugging friendliness. Having the attestor's "official identity" spelled out in clear text makes it really easy for adhoc manual analysis. OTOH, if the only identifying string is the key id, which may look like key_ring://1234abcd, a human debugger will have to run through hoops to figure out where this attestation comes from.

MarkLodato commented 1 year ago

Is there a downside to requiring it?

Yes:

That said, I do see some upsides:

What about creating a new _type?

{
  "_type": "https://in-toto.io/Attestation/v1.0",
  "attestor": "<URI>",
  "statement": {
    "subject": [
      {
        "name": "<NAME>",
        "digest": {"<ALGORITHM>": "<HEX_VALUE>"}
      },
      ...
    ],
    "predicateType": "<URI>",
    "predicate": { ... }
  }
}

This solves my layering concern, and it also gives users and implementations the clear option to support this or not.

MarkLodato commented 1 year ago

Or if you don't like the extra layer, renaming the type while putting attester, subject, and predicate* on the same level also makes sense.

MarkLodato commented 1 year ago

Actually I'm now realizing how you could solve this niche use case using a predicateType:

{
  "_type": "https://in-toto.io/Statement/v1.0",
  "subject": [ ... ],
  "predicateType": "https://in-toto.io/WasAttestedToBy/v1.0",
  "predicate": {
    "attestor": "<URI>",
    "predicateType": "(original predicateType)",
    "predicate": { (original predicate) }
  }
}

Maybe that's the best solution. Particular parties can use it when needed, but we don't clutter up the main spec.

AdamZWu commented 1 year ago

Cool, I like the nested predicate! An envelope within a sealed envelope. 😄


Oh, wait, the subject is left outside of the nested envelope...

If we follow the <subject> is <predicate> model, both seem better be encapsulated in the inner context of "on-behalf-of".

adityasaky commented 1 year ago

I might be missing something but does moving this field to the statement layer solve the SLSA Provenance problem with the field? I'm a little wary of adding this to the statement because I think we might have different interpretations for how this field is consumed during verification depending on the predicateType that we can't sufficiently capture in a statement level description of the field. I think I'm leaning towards leaving it in the predicate.

Also, +1 on not adding it to DSSE.

If we follow the is model, both seem better be encapsulated in the inner context of "on-behalf-of".

I think the top level statement would be invalid then. I'm ambivalent about the solution in Mark's comment at the moment, though I do think it'd be less confusing to have it in the predicate still.

MarkLodato commented 1 year ago

OK, given the pushback and the impending deadline, I'm fine not adding this for v1.0. Let's just leave builder.id for now. There's not enough bandwidth to work out alternatives and semantics.

TomHennen commented 1 year ago

That makes sense. This definitely isn't as clear as we expected.