[pbs-devel] [POC proxmox proxmox-backup 0/4] use custom DNS resolver for

[pbs-devel] [POC proxmox proxmox-backup 0/4] use custom DNS resolver for

[Christian Ebner]

As discussed in issue #4788 [0], statically linking of the
`proxmox-backup-client` still suffers from possible incompatible
dependencies on the NSS module libraries, further described in
[1].

This proof of concept drafts a possible workaround by relying on
`hickory-dns` [2], using the hickory-resolver for name resolution
instead of the default `getaddrinfo` based `GaiResolver` used by
the hyper http client, as suggested by Thomas.

These patches still need some polishing, but allow already to compile
a static binary without the glibc issues as descibied in the issue,
therefore sending them for possible more widespread testing.

Initial testing on ArchLinux with glic 2.40+r16+gaa533d58ff-2,
previously leading to floating point exceptions, now allows to backup
without issues using the patched client.

A major downside is the required dependencies for hickory-dns`, as with
the feature `system-config` enabled (used to read nameservers from
`/etc/resolv.conf`), also the windows dependencies on `ipconfig` and
it's subdependencies are required.

[0] https://bugzilla.proxmox.com/show_bug.cgi?id=4788
[1] https://sourceware.org/glibc/wiki/FAQ#Even_statically_linked_programs_need_some_shared_libraries_which_is_not_acceptable_for_me.__What_can_I_do.3F
[2] https://github.com/hickory-dns/hickory-dns

proxmox:

Christian Ebner (1):
  http: client: make https connector generic over resolver

 proxmox-http/Cargo.toml              |  1 +
 proxmox-http/debian/control          |  8 +++++---
 proxmox-http/src/client/connector.rs | 17 ++++++++++++-----
 proxmox-http/src/client/simple.rs    |  3 ++-
 4 files changed, 20 insertions(+), 9 deletions(-)

proxmox-backup:

Christian Ebner (3):
  Makefile: target for statically linked client binary
  Makefile: switch path based on build mode by using COMPILEDIR
  client: http: Use custom resolver for statically linked binary

 Cargo.toml                    |  6 +++
 Makefile                      |  9 ++++
 pbs-client/Cargo.toml         |  1 +
 pbs-client/src/http_client.rs | 81 +++++++++++++++++++++++++++++++++--
 4 files changed, 93 insertions(+), 4 deletions(-)

-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [POC proxmox 1/4] http: client: make https connector generic over resolver

[Christian Ebner]

Allow to instantiate a `HttpsConnector` not using the default
`getaddrinfo` based `GaiResolver` for domain name resolution, but
rather a custom resolver implementing the required traits.

The usecase for this is to swap out the DNS resolver for the
statically linked proxmox-backup-client binary, where the glibc
dependency is problematic because of possible ABI incompatibility.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 proxmox-http/Cargo.toml              |  1 +
 proxmox-http/debian/control          |  8 +++++---
 proxmox-http/src/client/connector.rs | 17 ++++++++++++-----
 proxmox-http/src/client/simple.rs    |  3 ++-
 4 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/proxmox-http/Cargo.toml b/proxmox-http/Cargo.toml
index a15c3367..4a78880c 100644
--- a/proxmox-http/Cargo.toml
+++ b/proxmox-http/Cargo.toml
@@ -21,6 +21,7 @@ openssl =  { version = "0.10", optional = true }
 serde_json = { workspace = true, optional = true }
 tokio = { workspace = true, features = [], optional = true }
 tokio-openssl = { workspace = true, optional = true }
+tower-service.workspace = true
 ureq = { version = "2.4", features = ["native-certs"], optional = true }
 url = { workspace = true, optional = true }
 
diff --git a/proxmox-http/debian/control b/proxmox-http/debian/control
index 2d4d74b7..f5402ae6 100644
--- a/proxmox-http/debian/control
+++ b/proxmox-http/debian/control
@@ -6,7 +6,8 @@ Build-Depends: debhelper (>= 12),
  cargo:native <!nocheck>,
  rustc:native <!nocheck>,
  libstd-rust-dev <!nocheck>,
- librust-anyhow-1+default-dev <!nocheck>
+ librust-anyhow-1+default-dev <!nocheck>,
+ librust-tower-service-0.3+default-dev <!nocheck>
 Maintainer: Proxmox Support Team <support@proxmox.com>
 Standards-Version: 4.6.2
 Vcs-Git: git://git.proxmox.com/git/proxmox.git
@@ -19,7 +20,8 @@ Architecture: any
 Multi-Arch: same
 Depends:
  ${misc:Depends},
- librust-anyhow-1+default-dev
+ librust-anyhow-1+default-dev,
+ librust-tower-service-0.3+default-dev
 Suggests:
  librust-proxmox-http+client-dev (= ${binary:Version}),
  librust-proxmox-http+client-sync-dev (= ${binary:Version}),
@@ -56,7 +58,7 @@ Depends:
  librust-hyper-0.14+stream-dev (>= 0.14.5-~~),
  librust-hyper-0.14+tcp-dev (>= 0.14.5-~~),
  librust-openssl-0.10+default-dev,
- librust-proxmox-compression-0.2+default-dev (>= 0.2.3-~~),
+ librust-proxmox-compression-0.2+default-dev (>= 0.2.4-~~),
  librust-tokio-1+default-dev (>= 1.6-~~),
  librust-tokio-1+io-util-dev (>= 1.6-~~),
  librust-tokio-openssl-0.6+default-dev (>= 0.6.1-~~)
diff --git a/proxmox-http/src/client/connector.rs b/proxmox-http/src/client/connector.rs
index 63b9d10c..c0435c60 100644
--- a/proxmox-http/src/client/connector.rs
+++ b/proxmox-http/src/client/connector.rs
@@ -6,6 +6,7 @@ use std::task::{Context, Poll};
 
 use futures::*;
 use http::Uri;
+use hyper::client::connect::dns::Name;
 use hyper::client::HttpConnector;
 use openssl::ssl::SslConnector;
 use tokio::io::{AsyncRead, AsyncReadExt, AsyncWrite, AsyncWriteExt};
@@ -23,8 +24,8 @@ use crate::{RateLimitedStream, ShareableRateLimit};
 type SharedRateLimit = Arc<dyn ShareableRateLimit>;
 
 #[derive(Clone)]
-pub struct HttpsConnector {
-    connector: HttpConnector,
+pub struct HttpsConnector<T> {
+    connector: HttpConnector<T>,
     ssl_connector: Arc<SslConnector>,
     proxy: Option<ProxyConfig>,
     tcp_keepalive: u32,
@@ -32,9 +33,9 @@ pub struct HttpsConnector {
     write_limiter: Option<SharedRateLimit>,
 }
 
-impl HttpsConnector {
+impl<T> HttpsConnector<T> {
     pub fn with_connector(
-        mut connector: HttpConnector,
+        mut connector: HttpConnector<T>,
         ssl_connector: SslConnector,
         tcp_keepalive: u32,
     ) -> Self {
@@ -122,7 +123,13 @@ impl HttpsConnector {
     }
 }
 
-impl hyper::service::Service<Uri> for HttpsConnector {
+impl<T> hyper::service::Service<Uri> for HttpsConnector<T>
+where
+    T: tower_service::Service<Name> + Clone + Send + Sync + 'static,
+    T::Future: Send,
+    T::Error: Into<Box<(dyn std::error::Error + Send + Sync + 'static)>>,
+    T::Response: std::iter::Iterator<Item = std::net::SocketAddr>,
+{
     type Response = MaybeTlsStream<RateLimitedStream<TcpStream>>;
     type Error = Error;
     #[allow(clippy::type_complexity)]
diff --git a/proxmox-http/src/client/simple.rs b/proxmox-http/src/client/simple.rs
index 062889ac..cb8bb777 100644
--- a/proxmox-http/src/client/simple.rs
+++ b/proxmox-http/src/client/simple.rs
@@ -8,6 +8,7 @@ use futures::*;
 #[cfg(all(feature = "client-trait", feature = "proxmox-async"))]
 use http::header::HeaderName;
 use http::{HeaderValue, Request, Response};
+use hyper::client::connect::dns::GaiResolver;
 use hyper::client::Client as HyperClient;
 use hyper::client::HttpConnector;
 use hyper::Body;
@@ -18,7 +19,7 @@ use crate::HttpOptions;
 
 /// Asynchronous HTTP client implementation
 pub struct Client {
-    client: HyperClient<HttpsConnector, Body>,
+    client: HyperClient<HttpsConnector<GaiResolver>, Body>,
     options: HttpOptions,
 }
 
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [POC proxmox-backup 2/4] Makefile: target for statically linked client binary

[Christian Ebner]

Adds the build target including workarounds to generate a statically
linked version of the proxmox-backup-client binary.

Suggested-by: Christoph Heiss <c.heiss@proxmox.com>
Originally-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 Makefile | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Makefile b/Makefile
index dfbaacab4..00e7f7401 100644
--- a/Makefile
+++ b/Makefile
@@ -227,3 +227,12 @@ upload: $(SERVER_DEB) $(CLIENT_DEB) $(RESTORE_DEB) $(DOC_DEB)
 	  | ssh -X repoman@repo.proxmox.com upload --product pbs --dist $(UPLOAD_DIST)
 	tar cf - $(CLIENT_DEB) $(CLIENT_DBG_DEB) | ssh -X repoman@repo.proxmox.com upload --product "pve,pmg,pbs-client" --dist $(UPLOAD_DIST)
 	tar cf - $(RESTORE_DEB) $(RESTORE_DBG_DEB) | ssh -X repoman@repo.proxmox.com upload --product "pve" --dist $(UPLOAD_DIST)
+
+.PHONY: proxmox-backup-client-static
+proxmox-backup-client-static:
+	mkdir -p target/release/deps/ && \
+          echo '!<arch>' > target/release/deps/libsystemd.a # workaround for to greedy linkage and proxmox-systemd
+	RUSTFLAGS='-C target-feature=+crt-static -C strip=debuginfo' \
+        $(CARGO) build $(CARGO_BUILD_ARGS) \
+          --package proxmox-backup-client --bin proxmox-backup-client \
+          --target x86_64-unknown-linux-gnu
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [POC proxmox-backup 3/4] Makefile: switch path based on build mode by using COMPILEDIR

[Christian Ebner]

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 00e7f7401..691dd2aad 100644
--- a/Makefile
+++ b/Makefile
@@ -230,8 +230,8 @@ upload: $(SERVER_DEB) $(CLIENT_DEB) $(RESTORE_DEB) $(DOC_DEB)
 
 .PHONY: proxmox-backup-client-static
 proxmox-backup-client-static:
-	mkdir -p target/release/deps/ && \
-          echo '!<arch>' > target/release/deps/libsystemd.a # workaround for to greedy linkage and proxmox-systemd
+	mkdir -p $(COMPILEDIR)/deps/ && \
+          echo '!<arch>' > $(COMPILEDIR)/deps/libsystemd.a # workaround for to greedy linkage and proxmox-systemd
 	RUSTFLAGS='-C target-feature=+crt-static -C strip=debuginfo' \
         $(CARGO) build $(CARGO_BUILD_ARGS) \
           --package proxmox-backup-client --bin proxmox-backup-client \
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [POC proxmox-backup 4/4] client: http: Use custom resolver for statically linked binary

[Christian Ebner]

The dependency on the `getaddrinfo` based `GaiResolver` used by
default for the `HttpClient` is not suitable for the statically
linked binary of the `proxmox-backup-client`, because of the
dependency on glibc NSS libraries, as described in glibc's FAQs [0].

As a workaround, conditionally compile the binary using the `hickory-dns`
resolver.

[0] https://sourceware.org/glibc/wiki/FAQ#Even_statically_linked_programs_need_some_shared_libraries_which_is_not_acceptable_for_me.__What_can_I_do.3F

Suggested-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 Cargo.toml                    |  6 +++
 pbs-client/Cargo.toml         |  1 +
 pbs-client/src/http_client.rs | 81 +++++++++++++++++++++++++++++++++--
 3 files changed, 84 insertions(+), 4 deletions(-)

diff --git a/Cargo.toml b/Cargo.toml
index fc1409d66..792226258 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -160,6 +160,7 @@ walkdir = "2"
 xdg = "2.2"
 zstd = { version = "0.12", features = [ "bindgen" ] }
 zstd-safe = "6.0"
+hickory-resolver = { git = "https://github.com/hickory-dns/hickory-dns", tag = "v0.24.1", default-features = false }
 
 [dependencies]
 anyhow.workspace = true
@@ -254,6 +255,11 @@ proxmox-rrd-api-types.workspace = true
 # Local path overrides
 # NOTE: You must run `cargo update` after changing this for it to take effect!
 [patch.crates-io]
+hickory-proto = { git = "https://github.com/hickory-dns/hickory-dns", tag = "v0.24.1" }
+ipconfig = { git = "https://github.com/liranringel/ipconfig", tag = "0.3.2" }
+widestring = { git = "https://github.com/starkat99/widestring-rs", tag = "v1.1.0" }
+windows-sys = { git = "https://github.com/microsoft/windows-rs", tag = "0.48.0" }
+winreg = { git = "https://github.com/gentoo90/winreg-rs", tag = "v0.50.0" }
 
 #proxmox-apt = { path = "../proxmox/proxmox-apt" }
 #proxmox-apt-api-types = { path = "../proxmox/proxmox-apt-api-types" }
diff --git a/pbs-client/Cargo.toml b/pbs-client/Cargo.toml
index 00c18b83a..785e7c4de 100644
--- a/pbs-client/Cargo.toml
+++ b/pbs-client/Cargo.toml
@@ -29,6 +29,7 @@ tokio = { workspace = true, features = [ "fs", "signal" ] }
 tokio-stream.workspace = true
 tower-service.workspace = true
 xdg.workspace = true
+hickory-resolver = { workspace = true, features = [ "system-config", "tokio-runtime" ] }
 
 pathpatterns.workspace = true
 
diff --git a/pbs-client/src/http_client.rs b/pbs-client/src/http_client.rs
index 8ae5edaa0..cdec8e42a 100644
--- a/pbs-client/src/http_client.rs
+++ b/pbs-client/src/http_client.rs
@@ -7,6 +7,8 @@ use futures::*;
 use http::header::HeaderValue;
 use http::Uri;
 use http::{Request, Response};
+#[cfg(not(target_feature = "crt-static"))]
+use hyper::client::connect::dns::GaiResolver;
 use hyper::client::{Client, HttpConnector};
 use hyper::Body;
 use openssl::{
@@ -32,6 +34,74 @@ use pbs_api_types::{Authid, RateLimitConfig, Userid};
 use super::pipe_to_stream::PipeToSendStream;
 use super::PROXMOX_BACKUP_TCP_KEEPALIVE_TIME;
 
+#[cfg(not(target_feature = "crt-static"))]
+type DnsResolver = GaiResolver;
+
+#[cfg(target_feature = "crt-static")]
+type DnsResolver = resolver::HickoryDnsResolver;
+
+#[cfg(target_feature = "crt-static")]
+mod resolver {
+    use std::net::SocketAddr;
+    use std::pin::Pin;
+    use std::sync::Arc;
+    use std::task::{Context, Poll};
+
+    use futures::Future;
+    use hickory_resolver::error::ResolveError;
+    use hickory_resolver::lookup_ip::LookupIpIntoIter;
+    use hickory_resolver::TokioAsyncResolver;
+    use hyper::client::connect::dns::Name;
+    use tower_service::Service;
+
+    pub(crate) struct SocketAddrIter {
+        inner: LookupIpIntoIter,
+    }
+
+    impl Iterator for SocketAddrIter {
+        type Item = SocketAddr;
+
+        fn next(&mut self) -> Option<Self::Item> {
+            self.inner.next().map(|ip_addr| SocketAddr::new(ip_addr, 0))
+        }
+    }
+
+    #[derive(Clone)]
+    pub(crate) struct HickoryDnsResolver {
+        inner: Arc<TokioAsyncResolver>,
+    }
+
+    impl HickoryDnsResolver {
+        pub(crate) fn new() -> Self {
+            Self {
+                inner: Arc::new(TokioAsyncResolver::tokio_from_system_conf().unwrap()),
+            }
+        }
+    }
+
+    impl Service<Name> for HickoryDnsResolver {
+        type Response = SocketAddrIter;
+        type Error = ResolveError;
+        type Future = Pin<Box<dyn Future<Output = Result<Self::Response, Self::Error>> + Send>>;
+
+        fn poll_ready(&mut self, _ctx: &mut Context<'_>) -> Poll<Result<(), Self::Error>> {
+            Poll::Ready(Ok(()))
+        }
+
+        fn call(&mut self, name: Name) -> Self::Future {
+            let inner = self.inner.clone();
+            Box::pin(async move {
+                inner
+                    .lookup_ip(name.as_str())
+                    .await
+                    .map(|r| SocketAddrIter {
+                        inner: r.into_iter(),
+                    })
+            })
+        }
+    }
+}
+
 /// Timeout used for several HTTP operations that are expected to finish quickly but may block in
 /// certain error conditions. Keep it generous, to avoid false-positive under high load.
 const HTTP_TIMEOUT: Duration = Duration::from_secs(2 * 60);
@@ -133,7 +203,7 @@ impl Default for HttpClientOptions {
 
 /// HTTP(S) API client
 pub struct HttpClient {
-    client: Client<HttpsConnector>,
+    client: Client<HttpsConnector<DnsResolver>>,
     server: String,
     port: u16,
     fingerprint: Arc<Mutex<Option<String>>>,
@@ -364,7 +434,8 @@ impl HttpClient {
             ssl_connector_builder.set_verify(openssl::ssl::SslVerifyMode::NONE);
         }
 
-        let mut httpc = HttpConnector::new();
+        let resolver = DnsResolver::new();
+        let mut httpc = HttpConnector::new_with_resolver(resolver);
         httpc.set_nodelay(true); // important for h2 download performance!
         httpc.enforce_http(false); // we want https...
 
@@ -525,7 +596,9 @@ impl HttpClient {
             _options: options,
         })
     }
+}
 
+impl HttpClient {
     /// Login
     ///
     /// Login is done on demand, so this is only required if you need
@@ -814,7 +887,7 @@ impl HttpClient {
     }
 
     async fn credentials(
-        client: Client<HttpsConnector>,
+        client: Client<HttpsConnector<DnsResolver>>,
         server: String,
         port: u16,
         username: Userid,
@@ -859,7 +932,7 @@ impl HttpClient {
     }
 
     async fn api_request(
-        client: Client<HttpsConnector>,
+        client: Client<HttpsConnector<DnsResolver>>,
         req: Request<Body>,
     ) -> Result<Value, Error> {
         Self::api_response(
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [POC proxmox proxmox-backup 0/4] use custom DNS resolver for

[Gabriel Goller]

On 28.09.2024 11:42, Christian Ebner wrote:
>As discussed in issue #4788 [0], statically linking of the
>`proxmox-backup-client` still suffers from possible incompatible
>dependencies on the NSS module libraries, further described in
>[1].
>
>This proof of concept drafts a possible workaround by relying on
>`hickory-dns` [2], using the hickory-resolver for name resolution
>instead of the default `getaddrinfo` based `GaiResolver` used by
>the hyper http client, as suggested by Thomas.
>
>These patches still need some polishing, but allow already to compile
>a static binary without the glibc issues as descibied in the issue,
>therefore sending them for possible more widespread testing.
>
>Initial testing on ArchLinux with glic 2.40+r16+gaa533d58ff-2,
>previously leading to floating point exceptions, now allows to backup
>without issues using the patched client.
>
>A major downside is the required dependencies for hickory-dns`, as with
>the feature `system-config` enabled (used to read nameservers from
>`/etc/resolv.conf`), also the windows dependencies on `ipconfig` and
>it's subdependencies are required.

This would be a dealbreaker for us IMO, but luckily this has already
been fixed on the main branch!

The whole 'ipconfig' crate is an optional dependency and should only be
pulled in if the target is windows. This is already implemented in the
latest version [1], but it doesn't yet work quite right. This pull
request (already merged) [0], solves this by removes the implicit
features (which are created on a optional dependency automatically).

This means we either use the main branch for now, or wait until the next
version is released and we can remove ipconfig, widestring, windows-sys
and winreg!

[0]: https://github.com/hickory-dns/hickory-dns/pull/2337
[1]: https://github.com/hickory-dns/hickory-dns/blob/7887002d1198ed9faaaae531e0f9815c10589f7a/crates/resolver/Cargo.toml#L104


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [POC proxmox proxmox-backup 0/4] use custom DNS resolver for

[Christian Ebner]

On 10/8/24 16:50, Gabriel Goller wrote:
> On 28.09.2024 11:42, Christian Ebner wrote:
>> As discussed in issue #4788 [0], statically linking of the
>> `proxmox-backup-client` still suffers from possible incompatible
>> dependencies on the NSS module libraries, further described in
>> [1].
>>
>> This proof of concept drafts a possible workaround by relying on
>> `hickory-dns` [2], using the hickory-resolver for name resolution
>> instead of the default `getaddrinfo` based `GaiResolver` used by
>> the hyper http client, as suggested by Thomas.
>>
>> These patches still need some polishing, but allow already to compile
>> a static binary without the glibc issues as descibied in the issue,
>> therefore sending them for possible more widespread testing.
>>
>> Initial testing on ArchLinux with glic 2.40+r16+gaa533d58ff-2,
>> previously leading to floating point exceptions, now allows to backup
>> without issues using the patched client.
>>
>> A major downside is the required dependencies for hickory-dns`, as with
>> the feature `system-config` enabled (used to read nameservers from
>> `/etc/resolv.conf`), also the windows dependencies on `ipconfig` and
>> it's subdependencies are required.
> 
> This would be a dealbreaker for us IMO, but luckily this has already
> been fixed on the main branch!

That is great! Was afraid to have to patch this out on our side.

> The whole 'ipconfig' crate is an optional dependency and should only be
> pulled in if the target is windows. This is already implemented in the
> latest version [1], but it doesn't yet work quite right. This pull
> request (already merged) [0], solves this by removes the implicit
> features (which are created on a optional dependency automatically).

Thanks a lot for looking deeper into this! I did miss that pull request 
while looking at this.

> This means we either use the main branch for now, or wait until the next
> version is released and we can remove ipconfig, widestring, windows-sys
> and winreg!

So only the hickory crates would need to be packaged as build 
dependencies if this is the way to move forward.
> [0]: https://github.com/hickory-dns/hickory-dns/pull/2337
> [1]: 
> https://github.com/hickory-dns/hickory-dns/blob/7887002d1198ed9faaaae531e0f9815c10589f7a/crates/resolver/Cargo.toml#L104




_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel