Container with network alias containing a space causes aardvark-dns crash

[l-austenfeld]

When you create a container that has a network alias containing a space, a malformed aardvark-dns config will be created that will cause it to crash.

❯ podman --version
podman version 5.1.1
❯ /usr/lib/podman/netavark --version
netavark 1.11.0
❯ /usr/lib/podman/aardvark-dns --version
aardvark-dns 1.11.0

Steps to reproduce:

• podman network create test to create a network with dns enabled
• podman run --rm -it --network test debian to create a working container. You can see a aardvark-dns process running in htop for example and dns requests in the container are working
• podman run --rm -it --network test --network-alias "test alias" debian in a second terminal. If you observe the aardvark-dns process using htop it will disappear when the container is started and dns in both containers is now broken

Contents of /run/user/1000/containers/networks/aardvark-dns/test:

10.89.1.1
8965e883bfc09b4ce60728c66bc85ed62fa589218174dbbecca1a00dcf0eafc7 10.89.1.4  eloquent_keller,8965e883bfc0
f8bd2db9d504b780bd9b5567e2823ce99db12d207cf468d8614d36a9ba242231 10.89.1.5  goofy_wilbur,test alias,f8bd2db9d504

[Luap99]

Such a name is not a valid dns or hostname, thus it would make the most sense for podman to reject it right away.

Of course we can improve error handling for aardvark-dns too but this is not a name that is going to work regardless.

[l-austenfeld]

I agree that such a name does not make sense, in my case it is the result of a forgejo actions runner generating a ci job name like build-image (amd64) as part of a job matrix (job names themselves are not allowed to have spaces) and then setting that as a container name alias.
Both podman and docker accept aliases like this (I assume for uses other than dns).

I don't know the best way to handle this, but rejecting invalid names before generating the config file sounds reasonable.

[l-austenfeld]

I temporarily fixed the issue on my system using the following patch:

diff --git a/src/dns/aardvark.rs b/src/dns/aardvark.rs
index 6c1314d..03906b9 100644
--- a/src/dns/aardvark.rs
+++ b/src/dns/aardvark.rs
@@ -282,7 +282,12 @@ impl Aardvark {
     }
 
     fn commit_entry(entry: &AardvarkEntry, mut file: File) -> Result<()> {
-        let container_names = entry.container_names.join(",");
+        let container_names = entry.container_names
+            .iter()
+            .filter(|s| !s.contains(' ') && !s.contains(','))
+            .map(|s| s.as_str())
+            .collect::<Vec<&str>>()
+            .join(",");
 
         let ipv4s = entry
             .container_ips_v4

This just filters out any name containing or ,.
Feel free to adapt this to you needs or ignore it should it not meet the code standards or if you have a better solution.

[Luap99]

Ideally podman would reject this right away when you try to create such a container. But as you mentioned if there is automated tooling that creates names like that today then returning an error would be breaking change.

So yes you patch look reasonable if the goal is to drop all invalid names. Although I would say we should drop all invalid dns names not just space and comma

[l-austenfeld]

I skimmed some of the RFCs that specify the DNS and concluded that I am not qualified to properly implement this.

I have another patch that filters all names through idna::domain_to_ascii_cow and discards names that fail the conversion. I can open a pull request if you want.

Another solution would be to do some basic checking here and to proper checking in aardvark-dns.
Currently aardvark-dns just calls String::to_lowercase (see here and here) which is only correct as long as the name only contains ASCII.