Add regular expression match for ref string input #61

TrumanLing · 2016-10-28T09:16:10Z

implementation for spec description "Object names in the refs
subdirectories MUST NOT include characters outside of the set of "A" to
"Z", "a" to "z", "0" to "9", the hyphen -, the dot ., and the underscore
_." at https://github.com/opencontainers/image-spec/blob/master/image-layout.md.

Signed-off-by: Ling FaKe [email protected]

wking · 2016-10-28T09:21:22Z

cmd/oci-create-runtime-bundle/main.go

 var gitCommit = ""

+// refRegexp for string ref should be complied with description "Object names in the refs subdirectories MUST NOT include characters outside of the set of "A" to "Z", "a" to "z", "0" to "9", the hyphen -, the dot ., and the underscore _." at https://github.com/opencontainers/image-spec/blob/master/image-layout.md.
+var refRegexp = regexp.MustCompile(`^[a-zA-Z0-9\-\.\_]+?$`)


This constant should probably be provided by image-spec, from which runtime-tools can import and use it.

Thanks for your proposal, I will try to submit it in image-spec.

wking · 2016-10-28T09:25:37Z

cmd/oci-create-runtime-bundle/main.go

+	if boolRefMatch != true {
+		v.stderr.Printf("ref name (%s) is invalid", v.ref)
+		os.Exit(1)
+	}


This probably belongs in image-layout validation like image/image.go's validate.

IMHO, parameter ref input should be validated as early as possible, which can reduces unnecessary software execution.

There is already [ref-oriented validation in image/image.go's validate/](https://github.com/TrumanLing/image-tools/blob/adbe39359bf2aac1aa01d3ce376036039c19fe32/image/image.go#L69). Adding that check here saves a few cycles in the oci-create-runtime-bundlecase, but completely misses the validation check in theoci-image-validatecase and the “library call toValidateLayout`” case.

@wking Greate suggestion! But image/image.go's validate is for oci-image-validate, so I think, it is reasonable to add regular expression checks at the beginning of image/image.go's unpack, validate and createRuntimeBundle, which are separately for oci-unpack, oci-image-validate and oci-create-runtime-bundle.

wking · 2016-10-29T22:22:44Z

image/image.go

 }

 func validate(w walker, refs []string, out *log.Logger) error {
+	var tmperr error


No need for tmperr. I'd rather have:

func validate(w walker, refs []string, out *log.Logger) (err error) {

above, use err for refsNameRegexpCheck, and possibly shift an := → = below where we currently setup err.

IMHO, the regular expression check should be at the beginning of the function. As for your recommendation, let the check below line := for err to avoid one more variable. But I think it is a little bit of breaking the logic with handling image validation, that means, the original func validate is for image validation, and not for parameter input check. So, I think the parameter check should be at the beginning, and the rest of code lines does its original operation, no insertion of other operation, even just only one line.

IMHO, the regular expression check should be at the beginning of the function.

I hadn't thought about that, but not that you bring it up… ;). I think it should be here, because:

If you run your check too early, you might not check anything and have invalid refs injected here slip through without getting caught.

A ref not existing seems more important than not meeting the regexp.

I feel pretty strongly about (1), but not particularly strongly about (2). Regardless of how (2) goes, the check should be inside that loop. Perhaps the point that the validated refs may not be parameters (when they come from listReferences) is enough to allow closer integration between this regexp validation and the existing logic.

I agree with you, but I updated it here, because ref is used starting from here.

wking · 2016-10-29T22:25:35Z

image/image.go

 }

 func unpack(w walker, dest, refName string) error {
+	if err := refsNameRegexpCheck(refName); err != nil {


I'm not convinced that we want to be doing validation as part of unpacking (folks who want it can call the validation code before calling unpack). But if that is not convincing, we can always add this for now, and remove it if/when someone gets around to filing a PR to pull the existing validation logic out of the unpacking code.

I'm not convinced that we want to be doing validation as part of unpacking (folks who want it can call the validation code before calling unpack).

I agree with you, but we can't avoid user's typing error. So I think, it is needed.

But if that is not convincing, we can always add this for now, and remove it if/when someone gets around to filing a PR to pull the existing validation logic out of the unpacking code.

Currently we add it like this, if there is better solution in the future, we will change to the better one.

On Sun, Oct 30, 2016 at 07:11:09PM -0700, TrumanLing wrote:

… but we can't avoid user's typing error…

That's not a problem. If the user typo's the ref name, unpacking will fail because the ref does not exist. Checking against the regexp in that case only “protects” you from the unlikely and presumably intentional case where the user is trying to unpack an illegal ref that already exists in the ref store. If the illegal ref is in the ref store it's not a compliant image, but I still think we should allow the user to unpack the ref (assuming the DAG it points to is unpackable).

OK, I understand your concern. I will change it after PR in image-spec is merged, which leads to CI failure without merging it.

implementation for spec description "Object names in the refs subdirectories MUST NOT include characters outside of the set of "A" to "Z", "a" to "z", "0" to "9", the hyphen -, the dot ., and the underscore _." at https://github.com/opencontainers/image-spec/blob/master/image-layout.md. Signed-off-by: Ling FaKe <[email protected]>

vbatts · 2017-02-08T19:29:27Z

failures in make lint

wking · 2017-02-08T19:35:55Z

On Wed, Feb 08, 2017 at 11:29:27AM -0800, Vincent Batts wrote: failures in `make lint`

Is it worth landing this for rc4 support with refs (and this restriction on their names) now removed from the spec for the next release [1] and no similar restriction on org.opencontainers.ref.name values [2]? I think we should just close this PR and wait for someone to file one with a more comprehensive catch-up to opencontainers/image-spec#533. [1]: https://github.com/opencontainers/image-spec/pull/533/files#diff-99bfcb610f238b942638d49d1ff2eaf4L58 [2]: https://github.com/opencontainers/image-spec/pull/533/files#diff-338aacf6337d00fc0cde843c3182c3f8R24

xiekeyang · 2017-02-09T02:48:22Z

Close for opencontainers/image-spec#533

wking reviewed Oct 28, 2016

View reviewed changes

wking mentioned this pull request Oct 28, 2016

Add regular expression for object names in 'refs' opencontainers/image-spec#421

Merged

wking reviewed Oct 29, 2016

View reviewed changes

xiekeyang closed this Feb 9, 2017

xiekeyang mentioned this pull request Feb 16, 2017

Bump version for v0.1.0 #116

Merged

Add regular expression match for ref string input #61

Add regular expression match for ref string input #61

Uh oh!

Conversation

TrumanLing commented Oct 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TrumanLing Oct 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TrumanLing Oct 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vbatts commented Feb 8, 2017

Uh oh!

wking commented Feb 8, 2017 via email

Uh oh!

xiekeyang commented Feb 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

TrumanLing Oct 31, 2016 •

edited

Loading

TrumanLing Oct 31, 2016 •

edited

Loading