review comments

Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>

Author: matthyx

pkg/objectcache/applicationprofilecache/applicationprofilecache.go

@@ -247,7 +247,7 @@ func (apc *ApplicationProfileCacheImpl) updateAllProfiles(ctx context.Context) {
 			}
 
 			// Verify signature if enabled
-			if err := apc.verifyApplicationProfile(fullProfile, workloadID, "profile"); err != nil {
+			if err := apc.verifyApplicationProfile(fullProfile, workloadID, "profile", true); err != nil {
 				// Continue to next profile as per requirements: skip on verification failure
 				continue
 			}
@@ -276,7 +276,7 @@ func (apc *ApplicationProfileCacheImpl) updateAllProfiles(ctx context.Context) {
 // verifyApplicationProfile verifies the profile signature if verification is enabled.
 // Returns error if verification fails, nil otherwise (including when verification is disabled).
 // Also updates profileState with error details if verification fails.
-func (apc *ApplicationProfileCacheImpl) verifyApplicationProfile(profile *v1beta1.ApplicationProfile, workloadID, context string) error {
+func (apc *ApplicationProfileCacheImpl) verifyApplicationProfile(profile *v1beta1.ApplicationProfile, workloadID, context string, recordFailure bool) error {
 	if !apc.cfg.EnableSignatureVerification {
 		return nil
 	}
@@ -298,7 +298,9 @@ func (apc *ApplicationProfileCacheImpl) verifyApplicationProfile(profile *v1beta
 		}
 
 		// Update profile state with verification error
-		apc.setVerificationFailed(workloadID, profile.Name, err)
+		if recordFailure {
+			apc.setVerificationFailed(workloadID, profile.Name, err)
+		}
 
 		return err
 	}
@@ -368,7 +370,7 @@ func (apc *ApplicationProfileCacheImpl) handleUserManagedProfile(profile *v1beta
 	}
 
 	// Verify signature if enabled
-	if err := apc.verifyApplicationProfile(fullUserProfile, toMerge.wlid, "user-managed profile"); err != nil {
+	if err := apc.verifyApplicationProfile(fullUserProfile, toMerge.wlid, "user-managed profile", false); err != nil {
 		return
 	}
 
@@ -593,18 +595,13 @@ func (apc *ApplicationProfileCacheImpl) addContainer(container *containercollect
 				}
 
 				// Verify signature if enabled
-				if err := apc.verifyApplicationProfile(fullProfile, workloadID, "user-defined profile"); err != nil {
-					logger.L().Warning("user-defined profile signature verification failed, continuing without signature",
-						helpers.String("containerID", containerID),
-						helpers.String("workloadID", workloadID),
-						helpers.String("namespace", container.K8s.Namespace),
-						helpers.String("profileName", userDefinedProfile),
-						helpers.Error(err))
+				if err := apc.verifyApplicationProfile(fullProfile, workloadID, "user-defined profile", false); err != nil {
 					// Update the profile state to indicate an error
 					profileState := &objectcache.ProfileState{
 						Error: fmt.Errorf("signature verification failed: %w", err),
 					}
 					apc.workloadIDToProfileState.Set(workloadID, profileState)
+					return nil
 				}
 
 				// Update the profile in the cache

pkg/objectcache/networkneighborhoodcache/networkneighborhoodcache.go

@@ -342,6 +342,8 @@ func (nnc *NetworkNeighborhoodCacheImpl) handleUserManagedNetworkNeighborhood(nn
 				Error:      fmt.Errorf("signature verification failed: %w", err),
 			}
 			nnc.workloadIDToProfileState.Set(toMerge.wlid, profileState)
+			// Evict stale merged profile from cache on verification failure
+			nnc.workloadIDToNetworkNeighborhood.Delete(toMerge.wlid)
 			return
 		}
 	}
@@ -362,13 +364,23 @@ func (nnc *NetworkNeighborhoodCacheImpl) handleUserManagedNetworkNeighborhood(nn
 				Error:      fmt.Errorf("signature verification failed: %w", err),
 			}
 			nnc.workloadIDToProfileState.Set(toMerge.wlid, profileState)
+			// Restore cache to originalNN on user-managed verification failure
+			nnc.workloadIDToNetworkNeighborhood.Set(toMerge.wlid, originalNN)
 			return
 		}
 	}
 
 	// Merge the network neighborhoods
 	mergedNN := nnc.performMerge(originalNN, fullUserNN)
 
+	// Clear stale signature annotations after merge
+	delete(mergedNN.Annotations, signature.AnnotationSignature)
+	delete(mergedNN.Annotations, signature.AnnotationCertificate)
+	delete(mergedNN.Annotations, signature.AnnotationRekorBundle)
+	delete(mergedNN.Annotations, signature.AnnotationIssuer)
+	delete(mergedNN.Annotations, signature.AnnotationIdentity)
+	delete(mergedNN.Annotations, signature.AnnotationTimestamp)
+
 	// Update the cache with the merged network neighborhood
 	nnc.workloadIDToNetworkNeighborhood.Set(toMerge.wlid, mergedNN)
 	// Update profile state for the merged profile

pkg/signature/cluster_flow_test.go

@@ -8,7 +8,6 @@ import (
 	"crypto/rand"
 	"crypto/x509"
 	"crypto/x509/pkix"
-	"encoding/base64"
 	"encoding/pem"
 	"math/big"
 	"testing"
@@ -101,48 +100,22 @@ func TestReproduceClusterVerificationFlow(t *testing.T) {
 		t.Fatalf("Failed to generate test certificate: %v", err)
 	}
 
-	// Add signature annotations
-	adapter.SetAnnotations(map[string]string{
-		"signature.kubescape.io/signature":   base64.StdEncoding.EncodeToString(sig),
-		"signature.kubescape.io/certificate": base64.StdEncoding.EncodeToString(certBytes),
-	})
-
-	// Now verify using the higher-level flow via CosignVerifier
-	// Create verifier and decode signature from annotations
-	sigObj, err := cosignAdapter.DecodeSignatureFromAnnotations(adapter.GetAnnotations())
-	if err != nil {
-		t.Fatalf("Failed to decode signature from annotations: %v", err)
-	}
-
-	// Parse certificate from decoded signature
-	block, _ := pem.Decode(sigObj.Certificate)
-	if block == nil {
-		t.Fatalf("Failed to decode PEM from certificate data: certificate data is %d bytes", len(sigObj.Certificate))
-	}
-	if block.Type != "CERTIFICATE" {
-		t.Fatalf("Wrong PEM block type: got %q, want CERTIFICATE", block.Type)
+	// Use the package-level annotation flow
+	sigObj := &Signature{
+		Signature:   sig,
+		Certificate: certBytes,
+		Timestamp:   time.Now().Unix(),
 	}
-
-	cert, err := x509.ParseCertificate(block.Bytes)
-	if err != nil {
-		t.Fatalf("Failed to parse certificate: %v", err)
-	}
-
-	// Verify using the certificate's public key (exercise the real verification path)
-	verifier, err := sigstore_signature.LoadVerifier(cert.PublicKey, crypto.SHA256)
+	annotations, err := cosignAdapter.EncodeSignatureToAnnotations(sigObj)
 	if err != nil {
-		t.Fatalf("Failed to load verifier from certificate: %v", err)
+		t.Fatalf("Failed to encode signature to annotations: %v", err)
 	}
+	adapter.SetAnnotations(annotations)
 
-	// Verify signature against hash string
-	err = verifier.VerifySignature(bytes.NewReader(sig), bytes.NewReader([]byte(hash)))
+	// Now verify using the higher-level flow
+	err = VerifyObjectAllowUntrusted(adapter)
 	if err != nil {
-		t.Fatalf("Verification failed: %v", err)
-	}
-
-	// Clean up: verify the signature is correctly stored and retrieved
-	if sigObj.Signature == nil {
-		t.Error("Signature was not properly decoded from annotations")
+		t.Fatalf("VerifyObjectAllowUntrusted failed: %v", err)
 	}
 }
 

pkg/signature/cosign_adapter.go

@@ -131,7 +131,9 @@ func (c *CosignAdapter) signKeyless(data []byte) (*Signature, error) {
 		if err != nil {
 			return nil, fmt.Errorf("failed to provide OIDC token: %w", err)
 		}
-		// In CI, identity/issuer are usually provided by the environment
+		// In a production environment, we should parse the JWT to extract "sub" and "iss"
+		// For now, we continue to use the constants as we lack a JWT parser dependency
+		// or direct access to the claims from the provider.
 		identity = sigstoreOIDC
 		issuer = sigstoreIssuer
 	} else {
@@ -324,6 +326,9 @@ func (c *CosignAdapter) GetPublicKeyPEM() ([]byte, error) {
 }
 
 func (c *CosignAdapter) VerifyData(data []byte, sig *Signature, allowUntrusted bool) error {
+	if sig == nil {
+		return fmt.Errorf("signature object is nil")
+	}
 	var verifier sigstore_signature.Verifier
 	var err error
 
@@ -332,68 +337,71 @@ func (c *CosignAdapter) VerifyData(data []byte, sig *Signature, allowUntrusted b
 	// For now, we continue to support the simplified verification but using sigstore's abstractions.
 
 	block, _ := pem.Decode(sig.Certificate)
-	if block != nil && (block.Type == "CERTIFICATE" || block.Type == "PUBLIC KEY") {
-		if block.Type == "CERTIFICATE" {
-			var cert *x509.Certificate
-			cert, err = x509.ParseCertificate(block.Bytes)
-			if err != nil {
-				return fmt.Errorf("failed to parse certificate: %w", err)
+	if block != nil && block.Type == "CERTIFICATE" {
+		var cert *x509.Certificate
+		cert, err = x509.ParseCertificate(block.Bytes)
+		if err != nil {
+			return fmt.Errorf("failed to parse certificate: %w", err)
+		}
+
+		if !allowUntrusted {
+			if cert.IsCA {
+				return fmt.Errorf("invalid certificate: must not be CA")
 			}
 
-			if !allowUntrusted {
-				if cert.IsCA {
-					return fmt.Errorf("invalid certificate: must not be CA")
-				}
+			if time.Unix(sig.Timestamp, 0).Before(cert.NotBefore) || time.Unix(sig.Timestamp, 0).After(cert.NotAfter) {
+				return fmt.Errorf("certificate was not valid at signing time")
+			}
 
-				if time.Now().Before(cert.NotBefore) || time.Now().After(cert.NotAfter) {
-					return fmt.Errorf("certificate is not valid at this time")
+			// In a production environment, we would verify the certificate chain here
+			// against the Fulcio root set and system roots.
+			// roots, _ := fulcioroots.Get()
+			// cert.Verify(x509.VerifyOptions{Roots: roots})
+
+			// Check identity. Fulcio certs store identity in Subject Alternative Name (SAN)
+			// but many systems still look at CommonName or use specific extensions.
+			// Sigstore's verify library is usually used for this, but for now we'll check SANs.
+			foundIdentity := false
+			if cert.Subject.CommonName == sig.Identity {
+				foundIdentity = true
+			} else {
+				for _, email := range cert.EmailAddresses {
+					if email == sig.Identity {
+						foundIdentity = true
+						break
+					}
 				}
-
-				// Check identity. Fulcio certs store identity in Subject Alternative Name (SAN)
-				// but many systems still look at CommonName or use specific extensions.
-				// Sigstore's verify library is usually used for this, but for now we'll check SANs.
-				foundIdentity := false
-				if cert.Subject.CommonName == sig.Identity {
-					foundIdentity = true
-				} else {
-					for _, email := range cert.EmailAddresses {
-						if email == sig.Identity {
+				if !foundIdentity {
+					for _, uri := range cert.URIs {
+						if uri.String() == sig.Identity {
 							foundIdentity = true
 							break
 						}
 					}
-					if !foundIdentity {
-						for _, uri := range cert.URIs {
-							if uri.String() == sig.Identity {
-								foundIdentity = true
-								break
-							}
-						}
-					}
-				}
-
-				if sig.Identity != "" && !foundIdentity {
-					return fmt.Errorf("identity mismatch: certificate does not match signature identity %q (CN: %q, SANs: %v)", sig.Identity, cert.Subject.CommonName, cert.EmailAddresses)
 				}
 			}
-			verifier, err = sigstore_signature.LoadVerifier(cert.PublicKey, crypto.SHA256)
-			if err != nil {
-				return fmt.Errorf("failed to load verifier from certificate: %w", err)
-			}
-		} else {
-			// PUBLIC KEY block
-			pubKey, err := cryptoutils.UnmarshalPEMToPublicKey(sig.Certificate)
-			if err != nil {
-				return fmt.Errorf("failed to parse public key: %w", err)
+
+			if sig.Identity != "" && !foundIdentity {
+				return fmt.Errorf("identity mismatch: certificate does not match signature identity %q (CN: %q, SANs: %v)", sig.Identity, cert.Subject.CommonName, cert.EmailAddresses)
 			}
-			verifier, err = sigstore_signature.LoadVerifier(pubKey, crypto.SHA256)
-			if err != nil {
-				return fmt.Errorf("failed to load verifier from public key: %w", err)
+
+			// Validate Rekor/CT evidence if Rekor bundle is present
+			if len(sig.RekorBundle) > 0 {
+				// In a full implementation, we would use cosign.VerifyBundle
+				// for now we acknowledge its presence for strict verification
+			} else if sig.Issuer != "local" && sig.Issuer != "" {
+				// For non-local certificates, we expect a Rekor bundle in strict mode
+				return fmt.Errorf("strict verification failed: missing Rekor bundle for certificate from %q", sig.Issuer)
 			}
 		}
+		verifier, err = sigstore_signature.LoadVerifier(cert.PublicKey, crypto.SHA256)
+		if err != nil {
+			return fmt.Errorf("failed to load verifier from certificate: %w", err)
+		}
 	} else {
+		// If not a certificate, it must be a public key
 		if !allowUntrusted {
-			return fmt.Errorf("untrusted certificate rejected: require valid x509 certificate chain")
+			return fmt.Errorf("untrusted public key rejected: require valid x509 certificate chain")
 		}
 
 		pubKey, err := cryptoutils.UnmarshalPEMToPublicKey(sig.Certificate)

pkg/signature/profiles/networkneighborhood_adapter_test.go

@@ -52,7 +52,7 @@ func TestNetworkNeighborhoodAdapter(t *testing.T) {
 	content := adapter.GetContent().(map[string]interface{})
 	assert.Equal(t, "NetworkNeighborhood", content["kind"])
 	assert.Equal(t, "softwarecomposition.kubescape.io/v1beta1", content["apiVersion"])
-	
+
 	metadata := content["metadata"].(map[string]interface{})
 	assert.Equal(t, "test-nn", metadata["name"])
 	assert.Equal(t, "test-ns", metadata["namespace"])
@@ -63,3 +63,37 @@ func TestNetworkNeighborhoodAdapter(t *testing.T) {
 
 	assert.Equal(t, nn, adapter.GetUpdatedObject())
 }
+
+func TestNetworkNeighborhoodAdapter_EmptyTypeMeta(t *testing.T) {
+	nn := &v1beta1.NetworkNeighborhood{
+		TypeMeta: metav1.TypeMeta{
+			Kind:       "",
+			APIVersion: "",
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-nn",
+			Namespace: "test-ns",
+		},
+		Spec: v1beta1.NetworkNeighborhoodSpec{
+			Containers: []v1beta1.NetworkNeighborhoodContainer{
+				{
+					Name: "test-container",
+				},
+			},
+		},
+	}
+
+	adapter := NewNetworkNeighborhoodAdapter(nn)
+	content := adapter.GetContent().(map[string]interface{})
+
+	assert.Equal(t, "NetworkNeighborhood", content["kind"])
+	assert.Equal(t, "softwarecomposition.kubescape.io/v1beta1", content["apiVersion"])
+
+	metadata := content["metadata"].(map[string]interface{})
+	assert.Equal(t, "test-nn", metadata["name"])
+	assert.Equal(t, "test-ns", metadata["namespace"])
+
+	spec := content["spec"].(v1beta1.NetworkNeighborhoodSpec)
+	assert.Equal(t, 1, len(spec.Containers))
+	assert.Equal(t, "test-container", spec.Containers[0].Name)
+}

pkg/signature/verify.go

@@ -21,11 +21,11 @@ func VerifyObject(obj SignableObject, opts ...VerifyOption) error {
 
 	annotations := obj.GetAnnotations()
 	if annotations == nil {
-		return fmt.Errorf("object has no annotations")
+		return fmt.Errorf("%w (missing %s annotation)", ErrObjectNotSigned, AnnotationSignature)
 	}
 
 	if _, ok := annotations[AnnotationSignature]; !ok {
-		return ErrObjectNotSigned
+		return fmt.Errorf("%w (missing %s annotation)", ErrObjectNotSigned, AnnotationSignature)
 	}
 
 	// useKeyless=true is fine for verification since we use the certificate