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

Handle type cast errors #850

Merged
merged 1 commit into from
Sep 6, 2023
Merged

Handle type cast errors #850

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
78 changes: 69 additions & 9 deletions pkg/container/container.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"errors"
"fmt"
"log"
"reflect"
"strings"
"time"

Expand Down Expand Up @@ -129,18 +130,55 @@ func extractAndValidateSignature(
// we can add information for the image
signatureVerification.IsVerified = verified
signatureVerification.IsBundleVerified = bundleVerified
signatureVerification.RekorLogId = proto.String(imageKeys["RekorLogID"].(string))
rekorLogID, err := readValueAs[string](imageKeys, "RekorLogID")
if err != nil {
log.Printf("error parsing value from imageKeys: %v", err)
} else {
signatureVerification.RekorLogId = proto.String(rekorLogID)
}

log_index := int32(imageKeys["RekorLogIndex"].(int64))
signatureVerification.RekorLogIndex = &log_index
rekorLogIndex, err := readValueAs[int64](imageKeys, "RekorLogIndex")
if err != nil {
log.Printf("error parsing value from imageKeys: %v", err)
} else {
log_index := int32(rekorLogIndex)
Copy link
Contributor

@jhrozek jhrozek Sep 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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;
}

signatureVerification.RekorLogIndex = &log_index
}

signatureTime, err := readValueAs[int64](imageKeys, "SignatureTime")
if err != nil {
log.Printf("error parsing value from imageKeys: %v", err)
} else {
signatureVerification.SignatureTime = timestamppb.New(time.Unix(signatureTime, 0))
}

workflowName, err := readValueAs[string](imageKeys, "WorkflowName")
if err != nil {
log.Printf("error parsing value from imageKeys: %v", err)
} else {
githubWorkflow.Name = workflowName
}

workflowRepository, err := readValueAs[string](imageKeys, "WorkflowRepository")
if err != nil {
log.Printf("error parsing value from imageKeys: %v", err)
} else {
githubWorkflow.Repository = workflowRepository
}

signature_time := timestamppb.New(time.Unix(imageKeys["SignatureTime"].(int64), 0))
signatureVerification.SignatureTime = signature_time
workflowSha, err := readValueAs[string](imageKeys, "WorkflowSha")
if err != nil {
log.Printf("error parsing value from imageKeys: %v", err)
} else {
githubWorkflow.CommitSha = workflowSha
}

githubWorkflow.Name = imageKeys["WorkflowName"].(string)
githubWorkflow.Repository = imageKeys["WorkflowRepository"].(string)
githubWorkflow.CommitSha = imageKeys["WorkflowSha"].(string)
githubWorkflow.Trigger = imageKeys["WorkflowTrigger"].(string)
workflowTrigger, err := readValueAs[string](imageKeys, "WorkflowTrigger")
if err != nil {
log.Printf("error parsing value from imageKeys: %v", err)
} else {
githubWorkflow.Trigger = workflowTrigger
}
} else {
log.Printf("error verifying image: %v", err)
}
Expand All @@ -149,6 +187,28 @@ func extractAndValidateSignature(
}
}

// readValueAs gets the typed value from the given accessor. Returns an error when the accessor
// doesn't find anything or when the type assertion fails.
func readValueAs[T any](data map[string]any, key string) (T, error) {
var out T

value, ok := data[key]
if !ok {
return out, fmt.Errorf("key %s not found in map", key)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}

if value == nil {
return out, fmt.Errorf("no value for key %s in map", key)
}

out, ok = value.(T)
if !ok {
return out, fmt.Errorf("could not type assert %v (value of key %v) to %v", value, key, reflect.TypeOf(out))
}

return out, nil
}

// ValidateSignature returns information about signature validation of a package
func ValidateSignature(ctx context.Context, accessToken string, package_owner string,
package_url string) (*pb.SignatureVerification, *pb.GithubWorkflow, error) {
Expand Down