Handle type cast errors

[eleftherias]

Closes #817

[jhrozek]

Looking good! I left some comments inline about error handling and printfs, but this is a great first patch

[jhrozek]

Let's not add the printfs since we already are returning an error, let's instead let the caller decide when to print

[jhrozek]

I think it's a good idea to return an error. Just a note for future -in case we reuse this function elsewhere, we might want to consider returning a special error so that the caller could decide to use a default or just treat the error as fatal. But for now, since container.go needs all the values, I think your approach is good.

[jhrozek]

I would reverse this logic. In this case, we want all the fields in the struct, so in case there's an error, I think we should just return the error up and let the caller handle it properly.

[jhrozek]

I think this also means you can assign directory to the structs.

[eleftherias]

In the current implementation, the function extractAndValidateSignature does not return an error up. In cases where there is an error, it only logs the error, for example https://github.com/stacklok/mediator/blob/4b8757adbb8ef9c6e5f60f31255a464368a55fc7/pkg/container/container.go#L143-L145
I'm wondering if this is intentional because the fields of signatureVerification aren't required? This is a similar question to what @rdimitrov wrote below.

[jhrozek]

Sorry, I was unclear. What I'm concerned about is that the function sets signed.true and verified.true quite early in the flow and then if we run into an issue, we end up with half-populated structures. Before this patch we would have just panicked which is not great either, but at least it's a defined state :-)

This function and the way it's called is really ready for refactoring -- I don't this that code should nest happy paths -- but for now, maybe what we could do is to move setting is.signed and is.verified to the inner-most block (where the happy path ends) so that they are set to true only if there are no errors before? An alternative is to just return errors properly all the way up, but that would probably require refactor of the caller, too.

[eleftherias]

Thanks, I understand now. I would favour the approach of returning the errors all the way up and doing the refactor in the caller, rather than postponing it. I'll work on that now.

[jhrozek]

Actually @eleftherias convinced me that returning errors at this point is probably not worth it, the structure has pointers that would be nil in case of an error during parsing and we should probably just log a message and let the policy do its job.

Sorry for the confusion!

[rdimitrov]

Oh, got it 👍

[rdimitrov]

Do we want to handle cases where the casting/value extracting failed, i.e. have mandatory values?

[jhrozek]

Do we want to handle cases where the casting/value extracting failed, i.e. have mandatory values?

Yes, I think so. Did you have anything else in mind than returning an error up?

[jhrozek]

I think this is strictly better than what we had before. I'll point out one thing to follow-up in a later patch.

[jhrozek]

This is IMO something to fix up later. Looking at a quick git grep in rekor, the log index is indeed int64. I don't understand why we down-cast it to int32. I think it would be safer to modify the protobuf structure to use int64 as well. So we'd change:

message SignatureVerification {
    bool is_signed = 1;
    bool is_verified = 2;
    bool is_bundle_verified = 3;
    optional string cert_identity = 4;
    optional string cert_issuer = 5;
    optional string rekor_log_id = 6;
    optional int32 rekor_log_index = 7;
    optional google.protobuf.Timestamp signature_time = 8;
}

to:

message SignatureVerification {
    bool is_signed = 1;
    bool is_verified = 2;
    bool is_bundle_verified = 3;
    optional string cert_identity = 4;
    optional string cert_issuer = 5;
    optional string rekor_log_id = 6;
    optional int64 rekor_log_index = 7;
    optional google.protobuf.Timestamp signature_time = 8;
}