try other agent addresses on 5XX error response #19497

cconard96 · 2025-04-17T13:15:31Z

Checklist before requesting a review

I have read the CONTRIBUTING document.
I have performed a self-review of my code.
I have added tests that prove my fix is effective or that my feature works.
This change requires a documentation update.

Description

Fixes #19479

I have no way to test, but I guess if GLPI gets a 500 level error with one agent address, it should still try the other ones.

cedric-anne · 2025-04-17T13:21:07Z

src/Agent.php

@@ -722,6 +723,10 @@ public function requestAgent($endpoint): Response
                self::$found_address = $address;
                break;
            } catch (\GuzzleHttp\Exception\RequestException $exception) {


What kind of error should prevent trying to other addresses here?

If the response status is a 4xx, does it means our request is wrong, or does it means that we reached an unexpected application that does not understand our request?
Same question for 5xx statuses.

Then remains the ConnectException. This one, for sure, means that the tested address is not reachable, and we have to try with other addresses.

What kind of error should prevent trying to other addresses here?

IMHO, we can try next address on any failure.

g-bougard · 2025-04-17T13:24:41Z

IMHO, I don't understand why initially we should stop on the first error. Unless an error is effectively telling the agent is definitively not reachable, even on other ips, but how to be sure when we are not still sure we even have all or goods ips ^^
To be honest, I would myself prefer to just remove the break on any error.

cedric-anne · 2025-04-17T13:31:51Z

IMHO, I don't understand why initially we should stop on the first error. Unless an error is effectively telling the agent is definitively not reachable, even on other ips, but how to be sure when we are not still sure we even have all or goods ips ^^ To be honest, I would myself prefer to just remove the break on any error.

It seems that it was your own proposal, see #13812.

Is there something in the response headers/content that can be used to be sure that an agent was reached?

g-bougard · 2025-04-17T14:46:53Z

Lol, 2 years ago.

Reading again my previous PR description, I think I would like to make the function returns earlier only when agent really tells that access is forbidden, i.e when it returns a 403 error code with a "Access denied" text. So I understand better this PR purpose. Here is an example curl request with verbose option:

[g-bougard@somewhere ~]$ curl -v http://192.168.0.99:62354/now
*   Trying 192.168.0.99:62354...
* Connected to 192.168.0.99 (192.168.0.99) port 62354
> GET /now HTTP/1.1
> Host: 192.168.0.99:62354
> User-Agent: curl/8.9.1
> Accept: */*
> 
* Request completely sent off
< HTTP/1.1 403 Access denied (invalid request (untrusted address))
< Date: Thu, 17 Apr 2025 14:43:31 GMT
< Server: libwww-perl-daemon/6.16
< Content-Length: 425
< Content-Type: text/html
< 
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
<html xmlns="http://www.w3.org/1999/xhtml"><head>

<meta content="text/html; charset=UTF-8" http-equiv="content-type" />
<title>GLPI-Agent</title>
<link rel="stylesheet" href="site.css" type="text/css" />

</head>
<body>
<div id="background">
<p>Access denied</p>
<p><a href="/">Back</a></p>
</div>
</body>
</html>
* Connection #0 to host 192.168.0.99 left intact

My guess so is we can not break on 5XX error response and try others.

cedric-anne · 2025-04-17T14:55:16Z

@g-bougard

Would it be possible to return Server: glpi-agent/1.x instead of Server: libwww-perl-daemon/6.16? 1.x would be preferable than 1.6, 1.7, ... to not expose a precise version to a potential attacker.

If this is possible, then we could stop trying requests when are sure that the response is returned by a GLPI Agent.

g-bougard · 2025-04-17T15:18:57Z

Probably, but Server: libwww-perl-daemon/ still means you have a perl process behind, and on port 62354, this most probably GLPI-Agent... or FIA, so what ever version.

If you want to check a header, check for libwww-perl-daemon or glpi-agent in Server. In later version based on another language, we would probably have to set this Server to glpi-agent/2.x or so.

g-bougard · 2025-04-18T15:39:47Z

src/Agent.php

-                // many addresses will be incorrect
+            } catch (\GuzzleHttp\Exception\ClientException $exception) {
+                // If a 4XX error seems to be coming from an agent, we can stop trying. For every other exception, we can try other addresses
+                if (str_contains($exception->getResponse()->getHeaderLine('Server'), 'libwww-perl-daemon')) {


Can you better check if "Server" header contains "libwww-perl-daemon" OR "glpi-agent" ?
Just to anticipate future versions of glpi-agent which won't use perl.

While reading some documentation about the Server header, I figured out that we should probably remove it from the response, to not expose too many information about the server itself. Indeed, it may help attackers to identify vulnerable servers. Also, I guess some security proxies are removing these headers. Relying on this header is probably a bad idea.

cedric-anne · 2025-04-22T07:47:38Z

src/Agent.php

+                // If a 4XX error seems to be coming from an agent, we can stop trying. For every other exception, we can try other addresses
+                if (str_contains($exception->getResponse()->getHeaderLine('Server'), 'libwww-perl-daemon')) {


The initial purpose was to handle 405 responses. IMHO, we should only handle this specific case.
@g-bougard Do you agree ?

Suggested change

// If a 4XX error seems to be coming from an agent, we can stop trying. For every other exception, we can try other addresses

if (str_contains($exception->getResponse()->getHeaderLine('Server'), 'libwww-perl-daemon')) {

// If a "405 Not allowed" error is returned by an agent expected address, we can stop trying. For every other exception, we can try other addresses.

if ($exception->getResponse()->getStatusCode() === 405) {

// "405 Not allowed" status is returned when agent don't trust server

A 405 doesn't seem like the right status code if the agent doesn't trust the server. This implies that the untrusted server could make the same request to the agent using a different method (GET, POST, etc) and the agent is expected to send an Allow header in the response indicating the allowed methods.

A 403 error is what should be used and seems like the only case where we know to stop trying to contact the agent at this time.

cedric-anne · 2025-04-22T07:53:15Z

src/Agent.php

+                // If a 4XX error seems to be coming from an agent, we can stop trying. For every other exception, we can try other addresses
+                if (str_contains($exception->getResponse()->getHeaderLine('Server'), 'libwww-perl-daemon')) {
+                    break;
+                }
            }


We should prevent connection/request/server exceptions to stop the execution.

Suggested change

}

} catch (\GuzzleHttp\Exception\TransferException $exception) {

// Many addresses will be incorrect. Catch the exception and try other addresses.

}

cedric-anne · 2025-04-22T07:57:49Z

src/Agent.php

-                // many addresses will be incorrect
+            } catch (\GuzzleHttp\Exception\ClientException $exception) {
+                // If a 4XX error seems to be coming from an agent, we can stop trying. For every other exception, we can try other addresses
+                if (str_contains($exception->getResponse()->getHeaderLine('Server'), 'libwww-perl-daemon')) {


While reading some documentation about the Server header, I figured out that we should probably remove it from the response, to not expose too many information about the server itself. Indeed, it may help attackers to identify vulnerable servers. Also, I guess some security proxies are removing these headers. Relying on this header is probably a bad idea.

try other agent addresses on 5XX error response

665b3f7

cconard96 requested review from trasher and g-bougard April 17, 2025 13:15

cedric-anne reviewed Apr 17, 2025

View reviewed changes

only stop if a 4xx error seems to come from agent

bb08fd2

g-bougard suggested changes Apr 18, 2025

View reviewed changes

cedric-anne reviewed Apr 22, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

try other agent addresses on 5XX error response #19497

try other agent addresses on 5XX error response #19497

cconard96 commented Apr 17, 2025

cedric-anne Apr 17, 2025

trasher Apr 17, 2025

g-bougard commented Apr 17, 2025

cedric-anne commented Apr 17, 2025

g-bougard commented Apr 17, 2025

cedric-anne commented Apr 17, 2025

g-bougard commented Apr 17, 2025

g-bougard Apr 18, 2025

cedric-anne Apr 22, 2025

cedric-anne Apr 22, 2025

cconard96 Apr 25, 2025

cedric-anne Apr 22, 2025

cedric-anne Apr 22, 2025

		// If a 4XX error seems to be coming from an agent, we can stop trying. For every other exception, we can try other addresses
		if (str_contains($exception->getResponse()->getHeaderLine('Server'), 'libwww-perl-daemon')) {

-                // If a 4XX error seems to be coming from an agent, we can stop trying. For every other exception, we can try other addresses
-                if (str_contains($exception->getResponse()->getHeaderLine('Server'), 'libwww-perl-daemon')) {
+                // If a "405 Not allowed" error is returned by an agent expected address, we can stop trying. For every other exception, we can try other addresses.
+                if ($exception->getResponse()->getStatusCode() === 405) {
+                    // "405 Not allowed" status is returned when agent don't trust server

-            }
+            } catch (\GuzzleHttp\Exception\TransferException $exception) {
+                // Many addresses will be incorrect. Catch the exception and try other addresses.
+            }

try other agent addresses on 5XX error response #19497

Are you sure you want to change the base?

try other agent addresses on 5XX error response #19497

Conversation

cconard96 commented Apr 17, 2025

Checklist before requesting a review

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

g-bougard commented Apr 17, 2025

cedric-anne commented Apr 17, 2025

g-bougard commented Apr 17, 2025

cedric-anne commented Apr 17, 2025

g-bougard commented Apr 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment