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

[Cosigned] Convert functions for webhookCIP from v1alpha1 #1736

Merged
merged 1 commit into from
Apr 12, 2022
Merged

[Cosigned] Convert functions for webhookCIP from v1alpha1 #1736

Show file tree
Hide file tree
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
1 change: 0 additions & 1 deletion pkg/apis/config/image_policies_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
Expand Down
72 changes: 65 additions & 7 deletions pkg/cosign/kubernetes/webhook/clusterimagepolicy/clusterimagepolicy_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 •
edited
Loading

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 •
edited
Loading

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
Expand All @@ -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 {
Expand All @@ -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
}
Expand All @@ -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)
Expand Down
6 changes: 3 additions & 3 deletions pkg/cosign/kubernetes/webhook/validator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ UoJou2P8sbDxpLiE/v3yLw1/jyOrCPWYHWFXnyyeGlkgSVefG54tNoK7Uw==
}},
Authorities: []webhookcip.Authority{
{
Keyless: &v1alpha1.KeylessRef{
Keyless: &webhookcip.KeylessRef{
URL: badURL,
},
},
Expand Down Expand Up @@ -315,7 +315,7 @@ UoJou2P8sbDxpLiE/v3yLw1/jyOrCPWYHWFXnyyeGlkgSVefG54tNoK7Uw==
}},
Authorities: []webhookcip.Authority{
{
Keyless: &v1alpha1.KeylessRef{
Keyless: &webhookcip.KeylessRef{
URL: fulcioURL,
},
},
Expand Down Expand Up @@ -358,7 +358,7 @@ UoJou2P8sbDxpLiE/v3yLw1/jyOrCPWYHWFXnyyeGlkgSVefG54tNoK7Uw==
}},
Authorities: []webhookcip.Authority{
{
Keyless: &v1alpha1.KeylessRef{
Keyless: &webhookcip.KeylessRef{
URL: fulcioURL,
},
CTLog: &v1alpha1.TLog{
Expand Down
73 changes: 4 additions & 69 deletions pkg/reconciler/clusterimagepolicy/clusterimagepolicy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,6 @@ package clusterimagepolicy
import (
"context"
"crypto"
"crypto/ecdsa"
"crypto/x509"
"encoding/json"
"encoding/pem"
"fmt"
"strings"

Expand All @@ -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"
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
Expand All @@ -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.
Expand Down
23 changes: 2 additions & 21 deletions pkg/reconciler/clusterimagepolicy/clusterimagepolicy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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-----\"}}}]}"}]`
)
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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{
Expand Down