Podman: /libpod/networks/create API generates broken conflist on non-conforming Mask values

[danopia]

/kind bug

Description

A valid request body to the create network API looks like:

{"Subnet":{"IP":"10.10.1.0","Mask":"////AA=="}}

Note that Mask is decoded as base64. If the Mask can decode, but isn't an expected bitmask afterwards, Podman stores an invalid conflist to disk.

Bodies that are accepted and produce an invalid conflist include:

• {"Subnet":{"IP":"10.10.1.0","Mask":"ffffff80"}}
• {"Subnet":{"IP":"10.10.1.0","Mask":""}}
• {"Subnet":{"IP":"10.10.1.0","Mask":"aaaaaaaaaaaaaaaaaaaa"}}

Steps to reproduce the issue:

1. sudo podman system service -t 30 tcp:127.0.0.1:8410

2. curl -H 'Content-type: application/json' -XPOST http://localhost:8410/v1.0.0/libpod/networks/create --data '{"Subnet":{"IP":"10.10.1.0","Mask":""}}'

3. curl http://localhost:8410/v1.0.0/libpod/networks/testnet/json

Describe the results you received:

[{"cniVersion":"0.4.0","name":"testnet","plugins":[{"bridge":"cni-podman1","hairpinMode":true,"ipMasq":true,"ipam":{"ranges":[[{"gateway":"?01","subnet":"\u003cnil\u003e"}]],"routes":[{"dst":"0.0.0.0/0"}],"type":"host-local"},"isGateway":true,"type":"bridge"},{"capabilities":{"portMappings":true},"type":"portmap"},{"backend":"","type":"firewall"},{"domainName":"dns.podman","type":"dnsname"}]}]

In particular: {"gateway":"?01","subnet":"\u003cnil\u003e"}

Describe the results you expected:

An error message on creation.

Additional information you deem important (e.g. issue happens only occasionally):

Nothing in the API docs says that Mask should be base64. https://docs.podman.io/en/latest/_static/api.html#operation/libpodCreateNetwork

With the invalid network created, the list and describe APIs show the invalid network config. However, creating new networks no longer works. The error payload is simply {"cause":"invalid IP address: ?01","message":"invalid IP address: ?01","response":500}. It seems that creating a new network has the effect of performing extra checks on all existing conflists.

Output of podman version:

Version:      2.0.4
API Version:  1
Go Version:   go1.14
Built:        Thu Jan  1 01:00:00 1970
OS/Arch:      linux/amd64

Output of podman info --debug:

host:
  arch: amd64
  buildahVersion: 1.15.0
  cgroupVersion: v1
  conmon:
    package: 'conmon: /usr/libexec/podman/conmon'
    path: /usr/libexec/podman/conmon
    version: 'conmon version 2.0.18, commit: '
  cpus: 2
  distribution:
    distribution: debian
    version: "10"
  eventLogger: file
  hostname: penguin
  idMappings:
    gidmap: null
    uidmap: null
  kernel: 5.4.40-04224-g891a6cce2d44
  linkmode: dynamic
  memFree: 15240572928
  memTotal: 15260385280
  ociRuntime:
    name: runc
    package: 'runc: /usr/sbin/runc'
    path: /usr/sbin/runc
    version: |-
      runc version 1.0.0~rc6+dfsg1
      commit: 1.0.0~rc6+dfsg1-3
      spec: 1.0.1
  os: linux
  remoteSocket:
    path: /run/podman/podman.sock
  rootless: false
  slirp4netns:
    executable: ""
    package: ""
    version: ""
  swapFree: 0
  swapTotal: 0
  uptime: 23h 1m 45s (Approximately 0.96 days)
registries:
  search:
  - docker.io
  - quay.io
store:
  configFile: /etc/containers/storage.conf
  containerStore:
    number: 0
    paused: 0
    running: 0
    stopped: 0
  graphDriverName: btrfs
  graphOptions: {}
  graphRoot: /var/lib/containers/storage
  graphStatus:
    Build Version: 'Btrfs v5.2.1 '
    Library Version: "102"
  imageStore:
    number: 2
  runRoot: /var/run/containers/storage
  volumePath: /var/lib/containers/storage/volumes
version:
  APIVersion: 1
  Built: 0
  BuiltTime: Thu Jan  1 01:00:00 1970
  GitCommit: ""
  GoVersion: go1.14
  OsArch: linux/amd64
  Version: 2.0.4

Package info (e.g. output of rpm -q podman or apt list podman):

podman/unknown,now 2.0.4~1 amd64 [installed]

Additional environment details (AWS, VirtualBox, physical, etc.):

lxc container (Chromebook Linux Beta)

[Luap99]

I did some debugging and found out that the problem resides in the golang net package. net.IP has a unmarshal method but net.IPMask has not. That's the reason why IP only accept a string (contrary to the podman api documetation), while Mask does not. The correct way to define the mask is [255,255,255.0], like the doc states. Not sure why base64 works.

So a vaild request would be {"Subnet":{"IP":"10.10.1.0","Mask: [255,255,255,0]}}.

[danopia]

Interesting, I tried having a byte slice in both arguments, but forgot to try a byte slice for just Mask. Good to see that works.

I tried base64 because it's how IPmask serializes: https://play.golang.org/p/BYbprvC-EPi which would logically mean that it's accepted when parsing as well.

[danopia]

So, using byte arrays actually makes it a bit easier to see what's going on here, since the unmarshalling isn't the problem.

Here's a byte slice of the wrong length:

> curl -H 'Content-type: application/json' -XPOST http://localhost:8410/v1.0.0/libpod/networks/create'?name=testnet' \
    --data '{"Subnet":{"IP":"10.10.1.0","Mask":[0,0]}}'
{"Filename":"/etc/cni/net.d/testnet.conflist"}

> grep -B1 -A2 subnet /etc/cni/net.d/testnet.conflist
                  {
                     "subnet": "\u003cnil\u003e",
                     "gateway": "?01"
                  }

And a nonstandard IPMask slice produces somewhat different, and slightly less broken networks:

> curl -H 'Content-type: application/json' -XPOST http://localhost:8410/v1.0.0/libpod/networks/create'?name=testnet' \
    --data '{"Subnet":{"IP":"10.10.1.0","Mask":[0,0,255,0]}}'
{"Filename":"/etc/cni/net.d/testnet.conflist"}

> grep -B1 -A2 subnet /etc/cni/net.d/testnet.conflist
                  {
                     "subnet": "10.10.1.0/0000ff00",
                     "gateway": "?0101"
                  }

[zhangguanzhang]

doc is :

Array of integers <uint8> (An IPMask is a bitmask that can be used to manipulate IP addresses for IP addressing and routing.)See type IPNet and func ParseCIDR for details.
--

[rhatdan]

Can someone open a PR to improve our documentation so that this becomes easier to discover?

[zhangguanzhang]

Can someone open a PR to improve our documentation so that this becomes easier to discover?

I think docs is ok， but I will submit pr to prevent this error from happening

[Luap99]

I think docs is ok， but I will submit pr to prevent this error from happening

Doc clearly states the IP has to be an unit8 array which is wrong (throws error), instead it has to be a string.

[danopia]

There's a handful of angles related to this ticket... nice and convoluted 😄

The docs for both Range and Subnet (two different /networks/create fields) are seemingly from the raw godoc of net.IPNet:

• IP: Array of integers  - Note that in this documentation, [... irrelevant info about golang internals ...]
• Mask: Array of integers - See type IPNet and func ParseCIDR [useless golang struct references]

Note that both fields claim to accept the same data type.

In reality, the podman API demands these types:

• IP: String form of an IP Address (e.g. "10.1.0.0")
• Mask: JSON array of integers in the range 0-255 (e.g. [255,255,255,0])
  
  
  
  • OR Base64 string of a bitmask (e.g. "////AA==")
  
  

So — on top of the missing validation which #7267 addresses — the docs also are plainly wrong about the IP subfield, and at best unclear about the Mask subfield.

(It _seems_ like Range and Subnet should merely accept a standard CIDR string and use net.ParseCIDR() to produce the actual values used internally. However, that is more of a feature request, presumably?)

[mheon]

Ideally these would both be strings, but that would also technically be a breaking API change.

At this point, I think we add validation to subnet to ensure it is valid, update the documentation to indicate the correct types, and change both to a string next time we make a breaking change to the API.

[github-actions[bot]]

A friendly reminder that this issue had no activity for 30 days.

[rhatdan]

@Luap99 @mheon Any movement on this issue?

[rhatdan]

@Luap99 @mheon @zhangguanzhang @danopia Is this still a valid issue? Or can I close?

[danopia]

The headline issue of writing broken conflists to disk seems fixed. I just tried all my original repros on 2.2.1 and it looks fine.

The REST API docs are still wrong and/or misleading. (one item on my notes...)

[baude]

mind giving us a doc pr for that?

[zhangguanzhang]

code fixes it, but doc need update