#8802 IPA test failing with long serial numbers
Closed: fixed by rcritten. Opened by edewata.

I'm trying to add 128-bit random serial numbers into PKI as described here:
https://github.com/dogtagpki/pki/wiki/Random-Certificate-Serial-Numbers-v3

The IPA installation and most of IPA tests seem to be working fine with shorter serial numbers (e.g. 24 bits). However, one of the tests sometimes fails with 32 bits, and it's consistently failing with 40 bits or longer.

This failure blocks PKI development since it breaks PKI CI.

Here are the logs:
https://github.com/edewata/pki/actions/runs/742693934

=================================== FAILURES ===================================
________________________ test_cert.test_0009_cert_find _________________________
self = <ipatests.test_xmlrpc.test_cert_plugin.test_cert object at 0x7fc8a77221c0>
    def test_0009_cert_find(self):
        """
        Verify that cert-find shows CA of the certificate without --all
        """
>       res = api.Command['cert_find'](min_serial_number=sn,
                                       max_serial_number=sn)['result'][0]
test_xmlrpc/test_cert_plugin.py:219: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
../ipalib/frontend.py:471: in __call__
    return self.__do_call(*args, **options)
../ipalib/frontend.py:499: in __do_call
    ret = self.run(*args, **options)
../ipalib/frontend.py:822: in run
    return self.forward(*args, **options)
../ipaclient/plugins/cert.py:135: in forward
    return super(cert_find, self).forward(*args, **options)
../ipalib/frontend.py:844: in forward
    return self.Backend.rpcclient.forward(self.forwarded_name,
../ipalib/rpc.py:1151: in forward
    return self._call_command(command, params)
../ipalib/rpc.py:1127: in _call_command
    return command(*params)
../ipalib/rpc.py:1281: in _call
    return self.__request(name, args)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
self = <ipalib.rpc.JSONServerProxy object at 0x7fc8a76f8be0>
name = 'cert_find/1'
args = ((), {'max_serial_number': 172438684545000970159254868079478095774, 'min_serial_number': 172438684545000970159254868079478095774, 'version': '2.240'})
    def __request(self, name, args):
        print_json = self.__verbose >= 2
        payload = {'method': unicode(name), 'params': args, 'id': 0}
        version = args[1].get('version', VERSION_WITHOUT_CAPABILITIES)
        payload = json_encode_binary(
            payload, version, pretty_print=print_json)
        if print_json:
            logger.info(
                'Request: %s',
                payload
            )
        response = self.__transport.request(
            self.__host,
            self.__handler,
            payload.encode('utf-8'),
            verbose=self.__verbose >= 3,
        )
        if print_json:
            logger.info(
                'Response: %s',
                json.dumps(json.loads(response), sort_keys=True, indent=4)
            )
        try:
            response = json_decode_binary(response)
        except ValueError as e:
            raise JSONError(error=str(e))
        error = response.get('error')
        if error:
            try:
                error_class = errors_by_code[error['code']]
            except KeyError:
                raise UnknownError(
                    code=error.get('code'),
                    error=error.get('message'),
                    server=self.__host,
                )
            else:
                kw = error.get('data', {})
                kw['message'] = error['message']
>               raise error_class(**kw)
E               ipalib.errors.ValidationError: invalid 'min_serial_number': can be at most 2147483647
../ipalib/rpc.py:1275: ValidationError
=========================== short test summary info ============================
FAILED test_xmlrpc/test_cert_plugin.py::test_cert::test_0009_cert_find - ipal...
================== 1 failed, 221 passed in 108.47s (0:01:48) ===================
Error: Process completed with exit code 1.

The PKI development build is available from this COPR repo:
https://copr.fedorainfracloud.org/coprs/edewata/pki-ca/builds/


serial_number is defined as an integer, thus it is validated to be an integer. Due to how it is implemented in XML-RPC, it cannot be more than 31 bit:

    kwargs = Param.kwargs + (
        ('minvalue', int, int(MININT)),
        ('maxvalue', int, int(MAXINT)),
    )

See https://docs.python.org/3/library/xmlrpc.client.html for details.

So to solve this we would need to:
- add a new parameter class to handle serial number, it has to be compatible with existing Int()
- change API of the cert* commands to use this new parameter class instead of Int()
- perhaps, add a logic to treat both strings and integers as XML-RPC will not work with true 128-bit numbers

A care should be taken with transformations because Python's unicode(int("large number")) applied to a large number would produce "L" suffix in the resulting string which is not what we want, for sure.

It's not just XML-RPC. JSON / JavaScript has a limitation, too. It cannot safely handle integers outside range -(2**53) - 1 to (2**53) - 1. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/MAX_SAFE_INTEGER

Can we safely increase the limit without breaking older clients or servers? I think random serial numbers require a new domain level.

A care should be taken with transformations because Python's unicode(int("large number")) applied to a large number would produce "L" suffix in the resulting string which is not what we want, for sure.

This is no longer true. It was a problem with Python 2 when we had a int type and a long type. Python 3 has one integer type and never adds a L suffix.

PS: There is no unicode() in Python 3 ;)

We definitely have to make it compatible with older clients.

@edewata how critical is to use 128-bit random serial numbers? Can we limit to 48-bit, for example?

Random serial numbers assume that there is virtually no chance of a collision. 48 or even 53 bits increase the change of a collision a lot. Like A LOT a lot.

The absolute minimum is 65 bits: 64 bit unsigned integer + one leading zero to make ASN.1 happy. The common approach is a 127bit unsigned integer + one leading zero. We cannot safely express the value as an integer in JSON. I guess we either need to use bytes here or perform some shenanigans with Decimals.

I am OK using bytes for the primary representation of the data. For old clients this means they would be unable to handle anything beyond the limits unless we fix the clients. We can certainly fix FreeIPA 4.6 clients (RHEL 7.9) but not anything older.

On the server side it is possible for IPA API to do proper detection of the new client and send back the right representation already so strictly speaking a new domain level is not really needed.

Older IPA servers won't be able to handle certificates with random serial numbers.

By the way how does Dogtag deal with large serial numbers? They cannot be safely used in JSON. Does Dogtag use text representation in JSON responses?

Larger entropy will reduce the risk of collisions, but there are actually 2 types of collisions:

  • collision between a new cert and another cert already in the local database (either created locally or replicated from another server)
  • collision between a new cert and another cert that has not been replicated to the local database

Most collisions will probably be the first type since certs are stored long term in the database. In this case I think the server should be able to handle collisions gracefully (i.e. cert enrollment should either fail or automatically find a new serial number), so this should not be an issue.

We need to worry about the second one since CA/DS will not see the conflict until the data is replicated, and I'm not clear how DS will handle replication conflicts, and whether the CA will need to revoke the conflicting certs. However, compared to the first type, there's a much shorter time between creating a cert and replicating it into all replicas, so the probability of having 2 identical serial numbers generated within that window should be much lower.

PKI will provide some parameters to configure the random serial number. By default it will use 128 bits. I guess if IPA is not ready yet you can configure it to use a shorter one temporarily, or just disable it for now. What do you think?

FYI, in JSON we are storing the serial numbers as hexadecimal strings:

{
    "total": 6,
    "entries": [
    {
        "id": "0x824dbe690f393bf49baa2f79b8fcf8d",
        "SubjectDN": "CN=localhost.localdomain,OU=pki-tomcat,O=EXAMPLE",
        "IssuerDN": "CN=Certificate Authority,O=EXAMPLE",
        ...

Question: are there other API parameters in the whole IPA API using the ipalib.parameters.Int type, for which the range -2**31..2**31-1 is too restrictive? We have to solve this for serial numbers, so it is a good time to see if there are other bugs lurking...

The other ones are uidNumber/gidNumber (they can be 2**32-1) but it is on purpose and should not be changed, as per recent discussion with Simo.

Question: are there other API parameters in the whole IPA API using the ipalib.parameters.Int type, for which the range -2**31..2**31-1 is too restrictive? We have to solve this for serial numbers, so it is a good time to see if there are other bugs lurking...

My PR https://github.com/freeipa/freeipa/pull/5709 lets us extend the values a bit. We are still limited by JSON restrictions. Can we introduce a new number type that transparently converts between hex string and base 10 number? Then we can use base 16 hex string on the wire and support hex string + base 10 as user input.

Note that existing ipalib.parameters.Int already supports octal numbers. It just doesn't add anything to support large numbers. So probably we don't need new type but rather can extend existing one to support hex and large numbers.

@ftweedal the Int limit is imposed by the xmlrpc protocol.

master:

  • a297ebbb8a277c7c3fcb44f6da182dde71447442 Add max/min safe integer

ipa-4-9:

  • 0c3a2dbfeaa73db868bacd4042de02a20b714d05 Add max/min safe integer

Metadata Update from @frenaud:
- Issue close_status updated to: fixed
- Issue status updated to: Closed (was: Open)

Is this really fixed though? PKI's goal is to use 128-bit serial numbers. Will IPA support only 52-bit serial numbers for now? In that case PKI needs to provide a configuration parameter for serial number length and IPA needs to specify it when installing PKI.

No, I think we need to reopen this bug and add the remaining patchs (to be written) that extend API for serial_number to handle string-based variant on both server and client side.

Metadata Update from @abbra:
- Issue status updated to: Open (was: Closed)

Just FYI, the initial implementation of RSNv3 in PKI has been merged:
https://github.com/dogtagpki/pki/pull/3918
This is not final yet. There will be additional changes later.

The COPR build is available for testing from this repo:
https://copr.fedorainfracloud.org/coprs/g/pki/master/

This is planned to be released in PKI 11.2 for F37.

I poked at this a bit yesterday. According to the KRA RSNv3 PR the values can be as big as 160-bit (https://github.com/dogtagpki/pki/pull/3920)

I think we can create a new parameter class, SerialNumber, to handle the validation of the huge values internally using int().

Random Serial numbers represent a big enough change IMHO that we allow it for new installs only. This avoids any replication, old client, etc issues. Since the API is downloaded from the server even older clients should be able to interoperate.

A new domain-level would provide a way to migrate by moving all servers to dl2, then enabling RSN, assuming that is possible post-install in PKI. I think I'd prefer to avoid this if possible.

This was addressed in this commit with the addition of a SerialNumber parameter type. The serial number was being treated as a decimal internally when that was unnecessary since we do not do math on the value beyond hex conversion. Treat it as a string except in the few cases we need a decimal value. We will not transmit it as decimal.

master: 83be923ac566df53472e2b7b15814f1a1d00e933

Metadata Update from @rcritten:
- Issue close_status updated to: fixed
- Issue status updated to: Closed (was: Open)

Metadata