Bump the shared secret maximum length to 256

[DimitriPapadopoulos]

• The FreeRADIUS library bumped it from 6 * 16 to 16 * 16 as well:
  User-password and shared secret limits too low, should be 128. FreeRADIUS/freeradius-client#112
  FreeRADIUS/freeradius-client@6ec5f43
• A value of 256 covers most commercial RADIUS servers, except the AWS Directory Service with an upper limit of 512 characters.
• In practice, I doubt anyone uses shared secrets larger than 256 out there.

Fixes #103.

[nmav]

This is an ABI breaking change. Please bump V_CURRENT in configure.ac and add a NEWS entry as well.

[nmav]

Thanks. Can you merge the changes into a single commit with a signed-off-by?

[DimitriPapadopoulos]

I could be wrong, but I think the abi-check CI error appeared after simply squashing the commits. Strange.

Any way, do we really need the major version bump?

The breaking case at hand is the one where software is built against a new version of the radcli library/header which advertises the new higher values of AUTH_PASS_LEN and MAX_SECRET_LENGTH, and run against an old version of the radcli dynamic library which uses the old lower values of AUTH_PASS_LEN and MAX_SECRET_LENGTH internally.

However, I question whether this changes the ABI: functions and their signatures don't change and abidiff shouldn't notice any difference. On the other hand, you may argue that the API changes, but the API is not the ABI. As far as I can understand, bumping sonames is usually associated with ABI changes.

But then again, even the API change is questionable. It all depends on the underlying promise made by putting AUTH_PASS_LEN and MAX_SECRET_LENGTH in the public radcli header. So what is the underlying promise? I would expect radcli to fail gracefully in all cases, if it cannot handle the size of the shared secret, whatever the values of AUTH_PASS_LEN and MAX_SECRET_LENGTH at build time. I don't see the exact maximal sizes as a hard part of the contract. The hard part of the contract is that radcli should bail out gracefully with a good error message when it cannot cope.

Finally, I don't understand the error message, which seems to imply a 6 → 5 instead of a 5 → 6 modification of the soname. And functions are not removed, where does this come from?

Checking radcli ABI
if test -f "./devel/ABI-$(uname -m).dump" && test "" != 1; then \
	abidiff --no-added-syms --hd2 ./include/radcli/ ./lib/.libs/libradcli.so devel/ABI-x86_64.dump; \
fi
ELF SONAME changed
Functions changes summary: 63 Removed, 0 Changed, 0 Added functions
Variables changes summary: 0 Removed, 0 Changed, 0 Added variable

SONAME changed from 'libradcli.so.6' to 'libradcli.so.5'

63 Removed functions:

[nmav]

The version bump is necessary. A software compiled with the new version cannot be linked with the older version since the sizes differ.

[DimitriPapadopoulos]

Why cannot it be linked? I don't see the size being part of the API. It's just a constant.

I cannot make any sense of the CI error, but I suspect it is unhappy with the 5 → 6 bump and suggests bumping down 6 → 5.

[nmav]

Why cannot it be linked? I don't see the size being part of the API. It's just a constant.

I cannot make any sense of the CI error, but I suspect it is unhappy with the 5 → 6 bump and suggests bumping down 6 → 5.

What happens when an application linked with the new constant is used with an older library that has the previous value of the constant, when a username is copied that exceeds the previous value?

[nmav]

This has been merged already.

[DimitriPapadopoulos]

I think such constants should not be part of the ABI. Functions that use that constant internally should exit gracefully, whenever an argument is incompatible, without the need to expose a constant. Client programmer, don't really care about the exact value of a constant, they only care whether the function is able to process or not a given argument. Said otherwise, I believe client code should look like this:

    if (process(arg) == 0) {
        log("cannot process string of length %d", strlen(arg));
        goto error;
    }

instead of:

    if strlen(arg) > AUTH_PASS_LEN) {
        log("cannot process string of length %d > %d", strlen(arg), AUTH_PASS_LEN);
        goto error;
    } else {
        process(arg);
   }

Currently, (i) I am not certain we handle or give the means to handle such errors gracefully, and (ii) it's indeed unclear whether the constant is part or not of the contract. Sure, it's in the header file, but code client should not use it. To be on the safe side, let's consider the constant is part of the ABI. I nevertheless would like to see it removed from the ABI in the future.