compression: gzip on inline/local contents is accepted without checking whether the contents are actually gzipped

[yasminvalim]

Created originally in Butane by @w453y: coreos/butane#730

---

Butane accepts compression: gzip on an inline or local file resource whose contents are not gzipped, and emits the declaration unchanged. The resulting config is valid as far as ignition-validate is concerned, and fails at first boot instead.

Butane already has these contents in memory and inspects them, it compresses them itself when that yields a smaller data URL, and sets compression accordingly. When the user sets the field instead, MakeDataURL takes it as a statement that the contents are already compressed (base/util/url.go, the else branch of util.NilOrEmpty(currentCompression)) and skips its own compression path. A gzip stream is identifiable from its first two bytes (1f 8b, RFC 1952), so the mismatch seems cheaply detectable at translation time.

Severity is low, I want to be upfront about that. The runtime failure is clean: I checked on both Flatcar and Fedora CoreOS and Ignition reports gzip: invalid header, names the file and the stage, fails ignition-files.service, and drops to an emergency shell. So this is about when the user finds out, not whether they can. The expectation stated in coreos/butane#332, that a mismatched declaration produces an Ignition error at runtime, does hold.

This came out of writing a cloud-init to Butane transpiler, where cloud-init's write_files encoding: gzip+b64 has to be turned into either plain contents or data:;base64, plus compression: gzip. Getting that choice backwards produces a config that passes --strict and fails at boot, which is why I went looking.

Reproduction

butane 0.29.0.

variant: flatcar
version: 1.1.0
storage:
  files:
    - path: /etc/t
      mode: 0644
      contents:
        inline: not actually gzipped
        compression: gzip

$ butane --pretty --strict < gz.bu
{
  "ignition": { "version": "3.4.0" },
  "storage": {
    "files": [
      {
        "path": "/etc/t",
        "contents": {
          "compression": "gzip",
          "source": "data:,not%20actually%20gzipped"
        },
        "mode": 420
      }
    ]
  }
}

Same with local:, where Butane reads the file itself. variant: fcos / version: 1.5.0 behaves identically.

For comparison, without the declaration Butane compresses the same content itself when it is worth it, e.g. 5000 bytes of A comes back as data:;base64,H4sIA... with compression: gzip set, so the magic bytes are something Butane produces in its own path a few lines further down.

Suggested fix

For inline and local only, where the contents are available: reject a user-supplied compression: gzip when the content does not start with 1f 8b.

Deliberately not suggesting:

• anything for source resources, since Butane cannot see remote content and cannot second-guess a child config's intent (the reasoning in docs: split configuration specs by version #332).
• the inverse check (flagging undeclared content that is gzipped), since docs: split configuration specs by version #332 explicitly excepts users who intend to write gzip-compressed data.

Related: coreos/butane#100, coreos/butane#123, coreos/butane#124, coreos/butane#244, coreos/butane#332, coreos/butane#341. coreos/butane#244 is the closest, the openshift variants already fail when compression is manually configured, though for a different reason.

Happy to send a PR if this seems worth doing.

[w453y]

Thanks for migrating @yasminvalim , two things from the original thread that didn't carry over:

@angelcerveraroldan and I discussed whether compression: gzip should mean "this is already gzipped" (validate, error on mismatch) or "gzip this for me". Conclusion was to keep current semantics and validate, with @prestist to confirm there's no deliberate reason for the passthrough.

The open question before implementing: it breaks TestTranslateFile/translate_1 in all seven base/ spec packages, since those fixtures set compression: gzip over plain text. Should the check apply to every spec, or only v0_8_exp and later?

[w453y]

@yasminvalim one small thing, the issue references (#100, #123, #124, #244, #332, #341) now resolve against this repo rather than coreos/butane. Could you prefix them with coreos/butane#?

Thanks!!!

[yasminvalim]

@w453y I added the 'coreos/butane#' prefix to those PR titles. Is this what you meant? We could also create a 'coreos/butane' label to add to the PRs if you prefer. Let me know if that works!

[w453y]

@yasminvalim No, I didn't mean that. I just want you to change the hyperlink for those PRs/Issues mentioned in the main description. Typically, in issues, we use #<some_number> to address a particular PR, which automatically resolves to the current repo. However, since we have migrated (basically copied the same markdown from another repo) from the earlier issue, #<some_number> resolves to this repo, and those PRs don't make sense for this issue at all.

All you have to do is edit the issue description and add the "coreos/butane" prefix to the numbers. For example, we have #332 (this resolves to the current repo's PR, which is of no use), but if I prefix it with coreos/butane#332, then it points to the original PR, which was meaningful for this issue.