Fixed dangling else: in issuer_or_subject_length()

[42xnor]

Changed the else to an if tot_len == 0: to match the doctring's listed functionality "raises: TypeError if return value is 0". The previous code would raise TypeError if the parameter common was falsy. It now raises the error if tot_len is zero.

[42xnor]

Fixes issue #30 .

[brentru]

@DJDevon3 Since you looked at this library recently, would you be able to review this PR?

[DJDevon3]

@42xnor I don't think checking for 0 is the right approach for a CSR. In my opinion all fields (except organization unit) should be required. Each field adds 11 plus len(data). By checking for 0 you could fill in 1 field with 1 digit and it would pass the raise but still fail the rest as empty str with a type error. To make a field optional it should have = None added to the type. I think it's a better idea to annotate that most fields are required. This is in better spirit of a CSR. You don't have to fill it in with factual information but you do at least have to fill it with something.

def issuer_or_subject_length(country: str, state_prov: str, city: str, org: str, common: str, org_unit: str = None) -> int:
    """
    Returns total length of provided certificate information.

    :param str country: Country of certificate (Required)
    :param str state_prov: State/province of certificate (Required)
    :param str city: City of certificate (Required)
    :param str org: Organization of certificate (Required)
    :param str common: Common data of certificate (Required)
    :param str org_unit: Organization unit of certificate (Optional)
    :raises: TypeError if return value is less than 55 or if any required field is empty
    :return: Total length of provided certificate information.
    """
    empty_fields = [field_name for field_name, field_value in {"Country": country, "State/Province": state_prov, "City": city, "Organization": org, "Common Data": common}.items() if not field_value]
    if empty_fields or (org_unit == ''):
        raise TypeError(f"The following Required fields cannot be empty: {', '.join(empty_fields)}.")
    
    info_lengths = [11 + len(item) for item in (country, state_prov, city, org, common) if item]
    total_length = sum(info_lengths)
    
    if total_length < 55:
        raise TypeError("Required fields cannot be blank.")
    
    return total_length

Since each required field is prepended with 11 and there are 5 required fields then 55 should be the minimum to check against. I've also added some code that will specify which required fields were left blank during the request so they can be filled in.

Here's an example to try with the function.

import busio
import board
from adafruit_atecc.adafruit_atecc import ATECC
import adafruit_atecc.adafruit_atecc_asn1 as asn1

i2c = busio.I2C(board.SCL, board.SDA, frequency=70000)
atecc = ATECC(i2c, address=0x60, debug=False)

# two fields left blank intentionally to trigger an error
issuer = asn1.issuer_or_subject_length(country="", state_prov="", city="New York", org="Adafruit", common=atecc.serial_number, org_unit=None)
print(f"Issuer Fields Length: {issuer}")

Serial output:

code.py output:
Traceback (most recent call last):
  File "code.py", line 9, in <module>
  File "/lib/adafruit_atecc/adafruit_atecc_asn1.py", line 259, in issuer_or_subject_length
TypeError: The following Required fields cannot be empty: State/Province, Country.

Is this acceptable or did I completely miss the boat?

[DJDevon3]

and when completely filled in and no optional org unit

issuer = asn1.issuer_or_subject_length(country="US", state_prov="NY", city="New York", org="Adafruit", common=atecc.serial_number)
print(f"Issuer Fields Length: {issuer}")

serial output

Issuer Fields Length: 93

as long as it's greater than 55 it'll pass.

[DJDevon3]

After thinking about it, the change I proposed would break any pre-existing chips that have any empty fields in the now required fields. I'm conflicted about this and will have to bring this up in a circuit python meeting.

Also the parameter common should never be false. Common is the device serial number and that is autofilled by the script (and chip itself) during chip lock. During a domain name CSR for example, common would be the domain name to protect. Since the address you want to protect for the ATECC crypto chip is the serial number, that is what is passed into the common field. Trying to set common to false would be the same as trying to set the domain name of a CSR to false which would fail every time because you can't ignore the common part of the CSR. A CSR should not function correctly doing that. So if your CSR is failing when passing false with common that should be expected behavior.