serialization: Drop rootfs.type (which had only one legal value)

[wking]

With 0c52d5d (serialization: remove windows-specific layers+base rootfs, 2016-08-26, #211) this field became a single-choice field. We could keep the field to make the current "layers" approach explicit in the face of future rootfs types, but we'd have to solidify the wording around it to cover what should happen when rootfs.type was not set, or if rootfs.type was an unrecognized value. It seems easier to remove it and make the "layers" approach implicit default, so that's what this commit does. We can restore the field later if/when we have another rootfs type without breaking forward compatibility.

Code translating between Docker and OCI image renderings can drop/restore this field while it is translating Docker ↔ OCI media types, so I don't expect compatibility issues from this change.

Spun off from this discussion.

[philips]

cc @stevvooe

[stevvooe]

👎

I made my opinion clear in #211 (comment). I'll repeat here, since you seem to have missed it:

This field was added as part of the modifications that came out with Docker 1.10 to address long standing image format issues. The rootfs provide image provenance over the root filesystem components that make up a given image. The type field provides some measure of algorithmic agility. Removing this field probably has unintended consequences.

Recently, this value for the field was removed from the docker landscape due to a better solution for identifying base image layers that could not be distributed. The field value would never have been seen in production ready versions of docker and has no place in OCI.

Note that, in practice, there is the possibility of encountering other values of this field, but it is unlikely. However, if they are encountered, that conversion cannot safely proceed. Removing this field is more likely to be actively harmful than leaving it in place.

I'm not defending whether or not this field is useful. I'm trying to stem the tide of unnecessary change. This is particularly pathological, as it is subtly different and not actively harmful.

[philips]

@stevvooe I am having a hard time understanding the meaning of this response. You are saying that this field is never used and shouldn't be interpreted but we should keep it so we don't drop it in serialization and deserialization? Sorry, just having a difficult time understanding the danger in dropping it. Is there a concrete example?

[wking]

On Wed, Aug 31, 2016 at 01:54:04PM -0700, Stephen Day wrote:

I made my opinion clear in
#211 (comment). I'll
repeat here, since you seem to have missed it…

I saw it, I'm just not convinced yet. Handling this issue by itself
in this PR should let us get to the bottom of our concerns without
holding up other work.

… Removing this field probably has unintended consequences.

I was looking for some details behind “unintended consequences” 1,
since I don't see any.

Note that, in practice, there is the possibility of encountering
other values of this field, but it is unlikely. However, if they are
encountered, that conversion cannot safely proceed. Removing this
field is more likely to be actively harmful than leaving it in
place.

The current image-spec wording does not say anything like “if you see
an unrecognized value, do …”. I'm not even sure what you're
suggesting should happen in that case. “cannot safely proceed” sounds
like “die with an error” (which makes sense to me). “leaving it in
place” sounds like “silently pass unrecognized values” (which does not
make sense to me).

I'm not defending whether or not this field is useful. I'm trying to
stem the tide of unnecessary change.

This spec is seeded by the Docker spec; it's not bound to exactly
match the Docker spec. When the translation is unambiguous (replace
media types following this mapping, add/remove rootfs.type, …), I
don't see a problem with divergence. Where that divergence makes the
OCI spec simpler (and keeps the mapping deterministic), I think we
want to do that.

[jonboulle]

I'm also having great difficulty understanding the rationale for keeping
this; if this value would never have been seen in production ready versions
of Docker, what is the concern? It seems like an extreme/obscure edge case.
If we really should retain it for some reason we should explicate why in
the spec.

On 31 August 2016 at 22:54, Stephen Day [email protected] wrote:

👎

I made my opinion clear in #211 (comment)
#211 (comment).
I'll repeat here, since you seem to have missed it:

This field was added as part of the modifications that came out with
Docker 1.10 to address long standing image format issues. The rootfs
provide image provenance over the root filesystem components that make up a
given image. The type field provides some measure of algorithmic agility.
Removing this field probably has unintended consequences.

Recently, this value for the field was removed from the docker landscape
due to a better solution for identifying base image layers that could not
be distributed. The field value would never have been seen in production
ready versions of docker and has no place in OCI.

Note that, in practice, there is the possibility of encountering other
values of this field, but it is unlikely. However, if they are encountered,
that conversion cannot safely proceed. Removing this field is more
likely to be actively harmful than leaving it in place.

I'm not defending whether or not this field is useful. I'm trying to stem
the tide of unnecessary change. This is particularly pathological, as it is
subtly different and not actively harmful.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#224 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACewN9p2X_fiqOQ4pgJTY-vlvul6LJVyks5qlentgaJpZM4JwxBj
.

[stevvooe]

I never said it wasn't interpreted. Docker uses it to internally interpret the meaning of rootfs. The field's role is to declare the meaning of the DiffIDs array. If this gets modified or fudged in any way, there is the possibility to open the format to malicious injection.

The fact is, the field exists in the wild, with varying values, for Windows container images. Though it isn't in "production", it may be seen as part of an earlier release. If these diff ids are interpreted incorrectly, an image may be incorrectly assembled. Retaining this field with this values makes it absolutely explicit what these diff ids actually mean.

This is particularly dangerous due to the way Go processes JSON, which would lead to this field being ignored in practice. If a config with a different value is encountered in the wild, it will be impossible to detect without making that definition explicit. Are you willing to take a risk that code converting from the docker image format always gets this subtlety just right?

I understand this may offend one's sense of aesthetics but leaving this in place and making it explicit is just the safest option.

[philips]

@stevvooe Do you have links to the code or whatever that caused this issue? Then we can make a big footnote for future readers.

[stevvooe]

@philips Yea, let me see what I can find here.

[wking]

On Wed, Aug 31, 2016 at 05:20:15PM -0700, Stephen Day wrote:

The field's role is to declare the meaning of the DiffIDs array.
If this gets modified or fudged in any way, there is the
possibility to open the format to malicious injection.

But as image-spec stands (with or without this PR), there's only one
possible meaning for the DiffIDs array.

Retaining this field with this values makes it absolutely explicit
what these diff ids actually mean.

As does not setting the field and referencing the config with a
descriptor holding the
application/vnd.oci.image.serialization.config.v1+json media type.

This is particularly dangerous due to the way Go processes JSON,
which would lead to this field being ignored in practice.

With the language in-flight in #164 (which I think should cover the
config JSON too 1), such unrecognized keys MUST NOT lead to errors.
If somebody points to a Docker config (including the rootfs.type
field, possibly with some other value) and claims it is an
application/vnd.oci.image.serialization.config.v1+json, they should
not be surprised when the tooling treats the referenced config as an
OCI config and ignores the Docker-only field.

The right approach is to translate the Docker config to an OCI config,
and then point at the OCI config and claim it's an
application/vnd.oci.image.serialization.config.v1+json. But the
current PR doesn't make this clear. I'll add notes to 2 tomorrow to
clarify the new translation step.

[stevvooe]

@wking I understand what you're saying and you're right from a theoretical perspective. However, we need to take a harm reduction approach to security. Introducing a potential security land mine then saying "whoops, should have read this part more carefully" isn't acceptable. The result is bad and we've gained nothing from having a "simpler" specification.

[wking]

On Wed, Aug 31, 2016 at 06:52:40PM -0700, Stephen Day wrote:

Introducing a potential security land mine then saying "whoops,
should have read this part more carefully" isn't acceptable.

So let's try and figure out what the side effects of a sloppy
translation are. In the current docker/docker master:

$ git log --oneline -G 'layers+base'
f342b27 Removing old custom RootFS behavior on Windows.
4fa0ecc Update image specification for content-addressability changes
d45a26d Windows: Support non-base-layered images
94d70d8 Windows libcontainerd implementation
6bb0d18 Move Container to its own package.
4352da7 Update daemon and docker core to use new content addressable storage
01ba0a9 Add image store
$ git describe --contains 01ba0a9
v1.10.0-rc1520^24
$ git describe --contains f342b27
fatal: cannot describe 'f342b27145d8f5af27cd5de1501551af275e899b'
$ git describe --tags f342b27
docs-v1.12.0-rc4-2016-07-15-403-gf342b27

So the removal will presumably land in Docker 1.13. That means that
Docker ≥ 1.10 and < 1.13 will respect (and possibly generate) images
using the layers+base type.

What were the semantics of the layers+base type, and what happens if
you interpret a layers+base config as a layers config? I'm not
familiar enough with the Docker code to get much from reading
moby/moby#25090, but I expect the fallout is that the rootfs is
built without the base layer underneath it. I expect the base layer
is a critical piece of any Windows images that used it, and containers
launched with a rootfs that was missing that expected base layer would
immediately die due to missing libraries or similar. That seems like
a sane way to handle “you botched the translation to an OCI format”.

Or is the base layer just some security patches, and layers+base
containers missing that base layer will silently run with those
security issues unpatched? If so, can you find an image like this?

[wking]

On Wed, Aug 31, 2016 at 06:44:57PM -0700, W. Trevor King wrote:

I'll add notes to [2] tomorrow to clarify the new translation step.

Added with cb9a5a9 → 5d032cb, which also rebases onto the (rapidly
evolving) master ;).

[stevvooe]

I've received a little more information on the role of this field. It actually selects the hashing algorithm used for the image ID/rootfs. If that algorithm changes, this field changes value. Removing it will make otherwise compatible images completely incompatible.

What reason do you have for making this incompatible? Is this just really about field hygiene?

[wking]

On Fri, Sep 02, 2016 at 03:35:44PM -0700, Stephen Day wrote:

I've received a little more information on the role of this
field. It actually selects the hashing algorithm used for the image
ID/rootfs.

Can you link to code? I'm not sure why you'd need something other
than the manifest digest for an image ID.

If that algorithm changes, this field changes value. Removing it
will make otherwise compatible images completely incompatible.

It's hard to say without looking at code, but if the field consumers
have a valid reason for switching hashing algorithms based on this
field, I expect we want to keep the field and document that logic.
If they use different hashing agorithms based on historical reasoning
that no longer holds water, then I'm likely to still be ok dropping
the field (but it would be nice to understand what “completely
incompatible” meant in more detail).

And translation is still deterministic and straightforward 1. I am
absolutely fine with making translation a concious choice, otherwise
what's the point of having two different specs? We'd just have the
OCI cite Docker (or vice versa).

What reason do you have for making this incompatible? Is this just
really about field hygiene?

Yes. Each property defined by the spec should have a clear purpose
and well-defined behavior. Currently rootfs.type has neither. In
that case, I think it's good to remove it, and see what happens.
You're arguing strongly for keeping it, but I still don't have a clear
handle on what goes wrong if we drop it (and users forget to
translate? And translating is somehow more difficult than 1?).
If/when you paint a clear picture for why the property is needed, I
think we want it in the spec with that picture. For example:

type (required, string) One of layers, other-value. This
selects the hashing algorithm used for the image
ID/roofs, which allows you to {FIXME:
why do we care?}. Libraries unpacking images MUST generate en error
if they do not recognize the value.

[stevvooe]

@wking At this point, I have painted a very clear picture of why this field is needed and you're simply rejecting the premises.

I'm sorry, but you're creating work and subtlety for no good reason. This PR and the implications that come with removing this field are a massive waste of time.

[wking]

On Fri, Sep 02, 2016 at 04:01:11PM -0700, Stephen Day wrote:

@wking At this point, I have painted a very clear picture of why
this field is needed and you're simply rejecting the premises.

Ah, you're saying 1 that touching the config at all changes the
ImageID 2. That makes a lot of sense (obviously the config hash
will change if you remove a field from the config JSON). What's the
downside to that? You get a different config hash, which gives you a
different manifest hash (but you already have a different manifest
hash, because the manifest layer descriptors have Docker→OCI media
types). The config JSON is tiny, so refetching that blob isn't a
big cost.

Maybe there are systems which handle trust by signing configs? In
that case, you could translate the rest of the Merkle tree to OCI
values, and as long as the config didn't change, those old
signatures would still validate [3]. Those signatures would break if
the registry removed the rootfs.type property.

But if the rootfs.type value is ‘layers’, the registry can leave the
config alone and call it an application/vnd.oci.image.config.v1+json.
With the wording in flight with #164 (which I still 4 think should
cover the config JSON too 5), implementations would ignore the
unknown property.

And if the rootfs.type value is not -defined-in-this-spec- [edit: ‘layers’] or an
extension understood by the implementation, then you're going to have
to get a new signature after a more invasive config translation.

Is that config-signature-preservation issue the only problem you see?
I'm personally fine requiring folks who don't use ‘layers’ to re-sign
after translating to an OCI config format. Folks using images that
rely on other values 6 should avoid using OCI-only implementations
for those images until they've had time to convert them.

[3]: I'm concerned about tarbomb exposure in that case and would like
to see signatures on at least the manifest level, but that's a
different issue.

[jonboulle]

I think @wking is raising a lot of valid points here. @stevvooe can you please give a tangible example of how this can cause a security issue? If it really is a potential landmine then we should be crystal clear in the spec about why it's so important so implementers can take appropriate caution. As it stands the implications are not obvious.

[stevvooe]

@jonboulle The value type=layers expresses the ChainID hash algorithm used as the image ID. If this is incorrect, it will change how the image is identified and verified in certain implementations. Put differently, for the purposes of image verification, it can only be used if and only the value of type is equal to the value layers. Making assumptions about this value when it is omitted leaves the possibility of error when addressing possible future security concerns. This is particularly problematic if there is a vulnerability found in both docker and OCI. If docker's import assumes the value is layers for OCI images but it was really updated due to some change, these images may be imported with malicious identifiers.

If you're asking for an example of an active exploit that involves some contrived sequence of events that lead to an injection, you're going to be disappointed. That simply isn't how security works. If we knew there was an exploit, we'd suggest a full fix for it. In this case, this PR is suggesting that we take implicitly, what was explicit before, which creates a possible risk that needs mitigation.

The safest option here is to leave the field in place and mark for removal in a later version if it can be shown that it is truly not needed. The best part is that this option requires no work at all, other than me, sitting here, asserting that this community should not take undue security risks.

The approach here is not dissimilar to putting ashtrays in airplane lavatories. They may look useless but they prevent the plane from catching on fire for the one asshole that doesn't follow the rules and smokes anyways. Just because the implications of removing the ash tray aren't obvious doesn't mean we remove the ash tray.

[stevvooe]

Here is an example of value of Type causing unspecified problems: moby/moby#25806. Likely fix, microsoft/docker@710b456, is to add an explicit filter on a value of Type.

[wking]

On Tue, Sep 06, 2016 at 12:56:27PM -0700, Stephen Day wrote:

@jonboulle The value type=layers expresses the ChainID hash
algorithm used as the image ID.

The code in 1 is not compatible with our ChainID definition 2,
because 2 does not say:

ChainID(layerN) is always the empty string if rootfs.type is
‘layers+base’ and the implementation OS is Windows.

Which seems like a pretty crazy thing to say (1 is pretty
straightforward, why such an odd special case?).

If this is incorrect, it will change how the image is identified and
verified in certain implementations. Put differently, for the
purposes of image verification, it can only be used if and only the
value of type is equal to the value layers. Making assumptions
about this value when it is omitted leaves the possibility of error
when addressing possible future security concerns.

I'm not clear on how the ChainID is ever useful for validating
images. It seems like it would only be useful for validating stacks
of layers, and then it's (obviously?) only useful for validating the
layers that you tell it about. The general principle is:

When naming an object, hash something high enough up the Merkle tree
to contain all that object's constituent parts.

I don't think the ChainID would ever fit that criterion for an image
(because it was missing ‘user’, etc.). If there are config media
types which allow layers+base but did not contain a reference to base,
then signing one of those config instances would not fit that
criterion either.

This is particularly problematic if there is a vulnerability found
in both docker and OCI. If docker's import assumes the value is
layers for OCI images but it was really updated due to some
change, these images may be imported with malicious identifiers.

If the OCI updates the config spec with different layer semantics, we
had better be bumping our media type too. If Docker imports a
application/vnd.oci.image.config.v2+json (where the correct Docker
analog is not rootfs.type = layers), and says “hmm, never heard of OCI
config v2. I'll just use my OCI config v1 importer logic”, then that
is pretty obviously Docker's problem, and not a security flaw in the
OCI spec.

One way to avoid confusion would be to clearly document (and provide a
Go API for?) which media types do meet the constituent parts criterion
for naming images. With the current OCI spec, that includes
application/vnd.oci.image.manifest.v1+json,
application/vnd.oci.image.manifest.list.v1+json, and (if you don't
mind the tarbomb exposure 3 or the inability to lookup blobs
directly by DiffID) application/vnd.oci.image.config.v1+json.

[stevvooe]

@wking I am not going to repeat my points again, so I would please ask that you take some time to read and understand my position before responding.

[mikebrow]

`/s/shuold/should/

[wking]

On Wed, Sep 07, 2016 at 02:31:48PM -0700, Mike Brown wrote:

•  Translators converting OCI configurations to Docker v1.1 or v1.2 configurations shuold always set `rootfs.type` to `layers`.
  

`/s/shuold/should/

Thanks. Fixed in 7402389 → 8f8e481.

[philips]

@wking wants to clarify that things MUST error out if this field is missing. @stevvooe and @wking will work to close this one and open a new one with that error out language or equivalent.

[wking]

On Wed, Sep 07, 2016 at 02:32:26PM -0700, Brandon Philips wrote:

@wking wants to clarify that things MUST error out if this field is
missing. @stevvooe and @wking will work to close this one and open a
new one with that error out language or equivalent.

I've filed this alternative approach as #277 (is that 50 issues/PRs in
the last 9 days? I hope that settles down before we start cutting 1.0
RCs ;). I'm fine with having this PR closed now, or it can wait until
#277 lands (or maybe folks will swing back over to this approach after
we kick #277 around for a bit ;).

[stevvooe]

@wking Closing this PR. If #277 stalls, we can re-open.