Skip to content

Integrated code lifecycle: Restrict build agent repository access to their current build jobs - #13515

Open
krusche wants to merge 45 commits into
developfrom
feature/build-agent-access-hardening
Open

Integrated code lifecycle: Restrict build agent repository access to their current build jobs#13515
krusche wants to merge 45 commits into
developfrom
feature/build-agent-access-hardening

Conversation

@krusche

@krusche krusche commented Aug 17, 2026

Copy link
Copy Markdown
Member

Summary

Until now, once a build agent had authenticated it was authorised to read every repository in the installation, from any address, for as long as its credential or key lived. This narrows that on three axes: a clone is only served to a known agent, only from an address that agent is currently connected from, and only for the repositories of a build job it is actually running.

Build agents no longer need a shared secret at all. On a node with local CI you can now leave build-agent-git-username and build-agent-git-password empty — they remain only for Jenkins with LocalVC, which is not an Artemis build agent.

Checklist

General

Server

  • Important: I implemented the changes with a very good performance and prevented too many (unnecessary) and too complex database calls.
  • I strictly followed the principle of data economy for all database calls.
  • I strictly followed the server coding and design guidelines and the REST API guidelines.
  • I added multiple integration tests (Spring) related to the features (with a high test coverage).
  • I added pre-authorization annotations according to the guidelines and checked the course groups for all new REST Calls (security).
  • I documented the Java code using JavaDoc style.

Client

  • I strictly followed the principle of data economy for all client-server REST calls.
  • I strictly followed the client coding guidelines.
  • I added multiple integration tests (Vitest) related to the features (with a high test coverage), while following the test guidelines.
  • I documented the TypeScript code using JSDoc style.
  • I translated all newly inserted strings into English and German.

Changes affecting Programming Exercises

  • High priority: I tested all changes and their related features with all corresponding user types on a test server configured with the integrated lifecycle setup (LocalVC and LocalCI).
  • I tested all changes and their related features with all corresponding user types on a test server configured with LocalVC and Jenkins.

Motivation and Context

#13503 made build-agent-use-ssh govern both sides and deprecated HTTPS basic authentication for build agents. It did not change what a build agent may do after it authenticates, which is everything:

Mechanism Entry point What was skipped
HTTPS basic LocalVCServletService.authenticateAndAuthorizeGitRequest rate limit, repository authorization, VCS access log
SSH key GitPublickeyAuthenticatorService.authenticateBuildAgentSshGitLocationResolverService repository authorization, VCS access log

Neither looked at where the caller was, nor at which repositories that caller legitimately needed. The most privileged reader in the installation was also the only one that left no audit trail.

Description

Origin — build agents register where they connect from. Core nodes observe the address on the agent's own cluster connection rather than trusting what the agent reports: the existing BuildAgentDTO.memberAddress is the agent's view of its local socket, so it is pre-NAT and a hostile agent can set it to anything. The observation is stored through the generic DistributedDataProvider adapter (new getConnectedClientAddresses(), implemented for Hazelcast, Redis and local), so any core node can authorize a clone from a local snapshot with no provider-specific call on the hot path.

Optionally bound which hosts may act as an agent at all:

artemis:
    continuous-integration:
        build-agent-network:
            allowed-ranges: ["10.0.0.0/8"]   # empty = no restriction
            trusted-proxies: ["10.0.0.1"]    # whose X-Forwarded-For may be believed

Scope and lifetime — a credential per build job. A build job carries a clone token minted when it is queued. Over HTTPS an agent authenticates with its own short name and that token, and the core node accepts it only when the requested repository is one the job declares and the job is still in the distributed processing list. There is deliberately no expiry: the build timeout already removes a job from that list, so a clock would only duplicate a bound the system already enforces. Over SSH the public key already identifies the agent, so no token is needed — the same processing list scopes what it may read.

Both paths now write a VCS access log entry naming the agent and the job.

PROXY protocol on the SSH listener. A load balancer that forwards port 7921 at the TCP level hides the client, which would make the origin check meaningless and put every SSH user into one rate limit bucket. Implemented through Apache MINA SSHD's ServerProxyAcceptor hook. Acceptance is keyed on the connection's source address, never on whether a header is present — a PROXY header is unauthenticated plain text, so believing one from any sender would let anyone reaching the port claim an arbitrary client address.

The Ansible counterpart is ls1intum/artemis-ansible-collection#237.

Containing the token. It travels inside BuildJobQueueItem so the claiming agent receives it, and that record is returned straight to instructors from BuildJobQueueResource, sent over STOMP, and logged whole at INFO by SharedQueueProcessingService. Four separate mechanisms close those exits, each with its own test, because none is visible from where a future change would break it:

Exit Closed by
REST + websocket payloads @JsonIgnore on the record component
Log output overridden toString()@JsonIgnore does nothing here
Websocket payloads (belt and braces) cleared in the existing removeUnnecessaryInformation
Database BuildJob copies named fields only

A fifth test asserts the opposite direction: the token must survive Java serialization, or the agent never receives it and every clone silently falls back to the deprecated shared credential.

Startup validation. LocalVCBuildAgentCredentialsValidator no longer refuses to start on blank credentials where local CI runs — clone tokens always work there, so that is now the configuration worth aiming for. Without local CI there are no build jobs, so the pair stays required.

Notes for reviewers

Three deliberate choices where the safe-looking option would have been wrong:

  1. An empty allowed-ranges means no restriction, not deny-all. The property is absent in every installation that upgrades.
  2. A middleware that cannot report client addresses disables only the address binding, logged loudly at startup. Refusing every build because the cluster cannot answer a question is a worse failure than not asking it. The allowlist and job scoping still apply.
  3. Every failure in the new HTTPS branch falls through to normal user authentication rather than rejecting. The username is an agent short name, which could collide with a real login, and that person must still be able to use their own credentials — the same reasoning that resolved the review on Integrated code lifecycle: Stop offering build agent password authentication when build agents use SSH #13503.

The agent short name is an identifier, not a credential: it is the Hazelcast client name, the key of the build agent information map, and it is shown in the admin UI. It selects whose jobs and whose addresses to check; the token is what authenticates.

vcs_access_log.user_id becomes nullable so a build agent clone can be recorded, since an agent authenticates as a build job rather than as a person.

Fixed after the first review round

develop has been merged in. #13503 is now merged, so its files have dropped out of this diff and the whole change here is the follow-up work. Seven issues were found while going over the merged result and exercising it on a real cluster, and all are fixed in this PR:

Issue Why it mattered
The stale-job cross-check removed a running job from the distributed processing map It has no grace period, so it fires in the window between claiming a job and registering its future. That used to only skew the running-job counts; now that this map authorizes a build agent's clone, the job's next clone returned 401 and the build ended at 0%. Reproduced as an "unrelated" flaky E2E test, with 19 rejections in one run
The clone token was written to the log in full LoggingAspect prints every argument and return value of a @Service, so generateCloneToken and tokenMatches leaked a live token under the development profile — the exact exit the masked toString() closes, reached another way
A person's access log amendment could land on a build agent's row Both "amend the newest entry" lookups take the newest row for the participation. An agent clone in between took the commit hash meant for the student's push
A blank entry in an address range list parsed as loopback A trailing comma in an environment variable was enough to quietly trust 127.0.0.1 as a proxy or PROXY-protocol source
Every human git fetch cost a distributed map read The clone-token check runs ahead of everything for any request with a Basic header. It now rejects a non-token credential on a local prefix comparison first
The clone-token rate limit default was below real agent traffic 300/min, against roughly 400/min from one agent at eight concurrent jobs — and the key is the source address, which agents behind one NAT share. Exceeding it fails the clone rather than slowing it. Raised to 1500
The audit table rendered undefined next to a build agent's name A build agent entry has no email, and the author cell concatenated the two unconditionally

Two smaller corrections: the startup bean threshold is 147, not 143 (the check would have failed), and the shared distributed-data contract suite now covers getConnectedClientAddresses, which CLAUDE.md requires of a new provider capability. Each fix has a regression test that was confirmed to fail without it.

Second review round

Four changes from review feedback, each verified on a running cluster:

The shared credential is refused where local CI runs, not deprecated. A local CI node no longer starts with build-agent-git-username or build-agent-git-password set. It needs neither: every build job carries a token covering its own assignment, test, solution and auxiliary repositories. The shipped configurations no longer set them, and the HTTPS shortcut is now unreachable where local CI runs rather than merely unconfigured, so a credential arriving by any other route still opens nothing. A local VC node without local CI — Jenkins with LocalVC — is unchanged.

The clone-token rate limit drops from 1500 to 30 per minute. 1500 was sized for build throughput, which is the wrong quantity for a rate limit: four repositories at two requests each, eight concurrent jobs on five second builds is already ~770 requests per minute from one agent, and any number chosen that way tracks how fast builds are rather than how fast someone can guess. Two changes decouple them, so 30 is the same order as the other credential checks:

  • An address a build agent is currently registered at is exempt automatically, as it connects. That is the dynamic form of artemis.rate-limiting.exempt-addresses, without a list anyone has to maintain.
  • Only a check that declines spends budget. An agent whose checks succeed never approaches the limit however many repositories it clones — which is what covers an agent with no registration to be exempt by.

Parsing the repository path also moved ahead of the distributed scan: the catch that ends the check returns without spending, so anything that could throw after the scan was a way to run it for free.

The origin binding is bound to Hazelcast, and Redis is why. It compares where the middleware observed an agent with where its clone arrives, and those are one path only when the agent's middleware connection terminates on a core node. Hazelcast clients connect to the cluster members, which are exactly those nodes. Redis is a separate service: with Redis in a container and the nodes on the host, one side sees the docker bridge gateway and the other loopback.

This was found by running it. An earlier attempt mapped Redis client names to agent short names so the binding would engage — and a multi-node Redis run then produced 11 failed, 3 passed, 279 refusals and no successful build. Providers now state which shape they are, and the binding is skipped where the observation cannot speak for the git path, exactly as it already was for an origin that cannot be observed at all. allowed-ranges and the per-build-job scoping are unaffected on every backend, since both are checked against the address of the request itself.

The ssh origin check had no test. Only the repository scoping did. SshBuildAgentOriginTest covers refusing a key from an unobserved address, refusing outside the allowlist, refusing when no address can be determined, and accepting an agent that passes — confirmed to fail when the check is removed.

Verified

The ProgrammingExerciseParticipation suite, on three topologies, with no shared credential configured anywhere:

Stack Result
Single node, LocalVC + LocalCI 15/15, no rejections. The agent shares the JVM with the core node, opens no client connection, and is correctly left unbound
Three nodes, Hazelcast, nginx LB 15/15, no rejections. artemis-build-agent-3 is a build-agent-only node, so it connects as a Hazelcast client and is bound — registered at 127.0.0.1, clones authorized against it and audited
Three nodes, Redis 15/15, no rejections, 92 audited BUILD_JOB_TOKEN clones across both agents. The binding is skipped and said so at startup

Also checked directly: a local CI node with a credential configured fails to start, naming the property; build agents used HTTPS with job tokens in every run (build-agent-use-ssh is unset, so false); the audit rows all carry a null user_id alongside the students' own untouched entries; and the admin UI shows Connects from for the bound agent and omits it for the co-located one.

Steps for Testing

Prerequisites:

  • 1 Instructor, 1 Student
  • A local integrated code lifecycle setup (LocalVC + LocalCI), 1 Programming Exercise
  1. With default configuration, participate as a student and push a commit. Confirm the build runs — the agent now clones with the build job's token.
  2. Open Server Administration → Build Agents, pick the agent, and confirm the Connects from row shows the address it is connected from.
  3. Confirm the clone is now audited: open the participation's VCS access log and check for an entry naming the build agent and the build job, with mechanism BUILD_JOB_TOKEN.
  4. Confirm the shared credential is no longer needed: clear build-agent-git-username and build-agent-git-password on the core node and the agent, restart both, push again, and confirm the build still runs. The startup log states which mechanisms the node accepts.
  5. Confirm the origin check bites: set artemis.continuous-integration.build-agent-network.allowed-ranges to a range that excludes the agent, restart, push, and confirm the clone is refused with a message naming the property.
  6. Confirm SSH scoping: with build-agent-use-ssh: true, try cloning a repository the agent has no build job for using the agent's key. It must be refused, while builds keep working.
  7. Confirm nothing regressed for humans: clone and push as the student over both HTTPS and SSH.

Testserver States

You can manage test servers using Helios. Check environment statuses in the environment list. To deploy to a test server, go to the CI/CD page, find your PR or branch, and trigger the deployment.

Review Progress

Code Review

  • Code Review 1
  • Code Review 2

Manual Tests

  • Test 1
  • Test 2

Test Coverage

Note: Some tests in the Test job did not pass (failure). Coverage below may be partial.

Client

Class/File Line Coverage Lines Expects Ratio
build-agent-details.component.ts 96.17% 378 63 16.7
build-agents.service.ts 95.00% 64 23 35.9
build-agent-information.model.ts not found (modified) 40 ? ?

Server

Class/File Line Coverage Lines
RateLimitConfigurationService.java not found (modified) 70
RateLimitService.java not found (modified) 93
AdminBuildJobQueueResource.java not found (modified) 200
BuildAgentAddressInfo.java not found (modified) 14
BuildJobQueueItem.java not found (modified) 60
BuildJobExecutionService.java not found (modified) 436
BuildJobGitService.java not found (modified) 248
SharedQueueProcessingService.java not found (modified) 626
BuildAgentGitPasswordValidator.java not found (modified) 63
BuildAgentNetworkConfiguration.java not found (modified) 22
BuildAgentNetworkPolicy.java not found (modified) 56
LoggingAspect.java not found (modified) 73
RateLimitingProperties.java not found (modified) 70
SshProxyProtocolConfiguration.java not found (modified) 15
RateLimitType.java not found (modified) 16
DistributedDataProvider.java not found (modified) 37
HazelcastDistributedDataProviderService.java not found (modified) 296
LocalDataProviderService.java not found (modified) 124
RedisClientListResolver.java not found (modified) 111
RedissonDistributedDataProviderService.java not found (modified) 233
HttpRequestUtils.java not found (modified) 137
IpAddresses.java not found (modified) 31
IpRangeSet.java not found (modified) 104
BuildAgentAddressRegistryService.java not found (modified) 278
BuildJobCloneTokenService.java not found (modified) 72
DistributedDataAccessService.java not found (modified) 228
LocalCIQueueWebsocketService.java not found (modified) 85
LocalCITriggerService.java not found (modified) 313
LocalVCBuildAgentCredentialsValidator.java not found (modified) 57
GitPublickeyAuthenticatorService.java not found (modified) 170
LocalVCServletService.java not found (modified) 943
SshGitLocationResolverService.java not found (modified) 112
VcsAccessLogService.java not found (modified) 85
ProxyProtocolAcceptor.java not found (modified) 189
SshConfiguration.java not found (modified) 66
SshConstants.java not found (modified) 17
AuthenticationMechanism.java not found (modified) 12
VcsAccessLogDTO.java not found (modified) 13
VcsAccessLogRepository.java not found (modified) 50

Last updated: 2026-08-25 16:17:24 UTC

krusche added 11 commits August 13, 2026 11:45
… when agents use ssh

Build agents authenticate either with the key pair they generate at
startup or with the shared build-agent git username and password, never
both. The core nodes accepted the credential pair regardless, so an
installation on ssh keys kept a repository-wide read shortcut open that
no build agent used, ahead of the rate limit, the authorization checks
and the VCS access log.

Make artemis.version-control.build-agent-use-ssh govern both sides: a
core node with ssh enabled rejects the credential pair, and both
credentials become optional so an ssh installation configures none. The
build agent rejects a missing credential at startup for the https case,
and the production password validation is skipped where the credential
is no longer accepted.
…enticate no build agent

Both build-agent git credentials became optional so that an ssh
installation need not configure a credential it never uses. Nothing then
kept a node with ssh disabled and no credential pair from starting, and
LocalVCServletService is lazy, so the state first surfaced as an
authentication failure in a build log.

Validate the pair eagerly on local VC nodes instead, and move the line
that records the accepted mechanism there, where it is logged at startup
rather than on the first git request.
…re node

The property was documented as installation-wide and symmetric, so that
a mismatch between core nodes and agents would fail every clone in
either direction. Only one direction is true. GitPublickeyAuthenticator
Service authenticates a registered build agent by its public key without
reading the property, so an agent on ssh clones successfully from a core
node still configured for https.

Leave that behaviour alone, because a key is per-agent and only reaches
a core node through an agent that joined the cluster, so there is no
shared secret to withdraw, and it lets agents move to ssh one at a time
before the core nodes follow. Say so instead, and give the rollout order
that this asymmetry implies.
The wording said a core node with ssh rejects the build-agent git
username and password. What it stops honouring is the build-agent
shortcut, which returns ahead of the rate limit, the authorization
checks and the access log and grants read to every repository. The pair
is still processed as ordinary Basic credentials afterwards, so an
operator who pointed the username at a real account would find that
account still authenticating, as itself and with its own access.

Say that instead, so the documentation matches the behaviour rather than
overstating it.
Scope the SSH-mode wording to the shortcut it actually closes. The property
stops the build-agent credential pair from granting read access to every
repository; the pair still goes through ordinary Basic authentication
afterwards, where it opens only what the named account may access. The docs,
the BuildAgentGitPasswordValidator javadoc and its log line claimed the pair
was rejected everywhere, which misleads operators about the remaining path.

Also mark HTTPS authentication for build agents as deprecated in the security
and multi-instance documentation, in both YAML config files, and in the startup
log lines that report the selected mechanism. SSH keys are per agent, generated
rather than configured, and never leave the agent, while the credential pair is
one static secret that every node holds and that opens every repository ahead
of the rate limit, the authorization checks and the access log. The default
stays false until the SSH properties have usable defaults and Jenkins LocalVC
no longer needs the pair.
…d job

An authenticated build agent could read every repository in the installation,
from any address, for as long as the shared credential or its key lived. This
narrows that on three axes.

Origin: build agents now register the address they connect to the cluster from.
The address is observed by the core nodes on the agent's own cluster connection
rather than reported by the agent, because the self-reported member address is
pre-NAT and forgeable. It is stored through the generic distributed adapter, so
any core node can authorize a git request from a local snapshot without a
provider-specific call on the hot path. A provider that cannot observe client
connections is treated as "unknown" rather than "nothing connected", so a
deployment whose middleware cannot answer is not left unable to build.

An optional allowlist of build agent networks bounds which hosts may act as an
agent at all. It is a config property rather than an admin setting, since it is
what prevents a rogue agent from registering. An empty list means no
restriction, so an upgrade does not stop existing agents.

Scope and lifetime: a build job now carries a clone token, minted when the job
is queued. Over https an agent authenticates with its own short name and that
token, which opens only the repositories the job declares and only while the
job sits in the distributed processing list. Nothing is time based; the build
timeout already removes a job from that list. Over ssh the key already
identifies the agent, so no token is needed and the same processing list scopes
which repositories it may read. Both paths now write a VCS access log entry,
which the old shortcut skipped entirely.

The token travels inside BuildJobQueueItem so the claiming agent receives it.
Four separate mechanisms keep it from going anywhere else, each with its own
test: @JsonIgnore for the REST and websocket payloads, since
BuildJobQueueResource returns that record straight to instructors and admins; a
redacting toString, because build jobs are logged whole at info level; clearing
it in the existing websocket stripping helper; and BuildJob copying named
fields only, so it never reaches the database.

The ssh listener learns PROXY protocol, wired through the ServerProxyAcceptor
hook Apache MINA SSHD provides. Without it a load balancer that forwards the
port at the TCP level hides the client, which would make the origin check
meaningless and share one rate limit bucket across all ssh users. A header is
required from configured proxy addresses and never believed from anyone else,
so direct connections are unaffected and a header cannot be forged. The shipped
nginx configuration enables it.

LocalVCBuildAgentCredentialsValidator no longer fails startup on blank
credentials where local CI runs: clone tokens always work there, so the state
worth aiming for is one with no shared secret at all. Without local CI there
are no build jobs, so the pair stays required. It remains the only mechanism
for Jenkins with LocalVC, which is not an Artemis build agent.

vcs_access_log.user_id becomes nullable so a build agent clone can be recorded;
the agent and build job identify the access instead.
…rdening

Rewrite the Build Agent Authentication section around the three mechanisms that
now exist, what each one is scoped to, and the fact that the shared credential
pair can be cleared entirely unless Jenkins clones from the installation. Add
the two new configuration groups, the NAT and degraded-middleware caveats, and
the nginx PROXY protocol setup with the warning that the two halves of it have
to change together.

Show the addresses an agent is registered to connect from on the build agent
details page, flagged when they fall outside the configured networks. Served by
a separate admin endpoint rather than added to BuildAgentInformation, which is
shared with the agent nodes and would need a distributed data migration.
… agent access logs

BuildJobGitServiceTest asserted the fail-fast on missing credentials that the
clone token replaces, so it now asserts the agent starts instead, and gains
coverage of which credential a clone actually presents: the current job's token
where one is bound, the deprecated shared pair otherwise, and nothing left
behind for the next job on a reused executor thread.

Add an integration test for a VCS access log entry without a user, which is the
only thing that exercises the nullable user_id and the DTO that used to
dereference it unconditionally.
…trust

A PROXY protocol header carries no signature and no secret, so nothing about it
proves it is genuine. The trust comes only from which source addresses may send
one, which means anything able to connect from a listed address can name an
arbitrary client and bypass the build agent origin check.

The documentation explained why acceptance is keyed on the source address but
never said to keep that list narrow, which is the part an operator has to act
on. Say it, and recommend restricting port 7921 at the firewall as the way to
remove the residual risk.

The compose env comments claimed nobody could forge a client address; the
accurate statement is that nobody outside those ranges can, and the ranges are
deliberately wide there because compose assigns addresses dynamically. Note
that this is acceptable only because a test stack is not a trust boundary.
@krusche
krusche requested a review from a team as a code owner August 17, 2026 07:09
Copilot AI lite review requested due to automatic review settings August 17, 2026 07:09
@krusche
krusche requested review from a team as code owners August 17, 2026 07:09
@github-project-automation github-project-automation Bot moved this to Work In Progress in Artemis Development Aug 17, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions github-actions Bot added tests server Pull requests that update Java code. (Added Automatically!) client Pull requests that update TypeScript code. (Added Automatically!) documentation database Pull requests that update the database. (Added Automatically!). Require a CRITICAL deployment. config-change Pull requests that change the config in a way that they require a deployment via Ansible. docker playwright buildagent Pull requests that affect the corresponding module core Pull requests that affect the corresponding module programming Pull requests that affect the corresponding module labels Aug 17, 2026
@krusche
krusche temporarily deployed to playwright-e2e-tests August 18, 2026 21:19 — with GitHub Actions Inactive
@krusche
krusche temporarily deployed to playwright-e2e-tests August 18, 2026 23:14 — with GitHub Actions Inactive
…r source

Review finding: the clone-token check sits ahead of the authentication rate limiter on
purpose, but past its cheap gates it reads the whole distributed processing job map.
getProcessingJobsForAgentByName delegates to getProcessingJobs, which is
new ArrayList<>(getDistributedProcessingJobs().values()) - every entry pulled and
deserialized. The gates above it are a single-key agent lookup and the build agent
network allowlist, and a registered agent name is an identifier rather than a secret:
it is the middleware client name and is shown in the admin UI. A caller inside the
build agent networks who knows one could therefore force that read in a loop with
arbitrary passwords, unbounded, precisely because this path runs before the limiter.

A per-source limit now runs immediately before the read. It is placed after the cheap
gates so ordinary non-agent traffic cannot consume the agents' budget, and over the
limit it falls through rather than rejecting, matching this method's documented
contract that it never rejects a request - it only declines to treat one as a build
agent clone, after which normal authentication and its own limiter answer the caller.

BUILD_AGENT_CLONE_TOKEN defaults to 300 rpm and is configurable like the other types.
Deliberately generous: an agent clones several repositories per job and runs jobs
concurrently, so the limit has to sit far above real agent traffic or it becomes the
stall it exists to prevent. It is a ceiling on abuse, not a throttle on agents.

Adding the enum constant broke the exhaustive switch in RateLimitConfigurationService,
which is the compiler pointing at the configuration plumbing; the property and its
accessor are wired alongside the existing types.

Tests: shouldNotReadTheProcessingJobsOnceTheSourceIsOverTheLimit asserts the expensive
call is never made while over the limit, for a wrong password and for the correct token,
because asserting the return value alone proves nothing - it is already false for a
wrong token. shouldNotConsumeTheLimitForAUsernameThatIsNotABuildAgent pins the ordering.
Confirmed the first test bites: removing the gate fails it.

@Claudia-Anthropica Claudia-Anthropica left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@krusche [high] The new per-source call does not actually bound the expensive processing-map read under the repository default: RateLimitService.enforcePerMinute returns immediately when global rate limiting is disabled, and application-artemis.yml sets artemis.rate-limiting.enabled: false. Make this security gate unconditional (or independently enabled) and add coverage for the default disabled-global state; the current mock-only over-limit test never exercises it. [high] The earlier startup ambiguity also remains: line 207 maps any first-ever Optional.empty()—including a Redis CLIENT LIST timeout—to UNOBSERVABLE, and addressObservationAvailable is still false, so an absent live agent receives the origin exemption from a wrong allowed address. Return distinct unsupported and failed outcomes from the provider and only exempt the former.

@krusche
krusche had a problem deploying to playwright-e2e-tests August 19, 2026 07:14 — with GitHub Actions Error
develop moved the distributed data provider from
localci/service/distributed to core/service/distributed (#13352), which
collided with the client-address observation added on this branch.

- DistributedDataProvider: keep both sides' imports, so the interface
  carries getConnectedClientAddresses alongside develop's getExpiringMap.
- LocalDataProviderService: port getConnectedClientAddresses into the new
  package and drop the file at the old path.
- RedisClientListResolver: develop rewrote this for Redis Cluster with a
  ClientListSnapshot completeness flag. Rather than reinstate the old
  shape, ClientListSnapshot now carries the address map, so the cluster
  aggregation and the completeness semantics are reused: an incomplete
  lookup yields Optional.empty rather than an empty map, which is the
  distinction this branch already relied on between "unknown" and
  "nothing connected". Per-node address sets are merged, since one
  client can hold connections to several cluster nodes.
- Repoint two branch tests at the new package.
@krusche
krusche temporarily deployed to playwright-e2e-tests August 23, 2026 23:29 — with GitHub Actions Inactive
Both conflicts are additive on each side, so both sides are kept:

- RateLimitingProperties: this branch adds the build agent clone-token
  RPM field, develop (#13552) adds the exempt-addresses accessors.
- LocalVCServletService: this branch imports BuildJobQueueItem and
  BuildAgentNetworkPolicy, develop imports DomainObject; all three are
  referenced in the merged body.
@krusche
krusche temporarily deployed to playwright-e2e-tests August 24, 2026 20:47 — with GitHub Actions Inactive
…stributed lookup

Adds a local prefix gate to the build agent clone-token check so that an
ordinary user's git fetch no longer costs a distributed map read, adds the
shared distributed-data contract case for getConnectedClientAddresses, and
corrects the Redisson javadoc for the cluster completeness semantics.
…gent's clone authorization

The cross-check that removes a build job from the distributed processing
map when it is not running locally had no grace period, so it could fire
in the window between claiming a job and registering its future. That used
to skew the running-job counts; now that a build agent's clone is
authorized against that map, it made the job's next clone fail with 401
and the build end at 0%.

Also excludes the clone token service from the argument-logging aspect,
which printed a live token as an argument and as a return value.
…nd announce address observability

A blank entry in one of the address range lists parsed as the loopback
address instead of being rejected, so a stray value - a trailing comma in
an environment variable is enough - would quietly let the local host name
an arbitrary client through a trusted proxy or a PROXY protocol header.

Also states once, rather than at debug per request, whether this node can
bind a build agent's clones to the addresses it connects from, and
corrects the documentation about which agents that binding applies to.
…d access log row

A build agent clone is recorded with no user, in the same table and for
the same participation as the person's own entries. The two paths that
amend the newest entry - the commit hash after a push, and the clone or
pull label - would take the agent's row whenever its clone landed in
between, attributing the person's operation to the agent and leaving
their own row incomplete. Both lookups now skip user-less entries.
…traffic and drop the undefined author

The 300 per minute default sat below what one busy agent produces - eight
concurrent jobs of four repositories on twenty second builds is already
around 400 - and the key is the source address, which several agents
behind one NAT gateway share. Exceeding it fails the clone rather than
slowing it, so the default now clears a small group of busy agents.

A build agent access log entry carries no email, which the audit table
concatenated into a literal "undefined" beside the agent's name.
The branch produces 147 edges in the startup bean graph, not 143: three
of its beans are eager and each pulls its dependencies in with it, so the
bean instantiation check would have failed on a threshold raised for only
one of them.
@krusche
krusche had a problem deploying to playwright-e2e-tests August 25, 2026 14:38 — with GitHub Actions Error
…re local CI runs, and bound the clone-token limit by guessing

A local CI node no longer accepts a static build-agent username and
password. Every build job there carries a token scoped to its own
assignment, test, solution and auxiliary repositories, so the shared pair
buys nothing and costs a secret that opens every repository in the
installation. The node refuses to start with either half configured, the
shipped configurations no longer set them, and the https shortcut is
unreachable where local CI runs rather than merely unconfigured. A local
VC node without local CI - Jenkins with LocalVC - is unaffected.

The clone-token rate limit drops from 1500 to 30 per minute, the same
order as the other credential checks, because agents no longer consume
it: an address a build agent is registered at is exempt automatically as
it connects, and a check that succeeds spends nothing, so only guessing
does.

Build agent origins are now also observed under Redis. Redis has no
member/client split, so a node's client name is its node identity rather
than the agent short name; the agent publishes that identity as its
member address, which maps the two together.
… guesses that reach the scan

The ssh half of the origin binding had no test: the key proves which agent
is connecting and nothing about from where, and only the repository
scoping that follows was covered.

Parsing the repository path also moves ahead of the distributed scan. The
catch at the end of the check returns without spending budget, which is
right for a malformed request, so anything that can throw after the scan
was a way to run it for free.
…bservation shares the git path

A multi-node run on Redis refused every clone: 279 rejections, no
successful build. The agents were registered at the docker bridge gateway,
which is where Redis accepted their connections, while their clones arrive
on loopback.

The comparison is only meaningful when a client's middleware connection
terminates on a core node, which is a property of the backend rather than
of the query. Hazelcast clients connect to the cluster members, which are
the nodes that also serve git; Redis is a separate service on its own
path. Providers now state which they are, and the binding is skipped where
it cannot speak - the same handling as an origin that cannot be observed
at all. The allowlist and the per-build-job scoping are unaffected on
every backend: both are checked against the address of the request.
@krusche
krusche had a problem deploying to playwright-e2e-tests August 25, 2026 15:43 — with GitHub Actions Failure
…onfiguration happens

Both preferred mechanisms configure themselves and neither needs a secret
from the operator, which the documentation did not make the deciding
point. It now names the order - ssh keys first, since each agent
generates its own pair at startup and publishes only the public half;
build job clone tokens second, which need no configuration at all; a
shared username and password only for Jenkins with LocalVC, and refused
on any node running local CI.

The same order is repeated in application-localvc.yml,
application-buildagent.yml and application-artemis.yml, next to the
properties themselves, because that is where somebody deciding this is
looking.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

admin buildagent Pull requests that affect the corresponding module client Pull requests that update TypeScript code. (Added Automatically!) config-change Pull requests that change the config in a way that they require a deployment via Ansible. core Pull requests that affect the corresponding module database Pull requests that update the database. (Added Automatically!). Require a CRITICAL deployment. docker documentation programming Pull requests that affect the corresponding module ready for review server Pull requests that update Java code. (Added Automatically!) tests

Projects

Status: Ready For Review
Status: In progress

Development

Successfully merging this pull request may close these issues.

3 participants