Modify the automatic determination of the Config type #62

zhouhao3 · 2016-10-31T02:14:35Z

The Config attribute may be empty, so cannot be used as the basis for judging the Config type, should use the RootFS is inevitable in this attribute to judge .
Signed-off-by: zhouhao [email protected]

zhouhao3 · 2016-11-11T06:14:23Z

ping @vbatts @wking @xiekeyang @coolljt0725 PTAL

wking · 2016-11-11T08:03:33Z

I have no opinion on peek-inside type detection implementations ;).

zhouhao3 · 2016-11-16T02:46:52Z

PTAL @stevvooe @philips @jbouzane @jstarks

xiekeyang · 2016-11-16T03:50:05Z

image/autodetect.go


-	case header.MediaType == "" && header.SchemaVersion == 0 && header.Config != nil:
-		// config files don't have mediaType/schemaVersion header
+	case header.RootFS != nil:


I think it is good to check rootfs(required) instead config(optional).
However, statement header.MediaType == "" && header.SchemaVersion == 0 had better to be kept, which are also terms differ with other json descriptors.

I think only need a unique attribute can determine the Config type, the other on some superfluous

xiekeyang · 2016-11-17T02:20:28Z

As to this PR, I think it is already good. However, @stevvooe suggested that validation work for JSON files belongs to image-spec repo, which seems to include Auto-detect on JOSN file.

@stevvooe WDYT? This modification is OK, or should be removed to image-spec repo?

stevvooe · 2016-11-18T20:24:45Z

@xiekeyang I am not sure what this is trying to do. Unfortunately, the use of mediaType to detect the type doesn't work, since descriptors describe the target to which they are pointing.

Really, trying to ascertain what a thing is with having a qualified reference presents a host of security problems. Really, you want to instruct the process what the type of the target item is dispatch to a handler. The type resolution is a trusted, deterministic process, either by assuming that all incoming objects are of the same type (such as descriptors) or through a secured mediatype (such as a signed metadata provider).

If we really want content auto detection, we need to have a hard look at the spec, but I don't think is really needed in any real use case.

zhouhao3 · 2016-11-28T02:01:05Z

I think there may be JSON file here to verify the location of the problem, but this method of verification I think it is possible, I have already explained above

xiekeyang · 2016-11-29T10:05:10Z

@stevvooe
I think you are encouraging to remove JSON media detection from image tools, as it is unnecessary, not deterministic and insecure. Right?
I agree that, because current detect use its media and specific object, which really seems not precise. If it is needed, at least it should be done in image spec repo.

stevvooe · 2016-11-30T23:14:22Z

@xiekeyang Correct.

@vbatts Do we have an open issue for this?

zhouhao3 · 2016-12-06T05:55:22Z

Means the JSON related validation into image - spec?

xiekeyang · 2016-12-06T06:57:06Z

Means the JSON related validation into image - spec?

I think so. And media type auto-detection might had better to be removed.

zhouhao3 · 2016-12-07T07:59:07Z

opencontainers/image-spec#481

zhouhao3 · 2017-01-11T08:47:24Z

@xiekeyang @stevvooe What do you think of this change?

stevvooe · 2017-01-12T00:50:43Z

image/autodetect.go

 	case "application/octet-stream":
 		return TypeImage, nil

 	case "text/plain; charset=utf-8":


Where is charset coming from? This looks really wrong.

Uh，This is the original code, the results by http.DetectContentType to determine the format.

Is there any reason autodetect can't be the default? This particular media type doesn't make sense. Media types don't actually have charsets (that is content types).

zhouhao3 · 2017-02-03T07:23:26Z

@xiekeyang @coolljt0725 On this PR, I think I can do is make such a change.PTAL

xiekeyang · 2017-02-07T02:45:58Z

Detection/validation stuff might need a issue involving design spec firstly. Based on which we should get the agreement. And then start to implement.

vbatts · 2017-02-08T19:26:40Z

Is this still needed?

xiekeyang · 2017-02-09T03:23:17Z

Closed.
@q384566678 Could you or others please put up an issue, to discuss about the framework of validation/detection/checking functionality on mediatype, json schema, length of array ...

zhouhao3 · 2017-07-25T05:55:08Z

I reopen this pr because the current auduto is wrong, mediatype has become optional.
ping @opencontainers/image-tools-maintainers PTAL.

stevvooe · 2017-07-25T23:58:18Z

@q384566678 Where does this get used? The autodetection looks right but it will be naive to the introduction of new types.

zhouhao3 · 2017-07-26T01:20:40Z

 {
  "schemaVersion": 2,
  "manifests": [
    {
      "mediaType": "application/vnd.oci.image.manifest.v1+json",
      "size": 622,
      "digest": "sha256:cd4c216fcb2ce4f1e1c18f2d53dd5a00f72b27ae21eaccafabe4c00b86e9326e",
      "platform": {
        "architecture": "ppc64le",
        "os": "linux"
      }
    },
    {
      "mediaType": "application/vnd.oci.image.manifest.v1+json",
      "size": 622,
      "digest": "sha256:cd4c216fcb2ce4f1e1c18f2d53dd5a00f72b27ae21eaccafabe4c00b86e9326e",
      "platform": {
        "architecture": "amd64",
        "os": "linux"
      }
    }
  ],
  "annotations": {
    "com.example.index.revision": "r124356"
  }
}

@stevvooe Above is an example of an index type, if we execute the validate command on it will fail:

➜  image-tools git:(master) ./oci-image-tool validate ../index 
1 errors detected: 
../index: validation failed: unable to determine type: unknown media type

Because through Autodetect can not correctly determine its type.

stevvooe · 2017-07-26T20:49:33Z

@q384566678 But why are we using Autodect here? Isn't this not an index.json where we already know the type?

zhouhao3 · 2017-07-27T01:25:26Z

@stevvooe Because we entered ./oci-image-tool validate ../index instead of ./oci-image-tool validate --type imageIndex ../index . If you enter a type with --type , you do not need to execute the Autodect function, but if you do not enter the validation type, you must use the Autodect function to determine the type.
The following is the relevant code：

	if typ == "" {
		if typ, err = image.Autodetect(name); err != nil {
			return errors.Wrap(err, "unable to determine type")
		}
	}

stevvooe · 2017-07-27T23:59:50Z

@q384566678 What is the use case of autodetect? This seems like the wrong direction.

zhouhao3 · 2017-07-28T09:26:11Z

@stevvooe The function of this function is when you execute the command, if not enter type, to determine the type. And there is no problem.

stevvooe · 2017-07-28T18:07:03Z

@q384566678 What? This seems incorrect. How can you validate something if you don't know the type?

zhouhao3 · 2017-07-29T09:58:47Z

@stevvooe Uh, as I said. if you don't enter the type (or you don't know the type), the Autodect will determine the type according to the path.

zhouhao3 · 2017-08-01T06:21:01Z

@xiekeyang @coolljt0725 PTAL

zhouhao3 · 2017-08-21T01:27:48Z

@xiekeyang @stevvooe @coolljt0725 @cyphar I think this problem needs to have a result, it's been a long time. I still think it's necessary to do this, or there will be validation errors like I said above.

xiekeyang · 2017-08-21T01:37:57Z

I still feel that we should follow [1] and [2].

[1]#62 (comment)
[2]#62 (comment)

stevvooe · 2017-08-22T00:29:33Z

@q384566678 I am not sure how to make this more clear: autodetection of types in OCI images is not a supported concept. It is insecure and broken in practice (see the history of the tar format). If we keep trying to do this, without major design changes, we will have the same broken behavior.

Let's remove autodetection and find ways in which we can accurately and correctly resolve types.

zhouhao3 · 2017-08-22T08:13:52Z

@stevvooe ok, I'll do it.

zhouhao3 · 2017-08-22T08:57:54Z

It is insecure and broken in practice (see the history of the tar format)

@stevvooe Do you have a specific link to this ? I can not find it.Thanks.

zhouhao3 · 2017-08-24T08:49:26Z

@stevvooe Can you explain the above question?

zhouhao3 · 2017-08-29T05:29:05Z

ping @stevvooe

zhouhao3 · 2017-09-04T08:29:58Z

ping @stevvooe @coolljt0725 @xiekeyang How about this revision?

coolljt0725 · 2017-09-07T07:33:05Z

cmd/oci-image-tool/validate.go

 		return schema.ValidatorMediaTypeImageIndex.Validate(f)
 	case image.TypeConfig:
 		return schema.ValidatorMediaTypeImageConfig.Validate(f)
+	case "JSON":


I don't think we introduce a new type JSON is a good idea here, JSON is not any type in image spec. I think it's better to keep the old way.

I also think the previous method is good, and I have not thought of other methods at present, but the suggestion of @stevvooe is to replace the previous method.

Signed-off-by: zhouhao <[email protected]>

Mashimiao · 2017-11-10T05:14:48Z

I think we should close this PR.
Though we may have different opinions on keep autodetect for image or not in #184,
but we all agree that we should remove autodetect for image files, Right?
So, I think we don't need this PR any more.
@vbatts @stevvooe @xiekeyang @coolljt0725

stevvooe · 2017-11-21T02:56:05Z

@Mashimiao I think we can close this.

zhouhao3 force-pushed the test-autodetect branch 3 times, most recently from a5a3b45 to 45af0d8 Compare November 11, 2016 05:53

xiekeyang reviewed Nov 16, 2016

View reviewed changes

zhouhao3 force-pushed the test-autodetect branch from 45af0d8 to 542326c Compare December 7, 2016 07:36

zhouhao3 mentioned this pull request Dec 7, 2016

validate.go: add autodetect opencontainers/image-spec#481

Closed

zhouhao3 force-pushed the test-autodetect branch 3 times, most recently from 99dcb01 to 61daf0d Compare December 7, 2016 08:29

stevvooe reviewed Jan 12, 2017

View reviewed changes

zhouhao3 force-pushed the test-autodetect branch 2 times, most recently from 17c1b4a to 6017ccc Compare February 3, 2017 07:07

xiekeyang mentioned this pull request Feb 9, 2017

image: make layer validation work #111

Merged

xiekeyang closed this Feb 9, 2017

zhouhao3 force-pushed the test-autodetect branch from 6017ccc to ad2fc03 Compare July 25, 2017 05:52

zhouhao3 force-pushed the test-autodetect branch 2 times, most recently from f85c031 to 83a481d Compare August 29, 2017 05:55

coolljt0725 reviewed Sep 7, 2017

View reviewed changes

This was referenced Sep 20, 2017

validate: remove autodetect for type and add require limit #184

Merged

ROADMAP for image-tools #187

Open

Modify the automatic determination of the Config type

9795a19

Signed-off-by: zhouhao <[email protected]>

zhouhao3 force-pushed the test-autodetect branch from 83a481d to 9795a19 Compare November 10, 2017 02:44

Mashimiao closed this Nov 21, 2017

zhouhao3 deleted the test-autodetect branch November 21, 2017 03:08

Modify the automatic determination of the Config type #62

Modify the automatic determination of the Config type #62

Uh oh!

Conversation

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

Uh oh!

Uh oh!

zhouhao3 commented Nov 11, 2016

Uh oh!

wking commented Nov 11, 2016 via email

Uh oh!

zhouhao3 commented Nov 16, 2016

Uh oh!

xiekeyang Nov 16, 2016

Choose a reason for hiding this comment

Uh oh!

zhouhao3 Nov 16, 2016

Choose a reason for hiding this comment

Uh oh!

xiekeyang commented Nov 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stevvooe commented Nov 18, 2016

Uh oh!

zhouhao3 commented Nov 28, 2016

Uh oh!

xiekeyang commented Nov 29, 2016

Uh oh!

stevvooe commented Nov 30, 2016

Uh oh!

zhouhao3 commented Dec 6, 2016

Uh oh!

xiekeyang commented Dec 6, 2016

Uh oh!

zhouhao3 commented Dec 7, 2016

Uh oh!

zhouhao3 commented Jan 11, 2017

Uh oh!

stevvooe Jan 12, 2017

Choose a reason for hiding this comment

Uh oh!

zhouhao3 Jan 12, 2017

Choose a reason for hiding this comment

Uh oh!

stevvooe Jan 12, 2017

Choose a reason for hiding this comment

Uh oh!

zhouhao3 commented Feb 3, 2017

Uh oh!

xiekeyang commented Feb 7, 2017

Uh oh!

vbatts commented Feb 8, 2017

Uh oh!

xiekeyang commented Feb 9, 2017

Uh oh!

zhouhao3 commented Jul 25, 2017

Uh oh!

stevvooe commented Jul 25, 2017

Uh oh!

zhouhao3 commented Jul 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stevvooe commented Jul 26, 2017

Uh oh!

zhouhao3 commented Jul 27, 2017

Uh oh!

stevvooe commented Jul 27, 2017

Uh oh!

zhouhao3 commented Jul 28, 2017

Uh oh!

stevvooe commented Jul 28, 2017

Uh oh!

zhouhao3 commented Jul 29, 2017

Uh oh!

zhouhao3 commented Aug 1, 2017

Uh oh!

zhouhao3 commented Aug 21, 2017

Uh oh!

xiekeyang commented Aug 21, 2017

Uh oh!

zhouhao3 commented Oct 31, 2016 •

edited

Loading

xiekeyang commented Nov 17, 2016 •

edited

Loading

zhouhao3 commented Jul 26, 2017 •

edited

Loading