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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 16 commits into from
Oct 2, 2023
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
3b4f23e
Renamed Verify to VerifyUnsafe.
phillmv Sep 27, 2023
91f6fc3
gofmt -s
phillmv Sep 27, 2023
06f8f08
Reverted VerifyUnsafe, VerifyWith* funcs.
phillmv Sep 28, 2023
c24c5e7
Introduced new WithoutIdentitiesUnsafe, WithoutArtifactUnsafe policy …
phillmv Sep 29, 2023
e00c64c
Merge branch 'main' into make-verify-interface-clearer
phillmv Sep 29, 2023
643a681
Updated comments.
phillmv Sep 29, 2023
3fc2f32
Encapsulated the construction of PolicyConfig inside PolicyBuilder,
phillmv Sep 29, 2023
11ccc70
Made receiver variables consistent with other PolicyOptions.
phillmv Sep 29, 2023
f8a40f9
Linting.
phillmv Sep 29, 2023
392c5e5
Added a length check in case no certificate identities are provided.
phillmv Sep 29, 2023
8dc24e5
renamed variable to identityPolicies.
phillmv Oct 2, 2023
650a0e4
Removed unnecessary pointer deref.
phillmv Oct 2, 2023
340d95e
Renamed VerifierConfigurator type to VerifierOption for consistency w…
phillmv Oct 2, 2023
975b16c
Toned down some of the comments around Unsafe PolicyOptions.
phillmv Oct 2, 2023
6f268a1
On 2nd thought, BuildConfig ought to return a pointer.
phillmv Oct 2, 2023
fc8568c
Added tests for policy options, made existing test use Safe path.
phillmv Oct 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions cmd/conformance/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ func main() {
log.Fatal(err)
}

_, err = sev.Verify(bun, policyConfig...)
_, err = sev.VerifyUnsafe(bun, policyConfig...)
phillmv marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
log.Fatal(err)
}
Expand Down Expand Up @@ -209,7 +209,7 @@ func main() {
log.Fatal(err)
}

_, err = sev.Verify(b, policyConfig...)
_, err = sev.VerifyUnsafe(b, policyConfig...)
if err != nil {
log.Fatal(err)
}
Expand Down
2 changes: 1 addition & 1 deletion cmd/sigstore-go/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ func run() error {
policyConfig = append(policyConfig, verify.WithArtifact(file))
}

res, err := sev.Verify(b, policyConfig...)
res, err := sev.VerifyUnsafe(b, policyConfig...)
if err != nil {
return err
}
Expand Down
14 changes: 7 additions & 7 deletions pkg/verify/signature_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,21 +56,21 @@ func TestEnvelopeSubject(t *testing.T) {
verifier, err := verify.NewSignedEntityVerifier(virtualSigstore, verify.WithTransparencyLog(1))
assert.NoError(t, err)

_, err = verifier.Verify(entity)
_, err = verifier.VerifyUnsafe(entity)
assert.NoError(t, err)

_, err = verifier.Verify(entity, verify.WithArtifact(bytes.NewBufferString(subjectBody)))
_, err = verifier.VerifyUnsafe(entity, verify.WithArtifact(bytes.NewBufferString(subjectBody)))
assert.NoError(t, err)

_, err = verifier.Verify(entity, verify.WithArtifactDigest("sha256", digest))
_, err = verifier.VerifyUnsafe(entity, verify.WithArtifactDigest("sha256", digest))
assert.NoError(t, err)

// Error: incorrect artifact
_, err = verifier.Verify(entity, verify.WithArtifact(bytes.NewBufferString("Hi, I am a different subject!")))
_, err = verifier.VerifyUnsafe(entity, verify.WithArtifact(bytes.NewBufferString("Hi, I am a different subject!")))
assert.Error(t, err)

// Error: incorrect digest algorithm
_, err = verifier.Verify(entity, verify.WithArtifactDigest("sha512", digest))
_, err = verifier.VerifyUnsafe(entity, verify.WithArtifactDigest("sha512", digest))
assert.Error(t, err)
}

Expand All @@ -85,15 +85,15 @@ func TestSignatureVerifierMessageSignature(t *testing.T) {
verifier, err := verify.NewSignedEntityVerifier(virtualSigstore, verify.WithTransparencyLog(1))
assert.NoError(t, err)

result, err := verifier.Verify(entity, verify.WithArtifact(bytes.NewBufferString(artifact)))
result, err := verifier.VerifyUnsafe(entity, verify.WithArtifact(bytes.NewBufferString(artifact)))
assert.NoError(t, err)

assert.Equal(t, result.Signature.Certificate.SubjectAlternativeName.Value, "[email protected]")
assert.Equal(t, result.VerifiedTimestamps[0].Type, "Tlog")

// should fail to verify with a different artifact
artifact2 := "Hi, I am a different artifact!"
result, err = verifier.Verify(entity, verify.WithArtifact(bytes.NewBufferString(artifact2)))
result, err = verifier.VerifyUnsafe(entity, verify.WithArtifact(bytes.NewBufferString(artifact2)))
assert.Error(t, err)
assert.Nil(t, result)
}
87 changes: 77 additions & 10 deletions pkg/verify/signed_entity.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,11 @@ type VerifierConfigurator func(*VerifierConfig) error
// NewSignedEntityVerifier creates a new SignedEntityVerifier. It takes a
// root.TrustedMaterial, which contains a set of trusted public keys and
// certificates, and a set of VerifierConfigurators, which set the config
// that determines the behaviour of the Verify function.
// that determines the behaviour of the Verify... functions.
//
// VerifierConfig's set of options should match the properties of a given
// Sigstore deployment, i.e. whether to expect SCTs, Tlog entries, or signed
// timestamps.
func NewSignedEntityVerifier(trustedMaterial root.TrustedMaterial, options ...VerifierConfigurator) (*SignedEntityVerifier, error) {
var err error
c := VerifierConfig{}
Expand Down Expand Up @@ -170,7 +174,7 @@ type PolicyOptions struct {
artifactDigestAlgorithm string
}

// WithCertificateIdentity allows the caller of Verify to enforce that the
// WithCertificateIdentity allows the caller of VerifyUnsafe to enforce that the
// SignedEntity being verified was created by a given identity, as defined by
// the Fulcio certificate embedded in the entity. If this policy is enabled,
// but the SignedEntity does not have a certificate, verification will fail.
Expand Down Expand Up @@ -199,7 +203,7 @@ func WithCertificateIdentity(identity CertificateIdentity) PolicyOptionConfigura
}
}

// WithArtifact allows the caller of Verify to enforce that the SignedEntity
// WithArtifact allows the caller of VerifyUnsafe to enforce that the SignedEntity
// being verified was created from, or references, a given artifact.
//
// If the SignedEntity contains a DSSE envelope, then the artifact digest is
Expand All @@ -216,7 +220,7 @@ func WithArtifact(artifact io.Reader) PolicyOptionConfigurator {
}
}

// WithArtifactDigest allows the caller of Verify to enforce that the
// WithArtifactDigest allows the caller of VerifyUnsafe to enforce that the
// SignedEntity being verified was created for a given artifact digest.
//
// If the SignedEntity contains a MessageSignature that was signed using the
Expand All @@ -237,27 +241,90 @@ func WithArtifactDigest(algorithm string, artifactDigest []byte) PolicyOptionCon
}
}

// Verify checks the cryptographic integrity of a given SignedEntity according
// VerifyWithArtifact checks the cryptographic integrity of a given SignedEntity,
// and enforces that the SignedEntity being verified:
// 1. was created from, or references, the provided artifact, and
// 2. was created by one of the provided CertificateIdentities.
//
// Verification will fail if the SignedEntity:
// - does not have a certificate,
// - does not have a valid signature, or was signed outside of the cert's
// validity period
//
// If and only if the verification is successful, this function returns a
// VerificationResult whose contents have been verified and which can be used
// for additional policy enforcement (i.e. enforcing the presence of a given
// provenance predicate).
//
// For a more convenient way to initialize a CertificateIdentity, consult the
// NewShortCertificateIdentity function.
//
// For more information, consult: VerifyUnsafe, WithArtifact, WithCertificateIdentity
func (v *SignedEntityVerifier) VerifyWithArtifact(entity SignedEntity, artifact io.Reader, identities ...CertificateIdentity) (*VerificationResult, error) {
policies := []PolicyOptionConfigurator{WithArtifact(artifact)}

for _, certID := range identities {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I've gone ahead and implemented this.

policies = append(policies, WithCertificateIdentity(certID))
}
return v.VerifyUnsafe(entity, policies...)
}

// VerifyWithArtifactDigest checks the cryptographic integrity of a given
// SignedEntity, and enforces that the SignedEntity being verified:
// 1. was created for, or references, the provided artifact digest, and
// 2. was created by one of the provided CertificateIdentities.
//
// Verification will fail if the SignedEntity:
// - does not have a certificate,
// - does not have a valid signature, or was signed outside of the cert's
// validity period
//
// If and only if the verification is successful, this function returns a
// VerificationResult whose contents have been verified and which can be used
// for additional policy enforcement (i.e. enforcing the presence of a given
// provenance predicate).
//
// For a more convenient way to initialize a CertificateIdentity, consult the
// NewShortCertificateIdentity function.
//
// For more information, consult: VerifyUnsafe, WithArtifactDigest, and
// WithCertificateIdentity
func (v *SignedEntityVerifier) VerifyWithArtifactDigest(entity SignedEntity, algorithm string, artifactDigest []byte, identities ...CertificateIdentity) (*VerificationResult, error) {
policies := []PolicyOptionConfigurator{WithArtifactDigest(algorithm, artifactDigest)}

for _, certID := range identities {
policies = append(policies, WithCertificateIdentity(certID))
}
return v.VerifyUnsafe(entity, policies...)
}

// VerifyUnsafe checks the cryptographic integrity of a given SignedEntity according
// to the options configured in the NewSignedEntityVerifier. Its purpose is to
// determine whether the SignedEntity was created by a Sigstore deployment we
// trust, as defined by keys in our TrustedMaterial.
//
// Verify then creates a VerificationResult struct whose contents' integrity
// have been verified. At the function caller's discretion, Verify may then
// verify the contents of the VerificationResults using supplied PolicyOptions.
// If and only if verification is successful, VerifyUnsafe will return a
// VerificationResult struct whose contents' integrity have been verified. At
// the function caller's discretion, VerifyUnsafe may then verify the contents
// of the VerificationResults using supplied PolicyOptions.
// See WithCertificateIdentity for more details.
//
// If the SignedEntity contains a MessageSignature, then the artifact or its
// digest must be provided to the Verify function, as it is required to verify
// digest must be provided to the VerifyUnsafe function, as it is required to verify
// the signature. See WithArtifact and WithArtifactDigest for more details.
//
// DO NOT USE THIS FUNCTION, unless you know what you are doing. As the name
// implies, this function is **not safe**: the semantics for what constitutes an
// appropriate level of verification and policy enforcement are subtle, and
// complex. Use VerifyWithArtifact or VerifyWithArtifactDigest instead.
//
// If no policy options are provided, callers of this function SHOULD:
// - (if the signed entity has a certificate) verify that its Subject Alternate
// Name matches a trusted identity, and that its Issuer field matches an
// expected value
// - (if the signed entity has a dsse envelope) verify that the envelope's
// statement's subject matches the artifact being verified
func (v *SignedEntityVerifier) Verify(entity SignedEntity, options ...PolicyOptionConfigurator) (*VerificationResult, error) {
func (v *SignedEntityVerifier) VerifyUnsafe(entity SignedEntity, options ...PolicyOptionConfigurator) (*VerificationResult, error) {
var err error
policy := &PolicyOptions{}
for _, opt := range options {
Expand Down
14 changes: 7 additions & 7 deletions pkg/verify/signed_entity_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ func TestEntitySignedByPublicGoodWithTlogVerifiesSuccessfully(t *testing.T) {
v, err := v.NewSignedEntityVerifier(tr, v.WithTransparencyLog(1))
assert.Nil(t, err)

res, err := v.Verify(entity)
res, err := v.VerifyUnsafe(entity)
assert.Nil(t, err)
assert.NotNil(t, res)

Expand All @@ -68,7 +68,7 @@ func TestEntitySignedByPublicGoodWithoutTimestampsVerifiesSuccessfully(t *testin
v, err := v.NewSignedEntityVerifier(tr, v.WithoutAnyObserverTimestampsInsecure())
assert.Nil(t, err)

res, err := v.Verify(entity)
res, err := v.VerifyUnsafe(entity)
assert.Nil(t, err)
assert.NotNil(t, res)
}
Expand All @@ -80,7 +80,7 @@ func TestEntitySignedByPublicGoodWithHighTlogThresholdFails(t *testing.T) {
v, err := v.NewSignedEntityVerifier(tr, v.WithTransparencyLog(2))
assert.Nil(t, err)

res, err := v.Verify(entity)
res, err := v.VerifyUnsafe(entity)
assert.NotNil(t, err)
assert.Nil(t, res)
}
Expand All @@ -92,7 +92,7 @@ func TestEntitySignedByPublicGoodExpectingTSAFails(t *testing.T) {
v, err := v.NewSignedEntityVerifier(tr, v.WithTransparencyLog(1), v.WithSignedTimestamps(1))
assert.Nil(t, err)

res, err := v.Verify(entity)
res, err := v.VerifyUnsafe(entity)
assert.NotNil(t, err)
assert.Nil(t, res)
}
Expand All @@ -110,15 +110,15 @@ func TestEntitySignedByPublicGoodWithCertificateIdentityVerifiesSuccessfully(t *

assert.Nil(t, err)

res, err := verifier.Verify(entity,
res, err := verifier.VerifyUnsafe(entity,
v.WithCertificateIdentity(badCI),
v.WithCertificateIdentity(goodCI))
assert.Nil(t, err)

assert.Equal(t, res.VerifiedIdentity.Issuer, v.ActionsIssuerValue)

// but if only pass in the bad CI, it will fail:
res, err = verifier.Verify(entity,
res, err = verifier.VerifyUnsafe(entity,
v.WithCertificateIdentity(badCI))
assert.NotNil(t, err)
assert.Nil(t, res)
Expand All @@ -140,7 +140,7 @@ func TestThatAllTheJSONKeysStartWithALowerCase(t *testing.T) {
verifier, err := v.NewSignedEntityVerifier(tr, v.WithTransparencyLog(1))
assert.Nil(t, err)

res, err := verifier.Verify(entity)
res, err := verifier.VerifyUnsafe(entity)
assert.Nil(t, err)

rawJSON, err := json.Marshal(res)
Expand Down