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

Quote systemd DefaultEnvironment Proxy values #23674

Merged

Conversation

feloy
Copy link
Contributor

@feloy feloy commented Aug 20, 2024

Quote systemd DefaultEnvironment Proxy values, as documented in systemd.conf man page:

Example:
DefaultEnvironment="VAR1=word1 word2" VAR2=word3 "VAR3=word 5 6" Sets three variables "VAR1", "VAR2", "VAR3".

Double quote is not escaped, as there is no chance it appears in a proxy value. User can still espace it if really necessary

Fixes #23277
Fixes podman-desktop/podman-desktop#7894

How to test

  1. Declare proxy env vars in containers.conf file, with comma special character in no_proxy, for example (please replace proxy IP/port with your own):
env = [
	'https_proxy=http://192.168.1.21:3128',
	'http_proxy=http://192.168.1.21:3128',
	'no_proxy=localhost,quay.io',
]
  1. Create and start a new podman machine, and ssh to it

  2. Check that no_proxy and NO_PROXY values are not escaped:

$ cat /etc/systemd/system.conf.d/default-env.conf 
[Manager]
DefaultEnvironment="http_proxy=http://192.168.1.21:3128"
DefaultEnvironment="https_proxy=http://192.168.1.21:3128"
DefaultEnvironment="no_proxy=host.containers.internal,quay.io"
DefaultEnvironment="HTTP_PROXY=http://192.168.1.21:3128"
DefaultEnvironment="HTTPS_PROXY=http://192.168.1.21:3128"
DefaultEnvironment="NO_PROXY=host.containers.internal,quay.io"
  1. Restart podman machine so podman process takes these values into consideration

  2. Check that proxy env vars are passed to podman process:

$ ps eww `pidof podman`
    PID TTY      STAT   TIME COMMAND
   1282 ?        Ssl    0:00 /usr/bin/podman --log-level=info system service HTTPS_PROXY=http://192.168.1.21:3128 HTTP_PROXY=http://192.168.1.21:3128 NO_PROXY=host.containers.internal,quay.io http_proxy=http://192.168.1.21:3128 https_proxy=http://192.168.1.21:3128 no_proxy=host.containers.internal,quay.io [...]
  1. From a client (podman outside of VM or podman-desktop):
  • pull an image from docker.io: proxy should log requests
  • pull an image from quay.io: proxy should not log any request

Does this PR introduce a user-facing change?

Values for proxy env variables won't be escaped anymore.

Changed the way the proxy environment variables are defined in the systemd configuration file. These values were previously escaped for shell input reuse (%q), they are now not escaped.

@feloy feloy changed the title Quote systemd DefaultEnvironment Proxy values, as documented in syste… Quote systemd DefaultEnvironment Proxy values Aug 20, 2024
@feloy feloy force-pushed the fix-23277/quote-default-env branch from c4d5272 to 7e3e532 Compare August 20, 2024 08:29
@feloy feloy marked this pull request as draft August 20, 2024 10:06
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 20, 2024
@feloy
Copy link
Contributor Author

feloy commented Aug 20, 2024

I'll add some tests

@feloy feloy force-pushed the fix-23277/quote-default-env branch 3 times, most recently from b70662a to f31a67c Compare August 21, 2024 09:59
@feloy feloy marked this pull request as ready for review August 21, 2024 10:06
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 21, 2024
@feloy
Copy link
Contributor Author

feloy commented Aug 21, 2024

I'll add some tests

Tests added, the PR is ready for review

@rhatdan
Copy link
Member

rhatdan commented Aug 21, 2024

LGTM

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

you will need to rebase on latest main to fix the CI

@@ -24,7 +24,7 @@ rm -f $SYSTEMD_CONF $ENVD_CONF $PROFILE_CONF

echo "[Manager]" >> $SYSTEMD_CONF
for proxy in %s; do
printf "DefaultEnvironment=%%q\n" "$proxy" >> $SYSTEMD_CONF
printf "DefaultEnvironment=\"%%s\"\n" "$proxy" >> $SYSTEMD_CONF
Copy link
Member

Choose a reason for hiding this comment

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

This will break if the vars contains a quote, I guess I am fine to trade quote is broken vs command is broken because the later is actually used in these vars while the quote most likely is never used.

However please fix the test and add a comma there

proxy1 := "http:// some special @;\" here"

Comment on lines 69 to 71
if err != nil {
t.Errorf("readFrom: %v", err)
}
Copy link
Member

Choose a reason for hiding this comment

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

we already use assert github.com/stretchr/testify/assert so please use that instead, i.e.

assert.NoError(t, err)

Comment on lines 73 to 75
if str != tt.want {
t.Errorf("getProxyScript()\n\n%v\n\nwant\n\n%v", str, tt.want)
}
Copy link
Member

Choose a reason for hiding this comment

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

here you can use

assert.Equal(t, tt.want, str)

@feloy feloy force-pushed the fix-23277/quote-default-env branch 3 times, most recently from e35b2aa to dd016e7 Compare August 26, 2024 09:27
@feloy feloy requested a review from Luap99 August 26, 2024 10:32
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

please squash your commits into one
changes LGTM otherwise

…md.conf man page:

Example:
DefaultEnvironment="VAR1=word1 word2" VAR2=word3 "VAR3=word 5 6"
Sets three variables "VAR1", "VAR2", "VAR3".

Double quote is not escaped, as there is no chance it appears in a proxy value. User can still espace it if really necessary

Signed-off-by: Philippe Martin <[email protected]>
@feloy feloy force-pushed the fix-23277/quote-default-env branch from dd016e7 to 3e58e04 Compare August 26, 2024 11:13
@feloy feloy requested a review from Luap99 August 26, 2024 12:11
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 26, 2024
Copy link
Contributor

openshift-ci bot commented Aug 26, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: feloy, Luap99

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 26, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit b765ab9 into containers:main Aug 26, 2024
92 checks passed
@TheConen
Copy link

Is this fix already included in the latest 5.2.3 release? I could not find it in the changelog.

@Luap99
Copy link
Member

Luap99 commented Sep 25, 2024

It was not backported so it will be in 5.3

@TheConen
Copy link

Is there something like a timeline / roadmap when 5.3 will be released?

@Luap99
Copy link
Member

Luap99 commented Sep 25, 2024

End of October/early November most likely

@stale-locking-app stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Dec 25, 2024
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators Dec 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. machine release-note
Projects
None yet
4 participants