From d22c312573ab750db6e1e7a795b0569b88fe5ed5 Mon Sep 17 00:00:00 2001 From: Kavitha <5756375+kkavitha@users.noreply.github.com> Date: Sun, 13 Mar 2022 09:39:37 -0400 Subject: [PATCH] Add public key validation (#1598) * Add public key validation Signed-off-by: Kavitha Krishnan * fix: key validation Signed-off-by: hectorj2f * fix: test valid policy using wrong key data Signed-off-by: hectorj2f Co-authored-by: hectorj2f --- .../v1alpha1/clusterimagepolicy_validation.go | 7 +++ .../clusterimagepolicy_validation_test.go | 44 +++++++++++++++- pkg/apis/utils/key_validations.go | 52 +++++++++++++++++++ .../invalid/both-regex-and-pattern.yaml | 6 ++- .../cosigned/invalid/invalid-pubkey.yaml | 24 +++++++++ .../keyref-with-multiple-properties.yaml | 6 ++- .../invalid/valid-keyref-and-keylessref.yaml | 2 +- .../testdata/cosigned/valid/valid-policy.yaml | 8 +-- 8 files changed, 142 insertions(+), 7 deletions(-) create mode 100644 pkg/apis/utils/key_validations.go create mode 100644 test/testdata/cosigned/invalid/invalid-pubkey.yaml diff --git a/pkg/apis/cosigned/v1alpha1/clusterimagepolicy_validation.go b/pkg/apis/cosigned/v1alpha1/clusterimagepolicy_validation.go index cdcf9c8cff5..05a3fc0a185 100644 --- a/pkg/apis/cosigned/v1alpha1/clusterimagepolicy_validation.go +++ b/pkg/apis/cosigned/v1alpha1/clusterimagepolicy_validation.go @@ -18,6 +18,7 @@ import ( "context" "strings" + "github.com/sigstore/cosign/pkg/apis/utils" "knative.dev/pkg/apis" ) @@ -93,6 +94,12 @@ func (key *KeyRef) Validate(ctx context.Context) *apis.FieldError { if key.KMS != "" || key.SecretRef != nil { errs = errs.Also(apis.ErrMultipleOneOf("data", "kms", "secretref")) } + validPubkey := utils.IsValidKey([]byte(key.Data)) + if !validPubkey { + errs = errs.Also( + apis.ErrInvalidValue(key.Data, "data"), + ) + } } else if key.KMS != "" && key.SecretRef != nil { errs = errs.Also(apis.ErrMultipleOneOf("data", "kms", "secretref")) } diff --git a/pkg/apis/cosigned/v1alpha1/clusterimagepolicy_validation_test.go b/pkg/apis/cosigned/v1alpha1/clusterimagepolicy_validation_test.go index 50f128d78e4..117d8c1767e 100644 --- a/pkg/apis/cosigned/v1alpha1/clusterimagepolicy_validation_test.go +++ b/pkg/apis/cosigned/v1alpha1/clusterimagepolicy_validation_test.go @@ -115,7 +115,7 @@ func TestKeyValidation(t *testing.T) { Authorities: []Authority{ { Key: &KeyRef{ - Data: "---some key data----", + Data: "-----BEGIN PUBLIC KEY-----\nMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEaEOVJCFtduYr3xqTxeRWSW32CY/s\nTBNZj4oIUPl8JvhVPJ1TKDPlNcuT4YphSt6t3yOmMvkdQbCj8broX6vijw==\n-----END PUBLIC KEY-----", KMS: "kms://key/path", }, }, @@ -123,6 +123,48 @@ func TestKeyValidation(t *testing.T) { }, }, }, + { + name: "Should fail when key has mixed valid and invalid data: %v", + expectErr: true, + errorString: "invalid value: -----BEGIN PUBLIC KEY-----\nMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEaEOVJCFtduYr3xqTxeRWSW32CY/s\nTBNZj4oIUPl8JvhVPJ1TKDPlNcuT4YphSt6t3yOmMvkdQbCj8broX6vijw==\n-----END PUBLIC KEY-----\n---somedata---", + policy: ClusterImagePolicy{ + Spec: ClusterImagePolicySpec{ + Images: []ImagePattern{ + { + Glob: "myglob", + }, + }, + Authorities: []Authority{ + { + Key: &KeyRef{ + Data: "-----BEGIN PUBLIC KEY-----\nMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEaEOVJCFtduYr3xqTxeRWSW32CY/s\nTBNZj4oIUPl8JvhVPJ1TKDPlNcuT4YphSt6t3yOmMvkdQbCj8broX6vijw==\n-----END PUBLIC KEY-----\n---somedata---", + }, + }, + }, + }, + }, + }, + { + name: "Should fail when key has malformed pubkey data: %v", + expectErr: true, + errorString: "invalid value: ---some key data----", + policy: ClusterImagePolicy{ + Spec: ClusterImagePolicySpec{ + Images: []ImagePattern{ + { + Glob: "myglob", + }, + }, + Authorities: []Authority{ + { + Key: &KeyRef{ + Data: "---some key data----", + }, + }, + }, + }, + }, + }, { name: "Should fail when key is empty: %v", expectErr: true, diff --git a/pkg/apis/utils/key_validations.go b/pkg/apis/utils/key_validations.go new file mode 100644 index 00000000000..c7b151c3dc6 --- /dev/null +++ b/pkg/apis/utils/key_validations.go @@ -0,0 +1,52 @@ +// 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 +// +// http://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. + +package utils + +import ( + "crypto/x509" + "encoding/pem" +) + +func IsValidKey(b []byte) bool { + valid := true + pems, validPEM := parsePEMKey(b) + if !validPEM { + return false + } + + for _, p := range pems { + _, err := x509.ParsePKIXPublicKey(p.Bytes) + if err != nil { + return false + } + } + + return valid +} + +func parsePEMKey(b []byte) ([]*pem.Block, bool) { + pemKey, rest := pem.Decode(b) + valid := true + if pemKey == nil { + return nil, false + } + pemBlocks := []*pem.Block{pemKey} + + if len(rest) > 0 { + list, check := parsePEMKey(rest) + return append(pemBlocks, list...), check + } + return pemBlocks, valid +} diff --git a/test/testdata/cosigned/invalid/both-regex-and-pattern.yaml b/test/testdata/cosigned/invalid/both-regex-and-pattern.yaml index c1340d204d1..da02dcb7a68 100644 --- a/test/testdata/cosigned/invalid/both-regex-and-pattern.yaml +++ b/test/testdata/cosigned/invalid/both-regex-and-pattern.yaml @@ -22,4 +22,8 @@ spec: regex: image.* authorities: - key: - data: "---somedata---" + data: | + -----BEGIN PUBLIC KEY----- + MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEZxAfzrQG1EbWyCI8LiSB7YgSFXoI + FNGTyQGKHFc6/H8TQumT9VLS78pUwtv3w7EfKoyFZoP32KrO7nzUy2q6Cw== + -----END PUBLIC KEY----- diff --git a/test/testdata/cosigned/invalid/invalid-pubkey.yaml b/test/testdata/cosigned/invalid/invalid-pubkey.yaml new file mode 100644 index 00000000000..66128082db0 --- /dev/null +++ b/test/testdata/cosigned/invalid/invalid-pubkey.yaml @@ -0,0 +1,24 @@ +# 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 +# +# http:#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 +spec: + images: + - glob: image* + authorities: + - key: + data: "---somedata---" diff --git a/test/testdata/cosigned/invalid/keyref-with-multiple-properties.yaml b/test/testdata/cosigned/invalid/keyref-with-multiple-properties.yaml index f0c32b3c98b..5fc31bc28ff 100644 --- a/test/testdata/cosigned/invalid/keyref-with-multiple-properties.yaml +++ b/test/testdata/cosigned/invalid/keyref-with-multiple-properties.yaml @@ -21,5 +21,9 @@ spec: - glob: image* authorities: - key: - data: "---somedata---" + data: | + -----BEGIN PUBLIC KEY----- + MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEZxAfzrQG1EbWyCI8LiSB7YgSFXoI + FNGTyQGKHFc6/H8TQumT9VLS78pUwtv3w7EfKoyFZoP32KrO7nzUy2q6Cw== + -----END PUBLIC KEY----- kms: "kms://url" diff --git a/test/testdata/cosigned/invalid/valid-keyref-and-keylessref.yaml b/test/testdata/cosigned/invalid/valid-keyref-and-keylessref.yaml index 5a8a0538572..b543ba7613c 100644 --- a/test/testdata/cosigned/invalid/valid-keyref-and-keylessref.yaml +++ b/test/testdata/cosigned/invalid/valid-keyref-and-keylessref.yaml @@ -24,4 +24,4 @@ spec: identities: - issuer: "issue-details" key: - data: "---somekey---" + kms: "kms://url" diff --git a/test/testdata/cosigned/valid/valid-policy.yaml b/test/testdata/cosigned/valid/valid-policy.yaml index e6e10c025a6..ee32abc0e8e 100644 --- a/test/testdata/cosigned/valid/valid-policy.yaml +++ b/test/testdata/cosigned/valid/valid-policy.yaml @@ -21,8 +21,6 @@ spec: - glob: images.* - glob: image* authorities: - - key: - data: "---another-public-key---" - keyless: ca-key: secretRef: @@ -36,7 +34,11 @@ spec: identities: - issuer: "issue-details1" - key: - data: "---some-key---" + data: | + -----BEGIN PUBLIC KEY----- + MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEaEOVJCFtduYr3xqTxeRWSW32CY/s + TBNZj4oIUPl8JvhVPJ1TKDPlNcuT4YphSt6t3yOmMvkdQbCj8broX6vijw== + -----END PUBLIC KEY----- - key: kms: "kms://key/path" - key: