feat(admissions): implement parsing of admissions extension #11903

hoefling · 2024-11-06T00:45:26Z

This is the sixth PR for #11875 which adds parsing of the Admissions extension object. As with #11892, I am least confident in the parsing part in certificate.rs.

Please also advise on the names and descriptions of the certificates added to test vectors. Is there maybe a file naming policy I should stick to?

The certificates and their origins are also described in the #11875 comment.

hoefling · 2024-11-06T01:32:26Z

Please also advise on this failure - could it be the introduced changes from this PR that caused it? Looks like a random failure thoughm, I also can't reproduce it locally in an ubuntu container. Log for future reference:

____________________ TestRSA.test_generate_rsa_keys[3-2048] ____________________
[gw1] linux -- Python 3.10.12 /__w/cryptography/cryptography/.nox/tests/bin/python3

self = <tests.hazmat.primitives.test_rsa.TestRSA object at 0x7f6f39949d80>
backend = <OpenSSLBackend(version: OpenSSL 3.0.2 15 Mar 2022, FIPS: False, Legacy: True)>
public_exponent = 3, key_size = 2048

    @pytest.mark.parametrize(
        ("public_exponent", "key_size"),
        itertools.product(
            (3, 65537),
            (1024, 1536, 2048),
        ),
    )
    def test_generate_rsa_keys(self, backend, public_exponent, key_size):
        if backend._fips_enabled:
            if key_size < backend._fips_rsa_min_key_size:
                pytest.skip(f"Key size not FIPS compliant: {key_size}")
            if public_exponent < backend._fips_rsa_min_public_exponent:
                pytest.skip(f"Exponent not FIPS compliant: {public_exponent}")
>       skey = rsa.generate_private_key(public_exponent, key_size, backend)

tests/hazmat/primitives/test_rsa.py:184: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

public_exponent = 3, key_size = 2048
backend = <OpenSSLBackend(version: OpenSSL 3.0.2 15 Mar 2022, FIPS: False, Legacy: True)>

    def generate_private_key(
        public_exponent: int,
        key_size: int,
        backend: typing.Any = None,
    ) -> RSAPrivateKey:
        _verify_rsa_parameters(public_exponent, key_size)
>       return rust_openssl.rsa.generate_private_key(public_exponent, key_size)
E       cryptography.exceptions.InternalError: Unknown OpenSSL error. This error is commonly encountered
E                           when another library is not cleaning up the OpenSSL error
E                           stack. If you are using cryptography with another library
E                           that uses OpenSSL try disabling it before reporting a bug.
E                           Otherwise please file an issue at
E                           https://github.com/pyca/cryptography/issues with
E                           information on how to reproduce this. (OpenSSL error)

.nox/tests/lib/python3.10/site-packages/cryptography/hazmat/primitives/asymmetric/rsa.py: InternalError

alex · 2024-11-06T01:34:09Z

That failure is a flake, I'll re-run that job.

…

On Tue, Nov 5, 2024 at 8:32 PM Oleg Höfling ***@***.***> wrote: Please also advise on this failure <https://github.com/pyca/cryptography/actions/runs/11695433392/job/32570858861?pr=11903> - could it be the introduced changes from this PR that caused it? Looks like a random failure thoughm, I also can't reproduce it locally in an ubuntu container. Log for future reference: ____________________ TestRSA.test_generate_rsa_keys[3-2048] ____________________ [gw1] linux -- Python 3.10.12 /__w/cryptography/cryptography/.nox/tests/bin/python3 self = <tests.hazmat.primitives.test_rsa.TestRSA object at 0x7f6f39949d80> backend = <OpenSSLBackend(version: OpenSSL 3.0.2 15 Mar 2022, FIPS: False, Legacy: True)> public_exponent = 3, key_size = 2048 @pytest.mark.parametrize( ("public_exponent", "key_size"), itertools.product( (3, 65537), (1024, 1536, 2048), ), ) def test_generate_rsa_keys(self, backend, public_exponent, key_size): if backend._fips_enabled: if key_size < backend._fips_rsa_min_key_size: pytest.skip(f"Key size not FIPS compliant: {key_size}") if public_exponent < backend._fips_rsa_min_public_exponent: pytest.skip(f"Exponent not FIPS compliant: {public_exponent}") > skey = rsa.generate_private_key(public_exponent, key_size, backend) tests/hazmat/primitives/test_rsa.py:184: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ public_exponent = 3, key_size = 2048 backend = <OpenSSLBackend(version: OpenSSL 3.0.2 15 Mar 2022, FIPS: False, Legacy: True)> def generate_private_key( public_exponent: int, key_size: int, backend: typing.Any = None, ) -> RSAPrivateKey: _verify_rsa_parameters(public_exponent, key_size) > return rust_openssl.rsa.generate_private_key(public_exponent, key_size) E cryptography.exceptions.InternalError: Unknown OpenSSL error. This error is commonly encountered E when another library is not cleaning up the OpenSSL error E stack. If you are using cryptography with another library E that uses OpenSSL try disabling it before reporting a bug. E Otherwise please file an issue at E https://github.com/pyca/cryptography/issues with E information on how to reproduce this. (OpenSSL error) .nox/tests/lib/python3.10/site-packages/cryptography/hazmat/primitives/asymmetric/rsa.py: InternalError — Reply to this email directly, view it on GitHub <#11903 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAAGBCBGY25N3CJ3DDY5XLZ7FWUDAVCNFSM6AAAAABRHW2CJWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINJYGUZDGOBVGM> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

-- All that is necessary for evil to succeed is for good people to do nothing.

hoefling · 2024-11-07T00:05:33Z

@alex both #11892 and this one should be ready now, I cherry-picked the necessary changes from the bugfix for profession OIDs, the added diff is 526e083 for #11892 and 56bec7f for this one.

alex · 2024-11-07T16:43:59Z

docs/development/test-vectors.rst

@@ -546,6 +546,15 @@ Custom X.509 Vectors
  This is an invalid certificate per :rfc:`5280` 4.2.1.12.
 * ``malformed-san.pem`` - A certificate with a malformed SAN.
 * ``malformed-ian.pem`` - A certificate with a malformed IAN.
+* ``admissions_extension_cert_e256.pem`` - A certificate with the ``Admissions``


Can you do a separate PR with these? We like to review them separately from the implementation.

@alex done! But unfortunately I had to extend the test and add an additional certificate in order to cover all corner cases - I hope this is fine.

alex · 2024-11-07T16:45:43Z

src/rust/src/x509/certificate.rs

+        Some(data) => oid_to_py_oid(py, data)?.to_object(py),
+        None => py.None(),


Instead of doing to_object() on the some path, I'd recommend doing a bind(py) on the None path. It's a moderately more flexible pattern.

Same for the py_url path.

alex · 2024-11-07T16:46:39Z

src/rust/src/x509/certificate.rs

+                py_registration_number,
+                py_add_profession_info,
+            ))?
+            .to_object(py);


to_object shouldn't be needed here.

alex · 2024-11-07T16:47:09Z

src/rust/src/x509/certificate.rs

+fn parse_profession_infos<'a>(
+    py: pyo3::Python<'_>,
+    profession_infos: &asn1::SequenceOf<'a, ProfessionInfo<'a>>,
+) -> CryptographyResult<pyo3::PyObject> {


This should return a Bound<'p, PyAny>, in general our habbit of returning PyObject all over hte place was a mistake we don't want to extend.

@alex done - I also went through other functions I added (parse_naming_authority and parse_admissions) and adjusted them as well; this also applies to your other suggestions. Please verify that I did what you indeed had in mind!

Signed-off-by: oleg.hoefling <[email protected]>

…overing the case of naming authority with no data Signed-off-by: oleg.hoefling <[email protected]>

Signed-off-by: oleg.hoefling <[email protected]>

…to Bound<PyAny> Signed-off-by: Oleg Hoefling <[email protected]>

…nd<PyAny> Signed-off-by: Oleg Hoefling <[email protected]>

Signed-off-by: Oleg Hoefling <[email protected]>

…o complete rust coverage Signed-off-by: Oleg Hoefling <[email protected]>

Signed-off-by: Oleg Hoefling <[email protected]>

alex · 2024-11-09T22:33:38Z

docs/development/test-vectors.rst

+* ``admissions_extension_cert_synthetic.pem`` - A certificate containing
+  the ``Admissions`` extension with synthetic data,
+  signed by ``x509/custom/ca/rsa_ca.pem`` CA.


"synthetic data" is a bit unclear, can you describe the structure of this cert, what makes it interesting/what is it for testing.

@alex I agree it wasn't the best naming, amended to have optional_data_not_provided suffix and a better description. Do you think it makes sense to also document the certificate generation, similar to this comment of mine or is it too much?

alex · 2024-11-09T22:34:49Z

src/rust/src/x509/certificate.rs

+) -> CryptographyResult<pyo3::Bound<'p, pyo3::PyAny>> {
+    let py_id = match &authority.id {
+        Some(data) => oid_to_py_oid(py, data)?,
+        None => py.None().bind(py).clone().into_any(),


Suggested change

None => py.None().bind(py).clone().into_any(),

None => py.None().into_bound(py),

@alex how did I miss this in pyo3's documentation 🙈

… added functions Signed-off-by: Oleg Hoefling <[email protected]>

Signed-off-by: Oleg Hoefling <[email protected]>

alex · 2024-11-11T01:05:03Z

docs/development/test-vectors.rst

+* ``admissions_extension_optional_data_not_provided.pem`` -
+  A certificate containing the ``Admissions`` extension with multiple admissions,
+  signed by ``x509/custom/ca/rsa_ca.pem`` CA. The admissions in this certificate
+  are prepared using synthetic data to verify the possible corner cases are handled
+  by the parser correctly (an admission missing naming authority or admission
+  authority, a profession info missing naming authority or profession OIDs
+  or the registration number etc).


This is fine (and won't block merging), but FYI for the future if you contribute again (which I hope you do!), our usual approach would be: "The admissions in this certificate have every field filled in with all the available options."

@alex I see - but I did also cover the case of all available fields fille out, too! The extension in the certificate contains five admissions in total, first one has all fields filled out, the remaining four test the corner cases. Either I still didn't a good job at describing the certificate then, will amend this. Or I didn't do the testing part properly - but wasn't sure how to test all the corner cases with missing fields, so I put every corner case in two certificates 🙈

hoefling force-pushed the feature/extension/admissions/parse branch from 0b9ef86 to 330b8cc Compare November 6, 2024 01:27

hoefling force-pushed the feature/extension/admissions/parse branch from 330b8cc to 7893e1e Compare November 6, 2024 20:20

hoefling mentioned this pull request Nov 6, 2024

fix(admissions): allow profession_oids field being none #11908

Merged

hoefling force-pushed the feature/extension/admissions/parse branch 2 times, most recently from 7e27924 to 0e01647 Compare November 7, 2024 00:02

hoefling force-pushed the feature/extension/admissions/parse branch from 0e01647 to ea70242 Compare November 7, 2024 09:09

alex reviewed Nov 7, 2024

View reviewed changes

hoefling added 13 commits November 9, 2024 14:14

feat: implement parsing of admissions extension

3ad9245

Signed-off-by: oleg.hoefling <[email protected]>

chore: add tests for admissions extension parsing

2991681

Signed-off-by: oleg.hoefling <[email protected]>

chore: use cryptography result return type

454b46c

Signed-off-by: oleg.hoefling <[email protected]>

chore: apply fixes done by cargo fmt and clippy

78da2c8

Signed-off-by: oleg.hoefling <[email protected]>

add gematik company name and the gmbh abbreviations to known words

d8c5632

Signed-off-by: oleg.hoefling <[email protected]>

fix: regenerate the synthetic certificate with additional admission c…

276f543

…overing the case of naming authority with no data Signed-off-by: oleg.hoefling <[email protected]>

fix: parse none for profession_oids if profession_oids is none

1a79a74

Signed-off-by: oleg.hoefling <[email protected]>

chore: apply formatting to changes in rust codebase

0d97a6c

Signed-off-by: oleg.hoefling <[email protected]>

refactor: switch return type of parse_profession_infos from PyObject …

3a61d50

…to Bound<PyAny> Signed-off-by: Oleg Hoefling <[email protected]>

refactor: switch return type of parse_naming_authority from PyObject …

2102458

…to Bound<PyAny> Signed-off-by: Oleg Hoefling <[email protected]>

refactor: switch return type of parse_admissions from PyObject to Bou…

39819dc

…nd<PyAny> Signed-off-by: Oleg Hoefling <[email protected]>

chore: remove gematik certs from repo

42b21e2

Signed-off-by: Oleg Hoefling <[email protected]>

chore: remove gematik certs from this pr

4ef7e87

Signed-off-by: Oleg Hoefling <[email protected]>

hoefling force-pushed the feature/extension/admissions/parse branch from ea70242 to 4ef7e87 Compare November 9, 2024 13:15

chore: extend parser tests with an additional synthetic certificate t…

20f74b9

…o complete rust coverage Signed-off-by: Oleg Hoefling <[email protected]>

hoefling force-pushed the feature/extension/admissions/parse branch from 1d8b0ee to 20f74b9 Compare November 9, 2024 13:34

chore: add description for the additional certificate without authority

7f39af5

Signed-off-by: Oleg Hoefling <[email protected]>

alex reviewed Nov 9, 2024

View reviewed changes

use into_bound(py) as shortcut, refrain from using to_object() in all…

1fbd230

… added functions Signed-off-by: Oleg Hoefling <[email protected]>

hoefling added 2 commits November 10, 2024 11:30

add better description for the admissions synthetic cert

20096f1

Signed-off-by: Oleg Hoefling <[email protected]>

adjust description to avoid using misspelled words

f4cdefd

Signed-off-by: Oleg Hoefling <[email protected]>

alex approved these changes Nov 11, 2024

View reviewed changes

alex merged commit fef1270 into pyca:main Nov 11, 2024
60 checks passed

hoefling deleted the feature/extension/admissions/parse branch November 11, 2024 07:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(admissions): implement parsing of admissions extension #11903

feat(admissions): implement parsing of admissions extension #11903

hoefling commented Nov 6, 2024

hoefling commented Nov 6, 2024

alex commented Nov 6, 2024 via email

hoefling commented Nov 7, 2024

alex Nov 7, 2024

hoefling Nov 9, 2024

alex Nov 7, 2024

alex Nov 7, 2024

alex Nov 7, 2024

hoefling Nov 9, 2024

alex Nov 9, 2024

hoefling Nov 10, 2024

alex Nov 9, 2024

hoefling Nov 10, 2024

alex Nov 11, 2024

hoefling Nov 12, 2024

		Some(data) => oid_to_py_oid(py, data)?.to_object(py),
		None => py.None(),

	None => py.None().bind(py).clone().into_any(),
	None => py.None().into_bound(py),

feat(admissions): implement parsing of admissions extension #11903

feat(admissions): implement parsing of admissions extension #11903

Conversation

hoefling commented Nov 6, 2024

hoefling commented Nov 6, 2024

alex commented Nov 6, 2024 via email

hoefling commented Nov 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment