wfe: Encoding nil arrays as `[]` and not `null`

[alexzorin]

While working on https://github.com/certbot/certbot/pull/7254 I noticed that Certbot was crashing (NoneType is not an iterable) against responses from Pebble.

It comes down to JSON being encoded as:

{
   "status": "deactivated",
   "identifier": {
      "type": "dns",
      "value": "www.example.com"
   },
   "challenges": null,
   "expires": "2019-07-24T23:03:42Z"
}

rather than

   "challenges": [],

I think it also affects other fields, such as the contact field in registration resources.

Reading RFC8555, I don't think that there is anything that describes the canonicalized value of an empty array. All it says is that the fields are required, which means that omitempty is not suitable.

Personally, I would lean towards [] because it is closer in meaning to "empty array" than null is.

In Go, you can control which way it gets encoded by setting a value of make([]T, 0) rather than nil.

But I'm honestly unsure whether this should be resolved on the server or client. It can easily be fixed in either place, but it would be good to gather some thoughts about what the right thing to do is and then identify and fix all the places where it could occur.

[jsha]

I think it makes sense to fix this in Pebble and always show []. I think treating empty arrays as nil is a Go-ism that leaks through into the JSON unmarshaling. Thanks for spotting this!

[eggsampler]

The RFC refers to the challenges object as 'array of object' so I think it does make sense to marshal any empty object as it's zero value in Go, rather than Go marshalling nil -> null.

It probably would make sense to codify this too, as there is already somewhat of a precedence for sending objects which play nicer with decoders: "{}" when responding to challenges

[jsha]

Just to be pedantic: The zero value of var []foo in Go is in fact nil. :-) What we want is to render empty challenges as []foo{}, which is not the zero value.

[eggsampler]

Ah sorry, initialised empty values :) - I've run up against this in Go myself where an acme server wouldn't accept "contact":null at some point.

[felixfontein]

The contact field in account information is optional, so I don't think Pebble should return [] if there are no contacts. The aim of Pebble is (among others) to point out potential bugs to implementers, so if someone forgets to support missing contact information when querying account information, it would be nice if Pebble would trigger that bug.

[alexzorin]

Agreed on all points. I mentioned contact because Pebble is currently encoding it as null, which can trigger decoding errors on the client.

{
   "status": "valid",
   "contact": null,
   "key": {
      "kty": "RSA",
      "n": "3kI5bizPH4idekKf2oKJCIB31u4IyxyDWwQgiBJf02ldQs-umnjVP65-uLnqNnhhRibUdGlPXdeMOl8Dl6uleEPstRxKc4j9y-p6SKt8RiDG2RBMiVegdYoonnaroaZlDj73p72SyaynNMrhx6FJS55nrKs0cDVXvxYrWaNRlmTbffLzT1_xgacofbBeTE6PJ-9p_2TRMYyukv5q9_GRe0MZx4X7BJVzluoSOtPTZvayWEzbO9vOf-_3PwRVT2hY8lQPvuYYAqF7iP8Cb9fEPJ7WathoCnv9nTB34MzoXvarXSakvNxFyMp5vFGux18r0XaHy2lPNu8MY-wgqoK7RQ",
      "e": "AQAB"
   }
}

It should be omitted entirely per your reasoning.

[jsha]

Oh right, good point @felixfontein. And now I'm remembering fixing a similar bug in Boulder. If you define contacts as a *[]string (optional), you should be able to represent the "not present" state as a nil value for the pointer (vs a nil value for the slice, which I think is what we're getting now). That should then get rendered to JSON as nothing, rather than a field with the value null.

[cpu]

Fixed in master. Thanks everyone!