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

Windows build images use SIDs for User/Group ownership #133

Merged
merged 12 commits into from
Feb 17, 2021

Conversation

micahyoung
Copy link
Member

@micahyoung micahyoung commented Jan 7, 2021

@micahyoung micahyoung force-pushed the windows-security-identifiers branch 4 times, most recently from 55af4f7 to faa51e4 Compare January 25, 2021 11:21
@micahyoung micahyoung marked this pull request as ready for review January 25, 2021 11:21
@micahyoung micahyoung requested a review from a team as a code owner January 25, 2021 11:21
@jabrown85
Copy link
Contributor

This might be out of scope for this, but how does kubernetes translate things like this in for windows based pods?

securityContext:
    fsGroup: <group-id>

I'm trying to imagine writing a single platform that can target windows and linux builders with a single implementation. Right now, a platform may read the CNB_GROUP_ID from the target builder and pop that into the securityContext for the pod so that volume ownership is as expected.

text/0000-windows-security-identifiers.md Outdated Show resolved Hide resolved
The spec states behavior that is currently unimplemented in `pack`/`lifecycle` and should be changed before it is implemented (example: [platform.md - **Build image**](https://github.com/buildpacks/spec/blob/313078611d8a7925cb69a241df58e7d749d7f364/platform.md#:~:text=CNB_USER_ID%20set%20to%20the%20user), related: [spec issue](https://github.com/buildpacks/spec/issues/129)):
> The image config's Env field has the environment variable CNB_USER_ID set to the user †UID/‡SID of the user specified in the User field.

Instead, Windows stack authors should specify *alternative* env vars for Windows `CNB_OWNER_SID`/`CNB_GROUP_SID` (instead of `CNB_USER_ID`/`CNB_GROUP_ID`) where the values are Windows Security Descriptor strings (ex: `CNB_OWNER_SID=S-1-5-93-2-1`). Elsewhere, in the platform spec (example: [creator](https://github.com/buildpacks/spec/blob/313078611d8a7925cb69a241df58e7d749d7f364/platform.md#creator)), lifecycle binaries are called with `-uid <uid>` and `-gid <gid>` flags. Instead, these flags shoud be defined as `-uid <†UID/‡SID>` or `-gid <†UID/‡SID>` to accept SIDs. Finally, all platforms/implementations should use this SID values to generate Windows image layer entries and container files with Security Descriptors instead of leaving them blank or converting from POSIX UID/GIDs.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On linux there are certain cases where the lifecycle need to be launched as root to access certain resources. For example, when pack runs certain phases in a daemon build it runs the container as root. The lifecycle opens a connection to the daemon socket before dropping privileges for the rest of the build. When run as root the lifecycle will also chown certain directories (cache, layers) if they are not writable as the unprivileged user (although this could be made unnescessary if the builder was required to provide the mount points with the correct permissions).

Have you investigated whether any of these concerns are relevant for windows containers and, if so, are the inputs above sufficient to perform analogous actions on windows?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely very relevant and while the current lifecycle implementation is unimplemented, it should be feasible to do so using Windows APIs like AdjustTokenPrivileges or LogonUserW (which have wrappers in x/sys/windows)

Copy link
Member Author

@micahyoung micahyoung Feb 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After digging in a bit more, it seems this impersonation behavior should work for Windows. There's not a lot of well documented examples out there but I stumbled on this Powershell Proof-of-concept demonstrating how an Administrator user can impersonate the SYSTEM user - technically to gain, rather than lose privileges - but translating this to Golang and to use the user SID instead of SYSTEM should be feasible, though maybe not trivial. I confirmed this PoC works when run as ContainerAdministrator on a servercore container (b/c only servercore has Powershell) so there's no container magic that should get in the way and I assume it's likely to work on nanoserver for binaries using the same Windows syscalls.

As for the specifics of establishing daemon socket connections and chowning files before dropping privileges, I'd have to continue to investigate, but it feels close enough that I assume we could find a workaround, if not use the same implementation.

But all the inputs as far as user/group SIDs are sufficient to do impersonation.


> If applicable, describe the differences between teaching this to existing users and new users.

SID usage will be very similar to Linux, though almost always using the SID for the standard Docker Windows user `ContainerUser`. Any Linux-specific documentation or existing Windows samples for Windows run images would need to change to reflect the new variables for Windows.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It it worth having create-builder automatically set CNB_USER_SID and CNB_GROUP_SID if it find that the build image is using one of the well known users? This might reduce burden on end users.

Copy link
Member Author

@micahyoung micahyoung Feb 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great suggestion, particularly since Docker's hard-coded ContainerUser/ContainerAdministrator SIDs that will most commonly be used, aren't well-known Windows SIDs. Having most stacks follow this pattern could also give us a mitigation if the SIDs ever changed for some reason.

I'll update the RFC with this suggestion

Copy link
Member Author

@micahyoung micahyoung Feb 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to add some text for that option but was wondering if it starts to contradict valid build image requirements of the spec:

Under Build Image

The platform MUST ensure that:

The image config's Env field has the environment variable CNB_USER_ID set to the user †UID/‡SID of the user specified in the User field.

(We'll be tweaking the var name here of course)

But I'm now feeling like, while this helps with pack builders, this may cause some delayed confusion in a case where a Stack Author makes a "build image" w/o the env vars, that works with pack but, potentially much later, a platform like kpack fails to validate while trying to make a cluster builder or something.

Perhaps, instead we could add a more helpful error message during pack builder create - when the env vars are missing, suggest the value to go back and add to the build image, based on a lookup table for ContainerUser, ContainerAdministrator (and perhaps even a /etc/passwd check for Linux images, if we wanted to go that far).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to close the loop on this?

@micahyoung
Copy link
Member Author

This might be out of scope for this, but how does kubernetes translate things like this in for windows based pods?

securityContext:
    fsGroup: <group-id>

I'm trying to imagine writing a single platform that can target windows and linux builders with a single implementation. Right now, a platform may read the CNB_GROUP_ID from the target builder and pop that into the securityContext for the pod so that volume ownership is as expected.

@jabrown85 I think the Windows equivalent securityContext (docs) would be:

  securityContext:
    windowsOptions:
      runAsUserName: "ContainerUser"

... so you'd likely end up grabbing the CNB_USER_SID instead to do the same for a Windows pod.

Does that line up with the use-case? Would the Group vs User mismatch be problematic?

@jabrown85
Copy link
Contributor

Does that line up with the use-case? Would the Group vs User mismatch be problematic?

That will probably be 👍

Micah Young and others added 11 commits February 3, 2021 20:43
Signed-off-by: Micah Young <ymicah@vmware.com>
Signed-off-by: Micah Young <ymicah@vmware.com>
Signed-off-by: Micah Young <ymicah@vmware.com>
Signed-off-by: Micah Young <ymicah@vmware.com>
Signed-off-by: Micah Young <ymicah@vmware.com>
Signed-off-by: Micah Young <ymicah@vmware.com>
Co-authored-by: Yael Harel <43007598+yaelharel@users.noreply.github.com>
Signed-off-by: Micah Young <ymicah@vmware.com>
- CNB_USER_SID is more semantically align with CNB_USER_ID as it reflects "the user whose permissions need to match the files" as opposed to "the owner of the user-owned files"

Signed-off-by: Micah Young <ymicah@vmware.com>
- Based on feedback from working group. Will be simpler and more explicit

Signed-off-by: Micah Young <ymicah@vmware.com>
Signed-off-by: Micah Young <ymicah@vmware.com>
Signed-off-by: Micah Young <ymicah@vmware.com>
Signed-off-by: Micah Young <ymicah@vmware.com>
@micahyoung micahyoung changed the title Windows run images use SIDs for User/Group ownership Windows build images use SIDs for User/Group ownership Feb 4, 2021

# Definitions
[definitions]: #definitions
* *Security Principal*: Any entity that can be authenticated by the Windows operating system, such as a user account, a computer account, or a thread or process that runs in the security context of a user or computer account, or the security groups for these accounts
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are many "or" in this sentence. Maybe it's better to have a list instead.

@ekcasey
Copy link
Member

ekcasey commented Feb 10, 2021

Final Comment Period with merge disposition, closing on 17 February, 2021

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants