[RFC 0076] Export should set SID of files in windows layers

[ekcasey]

RFC 0076
buildpacks/rfcs#133

• Replace CNB_USER_ID and CNB_GROUP_ID with CNB_USER_SID and CNB_GROUP_SID on windows stack images
• Replace -uid and -gid flags with -usid and -gsid flags for builds in windows environments
• Windows builds should check directory ownership and drop privileges when necessary (analagous to the current linux behavior) using these new inputs

02/17/2021 - Updated to reflect the result of the RFC above

[micahyoung]

That looks great!

TLDR

I feel like there's a couple key questions to answer about this:

Are we ok changing CNB_USER_ID/CNB_GROUP_ID (and creator -uid <int>) to take strings ("S-1-5-93-2-1") for Windows and integers for Linux?

If not, should we explore trying allow a stack author to express express an SID as an integer (i.e. CNB_USER_ID=9298495...)?

Separately, are we ok making ContainerUser and ContainerAdministrator be the de-facto USER options for stack authors in Windows?

If not, do we have alternative suggestions or architectural change ideas to support not-knowing a USER's SID before they're created?

Background context

We implemented a very minimal version based on groups instead of users in buildpacks/imgutil#47, which reads the header.Uid and does a very basic conversion into generic groups in header. PAXRecords["MSWINDOWS.rawsd"] instead basing it on actual Windows user's SID.

The format of an SID is like S-1-5-93-2-2 whose spec is described here in Microsoft docs.

S-R-I-S…
In this notation, the literal character "S" identifies the series of digits as a SID, R is the revision level, I is the identifier-authority value, and S… is one or more subauthority values.

An SID can either refer to a user or a group, and any file/directory can have its Owner and Group be any combination of a User or Group SID.

For lifecycle's purposes, it will need to get the SID from the platform, interpolate it into a Security Descriptor Definition Language string, then convert that into a base64-encoded Raw Serial Descriptor PAX record for header.PAXRecords["MSWINDOWS.rawsd"] with an implementation like this (ex: powershell is used here but this would have to be ported to Golang):

$sid = "S-1-5-93-2-2"
$sddlValue = "O:${sid}G:${sid}"
$sddl = (ConvertFrom-SddlString $sddlValue)
$sddlBytes = [byte[]]::New($sddl.RawDescriptor.BinaryLength)
$sddl.RawDescriptor.GetBinaryForm($sddlBytes, 0)
[Convert]::ToBase64String($sddlBytes)
# Output:
# AQAAgBQAAAAoAAAAAAAAAAAAAAABAwAAAAAABV0AAAACAAAAAgAAAAEDAAAAAAAFXQAAAAIAAAACAAAA

The first tricky part might be how to express SIDs as ENV values for CNB_USER_ID. I don't see the spec requiring integers so we could potentially make have CNB_USER_ID (or creator -uid <int>) accept multiple types of values: SID strings for Windows images, integers for Linux images.

If we really wanted SIDs to be serialized as integers though, I did some searching around and couldn't find any de-facto examples or conventions for expressing SIDs as ints in consts or whatnot but Golang does have some windows/syscall SID conversion helpers we could play around with if we did want to try casting it to an int, though a large SID may contain more bytes than even an int64 could contain.

The second tricky part (though this may be and issue for platforms, not lifecycle) is enabling a stack author to know their USER's SID in the first place... the Windows command line tools for creating users/groups cannot specify a pre-determined SID to use, instead Windows itself always chooses a new SID for you. Docker could help us out because they hard-code SIDs for two special users ContainerUser -> S-1-5-93-2-2 and ContainerAdministrator -> S-1-5-93-2-1 source. If the USER were set to ContainerUser and the CNB_USER_ID/CNB_GROUP_ID were set to some expression of S-1-5-93-2-2 then this could work pretty well. Those users are very appropriate and unlikely to change, however the cannot be renamed to something like "pack" using any command that i know of (though we could perhaps do this via registry).

But if a stack author didn't want to use those built-in users, and needed to create a user via a RUN command, then I'm not sure there's a great solution.

[ekcasey]

@micahyoung

The format of an SID is like S-1-5-93-2-2 whose spec is described here in Microsoft docs.

S-R-I-S…
In this notation, the literal character "S" identifies the series of digits as a SID, R is the revision level, I is the identifier-authority value, and S… is one or more subauthority values.

It seems like, given that the SID is not an integer, our current logic (set header.UID to CNB_USER_ID) would either break if users set CNB_USER_ID=<SID>, or at least produce nonsensical results. I don't think the imgutil PAX logic is being applied right now because we do set PAXRecords (see #340) https://github.com/buildpacks/imgutil/blob/main/layer/windows_writer.go#L42-L46

I wonder if the right long-term call here is to change the spec to accept CNB_SECURITY_ID and or a -sid flag when building windows images (instead of CNB_USER_ID and -uid). It seems more straightforward/explicit than converting ints?

As a stopgap maybe we should just apply generic docker ContainerUser SID whenever we would use CNB_USER_ID and the docker ContainerAdministrator SID whenever we would use the actual FS uid on linux.

[micahyoung]

I don't think the imgutil PAX logic is being applied right now because we do set PAXRecords (see #340) https://github.com/buildpacks/imgutil/blob/main/layer/windows_writer.go#L42-L46

Good call, I totally missed that PAXRecords are always initialized. That will break k8s Windows containers but Docker for Windows (on Win10 at least) won't care. I'll make an issue and PR to imgutil to always add MSWINDOWS.rawsd (but still initialize PAXRecords, if nil).

I'd love to put an acceptance-level regression test somewhere to catch this type of error ... I'll start a discussion on the yet-to-be-created thread.

I wonder if the right long-term call here is to change the spec to accept CNB_SECURITY_ID and or a -sid flag when building windows images (instead of CNB_USER_ID and -uid). It seems more straightforward/explicit than converting ints?

Yeah, giving the SID its own flag eventually makes sense. What do you think about CNB_SECURITY_DESCRIPTOR instead, which is the format the the SID gets converted into (see powershell example above)?

At it's simplest could be (O:S-1-5-93-2-2G:S-1-5-93-2-2 for ContainerUser) or stack authors could get as fine-grained with permissions as the want (buildx example) and we'd just apply that to each cnb-owned file.

As a stopgap maybe we should just apply generic docker ContainerUser SID whenever we would use CNB_USER_ID and the docker ContainerAdministrator SID whenever we would use the actual FS uid on linux.

Good call. I feel ContainerUser and ContainerAdministrator are safe to rely on for a stopgap. They're always defined in any container and very idiomatic for Windows - I've never seen a real-world exception to using them (outside the CF Windows rootfs). Microsoft even activates some extra k8s functionality that can only be used when running as those users.

But could you elaborate on the proposed stopgap implementation? I feel like we could still translate them in imgutil and just be very binary about it:

• When header.Uid == 0 && header.Gid == 0 (aka root-owned in pack)
  • Owner: ContainerAdministrator
  • Group: ContainerAdministrator
  • SDDL: O:S-1-5-93-2-1G:S-1-5-93-2-1
• When header.Uid == 1 && header.Gid == 1 (aka cnb-owned in pack)
  • Owner: ContainerUser
  • Group: ContainerUser
  • SDDL: O:S-1-5-93-2-2G:S-1-5-93-2-2
• Else:
  • Error "cannot translate permissions on Windows for UID: x; GID: x"

FWIW, these are practically the same SDDLs added when running PS:> New-Item c:\windows\temp\foo as ContainerUser/ ContainerAdministrator respectively.

Would this be acceptable or were you thinking something else?

[ekcasey]

What do you think about CNB_SECURITY_DESCRIPTOR instead, which is the format the the SID gets converted into (see powershell example above)?

Makes sense to me.

When header.Uid == 1 && header.Gid == 1 (aka cnb-owned in pack)

• Owner: ContainerUser
• Group: ContainerUser

Yes, we could just set UID/GID to 0 or 1 and let imgutil translate. It seems a bit indirect given that we will need to set MSWINDOWS.rawsd in lifecycle eventually (when we have a mechanism for accepting the security descriptor). Why not just set it directly? It would be easier to swap in the real values later.

The UID/GID -> SDDL translation in imgutil seems like an extremely valuable backstop to prevent bad images if the SDDL is unset. But eventually we will want to pick the exact values in a context aware way manner in lifecycle & pack no?

Else:

• Error "cannot translate permissions on Windows for UID: x; GID: x"

To provide the best safety net I think we keep translating all non-0 UID/GID pairs to ContainerUser instead of making 1 a special value.

[micahyoung]

Apologies for the slow reply, was away on vacation last week.

Yes, we could just set UID/GID to 0 or 1 and let imgutil translate. It seems a bit indirect given that we will need to set MSWINDOWS.rawsd in lifecycle eventually (when we have a mechanism for accepting the security descriptor). Why not just set it directly? It would be easier to swap in the real values later.

Good call on just setting MSWINDOWS.rawsd directly in lifecycle and pack. I supposed I've been thinking of the imgutil.WindowsWriter as a complete linux-layer => windows-layer API wrapper but I feel like we're reaching the limit of that approach with these PAXRecords fields.

The UID/GID -> SDDL translation in imgutil seems like an extremely valuable backstop to prevent bad images if the SDDL is unset. But eventually we will want to pick the exact values in a context aware way manner in lifecycle & pack no?

To provide the best safety net I think we keep translating all non-0 UID/GID pairs to ContainerUser instead of making 1 a special value.

That sounds fair and is close to the current imgutil implementation, though technically we're now not falling back to ContainerUser but rather BUILTIN/Users (of which ContainerUser and any others, are members). The nice part of falling back to BUILTIN/Users is that it technically accommodates any user that might have been created besides ContainerUser and ContainerAdministrator (i.e. cnb in samples)

By the time this is done though, I feel like most of the work involved in the end will be to:

• Add -sddl (or whatever we call it) flag (easy)
• Add layers.Factory.SID string to pass through (easy)
• Add NormalizingTarWriter.WithSID to convert the SDDL string to the binary/base64 format of hdr.PAXRecords[MSWINDOWS.rawsd']` (hard)

Assuming that's true, is there a piece we want to iterate on before the spec approval? Maybe just pretend when when -uid 0 -sid 0 that we got a -sddl "O:S-1-5-93-2-1G:S-1-5-93-2-1" (ContainerAdministrator) or otherwise -sddl "O:S-1-5-93-2-2G:S-1-5-93-2-2" for (ContainerUser) and drive all all the complexity while the spec works it's way through.

[ekcasey]

@micahyoung Sounds like a plan. I made a placeholder spec issue buildpacks/spec#129 for this.

It seem like nothing proposed here is in danger of being impossible (?) and you are fairly confident about the desired UX, so we might want to get the spec PR up first and then we can simultaneously work on the implementation as the spec goes through the review/release process.

[micahyoung]

After doing a bunch of experimenting on trying to convert SDDLs to rawsd, I feel it may be infeasible and we may need to go back to something like CNB_OWNER_SID and CNB_GROUP_SID. The issue I keep running into is that I couldn't find any way to convert from SDDL to rawsd that didn't rely on the Windows ConvertStringSecurityDescriptorToSecurityDescriptorW , which almost all SDDL implementations rely on, but we can't rely on because:

1. The syscall validates that any SIDs (ex: S-1-5-93-2-1) in the SDDL (ex: O:S-1-5-93-2-1G:S-1-5-93-2-1) are present on the system (and errors if the SID is unknown), which we can't assume because a run-image may contain an SID user that isn't present in the build-image (or whatever environment lifecycle runs in).
2. pack and other platforms that want to use lifecyle's implementation as a library for authoring layers on builders/buildpackages can't be expected to run on Windows hosts.

The closest non-Windows implementation I came across was the mkntfs's implementation for initializing a "blank" SD but it doesn't parse SDDLs, nor is it usable as a general library, instead it just serves a reference implementation for how to generate a very basic SD in C - which is very close to format needed for the rawsd.

So while the SDDL->rawsd conversion doesn't seem feasible, I feel an SID->rawsd conversion may be much simpler. We can make a template SD for something like O:S-1-5-93-2-1G:S-1-5-93-2-1 and then just replace the SID bytes with the bytes from a user-provided SID, which mkntfs roughly does with it's own structs like this (though I think we could come up with an even simpler implementaton):

	//owner sid
	sid = (SID*)((char*)sd + le32_to_cpu(sd->owner));
	sid->revision = 0x01;
	sid->sub_authority_count = 0x01;
	/* SECURITY_NT_SID_AUTHORITY (S-1-5) */
	sid->identifier_authority.value[0] = 0;
	sid->identifier_authority.value[1] = 0;
	sid->identifier_authority.value[2] = 0;
	sid->identifier_authority.value[3] = 0;
	sid->identifier_authority.value[4] = 0;
	sid->identifier_authority.value[5] = 5;
	sid->sub_authority[0] = const_cpu_to_le32(SECURITY_LOCAL_SYSTEM_RID);

	//group sid
	sid = (SID*)((char*)sd + le32_to_cpu(sd->group));
	sid->revision = 0x01;
	sid->sub_authority_count = 0x01;
	/* SECURITY_NT_SID_AUTHORITY (S-1-5) */
	sid->identifier_authority.value[0] = 0;
	sid->identifier_authority.value[1] = 0;
	sid->identifier_authority.value[2] = 0;
	sid->identifier_authority.value[3] = 0;
	sid->identifier_authority.value[4] = 0;
	sid->identifier_authority.value[5] = 5;
	sid->sub_authority[0] = const_cpu_to_le32(SECURITY_LOCAL_SYSTEM_RID);

But knowing this, would we ok with setting aside CNB_SECURITY_DESCRIPTOR for now and using something like CNB_OWNER_SID="S-1-5-93-2-1" and CNB_GROUP_SID="S-1-5-93-2-1"?

[micahyoung]

I put together a reference implementation that converts SID strings to rawsd values that make two SIDs the Owner and Group. The implementation matches the values that powershell generates and that we're currently using for imgutil/layer.WindowsWriter.

So I'd feel fine spec/rfc'ing this out at this point, assuming CNB_OWNER_SID and CNB_GROUP_SID are acceptable.

https://github.com/micahyoung/sid-to-rawsd