[zipus]

Closes #281

[kislyuk]

Thanks, this looks great!

FYI: SHA1 is deprecated for digital signatures due to cryptographic weaknesses. I'm totally on board with this change, but I will eventually make this code use a different digest algorithm by default; using SHA1 will require passing a custom setting. I will let you know here when I get around to that. For now, this is good.

[kislyuk]

Hmm it looks like there are some CI failures, I'll have to take a look.

[zipus]

Hmm it looks like there are some CI failures, I'll have to take a look.

Ok, let me know if u need anything from my side.

[zipus]

Hmm it looks like there are some CI failures, I'll have to take a look.

While developing on my end other modules depending on signxml I found tests failing due to some certs not having COUNTRY_NAME, ORGANIZATION_NAME, ORGANIZATIONAL_UNIT_NAME, COMMON_NAME. Might be that?

[kislyuk]

The changes look correct, I'll adjust the tests in master. Merging

[kislyuk]

Hi @zipus, I will most likely have to revert this change. The XAdES specification states that this element must not be present:

You are welcome to maintain this functionality in a fork or subclass, of course.

[zipus]

Hi @kislyuk, thanks for your time and effort. I will report the spec statement to gov.

[zipus]

The XAdES specification states that this element must not be present

The European rules related to digital signatures (COMMISSION IMPLEMENTING DECISION (EU) 2015/1506) states that the technical rule to follow is ETSI TS 103 171.

In that spec, it indicates that SigningCertificate is required:

This may change in the future with spec eIDAS 2.0, but actually this is what EU states for.

[kislyuk]

Thank you for researching this @zipus, I will keep the change but make it conditional on a setting that enables the profile described in the spec you linked. Hoping to get to this over the winter break starting next week.

[kislyuk]

FYI I have disabled the legacy signing certificate generation, to enable it you have to subclass XAdESSigner and set the class variable use_deprecated_legacy_signing_certificate to True.

This has now been released as v4.2.1

[kislyuk]

@zipus the linter is complaining about the following:

signxml/xades/xades.py:224: error: If x = b'abc' then f"{x}" or "{}".format(x) produces "b'abc'", not "abc". If this is desired behavior, use f"{x!r}" or "{!r}".format(x). Otherwise, decode the bytes  [str-bytes-safe]

This is for

issuer_name.text = "C={C},O={O},OU={OU},CN={CN}".format(
                    C=loaded_cert.issuer.get_attributes_for_oid(x509.oid.NameOID.COUNTRY_NAME)[0].value,
                    O=loaded_cert.issuer.get_attributes_for_oid(x509.oid.NameOID.ORGANIZATION_NAME)[0].value,
                    OU=loaded_cert.issuer.get_attributes_for_oid(x509.oid.NameOID.ORGANIZATIONAL_UNIT_NAME)[0].value,
                    CN=loaded_cert.issuer.get_attributes_for_oid(x509.oid.NameOID.COMMON_NAME)[0].value,
                )

The fields in question are typed str | bytes. Are you seeing correctly formatted issuer name?

[kislyuk]

Ah I see: https://cryptography.io/en/latest/x509/reference/#cryptography.x509.NameAttribute.value

This will generally be a str, the only times it can be a bytes is when oid is X500_UNIQUE_IDENTIFIER.

So we're fine