rootless-containers/slirp4netns

IPv6: hardcoded address `fd00::2` violates RFC 4193

AkihiroSuda opened this issue · 16 comments

https://github.com/rootless-containers/slirp4netns/blob/v1.1.9/slirp4netns.1.md

Default configuration:

* MTU:               1500
* CIDR:              10.0.2.0/24
* Gateway/Host:      10.0.2.2    (network address + 2)
* DNS:               10.0.2.3    (network address + 3)
* IPv6 CIDR:         fd00::/64
* IPv6 Gateway/Host: fd00::2
* IPv6 DNS:          fd00::3

According to RFC 4193, the addresses in fd00::/8 seems to need to be random

https://tools.ietf.org/html/rfc4193
https://networkengineering.stackexchange.com/questions/30128/ipv6-ula-vs-link-local

(slirp4netns --enable-ipv6 is still experimental and we can have breaking changes)

@giuseppe Could you take a look? What should we do with this?

is it really a risk for slirp4netns though? IPv6 is not routed so there is not really risk of collisions, or do you think we should just pick a random IP to honor the RFC? I am fine if we go this way (and if any problem arises users of slirp4netns can force the IP addr)

I think honoring RFC is not necessarily, but probably we will eventually need to support specifying random addr.
Not an urgent task.

I think it should pick a random one. For users who want a fixed subnet, a --cidr6 option should be added.

@pfandl @Luap99 Anybody of you interested in opening a PR?

If I can find some time, sure, but probably not in the next few weeks.

@AkihiroSuda do you want just commit pfandl@8cc216f ?

Thanks @pfandl for working on this!

  • For compatibility, can we keep the current wrong behavior by default, and opt-in to the correct behavior with a new flag like --ipv6-random? I guess this will help Podman to support older version of slirp4netns and the new version simultaneously (cc @rhatdan)
  • Could you update the man page?
  • Could you use strncpy family rather than strcpy family?
  • Would it be possible to drop dependency on libcrypt?

Thanks @pfandl for working on this!

* For compatibility, can we keep the current wrong behavior by default, and opt-in to the correct behavior with a new flag like `--ipv6-random`? I guess this will help Podman to support older version of slirp4netns and the new version simultaneously (cc @rhatdan)

yes

* Could you update the man page?

yes

* Could you use `strncpy` family rather than `strcpy` family?

yes

* Would it be possible to drop dependency on libcrypt?

Not too sure about that, I tried to mimic the sample code from the RFC
It is not clear to me if this is just a recommendation (at step 4 to compute a SHA1 hash) rather than a requirement. If it is a requirement and somebody knows how to compute the hash without libcrypt i guess we can drop the library, yes.

Can we take the necessary bits from here? https://code.woboq.org/gcc/libiberty/sha1.c.html

Also @AkihiroSuda as stated here #253 (comment) I took the mac of the lo device, which is zero, do you have any idea what we should take instead? See step 2 from the RFC .

@AkihiroSuda can we bump libslirp version? I am already using the hostx_fwd functions and we need a fix there for IPv6 forwarding anyway and I think it would be less work for me to bring all in at once?

Yes.

Using #if directive is more preferable, but not necessary if it too much complicates the code.

#if SLIRP_CONFIG_VERSION_MAX >= 3

SHA1

Dependency on libcrypt is fine, as it should be available on all distros.

I took the mac of the lo device, which is zero, do you have any idea what we should take instead? See step 2 from the RFC .

Why not take the MAC of tap0?
If the MAC cannot be take, we can just use /dev/urandom.

I think #if will work, thanks for the info. I guess it should be ready coming weekend.

@AkihiroSuda with man page, do you mean README.md?

I'm late to the party. I prefer slirp4netns to support the RFC.

Thanks for your work, @pfandl. Note that the sample code for generating random Global IDs is just a sample. The minimal requirement is for the number to be pseudo-random. I believe getrandom(2) should answer this need.

Here is the current PR for this issue: #276 (It seems that GitHub did not link them when the new PR was created)