Make unsafe uses of `Verify` function explicit / make `Verify` options safe by default #3

phillmv · 2023-09-27T14:10:16Z

What?

This PR renames re-organizes the logic SignedEntityVerifier.Verify's functional options in order to make unsafe uses of the Verify function explicit / make Verify's options safe by default.

After some discussion, it accomplishes this by:

Introducing WithoutArtifactUnsafe and WithoutIdentitiesUnsafe policy options
Reversing the logic that determines whether the Verify loop checks for the presence of artifacts and certificate identities; before, the default was to assume we would not check for artifacts or identities. Now that behaviour must be explicitly requested.
Adding a params struct to Verify for encapsulating the optional funcs while giving us the opportunity to expand policy options in the future.

Before

res, err := sev.Verify(entity)
// No identity is verified, no artifact is verified, this is unsafe!
// But you wouldn't know unless you Know Everything

After

res, err := sev.Verify(entity, NewPolicy(WithoutArtifactUnsafe(), WithoutIdentitiesUnsafe()))
// Same result as above, but now you have to _intentionally_ create this result

Why?

This PR seeks to make it explicit to any downstream users of this library what the preferred verification path is. A casual user browsing the package's Godoc may not realize that the previous Verify function had to be used very carefully and very intentionally in order to produce adequate results; we accidentally ended up with a Verify loop that by default would work with the absence of an artifact and certificate identities.

Accidentally foisting complexity is a common pitfall of verification libraries, and has led to many vulnerabilities.
To quote from the linked blog post,

In short, why is it easier for your users to do the wrong thing than it is for them to do the right thing?
…
If you do work on a [verification library, then you should]:

Make it easy to use the function(s) that check signatures and harder — much harder — to use the ones that accept no signature, invalid signatures, or "alg": "none".

Make it require significantly more effort on the developer’s part to accept "alg": "none" or unsigned tokens, and ensure it leaves warning signs in code that a reviewer can spot from 12 feet away.

I find this to be very compelling. For this reason, there should be an obviously safe path for callers to use that requires little to no thinking or knowledge of intricate details of the underlying specifications.

Whereas before,

Users had to know a priori that the Best Practice for the Verify function requires furnishing an artifact and a certificate identity

Now,

Users wishing to avoid supplying an artifact or a certificate identity must use options with Unsafe in the name.

Hopefully, any developer blindly typing WithoutIdentitiesUnsafe will feel foolish, think twice about what they are doing and stand out during code review.

Introduced VerifyWithArtifact and VerifyWithArtifactDigest to make it explicit what the preferred usage path is; users should not use VerifyUnsafe unless they are very very careful and know what they are doing.

kommendorkapten · 2023-09-27T14:20:32Z

pkg/verify/signed_entity.go

+func (v *SignedEntityVerifier) VerifyWithArtifact(entity SignedEntity, artifact io.Reader, identities ...CertificateIdentity) (*VerificationResult, error) {
+	policies := []PolicyOptionConfigurator{WithArtifact(artifact)}
+
+	for _, certID := range identities {


Don't we need to fail if no identities are passed? Or will verify unsafe fail if the set of identities is empty/nil?

Good catch! No harm in enforcing it in both spots, actually.

I think it's confusing to have so many ways of specifying CertificateIdentity. Should the user call verify.WithCertificateIdentity, or pass it in to Verify* (or both?)

Ideally, there would be one way for the user to specify this information.

Yeah, there should probably only be one way, for security products the API complexity should be minimal. Would it make sense to open an issue or GDoc for a discussion of the API for sigstore-go? Having an orthogonal API is crucial IMO.

Okay, so another idea @phillmv and I discussed 1-1: instead of having Verify* functions, go back to having one Verify function, but have it take arguments like insecureWithoutArtifactIsOkay or insecureWithoutCertificateIdentityIsOkay. Then Verify can look at the policy options, and if the the policy option is missing it can check if the corresponding insecure* option was passed.

That way there's one way to specify the certificate identity, and you still support broad use cases, and you're still requiring the developer type insecure.

Yes, and, what if they're just a positional ArtifactPolicyConfigurator / IdentityPolicyConfigurator, so we don't have to change any of the existing logic / add new interfaces?

i.e.

type ArtifactPolicyConfigurator func(*PolicyOptions) error

and

type IdentityPolicyConfigurator func(*PolicyOptions) error

and the existing logic stays the same.

hrm maybe it'd be like,

Verify(entity, ArtifactPolicyConfigurator, IdentityPolicyConfigurator…)

so you can be like,

Verify(entity, WithArtifact(), WithCertIdentity(), WithCertIdentity()…)

The only thing it forecloses is the ability to add new policy enforcement options in the future.

If we care for extensibility, for not have an options struct? Very simple to work with, and can easily extend the struct in the future without braking the API (i.e parameters added/removed). That is if course as long as the struct can be created with sensible defaults. I'm personally not a fan of the functional config pattern, even though I know it's popular in the Go world.

But @phillmv 's latest suggestion

Verify(entity, WithArtifact(), WithCertIdentity(), WithCertIdentity()…)

Looks good I think.

Had a little chat with @kommendorkapten, and maybe an options struct is the way. Something like:

id := NewShortCertificateIdentity("foo", "", "", "") policy := NewPolicy(WithArtifact(foo), WithId(id), WithId(id2)) Verify(entity, trustMaterial, policy)

This would preserve the option of extending policy in the future, for example, maybe adding the ability to enforce a predicate type?

Barring any objections, I think I'll try whipping this up!

OK, I've gone ahead and implemented this.

kommendorkapten · 2023-09-27T14:25:03Z

When verifying with a bundle, the payload in the DSSE envelope is the artifact (even in our case it contains an in-toto attestation that references another artifact). There is no safe Verify that only accepts bundle (only with either digest or artifact). I would think it makes sense to add a Verify that only verifies the DSSE envelope (with identities if signed by a certificate).

phillmv · 2023-09-27T14:36:06Z

When verifying with a bundle, the payload in the DSSE envelope is the artifact

@kommendorkapten Here we've been using "artifact" to mean the subject that was signed or is referenced in the attestation. If you were to pass in the DSSE envelope as the artifact, the function should fail 🤔.

Would it be less confusing if we named it VerifyWithSubject and VerifyWithSubjectDigest?

kommendorkapten · 2023-09-27T14:42:39Z

If you were to pass in the DSSE envelope as the artifact, the function should fail 🤔.

I'm talking about an edge case(?). As the Sigstore bundle does not mention anything about the content in the DSSE envelope, it can be a text string "foo bar", with payload type text/plain. That is a totally valid bundle. From our use-case this is moot, but from general sigstore perspective it's fine.

As I understand we do have a verify path for a "genereic" DSSE envelope: https://github.com/github/sigstore-go/blob/cd2b59895a54eafae3fd9ab080d4580114c5fdd8/pkg/verify/signature.go#L88

cmd/conformance/main.go

steiza · 2023-09-27T14:42:14Z

pkg/verify/signed_entity.go

+func (v *SignedEntityVerifier) VerifyWithArtifact(entity SignedEntity, artifact io.Reader, identities ...CertificateIdentity) (*VerificationResult, error) {
+	policies := []PolicyOptionConfigurator{WithArtifact(artifact)}
+
+	for _, certID := range identities {


I think it's confusing to have so many ways of specifying CertificateIdentity. Should the user call verify.WithCertificateIdentity, or pass it in to Verify* (or both?)

Ideally, there would be one way for the user to specify this information.

phillmv · 2023-09-27T19:59:06Z

As the Sigstore bundle does not mention anything about the content in the DSSE envelope, it can be a text string "foo bar", with payload type text/plain. That is a totally valid bundle. From our use-case this is moot, but from general sigstore perspective it's fine.

As a side note, I REALLY THINK we should require in-toto payload when dealing with dsse envelopes 😝. Having two separate methods for handling bare signatures (MessageSignature, raw DSSE envelope) is going to lead us down a path of madness.

kommendorkapten · 2023-09-28T06:07:52Z

If we require an in-toto attestation in the DSSE envelope, it should be clearly documented as this is a deviation from the bundle spec. I don't think it will be controversial as in the Sigstore community we should expect in-toto attestations. But it must be clear when reading the docs.

…options.

Signed-off-by: Phill MV <[email protected]>

added validation checks. Signed-off-by: Phill MV <[email protected]>

Signed-off-by: Phill MV <[email protected]>

phillmv · 2023-09-29T20:27:02Z

Re-requesting a review as I have implemented the changes suggested in the previous round of feedback!

cmd/conformance/main.go

pkg/verify/signed_entity.go

…ith PolicyOption.

Signed-off-by: Phill MV <[email protected]>

This way, we can't accidentally use a default PolicyConfig struct. Signed-Off-By: Phill MV <[email protected]>

Signed-Off-By: Phill MV <[email protected]>

codysoyland

LGTM!

Renamed Verify to VerifyUnsafe.

3b4f23e

Introduced VerifyWithArtifact and VerifyWithArtifactDigest to make it explicit what the preferred usage path is; users should not use VerifyUnsafe unless they are very very careful and know what they are doing.

phillmv requested review from steiza and a team as code owners September 27, 2023 14:10

kommendorkapten reviewed Sep 27, 2023

View reviewed changes

gofmt -s

91f6fc3

steiza reviewed Sep 27, 2023

View reviewed changes

phillmv added 3 commits September 29, 2023 13:42

Reverted VerifyUnsafe, VerifyWith* funcs.

06f8f08

Introduced new WithoutIdentitiesUnsafe, WithoutArtifactUnsafe policy …

c24c5e7

…options.

Merge branch 'main' into make-verify-interface-clearer

e00c64c

Signed-off-by: Phill MV <[email protected]>

phillmv changed the title ~~Add VerifyWithArtifact and VerifyWithArtifactDigest~~ Make unsafe uses of Verify function explicit / make Verify options safe by default Sep 29, 2023

phillmv added 5 commits September 29, 2023 15:52

Updated comments.

643a681

Signed-off-by: Phill MV <[email protected]>

Encapsulated the construction of PolicyConfig inside PolicyBuilder,

3fc2f32

added validation checks. Signed-off-by: Phill MV <[email protected]>

Made receiver variables consistent with other PolicyOptions.

11ccc70

Signed-off-by: Phill MV <[email protected]>

Linting.

f8a40f9

Signed-off-by: Phill MV <[email protected]>

Added a length check in case no certificate identities are provided.

392c5e5

Signed-off-by: Phill MV <[email protected]>

phillmv requested a review from a team September 29, 2023 20:26

kommendorkapten reviewed Oct 2, 2023

View reviewed changes

cmd/conformance/main.go Outdated Show resolved Hide resolved

kommendorkapten reviewed Oct 2, 2023

View reviewed changes

pkg/verify/signed_entity.go Show resolved Hide resolved

phillmv added 6 commits October 2, 2023 09:49

renamed variable to identityPolicies.

8dc24e5

Removed unnecessary pointer deref.

650a0e4

Renamed VerifierConfigurator type to VerifierOption for consistency w…

340d95e

…ith PolicyOption.

Toned down some of the comments around Unsafe PolicyOptions.

975b16c

Signed-off-by: Phill MV <[email protected]>

On 2nd thought, BuildConfig ought to return a pointer.

6f268a1

This way, we can't accidentally use a default PolicyConfig struct. Signed-Off-By: Phill MV <[email protected]>

Added tests for policy options, made existing test use Safe path.

fc8568c

Signed-Off-By: Phill MV <[email protected]>

codysoyland approved these changes Oct 2, 2023

View reviewed changes

phillmv merged commit 67b717d into main Oct 2, 2023
7 checks passed

phillmv deleted the make-verify-interface-clearer branch October 2, 2023 19:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make unsafe uses of `Verify` function explicit / make `Verify` options safe by default #3

Make unsafe uses of `Verify` function explicit / make `Verify` options safe by default #3

phillmv commented Sep 27, 2023 •

edited

Loading

kommendorkapten Sep 27, 2023

phillmv Sep 27, 2023

steiza Sep 27, 2023

kommendorkapten Sep 27, 2023

steiza Sep 27, 2023

phillmv Sep 27, 2023

phillmv Sep 27, 2023

kommendorkapten Sep 28, 2023

phillmv Sep 28, 2023

phillmv Sep 29, 2023

kommendorkapten commented Sep 27, 2023

phillmv commented Sep 27, 2023

kommendorkapten commented Sep 27, 2023

steiza Sep 27, 2023

phillmv commented Sep 27, 2023

kommendorkapten commented Sep 28, 2023

phillmv commented Sep 29, 2023 •

edited

Loading

codysoyland left a comment

Make unsafe uses of Verify function explicit / make Verify options safe by default #3

Make unsafe uses of Verify function explicit / make Verify options safe by default #3

Conversation

phillmv commented Sep 27, 2023 • edited Loading

What?

Before

After

Why?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kommendorkapten commented Sep 27, 2023

phillmv commented Sep 27, 2023

kommendorkapten commented Sep 27, 2023

Choose a reason for hiding this comment

phillmv commented Sep 27, 2023

kommendorkapten commented Sep 28, 2023

phillmv commented Sep 29, 2023 • edited Loading

codysoyland left a comment

Choose a reason for hiding this comment

Make unsafe uses of `Verify` function explicit / make `Verify` options safe by default #3

Make unsafe uses of `Verify` function explicit / make `Verify` options safe by default #3

phillmv commented Sep 27, 2023 •

edited

Loading

phillmv commented Sep 29, 2023 •

edited

Loading