-
Notifications
You must be signed in to change notification settings - Fork 151
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
Add domain proxy to buildah-oci-ta #1822
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -139,6 +139,62 @@ spec: | |
description: The name of the ConfigMap to read CA bundle data from. | ||
type: string | ||
default: trusted-ca | ||
- name: BUILD_IMAGE | ||
description: The buildah image to use. | ||
type: string | ||
default: quay.io/konflux-ci/buildah-task:latest@sha256:b2d6c32d1e05e91920cd4475b2761d58bb7ee11ad5dff3ecb59831c7572b4d0c | ||
- name: ENABLE_DOMAIN_PROXY | ||
description: Determines if domain proxy will be used when hermetic mode is enabled. | ||
type: string | ||
default: "false" | ||
- name: DOMAIN_PROXY_BYTE_BUFFER_SIZE | ||
description: The byte buffer size to use for the domain proxy. | ||
type: string | ||
default: 32768 | ||
- name: DOMAIN_PROXY_DOMAIN_SOCKET | ||
description: The domain socket to use for the domain proxy. | ||
type: string | ||
default: /tmp/domain-socket.sock | ||
- name: DOMAIN_PROXY_CONNECTION_TIMEOUT | ||
description: The connection timeout in milliseconds to use for the domain proxy. | ||
type: string | ||
default: 10000 | ||
- name: DOMAIN_PROXY_IDLE_TIMEOUT | ||
description: The idle timeout in milliseconds to use for the domain proxy. | ||
type: string | ||
default: 30000 | ||
- name: DOMAIN_PROXY_TARGET_ALLOWLIST | ||
description: Comma separated list of allowed target hosts for the domain proxy. | ||
type: string | ||
default: "" | ||
- name: DOMAIN_PROXY_ENABLE_INTERNAL_PROXY | ||
description: Determines if internal proxy will be used when domain proxy is enabled. | ||
type: string | ||
default: "false" | ||
- name: DOMAIN_PROXY_INTERNAL_PROXY_HOST | ||
description: Host of proxy used internally by the domain proxy. | ||
type: string | ||
default: "" | ||
- name: DOMAIN_PROXY_INTERNAL_PROXY_PORT | ||
description: Port of proxy used internally by the domain proxy. | ||
type: string | ||
default: "" | ||
- name: DOMAIN_PROXY_INTERNAL_PROXY_USER | ||
description: User of proxy used internally by the domain proxy. | ||
type: string | ||
default: "" | ||
- name: DOMAIN_PROXY_INTERNAL_PROXY_PASSWORD | ||
description: Password of proxy used internally by the domain proxy. | ||
type: string | ||
default: "" | ||
- name: DOMAIN_PROXY_INTERNAL_NON_PROXY_HOSTS | ||
description: Comma separated list of target hosts that bypass the proxy used internally by the domain proxy. | ||
type: string | ||
default: "" | ||
- name: DOMAIN_PROXY_HTTP_PORT | ||
description: The HTTP port to use for the domain proxy. | ||
type: string | ||
default: 8080 | ||
Comment on lines
+146
to
+197
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Which of these values have to potential to affect the security posture of the buildah task? Those parameters need to have a policy to limit the allowable values. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Currently the values of the following are unknown so we can't limit them:
They're also subject to change according to the build context, e.g. some will be obtained on the fly from the pipeline (potentially from components deployed as part of pipeline). The other parameters can technically be removed (except for ENABLE_DOMAIN_PROXY), as the defaults within the Domain Proxy are work just fine for current test scenarios. But there may come a point where the following may need to be changed:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In terms of security, we probably need a generic solution where the task can be used, not just for the PNC integration. Maybe this is just a matter of terminology what operation modes this task provides. There already is a HERMETIC parameter, which allows users to run the build with or without isolation. We are essentially adding a 3rd option here, a "controlled" internet access. The parameters DOMAIN_PROXY_INTERNAL_PROXY_* are used to define a "tracking proxy" and the domain proxy guarantees that all the request from the isolated environment go through this tracking proxy. Except the hosts listed in DOMAIN_PROXY_INTERNAL_NON_PROXY_HOSTS. Then it's up to users what king of "tracking proxy" they use, if the "tracking proxy" used does not track anything, the build is pretty similar to a non hermetic build. In case of PNC integration, users won't have access to manipulate this parameters. An option to make this case more clear is to change the HERMETIC (true|false) parameter to NETWORK_ISOLATION (HERMETIC|CONTROLLED|NONE). And make the domain proxy activation conditional based on this parameter. |
||
results: | ||
- name: IMAGE_DIGEST | ||
description: Digest of the image just built | ||
|
@@ -227,6 +283,32 @@ spec: | |
value: $(params.YUM_REPOS_D_SRC) | ||
- name: YUM_REPOS_D_TARGET | ||
value: $(params.YUM_REPOS_D_TARGET) | ||
- name: ENABLE_DOMAIN_PROXY | ||
value: $(params.ENABLE_DOMAIN_PROXY) | ||
- name: DOMAIN_PROXY_BYTE_BUFFER_SIZE | ||
value: $(params.DOMAIN_PROXY_BYTE_BUFFER_SIZE) | ||
- name: DOMAIN_PROXY_DOMAIN_SOCKET | ||
value: $(params.DOMAIN_PROXY_DOMAIN_SOCKET) | ||
- name: DOMAIN_PROXY_CONNECTION_TIMEOUT | ||
value: $(params.DOMAIN_PROXY_CONNECTION_TIMEOUT) | ||
- name: DOMAIN_PROXY_IDLE_TIMEOUT | ||
value: $(params.DOMAIN_PROXY_IDLE_TIMEOUT) | ||
- name: DOMAIN_PROXY_TARGET_ALLOWLIST | ||
value: $(params.DOMAIN_PROXY_TARGET_ALLOWLIST) | ||
- name: DOMAIN_PROXY_ENABLE_INTERNAL_PROXY | ||
value: $(params.DOMAIN_PROXY_ENABLE_INTERNAL_PROXY) | ||
- name: DOMAIN_PROXY_INTERNAL_PROXY_HOST | ||
value: $(params.DOMAIN_PROXY_INTERNAL_PROXY_HOST) | ||
- name: DOMAIN_PROXY_INTERNAL_PROXY_PORT | ||
value: $(params.DOMAIN_PROXY_INTERNAL_PROXY_PORT) | ||
- name: DOMAIN_PROXY_INTERNAL_PROXY_USER | ||
value: $(params.DOMAIN_PROXY_INTERNAL_PROXY_USER) | ||
- name: DOMAIN_PROXY_INTERNAL_PROXY_PASSWORD | ||
value: $(params.DOMAIN_PROXY_INTERNAL_PROXY_PASSWORD) | ||
- name: DOMAIN_PROXY_INTERNAL_NON_PROXY_HOSTS | ||
value: $(params.DOMAIN_PROXY_INTERNAL_NON_PROXY_HOSTS) | ||
- name: DOMAIN_PROXY_HTTP_PORT | ||
value: $(params.DOMAIN_PROXY_HTTP_PORT) | ||
volumeMounts: | ||
- mountPath: /shared | ||
name: shared | ||
|
@@ -240,7 +322,7 @@ spec: | |
- $(params.SOURCE_ARTIFACT)=/var/workdir/source | ||
- $(params.CACHI2_ARTIFACT)=/var/workdir/cachi2 | ||
- name: build | ||
image: quay.io/konflux-ci/buildah-task:latest@sha256:b2d6c32d1e05e91920cd4475b2761d58bb7ee11ad5dff3ecb59831c7572b4d0c | ||
image: $(params.BUILD_IMAGE) | ||
args: | ||
- --build-args | ||
- $(params.BUILD_ARGS[*]) | ||
|
@@ -526,7 +608,43 @@ spec: | |
# disable host subcription manager integration | ||
find /usr/share/rhel/secrets -type l -exec unlink {} \; | ||
|
||
unshare -Uf "${UNSHARE_ARGS[@]}" --keep-caps -r --map-users 1,1,65536 --map-groups 1,1,65536 -w "${SOURCE_CODE_DIR}/$CONTEXT" -- sh -c "$command" | ||
if [ "${HERMETIC}" == "true" ] && [ "${ENABLE_DOMAIN_PROXY}" == "true" ]; then | ||
echo "Build will be executed with domain proxy" | ||
/app/domain-proxy-server & | ||
server_pid=$! | ||
|
||
# Without expansion | ||
cat >> /app/build-script.sh << 'EOF' | ||
#!/bin/sh | ||
/app/domain-proxy-client & | ||
client_pid=$! | ||
EOF | ||
|
||
# With expansion | ||
cat >> /app/build-script.sh << EOF | ||
$command | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this run with the domain proxy while also having the network restricted from the build? I haven't tested this out, but it seems like the domain proxy would not have any value if there is no connectivity allowed in the first place. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question. I should have been more clear in my PR description. There are two components.
The components interact via Unix domain socket connection. See the Domain Proxy readme here: https://github.com/project-ncl/domain-proxy There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
EOF | ||
|
||
# Without expansion | ||
cat >> /app/build-script.sh << 'EOF' | ||
set +e | ||
kill $client_pid | ||
wait $client_pid | ||
set -e | ||
EOF | ||
|
||
cat /app/build-script.sh | ||
chmod +x /app/build-script.sh | ||
|
||
unshare -Uf "${UNSHARE_ARGS[@]}" --keep-caps -r --map-users 1,1,65536 --map-groups 1,1,65536 -w "${SOURCE_CODE_DIR}/$CONTEXT" -- /app/build-script.sh | ||
|
||
set +e | ||
kill $server_pid | ||
wait $server_pid | ||
set -e | ||
else | ||
unshare -Uf "${UNSHARE_ARGS[@]}" --keep-caps -r --map-users 1,1,65536 --map-groups 1,1,65536 -w "${SOURCE_CODE_DIR}/$CONTEXT" -- sh -c "$command" | ||
fi | ||
|
||
container=$(buildah from --pull-never "$IMAGE") | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We used to have a parameter like this, but we removed it in #792. If there is a legitimate use case where we need to enable this to be customized then we need to ensure that we have an EC policy preventing it by default.
Is there a reason why you wouldn't just prefer to make an additional task to update this image with Kustomize? Presumably your use cases would only work with a very particular image ... and that image should be kept up to date?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's correct that the use case only works with a particular image. The image will indeed be kept up to date.
Happy to look into using kustomize instead to change the image, however...
I believe the reason we don't want to create an additional task is because we'd have to keep the script in sync with builah-oci-ta, which has been a real pain for us the past few months as we've been developing our solutions for Middleware (PNC).
Please correct me if I'm misunderstanding your suggestion!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is large script block where the domain proxy integrates, I don't thing it can be manipulated by Kustomize.
@arewm can you point us to the "task generation process like we do for the remote builds" ?