Skip to content

feat: add a method for parsing the descriptor from a payload#261

Closed
byronchien wants to merge 2 commits intonotaryproject:mainfrom
byronchien:parse-descriptor
Closed

feat: add a method for parsing the descriptor from a payload#261
byronchien wants to merge 2 commits intonotaryproject:mainfrom
byronchien:parse-descriptor

Conversation

@byronchien
Copy link
Contributor

@byronchien byronchien commented Feb 2, 2023

Adds a helper to get the descriptor from the payload. Needed here instead of in notation-core-go since the payload needs to be parsed to envelope.Payload (in notation-go internal)

suggested order for review: notation-go #261 (this one) => notation-core-go #111 => notation #527 =>notation #528

Signed-off-by: Byron Chien chienb@amazon.com

Copy link
Contributor

@patrickzheng200 patrickzheng200 left a comment

Choose a reason for hiding this comment

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

Just an open discussion, where are we using this new method? Looks like it's a helper function but not used in notation.go.

Signed-off-by: Byron Chien <chienb@amazon.com>
@byronchien
Copy link
Contributor Author

Just an open discussion, where are we using this new method? Looks like it's a helper function but not used in notation.go.

using this function in inspect (PR here). Added the helper in notation-go rather than notation since it relies on a type not exported from notation-go (envelope.Payload).

Signed-off-by: Byron Chien <chienb@amazon.com>
@patrickzheng200
Copy link
Contributor

patrickzheng200 commented Feb 3, 2023

Just an open discussion, where are we using this new method? Looks like it's a helper function but not used in notation.go.

using this function in inspect (PR here). Added the helper in notation-go rather than notation since it relies on a type not exported from notation-go (envelope.Payload).

I see. IMO, notation.go should only expose APIs related to the core processes such as Sign and Verify. As a library code, this is to keep notation-go as clean as possible. Notation CLI also has its own internal/envelope file. We could add envelope.Payload struct to this file and move this helper function into Notation CLI.
(Note: notation CLI is one of the users of notation-go library. there will be non-CLI users as well.)

@byronchien
Copy link
Contributor Author

Just an open discussion, where are we using this new method? Looks like it's a helper function but not used in notation.go.

using this function in inspect (PR here). Added the helper in notation-go rather than notation since it relies on a type not exported from notation-go (envelope.Payload).

I see. IMO, notation.go should only expose APIs related to the core processes such as Sign and Verify. As a library code, this is to keep notation-go as clean as possible. Notation CLI also has its own internal/envelope file. We could add envelope.Payload struct to this file and move this helper function into Notation CLI. (Note: notation CLI is one of the users of notation-go library. there will be non-CLI users as well.)

makes sense, will add the struct and helper to notation instead


// GetDescriptorFromPayload parses a signature payload and returns the descriptor
// that was signed.
func GetDescriptorFromPayload(payload *signature.Payload) (*ocispec.Descriptor, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

According to effective go, Get prefix should be dropped.

Suggested change
func GetDescriptorFromPayload(payload *signature.Payload) (*ocispec.Descriptor, error) {
func DescriptorFromPayload(payload *signature.Payload) (*ocispec.Descriptor, error) {

Comment on lines +330 to +331
// GetDescriptorFromPayload parses a signature payload and returns the descriptor
// that was signed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: the descriptor was signed but may not be trusted.


// GetDescriptorFromPayload parses a signature payload and returns the descriptor
// that was signed.
func GetDescriptorFromPayload(payload *signature.Payload) (*ocispec.Descriptor, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The word Payload in DescriptorFromPayload is too broad. Consider DescriptorFromSignaturePayload.


var parsedPayload envelope.Payload

err := json.Unmarshal(payload.Content, &parsedPayload)
Copy link
Contributor

Choose a reason for hiding this comment

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

Security concern: payload media type is not checked before parsing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can parse the payload to an intermediate struct to check mediatype before parsing to the final payload struct.

@shizhMSFT shizhMSFT changed the title Add a method for parsing the descriptor from a payload feat: add a method for parsing the descriptor from a payload Feb 8, 2023
@byronchien
Copy link
Contributor Author

added DescriptorFromSignaturePayload to notation in #528

@byronchien byronchien closed this Feb 8, 2023
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.

4 participants

Comments