Start adding more verification with VerificationOpts struct #153

malancas · 2022-11-17T16:12:08Z

Summary

Part of #121. This PR starts adding additional verification using a VerificationOpts struct. More verification will be added in follow ups after some upstream changes to the timestamp package.

This PR adds the following verifications:

Verify the TSR's certificate identifier matches a provided TSA certificate
Verify the leaf certificate's subject name matches a provided subject
Verify that the TSR's embedded leaf certificate matches a provided TSA certificate
Verify the TSA certificate and the intermediates have the extended key usage set to only timestamping
Verify the OID of the TSR matches an expected OID
Verify the status of the TSR does not contain an error
Verify the TSR's nonce matches a provided nonce

Release Note

Documentation

Signed-off-by: Meredith Lancaster <[email protected]>

hectorj2f · 2022-11-17T16:30:34Z

cmd/timestamp-cli/app/verify.go

+	cmd.Flags().String("nonce", "", "optional nonce passed with the request")
+	cmd.Flags().Var(NewFlagValue(oidFlag, ""), "oid", "optional oid passed with the request")
+	cmd.Flags().String("subject", "", "expected leaf certificate subject")
+	cmd.Flags().Var(NewFlagValue(fileFlag, ""), "tsa-cert", "path to TSA cert")


nit: we could omit adding --tsa- as prefix to the flag.

Q: Is this an intermediate or root cert ?

Signed-off-by: Meredith Lancaster <[email protected]>

malancas · 2022-11-17T20:09:26Z

cmd/timestamp-cli/app/verify.go

@@ -85,6 +97,62 @@ func runVerify() (interface{}, error) {
 		return nil, err
 	}

+	opts, err := verification.NewVerificationOpts(ts, artifact, pemBytes)


@haydentherapper currently all verification is handled in a single function VerifyTimestampResponse exported from the verification package. Here I add new verification functions separately. Do you have thoughts on whether all new verification functions should be called from VerifyTimestampResponse or if they should be called directly from here?

I think it'd be best to have a single exported verification function that performs all other verifications. If you think there's value in exporting the sub-verification functions, maybe we put them under an internal package?

Happy to use a single exported function, I'll make that change.

Signed-off-by: Meredith Lancaster <[email protected]>

haydentherapper · 2022-11-18T18:42:11Z

pkg/verification/verify.go

+func NewVerifyOpts() (VerifyOpts, error) {
+	opts := VerifyOpts{}
+
+	oidFlagVal := viper.GetString("oid")


Sorry, I might have misunderstood, but we should keep flags out of the core library and just in the client. If you want to have this function that handles things like parsing OIDs, it should take in values that are already read in from the flags. Also, personally I prefer to not have files read from disk as part of the library, so I think this should also be handled in the client and the chain passed into the function.

So I'd say either:

Move NewVerifyOpts to the client application with all flag parsing there

Move NewVerifyOpts to the client app, but provide some helper utilities for constructing OIDs and splitting PEM chains into cert pools

Keep NewVerifyOpts here, but have it take in all values (OID as a string, PEM chain as a string), and read in flags in the client app

Does that seem reasonable?

I agree about keeping the flag parsing in the app CLI package. Just moved that over.

Signed-off-by: Meredith Lancaster <[email protected]>

hectorj2f · 2022-11-25T11:24:15Z

cmd/timestamp-cli/app/verify.go

@@ -43,6 +47,10 @@ func addVerifyFlags(cmd *cobra.Command) {
 	cmd.MarkFlagRequired("timestamp") //nolint:errcheck
 	cmd.Flags().Var(NewFlagValue(fileFlag, ""), "cert-chain", "path to certificate chain PEM file")
 	cmd.MarkFlagRequired("cert-chain") //nolint:errcheck
+	cmd.Flags().String("nonce", "", "optional nonce passed with the request")
+	cmd.Flags().Var(NewFlagValue(oidFlag, ""), "oid", "optional oid passed with the request")


Does this refer to the hash algorithm OID ? If so, we might want to explain it in the description.

If so, we could rename this flag to hash-algorithm-oid

No, this refers to the TSA policy OID. I'll update the description to "optional TSA policy OID passed with the request".

cmd/timestamp-cli/app/verify.go

hectorj2f · 2022-11-25T11:33:52Z

cmd/timestamp-cli/app/verify.go

+	if err != nil {
+		return verification.VerifyOpts{}, fmt.Errorf("failed to parse root and intermediate certs from cert-chain flag: %w", err)
+	}
+	if roots != nil && intermediates != nil {


Suggestion: The code looks good. I just share a suggestion here, feel free to adjust it.
To avoid all these != nil if statements for all the VerifyOpts fields, you could create functions for verifyOpts as such:

(v *VerifyOpts) OID(...) where you set the OID if passed.
(v *VerifyOpts) Nonce(...) where you set the Nonce if passed.
...
So you move all that logic inside these functions and only check for errors here.

Since the VerifyOpts fields will be initialized to their respective zero values when the struct instance is created, we could just set the fields to the values returned by the getter functions like getCerts and remove the if statements altogether, since the getters will return zero values if no valid flag value is found. But I'm happy to add methods if you prefer.

Signed-off-by: Meredith Lancaster <[email protected]>

…icate flag Signed-off-by: Meredith Lancaster <[email protected]>

malancas · 2022-12-06T22:23:53Z

@haydentherapper there are changes to the PKCS7 methods in haydentherapper/pkcs7#2 that can be used in this pull request. I think it would be better to merge that one first and then pull the updated dependency into this pull request. The dependency update should be straight forward.

haydentherapper · 2022-12-06T22:28:01Z

cmd/timestamp-cli/app/verify.go

+	if err != nil {
+		return verification.VerifyOpts{}, fmt.Errorf("failed to parse root and intermediate certs from certificate-chain flag: %w", err)
+	}
+	if roots != nil && intermediates != nil {


It should be fine to unconditionally set the options. Also if there's only a root provided and no intermediates, will this conditional be skipped?

Yes it will be skipped, I'll remove that.

pkg/verification/verify.go

haydentherapper

LGTM, just a few nits

Co-authored-by: Hayden B <[email protected]> Signed-off-by: Meredith Lancaster <[email protected]>

Signed-off-by: Meredith Lancaster <[email protected]>

…thority into verify-opts-struct

haydentherapper · 2022-12-06T23:25:21Z

@malancas what do you think if we first merged this PR, rather than wait for the other? Then we'll get the pkcs7 changes merged, update the timestamp repo fork too, and then update verification to pass the intermediates/roots? We won't cut a new release til we get the CA cert verification in

Signed-off-by: Meredith Lancaster <[email protected]>

malancas · 2022-12-07T02:30:47Z

@haydentherapper that sounds good. I just want to call out the skipped test TestVerifyPassLeafCertificate. This test is currently failing because verifySignature expects that the TSR contain certificates. This function gets called by the PKCS7#VerifyWithChain method in the VerifyTSRWithChain function.

We can change the verifySignature function upstream along with the other changes discussed or I can change the code here and make sure the test passes. Let me know what you think.

haydentherapper · 2022-12-07T22:35:38Z

@malancas Hm, yea, that's unfortunate. What do you think about updating the verifier to reject timestamps that do not include a certificate? We can leave all the other code in place, just add a TODO to remove the rejection once we update the pkcs7 library.

haydentherapper · 2022-12-07T22:37:29Z

Another idea, can you populate p7.Certificates without affecting the signature verification? you might be able to, if nothing is recomputed that would mess up signature verification

haydentherapper · 2022-12-07T22:39:18Z

Looking over the code, the signature is verified against p7.Content. I think signer.IssuerAndSerialNumber is unconditionally set, so then if you just populate p7.Certificates = []*x509.Certificate{leafCert}, I think it might work?

malancas · 2022-12-08T00:24:16Z

@haydentherapper populating p7.Certificates = []*x509.Certificate{leafCert} seems to work. I'll push a change with that.

Signed-off-by: Meredith Lancaster <[email protected]>

haydentherapper

Awesome!!

malancas added 8 commits November 8, 2022 09:30

add verification opts struct

fcfceba

Signed-off-by: Meredith Lancaster <[email protected]>

add optional nonce arg

f0c2d4b

Signed-off-by: Meredith Lancaster <[email protected]>

Merge branch 'main' into verify-opts-struct

6905a33

add oid flag

857d175

Signed-off-by: Meredith Lancaster <[email protected]>

start adding more verification functions

fa279a2

Signed-off-by: Meredith Lancaster <[email protected]>

add verification calls

3025cbf

Signed-off-by: Meredith Lancaster <[email protected]>

add more verification functions and tests

ccb8a18

Signed-off-by: Meredith Lancaster <[email protected]>

fix linter errors

35515ca

Signed-off-by: Meredith Lancaster <[email protected]>

hectorj2f reviewed Nov 17, 2022

View reviewed changes

malancas added 3 commits November 17, 2022 09:37

more linter fixes

13989e8

Signed-off-by: Meredith Lancaster <[email protected]>

update testing

567a92d

Signed-off-by: Meredith Lancaster <[email protected]>

handle optional flags, reorganize code

ccd6671

Signed-off-by: Meredith Lancaster <[email protected]>

malancas commented Nov 17, 2022

View reviewed changes

centralize verification function calls

b7c9c9f

Signed-off-by: Meredith Lancaster <[email protected]>

haydentherapper reviewed Nov 18, 2022

View reviewed changes

malancas added 8 commits November 21, 2022 11:35

move flag parsing to cli app package

31e870a

Signed-off-by: Meredith Lancaster <[email protected]>

move newVerifyOpts

008d1da

Signed-off-by: Meredith Lancaster <[email protected]>

remove unused function

8b296db

Signed-off-by: Meredith Lancaster <[email protected]>

add more error messaging

a0de29c

Signed-off-by: Meredith Lancaster <[email protected]>

return correct number of values

39f901b

Signed-off-by: Meredith Lancaster <[email protected]>

typo

7e61435

Signed-off-by: Meredith Lancaster <[email protected]>

run formatter

b6002a2

Signed-off-by: Meredith Lancaster <[email protected]>

update test data

2faf315

Signed-off-by: Meredith Lancaster <[email protected]>

malancas marked this pull request as ready for review November 21, 2022 20:11

malancas requested a review from a team as a code owner November 21, 2022 20:11

udpate issuer comparison

6096026

Signed-off-by: Meredith Lancaster <[email protected]>

hectorj2f reviewed Nov 25, 2022

View reviewed changes

cmd/timestamp-cli/app/verify.go Outdated Show resolved Hide resolved

hectorj2f reviewed Nov 25, 2022

View reviewed changes

cmd/timestamp-cli/app/verify.go Outdated Show resolved Hide resolved

hectorj2f reviewed Nov 25, 2022

View reviewed changes

malancas added 6 commits December 5, 2022 13:15

reoganized some functions

4bff132

Signed-off-by: Meredith Lancaster <[email protected]>

add leaf verification tests

807cbc8

Signed-off-by: Meredith Lancaster <[email protected]>

formatting

93b9e20

Signed-off-by: Meredith Lancaster <[email protected]>

add nonce and policy oid flags to verification test

7c04f4b

Signed-off-by: Meredith Lancaster <[email protected]>

add common name arg to verification test

0ed6163

Signed-off-by: Meredith Lancaster <[email protected]>

update verify test to include more flags. add verify test with certif…

c6dc949

…icate flag Signed-off-by: Meredith Lancaster <[email protected]>

haydentherapper reviewed Dec 6, 2022

View reviewed changes

pkg/verification/verify.go Outdated Show resolved Hide resolved

haydentherapper reviewed Dec 6, 2022

View reviewed changes

pkg/verification/verify.go Outdated Show resolved Hide resolved

haydentherapper reviewed Dec 6, 2022

View reviewed changes

pkg/verification/verify.go Outdated Show resolved Hide resolved

haydentherapper reviewed Dec 6, 2022

View reviewed changes

malancas and others added 7 commits December 6, 2022 16:07

Update pkg/verification/verify.go

9f08373

Co-authored-by: Hayden B <[email protected]> Signed-off-by: Meredith Lancaster <[email protected]>

Update pkg/verification/verify.go

137d49b

Co-authored-by: Hayden B <[email protected]> Signed-off-by: Meredith Lancaster <[email protected]>

Update pkg/verification/verify.go

1bc74b8

Co-authored-by: Hayden B <[email protected]> Signed-off-by: Meredith Lancaster <[email protected]>

clean up field if statements

215c8eb

Signed-off-by: Meredith Lancaster <[email protected]>

add temporary skip statement for test

47660e6

Signed-off-by: Meredith Lancaster <[email protected]>

formatting

c93a3ed

Signed-off-by: Meredith Lancaster <[email protected]>

Merge branch 'verify-opts-struct' of github.com:malancas/timestamp-au…

131993e

…thority into verify-opts-struct

update code for linter

0bbd9d5

Signed-off-by: Meredith Lancaster <[email protected]>

malancas added 2 commits December 7, 2022 17:26

update verification to add the leaf cert to the pkcs7 object

fc33633

Signed-off-by: Meredith Lancaster <[email protected]>

fix linter error

7545817

Signed-off-by: Meredith Lancaster <[email protected]>

haydentherapper approved these changes Dec 8, 2022

View reviewed changes

haydentherapper merged commit c9b8a1a into sigstore:main Dec 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Start adding more verification with VerificationOpts struct #153

Start adding more verification with VerificationOpts struct #153

malancas commented Nov 17, 2022 •

edited

Loading

hectorj2f Nov 17, 2022

malancas Nov 17, 2022 •

edited

Loading

haydentherapper Nov 17, 2022

malancas Nov 17, 2022

haydentherapper Nov 18, 2022

malancas Nov 21, 2022

hectorj2f Nov 25, 2022

hectorj2f Nov 25, 2022

malancas Nov 28, 2022

hectorj2f Nov 25, 2022

malancas Dec 1, 2022 •

edited

Loading

malancas commented Dec 6, 2022

haydentherapper Dec 6, 2022

malancas Dec 6, 2022 •

edited

Loading

haydentherapper left a comment

haydentherapper commented Dec 6, 2022

malancas commented Dec 7, 2022 •

edited

Loading

haydentherapper commented Dec 7, 2022

haydentherapper commented Dec 7, 2022

haydentherapper commented Dec 7, 2022

malancas commented Dec 8, 2022

haydentherapper left a comment

Start adding more verification with VerificationOpts struct #153

Start adding more verification with VerificationOpts struct #153

Conversation

malancas commented Nov 17, 2022 • edited Loading

Summary

Release Note

Documentation

Choose a reason for hiding this comment

malancas Nov 17, 2022 • edited Loading

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

malancas Dec 1, 2022 • edited Loading

Choose a reason for hiding this comment

malancas commented Dec 6, 2022

Choose a reason for hiding this comment

malancas Dec 6, 2022 • edited Loading

Choose a reason for hiding this comment

haydentherapper left a comment

Choose a reason for hiding this comment

haydentherapper commented Dec 6, 2022

malancas commented Dec 7, 2022 • edited Loading

haydentherapper commented Dec 7, 2022

haydentherapper commented Dec 7, 2022

haydentherapper commented Dec 7, 2022

malancas commented Dec 8, 2022

haydentherapper left a comment

Choose a reason for hiding this comment

malancas commented Nov 17, 2022 •

edited

Loading

malancas Nov 17, 2022 •

edited

Loading

malancas Dec 1, 2022 •

edited

Loading

malancas Dec 6, 2022 •

edited

Loading

malancas commented Dec 7, 2022 •

edited

Loading