Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rust: add PyCryptoOps, test #9355

Closed
wants to merge 3 commits into from

Conversation

woodruffw
Copy link
Contributor

Another breakout from #8873.

Signed-off-by: William Woodruff <[email protected]>
@woodruffw
Copy link
Contributor Author

I'm not 100% sure, but I'm pretty sure this test is failing because the code it tests is Python invoked through an import, and the Python side isn't actually pre-configured in the Rust tests:

nox > cargo clippy -- -D warnings
    Updating crates.io index
   Compiling cryptography-cffi v0.1.0 (/home/runner/work/cryptography/cryptography/src/rust/cryptography-cffi)
   Compiling cryptography-openssl v0.1.0 (/home/runner/work/cryptography/cryptography/src/rust/cryptography-openssl)
    Checking cryptography-x509 v0.1.0 (/home/runner/work/cryptography/cryptography/src/rust/cryptography-x509)
   Compiling cryptography-rust v0.1.0 (/home/runner/work/cryptography/cryptography/src/rust)
    Checking cryptography-x509-validation v0.1.0 (/home/runner/work/cryptography/cryptography/src/rust/cryptography-x509-validation)
    Finished dev [unoptimized + debuginfo] target(s) in 18.25s
nox > cargo test --no-default-features --all --no-run -q --message-format=json
nox > cargo test --no-default-features --all
    Finished test [unoptimized + debuginfo] target(s) in 0.04s
     Running unittests src/lib.rs (target/debug/deps/cryptography_cffi-c7e88f273d35133f)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running unittests src/lib.rs (target/debug/deps/cryptography_openssl-208fde574659ad[23](https://github.com/pyca/cryptography/actions/runs/5765081642/job/15630349204?pr=9355#step:14:24))

running 2 tests
test hmac::tests::test_digest_bytes ... ok
test tests::test_cvt ... ok

test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running unittests src/lib.rs (target/debug/deps/cryptography_rust-dead79b94956eaae)

running 11 tests
test error::tests::test_cryptographyerror_add_location ... ok
test pkcs7::tests::test_smime_canonicalize ... ok
test tests::test_constant_time_lt ... ok
test x[50](https://github.com/pyca/cryptography/actions/runs/5765081642/job/15630349204?pr=9355#step:14:51)9::sct::tests::test_hash_algorithm_to_attr ... ok
test x509::sct::tests::test_hash_algorithm_try_from ... ok
test x509::sct::tests::test_signature_algorithm_to_attr ... ok
test x509::sct::tests::test_signature_algorithm_try_from ... ok
test x509::sign::tests::test_identify_alg_params_for_hash_type ... ok
test x509::sign::tests::test_identify_key_type_for_algorithm_params ... ok
test error::tests::test_cryptographyerror_from ... ok
test x509::verify::tests::test_pycryptoops ... FAILED

failures:

---- x509::verify::tests::test_pycryptoops stdout ----
thread 'x509::verify::tests::test_pycryptoops' panicked at 'assertion failed: pk.is_ok()', src/x509/verify.rs:102:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

src/rust/src/x509/verify.rs Outdated Show resolved Hide resolved
...rather than smuggling the error type.

Signed-off-by: William Woodruff <[email protected]>
@woodruffw
Copy link
Contributor Author

but I'm pretty sure this test is failing because the code it tests is Python invoked through an import, and the Python side isn't actually pre-configured in the Rust tests:

Confirmed this: the key extraction fails on this:

                py.import(pyo3::intern!(
                    py,
                    "cryptography.hazmat.primitives.serialization"
                ))

@woodruffw
Copy link
Contributor Author

woodruffw commented Aug 4, 2023

Yeah, it's some kind of pathing issue: I'm running cargo test --no-default-features in a venv that contains the build cryptography package, but pyO3 has the following sys.path:

['/Users/william/.pyenv/versions/3.11.2/lib/python311.zip', '/Users/william/.pyenv/versions/3.11.2/lib/python3.11', '/Users/william/.pyenv/versions/3.11.2/lib/python3.11/lib-dynload', '/Users/william/.pyenv/versions/3.11.2/lib/python3.11/site-packages']

compared to python -c 'import sys; print(sys.path)':

['', '/Users/william/.pyenv/versions/3.11.2/lib/python311.zip', '/Users/william/.pyenv/versions/3.11.2/lib/python3.11', '/Users/william/.pyenv/versions/3.11.2/lib/python3.11/lib-dynload', '/Users/william/devel/cryptography/env/lib/python3.11/site-packages', '/Users/william/devel/cryptography/src']

Not sure why that is yet.

@woodruffw
Copy link
Contributor Author

This appears to be a manifestation of PyO3/pyo3#1741.

@woodruffw
Copy link
Contributor Author

Just to summarize the problems here:

  1. This changeset is trying to use cargo test to test some code that calls back into pure Python parts of cryptography. This doesn't work out of the box even with a venv loaded due to Can not link to virtual environment on OS X PyO3/pyo3#1741.
  2. (1) can be hacked around by explicitly setting PYTHONPATH, but this reveals deeper issues with trying to use cargo test on code that reaches into the Python parts of cryptography: doing so appears to double-load the various pyO3-defined classes, resulting in ObjectIdentifier, etc. classes that appear identical but have different type identities. This is probably unavoidable, by design: what we're effectively doing here is loading cryptography the way it was meant to be loaded, and then also loading our (non-extension) build of the Rust bits without any regard for their overlap.

Given these two, a breakout here is probably more trouble than its worth. I'll push up the final state here, then close and fold the relevant parts back into #8873.

Signed-off-by: William Woodruff <[email protected]>
@woodruffw woodruffw closed this Aug 4, 2023
@woodruffw woodruffw deleted the tob-verify-breakout branch August 4, 2023 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants