secure-systems-lab / dsse

A specification for signing methods and formats used by Secure Systems Lab projects.
https://dsse.dev
Apache License 2.0
66 stars 18 forks source link

Add support for signature extensions #61

Closed adityasaky closed 4 months ago

adityasaky commented 1 year ago

First draft of signature extensions including a stab at defining the sigstore extension in the repo. I think we'd perhaps be better off not listing extensions here.

59

znewman01 commented 1 year ago

CC Sigstore clients who might be consumers of this: @woodruffw @haydentherapper @codysoyland @bdehamer (and me, I'll take a look at some point soon)

TomHennen commented 1 year ago

I think this generally looks good to me, but I don't trust my crypto expertise to say one way or the other. @haydentherapper how do you think this is looking from a Sigstore perspective?

haydentherapper commented 1 year ago

I’m also generally good with this. I do expect some changes once this is actually used though, so I’d prefer we mark it as alpha or wip so it’s easy to make breaking changes.

TomHennen commented 1 year ago

Is this PR just waiting for me at this point? :)

adityasaky commented 1 year ago

@TomHennen I'm going to remove the sigstore.md file, I retained it while we checked if just a table was enough. Once I do that and squash my commits, I think we should be good to go with a review from you?

adityasaky commented 1 year ago

@TomHennen done!

TomHennen commented 1 year ago

Are we still waiting for @haydentherapper and @MarkLodato's last thoughts or should we go ahead and merge?

haydentherapper commented 1 year ago

I think we're just waiting on https://github.com/sigstore/protobuf-specs/pull/145?

adityasaky commented 1 year ago

Yes, sorry, I'm going to be addressing that thread this afternoon / evening and then updating this PR.

woodruffw commented 1 year ago

Sorry for the belated comment here, but to make sure I understand: the intended outcome here is to allow a DSSE envelope to contain a Sigstore bundle (or whatever else), correct?

In other words: ext will contain the bundle's fields, modulo the signature that would otherwise be in the bundle?

adityasaky commented 1 year ago

Essentially yes, the idea is to associate each signature in a DSSE envelope with its sigstore specific fields (tlog entries etc.). The idea is to use either the VerificationMaterials or a subset of that sigstore message rather than the bundle.

Also note that this PR introduces extensions generally and the sigstore extension.

woodruffw commented 1 year ago

Okay, thanks!

This is probably the wrong place to figure this out, but I'm still a little murky on the benefits of having the DSSE as the top-level envelope rather than the Sigstore bundle (or vice versa, except for compatibility reasons in extant Sigstore clients). But that's not a reason for it not to exist, just a lack of clarity on my part 🙂

adityasaky commented 1 year ago

https://github.com/sigstore/sig-clients/issues/9 might shed some light on that. The tl;dr for the sigstore bundle vs dsse extension:

adityasaky commented 9 months ago

https://github.com/sigstore/protobuf-specs/pull/145 has been merged, unblocking this. @mnm678 @TomHennen @trishankatdatadog @MarkLodato @JustinCappos

SantiagoTorres commented 7 months ago

Hi, I think I mentioned this w/ folks on other channels, but I was wondering whether authenticating the extension type (i.e., not the payload) w/ the signature is at all possible (I believe it is). This way, we are able to avoid tampering of the extension bits themselves (i.e., by forcing a commitment to include an extension such as the sigstore bundle), even if we don't know what the content of the extension will be (e.g., because we need a reply from a transparency log).

I don't think the delta for this is exactly large (or admittedly too small for the current impl), but I think the security gain is worthwhile. What're everybody's thoughts on this?

trishankatdatadog commented 7 months ago

I don't think the delta for this is exactly large (or admittedly too small for the current impl), but I think the security gain is worthwhile. What're everybody's thoughts on this?

Agreed. The question is: where? I believe the functionary (in-toto) / role (TUF) that bestows trust on the public key (and hence, the signature) in the first place is the best place to securely bind the extension type (along with other information we already distribute, such as key type, scheme, and so on) to the signature. WDYT?

adityasaky commented 7 months ago

I really like the idea of signing over the extension kind!

I think putting it in the policy (in-toto layout, TUF metadata, etc.) is one half of it. I parsed @SantiagoTorres's comment as a recommendation to sign over the extension kind as well, so a signature that uses an extension cannot be verified without explicitly acknowledging the existence of the extension.

This would require a modification to the signing protocol, probably in the PAE?

trishankatdatadog commented 7 months ago

@adityasaky Yes, you need both for maximum coverage.

SantiagoTorres commented 7 months ago

yeah, essentially, adding it to the PAE would do the trick, but I think that'd break backwards compat unless you add an if statement to the PAE computation in the case of empty sigs (or an if statement and call this dssev2). I'm curious to know what does @MarkLodato and @haydentherapper think about this tho!

MarkLodato commented 7 months ago

I just realized that I never recorded my disagreement with the extension mechanism on GitHub (though I had said it during our meeting). I just added https://github.com/secure-systems-lab/dsse/issues/59#issuecomment-1969341050 now. As I said there, my preference would be to merge the Sigstore bundle into DSSE directly so that other non-Sigstore use cases can take advantage. That's why I hadn't been commenting on this PR, since it seemed like I was the only one who disagreed.

That said, I'd recommend against signing the kind. The extension is should just be extra metadata required for verification, no? I don't think it should fundamentally alter the signature?

SantiagoTorres commented 7 months ago

Thanks for the refresher on #56!

My position on hashing the kind lands around the fact that some security properties could be "peeled off" by a transport (be it malicious or not). One case would be the fact that a sigstore bundle exists alltogether, thus removing sigstore. This, at best, would make a paranoid verifier know that something is iffy (i.e., because it expects the extension), at worse, it'd remove sigstore's security benefits wholesale.

I'm not sure if we want to make it so that verifiers need to enforce policy on extensions existing, and I'm a little nervous about things not being DSSE anymore in that case :)

jkjell commented 7 months ago

I'm not sure if we want to make it so that verifiers need to enforce policy on extensions existing

Isn't that what Sigstore is already doing with cosign? For example:

$ cosign verify <image URI> --certificate-identity=name@example.com
                            --certificate-oidc-issuer=https://accounts.example.com

Perhaps there's a difference to "enforce policy" versus "defining a functionary" that I'm conflating. ☺️

MarkLodato commented 7 months ago

But is it sufficient to only sign the kind and not the value? In particular, couldn't the attacker still peel off fields within the bundle to have the same effect, e.g. deleting the inclusion proof?

My inclination would be to either:

trishankatdatadog commented 7 months ago

I'm not sure if we want to make it so that verifiers need to enforce policy on extensions existing, and I'm a little nervous about things not being DSSE anymore in that case :)

My concern is that, with either option, DSSE could stop being DS anymore, because, either way, where do you draw the line for more and more extensions?

At least with Option 2, it's sparkling clear how the verifier should handle the envelope. With Option 1, anything goes (especially with an evergrowing combination of extensions) unless there is sparkling clear specification.

What did I miss?

SantiagoTorres commented 7 months ago

But is it sufficient to only sign the kind and not the value? In particular, couldn't the attacker still peel off fields within the bundle to have the same effect, e.g. deleting the inclusion proof?

I agree, I think my point is that this could be a "reasonable" compromise between options 1 and 2. By signing the kind you are able to say: "There is supposed to be a non-empty payload on this". The attack surface is thereby reduced to the attacker needs to mutate a value inside of the payload, rather than remove the extension altogether.

In the case of sigstore this would mean that the attacker either needs to forge an inclusion proof, or something akin, as opposed to "there was no sigstore related information" (which then the client needs to verify in its own way).

MarkLodato commented 7 months ago

To me, that's the worst of both worlds. I don't see the signing of the kind providing any security value since the attacker can still modify the bundle, yet it breaks backwards compatibility and adds complexity.

SantiagoTorres commented 7 months ago

Hmm, I just explained the security win w/ the bundle example. Let me repeat: the attacker can't forge a bundle if they can't authenticate the bundle themselves and they must provide a bundle.

Agreed on the complexity and backwards compat, however.

I'm not super married to either option, but I wanted to put this forward for discussion

MarkLodato commented 7 months ago

the attacker can't forge a bundle if they can't authenticate the bundle themselves and they must provide a bundle.

What is stopping someone from deleting the tlog_entires and timestamp_verification_data from within the bundle?

SantiagoTorres commented 7 months ago

What is stopping someone from deleting the tlog_entires and timestamp_verification_data from within the bundle?

At that point we'd be arguing whether the extension itself should be structured in form x or y (i.e., are tlog_entries or timestamp_verification_data optional?). We still know we expect a sigstore bundle, however.

MarkLodato commented 7 months ago

Unless you can point to a specific attack that this would stop for Sigstore (our only use case for extensions thus far), I am strongly against signing the kind. It just adds complexity and breaks backwards compatibility for seemingly no benefit. "Know we expect a sigstore bundle" doesn't really provide any value. The verifier already knows that they need a sigstore bundle; they're not just going to blindly accept any arbitrary signature from any source.

mrjoelkamp commented 7 months ago

@SantiagoTorres I'm not sure if we want to make it so that verifiers need to enforce policy on extensions existing, and I'm a little nervous about things not being DSSE anymore in that case :)

We currently have some prototypes using the DSSE extension as defined in this PR (option 2 in #59) and the enforcement of what extensions the signature is required to have can be handled in a policy (rego) in the client verification process, similarly to how you would require specific attestations to be attached to the image, the policy can require that the DSSE signature have specific extensions. That way you can enforce a transparency log (TL) entry and/or a timestamp authority (TSA) to be attached to the signature. I believe it makes most sense to handle this in policy vs trying to enforce it in DSSE.

SantiagoTorres commented 7 months ago

the enforcement of what extensions the signature is required to have can be handled in a policy (rego) in the client verification process, similarly to how you would require specific attestations to be attached to the image, the policy can require that the DSSE signature have specific extensions.

I agree that these two are equivalent. I'm mostly being paranoid about not everybody implementing this check.

To reiterate, I'm not strongly married to the idea of hashing the key, but I wanted to properly discuss it being a possibility. Where I'm coming from, the hashing of the key forces the DSSE layer to ensure the signature itself fails close if a particular extension isn't present. Instead, an implementer of verification could foot-gun themselves into passing verification if the extension isn't present.

Don't get me wrong, both would be a bug, but the latter would make it so that the signature portion is the one that fails, rather than a semantic layer above.

I don't see a lot of excitement/support for this portion, so it's perhaps best to forget about that portion :sweat_smile:

adityasaky commented 5 months ago

I do like the idea of including the extension's kind in the signature. I think it builds at least some guardrails as someone verifying a sigstore signature must explicitly acknowledge they're verifying a sigstore signature, even if they don't do all the right things at the policy layer above such as checking the subject, verifying inclusion in rekor etc.

What's the best way to unblock this?

lukpueh commented 5 months ago

I think this is fine. Though it would be nice, if we could find consensus with the original author of the dsse spec. cc @MarkLodato

MarkLodato commented 5 months ago

The proposal as it is currently written looks good to me. Once it is marked as ready for review and comments are cleaned up, I'm happy to approve.

I don't think signing "kind" is a good idea. To repeat:

  1. It breaks backwards compatibility.
  2. It adds complexity.
  3. It adds almost no security value, as far as I can tell.

I suggest keeping this update limited to just adding an unauthenticated extension. A separate issue could discuss updates to the protocol to make some extensions required, which IMO would require a lot more design.

adityasaky commented 5 months ago

Note: I'll squash my commits when this is ready to merge, I'm holding off from too many force pushes at this point.

haydentherapper commented 5 months ago

Not to derail this, but do we have a project in mind within Sigstore that will use this? Sigstore adoption is quite different than when this draft started, and the Sigstore clients currently have all implemented the protobuf-based bundle format. I don't expect we'll see another format introduced to avoid any fracturing of supported formats across clients.

Generally I think this is useful outside of Sigstore too, but just wanted to bring this up given Sigstore is called out explicitly here.

lukpueh commented 5 months ago

Btw. have we resolved the question, if an extension makes the value of the sig bytes obsolete? See https://github.com/secure-systems-lab/dsse/issues/59#issuecomment-1569028836 and following comments.

adityasaky commented 5 months ago

Btw. have we resolved the question, if an extension makes the value of the sig bytes obsolete? See https://github.com/secure-systems-lab/dsse/issues/59#issuecomment-1569028836 and following comments.

For the sigstore case, yes. I think that comment was based on the extension being the sigstore bundle proto message rather than the verification_material proto message. Perhaps in the mention of an extension not containing another DSSE envelope (suggested by @haydentherapper IIRC), we should also clarify it can't contain sig bytes either.

Not to derail this, but do we have a project in mind within Sigstore that will use this? Sigstore adoption is quite different than when this draft started, and the Sigstore clients currently have all implemented the protobuf-based bundle format. I don't expect we'll see another format introduced to avoid any fracturing of supported formats across clients.

Another that comes to mind is spiffe/spire, I think @jkjell may be able to weigh in there.

Re sigstore: I definitely understand not wanting to fracture formats! I think the bundle format is great for the cases where we have a single sigstore signature on a DSSE envelope, while the extension is great for multiple signature scenarios, where only some of them may be sigstore signatures (I can see this happening with in-toto and TUF scenarios). If we end up not signing over the extension kind, it's possible sigstore clients don't need to be aware of the extension right away, with the conversion into the bundle format happening at the layer invoking the client (eg: an in-toto or TUF implementation). The invoking clients still benefit from having extensions for the multi-sig, mixed signing scenarios as they wouldn't have to duplicate envelopes / payloads and reason about all of them. WDYT?

jkjell commented 5 months ago

As Aditya mentioned, signing with SPIFFE x509-SVID issued cert was another likely extension format. It has a bundle format defined as well. That would also probably have a timestamp signature associated with it.

I think the proposal looks good as is, and we should move forward with it.

Besides multiple signatures and multiple extensions, the sigstore verification process is a good example of a reason to not sign over the extension type. There are many possible verification flows for the single extension. What version is the bundle? Is it using a Rekor entry or not? Are you doing an online verification of the CTL?

From a practical standpoint, I can see other consumers, outside of Sigstore (Witness, SLSA, etc), supporting the Sigstore extension-type but, I doubt Sigstore would have any use for supporting any other extension type.

haydentherapper commented 4 months ago

If we end up not signing over the extension kind

I might have forgotten, but was this discussed previously? I assume we wouldn't want to sign over the extension to avoid this being a breaking change and mandating clients understand the extension format.

The invoking clients still benefit from having extensions for the multi-sig, mixed signing scenarios as they wouldn't have to duplicate envelopes / payloads and reason about all of them. WDYT?

I agree that I think this approach is better for multi-sigs given the current Sigstore bundle, as the bundle doesn't support a mapping between sigs and verification keys/certs. It's a little hard for me to predict what the exact use case currently since we haven't had a need for multi-sig DSSEs, but we should probably document somewhere that if the need does arise, we should revisit using this format.

trishankatdatadog commented 4 months ago

I might have forgotten, but was this discussed previously? I assume we wouldn't want to sign over the extension to avoid this being a breaking change and mandating clients understand the extension format.

If signing over kind is a breaking change, then it should be a V2 requirement (if at all) @SantiagoTorres @JustinCappos

adityasaky commented 4 months ago

If signing over kind is a breaking change, then it should be a V2 requirement (if at all)

To clarify, this PR does not propose signing over the extension kind, though it was discussed in the comments. As it stands, this PR does not introduce a breaking change, and one of the points against signing over the kind was to avoid breaking backwards compatibility.

I propose we move ahead with this PR (pending reviews from @SantiagoTorres, @JustinCappos, @lukpueh) but hold off on creating a release of the specification. Before we actually release with extensions, we can nail down whether we want to sign over the kind + anything else that needs to be discussed. Thoughts? :smile:

trishankatdatadog commented 4 months ago

I propose we move ahead with this PR (pending reviews from @SantiagoTorres, @JustinCappos, @lukpueh) but hold off on creating a release of the specification. Before we actually release with extensions, we can nail down whether we want to sign over the kind + anything else that needs to be discussed. Thoughts?

That might be too confusing to casual observers who need to realise that just because something is there in a commit, doesn't mean it's been released. I argue for pushing for consensus (been too long, sorry on behalf of everyone) and releasing ASAP. Any objections from the other 8 maintainers?

MarkLodato commented 4 months ago

That might be too confusing to casual observers who need to realise that just because something is there in a commit, doesn't mean it's been released. I argue for pushing for consensus (been too long, sorry on behalf of everyone) and releasing ASAP. Any objections from the other 8 maintainers?

I agree. We don't have a formal release process, so we're currently living at head. Submitted == released. Perhaps an alternative is marking the extension field as "experimental", with a warning that the design is subject to change? I don't foresee any changes, but it probably is a good idea to get some real-world feedback before committing.

adityasaky commented 4 months ago

Marked extensions as experimental. I also bumped the version to 1.1.0-draft, I'm not sure if we have a process there.

adityasaky commented 4 months ago

As discussed in https://github.com/secure-systems-lab/dsse/pull/61#discussion_r1596835160, #67 has been merged with a versioning scheme and v1.0.1 has been tagged. I've updated this PR to specify the version as just v1.1.0 in each .md file, and I've updated the base branch to be devel, which points to the current tip of the master branch.

adityasaky commented 4 months ago

Are we ready to merge this into devel? I see the outstanding review request is for @SantiagoTorres. :smile:

adityasaky commented 4 months ago

Thank you everyone!