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 digest validation utils and examples #20887

Merged
merged 10 commits into from
May 31, 2023
Merged

Conversation

tadelesh
Copy link
Member

resolve: #20836

@tadelesh
Copy link
Member Author

tadelesh commented May 23, 2023

@jhendrixMSFT @JeffreyRichter please help to review the digest validation utils. You could refer all example files for the usage.

A brief explanation:

  1. GetManifest, GetBlob, GetChunk all use DigestValidationReader to read stream while validate.
  2. if UploadManifest by tag, we could not know the sig alg before service respond, so user need to do the read twice with DigestValidationReader to validate digest.
  3. UploadChunk keep the same with previous version, only some code tidy.

@tadelesh tadelesh requested a review from jhendrixMSFT May 25, 2023 03:25
@tadelesh
Copy link
Member Author

@JeffreyRichter The latest API view for azcontainerregistry could be found here. The change include removal of useless marshal method and new-added utils for digest validation. Do I need to have a former meeting review with you?

@JeffreyRichter
Copy link
Member

I had just 1 comment about hash algorithms

@tadelesh
Copy link
Member Author

tadelesh commented May 26, 2023

I had just 1 comment about hash algorithms

Thanks for the quick review. I've replied the comment. The hash algorithm is considered already.

@tadelesh tadelesh merged commit 2e91823 into Azure:main May 31, 2023
}

func parseDigestValidator(digest string) (digestValidator, error) {
alg := digest[:strings.Index(digest, ":")]
Copy link
Member

Choose a reason for hiding this comment

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

There might be an "index out of range" issue if digest does not contain a :.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will fix before GA.

Comment on lines +32 to +36
if v, ok := validatorCtors[alg]; ok {
return v(), nil
} else {
return nil, ErrDigestAlgNotSupported
}
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

It applies to all occurrences in this file.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will fix before GA.

end = size
}
chunkReader := io.NewSectionReader(f, current, end-current)
uploadResp, err := blobClient.UploadChunk(context.TODO(), location, chunkReader, calculator, &azcontainerregistry.BlobClientUploadChunkOptions{RangeStart: to.Ptr(int32(current)), RangeEnd: to.Ptr(int32(end - 1))})
Copy link
Member

Choose a reason for hiding this comment

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

Just curious if calculator can be reused in another process or not.

Copy link
Member Author

Choose a reason for hiding this comment

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

No. The calculator will hold the hash calculation status, so one calculator for one upload. I'll added some doc to explain it before GA.

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.

Add digest validation to all related operations for azcontainerregistry
4 participants