TFTP feature

[stejskalleos]

Why are you introducing these changes? (Problem description, related links)

Support the TFTP Smart Proxy feature.

What are the changes introduced in this pull request?

• foremanctl deploy ---add-feature tftp

How to test this pull request

./foremanctl deploy --help

  --tftp-root FOREMAN_PROXY_TFTP_ROOT
                        Directory to serve TFTP files from. (persisted)
  --tftp-servername FOREMAN_PROXY_TFTP_SERVERNAME
                        Server name to use for TFTP. (persisted)

./foremanctl deploy --add-feature foreman-proxy --add-feature tftp \
  --tftp-root /var/lib/foreman-custom-tftp \
  --tftp-servername something-else.example.com \

Go to https://quadlet.example.com/smart_proxies/2-quadlet-example-com, TFTP should be enabled.

vagrant ssh quadlet
sudo podman exec -it foreman-proxy /bin/bash

cat /etc/foreman-proxy/settings.d/tftp.yml

---
:enabled: false
:tftproot: /var/lib/foreman-custom-tftp
:tftp_servername: something-else.example.com
:tftp_connect_timeout: 10
:verify_server_cert: true

./foremanctl deploy --remove-feature tftp
./foremanctl features
=>

tftp                      available    Enable TFTP feature on Foreman Proxy

cat /etc/foreman-proxy/settings.d/tftp.yml

---
:enabled: false
:tftproot: /var/lib/foreman-custom-tftp
:tftp_servername: something-else.example.com
:tftp_connect_timeout: 10
:verify_server_cert: true

Go to https://quadlet.example.com/smart_proxies/2-quadlet-example-com, TFTP should be disabled.

Checklist

• Tests added/updated (if applicable)
• Documentation updated (if applicable)

[ekohl]

Does this need a default? IIRC the code has a fallback on the Smart Proxy's hostname already so this doesn't add anything. In a remote setup it will also be guaranteed wrong.

[lzap]

Hey, this is completely random (I was just browsing this repository) but:

TFTP clients usually do not have DNS stack available, therefore, this must be IP address if anything. I see fqdn so I assume this would be a hostname. I remember there was some FQDN>IP hack somewhere but using IP was always the best and the safest thing to do.

This has always been terrible name, servername. What it does is sets up next-server which is probably the root cause of all of this terminology. But what users typically need to set is an IP address.

[ekohl]

You're right, that is a better value. Ideally this value defaults to empty and we force users to provide the value when they enable the TFTP feature. Then input validation forces a valid IPv4 address.

[stejskalleos]

We could at least rename the parameter in foremactl.
Change it to tftp-ip and update the description.
WDYT?

[evgeni]

Can we get a test that sets up a tftp-server (on the quadlet machine), instructs the proxy to create a file in the tftp root and uses tftp to fetch that file?

[evgeni]

Ewouds comment in jira made me realize that this description is wrong.
We also use /var/lib/tftpboot for the httpboot module (https://github.com/theforeman/smart-proxy/blob/3e249dd019dd36f41b546fe44c045ab055c1da6c/modules/httpboot/httpboot_plugin.rb#L13) and do not allow users to configure that in Puppet.

It is unclear to me how the HTTPBoot thing obtains the files (seems there is no code for that), so maybe it relies on the TFTP module to place the files and just exposes them over HTTP instead of TFTP?

(This is not blocking the feature here, just food for further thought going forward)

[arvind4501]

I also faced similar doubts while just scaffolding httpboot feature, and i realised its use /var/lib/tftp which is not created by httpboot, which makes me think why we don't add tftp as dependency for httpboot. for full context you can refer #518 (comment)

[ekohl]

which makes me think why we don't add tftp as dependency for httpboot.

You may find theforeman/smart-proxy@db33bc6 interesting.

Perhaps we should call it the netboot directory? I already had https://github.com/theforeman/puppet-foreman_proxy/blob/master/manifests/tftp/netboot.pp in the old installer.

[evgeni]

"interesting" as in "wait, what!?"? ;-)

How is the directory populated, when the TFTP feature is off, but HTTPBoot is on?

[ekohl]

Magic? In theforeman/puppet-foreman_proxy@e4b7763 I wrote:

To get the netboot files, the TFTP feature must still be enabled.

It was a long discussion that was rather tedious. I'm not sure the whole use case of netbooting with HTTPBoot without TFTP was ever really well supported.

[arvind4501]

How is the directory populated, when the TFTP feature is off, but HTTPBoot is on?

AFAIK, no population happens, just configuration, As of today, by default httpboot enablement will add :root_dir: /var/lib/tftpboot in config, and still be indirectly dependent on tftp to create and populate that directory

[stejskalleos]

Can we get a test that sets up a tftp-server (on the quadlet machine), instructs the proxy to create a file in the tftp root and uses tftp to fetch that file?

I'm still working on this test; I will add it in the upcoming commit.

[stejskalleos]

@ehelms applied comments from your review.
Can't add the tests for the feature because of a broken local environment.
I'm trying to figure it out in the meantime.

[ehelms]

Given the validation message below, I'd include this line here in some form:

  This should be the IP address of the TFTP server (TFTP clients typically do not have DNS available).

[ehelms]

Which I will also point out is slightly ironic we call it servername and then state it should be the IP address.

[ekohl]

Technically we're talking about https://www.rfc-editor.org/info/rfc2132/#section-9.4. DHCP option 66 is called TFTP server name. Sadly it doesn't describe at all what the content of it should be at the protocol level.

If we look at what we do for ISC DHCP (https://github.com/theforeman/smart-proxy/blob/3e249dd019dd36f41b546fe44c045ab055c1da6c/modules/dhcp_common/isc/omapi_provider.rb#L205-L217) then that's basically trying to send an IP, but ISC DHCP itself can also be configured with a hostname where the service does a lookup.

Kea's all-options.json actually has an example that is a hostname: https://github.com/isc-projects/kea/blob/f79874a1f22c29b96ee543f5bc905960818991f7/doc/examples/kea4/all-options.json#L787-L798.

So I'm not sure we need to be so strict in only accepting an IP. Apologies for not digging into that deep enough before.

[evgeni]

@stejskalleos would you mind rebasing this one? there is a conflict :/

[evgeni]

And another rebase is in order.

@Gauravtalreja1 you wanted to have a look here before we merge? Or can we merge to save Leos from rebasing as a service?

[evgeni]

(rebased as leos is busy)

[Gauravtalreja1]

Isn't this too much open permissions for a dir? I checked previously we had 755, is this change intended?

[stejskalleos]

It doesn't work with 755. You need to have all the permissions for the quadlet service.

The fix might be to create a new dedicated user or user group and run the container with it, but I'm not sure if it is the correct approach.

[ekohl]

I think mode 0777 is not acceptable from a security perspective. If it doesn't work, we need to find another solution.

Note the smart-proxy process runs as user foreman-proxy (https://github.com/theforeman/foreman-oci-images/blob/776ba097225f0308d34eb48f6b4ccdb8a63b72c5/images/foreman-proxy/Containerfile#L17) so that user will need to have write permission.

We may need to guarantee the same UID is used within the container and the host, or on the remote server.

In short, agreed with @Gauravtalreja1 this needs to be addressed before we merge it.

[stejskalleos]

Updated with 0755, the owner is now foreman-proxy user (998)

[Gauravtalreja1]

@stejskalleos I tried re-running the deploy with --tftp-root /var/lib/my_tftpboot/ and permissions are updated as 0755 but owner is not set to foreman-proxy instead it is polkitd

# ls -lZ /var/lib/ | grep my_tftpboot
drwxr-xr-x. 4 polkitd        polkitd        unconfined_u:object_r:var_lib_t:s0                45 Jun 23 13:31 my_tftpboot

[ekohl]

The UID on the host and in the container may not be the same. We don't have a reserved UID. This may actually prove to be a problem for NFS mounts and on the next container rebuild that 998 may be 997.

Can't podman map the user ID?

[stejskalleos]

My assumption was that the ID would always be the same, which is why the values were hardcoded.
Why would the ID change anyway? What if I hardcode it in the container image?

I'm thinking about the correct solution here. Adding z,Z, or U flags to the volume breaks either smart proxy access or TFTP access, so that's no use.

[Gauravtalreja1]

IIUC, label=disable effectively disables SELinux for the container. Could you explain the rationale behind this change and the underlying issue it is meant to resolve?

[stejskalleos]

That's the only way to make the container read, write, and list data in the directory.

[Gauravtalreja1]

Should we be explicit and mention what we'll use default if not specified?

And should we add a validation to check if its valid directory and not file or something else? I tried passing a file I manually created for --tftp-root and it failed at very later stage, so such validation would help when failed earlier

Suggested change

	help: Directory to serve TFTP files from.
	help: Directory to serve TFTP files from, defaults to /var/lib/tftpboot if not specified.

[Gauravtalreja1]

@stejskalleos I see help text is updated as per this, but could you take a look at the validation part for tfpt-root option?

[stejskalleos]

And should we add a validation to check if its valid directory and not file or something else? I tried passing a file I manually created for --tftp-root and it failed at very later stage, so such validation would help when failed earlier

I mean, we could, but it is kinda the user's fault for passing a file instead of a directory, and the overall effort to do it early just isn't worth it IMO. We have validation, and thats important IMO.

[Gauravtalreja1]

Should we add a foreman-proxy feature as a requirement here as well?

I tried to enable tftp without foreman-proxy and it was enabled without any errors, but I know ideally it wouldn't work, so should we add a better validation here to check if tftp is present in required_in_list or do we want to enable foreman-proxy feature as dependancy to TFTP?

[stejskalleos]

@ekohl What is the correct behavior in this scenario:

User runs foremanctl deploy --add-feature tftp without the --add-feature foreman-proxy (in the current run or in the previous one).

What's the expected behavior?
a. Foremanctl should fail with an error about the required feature
b. Foremanctl should automatically add the smart proxy feature

[ekohl]

I don't think tftp should silently pull in foreman-proxy. Adding a whole separate service is something that must be done explicitly IMHO because you may also need to open up firewall ports. So I'd lean more to an error that a required feature is not enabled.

This will also apply to all other Smart Proxy features, like DNS and DHCP. I lean to tracking that as a follow up task because I doubt we fixed in other places.

[stejskalleos]

Created https://redhat.atlassian.net/browse/SAT-46972

[Gauravtalreja1]

I tried this manually, and whenever I invoke this API, it creates the directory structure on the Foreman side rather than on the TFTP server.

Additionally, when I run the tftp command from the test you added, it fails with a File not found error because the expected files are not present on the TFTP server.

Could you elaborate on the expected workflow here and how this test is supposed to work? Am I missing a step that synchronizes or publishes the generated content to the TFTP server?

[stejskalleos]

There is nothing else needed; just replicate the test steps, and it should work. The CI is passing, so it's working.

[ekohl]

The core design is that it will be created on the Foreman Proxy side (which may also run a Foreman). It's expected that the admin makes sure the TFTP server also uses the same directory. For example, using NFS as done in https://docs.theforeman.org/3.19/Integrating_Provisioning_Infrastructure_Services/index-foreman-el.html#integrating-a-generic-tftp-server.

Ideally we'd have an accompanying docs PR that explains the end-to-end workflow.

[stejskalleos]

I don't think failed CI is related to my changes:

TASK [theforeman.forklift.sos_report : Find sosreport file] ********************
Error: : Task failed: Failed to create temporary directory. In some cases, you may have been able to authenticate and did not have permissions on the target directory. Consider changing the remote tmp path in ansible.cfg to a path rooted in "/tmp", for more error information use -vvv. Failed command was: ( umask 77 && mkdir -p "` echo ~/.ansible/tmp `"&& mkdir "` echo ~/.ansible/tmp/ansible-tmp-1782200501.2489645-21366-92798372804531 `" && echo ansible-tmp-1782200501.2489645-21366-92798372804531="` echo ~/.ansible/tmp/ansible-tmp-1782200501.2489645-21366-92798372804531 `" ), exited with result 1
Origin: /home/runner/work/foremanctl/foremanctl/build/collections/forge/ansible_collections/theforeman/forklift/roles/sos_report/tasks/sosreport_fetch_results.yml:2:3

1 ---
2 - name: 'Find sosreport file'
    ^ column 3

fatal: [quadlet]: UNREACHABLE! => {
    "changed": false,
    "unreachable": true
}

MSG:

Task failed: Failed to create temporary directory. In some cases, you may have been able to authenticate and did not have permissions on the target directory. Consider changing the remote tmp path in ansible.cfg to a path rooted in "/tmp", for more error information use -vvv. Failed command was: ( umask 77 && mkdir -p "` echo ~/.ansible/tmp `"&& mkdir "` echo ~/.ansible/tmp/ansible-tmp-1782200501.2489645-21366-92798372804531 `" && echo ansible-tmp-1782200501.2489645-21366-92798372804531="` echo ~/.ansible/tmp/ansible-tmp-1782200501.2489645-21366-92798372804531 `" ), exited with result 1

[evgeni]

it's not, I've just reported it to fapolicyd upstream: linux-application-whitelisting/fapolicyd#413