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.

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

[Cosigned] Convert functions for webhookCIP from v1alpha1 #1736

Merged
merged 1 commit into from
Apr 12, 2022
Merged
Changes from all commits
Commits
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
Create convert functions for internal CIP
Signed-off-by: Denny Hoang <dhoang@vmware.com>
DennyHoang committed Apr 12, 2022

Unverified

This user has not yet uploaded their public signing key.
commit c8ed1114dbf71a17c28d5da73ba897bdeaee3f7f
1 change: 0 additions & 1 deletion pkg/apis/config/image_policies_test.go
Original file line number Diff line number Diff line change
@@ -158,7 +158,6 @@ func checkPublicKey(t *testing.T, gotKey *ecdsa.PublicKey) {

// pem.EncodeToMemory has an extra newline at the end
got := strings.TrimSuffix(string(pemBytes), "\n")

if got != inlineKeyData {
t.Errorf("Did not get what I wanted %s, got %s", inlineKeyData, string(pemBytes))
}
Original file line number Diff line number Diff line change
@@ -21,6 +21,8 @@ import (
"encoding/pem"

"github.com/sigstore/cosign/pkg/apis/cosigned/v1alpha1"

"knative.dev/pkg/apis"
)

// ClusterImagePolicy defines the images that go through verification
@@ -37,7 +39,7 @@ type Authority struct {
// +optional
Key *KeyRef `json:"key,omitempty"`
// +optional
Keyless *v1alpha1.KeylessRef `json:"keyless,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: I am considering whether we should create again all the resources again. As a consequence, we should create the type for ImagePattern, Sources and Identities here to be consistent.

Copy link
Contributor

Choose a reason for hiding this comment

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

@vaikas wdyt?

Copy link
Contributor

@vaikas vaikas Apr 12, 2022

Choose a reason for hiding this comment

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

Consistency is good indeed :) So, I'd be down for recreating them here, or always use one from the CRD. I don't think I have strong feelings on one or the other. If we can import the CRD ones, seems better to only have to update one place.

Copy link
Contributor Author

@DennyHoang DennyHoang Apr 12, 2022

Choose a reason for hiding this comment

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

I kept the other ones as imports from the CRD because we arent currently manipulating it at the moment and it is a direct 1:1 mapping.
We actually manipulate keyRefs so that is why Authorities and key[less]Refs were re-implemented as required.

When the other properties diverge, I feel that updating it then makes more sense to keep potential updates in one place as Ville mentions until that diverging.

If there are any strong feelings for one or the other, it is easy enough to replicate the structs and updating tests.

Keyless *KeylessRef `json:"keyless,omitempty"`
// +optional
Sources []v1alpha1.Source `json:"source,omitempty"`
// +optional
@@ -50,15 +52,21 @@ type KeyRef struct {
// Data contains the inline public key
// +optional
Data string `json:"data,omitempty"`
// KMS contains the KMS url of the public key
// +optional
KMS string `json:"kms,omitempty"`
// PublicKeys are not marshalled because JSON unmarshalling
// errors for *big.Int
// +optional
PublicKeys []*ecdsa.PublicKey `json:"-"`
}

type KeylessRef struct {
// +optional
URL *apis.URL `json:"url,omitempty"`
// +optional
Identities []v1alpha1.Identity `json:"identities,omitempty"`
// +optional
CACert *KeyRef `json:"ca-cert,omitempty"`
}

// UnmarshalJSON populates the PublicKeys using Data because
// JSON unmashalling errors for *big.Int
func (k *KeyRef) UnmarshalJSON(data []byte) error {
@@ -73,7 +81,7 @@ func (k *KeyRef) UnmarshalJSON(data []byte) error {
k.Data = ret["data"]

if ret["data"] != "" {
publicKeys, err = convertKeyDataToPublicKeys(ret["data"])
publicKeys, err = ConvertKeyDataToPublicKeys(ret["data"])
if err != nil {
return err
}
@@ -84,9 +92,59 @@ func (k *KeyRef) UnmarshalJSON(data []byte) error {
return nil
}

func convertKeyDataToPublicKeys(pubKey string) ([]*ecdsa.PublicKey, error) {
keys := []*ecdsa.PublicKey{}
func ConvertClusterImagePolicyV1alpha1ToWebhook(in *v1alpha1.ClusterImagePolicy) *ClusterImagePolicy {
copyIn := in.DeepCopy()
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, I think this is not stricly necessary because we don't modify, but since we use the images from L105 as inputs, can't think right now if that will make a difference.


outAuthorities := make([]Authority, 0)
for _, authority := range copyIn.Spec.Authorities {
outAuthority := convertAuthorityV1Alpha1ToWebhook(authority)
outAuthorities = append(outAuthorities, *outAuthority)
}

return &ClusterImagePolicy{
Images: copyIn.Spec.Images,
Authorities: outAuthorities,
}
}

func convertAuthorityV1Alpha1ToWebhook(in v1alpha1.Authority) *Authority {
keyRef := convertKeyRefV1Alpha1ToWebhook(in.Key)
keylessRef := convertKeylessRefV1Alpha1ToWebhook(in.Keyless)

return &Authority{
Key: keyRef,
Keyless: keylessRef,
Sources: in.Sources,
CTLog: in.CTLog,
}
}

func convertKeyRefV1Alpha1ToWebhook(in *v1alpha1.KeyRef) *KeyRef {
if in == nil {
return nil
}

return &KeyRef{
Data: in.Data,
}
}

func convertKeylessRefV1Alpha1ToWebhook(in *v1alpha1.KeylessRef) *KeylessRef {
if in == nil {
return nil
}

CACertRef := convertKeyRefV1Alpha1ToWebhook(in.CACert)

return &KeylessRef{
URL: in.URL,
Identities: in.Identities,
CACert: CACertRef,
}
}

func ConvertKeyDataToPublicKeys(pubKey string) ([]*ecdsa.PublicKey, error) {
keys := []*ecdsa.PublicKey{}
pems := parsePems([]byte(pubKey))
for _, p := range pems {
key, err := x509.ParsePKIXPublicKey(p.Bytes)
6 changes: 3 additions & 3 deletions pkg/cosign/kubernetes/webhook/validator_test.go
Original file line number Diff line number Diff line change
@@ -272,7 +272,7 @@ UoJou2P8sbDxpLiE/v3yLw1/jyOrCPWYHWFXnyyeGlkgSVefG54tNoK7Uw==
}},
Authorities: []webhookcip.Authority{
{
Keyless: &v1alpha1.KeylessRef{
Keyless: &webhookcip.KeylessRef{
URL: badURL,
},
},
@@ -315,7 +315,7 @@ UoJou2P8sbDxpLiE/v3yLw1/jyOrCPWYHWFXnyyeGlkgSVefG54tNoK7Uw==
}},
Authorities: []webhookcip.Authority{
{
Keyless: &v1alpha1.KeylessRef{
Keyless: &webhookcip.KeylessRef{
URL: fulcioURL,
},
},
@@ -358,7 +358,7 @@ UoJou2P8sbDxpLiE/v3yLw1/jyOrCPWYHWFXnyyeGlkgSVefG54tNoK7Uw==
}},
Authorities: []webhookcip.Authority{
{
Keyless: &v1alpha1.KeylessRef{
Keyless: &webhookcip.KeylessRef{
URL: fulcioURL,
},
CTLog: &v1alpha1.TLog{
73 changes: 4 additions & 69 deletions pkg/reconciler/clusterimagepolicy/clusterimagepolicy.go
Original file line number Diff line number Diff line change
@@ -17,10 +17,6 @@ package clusterimagepolicy
import (
"context"
"crypto"
"crypto/ecdsa"
"crypto/x509"
"encoding/json"
"encoding/pem"
"fmt"
"strings"

@@ -30,12 +26,14 @@ import (
clusterimagepolicyreconciler "github.com/sigstore/cosign/pkg/client/injection/reconciler/cosigned/v1alpha1/clusterimagepolicy"
webhookcip "github.com/sigstore/cosign/pkg/cosign/kubernetes/webhook/clusterimagepolicy"
"github.com/sigstore/cosign/pkg/reconciler/clusterimagepolicy/resources"

corev1 "k8s.io/api/core/v1"
apierrs "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
"k8s.io/client-go/kubernetes"
corev1listers "k8s.io/client-go/listers/core/v1"

"knative.dev/pkg/logging"
"knative.dev/pkg/reconciler"
"knative.dev/pkg/system"
@@ -73,31 +71,15 @@ var _ clusterimagepolicyreconciler.Finalizer = (*Reconciler)(nil)
// ReconcileKind implements Interface.ReconcileKind.
func (r *Reconciler) ReconcileKind(ctx context.Context, cip *v1alpha1.ClusterImagePolicy) reconciler.Event {
cipCopy, cipErr := r.inlinePublicKeys(ctx, cip)
if cipErr != nil {
// Handle error here
r.handleCIPError(ctx, cip.Name)
return cipErr
}

// Converting external CIP to webhook CIP
bytes, err := json.Marshal(&cipCopy.Spec)
if err != nil {
return err
}

var webhookCIP *webhookcip.ClusterImagePolicy
if err := json.Unmarshal(bytes, &webhookCIP); err != nil {
return err
}

webhookCIP, cipErr = r.convertKeyData(ctx, webhookCIP)
if cipErr != nil {
r.handleCIPError(ctx, cip.Name)
// Note that we return the error about the Invalid cip here to make
// sure that it's surfaced.
return cipErr
}

webhookCIP := webhookcip.ConvertClusterImagePolicyV1alpha1ToWebhook(cipCopy)

// See if the CM holding configs exists
existing, err := r.configmaplister.ConfigMaps(system.Namespace()).Get(config.ImagePoliciesConfigName)
if err != nil {
@@ -149,24 +131,6 @@ func (r *Reconciler) FinalizeKind(ctx context.Context, cip *v1alpha1.ClusterImag
return r.removeCIPEntry(ctx, existing, cip.Name)
}

// convertKeyData will go through the CIP and try to convert key data
// to ecdsa.PublicKey and store it in the returned CIP
// When PublicKeys are successfully set, the authority key's data will be
// cleared out
func (r *Reconciler) convertKeyData(ctx context.Context, cip *webhookcip.ClusterImagePolicy) (*webhookcip.ClusterImagePolicy, error) {
for _, authority := range cip.Authorities {
if authority.Key != nil && authority.Key.Data != "" {
keys, err := convertAuthorityKeys(ctx, authority.Key.Data)
if err != nil {
return nil, err
}
// When publicKeys are successfully converted, clear out Data
authority.Key.PublicKeys = keys
}
}
return cip, nil
}

func (r *Reconciler) handleCIPError(ctx context.Context, cipName string) {
// The CIP is invalid, try to remove CIP from the configmap
existing, err := r.configmaplister.ConfigMaps(system.Namespace()).Get(config.ImagePoliciesConfigName)
@@ -179,35 +143,6 @@ func (r *Reconciler) handleCIPError(ctx context.Context, cipName string) {
}
}

func convertAuthorityKeys(ctx context.Context, pubKey string) ([]*ecdsa.PublicKey, error) {
keys := []*ecdsa.PublicKey{}

logging.FromContext(ctx).Debugf("Got public key: %v", pubKey)

pems := parsePems([]byte(pubKey))
for _, p := range pems {
key, err := x509.ParsePKIXPublicKey(p.Bytes)
if err != nil {
return nil, err
}
keys = append(keys, key.(*ecdsa.PublicKey))
}
return keys, nil
}

func parsePems(b []byte) []*pem.Block {
p, rest := pem.Decode(b)
if p == nil {
return nil
}
pems := []*pem.Block{p}

if rest != nil {
return append(pems, parsePems(rest)...)
}
return pems
}

// inlinePublicKeys will go through the CIP and try to read the referenced
// secrets, KMS keys and convert them into inlined data. Makes a copy of the CIP
// before modifying it and returns the copy.
23 changes: 2 additions & 21 deletions pkg/reconciler/clusterimagepolicy/clusterimagepolicy_test.go
Original file line number Diff line number Diff line change
@@ -81,9 +81,6 @@ RCTfQ5s1kD+hGMSE1rH7s46hmXEeyhnlRnaGF8eMU/SBJE/2NKPnxE7WzQ==
// two entries but only one is being removed. For keyless entry.
removeSingleEntryKeylessPatch = `[{"op":"remove","path":"/data/test-cip-2"}]`

// This is the patch for inlined secret for key ref data
inlinedSecretKeyPatch = `[{"op":"replace","path":"/data/test-cip","value":"{\"images\":[{\"glob\":\"ghcr.io/example/*\"}],\"authorities\":[{\"key\":{\"data\":\"-----BEGIN PUBLIC KEY-----\\nMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAExB6+H6054/W1SJgs5JR6AJr6J35J\\nRCTfQ5s1kD+hGMSE1rH7s46hmXEeyhnlRnaGF8eMU/SBJE/2NKPnxE7WzQ==\\n-----END PUBLIC KEY-----\"}}]}"}]`

// This is the patch for inlined secret for keyless cakey ref data
inlinedSecretKeylessPatch = `[{"op":"replace","path":"/data/test-cip-2","value":"{\"images\":[{\"glob\":\"ghcr.io/example/*\"}],\"authorities\":[{\"keyless\":{\"ca-cert\":{\"data\":\"-----BEGIN PUBLIC KEY-----\\nMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAExB6+H6054/W1SJgs5JR6AJr6J35J\\nRCTfQ5s1kD+hGMSE1rH7s46hmXEeyhnlRnaGF8eMU/SBJE/2NKPnxE7WzQ==\\n-----END PUBLIC KEY-----\"}}}]}"}]`
)
@@ -475,11 +472,10 @@ func TestReconcile(t *testing.T) {
},
}}),
),
makeConfigMapWithTwoEntriesNotPublicKeyFromSecret(),
makeSecret(keySecretName, validPublicKeyData),
},
WantPatches: []clientgotesting.PatchActionImpl{
makePatch(inlinedSecretKeyPatch),
WantCreates: []runtime.Object{
makeConfigMap(),
},
PostConditions: []func(*testing.T, *TableRow){
AssertTrackingSecret(system.Namespace(), keySecretName),
@@ -629,21 +625,6 @@ func makeConfigMapWithTwoEntries() *corev1.ConfigMap {
}
}

// Same as MakeConfigMapWithTwoEntries but the inline data is not the secret
// so we will replace it with the secret data
func makeConfigMapWithTwoEntriesNotPublicKeyFromSecret() *corev1.ConfigMap {
return &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Namespace: system.Namespace(),
Name: config.ImagePoliciesConfigName,
},
Data: map[string]string{
cipName: `{"images":[{"glob":"ghcr.io/example/*"}],"authorities":[{"key":{"data":"NOT A REAL PUBLIC KEY"}}]}`,
cipName2: "remove me please",
},
}
}

func makePatch(patch string) clientgotesting.PatchActionImpl {
return clientgotesting.PatchActionImpl{
ActionImpl: clientgotesting.ActionImpl{