CertificationRequestInfo Attributes should not be optional

[gareththered]

Currently, within the csr.py module CertificationRequestInfo is defined as:

class CertificationRequestInfo(Sequence):
    _fields = [
        ('version', Version),
        ('subject', Name),
        ('subject_pk_info', PublicKeyInfo),
        ('attributes', CRIAttributes, {'implicit': 0, 'optional': True}),
    ]

However, there is no mention in RFC 2986 of the attributes field being optional (as shown above). Also, OpenSSL's req has the following to say about it within it's man pages under the -asn1-kludge option:

More precisely the Attributes in a PKCS#10 certificate request are defined as a SET OF Attribute. They are not OPTIONAL so if no attributes are present then they should be encoded as an empty SET OF. The invalid form does not include the empty SET OF whereas the correct form does.

I currently cannot create a CertificationRequestInfo with an empty SET OF attributes. That is, I either have to leave it out (non-compliant) or add at least one attribute (which I don't need).

[wbond]

I believe the optional part is there for parsing the invalid form. You can see it mentioned in the changelog under version 0.13.0.

Can you post the code you are using where you try to create a CRI with an empty set of attributes? It wouldn't surprise me if the optional flag is causing the attributes to be skipped if they are empty. I'll just be easier for me to debug/fix if I start from where you are.

[gareththered]

The following example I've cobbled together shows the issue:

from asn1crypto import algos, csr, keys, pem, core
from asn1crypto.x509 import Name

#-------------------------------------------

# Copied from another module and simplified
# for this example

from Crypto.Hash import SHA256
from Crypto.PublicKey import RSA
from Crypto.Signature import PKCS1_v1_5
from asn1crypto.core import OctetBitString
import oscrypto.asymmetric

class Signature(OctetBitString):
    SUPPORTED_ALGORITHMS = {
        '1.2.840.113549.1.1.11': (SHA256, PKCS1_v1_5)
        # More OIDs here...
    }
    def __init__(self, data, key, algorithm):
        oid = algorithm['algorithm'].dotted
        param = algorithm['parameters']
        if oid in self.SUPPORTED_ALGORITHMS:
            hash, scheme = self.SUPPORTED_ALGORITHMS[oid]
            hash_value = hash.new(data)
            signer = scheme.new(key)
            signature_value = signer.sign(hash_value)
            super().__init__(bytes(signature_value))

#------------------------------------------------

with open('test.key', 'rb') as f:
    key_file = f.read()
    if pem.detect(key_file):
        _, _, key_file = pem.unarmor(key_file)
    priv_key = RSA.import_key(key_file)
    # TODO: key is PKCS#1 RSA - there has to be a better way than:
    pk_info = oscrypto.asymmetric.load_private_key(key_file).public_key.asn1

cri = csr.CertificationRequestInfo()
cri['version'] = 'v1'
cri['subject'] = Name.build(
    {'country_name': 'GB', 'common_name': 'Gareth Williams'}
)
cri['subject_pk_info'] = pk_info

# THIS ONE:
#cri['attributes'] = csr.CRIAttributes()

csr = csr.CertificationRequest()
csr['certification_request_info'] = cri
csr['signature_algorithm'] = algos.SignedDigestAlgorithm(
    {'algorithm': 'sha256_rsa'}
)
sig = Signature(cri.dump(), priv_key, csr['signature_algorithm'])
csr['signature'] = sig

with open('test_csr.der','wb') as csrfile:
    csrfile.write(csr.dump())

Ran as is, it will create a CSR without the attribute field and is therefore non-compliant.

With the comment (line below # THIS ONE:) removed, I get:
ValueError: Value for field "attributes" of asn1crypto.csr.CertificationRequestInfo is not set

I've also tried changing that line to:
cri['attributes'] = csr.CRIAttributes(value=core.Null())
but I get:
TypeError: 'Null' object is not iterable

I've tried many variations on the above, and get nowhere.

[joernheissler]

This works:

cri['attributes'] = csr.CRIAttributes([])

[gareththered]

@joernheissler - thank you. Annoyingly, I didn't try that one! I've just confirmed with openssl req that with the above, the generated request does indeed have the empty SET OF Attributes.

[wbond]

I'm going to reopen this, as a record that we should find a way to parse the invalid encoding, but always generate the valid encoding.

It may end up being we need to add a concept like "optional": "parse" instead of "optional": True to indicate we should allow it to be absent when parsing. We have some other parsing-only configuration, like certain structures we allow invalid string tags, but always generate the correct tag when dumping.

[joernheissler]

Those snippets run with "optional": False and "optional": True:

#!/usr/bin/env python3

from base64 import b64decode, b64encode

from asn1crypto.csr import CertificationRequestInfo
from asn1crypto.keys import PublicKeyInfo
from asn1crypto.x509 import Name

pub = PublicKeyInfo.load(b64decode(
    "MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAE4wKygiF54dF09fhqdWV8wgE8jD"
    "GjxDXQG0iW4UVv4e0jJn63lSz1QStNaG/QPcXuq35v3e9vjS+nZDU/+fR7Bw=="
))

cri = CertificationRequestInfo({
    "version": "v1",
    "subject": Name.build({"common_name": "foo"}),
    "subject_pk_info": pub,
    "attributes": [],
})

print(b64encode(cri.dump(True)).decode())
cri["attributes"] = None
print(b64encode(cri.dump(True)).decode())

#!/usr/bin/env python3

from base64 import b64decode
from asn1crypto.csr import CertificationRequestInfo

a = CertificationRequestInfo.load(b64decode(
    "MG4CAQAwDjEMMAoGA1UEAwwDZm9vMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAE4wKygiF54dF0"
    "9fhqdWV8wgE8jDGjxDXQG0iW4UVv4e0jJn63lSz1QStNaG/QPcXuq35v3e9vjS+nZDU/+fR7Bw=="
))

b = CertificationRequestInfo.load(b64decode(
    "MHACAQAwDjEMMAoGA1UEAwwDZm9vMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAE4wKygiF54dF0"
    "9fhqdWV8wgE8jDGjxDXQG0iW4UVv4e0jJn63lSz1QStNaG/QPcXuq35v3e9vjS+nZDU/+fR7B6AA"
))

print(a.native)
print(b.native)

Perhaps this optional thing isn't working at all?

[wbond]

The encoding from a seems to be correct, and is optionally omitting the attributes since it is "empty" - https://lapo.it/asn1js/#MG4CAQAwDjEMMAoGA1UEAwwDZm9vMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAE4wKygiF54dF09fhqdWV8wgE8jDGjxDXQG0iW4UVv4e0jJn63lSz1QStNaG_QPcXuq35v3e9vjS-nZDU_-fR7Bw.

The one from b seems incorrect, as it has appending a Null item to the end - https://lapo.it/asn1js/#MHACAQAwDjEMMAoGA1UEAwwDZm9vMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAE4wKygiF54dF09fhqdWV8wgE8jDGjxDXQG0iW4UVv4e0jJn63lSz1QStNaG_QPcXuq35v3e9vjS-nZDU_-fR7B6AA.

[joernheissler]

The encoding from a seems to be correct

The one from b seems incorrect, as it has appending a Null item to the end

I think b is correct while a is not. Compare to CSRs generated by any reputable software.

[wbond]

Shows how much I remember of the nuance of ASN.1 encoding off of the top of my head.

[gareththered]

'b' also aligns with the statement from OpenSSL's req man page where further down the man page under DIAGNOSTICS it again mentions the expected 0xa0 0x00 of the empty SET OF.

[wbond]

Oh, wait, one is a correct encoding of the field omitted and the other is an empty set?

It doesn't sound to me like optional is broken, at all, if it lets you omit a field. That is the whole point.

Now, setting a field with the type SET OF to Python None resulting in an empty set, whereas setting it to Python [] not creating an empty set sounds like an inconsistency that should probably be investigated. In my mind a None should probably result in a set being omitted if it is optional.

There is clearly an issue that CertificateRequestInfo has the field as marked optional - it should only be optional for parsing. But that doesn't indicate that optional is broken.

[joernheissler]

It doesn't sound to me like optional is broken, at all, if it lets you omit a field. That is the whole point.

I mean if I set 'optional': False I can still omit the field. My expectation was to get some Exception instead.

[wbond]

I mean if I set 'optional': False I can still omit the field. My expectation was to get some Exception instead.

Can you provide some sample code that shows this?

It honestly has been a while since I have done anything with ASN.1 serialization, and I may just be not thinking clearly. As far as I recall, "optional": True indicates that serialized ASN.1 data may not include this field, it may be omitted from the byte stream. Otherwise the parser will expect it to be there. So when serializing a field that it set to "optional": False, I believe asn1crypto will write an empty value of the appropriate type. When parsing, it allows the field to be missing in the byte stream. As far as I recall, it isn't used to validate that a field has been set.

[wbond]

To further clarify:

As far as I recall, it isn't used to validate that a field has been set.

Should read: "As far as I recall, it isn't used to validate that a field has been set when dumping."