Skip to content

Commit

Permalink
oci: sort remaining quirks in cosign verify logic
Browse files Browse the repository at this point in the history 
This commit properly sets `IgnoreTlog` to `true` when a public key is
provided to check the signature against, which matches the (silent)
default behavior from cosign v1.

However, during this exercise it has become apparant that this
assumption isn't necessarily true. As you can theoretically have a
custom key and a tlog entry.

Given this, we should inventarise the possible configuration options
and the potential value they have to users (e.g. defining a custom
Rekor URL seems to be valuable as well), and extend our API to
facilitate these needs.

Signed-off-by: Hidde Beydals <hidde@hhh.computer>
  • Loading branch information
@hiddeco
hiddeco committed May 17, 2023
1 parent 0c95ab5 commit a8f4fed
Show file tree
Hide file tree
Showing 4 changed files with 27 additions and 20 deletions.
25 changes: 13 additions & 12 deletions internal/controller/helmchart_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import (
"errors"
"fmt"
"io"
"io/ioutil"
"net/http"
"os"
"path"
Expand Down Expand Up @@ -1058,7 +1057,8 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) {
)

// Load a test chart
chartData, err := ioutil.ReadFile(chartPath)
chartData, err := os.ReadFile(chartPath)
g.Expect(err).NotTo(HaveOccurred())

// Upload the test chart
metadata, err := loadTestChartToOCI(chartData, chartPath, testRegistryServer)
Expand Down Expand Up @@ -2333,16 +2333,16 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) {

builder := fakeclient.NewClientBuilder().WithScheme(testEnv.GetScheme())
workspaceDir := t.TempDir()
server, err := setupRegistryServer(ctx, workspaceDir, tt.registryOpts)

server, err := setupRegistryServer(ctx, workspaceDir, tt.registryOpts)
g.Expect(err).NotTo(HaveOccurred())

// Load a test chart
chartData, err := ioutil.ReadFile(chartPath)
chartData, err := os.ReadFile(chartPath)
g.Expect(err).ToNot(HaveOccurred())

// Upload the test chart
metadata, err := loadTestChartToOCI(chartData, chartPath, server)
g.Expect(err).NotTo(HaveOccurred())
g.Expect(err).ToNot(HaveOccurred())

repo := &helmv1.HelmRepository{
Expand Down Expand Up @@ -2452,7 +2452,8 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_verifySignature(t *testing.T
)

// Load a test chart
chartData, err := ioutil.ReadFile(chartPath)
chartData, err := os.ReadFile(chartPath)
g.Expect(err).ToNot(HaveOccurred())

// Upload the test chart
metadata, err := loadTestChartToOCI(chartData, chartPath, server)
Expand Down Expand Up @@ -2504,10 +2505,10 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_verifySignature(t *testing.T
},
want: sreconcile.ResultEmpty,
wantErr: true,
wantErrMsg: "chart verification error: failed to verify <url>: no matching signatures:",
wantErrMsg: "chart verification error: failed to verify <url>: no signatures found for image",
assertConditions: []metav1.Condition{
*conditions.TrueCondition(sourcev1.BuildFailedCondition, "ChartVerificationError", "chart verification error: failed to verify <url>: no matching signatures:"),
*conditions.FalseCondition(sourcev1.SourceVerifiedCondition, sourcev1.VerificationError, "chart verification error: failed to verify <url>: no matching signatures:"),
*conditions.TrueCondition(sourcev1.BuildFailedCondition, "ChartVerificationError", "chart verification error: failed to verify <url>: no signatures found for image"),
*conditions.FalseCondition(sourcev1.SourceVerifiedCondition, sourcev1.VerificationError, "chart verification error: failed to verify <url>: no signatures found for image"),
},
},
{
Expand All @@ -2522,8 +2523,8 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_verifySignature(t *testing.T
want: sreconcile.ResultEmpty,
wantErr: true,
assertConditions: []metav1.Condition{
*conditions.TrueCondition(sourcev1.BuildFailedCondition, "ChartVerificationError", "chart verification error: failed to verify <url>: no matching signatures:"),
*conditions.FalseCondition(sourcev1.SourceVerifiedCondition, sourcev1.VerificationError, "chart verification error: failed to verify <url>: no matching signatures:"),
*conditions.TrueCondition(sourcev1.BuildFailedCondition, "ChartVerificationError", "chart verification error: failed to verify <url>: no signatures found for image"),
*conditions.FalseCondition(sourcev1.SourceVerifiedCondition, sourcev1.VerificationError, "chart verification error: failed to verify <url>: no signatures found for image"),
},
},
{
Expand Down Expand Up @@ -2696,7 +2697,7 @@ func loadTestChartToOCI(chartData []byte, chartPath string, server *registryClie
}

// Load a test chart
chartData, err = ioutil.ReadFile(chartPath)
chartData, err = os.ReadFile(chartPath)
if err != nil {
return nil, err
}
Expand Down
4 changes: 3 additions & 1 deletion internal/controller/ocirepository_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1095,7 +1095,7 @@ func TestOCIRepository_reconcileSource_verifyOCISourceSignature(t *testing.T) {
assertConditions: []metav1.Condition{
*conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new revision '<revision>' for '<url>'"),
*conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new revision '<revision>' for '<url>'"),
*conditions.FalseCondition(sourcev1.SourceVerifiedCondition, sourcev1.VerificationError, "failed to verify the signature using provider '<provider> keyless': no matching signatures"),
*conditions.FalseCondition(sourcev1.SourceVerifiedCondition, sourcev1.VerificationError, "failed to verify the signature using provider '<provider> keyless': no signatures found for image"),
},
},
{
Expand Down Expand Up @@ -1193,6 +1193,8 @@ func TestOCIRepository_reconcileSource_verifyOCISourceSignature(t *testing.T) {

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
g := NewWithT(t)

obj := &ociv1.OCIRepository{
ObjectMeta: metav1.ObjectMeta{
GenerateName: "verify-oci-source-signature-",
Expand Down
4 changes: 1 addition & 3 deletions internal/controller/suite_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import (
"context"
"fmt"
"io"
"io/ioutil"
"math/rand"
"os"
"path/filepath"
Expand Down Expand Up @@ -164,8 +163,7 @@ func setupRegistryServer(ctx context.Context, workspaceDir string, opts registry
}

htpasswdPath := filepath.Join(workspaceDir, testRegistryHtpasswdFileBasename)
err = ioutil.WriteFile(htpasswdPath, []byte(fmt.Sprintf("%s:%s\n", testRegistryUsername, string(pwBytes))), 0644)
if err != nil {
if err = os.WriteFile(htpasswdPath, []byte(fmt.Sprintf("%s:%s\n", testRegistryUsername, string(pwBytes))), 0644); err != nil {
return nil, fmt.Errorf("failed to create htpasswd file: %s", err)
}

Expand Down
14 changes: 10 additions & 4 deletions internal/oci/verifier.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,10 @@ import (
"context"
"crypto"
"fmt"
"github.com/sigstore/cosign/v2/cmd/cosign/cli/rekor"

"github.com/google/go-containerregistry/pkg/v1/remote"
"github.com/sigstore/cosign/v2/cmd/cosign/cli/fulcio"
"github.com/sigstore/cosign/v2/cmd/cosign/cli/rekor"
"github.com/sigstore/cosign/v2/pkg/cosign"
ociremote "github.com/sigstore/cosign/v2/pkg/oci/remote"

Expand Down Expand Up @@ -93,6 +93,11 @@ func NewCosignVerifier(ctx context.Context, opts ...Options) (*CosignVerifier, e
// If there is no public key provided, it will try keyless verification.
// https://github.com/sigstore/cosign/blob/main/KEYLESS.md.
if len(o.PublicKey) > 0 {
checkOpts.Offline = true
// TODO(hidde): this is an oversight in our implementation. As it is
// theoretically possible to have a custom PK, without disabling tlog.
checkOpts.IgnoreTlog = true

pubKeyRaw, err := cryptoutils.UnmarshalPEMToPublicKey(o.PublicKey)
if err != nil {
return nil, err
Expand All @@ -102,9 +107,6 @@ func NewCosignVerifier(ctx context.Context, opts ...Options) (*CosignVerifier, e
if err != nil {
return nil, err
}

checkOpts.Offline = true

} else {
rcerts, err := fulcio.GetRoots()
if err != nil {
Expand All @@ -124,6 +126,10 @@ func NewCosignVerifier(ctx context.Context, opts ...Options) (*CosignVerifier, e
}
checkOpts.RekorClient = rc

// This performs an online fetch of the Rekor public keys, but this is needed
// for verifying tlog entries (both online and offline).
// TODO(hidde): above note is important to keep in mind when we implement
// "offline" tlog above.
checkOpts.RekorPubKeys, err = cosign.GetRekorPubs(ctx)
if err != nil {
return nil, fmt.Errorf("unable to get Rekor public keys: %w", err)
Expand Down

0 comments on commit a8f4fed

Please sign in to comment.