Possible code duplication

[lukehinds]

Was reading over the code and noticed the following:

slsa-verifier/verifiers/internal/gcb/provenance.go

Lines 413 to 430 in 798db79

	func decodeSignature(s string) ([]byte, []error) {
	var errs []error
	rsig, err := base64.StdEncoding.DecodeString(s)
	if err == nil {
	// No error, return the value.
	return rsig, nil
	}
	errs = append(errs, err)
	rsig, err = base64.URLEncoding.DecodeString(s)
	if err == nil {
	// No error, return the value.
	return rsig, nil
	}
	errs = append(errs, err)
	return nil, errs
	}

There maybe some logic in the duplication, if so this is invalid, but I thought it worth raising in case it's a mistake and causes some edge case bug.

[ianlewis]

@lukehinds Did you mean to close this? Even if the code is valid for some reason it's a bit unintuitive so perhaps could use a comment.

[ianlewis]

It looks like where decodeSignature is used is also strange as it returns an []error but that slice isn't checked.

slsa-verifier/verifiers/internal/gcb/provenance.go

Lines 463 to 467 in 798db79

	rsig, es := decodeSignature(sig.Sig)
	if err != nil {
	errs = append(errs, es...)
	continue
	}

[ianlewis]

I'll reopen since this looks strange to me.

/cc @laurentsimon since it looks like this was added in #251

[lukehinds]

@lukehinds Did you mean to close this? Even if the code is valid for some reason it's a bit unintuitive so perhaps could use a comment.

I did, but perhaps it does merit some attention. My eyes did not make out URLEncoding and StdEncoding as different.

[laurentsimon]

Let me explain. We try both encoding because, when verifying GCB provenance, it sometimes failed due to an encoding issue. So I added this "redundant-looking" base64 decoding to avoid the problem. I agree it's worth adding a comment, which I should have done :/

decodeSignature() is called

slsa-verifier/verifiers/internal/gcb/provenance.go

Line 463 in 798db79

rsig, es := decodeSignature(sig.Sig)

and appended to a list of errors

slsa-verifier/verifiers/internal/gcb/provenance.go

Line 465 in 798db79

errs = append(errs, es...)

If the loop (which iterates over all the signatures in the envelope) fails, we return all the errors we encountered

slsa-verifier/verifiers/internal/gcb/provenance.go

Line 487 in 798db79

return fmt.Errorf("%w: %v", serrors.ErrorNoValidSignature, errs)

. The caller itself iterates over all the attestations, so this error is also appended by the caller

slsa-verifier/verifiers/internal/gcb/provenance.go

Line 500 in 798db79

errs = append(errs, err)

which is eventually returned as a single error

slsa-verifier/verifiers/internal/gcb/provenance.go

Line 507 in 798db79

return fmt.Errorf("%w: %v", serrors.ErrorNoValidSignature, errs)

.

The verifySignature() originates from

slsa-verifier/verifiers/internal/gcb/verifier.go

Line 54 in 798db79

if err = prov.VerifySignature(); err != nil {

and it would fail at this line.

[ianlewis]

@lukehinds

I did, but perhaps it does merit some attention. My eyes did not make out URLEncoding and StdEncoding as different.

tbh, you're in good company. I didn't see that either... Yeah, probably a comment would have been good to make it clearer.

@laurentsimon

It looks like where decodeSignature is used is also strange as it returns an []error but that slice isn't checked.

slsa-verifier/verifiers/internal/gcb/provenance.go

Lines 463 to 467 in 798db79

	rsig, es := decodeSignature(sig.Sig)
	if err != nil {
	errs = append(errs, es...)
	continue
	}

I think you want the error checking line to be

if len(es) != 0 {

perhaps?