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

add signed chart check, tests and doc #287

Merged
merged 1 commit into from
Oct 28, 2022

Conversation

mmulholla
Copy link
Collaborator

  • Added new check for signed charts
  • Added unit tests
  • Added python test

Chart verifier logic for new check is documented here: https://lucid.app/lucidchart/1fd9bb6c-98b9-4a20-8d69-8af003ce8ee1/edit?invitationId=inv_99e435e0-1db6-4181-873d-4ce60bef8d48&page=0_0#

Jora issue: https://issues.redhat.com/browse/HELM-410

cmd/verify.go Outdated
return cmd
}

func init() {
rootCmd.AddCommand(NewVerifyCmd(viper.GetViper()))
}

func readPublicKeyFile(keyFilename string) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the function readPublicKeyFile getting used somewhere.If not then we can remove this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed it

})

if checkErr != nil {
return nil, NewCheckErr(checkErr)
}
_ = result.AddCheck(check, r)

if check.CheckId.Name == apiChecks.SignatureIsValid {
if len(c.publicKeys) == 1 && strings.Contains(r.Reason, checks.ChartSigned) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The public keys is an array of string, but here we are checking whether it contains only one public key in c.publicKeys array. So if the user has to specify the public key per chart then why not have the type of c.publicKeys as a string instead of array.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

With an array we do not know which key was the correct one so cannot add digest to report. With cli we only get one key so if chart verifies we know that is the correct key so we can then add digest to report.


userCacheDir = getCacheDir(opts)
if userCacheDir == "" {
return ChartCacheItem{}, err
}
cacheDir := path.Join(userCacheDir, "chart-verifier")
Copy link
Contributor

Choose a reason for hiding this comment

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

If this line cacheDir := path.Join(userCacheDir, "chart-verifier") required here now? getCacheDir(opts *CheckOptions) func seems to be returning the repocache/usercache with chart-verifier joined to the path.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Goof spot, no longer needed. Removed.

Copy link
Contributor

@Kartikey-star Kartikey-star left a comment

Choose a reason for hiding this comment

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

Looks good to me .As discussed we would need to change the way the public keys of developer are used for testing purpose but that should be addressed with a different PR.

@mmulholla mmulholla merged commit d5cc953 into redhat-certification:main Oct 28, 2022
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