feat: implement management client with auth module by shefali-kamal · Pull Request #49 · veraison/rust-apiclient

shefali-kamal · 2026-09-23T15:06:47Z

implement management client for veraison/services
add auth module required to submit the policy to the management server

paulhowardarm

Nice extra function for the Rust client, thanks. Looks good overall, although I'm not too familiar with the management API, so definitely worth a second review. I have requested some naming changes, and offered some advice on zeroing sensitive data fields in the authentication parts.

paulhowardarm · 2026-09-25T12:21:06Z

+
+/// Supplies the value of the HTTP Authorization header.
+#[async_trait::async_trait]
+pub trait IAuthenticator {


The name IAuthenticator is not idiomatic for Rust traits. At the very least, the I prefix should be removed, because this is not a convention in Rust. Authenticator is probably suitable, but an alternative would be Authenticate. Rust tends to use verbs or agent nouns for traits.

updated to Authenticator.

paulhowardarm · 2026-09-25T12:30:31Z

+
+/// A Basic authenticator that uses a username and password to generate a Basic Authorization header.
+#[derive(Default, Deserialize)]
+pub struct BasicAuthenticator {


Since this type contains a secret (password), it would probably be good practice to zeroize it on drop. Look at using the zeroize crate and provide a Drop implementation. See: https://docs.rs/zeroize/latest/zeroize/

done, please check.

paulhowardarm · 2026-09-25T12:31:19Z

+}
+
+/// An OAuth2 authenticator that uses the Resource Owner Password Credentials grant to obtain an access token and generate a Bearer Authorization header.
+#[derive(Default, Deserialize)]


As with BasicAuthenticator, we probably want to ensure any secret-containing fields are zeroized on drop.

paulhowardarm · 2026-09-25T12:32:32Z

@@ -90,6 +134,7 @@ pub trait ConfigureHttp: Sized {
 /// The common builder allows for the configuration of custom TLS root certificates along with
 /// middleware layers for client-side caching.


Update the comment here - middleware layers for client-side caching or authentication.

paulhowardarm · 2026-09-25T12:34:27Z

+        let authentication = AuthenticationMiddleware {
+            authenticator: Arc::new(Mutex::new(self.authenticator)),
+        };
        // Now add any required middleware to the client


Need to reposition the comment about adding middleware, because we've already started doing so.

paulhowardarm · 2026-09-25T12:37:14Z

+}
+
+/// A builder for [PolicyRunner] objects.
+pub struct PolicyRunnerBuilder {


I would reconsider the names PolicyRunner and PolicyRunnerBuilder. I don't think Runner is very suitable in this context. I suspect this might have been borrowed from the CoSERV client, which uses QueryRunner. But it works in that case because the CoSERV client is about running queries. I think PolicyManager and PolicyManagerBuilder would make more sense than Runner here.

updated as per suggestion.

- implement management client for veraison/services - add auth module required to submit the policy to the management server Signed-off-by: Badiyasar, Harshita <Harshita.Badiyasar@fujitsu.com>

shefali-kamal · 2026-10-23T06:42:49Z

Hi @paulhowardarm ,
Thanks for the review comments. Can you please check the comments addressed and if it looks good approve the PR.

paulhowardarm

Thank you for addressing my earlier observations. Sorry for the delay on re-review, but this LGTM now.

shefali-kamal · 2026-10-23T11:32:09Z

Hi @thomas-fossati ,
Please review this PR. We have received approval from Paul.

thomas-fossati

Thanks for the nice contribution and sorry for taking so long to review.

I have inlined a few comments.

thomas-fossati · 2026-10-29T08:41:52Z

+    ) -> MiddlewareResult<reqwest::Response> {
+        let header = self
+            .authenticator
+            .lock()


this is good, because it avoids n parallel refreshes, but if the IdP is slow or hung up this will block indefinitely. We should probably add a "reasonable" timeout to the token request.

thomas-fossati · 2026-10-29T08:44:00Z

+            .append_pair("scope", "openid")
+            .finish();
+        let response = client
+            .post(&self.token_url)


(see comment in http.rs about "locking forever".)

we should put a timeout on this request, say 15-20 seconds.

thomas-fossati · 2026-10-29T08:54:03Z

+{
+    let mut builder = HttpClientBuilder::new();
+    for path in cert_paths {
+        builder = builder.with_root_certificate(PathBuf::from(path.as_ref()));


hold on, with_root_certificate is not additive, right? so, if len(cert_paths) > 1 only the last cert survives, which is probably not what you intended.

thomas-fossati · 2026-10-29T09:50:56Z

+        let mut url = self.base_url.clone();
+        let path = template.replace(":scheme", scheme).replace(
+            ":uuid",
+            &policy_id.map_or_else(String::new, |id| id.to_string()),
+        );
+        url.set_path(&path);


this throws away the base URL's path, which may be problematic if the service is behind a reverse proxy/LB that mounts the service on a dedicated path.

another thought: who selects scheme? Asking because applying set_path on an arbitrary input may result in "path evasion" situations.

thomas-fossati · 2026-10-29T09:54:30Z

+
+impl CachedTokenResponse {
+    // If expiry is not given then do not cache the token, and always obtain a new one.
+    fn is_expired(&self) -> bool {


this can become a bit wobbly when the token is nearly but not quite yet expired: it'll pass the check but then it'll get rejected.
I suggest to subtract a few seconds of skew (say 30s).

A (slightly more convoluted) alternative would be to retry once on 401.

thomas-fossati · 2026-10-29T09:58:12Z

+    }
+
+    /// Get all policies for a scheme. (optially filtered by name)
+    pub async fn get_policies(&self, scheme: &str, name: &str) -> Result<Vec<Policy>, Error> {


maybe name: Option<&str>

thomas-fossati · 2026-10-29T09:58:55Z

+        Self::policy_from_response(Self::expect_status(response, StatusCode::OK).await?).await
+    }
+
+    /// Get all policies for a scheme. (optially filtered by name)


Suggested change

/// Get all policies for a scheme. (optially filtered by name)

/// Get all policies for a scheme. (optionally filtered by name)

thomas-fossati · 2026-10-29T10:00:54Z

+            .append_pair("grant_type", "password")
+            .append_pair("username", &self.username)
+            .append_pair("password", &self.password)
+            .append_pair("scope", "openid")


why is this hardcoded? I think that the scope should be configurable.

thomas-fossati · 2026-10-29T10:03:43Z

        }
    }
+
+    /// Obtains the capabilities and endpoints of the Veraison verification service.


Suggested change

/// Obtains the capabilities and endpoints of the Veraison verification service.

/// Obtains the capabilities and endpoints of the Veraison management service.

thomas-fossati · 2026-10-29T10:04:31Z

+const CREATE_POLICY_ENDPOINT: &str = "/management/v1/policy/:scheme";
+const GET_POLICY_ENDPOINT: &str = "/management/v1/policy/:scheme/:uuid";
+const DEACTIVATE_POLICIES_ENDPOINT: &str = "/management/v1/policies/:scheme/deactivate";
+const GET_ACTIVE_POLICY_ENDPOINT: &str = "/management/v1/policy/:scheme";
+const ACTIVATE_POLICY_ENDPOINT: &str = "/management/v1/policy/:scheme/:uuid/activate";
+const GET_POLICIES_ENDPOINT: &str = "/management/v1/policies/:scheme";


unclear if these are hardcoded or discovered?

shefali-kamal requested review from paulhowardarm and thomas-fossati September 23, 2026 15:06

paulhowardarm requested changes Sep 25, 2026

View reviewed changes

feat: implement management client with auth module

2ad447f

- implement management client for veraison/services - add auth module required to submit the policy to the management server Signed-off-by: Badiyasar, Harshita <Harshita.Badiyasar@fujitsu.com>

shefali-kamal force-pushed the main branch from a59971f to 2ad447f Compare September 29, 2026 13:48

shefali-kamal requested a review from paulhowardarm September 29, 2026 13:57

THS-on self-assigned this Oct 20, 2026

paulhowardarm approved these changes Oct 23, 2026

View reviewed changes

thomas-fossati reviewed Oct 29, 2026

View reviewed changes

		@@ -90,6 +134,7 @@ pub trait ConfigureHttp: Sized {
		/// The common builder allows for the configuration of custom TLS root certificates along with
		/// middleware layers for client-side caching.

	/// Get all policies for a scheme. (optially filtered by name)
	/// Get all policies for a scheme. (optionally filtered by name)

	/// Obtains the capabilities and endpoints of the Veraison verification service.
	/// Obtains the capabilities and endpoints of the Veraison management service.

Conversation

shefali-kamal commented Sep 23, 2026

Uh oh!

paulhowardarm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shefali-kamal commented Oct 23, 2026

Uh oh!

paulhowardarm left a comment

Choose a reason for hiding this comment

Uh oh!

shefali-kamal commented Oct 23, 2026

Uh oh!

thomas-fossati left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants