Skip to content

Commit

Permalink
Fix publicKey unmarshal (#1719)
Browse files Browse the repository at this point in the history 
- Add key sign verify for cosigned github workflow

Signed-off-by: Denny Hoang <dhoang@vmware.com>
  • Loading branch information
@DennyHoang
DennyHoang authored Apr 7, 2022
1 parent 460ad52 commit d03404e
Show file tree
Hide file tree
Showing 7 changed files with 183 additions and 23 deletions.
61 changes: 55 additions & 6 deletions .github/workflows/kind-cluster-image-policy.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -120,9 +120,9 @@ jobs:
echo Created image $demoimage2
popd
- name: Deploy ClusterImagePolicy
- name: Deploy ClusterImagePolicy With Keyless Signing
run: |
kubectl apply -f ./test/testdata/cosigned/e2e/cip.yaml
kubectl apply -f ./test/testdata/cosigned/e2e/cip-keyless.yaml
- name: Sign demoimage with cosign
run: |
Expand All @@ -134,12 +134,12 @@ jobs:
- name: Deploy jobs and verify signed works, unsigned fails
run: |
kubectl create namespace demo
kubectl label namespace demo cosigned.sigstore.dev/include=true
kubectl create namespace demo-keyless-signing
kubectl label namespace demo-keyless-signing cosigned.sigstore.dev/include=true
echo '::group:: test job success'
# We signed this above, this should work
if ! kubectl create -n demo job demo --image=${{ env.demoimage }} ; then
if ! kubectl create -n demo-keyless-signing job demo --image=${{ env.demoimage }} ; then
echo Failed to create Job in namespace without label!
exit 1
else
Expand All @@ -149,7 +149,56 @@ jobs:
echo '::group:: test job rejection'
# We did not sign this, should fail
if kubectl create -n demo job demo2 --image=${{ env.demoimage2 }} ; then
if kubectl create -n demo-keyless-signing job demo2 --image=${{ env.demoimage2 }} ; then
echo Failed to block unsigned Job creation!
exit 1
else
echo Successfully blocked Job creation with unsigned image
fi
echo '::endgroup::'
- name: Generate New Signing Key
run: |
COSIGN_PASSWORD="" ./cosign generate-key-pair
- name: Deploy ClusterImagePolicy With Key Signing
run: |
yq '. | .spec.authorities[0].key.data |= load_str("cosign.pub")' ./test/testdata/cosigned/e2e/cip-key.yaml | \
kubectl apply -f -
- name: Verify with two CIP, one not signed with public key
run: |
if kubectl create -n demo-key-signing job demo --image=${{ env.demoimage }}; then
echo Failed to block unsigned Job creation!
exit 1
fi
- name: Sign demoimage with cosign key
run: |
./cosign sign --key cosign.key --force --allow-insecure-registry ${{ env.demoimage }}
- name: Verify with cosign
run: |
./cosign verify --key cosign.pub --allow-insecure-registry ${{ env.demoimage }}
- name: Deploy jobs and verify signed works, unsigned fails
run: |
kubectl create namespace demo-key-signing
kubectl label namespace demo-key-signing cosigned.sigstore.dev/include=true
echo '::group:: test job success'
# We signed this above, this should work
if ! kubectl create -n demo-key-signing job demo --image=${{ env.demoimage }} ; then
echo Failed to create Job in namespace without label!
exit 1
else
echo Succcessfully created Job with signed image
fi
echo '::endgroup:: test job success'
echo '::group:: test job rejection'
# We did not sign this, should fail
if kubectl create -n demo-key-signing job demo2 --image=${{ env.demoimage2 }} ; then
echo Failed to block unsigned Job creation!
exit 1
else
Expand Down
59 changes: 58 additions & 1 deletion internal/pkg/apis/cosigned/clusterimagepolicy_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ package clusterimagepolicy

import (
"crypto/ecdsa"
"crypto/x509"
"encoding/json"
"encoding/pem"

"github.com/sigstore/cosign/pkg/apis/cosigned/v1alpha1"
)
Expand Down Expand Up @@ -50,6 +53,60 @@ type KeyRef struct {
// 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:"publicKeys,omitempty"`
PublicKeys []*ecdsa.PublicKey `json:"-"`
}

// UnmarshalJSON populates the PublicKeys using Data because
// JSON unmashalling errors for *big.Int
func (k *KeyRef) UnmarshalJSON(data []byte) error {
var publicKeys []*ecdsa.PublicKey
var err error

ret := make(map[string]string)
if err = json.Unmarshal(data, &ret); err != nil {
return err
}

k.Data = ret["data"]

if ret["data"] != "" {
publicKeys, err = convertKeyDataToPublicKeys(ret["data"])
if err != nil {
return err
}
}

k.PublicKeys = publicKeys

return nil
}

func convertKeyDataToPublicKeys(pubKey string) ([]*ecdsa.PublicKey, error) {
keys := []*ecdsa.PublicKey{}

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
}
32 changes: 32 additions & 0 deletions pkg/apis/config/image_policies_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@
package config

import (
"crypto/ecdsa"
"crypto/x509"
"encoding/pem"
"strings"
"testing"

internalcip "github.com/sigstore/cosign/internal/pkg/apis/cosigned"
Expand Down Expand Up @@ -81,6 +85,7 @@ func TestGetAuthorities(t *testing.T) {
if got := c[matchedPolicy][0].Key.Data; got != want {
t.Errorf("Did not get what I wanted %q, got %+v", want, got)
}
checkPublicKey(t, c[matchedPolicy][0].Key.PublicKeys[0])

// Test multiline yaml cert
c, err = defaults.GetMatchingPolicies("inlinecert")
Expand All @@ -90,6 +95,8 @@ func TestGetAuthorities(t *testing.T) {
if got := c[matchedPolicy][0].Key.Data; got != want {
t.Errorf("Did not get what I wanted %q, got %+v", want, got)
}
checkPublicKey(t, c[matchedPolicy][0].Key.PublicKeys[0])

// Test multiline cert but json encoded
c, err = defaults.GetMatchingPolicies("ghcr.io/example/*")
checkGetMatches(t, c, err)
Expand All @@ -98,6 +105,8 @@ func TestGetAuthorities(t *testing.T) {
if got := c[matchedPolicy][0].Key.Data; got != want {
t.Errorf("Did not get what I wanted %q, got %+v", want, got)
}
checkPublicKey(t, c[matchedPolicy][0].Key.PublicKeys[0])

// Test multiple matches
c, err = defaults.GetMatchingPolicies("regexstringtoo")
checkGetMatches(t, c, err)
Expand All @@ -109,6 +118,8 @@ func TestGetAuthorities(t *testing.T) {
if got := c[matchedPolicy][0].Key.Data; got != want {
t.Errorf("Did not get what I wanted %q, got %+v", want, got)
}
checkPublicKey(t, c[matchedPolicy][0].Key.PublicKeys[0])

matchedPolicy = "cluster-image-policy-5"
want = "inlinedata here"
if got := c[matchedPolicy][0].Key.Data; got != want {
Expand All @@ -131,3 +142,24 @@ func checkGetMatches(t *testing.T, c map[string][]internalcip.Authority, err err
}
t.Error("Wanted a config and non-zero authorities, got no authorities")
}

func checkPublicKey(t *testing.T, gotKey *ecdsa.PublicKey) {
t.Helper()

derBytes, err := x509.MarshalPKIXPublicKey(gotKey)
if err != nil {
t.Error("Failed to Marshal Key =", err)
}

pemBytes := pem.EncodeToMemory(&pem.Block{
Type: "PUBLIC KEY",
Bytes: derBytes,
})

// 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))
}
}
1 change: 0 additions & 1 deletion pkg/reconciler/clusterimagepolicy/clusterimagepolicy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,6 @@ func (r *Reconciler) convertKeyData(ctx context.Context, cip *internalcip.Cluste
}
// When publicKeys are successfully converted, clear out Data
authority.Key.PublicKeys = keys
authority.Key.Data = ""
}
}
return cip, nil
Expand Down
22 changes: 8 additions & 14 deletions pkg/reconciler/clusterimagepolicy/clusterimagepolicy_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import (
"crypto/ecdsa"
"crypto/elliptic"
"crypto/rand"
"fmt"
"strings"
"testing"

logtesting "knative.dev/pkg/logging/testing"
Expand Down Expand Up @@ -64,10 +64,10 @@ RCTfQ5s1kD+hGMSE1rH7s46hmXEeyhnlRnaGF8eMU/SBJE/2NKPnxE7WzQ==
-----END PUBLIC KEY-----`

// This is the patch for replacing a single entry in the ConfigMap
replaceCIPPatch = `[{"op":"replace","path":"/data/test-cip","value":"{\"images\":[{\"glob\":\"ghcr.io/example/*\"}],\"authorities\":[{\"key\":{\"publicKeys\":[{\"Curve\":{\"P\":115792089210356248762697446949407573530086143415290314195533631308867097853951,\"N\":115792089210356248762697446949407573529996955224135760342422259061068512044369,\"B\":41058363725152142129326129780047268409114441015993725554835256314039467401291,\"Gx\":48439561293906451759052585252797914202762949526041747995844080717082404635286,\"Gy\":36134250956749795798585127919587881956611106672985015071877198253568414405109,\"BitSize\":256,\"Name\":\"P-256\"},\"X\":88707635920070641755721733257804385133633538985728333477702186279848359565508,\"Y\":60084629679813308197062610650536235858799576629081565335301707208097929287373}]}}]}"}]`
replaceCIPPatch = `[{"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 adding an entry for non-existing KMS for cipName2
addCIP2Patch = `[{"op":"add","path":"/data/test-cip-2","value":"{\"images\":[{\"glob\":\"ghcr.io/example/*\"}],\"authorities\":[{\"key\":{}}]}"}]`
addCIP2Patch = `[{"op":"add","path":"/data/test-cip-2","value":"{\"images\":[{\"glob\":\"ghcr.io/example/*\"}],\"authorities\":[{\"key\":{\"data\":\"azure-kms://foo/bar\"}}]}"}]`

// This is the patch for removing the last entry, leaving just the
// configmap objectmeta, no data.
Expand All @@ -82,7 +82,7 @@ RCTfQ5s1kD+hGMSE1rH7s46hmXEeyhnlRnaGF8eMU/SBJE/2NKPnxE7WzQ==
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\":{\"publicKeys\":[{\"Curve\":{\"P\":115792089210356248762697446949407573530086143415290314195533631308867097853951,\"N\":115792089210356248762697446949407573529996955224135760342422259061068512044369,\"B\":41058363725152142129326129780047268409114441015993725554835256314039467401291,\"Gx\":48439561293906451759052585252797914202762949526041747995844080717082404635286,\"Gy\":36134250956749795798585127919587881956611106672985015071877198253568414405109,\"BitSize\":256,\"Name\":\"P-256\"},\"X\":88707635920070641755721733257804385133633538985728333477702186279848359565508,\"Y\":60084629679813308197062610650536235858799576629081565335301707208097929287373}]}}]}"}]`
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 @@ -580,7 +580,7 @@ func makeConfigMap() *corev1.ConfigMap {
Name: config.ImagePoliciesConfigName,
},
Data: map[string]string{
cipName: `{"images":[{"glob":"ghcr.io/example/*"}],"authorities":[{"key":{"publicKeys":[{"Curve":{"P":115792089210356248762697446949407573530086143415290314195533631308867097853951,"N":115792089210356248762697446949407573529996955224135760342422259061068512044369,"B":41058363725152142129326129780047268409114441015993725554835256314039467401291,"Gx":48439561293906451759052585252797914202762949526041747995844080717082404635286,"Gy":36134250956749795798585127919587881956611106672985015071877198253568414405109,"BitSize":256,"Name":"P-256"},"X":88707635920070641755721733257804385133633538985728333477702186279848359565508,"Y":60084629679813308197062610650536235858799576629081565335301707208097929287373}]}}]}`,
cipName: `{"images":[{"glob":"ghcr.io/example/*"}],"authorities":[{"key":{"data":"-----BEGIN PUBLIC KEY-----\nMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAExB6+H6054/W1SJgs5JR6AJr6J35J\nRCTfQ5s1kD+hGMSE1rH7s46hmXEeyhnlRnaGF8eMU/SBJE/2NKPnxE7WzQ==\n-----END PUBLIC KEY-----"}}]}`,
},
}
}
Expand All @@ -591,13 +591,7 @@ func patchKMS(ctx context.Context, t *testing.T, kmsKey string) clientgotesting.
t.Fatalf("Failed to read KMS key ID %q: %v", kmsKey, err)
}

authorityKeys, err := convertAuthorityKeys(ctx, pubKey)
if err != nil || len(authorityKeys) == 0 {
t.Fatalf("Failed to convert KMS public key ID %q: %v", kmsKey, err)
}

pubKeyString := fmt.Sprintf(`{\"Curve\":{\"P\":115792089210356248762697446949407573530086143415290314195533631308867097853951,\"N\":115792089210356248762697446949407573529996955224135760342422259061068512044369,\"B\":41058363725152142129326129780047268409114441015993725554835256314039467401291,\"Gx\":48439561293906451759052585252797914202762949526041747995844080717082404635286,\"Gy\":36134250956749795798585127919587881956611106672985015071877198253568414405109,\"BitSize\":256,\"Name\":\"P-256\"},\"X\":%d,\"Y\":%d}`, authorityKeys[0].X, authorityKeys[0].Y)
patch := `[{"op":"add","path":"/data","value":{"test-kms-cip":"{\"images\":[{\"glob\":\"ghcr.io/example/*\"}],\"authorities\":[{\"key\":{\"publicKeys\":[` + pubKeyString + `]}}]}"}}]`
patch := `[{"op":"add","path":"/data","value":{"test-kms-cip":"{\"images\":[{\"glob\":\"ghcr.io/example/*\"}],\"authorities\":[{\"key\":{\"data\":\"` + strings.ReplaceAll(pubKey, "\n", "\\\\n") + `\"}}]}"}}]`

return clientgotesting.PatchActionImpl{
ActionImpl: clientgotesting.ActionImpl{
Expand All @@ -616,7 +610,7 @@ func makeDifferentConfigMap() *corev1.ConfigMap {
Name: config.ImagePoliciesConfigName,
},
Data: map[string]string{
cipName: `{"images":[{"glob":"ghcr.io/example/*"}],"authorities":[{"key":{"data":"-----BEGIN NOTPUBLIC KEY-----\nMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAExB6+H6054/W1SJgs5JR6AJr6J35J\nRCTfQ5s1kD+hGMSE1rH7s46hmXEeyhnlRnaGF8eMU/SBJE/2NKPnxE7WzQ==\n-----END NOTPUBLIC KEY-----","publicKeys":[{"Curve":{"P":115792089210356248762697446949407573530086143415290314195533631308867097853951,"N":115792089210356248762697446949407573529996955224135760342422259061068512044369,"B":41058363725152142129326129780047268409114441015993725554835256314039467401291,"Gx":48439561293906451759052585252797914202762949526041747995844080717082404635286,"Gy":36134250956749795798585127919587881956611106672985015071877198253568414405109,"BitSize":256,"Name":"P-256"},"X":88707635920070641755721733257804385133633538985728333477702186279848359565508,"Y":60084629679813308197062610650536235858799576629081565335301707208097929287373}]}}]}`,
cipName: `{"images":[{"glob":"ghcr.io/example/*"}],"authorities":[{"key":{"data":"-----BEGIN NOTPUBLIC KEY-----\nMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAExB6+H6054/W1SJgs5JR6AJr6J35J\nRCTfQ5s1kD+hGMSE1rH7s46hmXEeyhnlRnaGF8eMU/SBJE/2NKPnxE7WzQ==\n-----END NOTPUBLIC KEY-----"}}]}`,
},
}
}
Expand All @@ -629,7 +623,7 @@ func makeConfigMapWithTwoEntries() *corev1.ConfigMap {
Name: config.ImagePoliciesConfigName,
},
Data: map[string]string{
cipName: `{"images":[{"glob":"ghcr.io/example/*"}],"authorities":[{"key":{\"publicKeys\":[{\"Curve\":{\"P\":115792089210356248762697446949407573530086143415290314195533631308867097853951,\"N\":115792089210356248762697446949407573529996955224135760342422259061068512044369,\"B\":41058363725152142129326129780047268409114441015993725554835256314039467401291,\"Gx\":48439561293906451759052585252797914202762949526041747995844080717082404635286,\"Gy\":36134250956749795798585127919587881956611106672985015071877198253568414405109,\"BitSize\":256,\"Name\":\"P-256\"},\"X\":88707635920070641755721733257804385133633538985728333477702186279848359565508,\"Y\":60084629679813308197062610650536235858799576629081565335301707208097929287373}]}}]}`,
cipName: `{"images":[{"glob":"ghcr.io/example/*"}],"authorities":[{"key":{"data":"-----BEGIN PUBLIC KEY-----\nMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAExB6+H6054/W1SJgs5JR6AJr6J35J\nRCTfQ5s1kD+hGMSE1rH7s46hmXEeyhnlRnaGF8eMU/SBJE/2NKPnxE7WzQ==\n-----END PUBLIC KEY-----"}}]}`,
cipName2: "remove me please",
},
}
Expand Down
29 changes: 29 additions & 0 deletions test/testdata/cosigned/e2e/cip-key.yaml
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
# Copyright 2022 The Sigstore Authors.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# https://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

apiVersion: cosigned.sigstore.dev/v1alpha1
kind: ClusterImagePolicy
metadata:
name: image-policy-key
spec:
images:
- glob: registry.local:5000/cosigned/demo*
authorities:
- key:
data: |
-----BEGIN PUBLIC KEY-----
MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEZxAfzrQG1EbWyCI8LiSB7YgSFXoI
FNGTyQGKHFc6/H8TQumT9VLS78pUwtv3w7EfKoyFZoP32KrO7nzUy2q6Cw==
-----END PUBLIC KEY-----
2 changes: 1 addition & 1 deletion test/testdata/cosigned/e2e/cip.yaml → test/testdata/cosigned/e2e/cip-keyless.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
apiVersion: cosigned.sigstore.dev/v1alpha1
kind: ClusterImagePolicy
metadata:
name: image-policy
name: image-policy-keyless
spec:
images:
- glob: registry.local:5000/cosigned/demo*
Expand Down

0 comments on commit d03404e

Please sign in to comment.