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

wip: notation implementation #1885

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

wip: notation implementation #1885

wants to merge 4 commits into from

Conversation

phbelitz
Copy link
Member

Implementation for notation

Checklist

  • PR is rebased to/aimed at branch develop
  • PR follows Contributing Guide
  • Added tests (if necessary)
  • Extended README/Documentation (if necessary)
  • Adjusted versions of image and Helm chart in Chart.yaml (if necessary)

# simple test cases for notation
- id: unsigned
txt: Testing unsigned image...
ref: ghcr.io/sse-secure-systems/testimage:notation-unsign
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ref: ghcr.io/sse-secure-systems/testimage:notation-unsign
ref: ghcr.io/sse-secure-systems/testimage:notation-unsigned

expected_msg: error during notation validation
- id: signed
txt: Testing signed image...
ref: ghcr.io/sse-secure-systems/testimage:notation-sign
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ref: ghcr.io/sse-secure-systems/testimage:notation-sign
ref: ghcr.io/sse-secure-systems/testimage:notation-signed

install "make"
multi_test "notation/cases.yaml"
uninstall "make"
}
Copy link
Member

Choose a reason for hiding this comment

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

nit: missing EOF newline

Copy link
Member

Choose a reason for hiding this comment

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

also for other files below

type InMemoryTrustStore struct {
trustRoots []auth.TrustRoot
certs map[string][]*x509.Certificate
truststore.X509TrustStore
Copy link
Member

Choose a reason for hiding this comment

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

Where is this used, isn't the interface already satisfied?

Comment on lines +130 to +132
SignatureVerification: trustpolicy.SignatureVerification{
VerificationLevel: trustpolicy.LevelStrict.Name,
},
Copy link
Member

Choose a reason for hiding this comment

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

What about VerifyTimestamp? Without having looked at it this looks like something we want to have an opinion on

VerificationLevel: trustpolicy.LevelStrict.Name,
},
TrustStores: utils.Map(trs, func(tr auth.TrustRoot) string { return fmt.Sprintf("ca:%s", tr.Name) }),
TrustedIdentities: []string{"*"},
Copy link
Member

Choose a reason for hiding this comment

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

Without knowing anything, this looks weird 🤔

Comment on lines +12 to +14
func TestNewInMemoryTrustStore(t *testing.T) {
// already tested in notation_validator_test.go
}
Copy link
Member

Choose a reason for hiding this comment

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

Why have this test case here then?

[]string{},
"no trust roots provided for validator",
},
{ // 3: unmarschal error
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{ // 3: unmarschal error
{ // 3: unmarshal error

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants