Previously After RoundTrip returns successfully (err == nil, res != nil), the code checks if the original client request's context was canceled. If canceled, it returns immediately without closing res.Body.
There is a race window where:
1) RoundTrip completes successfully (res is non-nil)
2) The client cancels the request context (closes connection)
3) The context check at line 484 sees the cancellation
4) The function returns without closing res.Body
The response body holds a reference to the underlying TCP connection. Without closing it, the connection is permanently leaked along with the transport goroutines (readLoop + writeLoop or dialConnFor).
bug was introduced at https://github.com/VictoriaMetrics/VictoriaMetrics/pull/10233
Fixes https://github.com/VictoriaMetrics/VictoriaMetrics/issues/10833
This reverts commit b3c03c023c.
Reason for revert: the original logic was correct from the user's perspective:
- The -maxRequestBodySizeToRetry command-line flag controls the size of the request body,
which could be retried on backend failure. The meaining of this flag wasn't changed after
the introduction of the -requestBufferSize flag in the commit e31abfc25c
(see https://github.com/VictoriaMetrics/VictoriaMetrics/issues/10309 )
- The -requestBufferSize flag controls the size of the buffer for reading request body
before sending sending it to the backend and before applying concurrency limits.
These flags are independent from user's perspective. The fact that these flags share the implementation,
sholdn't be known to the user - this is an implementation detail, which allows avoiding double buffering.
Both flags enable request buffering. If the user wants disabling of all the request buffering,
then both flags must be set to 0. That's why these flags are cross-mentioned in their -help descriptions.
Also the reverted commit had the following issues:
- It reduced the default value for the -requestBufferSize flag from 32KiB to 16KiB.
The 32KiB value has been calculated and justified at https://github.com/VictoriaMetrics/VictoriaMetrics/issues/10309 .
It shouldn't increase vmagent memory usage too much for typical workloads.
For example, if vmagent handles 10K concurrent requests, then the memory overhead for the request buffering
will be 10K*32KiB=320MiB. This is a small price for being able to efficiently handling 10K concurrent requests.
- It added a dot to the end of the https://docs.victoriametrics.com/victoriametrics/vmauth/#request-body-buffering link
in the description for the description of the -requestBufferSize flag. This breaks clicking the link in some environments,
since the trailing dot is considered as a part of the url.
- It added a superflouous whitespace in front of the 'Disabling request buffering' text inside the description
for the -requstBufferSize flag.
- It introduced an unnecessary complexity to the user by mentioning that the zero value
at -maxBufferSize disables buffering for request reties (these things must be independent
from the user's perspective).
- It changed the bufferedBody logic in non-trivial ways, which aren't related to the original issue.
If these changes are needed, then they must be justified in a separate issue and must be prepared
in a separate pull request / commit.
Updates https://github.com/VictoriaMetrics/VictoriaMetrics/issues/10675
Updates https://github.com/VictoriaMetrics/VictoriaMetrics/pull/10677
Follow-up commit for
211fb08028
Address @f41gh7 review comments:
- Move code from `lib/osinfo` to `lib/appmetrics`.
- Make the logic private.
- Use metrics.WriteGaugeUint64 func.
- Remove registration logic from `app/xxx/main.go`.
- Remove `lib/osinfo` package.
Previously introduced flag `requestBufferSize` raised default value for
in-memory buffer from 16KB to 32KB. It could increase memory usage for
vmauth. Also it made unclean how to actually disable requests buffering.
This commit aligns flags value to the 16KB. And disables requests
buffering if any of flags value are 0 as mentioned at flags description.
If any of flags have non-default value, those value are used as max size
for request buffer. If both flags are modified - bigger value wins.
Fixes https://github.com/VictoriaMetrics/VictoriaMetrics/issues/10675
Previously there was a data-race, when targetURL was concurrently
updated in case of default url route.
This commit fixes data-race and adds concurrency to the routing tests.
Related PR https://github.com/VictoriaMetrics/VictoriaMetrics/pull/10626
Add support for [OpenID Connect
Discovery](https://openid.net/specs/openid-connect-discovery-1_0.html#IANA)
as an alternative way to obtain verification keys and rotate them
automatically.
`jwt` configuration should allow **exactly one** of the following
verification modes: `public_keys`, `oidc`, `skip_verify`. These options
must be mutually exclusive.
Example: OIDC configuration
```yaml
users:
- jwt:
oidc:
issuer: http://identity-provider.com
```
When `oidc` is enabled:
1. On startup, `vmauth` fetches:
```
{issuer}/.well-known/openid-configuration
```
2. Extracts `jwks_uri`.
3. Fetches [JWK
keys](https://openid.net/specs/draft-jones-json-web-key-03.html#ExampleJWK)
from `jwks_uri`.
4. Uses discovered keys to verify JWT tokens.
Related to
https://github.com/VictoriaMetrics/VictoriaMetrics/issues/10585
Failure handling:
* If discovery fails at startup:
* No keys are available.
* The user is skipped.
* Discovery runs periodically in background (e.g., every 1 minute).
* If keys become available later, authentication should start working
automatically.
* If keys were previously fetched and the identity provider becomes
unavailable:
* Cached keys must be preserved.
* Authentication continues using cached keys.
#### JWT Requirements in OIDC Mode
When `oidc` is enabled:
* `iss` claim becomes
[mandatory](https://openid.net/specs/openid-connect-core-1_0.html#IDToken).
* `iss` [must
match](https://openid.net/specs/openid-connect-core-1_0.html#RotateEncKeys):
* `oidc.issuer` from config.
* `issuer` returned in the OpenID configuration document.
* JWT header must contain `kid`.
* `kid` must be used to select the appropriate key from JWKS.
* Tokens without `kid` must be rejected.
* Tokens without `iss` must be rejected.
Rationale
* Enables automatic key rotation.
* Eliminates manual public key configuration.
* Maintains compatibility with standard OIDC providers.
---------
Signed-off-by: Max Kotliar <kotlyar.maksim@gmail.com>
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
Add new option per-user to print access logs. Such logs
contain limited amount of information to prevent exposing
sensitive data.
Access logs can be enabled/disabled via hot-reload and could
help locating clients that incorrectly use or abuse vmauth.
See https://github.com/VictoriaMetrics/VictoriaMetrics/issues/5936
This change adds some context to the error when all backend failed. From
support cases it seems like without the context users might not know
what to do with this error message. Clarification advises them to check
the prev error messages.
### Describe Your Changes
Adds JWT authentication support to vmauth with signature verification
and tenant-based access control. For now, public_keys have to set
explisitly in the config, OIDC discovery will be added in upcoming PRs.
Related to
https://github.com/VictoriaMetrics/VictoriaMetrics/issues/10445
Key Features
- JWT Configuration: Added `jwt_token` field to user config supporting
RSA/ECDSA public keys or skip_verify mode (for testing purposes).
- Token Validation: Verifies JWT signatures, checks expiration, and
extracts vm_access claims
- Compatible with vmgateway: jwt tokens issued for vmgateway should work
with vmauth too.
Examples
```yaml
users:
- jwt_token:
public_keys:
- |
-----BEGIN PUBLIC KEY-----
MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEA...
-----END PUBLIC KEY-----
url_prefix: "http://victoria-metrics:8428/"
```
```yaml
users:
- jwt_token:
skip_verify: true
url_prefix: "http://victoria-metrics:8428/"
```
Constraints
- JWT tokens cannot be mixed with other auth methods (bearer_token,
username, password)
- Requires at least one public key OR skip_verify=true
- Limited to single JWT user (multiple JWT users will be supported in
the future)
Next steps
- Multiple `jwt_token` support.
- Claim matching
- Claim based routing
- OIDC\JWKS support
### Checklist
The following checks are **mandatory**:
- [ ] My change adheres to [VictoriaMetrics contributing
guidelines](https://docs.victoriametrics.com/victoriametrics/contributing/#pull-request-checklist).
- [ ] My change adheres to [VictoriaMetrics development
goals](https://docs.victoriametrics.com/victoriametrics/goals/).
---------
Co-authored-by: Pablo (Tomas) Fernandez <46322567+TomFern@users.noreply.github.com>
Without measuring this, we have a blind spot. Exposing it as a metric
improves visibility and should save time during future debugging
sessions.
Inspired by review commit
c9596a0364 (r173621968)
The `err` may contain information about request cancelation performed by the server code.
In such cases the error must be logged. The error must be ignored only if the client canceled the request.
This is a follow-up for the commit c9596a0364
Updates https://github.com/VictoriaMetrics/VictoriaMetrics/issues/10078
Previously a short spike in the number of concurrent requests immediately led to `429 Too Many Requests` errors
when the number of concurrent requests exceeds -maxConcurrentRequests or -maxConcurrentPerUserRequests.
This commit allows processing short spikes in the number of concurrent requests during the -maxQueueDuration timeout.
The requests are rejected only if they couldn't be served accroding to the concurrency limits during the -maxQueueDuration.
See https://github.com/VictoriaMetrics/VictoriaMetrics/issues/10078
Updates https://github.com/VictoriaMetrics/VictoriaMetrics/pull/10112
follow up https://github.com/VictoriaMetrics/VictoriaMetrics/pull/10177
Add `vmauth_user_request_backend_requests_total` and
`vmauth_unauthorized_user_request_backend_requests_total` which track
the number of user request errors, and aligned with
`vmauth_user_requests_total`.
The existing `vmauth_http_request_errors_total` currently only counts
requests with `invalid_auth_token`. Once authorization has passed, any
subsequent request errors are tracked under
`xxx_user_request_backend_requests_total`.
We have `vmauth_user_requests_total` and
`vmauth_unauthorized_user_requests_total` to track requests from the
user side. However, in scenarios such as request timeouts or when the
response code matches `retry_status_code`, a single request may be
retried across multiple backends.
Exposing counters `vmauth_user_request_backend_requests_total` and
`vmauth_unauthorized_user_request_backend_requests_total` that track the
number of requests sent to backends provides insight into the routing
logic and can help identify if requests are being consistently retried,
which may contribute to increased request duration.
Related PR https://github.com/VictoriaMetrics/VictoriaMetrics/pull/10171
Currently, backendErrors may be counted twice if a request to the
backend fails due to context.DeadlineExceeded.
9bc7a17d80/app/vmauth/main.go (L328)9bc7a17d80/app/vmauth/main.go (L294)
And we increment this counter in a way that is somewhat inconsistent.
Given that the counter's name is `xx_request_backend_errors_total`, it
should only increase when a backend request returns an error. This value
can exceed the user request error count if multiple backend requests
fail for a single user request.
The `xxx_request_backend_errors_total` counter should be used in
conjunction with the `xxx_request_backend_requests_total` introduced in
https://github.com/VictoriaMetrics/VictoriaMetrics/pull/10171.
Previously a misleading random error could be logged for canceled and/or timed out requests to vmauth.
Consistently log the request timeout error for timed out requests.
While at it, do not log errors for requests canceled by the remote client, since such logs aren't actionable
and just pollute error logs generated by vmauth.
- Rename copyStream to copyStreamToClient in order to make it more clear
that the stream must be copied from backend to client.
- Make sure that the client implements net/http.Flusher interface.
It is a programming error (BUG) if the client passed to copyStreamToClient
doesn't implement net/http.Flusher interface.
- Do not write zero-length data to the backend.
Updates https://github.com/VictoriaMetrics/VictoriaLogs/issues/667
This is needed for VictoriaLogs, which allows limiting query results with the given set of extra filters
specified via extra_filters query arg. The request url can contain multiple extra_filters query args -
they are all applied with AND logic to the query. See https://docs.victoriametrics.com/victorialogs/querying/#extra-filters
The merge_query_args option at vmauth allows merging the extra_filters provided by the client
(such as Grafana plugin for VictoriaLogs or built-in web UI) with the extra_filters specified in the backend
url at vmauth config.
This is needed for https://github.com/VictoriaMetrics/VictoriaLogs/issues/106
This functionality was broken in
0e313e5355
Was caught by integration tests:
```
--- FAIL: TestSingleVMAuthRouterWithInternalAddr (5.00s)
vmauth_routing_test.go:148: Could not start vmauth: could not extract some or all regexps from stderr: ["pprof handlers are exposed at http://(.*:\\d{1,5})/debug/pprof/"]
```
Signed-off-by: hagen1778 <roman@victoriametrics.com>
Currently, requests failing due to network timeout would receive "200
OK" while producing a warning log message about the timeout. This
behaviour is confusing and might produce unexpected issues as it is not
possible to retry errors properly.
Change this to return "502 Bad Gateway" response so that error can be
handled by the client.
See: https://github.com/VictoriaMetrics/VictoriaMetrics/issues/8621
Config for testing:
```
unauthorized_user:
url_prefix: "http://example.com:9800"
```
Before the change:
```
* Trying 127.0.0.1:8427...
* Connected to 127.0.0.1 (127.0.0.1) port 8427
* using HTTP/1.x
> HEAD /api/v1/query HTTP/1.1
> Host: 127.0.0.1:8427
> User-Agent: curl/8.12.1
> Accept: */*
>
* Request completely sent off
/* NOTE: 30 seconds timeout passes */
< HTTP/1.1 200 OK
HTTP/1.1 200 OK
< Vary: Accept-Encoding
Vary: Accept-Encoding
< X-Server-Hostname: pc
X-Server-Hostname: pc
< Date: Tue, 01 Apr 2025 08:54:05 GMT
Date: Tue, 01 Apr 2025 08:54:05 GMT
<
* Connection #0 to host 127.0.0.1 left intact
```
After:
```
* Trying 127.0.0.1:8427...
* Connected to 127.0.0.1 (127.0.0.1) port 8427
* using HTTP/1.x
> HEAD /api/v1/query HTTP/1.1
> Host: 127.0.0.1:8427
> User-Agent: curl/8.12.1
> Accept: */*
>
* Request completely sent off
< HTTP/1.1 502 Bad Gateway
HTTP/1.1 502 Bad Gateway
< Content-Type: text/plain; charset=utf-8
Content-Type: text/plain; charset=utf-8
< Vary: Accept-Encoding
Vary: Accept-Encoding
< X-Content-Type-Options: nosniff
X-Content-Type-Options: nosniff
< X-Server-Hostname: pc
X-Server-Hostname: pc
< Date: Tue, 01 Apr 2025 09:13:57 GMT
Date: Tue, 01 Apr 2025 09:13:57 GMT
< Content-Length: 109
Content-Length: 109
<
* Connection #0 to host 127.0.0.1 left intact
```
Signed-off-by: Zakhar Bessarab <z.bessarab@victoriametrics.com>
vmauth uses 'lib/httpserver' for serving HTTP requests. This server
unconditionally defines built-in routes (such as '/metrics',
'/health', etc). It makes impossible to proxy `HTTP` requests to backends with the same routes.
Since vmauth's httpserver matches built-in route and return local
response.
This commit adds new flag `httpInternalListenAddr` with
default empty value. Which removes internal API routes from public
router and exposes it at separate http server.
For example given configuration disables private routes at `0.0.0.0:8427` address and serves it at `0.0.0.0:8426`:
`./bin/vmauth --auth.config=config.yaml --httpListenAddr=:8427 --httpInternalListenAddr=127.0.0.1:8426`
Related issues:
- https://github.com/VictoriaMetrics/VictoriaMetrics/issues/6468
- https://github.com/VictoriaMetrics/VictoriaMetrics/issues/7345
Sync.Pool for readTrackingBody was added in order to reduce potential
load on garbage collector. But golang net/http standard library does not
allow to reuse request body, since it closes body asynchronously after
return. Related issue: https://github.com/golang/go/issues/51907
This commit removes sync.Pool in order to fix potential panic and data
race at requests processing.
Affected releases:
- all releases after v1.97.7
Related issue:
https://github.com/VictoriaMetrics/VictoriaMetrics/issues/8051
Signed-off-by: f41gh7 <nik@victoriametrics.com>
Co-authored-by: Roman Khavronenko <roman@victoriametrics.com>
vmauth started to use request.Host after commit
f4776fec1b for`src_hosts` routing rules.
This commit adds http.Request.Host to the debugInfo output in order to
be consistent with routing logic.
### Describe Your Changes
Please provide a brief description of the changes you made. Be as
specific as possible to help others understand the purpose and impact of
your modifications.
### Checklist
The following checks are **mandatory**:
- [ ] My change adheres [VictoriaMetrics contributing
guidelines](https://docs.victoriametrics.com/contributing/).
---------
Signed-off-by: f41gh7 <nik@victoriametrics.com>
Requests processed by built-in HTTP server has the [origin
form](https://datatracker.ietf.org/doc/html/rfc7230#section-5.3) rather
than the absolute form.
So in[Request.URL](https://pkg.go.dev/net/http#Request), fields other than
Path and RawQuery will be empty.
> // For server requests, the URL is parsed from the URI
> // supplied on the Request-Line as stored in RequestURI. For
> // most requests, fields other than Path and RawQuery will be
> // empty. (See RFC 7230, Section 5.3)
Using `request.Host` field instead to match `src_hosts` fixes issue and allows to route requests properly.
An addition It allows user to route requests with customized `Host` header.
Previously, there was no option to replace value of `X-Forwarded-For`
HTTP Header. It was only possible to completely remove it. It's not good
solution, since backend may require this information. But using direct
value of this header is insecure. And requires complex knowledge of
infrastruce at backend side (see spoofing X-Forwarded-For articles).
This commit adds new flag, that replaces content of `X-Forwarded-For`
HTTP Header value with current `RemoteAddress` of client that send
request.
It should be used if `vmauth` is directly attached to the internet.
Related issue:
https://github.com/VictoriaMetrics/VictoriaMetrics/issues/6883
---------
Signed-off-by: f41gh7 <nik@victoriametrics.com>
Additional info from the dump can be used to debug rotuing rules.
https://pkg.go.dev/net/http/httputil#DumpRequest
### Describe Your Changes
Please provide a brief description of the changes you made. Be as
specific as possible to help others understand the purpose and impact of
your modifications.
### Checklist
The following checks are **mandatory**:
- [ ] My change adheres [VictoriaMetrics contributing
guidelines](https://docs.victoriametrics.com/contributing/).
---------
Signed-off-by: hagen1778 <roman@victoriametrics.com>
This commit makes vmauth respect the routing config for unauthorized
requests for requests that despite having Authorization header failed to
authorize successfully.
It covers the following use-cases:
- vmauth is used at load-balanacer and must forward requests as is. There is no any authorization configs.
- vmauth has authorization config, but it must forward requests with invalid credential tokens to some other backend.
related issue:
https://github.com/VictoriaMetrics/VictoriaMetrics/issues/7543
---------
Signed-off-by: Andrii <andriibeee@gmail.com>
### Describe Your Changes
Change response code to 502 to align it with behaviour of other existing
reverse proxies. Currently, the following reverse proxies will return
502 in case an upstream is not available: nginx, traefik, caddy, apache.
Signed-off-by: Zakhar Bessarab <z.bessarab@victoriametrics.com>
