From 8b7f3d3ae6ee3ebf9c9248765d9b36c2cd114139 Mon Sep 17 00:00:00 2001 From: Aleksander <170264518+t-aleksander@users.noreply.github.com> Date: Wed, 3 Jun 2026 12:07:06 +0200 Subject: [PATCH 1/4] reduce panics --- examples/add_sync.rs | 4 +- examples/bind_sync.rs | 2 +- examples/bind_sync_tcp_stream.rs | 2 +- examples/compare_sync.rs | 2 +- examples/connect_tls_ipaddr_sync.rs | 2 +- examples/moddn_sync.rs | 6 +- examples/modify_relax_sync.rs | 4 +- examples/search_abandon.rs | 2 +- examples/search_adapted.rs | 10 +- examples/search_adapted_paged_plus.rs | 4 +- examples/search_adapted_paged_plus_sync.rs | 4 +- examples/search_entrystream_sync.rs | 4 +- examples/search_starttls_noverify.rs | 4 +- examples/search_url_params_sync.rs | 2 +- examples/txn_sync.rs | 4 +- examples/whoami_external.rs | 2 +- examples/whoami_external_sync.rs | 2 +- src/adapters.rs | 32 +++- src/conn.rs | 96 ++++++---- src/controls_impl.rs | 90 +++++++-- src/controls_impl/assertion.rs | 50 ++++- src/controls_impl/content_sync.rs | 185 +++++++++++++------ src/controls_impl/matched_values.rs | 39 +++- src/controls_impl/paged_results.rs | 33 +++- src/controls_impl/read_entry.rs | 25 ++- src/exop_impl.rs | 36 +++- src/exop_impl/passmod.rs | 42 ++++- src/exop_impl/txn.rs | 87 +++++++-- src/exop_impl/whoami.rs | 29 ++- src/ldap.rs | 19 +- src/lib.rs | 4 +- src/protocol.rs | 50 +++-- src/result.rs | 202 ++++++++++++++++++--- src/search.rs | 189 ++++++++++++++----- 34 files changed, 973 insertions(+), 295 deletions(-) diff --git a/examples/add_sync.rs b/examples/add_sync.rs index 7d2ee93..ec7412f 100644 --- a/examples/add_sync.rs +++ b/examples/add_sync.rs @@ -20,6 +20,6 @@ fn main() -> Result<()> { ], )? .success()?; - println!("{:?}", res); - Ok(ldap.unbind()?) + println!("{res:?}"); + ldap.unbind() } diff --git a/examples/bind_sync.rs b/examples/bind_sync.rs index 6b211e6..d9fcd4c 100644 --- a/examples/bind_sync.rs +++ b/examples/bind_sync.rs @@ -9,5 +9,5 @@ fn main() -> Result<()> { let _res = ldap .simple_bind("cn=Manager,dc=example,dc=org", "secret")? .success()?; - Ok(ldap.unbind()?) + ldap.unbind() } diff --git a/examples/bind_sync_tcp_stream.rs b/examples/bind_sync_tcp_stream.rs index 4aa4e33..7534f6f 100644 --- a/examples/bind_sync_tcp_stream.rs +++ b/examples/bind_sync_tcp_stream.rs @@ -13,5 +13,5 @@ fn main() -> Result<()> { let _res = ldap .simple_bind("cn=Manager,dc=example,dc=org", "secret")? .success()?; - Ok(ldap.unbind()?) + ldap.unbind() } diff --git a/examples/compare_sync.rs b/examples/compare_sync.rs index 3a07141..86941d5 100644 --- a/examples/compare_sync.rs +++ b/examples/compare_sync.rs @@ -15,5 +15,5 @@ fn main() -> Result<()> { )? .equal()?; println!("{}equal", if eq { "" } else { "not " }); - Ok(ldap.unbind()?) + ldap.unbind() } diff --git a/examples/connect_tls_ipaddr_sync.rs b/examples/connect_tls_ipaddr_sync.rs index 8b5b7cb..c493860 100644 --- a/examples/connect_tls_ipaddr_sync.rs +++ b/examples/connect_tls_ipaddr_sync.rs @@ -8,5 +8,5 @@ fn main() -> Result<()> { LdapConnSettings::new().set_no_tls_verify(true), "ldaps://127.0.0.1:2636", )?; - Ok(ldap.unbind()?) + ldap.unbind() } diff --git a/examples/moddn_sync.rs b/examples/moddn_sync.rs index 550b9d8..5deb456 100644 --- a/examples/moddn_sync.rs +++ b/examples/moddn_sync.rs @@ -26,8 +26,8 @@ fn main() -> Result<()> { "next" => (NEXT_RDN, TEST_RDN), _ => panic!("unexpected uid"), }; - let dn = format!("{},ou=People,dc=example,dc=org", cur_rdn); + let dn = format!("{cur_rdn},ou=People,dc=example,dc=org"); let res = ldap.modifydn(&dn, new_rdn, true, None)?.success()?; - println!("{:?}", res); - Ok(ldap.unbind()?) + println!("{res:?}"); + ldap.unbind() } diff --git a/examples/modify_relax_sync.rs b/examples/modify_relax_sync.rs index 4cc2723..37d8f2e 100644 --- a/examples/modify_relax_sync.rs +++ b/examples/modify_relax_sync.rs @@ -43,6 +43,6 @@ fn main() -> Result<()> { .with_controls(RelaxRules.critical()) .modify("uid=inejge,ou=People,dc=example,dc=org", mod_vec)? .success()?; - println!("{:?}", res); - Ok(ldap.unbind()?) + println!("{res:?}"); + ldap.unbind() } diff --git a/examples/search_abandon.rs b/examples/search_abandon.rs index 1737230..b901c89 100644 --- a/examples/search_abandon.rs +++ b/examples/search_abandon.rs @@ -29,5 +29,5 @@ async fn main() -> Result<()> { let _res = stream.finish().await; let msgid = stream.ldap_handle().last_id(); ldap.abandon(msgid).await?; - Ok(ldap.unbind().await?) + ldap.unbind().await } diff --git a/examples/search_adapted.rs b/examples/search_adapted.rs index 17d6fa3..d69852f 100644 --- a/examples/search_adapted.rs +++ b/examples/search_adapted.rs @@ -22,10 +22,10 @@ async fn main() -> Result<()> { .await?; while let Some(entry) = search.next().await? { let entry = SearchEntry::construct(entry); - println!("{:?}", entry); + println!("{entry:?}"); } let res = search.finish().await.success()?; - println!("{:?}", res); + println!("{res:?}"); println!("--- all objects"); let mut search = ldap .streaming_search( @@ -40,10 +40,10 @@ async fn main() -> Result<()> { println!("refs: {:?}", parse_refs(entry.0)); } else { let entry = SearchEntry::construct(entry); - println!("{:?}", entry); + println!("{entry:?}"); } } let res = search.finish().await.success()?; - println!("{:?}", res); - Ok(ldap.unbind().await?) + println!("{res:?}"); + ldap.unbind().await } diff --git a/examples/search_adapted_paged_plus.rs b/examples/search_adapted_paged_plus.rs index 5223576..f497001 100644 --- a/examples/search_adapted_paged_plus.rs +++ b/examples/search_adapted_paged_plus.rs @@ -26,8 +26,8 @@ async fn main() -> Result<()> { .await?; while let Some(entry) = search.next().await? { let entry = SearchEntry::construct(entry); - println!("{:?}", entry); + println!("{entry:?}"); } let _res = search.finish().await.success()?; - Ok(ldap.unbind().await?) + ldap.unbind().await } diff --git a/examples/search_adapted_paged_plus_sync.rs b/examples/search_adapted_paged_plus_sync.rs index 9305361..9418625 100644 --- a/examples/search_adapted_paged_plus_sync.rs +++ b/examples/search_adapted_paged_plus_sync.rs @@ -23,8 +23,8 @@ fn main() -> Result<()> { )?; while let Some(entry) = search.next()? { let entry = SearchEntry::construct(entry); - println!("{:?}", entry); + println!("{entry:?}"); } let _res = search.result().success()?; - Ok(ldap.unbind()?) + ldap.unbind() } diff --git a/examples/search_entrystream_sync.rs b/examples/search_entrystream_sync.rs index 3c41ba2..0176968 100644 --- a/examples/search_entrystream_sync.rs +++ b/examples/search_entrystream_sync.rs @@ -20,7 +20,7 @@ fn main() -> Result<()> { )?; while let Some(entry) = search.next()? { let entry = SearchEntry::construct(entry); - println!("{:?}", entry); + println!("{entry:?}"); } // The following two statements show how one would // Abandon a Search. The statements are commented out @@ -33,5 +33,5 @@ fn main() -> Result<()> { //let msgid = search.last_id(); //ldap.abandon(msgid)?; let _res = search.result().success()?; - Ok(ldap.unbind()?) + ldap.unbind() } diff --git a/examples/search_starttls_noverify.rs b/examples/search_starttls_noverify.rs index 9e96137..896946c 100644 --- a/examples/search_starttls_noverify.rs +++ b/examples/search_starttls_noverify.rs @@ -28,8 +28,8 @@ async fn main() -> Result<()> { .await?; while let Some(entry) = search.next().await? { let entry = SearchEntry::construct(entry); - println!("{:?}", entry); + println!("{entry:?}"); } let _res = search.finish().await.success()?; - Ok(ldap.unbind().await?) + ldap.unbind().await } diff --git a/examples/search_url_params_sync.rs b/examples/search_url_params_sync.rs index 0a837ae..04a6e9d 100644 --- a/examples/search_url_params_sync.rs +++ b/examples/search_url_params_sync.rs @@ -22,5 +22,5 @@ fn main() -> Result<()> { for entry in rs { println!("{:?}", SearchEntry::construct(entry)); } - Ok(ldap.unbind()?) + ldap.unbind() } diff --git a/examples/txn_sync.rs b/examples/txn_sync.rs index 1771faa..50e3eb1 100644 --- a/examples/txn_sync.rs +++ b/examples/txn_sync.rs @@ -42,7 +42,7 @@ fn main() -> Result<()> { if expo.val.is_some() { let end_txn = expo.parse::(); - println!("{:?}", end_txn); + println!("{end_txn:?}"); } - Ok(ldap.unbind()?) + ldap.unbind() } diff --git a/examples/whoami_external.rs b/examples/whoami_external.rs index 5121711..39bff2b 100644 --- a/examples/whoami_external.rs +++ b/examples/whoami_external.rs @@ -19,5 +19,5 @@ async fn main() -> Result<()> { let (exop, _res) = ldap.extended(WhoAmI).await?.success()?; let whoami: WhoAmIResp = exop.parse(); println!("{}", whoami.authzid); - Ok(ldap.unbind().await?) + ldap.unbind().await } diff --git a/examples/whoami_external_sync.rs b/examples/whoami_external_sync.rs index 914156f..725ba37 100644 --- a/examples/whoami_external_sync.rs +++ b/examples/whoami_external_sync.rs @@ -17,5 +17,5 @@ fn main() -> Result<()> { let (exop, _res) = ldap.extended(WhoAmI)?.success()?; let whoami: WhoAmIResp = exop.parse(); println!("{}", whoami.authzid); - Ok(ldap.unbind()?) + ldap.unbind() } diff --git a/src/adapters.rs b/src/adapters.rs index 4f75c0d..a27e434 100644 --- a/src/adapters.rs +++ b/src/adapters.rs @@ -400,15 +400,29 @@ where for (cno, ctrl) in ctrls.iter().enumerate() { if let Control(Some(ControlType::PagedResults), ref raw) = *ctrl { pr_index = Some(cno); - let pr: controls::PagedResults = raw.parse(); + let pr = controls::PagedResults::try_parse( + raw.val.as_deref().ok_or_else(|| { + LdapError::DecodingError(String::from( + "paged results control without value", + )) + })?, + )?; if pr.cookie.is_empty() { break; } - let ldap_ref = self.ldap.as_ref().expect("ldap_ref"); + let ldap_ref = self.ldap.as_ref().ok_or_else(|| { + LdapError::AdapterInit(String::from( + "paged results adapter used before start", + )) + })?; let mut ldap = ldap_ref.clone(); ldap.timeout = ldap_ref.timeout; ldap.search_opts = ldap_ref.search_opts.clone(); - let mut controls = ldap_ref.controls.clone().expect("saved ctrls"); + let mut controls = ldap_ref.controls.clone().ok_or_else(|| { + LdapError::AdapterInit(String::from( + "paged results adapter missing saved controls", + )) + })?; controls.push( controls::PagedResults { size: self.page_size, @@ -417,13 +431,13 @@ where .into(), ); ldap.controls = Some(controls); + let attrs = self.attrs.as_ref().ok_or_else(|| { + LdapError::AdapterInit(String::from( + "paged results adapter missing saved attributes", + )) + })?; let new_stream = match ldap - .streaming_search( - &self.base, - self.scope, - &self.filter, - self.attrs.as_ref().unwrap(), - ) + .streaming_search(&self.base, self.scope, &self.filter, attrs) .await { Ok(strm) => strm, diff --git a/src/conn.rs b/src/conn.rs index e4ba5c9..4575a9f 100644 --- a/src/conn.rs +++ b/src/conn.rs @@ -17,7 +17,7 @@ use crate::RequestId; use crate::exop_impl::StartTLS; use crate::ldap::Ldap; use crate::protocol::{ItemSender, LdapCodec, LdapOp, MaybeControls, MiscSender, ResultSender}; -use crate::result::{LdapError, Result}; +use crate::result::{LdapError, LdapResult, Result}; use crate::search::SearchItem; use lber::structures::{Null, Tag}; @@ -494,16 +494,16 @@ impl LdapConnAsync { port = url_port; } let (_hostname, host_port) = match url.host_str() { - Some("") => ("localhost", format!("localhost:{}", port)), - Some(h) => (h, format!("{}:{}", h, port)), - _ => panic!("unexpected None from url.host_str()"), + Some("") => ("localhost", format!("localhost:{port}")), + Some(h) => (h, format!("{h}:{port}")), + None => return Err(LdapError::EmptyHost), }; let stream = match settings.std_stream { None => TcpStream::connect(host_port.as_str()).await?, Some(StdStream::Tcp(_)) => { - let stream = match settings.std_stream.take().expect("StdStream") { - StdStream::Tcp(stream) => stream, - _ => panic!("non-tcp stream in enum"), + let stream = match settings.std_stream.take() { + Some(StdStream::Tcp(stream)) => stream, + _ => return Err(LdapError::MismatchedStreamType), }; stream.set_nonblocking(true)?; TcpStream::from_std(stream)? @@ -534,7 +534,7 @@ impl LdapConnAsync { let tls_stream = if let ConnType::Tcp(stream) = parts.io { LdapConnAsync::create_tls_stream(settings, _hostname, stream).await? } else { - panic!("underlying stream not TCP"); + return Err(LdapError::MismatchedStreamType); }; #[cfg(any(feature = "gssapi", feature = "ntlm"))] { @@ -544,7 +544,7 @@ impl LdapConnAsync { conn.stream = parts.codec.framed(ConnType::Tls(tls_stream)); ldap.has_tls = true; } - _ => unimplemented!(), + s => return Err(LdapError::UnknownScheme(String::from(s))), } Ok((conn, ldap)) } @@ -557,7 +557,7 @@ impl LdapConnAsync { ) -> Result> { let connector = match settings.connector { Some(connector) => connector, - None => LdapConnAsync::create_connector(&settings), + None => LdapConnAsync::create_connector(&settings)?, }; TokioTlsConnector::from(connector) .connect(hostname, stream) @@ -612,12 +612,12 @@ impl LdapConnAsync { } #[cfg(feature = "tls-native")] - fn create_connector(settings: &LdapConnSettings) -> TlsConnector { + fn create_connector(settings: &LdapConnSettings) -> Result { let mut builder = TlsConnector::builder(); if settings.no_tls_verify { builder.danger_accept_invalid_certs(true); } - builder.build().expect("connector") + Ok(builder.build()?) } #[cfg(all(any(feature = "gssapi", feature = "ntlm"), feature = "tls-native"))] @@ -772,7 +772,7 @@ impl LdapConnAsync { self.searchmap.insert(id, search_tx.clone()); } if let Err(e) = self.stream.send((id, tag, controls)).await { - warn!("socket send error: {}", e); + warn!("socket send error: {e}"); return Err(LdapError::from(e)); } else { match op { @@ -789,15 +789,15 @@ impl LdapConnAsync { }, LdapOp::Unbind => { if let Err(e) = self.stream.get_mut().shutdown().await { - warn!("socket shutdown error: {}", e); + warn!("socket shutdown error: {e}"); } if let Err(e) = self.stream.close().await { - warn!("socket close error: {}", e); + warn!("socket close error: {e}"); } }, } if let Err(e) = tx.send((Tag::Null(Null { ..Default::default() }), vec![])) { - warn!("ldap null result send error: {:?}", e); + warn!("ldap null result send error: {e:?}"); } } } else { @@ -812,10 +812,10 @@ impl LdapConnAsync { match self.get_peer_certificate() { Ok(v) => { if let Err(e) = tx.send(v) { - warn!("Couldn't send peer certificate over channel: {:?}", e); + warn!("Couldn't send peer certificate over channel: {e:?}"); } }, - Err(e) => warn!("Couldn't get peer certificate: {}", e), + Err(e) => warn!("Couldn't get peer certificate: {e}"), } }, } @@ -827,38 +827,60 @@ impl LdapConnAsync { let (id, (tag, controls)) = match resp { None => break, Some(Err(e)) => { - warn!("socket receive error: {}", e); + warn!("socket receive error: {e}"); return Err(LdapError::from(e)); }, Some(Ok(resp)) => resp, }; if let Some(tx) = self.searchmap.get(&id) { - let protoop = if let Tag::StructureTag(protoop) = tag { - protoop - } else { - panic!("unmatched tag structure: {:?}", tag); - }; - let (item, mut remove) = match protoop.id { - 4 | 25 => (SearchItem::Entry(protoop), false), - 5 => (SearchItem::Done(Tag::StructureTag(protoop).into()), true), - 19 => (SearchItem::Referral(protoop), false), - _ => panic!("unrecognized op id: {}", protoop.id), + let item = match tag { + Tag::StructureTag(protoop) => match protoop.id { + 4 | 25 => Some((SearchItem::Entry(protoop), false)), + 5 => { + let res = LdapResult::try_from_tag(Tag::StructureTag(protoop)) + .unwrap_or_else(|e| { + // Synthesize a protocolError result so the search + // terminates with a non-zero rc instead of hanging on + // a Done message that is never delivered. + warn!("ldap search done parse error, op={id}: {e}"); + LdapResult { + rc: 2, // protocolError + matched: String::from(""), + text: format!("{e}"), + refs: vec![], + ctrls: vec![], + } + }); + Some((SearchItem::Done(res), true)) + } + 19 => Some((SearchItem::Referral(protoop), false)), + other => { + warn!("unrecognized op id {other}, op={id}, skipping message"); + None + } + }, + other => { + warn!("unmatched tag structure, op={id}, skipping message: {other:?}"); + None + } }; - if let Err(e) = tx.send((item, controls)) { - warn!("ldap search item send error, op={}: {:?}", id, e); - remove = true; - } - if remove { - self.searchmap.remove(&id); + if let Some((item, mut remove)) = item { + if let Err(e) = tx.send((item, controls)) { + warn!("ldap search item send error, op={id}: {e:?}"); + remove = true; + } + if remove { + self.searchmap.remove(&id); + } } } else if let Some(tx) = self.resultmap.remove(&id) { if let Err(e) = tx.send((tag, controls)) { - warn!("ldap result send error: {:?}", e); + warn!("ldap result send error: {e:?}"); } let mut msgmap = self.msgmap.lock().expect("msgmap mutex (stream rx)"); msgmap.1.remove(&id); } else { - warn!("unmatched id: {}", id); + warn!("unmatched id: {id}"); } }, }; diff --git a/src/controls_impl.rs b/src/controls_impl.rs index a32deb9..dac53fb 100644 --- a/src/controls_impl.rs +++ b/src/controls_impl.rs @@ -5,6 +5,8 @@ use lber::structure::{PL, StructureTag}; use lber::structures::{ASNTag, Boolean, OctetString, Sequence, Tag}; use lber::universal::Types; +use crate::result::{LdapError, Result}; + /// Recognized control types. /// /// The variants can't be exhaustively matched, since the list of @@ -26,8 +28,8 @@ mod assertion; pub use self::assertion::Assertion; mod content_sync; -pub use self::content_sync::parse_syncinfo; pub use self::content_sync::{EntryState, RefreshMode, SyncDone, SyncInfo, SyncRequest, SyncState}; +pub use self::content_sync::{parse_syncinfo, try_parse_syncinfo}; mod paged_results; pub use self::paged_results::PagedResults; @@ -201,19 +203,29 @@ pub fn build_tag(rc: RawControl) -> StructureTag { .into_structure() } -pub fn parse_controls(t: StructureTag) -> Vec { - let tags = t.expect_constructed().expect("result sequence").into_iter(); +pub fn parse_controls(t: StructureTag) -> Result> { + fn decode>(msg: S) -> LdapError { + LdapError::DecodingError(msg.into()) + } + + let tags = t + .expect_constructed() + .ok_or_else(|| decode("control result sequence"))? + .into_iter(); let mut ctrls = Vec::new(); for ctrl in tags { - let mut components = ctrl.expect_constructed().expect("components").into_iter(); + let mut components = ctrl + .expect_constructed() + .ok_or_else(|| decode("control components"))? + .into_iter(); let ctype = String::from_utf8( components .next() - .expect("element") + .ok_or_else(|| decode("missing control type element"))? .expect_primitive() - .expect("octet string"), + .ok_or_else(|| decode("control type octet string"))?, ) - .expect("control type"); + .map_err(|e| decode(format!("control type is not valid UTF-8: {e}")))?; let next = components.next(); let (crit, maybe_val) = match next { None => (false, None), @@ -221,18 +233,72 @@ pub fn parse_controls(t: StructureTag) -> Vec { StructureTag { id, ref payload, .. } if id == Types::Boolean as u64 => match *payload { - PL::P(ref v) => (v[0] != 0, components.next()), - PL::C(_) => panic!("decoding error"), + PL::P(ref v) => (v.first().is_some_and(|b| *b != 0), components.next()), + PL::C(_) => return Err(decode("control criticality not primitive")), }, StructureTag { id, .. } if id == Types::OctetString as u64 => { (false, Some(c.clone())) } - _ => panic!("decoding error"), + _ => return Err(decode("unexpected control component")), }, }; - let val = maybe_val.map(|v| v.expect_primitive().expect("octet string")); + let val = match maybe_val { + None => None, + Some(v) => Some( + v.expect_primitive() + .ok_or_else(|| decode("control value octet string"))?, + ), + }; let known_type = CONTROLS.get(&*ctype).copied(); ctrls.push(Control(known_type, RawControl { ctype, crit, val })); } - ctrls + Ok(ctrls) +} + +#[cfg(test)] +mod test { + use super::*; + + // A primitive top-level tag is not a control sequence; this used to panic + // in `expect("result sequence")`. + #[test] + fn parse_controls_non_sequence_is_error() { + let tag = Tag::OctetString(OctetString { + inner: b"not a sequence".to_vec(), + ..Default::default() + }) + .into_structure(); + assert!(matches!( + parse_controls(tag), + Err(LdapError::DecodingError(_)) + )); + } + + // A control whose entry is a bare primitive (not a sequence of components) + // used to panic in `expect("components")`. + #[test] + fn parse_controls_non_sequence_entry_is_error() { + let tag = Tag::Sequence(Sequence { + inner: vec![Tag::OctetString(OctetString { + inner: b"bogus".to_vec(), + ..Default::default() + })], + ..Default::default() + }) + .into_structure(); + assert!(matches!( + parse_controls(tag), + Err(LdapError::DecodingError(_)) + )); + } + + #[test] + fn parse_controls_empty_is_ok() { + let tag = Tag::Sequence(Sequence { + inner: vec![], + ..Default::default() + }) + .into_structure(); + assert!(matches!(parse_controls(tag), Ok(v) if v.is_empty())); + } } diff --git a/src/controls_impl/assertion.rs b/src/controls_impl/assertion.rs index 5328bc3..75ab5d2 100644 --- a/src/controls_impl/assertion.rs +++ b/src/controls_impl/assertion.rs @@ -2,6 +2,7 @@ use bytes::BytesMut; use super::{MakeCritical, RawControl}; use crate::filter::parse; +use crate::result::{LdapError, Result}; use lber::structures::ASNTag; use lber::write; @@ -16,24 +17,53 @@ pub struct Assertion { impl> Assertion { /// Create a new control instance with the specified filter. + /// + /// Panics if the filter string can't be parsed; see + /// [`try_new()`](Self::try_new) for a non-panicking variant. #[allow(clippy::new_ret_no_self)] pub fn new(filter: S) -> RawControl { - Assertion { filter }.into() + Assertion::try_new(filter).expect("filter") } -} - -impl MakeCritical for Assertion {} -impl> From> for RawControl { - fn from(assn: Assertion) -> RawControl { - let filter_ref = assn.filter.as_ref(); - let filter = parse(filter_ref).expect("filter").into_structure(); + /// Create a new control instance with the specified filter, returning a + /// filter parsing error on a malformed filter string. + pub fn try_new(filter: S) -> Result { + let filter_ref = filter.as_ref(); + let filter = parse(filter_ref) + .map_err(|_| LdapError::FilterParsing)? + .into_structure(); let mut buf = BytesMut::with_capacity(filter_ref.len()); // ballpark write::encode_into(&mut buf, filter).expect("encoded"); - RawControl { + Ok(RawControl { ctype: ASSERTION_OID.to_owned(), crit: false, val: Some(Vec::from(&buf[..])), - } + }) + } +} + +impl MakeCritical for Assertion {} + +impl> From> for RawControl { + fn from(assn: Assertion) -> RawControl { + Assertion::try_new(assn.filter).expect("filter") + } +} + +#[cfg(test)] +mod test { + use super::*; + + #[test] + fn try_new_invalid_filter_is_error() { + assert!(matches!( + Assertion::try_new("(this is not a filter"), + Err(LdapError::FilterParsing) + )); + } + + #[test] + fn try_new_valid_filter_is_ok() { + assert!(Assertion::try_new("(cn=foo)").is_ok()); } } diff --git a/src/controls_impl/content_sync.rs b/src/controls_impl/content_sync.rs index 2d7e897..f98beb9 100644 --- a/src/controls_impl/content_sync.rs +++ b/src/controls_impl/content_sync.rs @@ -2,6 +2,7 @@ use std::collections::HashSet; use crate::ResultEntry; use crate::controls::{ControlParser, MakeCritical, RawControl}; +use crate::result::{LdapError, Result}; use bytes::BytesMut; @@ -99,46 +100,62 @@ pub enum EntryState { Delete, } -impl ControlParser for SyncState { - fn parse(val: &[u8]) -> Self { +impl SyncState { + /// Parse a Sync State control value, returning a decoding error on a + /// malformed or unexpected value. + pub fn try_parse(val: &[u8]) -> Result { + fn decode>(msg: S) -> LdapError { + LdapError::DecodingError(msg.into()) + } + let mut tags = match parse_tag(val) { IResult::Ok((_, tag)) => tag, - _ => panic!("syncstate: failed to parse tag"), + _ => return Err(decode("syncstate: failed to parse tag")), } .expect_constructed() - .expect("syncstate: elements") + .ok_or_else(|| decode("syncstate: elements"))? .into_iter(); let state = match match parse_uint( tags.next() - .expect("syncstate: element 1") + .ok_or_else(|| decode("syncstate: missing state element"))? .match_class(TagClass::Universal) .and_then(|t| t.match_id(Types::Enumerated as u64)) .and_then(|t| t.expect_primitive()) - .expect("syncstate: state") + .ok_or_else(|| decode("syncstate: state"))? .as_slice(), ) { Ok((_, state)) => state, - _ => panic!("syncstate: failed to parse state"), + _ => return Err(decode("syncstate: failed to parse state")), } { 0 => EntryState::Present, 1 => EntryState::Add, 2 => EntryState::Modify, 3 => EntryState::Delete, - _ => panic!("syncstate: unknown state"), + _ => return Err(decode("syncstate: unknown state")), }; let entry_uuid = tags .next() - .expect("syncstate: element 2") + .ok_or_else(|| decode("syncstate: missing entryUUID element"))? .expect_primitive() - .expect("syncstate: entryUUID"); - let cookie = tags - .next() - .map(|tag| tag.expect_primitive().expect("syncstate: synCookie")); - SyncState { + .ok_or_else(|| decode("syncstate: entryUUID"))?; + let cookie = match tags.next() { + None => None, + Some(tag) => Some( + tag.expect_primitive() + .ok_or_else(|| decode("syncstate: synCookie"))?, + ), + }; + Ok(SyncState { state, entry_uuid, cookie, - } + }) + } +} + +impl ControlParser for SyncState { + fn parse(val: &[u8]) -> Self { + SyncState::try_parse(val).expect("syncstate") } } @@ -149,14 +166,20 @@ pub struct SyncDone { pub refresh_deletes: bool, } -impl ControlParser for SyncDone { - fn parse(val: &[u8]) -> Self { +impl SyncDone { + /// Parse a Sync Done control value, returning a decoding error on a + /// malformed or unexpected value. + pub fn try_parse(val: &[u8]) -> Result { + fn decode>(msg: S) -> LdapError { + LdapError::DecodingError(msg.into()) + } + let tags = match parse_tag(val) { Ok((_, tag)) => tag, - _ => panic!("syncdone: failed to parse tag"), + _ => return Err(decode("syncdone: failed to parse tag")), } .expect_constructed() - .expect("syncdone: elements") + .ok_or_else(|| decode("syncdone: elements"))? .into_iter(); let mut cookie = None; let mut refresh_deletes = false; @@ -165,22 +188,28 @@ impl ControlParser for SyncDone { StructureTag { id, payload, .. } if id == Types::OctetString as u64 => { cookie = Some(match payload { PL::P(ostr) => ostr, - PL::C(_) => panic!("syncdone: constructed octet string?"), + PL::C(_) => return Err(decode("syncdone: constructed octet string?")), }); } StructureTag { id, payload, .. } if id == Types::Boolean as u64 => { refresh_deletes = match payload { - PL::P(ostr) => ostr[0] != 0, - PL::C(_) => panic!("syncdone: constructed boolean?"), + PL::P(ostr) => ostr.first().is_some_and(|b| *b != 0), + PL::C(_) => return Err(decode("syncdone: constructed boolean?")), }; } - _ => panic!("syncdone: unrecognized component"), + _ => return Err(decode("syncdone: unrecognized component")), } } - SyncDone { + Ok(SyncDone { cookie, refresh_deletes, - } + }) + } +} + +impl ControlParser for SyncDone { + fn parse(val: &[u8]) -> Self { + SyncDone::try_parse(val).expect("syncdone") } } @@ -204,43 +233,63 @@ pub enum SyncInfo { } /// Parse the Sync Info value from the Search result entry. +/// +/// Panics on a malformed message; see [`try_parse_syncinfo`] for a fallible variant. pub fn parse_syncinfo(entry: ResultEntry) -> SyncInfo { + try_parse_syncinfo(entry).expect("syncinfo") +} + +/// Parse the Sync Info value from the Search result entry, returning a decoding +/// error on a malformed or unexpected message. +pub fn try_parse_syncinfo(entry: ResultEntry) -> Result { + fn decode>(msg: S) -> LdapError { + LdapError::DecodingError(msg.into()) + } + let mut tags = entry .0 .match_id(25) .and_then(|t| t.expect_constructed()) - .expect("intermediate seq") + .ok_or_else(|| decode("syncinfo: intermediate seq"))? .into_iter(); loop { match tags.next() { - None => panic!("syncinfo: out of tags"), + None => return Err(decode("syncinfo: out of tags")), Some(tag) if tag.id == 0 => { - let oid = String::from_utf8(tag.expect_primitive().expect("octet string")) - .expect("intermediate oid"); + let oid = String::from_utf8( + tag.expect_primitive() + .ok_or_else(|| decode("syncinfo: oid octet string"))?, + ) + .map_err(|e| decode(format!("syncinfo: oid is not valid UTF-8: {e}")))?; if oid != SYNC_INFO_OID { - panic!("syncinfo: oid mismatch"); + return Err(decode("syncinfo: oid mismatch")); } } Some(tag) if tag.id == 1 => { - let syncinfo_val = - match parse_tag(tag.expect_primitive().expect("octet string").as_ref()) { - Ok((_, tag)) => tag, - _ => panic!("syncinfo: error parsing value"), - }; + let syncinfo_val = match parse_tag( + tag.expect_primitive() + .ok_or_else(|| decode("syncinfo: value octet string"))? + .as_ref(), + ) { + Ok((_, tag)) => tag, + _ => return Err(decode("syncinfo: error parsing value")), + }; return match syncinfo_val { StructureTag { id, class, payload } if class == TagClass::Context && id < 4 => { match id { 0 => { let cookie = match payload { PL::P(payload) => payload, - PL::C(_) => panic!("syncinfo: [0] not primitive"), + PL::C(_) => return Err(decode("syncinfo: [0] not primitive")), }; - SyncInfo::NewCookie(cookie) + Ok(SyncInfo::NewCookie(cookie)) } 1..=3 => { let mut syncinfo_val = match payload { PL::C(payload) => payload, - PL::P(_) => panic!("syncinfo: [1,2,3] not a sequence"), + PL::P(_) => { + return Err(decode("syncinfo: [1,2,3] not a sequence")); + } } .into_iter(); let mut sync_cookie = None; @@ -265,8 +314,9 @@ pub fn parse_syncinfo(entry: ResultEntry) -> SyncInfo { { flag = comp .expect_primitive() - .expect("octet string")[0] - != 0; + .ok_or_else(|| decode("syncinfo: flag"))? + .first() + .is_some_and(|b| *b != 0); } StructureTag { id, class, .. } if class == TagClass::Universal @@ -275,42 +325,69 @@ pub fn parse_syncinfo(entry: ResultEntry) -> SyncInfo { { uuids = comp .expect_constructed() - .expect("uuid set") + .ok_or_else(|| decode("syncinfo: uuid set"))? .into_iter() .map(|u| { - u.expect_primitive().expect("octet string") + u.expect_primitive().ok_or_else(|| { + decode("syncinfo: uuid octet string") + }) }) - .collect(); + .collect::>()?; + } + _ => { + return Err(decode( + "syncinfo: unexpected component", + )); } - _ => panic!(), }, } pass += 1; } match id { - 1 => SyncInfo::RefreshDelete { + 1 => Ok(SyncInfo::RefreshDelete { cookie: sync_cookie, refresh_done: flag, - }, - 2 => SyncInfo::RefreshPresent { + }), + 2 => Ok(SyncInfo::RefreshPresent { cookie: sync_cookie, refresh_done: flag, - }, - 3 => SyncInfo::SyncIdSet { + }), + 3 => Ok(SyncInfo::SyncIdSet { cookie: sync_cookie, refresh_deletes: flag, sync_uuids: uuids, - }, - _ => panic!("syncinfo: got id > 3"), + }), + _ => Err(decode("syncinfo: got id > 3")), } } - _ => panic!("syncinfo: got id > 3"), + _ => Err(decode("syncinfo: got id > 3")), } } - _ => panic!("syncinfo: got id > 3"), + _ => Err(decode("syncinfo: got id > 3")), }; } - _ => panic!("syncinfo: unrecognized tag"), + _ => return Err(decode("syncinfo: unrecognized tag")), } } } + +#[cfg(test)] +mod test { + use super::*; + + #[test] + fn sync_state_garbage_is_error() { + assert!(matches!( + SyncState::try_parse(&[0xff, 0xff]), + Err(LdapError::DecodingError(_)) + )); + } + + #[test] + fn sync_done_garbage_is_error() { + assert!(matches!( + SyncDone::try_parse(&[0xff, 0xff]), + Err(LdapError::DecodingError(_)) + )); + } +} diff --git a/src/controls_impl/matched_values.rs b/src/controls_impl/matched_values.rs index 0ce6b4e..99de3c6 100644 --- a/src/controls_impl/matched_values.rs +++ b/src/controls_impl/matched_values.rs @@ -4,6 +4,7 @@ use lber::{structures::ASNTag, write}; use super::RawControl; use crate::filter::parse_matched_values; +use crate::result::{LdapError, Result}; pub const MATCHED_VALUES_OID: &str = "1.2.826.0.1.3344810.2.3"; @@ -20,24 +21,46 @@ pub struct MatchedValues { impl> MatchedValues { /// Create a new control instance with the specified filter. + /// + /// Panics if the filter string can't be parsed; see + /// [`try_new()`](Self::try_new) for a non-panicking variant. #[allow(clippy::new_ret_no_self)] pub fn new(filter: S) -> RawControl { - MatchedValues { filter }.into() + MatchedValues::try_new(filter).expect("filter") } -} -impl> From> for RawControl { - fn from(assn: MatchedValues) -> RawControl { - let filter_ref = assn.filter.as_ref(); + /// Create a new control instance with the specified filter, returning a + /// filter parsing error on a malformed filter string. + pub fn try_new(filter: S) -> Result { + let filter_ref = filter.as_ref(); let filter = parse_matched_values(filter_ref) - .expect("filter") + .map_err(|_| LdapError::FilterParsing)? .into_structure(); let mut buf = BytesMut::with_capacity(filter_ref.len()); // ballpark write::encode_into(&mut buf, filter).expect("encoded"); - RawControl { + Ok(RawControl { ctype: MATCHED_VALUES_OID.to_owned(), crit: false, val: Some(Vec::from(&buf[..])), - } + }) + } +} + +impl> From> for RawControl { + fn from(assn: MatchedValues) -> RawControl { + MatchedValues::try_new(assn.filter).expect("filter") + } +} + +#[cfg(test)] +mod test { + use super::*; + + #[test] + fn try_new_invalid_filter_is_error() { + assert!(matches!( + MatchedValues::try_new("(this is not a filter"), + Err(LdapError::FilterParsing) + )); } } diff --git a/src/controls_impl/paged_results.rs b/src/controls_impl/paged_results.rs index 38fe408..7c02f4d 100644 --- a/src/controls_impl/paged_results.rs +++ b/src/controls_impl/paged_results.rs @@ -1,4 +1,5 @@ use super::{ControlParser, MakeCritical, RawControl}; +use crate::result::{LdapError, Result}; use bytes::BytesMut; @@ -52,33 +53,45 @@ impl From for RawControl { } } -impl ControlParser for PagedResults { - fn parse(val: &[u8]) -> PagedResults { +impl PagedResults { + /// Parse a Paged Results control value, returning a decoding error on a + /// malformed or unexpected value. + pub fn try_parse(val: &[u8]) -> Result { + fn decode>(msg: S) -> LdapError { + LdapError::DecodingError(msg.into()) + } + let mut pr_comps = match parse_tag(val) { Ok((_, tag)) => tag, - _ => panic!("failed to parse paged results value components"), + _ => return Err(decode("failed to parse paged results value components")), } .expect_constructed() - .expect("paged results components") + .ok_or_else(|| decode("paged results components"))? .into_iter(); let size = match parse_uint( pr_comps .next() - .expect("element") + .ok_or_else(|| decode("missing paged results size element"))? .match_class(TagClass::Universal) .and_then(|t| t.match_id(Types::Integer as u64)) .and_then(|t| t.expect_primitive()) - .expect("paged results size") + .ok_or_else(|| decode("paged results size"))? .as_slice(), ) { Ok((_, size)) => size as i32, - _ => panic!("failed to parse size"), + _ => return Err(decode("failed to parse size")), }; let cookie = pr_comps .next() - .expect("element") + .ok_or_else(|| decode("missing paged results cookie element"))? .expect_primitive() - .expect("octet string"); - PagedResults { size, cookie } + .ok_or_else(|| decode("paged results cookie octet string"))?; + Ok(PagedResults { size, cookie }) + } +} + +impl ControlParser for PagedResults { + fn parse(val: &[u8]) -> PagedResults { + PagedResults::try_parse(val).expect("paged results") } } diff --git a/src/controls_impl/read_entry.rs b/src/controls_impl/read_entry.rs index 61e139f..95a2527 100644 --- a/src/controls_impl/read_entry.rs +++ b/src/controls_impl/read_entry.rs @@ -3,6 +3,7 @@ use std::collections::HashMap; use bytes::BytesMut; use super::{ControlParser, MakeCritical, RawControl}; +use crate::result::{LdapError, Result}; use crate::search::{ResultEntry, SearchEntry}; use lber::parse::parse_tag; use lber::structures::{ASNTag, OctetString, Sequence, Tag}; @@ -106,16 +107,28 @@ fn from_read_entry>(re: ReadEntry) -> RawControl { } } -impl ControlParser for ReadEntryResp { - fn parse(val: &[u8]) -> ReadEntryResp { +impl ReadEntryResp { + /// Parse a Pre-Read or Post-Read control value, returning a decoding error + /// on a malformed or unexpected value. + pub fn try_parse(val: &[u8]) -> Result { let tag = match parse_tag(val) { Ok((_, tag)) => tag, - _ => panic!("failed to parse pre-read attribute values"), + _ => { + return Err(LdapError::DecodingError(String::from( + "failed to parse pre-read attribute values", + ))); + } }; - let se = SearchEntry::construct(ResultEntry::new(tag)); - ReadEntryResp { + let se = SearchEntry::try_construct(ResultEntry::new(tag))?; + Ok(ReadEntryResp { attrs: se.attrs, bin_attrs: se.bin_attrs, - } + }) + } +} + +impl ControlParser for ReadEntryResp { + fn parse(val: &[u8]) -> ReadEntryResp { + ReadEntryResp::try_parse(val).expect("read entry response") } } diff --git a/src/exop_impl.rs b/src/exop_impl.rs index 17bad2b..6006cc8 100644 --- a/src/exop_impl.rs +++ b/src/exop_impl.rs @@ -1,6 +1,8 @@ use lber::common::TagClass; use lber::structures::{OctetString, Tag}; +use crate::result::{LdapError, Result}; + mod whoami; pub use self::whoami::{WhoAmI, WhoAmIResp}; @@ -43,12 +45,12 @@ pub trait ExopParser { fn parse(val: &[u8]) -> Self; } -pub fn construct_exop(exop: Exop) -> Vec { - assert!(exop.name.is_some()); +pub fn construct_exop(exop: Exop) -> Result> { + let name = exop.name.ok_or(LdapError::EmptyExopName)?; let mut seq = vec![Tag::OctetString(OctetString { id: 0, class: TagClass::Context, - inner: exop.name.unwrap().into_bytes(), + inner: name.into_bytes(), })]; if let Some(val) = exop.val { seq.push(Tag::OctetString(OctetString { @@ -57,5 +59,31 @@ pub fn construct_exop(exop: Exop) -> Vec { inner: val, })); } - seq + Ok(seq) +} + +#[cfg(test)] +mod test { + use super::*; + + #[test] + fn construct_exop_without_name_is_error() { + let exop = Exop { + name: None, + val: Some(vec![1, 2, 3]), + }; + assert!(matches!( + construct_exop(exop), + Err(LdapError::EmptyExopName) + )); + } + + #[test] + fn construct_exop_with_name_is_ok() { + let exop = Exop { + name: Some(String::from("1.2.3")), + val: None, + }; + assert!(construct_exop(exop).is_ok()); + } } diff --git a/src/exop_impl/passmod.rs b/src/exop_impl/passmod.rs index 156c7a1..baaa04b 100644 --- a/src/exop_impl/passmod.rs +++ b/src/exop_impl/passmod.rs @@ -1,4 +1,5 @@ use super::{Exop, ExopParser}; +use crate::result::{LdapError, Result}; use bytes::BytesMut; @@ -84,26 +85,51 @@ impl<'a> From> for Exop { } } -impl ExopParser for PasswordModifyResp { - fn parse(val: &[u8]) -> PasswordModifyResp { +impl PasswordModifyResp { + /// Parse a Password Modify response value, returning a decoding error on a + /// malformed or unexpected value. + pub fn try_parse(val: &[u8]) -> Result { + fn decode>(msg: S) -> LdapError { + LdapError::DecodingError(msg.into()) + } + let tags = match parse_tag(val) { Ok((_, tag)) => tag, - _ => panic!("failed to parse password modify return value"), + _ => return Err(decode("failed to parse password modify return value")), }; let mut tags = tags .expect_constructed() - .expect("password modify sequence") + .ok_or_else(|| decode("password modify sequence"))? .into_iter(); let gen_pass = tags .next() - .expect("element") + .ok_or_else(|| decode("missing generated password element"))? .match_class(TagClass::Context) .and_then(|t| t.match_id(0)) .and_then(|t| t.expect_primitive()) - .expect("generated password") + .ok_or_else(|| decode("generated password"))? .as_slice() .to_owned(); - let gen_pass = String::from_utf8(gen_pass).expect("generated password not UTF-8"); - PasswordModifyResp { gen_pass } + let gen_pass = String::from_utf8(gen_pass).map_err(|_| LdapError::DecodingUTF8)?; + Ok(PasswordModifyResp { gen_pass }) + } +} + +impl ExopParser for PasswordModifyResp { + fn parse(val: &[u8]) -> PasswordModifyResp { + PasswordModifyResp::try_parse(val).expect("password modify response") + } +} + +#[cfg(test)] +mod test { + use super::*; + + #[test] + fn password_modify_resp_garbage_is_error() { + assert!(matches!( + PasswordModifyResp::try_parse(&[0xff, 0xff]), + Err(LdapError::DecodingError(_)) + )); } } diff --git a/src/exop_impl/txn.rs b/src/exop_impl/txn.rs index 202f70f..a13fdf9 100644 --- a/src/exop_impl/txn.rs +++ b/src/exop_impl/txn.rs @@ -1,3 +1,5 @@ +use std::str; + use bytes::BytesMut; use lber::{ common::TagClass, @@ -7,11 +9,13 @@ use lber::{ universal::Types, write, }; -use std::str; - -use crate::{controls::Control, controls_impl::parse_controls}; use super::{Exop, ExopParser}; +use crate::{ + controls::Control, + controls_impl::parse_controls, + result::{LdapError, Result}, +}; pub const TXN_START_OID: &str = "1.3.6.1.1.21.1"; pub const TXN_END_OID: &str = "1.3.6.1.1.21.3"; @@ -43,11 +47,21 @@ impl From for Exop { } } +impl StartTxnResp { + /// Parse a Start Transaction response value, returning a decoding error on + /// a non-UTF-8 value. + pub fn try_parse(val: &[u8]) -> Result { + Ok(StartTxnResp { + txn_id: str::from_utf8(val) + .map_err(|_| LdapError::DecodingUTF8)? + .to_owned(), + }) + } +} + impl ExopParser for StartTxnResp { fn parse(val: &[u8]) -> StartTxnResp { - StartTxnResp { - txn_id: str::from_utf8(val).expect("txn_id").to_owned(), - } + StartTxnResp::try_parse(val).expect("start txn response") } } @@ -112,19 +126,24 @@ impl<'a> From> for Exop { } } -impl ExopParser for EndTxnResp { - fn parse(val: &[u8]) -> EndTxnResp { +impl EndTxnResp { + /// Parse an End Transaction response value, returning a decoding error. + pub fn try_parse(val: &[u8]) -> Result { + fn decode>(msg: S) -> LdapError { + LdapError::DecodingError(msg.into()) + } + let mut tags = match parse_tag(val) { Ok((_, tag)) => tag, - _ => panic!("endtxnresp: failed to parse tag"), + _ => return Err(decode("endtxnresp: failed to parse tag")), } .expect_constructed() - .expect("endtxnresp: elements") + .ok_or_else(|| decode("endtxnresp: elements"))? .into_iter(); let mut msg_id = None; let mut upds_ctrls = None; - while let Some(tag) = tags.next() { + for tag in tags.by_ref() { match tag { StructureTag { id, @@ -133,7 +152,7 @@ impl ExopParser for EndTxnResp { } if id == Types::Integer as u64 && class == TagClass::Universal => { msg_id = Some(match parse_uint(v.as_slice()) { Ok((_, size)) => size as i32, - _ => panic!("failed to parse msg_id"), + _ => return Err(decode("failed to parse msg_id")), }); } StructureTag { @@ -143,18 +162,21 @@ impl ExopParser for EndTxnResp { } if id == Types::Sequence as u64 && class == TagClass::Universal => { let mut ctrls = Vec::with_capacity(tags.len() / 2); while !tags.is_empty() { - let controls = parse_controls(tags.pop().expect("element")); + let controls = parse_controls( + tags.pop() + .ok_or_else(|| decode("endtxnresp: missing controls element"))?, + )?; let msg_id = match parse_uint( tags.pop() - .expect("element") + .ok_or_else(|| decode("endtxnresp: missing message id element"))? .match_class(TagClass::Universal) .and_then(|t| t.match_id(Types::Integer as u64)) .and_then(|t| t.expect_primitive()) - .expect("message id") + .ok_or_else(|| decode("endtxnresp: message id"))? .as_slice(), ) { Ok((_, id)) => id as i32, - _ => panic!("failed to parse msg_id"), + _ => return Err(decode("failed to parse msg_id")), }; ctrls.push((msg_id, controls)); } @@ -162,14 +184,41 @@ impl ExopParser for EndTxnResp { break; } - _ => panic!("failed to parse endtxnresp"), + _ => return Err(decode("failed to parse endtxnresp")), } } if tags.next().is_some() { - panic!("failed to parse endtxnresp"); + return Err(decode("failed to parse endtxnresp")); } - EndTxnResp { msg_id, upds_ctrls } + Ok(EndTxnResp { msg_id, upds_ctrls }) + } +} + +impl ExopParser for EndTxnResp { + fn parse(val: &[u8]) -> EndTxnResp { + EndTxnResp::try_parse(val).expect("end txn response") + } +} + +#[cfg(test)] +mod test { + use super::*; + + #[test] + fn start_txn_resp_non_utf8_is_error() { + assert!(matches!( + StartTxnResp::try_parse(&[0xff, 0xfe]), + Err(LdapError::DecodingUTF8) + )); + } + + #[test] + fn end_txn_resp_garbage_is_error() { + assert!(matches!( + EndTxnResp::try_parse(&[0xff, 0xff]), + Err(LdapError::DecodingError(_)) + )); } } diff --git a/src/exop_impl/whoami.rs b/src/exop_impl/whoami.rs index 6f96e4e..6ac4318 100644 --- a/src/exop_impl/whoami.rs +++ b/src/exop_impl/whoami.rs @@ -1,6 +1,7 @@ use std::str; use super::{Exop, ExopParser}; +use crate::result::{LdapError, Result}; pub const WHOAMI_OID: &str = "1.3.6.1.4.1.4203.1.11.3"; @@ -29,10 +30,32 @@ impl From for Exop { } } +impl WhoAmIResp { + /// Parse a Who Am I response value, returning a decoding error. + pub fn try_parse(val: &[u8]) -> Result { + Ok(WhoAmIResp { + authzid: str::from_utf8(val) + .map_err(|_| LdapError::DecodingUTF8)? + .to_owned(), + }) + } +} + impl ExopParser for WhoAmIResp { fn parse(val: &[u8]) -> WhoAmIResp { - WhoAmIResp { - authzid: str::from_utf8(val).expect("authzid").to_owned(), - } + WhoAmIResp::try_parse(val).expect("whoami response") + } +} + +#[cfg(test)] +mod test { + use super::*; + + #[test] + fn whoami_resp_non_utf8_is_error() { + assert!(matches!( + WhoAmIResp::try_parse(&[0xff, 0xfe]), + Err(LdapError::DecodingUTF8) + )); } } diff --git a/src/ldap.rs b/src/ldap.rs index 237e40d..b6ccf2e 100644 --- a/src/ldap.rs +++ b/src/ldap.rs @@ -392,6 +392,15 @@ impl Ldap { let mut buf = client_ctx .unwrap(&token) .map_err(|e| LdapError::GssapiOperationError(format!("{:#}", e)))?; + // The SASL security layer message is a fixed 4 octets: a one-byte layer + // bitmask followed by a three-byte maximum buffer size. A shorter message + // from a broken or hostile server would otherwise panic on indexing below. + if buf.len() != 4 { + return Err(LdapError::GssapiOperationError(format!( + "expected 4-octet SASL security layer message, got {}", + buf.len() + ))); + } let needed_layer = if self.has_tls { GSSAUTH_P_NONE } else { @@ -415,8 +424,10 @@ impl Ldap { if res.rc == 0 { if needed_layer == GSSAUTH_P_PRIVACY { buf[0] = 0; - let send_max_size = - u32::from_be_bytes((&buf[..]).try_into().expect("send max size")); + let size_bytes: [u8; 4] = (&buf[..]).try_into().map_err(|_| { + LdapError::GssapiOperationError(String::from("malformed send max size")) + })?; + let send_max_size = u32::from_be_bytes(size_bytes); if send_max_size == 0 { warn!("got zero send_max_size, will be treated as unlimited"); } @@ -477,7 +488,7 @@ impl Ldap { let mut ntlm = Ntlm::new(); let identity = AuthIdentity { - username: Username::parse(username).unwrap(), + username: Username::parse(username)?, password: password.to_string().into(), }; let mut acq_creds = ntlm @@ -813,7 +824,7 @@ impl Ldap { let req = Tag::Sequence(Sequence { id: 23, class: TagClass::Application, - inner: construct_exop(exop.into()), + inner: construct_exop(exop.into())?, }); self.op_call(LdapOp::Single, req) .await diff --git a/src/lib.rs b/src/lib.rs index 3b68b8c..84c4faf 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -204,7 +204,6 @@ pub mod controls { //! # Ok(()) //! # } pub use crate::controls_impl::TxnSpec; - pub use crate::controls_impl::parse_syncinfo; pub use crate::controls_impl::{ Assertion, ManageDsaIt, MatchedValues, PagedResults, ProxyAuth, RelaxRules, }; @@ -216,6 +215,7 @@ pub mod controls { EntryState, RefreshMode, SyncDone, SyncInfo, SyncRequest, SyncState, }; pub use crate::controls_impl::{PostRead, PostReadResp, PreRead, PreReadResp, ReadEntryResp}; + pub use crate::controls_impl::{parse_syncinfo, try_parse_syncinfo}; } mod controls_impl; mod exop_impl; @@ -250,10 +250,10 @@ pub use conn::{LdapConnAsync, LdapConnSettings, StdStream}; pub use filter::parse as parse_filter; pub use ldap::{Ldap, Mod}; pub use result::{LdapError, LdapResult, SearchResult}; -pub use search::parse_refs; pub use search::{ DerefAliases, ResultEntry, Scope, SearchEntry, SearchOptions, SearchStream, StreamState, }; +pub use search::{parse_refs, try_parse_refs}; #[cfg(feature = "sync")] pub use sync::{EntryStream, LdapConn}; pub use util::{LdapUrlExt, LdapUrlParams, dn_escape, get_url_params, ldap_escape, ldap_unescape}; diff --git a/src/protocol.rs b/src/protocol.rs index e67d173..d796b46 100644 --- a/src/protocol.rs +++ b/src/protocol.rs @@ -51,12 +51,12 @@ pub enum LdapOp { #[allow(clippy::type_complexity)] fn decode_inner(buf: &mut BytesMut) -> Result))>, io::Error> { - let decoding_error = io::Error::new(io::ErrorKind::Other, "decoding error"); + let decoding_error = || io::Error::other("decoding error"); let mut parser = lber::Parser::new(); let binding = parser.parse(buf); let (i, tag) = match binding { Err(e) if e.is_incomplete() => return Ok(None), - Err(_e) => return Err(decoding_error), + Err(_e) => return Err(decoding_error()), Ok((i, ref tag)) => (i, tag), }; buf.advance(buf.len() - i.len()); @@ -66,9 +66,9 @@ fn decode_inner(buf: &mut BytesMut) -> Result tags, - None => return Err(decoding_error), + None => return Err(decoding_error()), }; - let mut maybe_controls = tags.pop().expect("element"); + let mut maybe_controls = tags.pop().ok_or_else(decoding_error)?; let has_controls = match maybe_controls { StructureTag { id, @@ -76,7 +76,7 @@ fn decode_inner(buf: &mut BytesMut) -> Result match *payload { PL::C(_) => true, - PL::P(_) => return Err(decoding_error), + PL::P(_) => return Err(decoding_error()), }, StructureTag { id, class, .. } if class == TagClass::Context && id == 10 => { // Active Directory bug workaround @@ -86,31 +86,30 @@ fn decode_inner(buf: &mut BytesMut) -> Result false, }; let (protoop, controls) = if has_controls { - (tags.pop().expect("element"), Some(maybe_controls)) + (tags.pop().ok_or_else(decoding_error)?, Some(maybe_controls)) } else { (maybe_controls, None) }; let controls = match controls { - Some(controls) => parse_controls(controls), + Some(controls) => parse_controls(controls).map_err(|e| io::Error::other(e.to_string()))?, None => vec![], }; let msgid = match parse_uint( tags.pop() - .expect("element") - .match_class(TagClass::Universal) + .and_then(|t| t.match_class(TagClass::Universal)) .and_then(|t| t.match_id(Types::Integer as u64)) .and_then(|t| t.expect_primitive()) - .expect("message id") + .ok_or_else(decoding_error)? .as_slice(), ) { Ok((_, id)) => id as i32, - _ => return Err(decoding_error), + _ => return Err(decoding_error()), }; Ok(Some((msgid, (Tag::StructureTag(protoop), controls)))) } @@ -142,7 +141,11 @@ impl Decoder for LdapCodec { if buf.len() < U32_SIZE { return Err(io::Error::new(io::ErrorKind::Other, "invalid SASL buffer")); } - let sasl_len = u32::from_be_bytes(buf[0..U32_SIZE].try_into().unwrap()); + let sasl_len = u32::from_be_bytes( + buf[0..U32_SIZE] + .try_into() + .map_err(|_| io::Error::new(io::ErrorKind::Other, "invalid SASL buffer"))?, + ); if buf.len() - U32_SIZE < sasl_len as usize { return Ok(None); } @@ -243,3 +246,24 @@ impl Encoder<(RequestId, Tag, MaybeControls)> for LdapCodec { Ok(()) } } + +#[cfg(test)] +mod test { + use super::*; + + // A complete but empty BER sequence has no message id / protoop elements; + // popping them used to panic in `expect("element")`. + #[test] + fn decode_inner_empty_sequence_is_error() { + let mut buf = BytesMut::from(&b"\x30\x00"[..]); + assert!(decode_inner(&mut buf).is_err()); + } + + // A truncated frame is incomplete, not malformed: return Ok(None) so the + // codec waits for more bytes. + #[test] + fn decode_inner_incomplete_is_none() { + let mut buf = BytesMut::from(&b"\x30\x05"[..]); + assert!(matches!(decode_inner(&mut buf), Ok(None))); + } +} diff --git a/src/result.rs b/src/result.rs index 802e8f1..86cae14 100644 --- a/src/result.rs +++ b/src/result.rs @@ -17,7 +17,7 @@ use crate::ldap::SaslCreds; use crate::protocol::MiscSender; use crate::protocol::{LdapOp, MaybeControls, ResultSender}; use crate::search::ResultEntry; -use crate::search::parse_refs; +use crate::search::try_parse_refs; use lber::common::TagClass; use lber::parse::parse_uint; @@ -46,6 +46,10 @@ pub enum LdapError { #[error("the stream type in LdapConnSettings does not match the URL")] MismatchedStreamType, + /// The URL has no host component for a network scheme. + #[error("missing host in LDAP URL")] + EmptyHost, + /// Encapsulated I/O error. #[error("I/O error: {source}")] Io { @@ -96,6 +100,10 @@ pub enum LdapError { #[error("premature end of search stream")] EndOfStream, + /// A protocol message from the server was malformed or unexpected. + #[error("protocol decoding error: {0}")] + DecodingError(String), + /// URL parsing error. #[error("url parse error: {source}")] UrlParsing { @@ -142,6 +150,10 @@ pub enum LdapError { #[error("empty value set for Add")] AddNoValues, + /// No name provided for an extended operation request. + #[error("missing name in extended operation request")] + EmptyExopName, + /// No values provided for the Add operation. #[error("adapter init error: {0}")] AdapterInit(String), @@ -186,7 +198,7 @@ impl From for io::Error { fn from(le: LdapError) -> io::Error { match le { LdapError::Io { source, .. } => source, - _ => io::Error::new(io::ErrorKind::Other, format!("{}", le)), + _ => io::Error::other(format!("{le}")), } } } @@ -230,6 +242,14 @@ impl From for LdapResult { } } +impl LdapResult { + /// Parse an LDAP result message, returning a decoding error on a malformed + /// or unexpected protocol message. + pub(crate) fn try_from_tag(t: Tag) -> Result { + Ok(LdapResultExt::try_from_tag(t)?.0) + } +} + impl Error for LdapResult {} impl fmt::Display for LdapResult { @@ -318,12 +338,46 @@ impl LdapResult { #[derive(Clone, Debug)] pub(crate) struct LdapResultExt(pub LdapResult, pub Exop, pub SaslCreds); +#[doc(hidden)] impl From for LdapResultExt { fn from(t: Tag) -> LdapResultExt { + match LdapResultExt::try_from_tag(t) { + Ok(ext) => ext, + Err(e) => { + // Preserve the historical infallible API: rather than aborting, + // synthesize a protocolError result so callers see a non-zero rc. + warn!("failed to parse LDAP result: {e}"); + LdapResultExt( + LdapResult { + rc: 2, // protocolError + matched: String::from(""), + text: format!("{e}"), + refs: vec![], + ctrls: vec![], + }, + Exop { + name: None, + val: None, + }, + SaslCreds(None), + ) + } + } + } +} + +impl LdapResultExt { + /// Parse an LDAP result message, returning a decoding error on a malformed + /// or unexpected protocol message. + pub(crate) fn try_from_tag(t: Tag) -> Result { + fn decode>(msg: S) -> LdapError { + LdapError::DecodingError(msg.into()) + } + let t = match t { Tag::StructureTag(t) => t, Tag::Null(_) => { - return LdapResultExt( + return Ok(LdapResultExt( LdapResult { rc: 0, matched: String::from(""), @@ -336,37 +390,40 @@ impl From for LdapResultExt { val: None, }, SaslCreds(None), - ); + )); } - _ => unimplemented!(), + _ => return Err(decode("unexpected top-level tag in result")), }; - let mut tags = t.expect_constructed().expect("result sequence").into_iter(); + let mut tags = t + .expect_constructed() + .ok_or_else(|| decode("result sequence"))? + .into_iter(); let rc = match parse_uint( tags.next() - .expect("element") + .ok_or_else(|| decode("missing result code element"))? .match_class(TagClass::Universal) .and_then(|t| t.match_id(Types::Enumerated as u64)) .and_then(|t| t.expect_primitive()) - .expect("result code") + .ok_or_else(|| decode("result code"))? .as_slice(), ) { Ok((_, rc)) => rc as u32, - _ => panic!("failed to parse result code"), + _ => return Err(decode("failed to parse result code")), }; let matched = String::from_utf8( tags.next() - .expect("element") + .ok_or_else(|| decode("missing matched DN element"))? .expect_primitive() - .expect("octet string"), + .ok_or_else(|| decode("matched DN octet string"))?, ) - .expect("matched dn"); + .map_err(|e| decode(format!("matched DN is not valid UTF-8: {e}")))?; let text = String::from_utf8( tags.next() - .expect("element") + .ok_or_else(|| decode("missing diagnostic message element"))? .expect_primitive() - .expect("octet string"), + .ok_or_else(|| decode("diagnostic message octet string"))?, ) - .expect("diagnostic message"); + .map_err(|e| decode(format!("diagnostic message is not valid UTF-8: {e}")))?; let mut refs = Vec::new(); let mut exop_name = None; let mut exop_val = None; @@ -376,25 +433,34 @@ impl From for LdapResultExt { None => break, Some(comp) => match comp.id { 3 => { - refs.extend(parse_refs(comp)); + refs.extend(try_parse_refs(comp)?); } 7 => { - sasl_creds = Some(comp.expect_primitive().expect("octet string")); + sasl_creds = Some( + comp.expect_primitive() + .ok_or_else(|| decode("SASL credentials octet string"))?, + ); } 10 => { exop_name = Some( - String::from_utf8(comp.expect_primitive().expect("octet string")) - .expect("exop name"), + String::from_utf8( + comp.expect_primitive() + .ok_or_else(|| decode("exop name octet string"))?, + ) + .map_err(|e| decode(format!("exop name is not valid UTF-8: {e}")))?, ); } 11 => { - exop_val = Some(comp.expect_primitive().expect("octet string")); + exop_val = Some( + comp.expect_primitive() + .ok_or_else(|| decode("exop value octet string"))?, + ); } _ => (), }, } } - LdapResultExt( + Ok(LdapResultExt( LdapResult { rc, matched, @@ -407,7 +473,7 @@ impl From for LdapResultExt { val: exop_val, }, SaslCreds(sasl_creds), - ) + )) } } @@ -503,3 +569,95 @@ impl ExopResult { } } } + +#[cfg(test)] +mod test { + use super::*; + use lber::structure::{PL, StructureTag}; + + fn prim(class: TagClass, id: u64, val: &[u8]) -> StructureTag { + StructureTag { + class, + id, + payload: PL::P(val.to_vec()), + } + } + + // A bare primitive tag is not a valid result sequence and used to panic + // in `expect_constructed`. It must now surface as a decoding error. + #[test] + fn try_from_tag_primitive_is_error() { + let tag = Tag::StructureTag(prim(TagClass::Application, 5, b"")); + match LdapResultExt::try_from_tag(tag) { + Err(LdapError::DecodingError(_)) => {} + other => panic!("expected DecodingError, got {other:?}"), + } + } + + // An unexpected top-level tag variant used to hit `unimplemented!()`. + #[test] + fn try_from_tag_unexpected_variant_is_error() { + use lber::structures::Integer; + let tag = Tag::Integer(Integer { + inner: 1, + ..Default::default() + }); + match LdapResultExt::try_from_tag(tag) { + Err(LdapError::DecodingError(_)) => {} + other => panic!("expected DecodingError, got {other:?}"), + } + } + + // A result sequence missing the result-code element used to hit + // `expect("element")`. + #[test] + fn try_from_tag_missing_result_code_is_error() { + let tag = Tag::StructureTag(StructureTag { + class: TagClass::Application, + id: 5, + payload: PL::C(vec![]), + }); + match LdapResultExt::try_from_tag(tag) { + Err(LdapError::DecodingError(_)) => {} + other => panic!("expected DecodingError, got {other:?}"), + } + } + + // A Null tag is the success sentinel, not a malformed message. + #[test] + fn try_from_tag_null_is_success() { + let tag = Tag::Null(Default::default()); + let ext = LdapResultExt::try_from_tag(tag).expect("null parses"); + assert_eq!(ext.0.rc, 0); + } + + #[test] + fn try_from_tag_wellformed() { + let tag = Tag::StructureTag(StructureTag { + class: TagClass::Application, + id: 5, + payload: PL::C(vec![ + prim(TagClass::Universal, Types::Enumerated as u64, &[0]), + prim( + TagClass::Universal, + Types::OctetString as u64, + b"cn=matched", + ), + prim(TagClass::Universal, Types::OctetString as u64, b"diag"), + ]), + }); + let res = LdapResult::try_from_tag(tag).expect("wellformed parses"); + assert_eq!(res.rc, 0); + assert_eq!(res.matched, "cn=matched"); + assert_eq!(res.text, "diag"); + } + + // The search-done path in the connection loop relies on this: a malformed + // message becomes an rc=2 result so the search terminates instead of hanging. + #[test] + fn from_tag_malformed_is_protocol_error() { + let tag = Tag::StructureTag(prim(TagClass::Application, 5, b"")); + let res: LdapResult = tag.into(); + assert_eq!(res.rc, 2); + } +} diff --git a/src/search.rs b/src/search.rs index 6ccafac..0f2ccfc 100644 --- a/src/search.rs +++ b/src/search.rs @@ -1,21 +1,23 @@ -use std::collections::HashMap; -use std::fmt::Debug; -use std::sync::Arc; -use std::time::Duration; +use std::{collections::HashMap, fmt::Debug, sync::Arc, time::Duration}; -use crate::adapters::Adapter; -use crate::controls::Control; -use crate::ldap::Ldap; -use crate::parse_filter; -use crate::protocol::LdapOp; -use crate::result::{LdapError, LdapResult, Result}; +use lber::{ + common::TagClass, + structure::StructureTag, + structures::{Boolean, Enumerated, Integer, OctetString, Sequence, Tag}, +}; +use tokio::{ + sync::{Mutex, mpsc}, + time, +}; -use tokio::sync::{Mutex, mpsc}; -use tokio::time; - -use lber::common::TagClass; -use lber::structure::StructureTag; -use lber::structures::{Boolean, Enumerated, Integer, OctetString, Sequence, Tag}; +use crate::{ + adapters::Adapter, + controls::Control, + ldap::Ldap, + parse_filter, + protocol::LdapOp, + result::{LdapError, LdapResult, Result}, +}; /// Possible values for search scope. #[derive(Clone, Copy, Debug, PartialEq)] @@ -148,49 +150,66 @@ pub struct SearchEntry { impl SearchEntry { /// Parse raw BER data and convert it into attribute map(s). /// - /// __Note__: this function will panic on parsing error. + /// __Note__: this function will panic on parsing error. Use + /// [`try_construct()`](#method.try_construct) for a non-panicking variant + /// that returns a [`Result`](type.Result.html) instead. pub fn construct(re: ResultEntry) -> SearchEntry { + SearchEntry::try_construct(re).expect("entry") + } + + /// Parse raw BER data and convert it into attribute map(s), returning a + /// decoding error. + pub fn try_construct(re: ResultEntry) -> Result { + fn decode>(msg: S) -> LdapError { + LdapError::DecodingError(msg.into()) + } + let mut tags = re.0.match_id(4) .and_then(|t| t.expect_constructed()) - .expect("entry") + .ok_or_else(|| decode("entry"))? .into_iter(); let dn = String::from_utf8( tags.next() - .expect("element") + .ok_or_else(|| decode("missing DN element"))? .expect_primitive() - .expect("octet string"), + .ok_or_else(|| decode("DN octet string"))?, ) - .expect("dn"); + .map_err(|e| decode(format!("DN is not valid UTF-8: {e}")))?; let mut attr_vals = HashMap::new(); let mut bin_attr_vals = HashMap::new(); let attrs = tags .next() - .expect("element") + .ok_or_else(|| decode("missing attributes element"))? .expect_constructed() - .expect("attrs") + .ok_or_else(|| decode("attrs"))? .into_iter(); for a_v in attrs { let mut part_attr = a_v .expect_constructed() - .expect("partial attribute") + .ok_or_else(|| decode("partial attribute"))? .into_iter(); let a_type = String::from_utf8( part_attr .next() - .expect("element") + .ok_or_else(|| decode("missing attribute type element"))? .expect_primitive() - .expect("octet string"), + .ok_or_else(|| decode("attribute type octet string"))?, ) - .expect("attribute type"); + .map_err(|e| decode(format!("attribute type is not valid UTF-8: {e}")))?; let mut any_binary = false; let values = part_attr .next() - .expect("element") + .ok_or_else(|| decode("missing attribute values element"))? .expect_constructed() - .expect("values") + .ok_or_else(|| decode("values"))? + .into_iter() + .map(|t| { + t.expect_primitive() + .ok_or_else(|| decode("value octet string")) + }) + .collect::>>()? .into_iter() - .map(|t| t.expect_primitive().expect("octet string")) .filter_map(|s| { if let Ok(s) = std::str::from_utf8(s.as_ref()) { return Some(s.to_owned()); @@ -204,21 +223,24 @@ impl SearchEntry { }) .collect::>(); if any_binary { - bin_attr_vals.get_mut(&a_type).expect("bin vector").extend( - values - .into_iter() - .map(String::into_bytes) - .collect::>>(), - ); + bin_attr_vals + .get_mut(&a_type) + .ok_or_else(|| decode("bin vector"))? + .extend( + values + .into_iter() + .map(String::into_bytes) + .collect::>>(), + ); } else { attr_vals.insert(a_type, values); } } - SearchEntry { + Ok(SearchEntry { dn, attrs: attr_vals, bin_attrs: bin_attr_vals, - } + }) } } @@ -707,8 +729,7 @@ where let last_id = self.ldap.last_id; if let Err(e) = self.ldap.id_scrub_tx.send(last_id) { warn!( - "error sending scrub message from SearchStream::finish() for ID {}: {}", - last_id, e + "error sending scrub message from SearchStream::finish() for ID {last_id}: {e}" ); } } @@ -840,12 +861,92 @@ where } /// Parse the referrals from the supplied BER-encoded sequence. +/// +/// __Note__: this function will panic on parsing error. Use +/// [`try_parse_refs()`](fn.try_parse_refs.html) for a non-panicking variant. pub fn parse_refs(t: StructureTag) -> Vec { + try_parse_refs(t).expect("referrals") +} + +/// Parse the referrals from the supplied BER-encoded sequence, returning a +/// decoding error on malformed input. +pub fn try_parse_refs(t: StructureTag) -> Result> { + fn decode>(msg: S) -> LdapError { + LdapError::DecodingError(msg.into()) + } + t.expect_constructed() - .expect("referrals") + .ok_or_else(|| decode("referrals"))? .into_iter() - .map(|t| t.expect_primitive().expect("octet string")) - .map(String::from_utf8) - .map(|s| s.expect("uri")) + .map(|t| { + let bytes = t + .expect_primitive() + .ok_or_else(|| decode("referral octet string"))?; + String::from_utf8(bytes) + .map_err(|e| decode(format!("referral URI is not valid UTF-8: {e}"))) + }) .collect() } + +#[cfg(test)] +mod test { + use lber::structure::PL; + + use super::*; + + fn prim(id: u64, val: &[u8]) -> StructureTag { + StructureTag { + class: TagClass::Universal, + id, + payload: PL::P(val.to_vec()), + } + } + + fn cons(class: TagClass, id: u64, inner: Vec) -> StructureTag { + StructureTag { + class, + id, + payload: PL::C(inner), + } + } + + // A search entry whose top-level tag doesn't match id 4 used to panic in + // `expect("entry")`; it must now return a decoding error. + #[test] + fn try_construct_wrong_tag_is_error() { + let re = ResultEntry::new(prim(5, b"")); + match SearchEntry::try_construct(re) { + Err(LdapError::DecodingError(_)) => {} + other => panic!("expected DecodingError, got {other:?}"), + } + } + + #[test] + fn try_construct_wellformed() { + let entry = cons( + TagClass::Application, + 4, + vec![ + prim(4, b"cn=test"), + cons( + TagClass::Universal, + 16, + vec![cons( + TagClass::Universal, + 16, + vec![ + prim(4, b"cn"), + cons(TagClass::Universal, 17, vec![prim(4, b"test")]), + ], + )], + ), + ], + ); + let se = SearchEntry::try_construct(ResultEntry::new(entry)).expect("wellformed entry"); + assert_eq!(se.dn, "cn=test"); + assert_eq!( + se.attrs.get("cn").map(|v| v.as_slice()), + Some(&["test".to_string()][..]) + ); + } +} From f3fdf059b5d6156089f7317616954d49e27bd6e7 Mon Sep 17 00:00:00 2001 From: Aleksander <170264518+t-aleksander@users.noreply.github.com> Date: Wed, 3 Jun 2026 12:44:57 +0200 Subject: [PATCH 2/4] new pipelines --- .github/workflows/build.yml | 154 +++++++++++++++++++++++++++++------- deny.toml | 40 ++++++++++ examples/search_abandon.rs | 5 +- src/conn.rs | 5 +- src/ldap.rs | 2 +- src/lib.rs | 2 +- src/util.rs | 4 +- 7 files changed, 177 insertions(+), 35 deletions(-) create mode 100644 deny.toml diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index bfa3fad..8efede3 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -8,58 +8,158 @@ on: branches: - master +permissions: + contents: read + jobs: build: - runs-on: ${{ matrix.os }} + runs-on: + - codebuild-defguard-ldap3-runner-${{ github.run_id }}-${{ github.run_attempt }} + - instance-size:large + + container: public.ecr.aws/docker/library/rust:1 strategy: matrix: - os: [ubuntu-latest, windows-2022, macOS-latest] rust: [stable, nightly] features: ["default", "sync,tls-rustls-ring"] + env: + CARGO_TERM_COLOR: always + RUSTC_WRAPPER: sccache + SCCACHE_BUCKET: defguard-gh-build-cache + SCCACHE_REGION: eu-central-1 + AWS_ACCESS_KEY_ID: ${{ secrets.S3_CACHE_ACCESS_KEY }} + AWS_SECRET_ACCESS_KEY: ${{ secrets.S3_CACHE_SECRET_KEY }} + steps: - name: Checkout code - uses: actions/checkout@v1 - - name: Install rust toolchain - uses: actions-rs/toolchain@v1 + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6 + + - name: Export workspace-relative cache paths + run: | + echo "CARGO_HOME=$GITHUB_WORKSPACE/.cargo" >> $GITHUB_ENV + echo "$GITHUB_WORKSPACE/.cargo/bin" >> $GITHUB_PATH + + - name: Cache cargo registry + uses: actions/cache@27d5ce7f107fe9357f9df03efb73ab90386fccae # v5 with: - toolchain: ${{ matrix.rust }} - override: true + path: | + ${{ env.CARGO_HOME }}/registry/index + ${{ env.CARGO_HOME }}/registry/cache + ${{ env.CARGO_HOME }}/git/db + key: cargo-registry-${{ hashFiles('**/Cargo.lock') }} + restore-keys: cargo-registry- + + - name: Run sccache-cache + uses: mozilla-actions/sccache-action@7d986dd989559c6ecdb630a3fd2557667be217ad # v0.0.9 + + - name: Install Rust toolchain + run: | + rustup toolchain install ${{ matrix.rust }} --profile minimal + rustup default ${{ matrix.rust }} + - name: Build - uses: actions-rs/cargo@v1 # Force to build without warnings env: RUSTFLAGS: '-D warnings' - with: - command: build - args: --verbose --no-default-features --features ${{ matrix.features }} + run: cargo build --verbose --no-default-features --features ${{ matrix.features }} + - name: Run tests - uses: actions-rs/cargo@v1 - # force to build tests without warnings + # Force to build tests without warnings env: RUSTFLAGS: '-D warnings' + run: cargo test --verbose --no-default-features --features ${{ matrix.features }} + + - name: Show sccache stats + if: always() + run: sccache --show-stats + + clippy: + + runs-on: + - codebuild-defguard-ldap3-runner-${{ github.run_id }}-${{ github.run_attempt }} + - instance-size:large + + container: public.ecr.aws/docker/library/rust:1 + + strategy: + matrix: + features: ["default", "sync,tls-rustls-ring"] + + env: + CARGO_TERM_COLOR: always + RUSTC_WRAPPER: sccache + SCCACHE_BUCKET: defguard-gh-build-cache + SCCACHE_REGION: eu-central-1 + AWS_ACCESS_KEY_ID: ${{ secrets.S3_CACHE_ACCESS_KEY }} + AWS_SECRET_ACCESS_KEY: ${{ secrets.S3_CACHE_SECRET_KEY }} + + steps: + - name: Checkout code + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6 + + - name: Export workspace-relative cache paths + run: | + echo "CARGO_HOME=$GITHUB_WORKSPACE/.cargo" >> $GITHUB_ENV + echo "$GITHUB_WORKSPACE/.cargo/bin" >> $GITHUB_PATH + + - name: Cache cargo registry + uses: actions/cache@27d5ce7f107fe9357f9df03efb73ab90386fccae # v5 with: - command: test - args: --verbose --no-default-features --features ${{ matrix.features }} + path: | + ${{ env.CARGO_HOME }}/registry/index + ${{ env.CARGO_HOME }}/registry/cache + ${{ env.CARGO_HOME }}/git/db + key: cargo-registry-${{ hashFiles('**/Cargo.lock') }} + restore-keys: cargo-registry- + + - name: Run sccache-cache + uses: mozilla-actions/sccache-action@7d986dd989559c6ecdb630a3fd2557667be217ad # v0.0.9 + + - name: Run clippy linter + run: | + rustup component add clippy + cargo clippy --all-targets --no-default-features --features ${{ matrix.features }} -- -D warnings + + - name: Show sccache stats + if: always() + run: sccache --show-stats rustfmt_check: - runs-on: ubuntu-latest + runs-on: + - codebuild-defguard-ldap3-runner-${{ github.run_id }}-${{ github.run_attempt }} + - instance-size:small + + container: public.ecr.aws/docker/library/rust:1 steps: - name: Checkout code - uses: actions/checkout@v1 - - name: Ensure that rustfmt is installed - uses: actions-rs/toolchain@v1 - with: - toolchain: stable - override: true - components: rustfmt - - name: Run rustfmt - uses: actions-rs/cargo@v1 + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6 + + - name: Check format + run: | + rustup component add rustfmt + cargo fmt --all -- --check + + cargo_deny: + + runs-on: + - codebuild-defguard-ldap3-runner-${{ github.run_id }}-${{ github.run_attempt }} + - instance-size:small + + container: public.ecr.aws/docker/library/rust:1 + + steps: + - name: Checkout code + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6 + + - name: Install cargo-deny + uses: taiki-e/install-action@3235f8901fd37ffed0052b276cec25a362fb82e9 # v2 with: - command: fmt - args: --all -- --check + tool: cargo-deny + - name: Run cargo deny + run: cargo deny check diff --git a/deny.toml b/deny.toml new file mode 100644 index 0000000..7f7c1ff --- /dev/null +++ b/deny.toml @@ -0,0 +1,40 @@ +[graph] +all-features = true + +[advisories] +version = 2 +ignore = [ + # No constant-time RSA implementation is available yet; reachable only + # through the optional sspi (NTLM) dependency, not the default build. + { id = "RUSTSEC-2023-0071", reason = "https://github.com/RustCrypto/RSA/issues/19" }, +] + +[licenses] +version = 2 +allow = [ + "MIT", + "Apache-2.0", + "Apache-2.0 WITH LLVM-exception", + "MPL-2.0", + "BSD-2-Clause", + "BSD-3-Clause", + "Unicode-3.0", + "Zlib", + "ISC", + "BSL-1.0", + "0BSD", + "CC0-1.0", + "OpenSSL", + "CDLA-Permissive-2.0", + "NCSA", +] +confidence-threshold = 0.8 + +[bans] +multiple-versions = "warn" +wildcards = "allow" + +[sources] +unknown-registry = "warn" +unknown-git = "warn" +allow-registry = ["https://github.com/rust-lang/crates.io-index"] diff --git a/examples/search_abandon.rs b/examples/search_abandon.rs index b901c89..f3b63b5 100644 --- a/examples/search_abandon.rs +++ b/examples/search_abandon.rs @@ -23,9 +23,8 @@ async fn main() -> Result<()> { vec!["l"], ) .await?; - while let Some(_r) = stream.next().await? { - break; - } + // Fetch a single entry, then abandon the rest of the search. + let _ = stream.next().await?; let _res = stream.finish().await; let msgid = stream.ldap_handle().last_id(); ldap.abandon(msgid).await?; diff --git a/src/conn.rs b/src/conn.rs index 4575a9f..3c5f8c3 100644 --- a/src/conn.rs +++ b/src/conn.rs @@ -55,6 +55,9 @@ compile_error!( use tokio_util::codec::{Decoder, Framed}; use url::{self, Url}; +// One value exists per connection, so the size gap between the TLS and other +// variants doesn't matter; boxing the stream would only add indirection. +#[allow(clippy::large_enum_variant)] #[derive(Debug)] enum ConnType { Tcp(TcpStream), @@ -126,7 +129,7 @@ static CACERTS: LazyLock = LazyLock::new(|| { vec![] }; for cert in cert_vec { - if let Ok(_) = store.add(cert) {} + let _ = store.add(cert); } store }); diff --git a/src/ldap.rs b/src/ldap.rs index b6ccf2e..b02ffef 100644 --- a/src/ldap.rs +++ b/src/ldap.rs @@ -157,7 +157,7 @@ impl Ldap { let last_ldap_id = msgmap.0; let mut next_ldap_id = last_ldap_id; loop { - if next_ldap_id == std::i32::MAX { + if next_ldap_id == i32::MAX { next_ldap_id = 1; } else { next_ldap_id += 1; diff --git a/src/lib.rs b/src/lib.rs index 84c4faf..01d2f17 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -46,7 +46,7 @@ //! server if possible. See [`Ldap::sasl_ntlm_bind()`](struct.Ldap.html#method.sasl_ntlm_bind). //! //! * __tls__ (enabled by default): TLS support, backed by the `native-tls` crate, which uses -//! a platform-specific TLS backend. This is an alias for __tls-native__. +//! a platform-specific TLS backend. This is an alias for __tls-native__. //! //! * __tls-rustls-...__ (disabled by default): TLS support, backed by the Rustls library. The //! bare __tls-rustls__ flag, used previously for this purpose, won't work by itself; one diff --git a/src/util.rs b/src/util.rs index 17b0f90..8103b2c 100644 --- a/src/util.rs +++ b/src/util.rs @@ -344,10 +344,10 @@ pub fn ldap_unescape<'a, S: Into>>(val: S) -> Result> _ => (), } } - if output.is_some() { + if let Some(output) = output { if let Unescaper::Value(_) = esc { Ok(Cow::Owned( - String::from_utf8(output.unwrap()).map_err(|_| LdapError::DecodingUTF8)?, + String::from_utf8(output).map_err(|_| LdapError::DecodingUTF8)?, )) } else { Err(LdapError::DecodingUTF8) From 4f6f1def382366c44aab6092c81f440dc64e12d8 Mon Sep 17 00:00:00 2001 From: Aleksander <170264518+t-aleksander@users.noreply.github.com> Date: Wed, 3 Jun 2026 12:46:35 +0200 Subject: [PATCH 3/4] Trigger workflows From db9a6b7da79d862ff0b6d0982b19e7385ee626fb Mon Sep 17 00:00:00 2001 From: Aleksander <170264518+t-aleksander@users.noreply.github.com> Date: Wed, 3 Jun 2026 12:50:03 +0200 Subject: [PATCH 4/4] trigger on prs to dev --- .github/workflows/build.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 8efede3..f7ba47e 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -7,6 +7,7 @@ on: pull_request: branches: - master + - dev permissions: contents: read