Conversation
|
@Enet4 This ended up being quite a bit larger than I first thought, happy to change any of the direction. I personally like the multiple traits quite a bit as it definitely simplifies repeated code, but I totally understand if this is not a direction you want to go in, especially since its a breaking change. |
|
Also I built this on top of the other |
Enet4
left a comment
There was a problem hiding this comment.
This is wonderfully appreciated! I've already gone through most of the code, the addition to the API seems simple and clean (the tls_config option in the association builders). Please see the comments inline for the next steps!
(I also took the liberty of resolving the small conflict in .gitignore)
|
I have a decent amount to work on with this which will probably take me a week or two. In the meantime, do you think you could take a look at #677 as this branch is built on that MR |
|
There are still a few things to clean up (left open threads), but I did go ahead and implement the TLS options and test with |
|
At some point I would also like to refactor both storescu and storescp to have some reusable library functions to both allow for more "integration" testing like I mentioned on Zulip but I think this MR is already huge and I probably shouldn't add any more to it :) |
|
There were a lot of changes since the last reiteration. Let me know if you need assistance rebasing this PR. |
|
There are too many changes here to easily rebase, if you don't mind I might just wait until #708 is merged into |
6b9df45 to
af7a5bd
Compare
ENH: Tests, documentation and linting for TLS MAIN: fix doctests missing assets and feature gates MAIN: Make doctests happy Apply suggestions from code review Co-authored-by: Eduardo Pinho <enet4mikeenet@gmail.com>
MAIN: More review changes * Update MSRV in workflow to 1.75.0 * Change `tls` feature flag to `sync-tls` to clarify difference with `async-tls` * Install default cryptoprovider in tests MAIN: Fix Cargo.toml
af7a5bd to
dcd45a7
Compare
|
@Enet4 Okay i did end up rebasing as it was a good exercise to make sure all the changes were applied correctly. Should be good for a final review now! |
There was a problem hiding this comment.
Thanks for the rebase! Aside from the notes, I have been trying to run the tests locally with no success.
running 2 tests
test test_tls_connection_sync ... FAILED
test test_tls_connection_async ... FAILED
failures:
---- test_tls_connection_sync stdout ----
All certs exist, exiting
thread 'test_tls_connection_sync' panicked at ul/tests/association.rs:167:10:
Failed to establish TLS association: ReceivePdu { source: ReadPdu { source: Custom { kind: InvalidData, error: AlertReceived(DecryptError) }, backtrace: Backtrace [{ fn: "snafu::backtrace_impl::<impl snafu::GenerateImplicitData for std::backtrace::Backtrace>::generate", file: "$HOME/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/snafu-0.8.9/src/backtrace_impl_std.rs", line: 5 }, { fn: "snafu::GenerateImplicitData::generate_with_source", file: "$HOME/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/snafu-0.8.9/src/lib.rs", line: 1361 }, { fn: "<dicom_ul::pdu::ReadPduSnafu as snafu::IntoError<dicom_ul::pdu::ReadError>>::into_error", file: "~/ul/src/pdu/mod.rs", line: 71 }, { fn: "<core::result::Result<T,E> as snafu::ResultExt<T,E>>::context", file: "$HOME/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/snafu-0.8.9/src/lib.rs", line: 902 }, { fn: "dicom_ul::association::read_pdu_from_wire", file: "~/ul/src/association/mod.rs", line: 576 }, { fn: "dicom_ul::association::client::ClientAssociationOptions::establish_impl", file: "~/ul/src/association/client.rs", line: 835 }, { fn: "dicom_ul::association::client::ClientAssociationOptions::establish_tls", file: "~/ul/src/association/client.rs", line: 514 }, { fn: "association::test_tls_connection_sync", file: "~/ul/tests/association.rs", line: 166 }, { fn: "association::test_tls_connection_sync::{{closure}}", file: "~/ul/tests/association.rs", line: 117 }, { fn: "core::ops::function::FnOnce::call_once", file: "$HOME/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs", line: 253 }, { fn: "core::ops::function::FnOnce::call_once", file: "/rustc/1159e78c4747b02ef996e55082b704c09b970588/library/core/src/ops/function.rs", line: 253 }, { fn: "test::__rust_begin_short_backtrace", file: "/rustc/1159e78c4747b02ef996e55082b704c09b970588/library/test/src/lib.rs", line: 648 }, { fn: "test::run_test_in_process::{{closure}}", file: "/rustc/1159e78c4747b02ef996e55082b704c09b970588/library/test/src/lib.rs", line: 671 }, { fn: "<core::panic::unwind_safe::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once", file: "/rustc/1159e78c4747b02ef996e55082b704c09b970588/library/core/src/panic/unwind_safe.rs", line: 272 }, { fn: "std::panicking::catch_unwind::do_call", file: "/rustc/1159e78c4747b02ef996e55082b704c09b970588/library/std/src/panicking.rs", line: 589 }, { fn: "std::panicking::catch_unwind", file: "/rustc/1159e78c4747b02ef996e55082b704c09b970588/library/std/src/panicking.rs", line: 552 }, { fn: "std::panic::catch_unwind", file: "/rustc/1159e78c4747b02ef996e55082b704c09b970588/library/std/src/panic.rs", line: 359 }, { fn: "test::run_test_in_process", file: "/rustc/1159e78c4747b02ef996e55082b704c09b970588/library/test/src/lib.rs", line: 671 }, { fn: "test::run_test::{{closure}}", file: "/rustc/1159e78c4747b02ef996e55082b704c09b970588/library/test/src/lib.rs", line: 592 }, { fn: "test::run_test::{{closure}}", file: "/rustc/1159e78c4747b02ef996e55082b704c09b970588/library/test/src/lib.rs", line: 622 }, { fn: "std::sys::backtrace::__rust_begin_short_backtrace", file: "/rustc/1159e78c4747b02ef996e55082b704c09b970588/library/std/src/sys/backtrace.rs", line: 158 }, { fn: "std::thread::Builder::spawn_unchecked_::{{closure}}::{{closure}}", file: "/rustc/1159e78c4747b02ef996e55082b704c09b970588/library/std/src/thread/mod.rs", line: 559 }, { fn: "<core::panic::unwind_safe::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once", file: "/rustc/1159e78c4747b02ef996e55082b704c09b970588/library/core/src/panic/unwind_safe.rs", line: 272 }, { fn: "std::panicking::catch_unwind::do_call", file: "/rustc/1159e78c4747b02ef996e55082b704c09b970588/library/std/src/panicking.rs", line: 589 }, { fn: "std::panicking::catch_unwind", file: "/rustc/1159e78c4747b02ef996e55082b704c09b970588/library/std/src/panicking.rs", line: 552 }, { fn: "std::panic::catch_unwind", file: "/rustc/1159e78c4747b02ef996e55082b704c09b970588/library/std/src/panic.rs", line: 359 }, { fn: "std::thread::Builder::spawn_unchecked_::{{closure}}", file: "/rustc/1159e78c4747b02ef996e55082b704c09b970588/library/std/src/thread/mod.rs", line: 557 }, { fn: "core::ops::function::FnOnce::call_once{{vtable.shim}}", file: "/rustc/1159e78c4747b02ef996e55082b704c09b970588/library/core/src/ops/function.rs", line: 253 }, { fn: "<alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once", file: "/rustc/1159e78c4747b02ef996e55082b704c09b970588/library/alloc/src/boxed.rs", line: 1971 }, { fn: "std::sys::pal::unix::thread::Thread::new::thread_start", file: "/rustc/1159e78c4747b02ef996e55082b704c09b970588/library/std/src/sys/pal/unix/thread.rs", line: 107 }] } }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread '<unnamed>' panicked at ul/tests/association.rs:140:14:
Failed to establish TLS association: ReceivePdu { source: ReadPdu { source: Custom { kind: InvalidData, error: InvalidCertificate(BadSignature) }, backtrace: Backtrace [{ fn: "snafu::backtrace_impl::<impl snafu::GenerateImplicitData for std::backtrace::Backtrace>::generate", file: "$HOME/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/snafu-0.8.9/src/backtrace_impl_std.rs", line: 5 }, { fn: "snafu::GenerateImplicitData::generate_with_source", file: "$HOME/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/snafu-0.8.9/src/lib.rs", line: 1361 }, { fn: "<dicom_ul::pdu::ReadPduSnafu as snafu::IntoError<dicom_ul::pdu::ReadError>>::into_error", file: "~/ul/src/pdu/mod.rs", line: 71 }, { fn: "<core::result::Result<T,E> as snafu::ResultExt<T,E>>::context", file: "$HOME/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/snafu-0.8.9/src/lib.rs", line: 902 }, { fn: "dicom_ul::association::read_pdu_from_wire", file: "~/ul/src/association/mod.rs", line: 576 }, { fn: "dicom_ul::association::server::ServerAssociationOptions<A>::establish_tls", file: "~/ul/src/association/server.rs", line: 719 }, { fn: "association::test_tls_connection_sync::{{closure}}", file: "~/ul/tests/association.rs", line: 139 }, { fn: "std::sys::backtrace::__rust_begin_short_backtrace", file: "$HOME/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys/backtrace.rs", line: 158 }, { fn: "std::thread::Builder::spawn_unchecked_::{{closure}}::{{closure}}", file: "$HOME/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/thread/mod.rs", line: 559 }, { fn: "<core::panic::unwind_safe::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once", file: "$HOME/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/panic/unwind_safe.rs", line: 272 }, { fn: "std::panicking::catch_unwind::do_call", file: "$HOME/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs", line: 589 }, { fn: "__rust_try" }, { fn: "std::panicking::catch_unwind", file: "$HOME/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs", line: 552 }, { fn: "std::panic::catch_unwind", file: "$HOME/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panic.rs", line: 359 }, { fn: "std::thread::Builder::spawn_unchecked_::{{closure}}", file: "$HOME/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/thread/mod.rs", line: 557 }, { fn: "core::ops::function::FnOnce::call_once{{vtable.shim}}", file: "$HOME/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs", line: 253 }, { fn: "<alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once", file: "/rustc/1159e78c4747b02ef996e55082b704c09b970588/library/alloc/src/boxed.rs", line: 1971 }, { fn: "std::sys::pal::unix::thread::Thread::new::thread_start", file: "/rustc/1159e78c4747b02ef996e55082b704c09b970588/library/std/src/sys/pal/unix/thread.rs", line: 107 }] } }
---- test_tls_connection_async stdout ----
All certs exist, exiting
Error: ReceivePdu { source: ReadPdu { source: Custom { kind: InvalidData, error: AlertReceived(DecryptError) }, backtrace: Backtrace [{ fn: "snafu::backtrace_impl::<impl snafu::GenerateImplicitData for std::backtrace::Backtrace>::generate", file: "$HOME/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/snafu-0.8.9/src/backtrace_impl_std.rs", line: 5 }, { fn: "snafu::GenerateImplicitData::generate_with_source", file: "$HOME/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/snafu-0.8.9/src/lib.rs", line: 1361 }, { fn: "<dicom_ul::pdu::ReadPduSnafu as snafu::IntoError<dicom_ul::pdu::ReadError>>::into_error", file: "~/ul/src/pdu/mod.rs", line: 71 }, { fn: "<core::result::Result<T,E> as snafu::ResultExt<T,E>>::context", file: "$HOME/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/snafu-0.8.9/src/lib.rs", line: 902 }, { fn: "dicom_ul::association::read_pdu_from_wire_async::{{closure}}", file: "~/ul/src/association/mod.rs", line: 616 }, { fn: "dicom_ul::association::client::ClientAssociationOptions::establish_impl_async::{{closure}}::{{closure}}", file: "~/ul/src/association/client.rs", line: 1152 }, { fn: "dicom_ul::association::timeout::{{closure}}", file: "~/ul/src/association/mod.rs", line: 529 }, { fn: "dicom_ul::association::client::ClientAssociationOptions::establish_impl_async::{{closure}}", file: "~/ul/src/association/client.rs", line: 1154 }, { fn: "dicom_ul::association::client::ClientAssociationOptions::establish_tls_async::{{closure}}", file: "~/ul/src/association/client.rs", line: 1218 }, { fn: "association::test_tls_connection_async::{{closure}}", file: "~/ul/tests/association.rs", line: 230 }, { fn: "<core::pin::Pin<P> as core::future::future::Future>::poll", file: "$HOME/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/future/future.rs", line: 133 }, { fn: "tokio::runtime::park::CachedParkThread::block_on::{{closure}}", file: "$HOME/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.45.1/src/runtime/park.rs", line: 284 }, { fn: "tokio::task::coop::with_budget", file: "$HOME/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.45.1/src/task/coop/mod.rs", line: 167 }, { fn: "tokio::task::coop::budget", file: "$HOME/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.45.1/src/task/coop/mod.rs", line: 133 }, { fn: "tokio::runtime::park::CachedParkThread::block_on", file: "$HOME/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.45.1/src/runtime/park.rs", line: 284 }, { fn: "tokio::runtime::context::blocking::BlockingRegionGuard::block_on", file: "$HOME/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.45.1/src/runtime/context/blocking.rs", line: 66 }, { fn: "tokio::runtime::scheduler::multi_thread::MultiThread::block_on::{{closure}}", file: "$HOME/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.45.1/src/runtime/scheduler/multi_thread/mod.rs", line: 87 }, { fn: "tokio::runtime::context::runtime::enter_runtime", file: "$HOME/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.45.1/src/runtime/context/runtime.rs", line: 65 }, { fn: "tokio::runtime::scheduler::multi_thread::MultiThread::block_on", file: "$HOME/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.45.1/src/runtime/scheduler/multi_thread/mod.rs", line: 86 }, { fn: "tokio::runtime::runtime::Runtime::block_on_inner", file: "$HOME/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.45.1/src/runtime/runtime.rs", line: 358 }, { fn: "tokio::runtime::runtime::Runtime::block_on", file: "$HOME/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.45.1/src/runtime/runtime.rs", line: 330 }, { fn: "association::test_tls_connection_async", file: "~/ul/tests/association.rs", line: 242 }, { fn: "association::test_tls_connection_async::{{closure}}", file: "~/ul/tests/association.rs", line: 184 }, { fn: "core::ops::function::FnOnce::call_once", file: "$HOME/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs", line: 253 }, { fn: "core::ops::function::FnOnce::call_once", file: "/rustc/1159e78c4747b02ef996e55082b704c09b970588/library/core/src/ops/function.rs", line: 253 }, { fn: "test::__rust_begin_short_backtrace", file: "/rustc/1159e78c4747b02ef996e55082b704c09b970588/library/test/src/lib.rs", line: 648 }, { fn: "test::run_test_in_process::{{closure}}", file: "/rustc/1159e78c4747b02ef996e55082b704c09b970588/library/test/src/lib.rs", line: 671 }, { fn: "<core::panic::unwind_safe::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once", file: "/rustc/1159e78c4747b02ef996e55082b704c09b970588/library/core/src/panic/unwind_safe.rs", line: 272 }, { fn: "std::panicking::catch_unwind::do_call", file: "/rustc/1159e78c4747b02ef996e55082b704c09b970588/library/std/src/panicking.rs", line: 589 }, { fn: "std::panicking::catch_unwind", file: "/rustc/1159e78c4747b02ef996e55082b704c09b970588/library/std/src/panicking.rs", line: 552 }, { fn: "std::panic::catch_unwind", file: "/rustc/1159e78c4747b02ef996e55082b704c09b970588/library/std/src/panic.rs", line: 359 }, { fn: "test::run_test_in_process", file: "/rustc/1159e78c4747b02ef996e55082b704c09b970588/library/test/src/lib.rs", line: 671 }, { fn: "test::run_test::{{closure}}", file: "/rustc/1159e78c4747b02ef996e55082b704c09b970588/library/test/src/lib.rs", line: 592 }, { fn: "test::run_test::{{closure}}", file: "/rustc/1159e78c4747b02ef996e55082b704c09b970588/library/test/src/lib.rs", line: 622 }, { fn: "std::sys::backtrace::__rust_begin_short_backtrace", file: "/rustc/1159e78c4747b02ef996e55082b704c09b970588/library/std/src/sys/backtrace.rs", line: 158 }, { fn: "std::thread::Builder::spawn_unchecked_::{{closure}}::{{closure}}", file: "/rustc/1159e78c4747b02ef996e55082b704c09b970588/library/std/src/thread/mod.rs", line: 559 }, { fn: "<core::panic::unwind_safe::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once", file: "/rustc/1159e78c4747b02ef996e55082b704c09b970588/library/core/src/panic/unwind_safe.rs", line: 272 }, { fn: "std::panicking::catch_unwind::do_call", file: "/rustc/1159e78c4747b02ef996e55082b704c09b970588/library/std/src/panicking.rs", line: 589 }, { fn: "std::panicking::catch_unwind", file: "/rustc/1159e78c4747b02ef996e55082b704c09b970588/library/std/src/panicking.rs", line: 552 }, { fn: "std::panic::catch_unwind", file: "/rustc/1159e78c4747b02ef996e55082b704c09b970588/library/std/src/panic.rs", line: 359 }, { fn: "std::thread::Builder::spawn_unchecked_::{{closure}}", file: "/rustc/1159e78c4747b02ef996e55082b704c09b970588/library/std/src/thread/mod.rs", line: 557 }, { fn: "core::ops::function::FnOnce::call_once{{vtable.shim}}", file: "/rustc/1159e78c4747b02ef996e55082b704c09b970588/library/core/src/ops/function.rs", line: 253 }, { fn: "<alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once", file: "/rustc/1159e78c4747b02ef996e55082b704c09b970588/library/alloc/src/boxed.rs", line: 1971 }, { fn: "std::sys::pal::unix::thread::Thread::new::thread_start", file: "/rustc/1159e78c4747b02ef996e55082b704c09b970588/library/std/src/sys/pal/unix/thread.rs", line: 107 }] } }
failures:
test_tls_connection_async
test_tls_connection_sync
test result: FAILED. 0 passed; 2 failed; 0 ignored; 0 measured; 6 filtered out; finished in 0.13s
error: test failed, to rerun pass `-p dicom-ul --test association`
3a37c28 to
8ccaf3e
Compare
8ccaf3e to
139f821
Compare
|
I'm not sure what the issue is with the testing, but I was able to reproduce once on my machine and just deleting the certs dir fixed it, I'm wondering if the old openssl generated certs didn't work for some reason |
399842d to
7efc253
Compare
Enet4
left a comment
There was a problem hiding this comment.
So yes, the mystery of the failing test has been solved! More recommendations inline, hoping that this will be the last iteration before it is brought upstream.
Co-authored-by: Eduardo Pinho <enet4mikeenet@gmail.com>
|
LMK if anything else is needed here. No rush on my side other than that I would prefer not to have to rebase to master again if there are more PRs merged :) |
Enet4
left a comment
There was a problem hiding this comment.
It takes a bit of fiddling with the certificate stores to get the TLS formula right, but it works! 🎉
I've just added some comments on a few more things to fix before merging.
* Split server-only TLS options into separate Acceptor struct * Add enum for missing PEM components * Rename TLSOptions -> TlsOptions * Make CryptoProvider non_exhaustive to allow future adding * Fix package names in logging directives
a56e800 to
fab9f43
Compare
Co-authored-by: Eduardo Pinho <enet4mikeenet@gmail.com>
peer_ae_title,max_pdu_length)tlsfeature flagtokio-rustlsbehindasync-tlsfeature flagAssociationtrait for common (non-io) featuresSyncAssociationandAsyncAssociationforstream-related features.
which are needed but should not be called by users
ServerAssociationandClientAssociationintotwo different structs (async vs sync) and implemented respective
traits
ServerAssociationOptionsandClientAssociationOptionsAssociationOptionsstructs to establisha TLS connection both sync and async.
Dropfor bothServerAssociationandAsyncServerAssociationabortwhereas the client methods usereleaseinstructions