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

[release/7.0] Fix PAX extended attribute reading logic to treat '=' character as valid in the value strings. #83177

Merged
merged 5 commits into from
Mar 10, 2023

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Mar 9, 2023

Backport of #82810 to release/7.0

/cc @carlossanlop

Customer Impact

The dotnet/sdk-container-builds team reported that they were unable to read TAR entries in the PAX format that contained the = character in their extended attribute metadata.

In the PAX TAR format, each entry representing a filesystem object is always preceded by an entry containing only metadata information in its data section. This metatada is known as extended attributes, and they are stored in the form of key-value-pairs, where the separator character between keys and values is the = character, and the separator of one key-value with another is the \n character.

The sdk-container-builds team is using those extended attributes to store Windows Security Descriptor data. An example of a value that they were unable to read is:

  • Key: [MSWINDOWS.rawsd](https://github.com/buildpacks/rfcs/blob/main/text/0076-windows-security-identifiers.md)
  • Value: AQAAgBQAAAAkAAAAAAAAAAAAAAABAgAAAAAABSAAAAAhAgAAAQIAAAAAAAUgAAAAIQIAAA==

The root cause of this bug is that we were explicitly disallowing any = characters in the value part of the key-value-pair, after detecting the very first one and considering it the separator. In reality, the TAR spec does not specify that this restriction should be in place, and we should allow any number of = characters in the value section, since we only care about the first one.

So the fix was to remove the code block that was preventing the use of more than one =.

Testing

  • Manually compared the behavior of our APIs to that from the Ubuntu gnu tar tool to ensure the behavior was not unexpectedly different.
  • Manually tested the APIs with the code fix to ensure we could write and read entries with such metadata.
  • Added unit tests to verify that we are able to accept key-value-pairs that contain various combinations of = characters in the value string, both in our sync and async APIs.
  • Added round-trip tests to ensure we can write and read entries with this kind of metadata.

Risk

Low.

The fix just removes an if block that prevented a desired behavior.

Even though the PAX format is the default one in our TarFile APIs, users can only specify custom extended attributes when manually constructing a TAR archive using the advanced APIs: a TarWriter instance, and the PaxTarEntry constructor that takes a dictionary.

No OOB changes needed for this assembly.

Notes

Fixing this issue made us discover that we could add more robust validation in the PaxTarEntry constructor that takes a dictionary of key-value-pairs to use as extended attributes, to safe-guard users from creating TAR archives that contain forbidden characters in these values. We will merge that fix in .NET 8. It may not meet the bar for a backport by itself.

@carlossanlop carlossanlop marked this pull request as draft March 9, 2023 02:31
@carlossanlop carlossanlop self-assigned this Mar 9, 2023
@carlossanlop carlossanlop added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it labels Mar 9, 2023
@carlossanlop carlossanlop added this to the 7.0.x milestone Mar 10, 2023
@carlossanlop carlossanlop removed NO-REVIEW Experimental/testing PR, do NOT review it NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) labels Mar 10, 2023
@carlossanlop carlossanlop marked this pull request as ready for review March 10, 2023 01:20
@carlossanlop carlossanlop added the Servicing-consider Issue for next servicing release review label Mar 10, 2023
@carlossanlop carlossanlop modified the milestones: 7.0.x, 7.0.5 Mar 10, 2023
@carlossanlop carlossanlop added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Mar 10, 2023
@carlossanlop
Copy link
Member

Approved by Tactics via email.
Signed-off.
No OOB changes needed.
CI failure is unrelated.
Ready to merge. :shipit:

@carlossanlop carlossanlop merged commit 5b74781 into release/7.0 Mar 10, 2023
@carlossanlop carlossanlop deleted the backport/pr-82810-to-release/7.0 branch March 10, 2023 23:35
@ghost ghost locked as resolved and limited conversation to collaborators Apr 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Formats.Tar Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants