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

fix: Scrutinize references for digests posing as tags #326

Merged

Conversation

nima
Copy link
Contributor

@nima nima commented Sep 19, 2022

This change prevents a token sneaking in as a tag, then later getting validated as a digest. In order to do that, a new struct member has been introduced to registry.Reference, namely, ReferenceType. This can take one of the small finite set of values: Undefined, None, Tag, and Digest. None is applicable only to valid form D, and Undefined is the zero/null/empty default value to force explicitness.

Also note that in valid form B, we're still allowing the redundant tag, is there a reason to allow for this (as opposed to considering it ambiguous, and deeming it invalid)? just noticed this was already answered by @Wwwsylvia in #303.

Resolves #303.

Signed-off-by: Nima Talebi github@nima.id.au

@nima nima force-pushed the increase-reference-validation-scrutiny branch from 2ce27ee to d5f02ab Compare September 19, 2022 21:18
@codecov-commenter
Copy link

codecov-commenter commented Sep 20, 2022

Codecov Report

Merging #326 (5850036) into main (c1f42a4) will decrease coverage by 0.09%.
The diff coverage is 82.35%.

@@            Coverage Diff             @@
##             main     #326      +/-   ##
==========================================
- Coverage   72.18%   72.09%   -0.10%     
==========================================
  Files          38       38              
  Lines        3635     3669      +34     
==========================================
+ Hits         2624     2645      +21     
- Misses        752      766      +14     
+ Partials      259      258       -1     
Impacted Files Coverage Δ
registry/reference.go 68.42% <74.35%> (-6.85%) ⬇️
registry/remote/repository.go 67.24% <93.10%> (+0.57%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

I was expecting that updating the implementation of the ParseReference() function only without touching others should fix the bug.

registry/reference.go Outdated Show resolved Hide resolved
registry/remote/repository.go Outdated Show resolved Hide resolved
@nima nima force-pushed the increase-reference-validation-scrutiny branch 2 times, most recently from ecf0009 to 92c149a Compare September 20, 2022 04:25
errdef/errors.go Outdated Show resolved Hide resolved
registry/remote/repository.go Outdated Show resolved Hide resolved
Copy link
Contributor Author

@nima nima left a comment

Choose a reason for hiding this comment

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

Annotated the changes in an attempt to add more clarity.

@nima nima force-pushed the increase-reference-validation-scrutiny branch from 92c149a to c26dda2 Compare September 20, 2022 04:51
@nima
Copy link
Contributor Author

nima commented Sep 20, 2022

Added a couple more tests in the last push; no code-changes otherwise.

errdef/errors.go Outdated Show resolved Hide resolved
registry/remote/repository.go Outdated Show resolved Hide resolved
@nima nima force-pushed the increase-reference-validation-scrutiny branch 3 times, most recently from 52ed305 to a4558ea Compare September 20, 2022 05:55
registry/reference.go Outdated Show resolved Hide resolved
@nima nima force-pushed the increase-reference-validation-scrutiny branch from a4558ea to e2cc8b7 Compare September 20, 2022 15:44
Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

Let's try not to have any breaking changes unless it is necessary with justifications.

registry/reference.go Outdated Show resolved Hide resolved
registry/reference.go Outdated Show resolved Hide resolved
registry/reference.go Outdated Show resolved Hide resolved
registry/remote/repository.go Outdated Show resolved Hide resolved
registry/remote/repository.go Outdated Show resolved Hide resolved
registry/remote/repository.go Outdated Show resolved Hide resolved
registry/reference.go Outdated Show resolved Hide resolved
@nima nima force-pushed the increase-reference-validation-scrutiny branch 2 times, most recently from 2d32441 to 056dc8c Compare September 21, 2022 19:29
registry/remote/repository.go Outdated Show resolved Hide resolved
registry/reference.go Show resolved Hide resolved
@nima nima force-pushed the increase-reference-validation-scrutiny branch 6 times, most recently from 97f3c2b to e8cf363 Compare September 23, 2022 04:04
registry/reference.go Show resolved Hide resolved
registry/reference.go Outdated Show resolved Hide resolved
registry/reference.go Outdated Show resolved Hide resolved
registry/reference.go Outdated Show resolved Hide resolved
registry/reference.go Outdated Show resolved Hide resolved
registry/reference.go Outdated Show resolved Hide resolved
registry/reference.go Outdated Show resolved Hide resolved
registry/reference.go Show resolved Hide resolved
registry/reference.go Outdated Show resolved Hide resolved
registry/remote/repository.go Outdated Show resolved Hide resolved
@nima nima force-pushed the increase-reference-validation-scrutiny branch from e8cf363 to 8bd2510 Compare September 23, 2022 22:55
@nima nima requested a review from shizhMSFT September 23, 2022 22:55
registry/reference.go Outdated Show resolved Hide resolved
registry/reference.go Outdated Show resolved Hide resolved
registry/remote/repository.go Outdated Show resolved Hide resolved
registry/reference.go Outdated Show resolved Hide resolved
registry/remote/repository.go Outdated Show resolved Hide resolved
registry/remote/repository.go Outdated Show resolved Hide resolved
registry/remote/repository.go Outdated Show resolved Hide resolved
@nima nima force-pushed the increase-reference-validation-scrutiny branch from 8bd2510 to 5850036 Compare September 29, 2022 20:04
Signed-off-by: Nima Talebi <github@nima.id.au>
@nima nima force-pushed the increase-reference-validation-scrutiny branch from 5850036 to 960f612 Compare September 29, 2022 20:50
@nima nima requested a review from shizhMSFT September 30, 2022 02:40
Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

LGTM

@shizhMSFT shizhMSFT merged commit 56c9619 into oras-project:main Sep 30, 2022
@nima nima deleted the increase-reference-validation-scrutiny branch October 31, 2022 20:58
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.

Review registry.ParseReference function
3 participants