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

feat(cnbBuild): support builders with different CNB user ids #4625

Merged
merged 1 commit into from
Nov 2, 2023

Conversation

pbusko
Copy link
Member

@pbusko pbusko commented Oct 10, 2023

Changes

The cnbBuild will read CNB user information from environment variables to enable support for Paketo Jammy builders.

It enforces to run piper as root, the user for CNB lifecycle will be taken from the CNB_USER_ID and CNB_GROUP_ID. When the process is invoked, privileges are dropped down to this gid/uid.

Also this PR introduces two new features to the core packages:

  • RunExecutableWithAttrs which allows to pass syscall.SysProcAttr to the execution.

  • Recursive Chown

  • Tests

  • Documentation

@pbusko
Copy link
Member Author

pbusko commented Oct 10, 2023

/it-go

@pbusko pbusko force-pushed the cnbbuild-support-different-users branch 2 times, most recently from 883f738 to 6e588ed Compare October 10, 2023 14:52
@pbusko
Copy link
Member Author

pbusko commented Oct 10, 2023

/it-go

@pbusko pbusko force-pushed the cnbbuild-support-different-users branch from 6e588ed to c9e432b Compare October 10, 2023 15:22
@pbusko
Copy link
Member Author

pbusko commented Oct 10, 2023

/it-go

@pbusko pbusko marked this pull request as ready for review October 11, 2023 06:57
@pbusko pbusko requested review from a team as code owners October 11, 2023 06:57
@c0d1ngm0nk3y c0d1ngm0nk3y force-pushed the cnbbuild-support-different-users branch from c9e432b to fdb9d22 Compare October 11, 2023 14:47
@pbusko
Copy link
Member Author

pbusko commented Oct 12, 2023

/it-go

@pbusko pbusko requested a review from a team October 13, 2023 07:45
@rodibrin rodibrin added the security Potential security threat label Oct 17, 2023
@rodibrin
Copy link
Member

The Paketo Jammy builders changed the user id handling for security reasons, see https://github.com/orgs/paketo-buildpacks/discussions/188.

To default to user root (and chown the working directory) jeopardizes this.

Instead, use the docker options introduced once for the same topic here: caee8db

@pbusko
Copy link
Member Author

pbusko commented Oct 17, 2023

The Paketo Jammy builders changed the user id handling for security reasons, see https://github.com/orgs/paketo-buildpacks/discussions/188.

To default to user root (and chown the working directory) jeopardizes this.

Instead, use the docker options introduced once for the same topic here: caee8db

This change is exactly for the purpose of supporting Jammy/non-Jammy builders simultaneously. On Jenkins we can not run as arbitrary user (which can be effectively anything in CNB), due to the fact that the user must match the UID of the JNLP container (or root), otherwise the dockerExecuteOnKubernetes will fail to start. The change will ensure that the piper step itself will be started as root, and the actual lifecycle steps are running under dynamically configured users via environment variables, hence enabling support for Paketo Jammy and Bionic builders.

Also this change lays down the ground for future support of the CNB Extensions, which require root privileges to invoke Kaniko builds during the extension phase.

@pbusko pbusko force-pushed the cnbbuild-support-different-users branch from fdb9d22 to 7acbdb5 Compare October 17, 2023 13:07
@pbusko
Copy link
Member Author

pbusko commented Oct 17, 2023

/it-go

@rodibrin rodibrin removed the security Potential security threat label Oct 23, 2023
@pbusko pbusko force-pushed the cnbbuild-support-different-users branch from 7acbdb5 to c68c59b Compare October 24, 2023 14:27
@pbusko
Copy link
Member Author

pbusko commented Oct 24, 2023

/it-go

@c0d1ngm0nk3y
Copy link
Member

All checks have passed

That doesn't sound like Windows should be supported. How to ensure it?

@pbusko pbusko requested a review from a team October 26, 2023 07:19
@pbusko pbusko force-pushed the cnbbuild-support-different-users branch from c68c59b to b17c45e Compare October 27, 2023 13:21
@pbusko
Copy link
Member Author

pbusko commented Oct 27, 2023

/it-go

@c0d1ngm0nk3y
Copy link
Member

@rodibrin

rodibrin removed the security label

Does this mean you don't have open questions about merging this anymore?

@pbusko pbusko force-pushed the cnbbuild-support-different-users branch from b17c45e to bc31e69 Compare October 30, 2023 13:21
@pbusko
Copy link
Member Author

pbusko commented Oct 30, 2023

/it-go

@rodibrin
Copy link
Member

rodibrin commented Oct 31, 2023

Does this mean you don't have open questions about merging this anymore?

my only concern left is about the loss of windows as a testing platform.

Btw: could we remove the dockeroptions, which were (afaiu) invented to pass login data to cnbBuild? Should be replaceable.

@c0d1ngm0nk3y
Copy link
Member

my only concern left is about the loss of windows as a testing platform.

If windows should be a testing platform or not, should not be decided on an individual pr where a windows user happened to the reviewer. That should be a goal of the project and the pre-commit checks should at least verify that it is compilable on windows. That is at least my opinion.

Btw: could we remove the dockeroptions, which were (afaiu) invented to pass login data to cnbBuild? Should be replaceable.

Which dockeroptions are you referring to? The one for strating cnbBuild as root?

@rodibrin
Copy link
Member

Which dockeroptions are you referring to? The one for strating cnbBuild as root?

yes, Infer Kubernetes securityContext from dockerOpti…

@rodibrin
Copy link
Member

If windows should be a testing platform or not, should not be decided on an individual pr where a windows user happened to the reviewer.

the point is that this PR stops windows to be a platform for testing. officially, linux is the only platform supported, however there might be some people around which would appreciate windows support.
If it is possible to make the code buildable on windows with little effort, I'd be happy.

Co-authored-by: Pavel Busko <[email protected]>
Co-authored-by: Ralf Pannemans <[email protected]>
@pbusko pbusko force-pushed the cnbbuild-support-different-users branch from bc31e69 to dbdb4f5 Compare November 2, 2023 08:06
@pbusko
Copy link
Member Author

pbusko commented Nov 2, 2023

/it-go

Copy link

sonarqubecloud bot commented Nov 2, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@c0d1ngm0nk3y c0d1ngm0nk3y merged commit 26bfec1 into master Nov 2, 2023
11 checks passed
@c0d1ngm0nk3y c0d1ngm0nk3y deleted the cnbbuild-support-different-users branch November 2, 2023 15:03
maxatsap pushed a commit to maxatsap/jenkins-library that referenced this pull request Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants