diff --git a/changelog/unreleased/pr-26572.toml b/changelog/unreleased/pr-26572.toml new file mode 100644 index 000000000000..074a76a23a79 --- /dev/null +++ b/changelog/unreleased/pr-26572.toml @@ -0,0 +1,4 @@ +type = "fixed" +message = "Accept collector ingest mTLS connections only from certificates that are bound to an active collector instance." + +pulls = ["26572"] diff --git a/graylog2-server/src/main/java/org/graylog/collectors/CertBindingResolver.java b/graylog2-server/src/main/java/org/graylog/collectors/CertBindingResolver.java new file mode 100644 index 000000000000..995a5ffe2bae --- /dev/null +++ b/graylog2-server/src/main/java/org/graylog/collectors/CertBindingResolver.java @@ -0,0 +1,236 @@ +/* + * Copyright (C) 2020 Graylog, Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the Server Side Public License, version 1, + * as published by MongoDB, Inc. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * Server Side Public License for more details. + * + * You should have received a copy of the Server Side Public License + * along with this program. If not, see + * . + */ +package org.graylog.collectors; + +import com.github.benmanes.caffeine.cache.Caffeine; +import com.github.benmanes.caffeine.cache.LoadingCache; +import com.github.benmanes.caffeine.cache.Ticker; +import com.google.common.eventbus.EventBus; +import com.google.common.eventbus.Subscribe; +import com.google.common.util.concurrent.AbstractIdleService; +import jakarta.inject.Inject; +import jakarta.inject.Singleton; +import org.graylog.collectors.events.CollectorInstanceCertsChangedEvent; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.time.Clock; +import java.time.Duration; +import java.time.Instant; +import java.util.Objects; +import java.util.Optional; +import java.util.concurrent.Executor; +import java.util.concurrent.atomic.AtomicLong; + +/** + * Resolves a client-certificate fingerprint to the collector instance it currently binds to — the single + * authority on ingest-path certificate validity. A fingerprint binds while it occupies one of the + * instance's certificate slots (active, next, or previous) and the binding is within its validity: the + * certificate's own expiry, capped for a superseded (previous-slot) certificate by the rotation grace + * period ({@link CollectorsConfig#collectorCertRotationGracePeriod()}). Since TLS never re-validates an + * established connection, this per-request resolution is the sole post-handshake cut for rotated, revoked, + * and expired certificates. + *

+ * Resolved bindings are cached internally so the ingest mTLS path avoids a MongoDB lookup per request. + * Entries expire after a constant idle window ({@link #IDLE_EXPIRY}, sliding on access) — deliberately + * longer than the ingest transport's forced idle connection timeout, so an entry warmed at the TLS + * handshake always outlives its connection and per-request resolutions never load on the Netty event + * loop. Validity is enforced on every read, so a cut never requires a reload. The cache is prewarmed at + * startup with active fingerprints and kept consistent by {@link CollectorInstanceCertsChangedEvent}s, + * which refresh touched entries in place. + *

+ * Refreshes are serve-stale-while-revalidating: a failed reload retains the current value, + * and {@code refreshAfterWrite} re-resolves every entry that is still being accessed once its value + * is older than half the idle window. + */ +@Singleton +public class CertBindingResolver extends AbstractIdleService { + + public static final long MAX_SIZE = 1_000_000L; // safety net + // Using a fixed cache expiry > idle connection timeout (See CollectorIngestHttpTransport). This makes sure that + // cert lookups in the request path are cache hits, after the TLS handshake has populated the cache. + private static final Duration IDLE_EXPIRY = Duration.ofMinutes(60); + private static final Logger LOG = LoggerFactory.getLogger(CertBindingResolver.class); + + private final CollectorsConfigService configService; + private final CollectorInstanceService instanceService; + private final Executor executor; + private final EventBus eventBus; + private final Clock clock; + + private final LoadingCache> cache; + + @Inject + public CertBindingResolver(CollectorsConfigService configService, + CollectorInstanceService instanceService, + EventBus eventBus, + Clock clock, + @CollectorCertCacheRefreshExecutor Executor executor) { + this.configService = configService; + this.instanceService = instanceService; + this.executor = executor; + this.eventBus = eventBus; + this.clock = clock; + this.cache = Caffeine.newBuilder() + .maximumSize(MAX_SIZE) + .executor(executor) // refresh runs on this executor + .ticker(clockTicker()) // drive Caffeine's expiry off the same Clock as the binding deadlines + .expireAfterAccess(IDLE_EXPIRY) + .refreshAfterWrite(IDLE_EXPIRY.dividedBy(2)) // self-healing bound for failed/missed refreshes + .build(this::loadBinding); + } + + @Override + protected void startUp() throws Exception { + eventBus.register(this); + executor.execute(this::preWarm); + } + + @Override + protected void shutDown() throws Exception { + eventBus.unregister(this); + } + + /** + * Resolves a certificate fingerprint to the instance UID it currently binds to. Returns an empty + * optional if the fingerprint is unknown, its binding is no longer valid (superseded certificate past + * the rotation grace period, or expired certificate), or resolution fails. + */ + public Optional resolve(String fingerprint) { + try { + return cache.get(fingerprint) + .filter(binding -> binding.isValidAt(clock.instant())) + .map(CertBinding::instanceUid); + } catch (Exception e) { + LOG.error("Error resolving collector instance for fingerprint {}.", fingerprint, e); + } + return Optional.empty(); + } + + /** + * Maps a fingerprint to its binding from the instance's certificate slots. A pure mapping — no clock + * is consulted: every binding carries the instant it stops being honored + * ({@link CertBinding#validUntil()}), and {@link #resolve(String)} evaluates validity against it. + * Slot fields that should exist but don't (corrupt document) yield an already-expired binding rather + * than an error. + */ + private Optional loadBinding(String fingerprint) { + final var binding = instanceService.findByActiveOrNextOrPreviousFingerprint(fingerprint) + .map(instance -> { + if (fingerprint.equals(instance.activeCertificateFingerprint())) { + return loadedBinding(fingerprint, instance.instanceUid(), "active", + instance.activeCertificateExpiresAt()); + } + if (instance.previousCertificateFingerprint().filter(fingerprint::equals).isPresent()) { + final var gracePeriod = configService.getOrDefault().collectorCertRotationGracePeriod(); + final var graceDeadline = instance.certificatesRotatedAt() + .map(rotatedAt -> rotatedAt.plus(gracePeriod)) + .orElse(Instant.MIN); + final var certExpiry = instance.previousCertificateExpiresAt().orElse(Instant.MIN); + return loadedBinding(fingerprint, instance.instanceUid(), "previous", + graceDeadline.isBefore(certExpiry) ? graceDeadline : certExpiry); + } + if (instance.nextCertificateFingerprint().filter(fingerprint::equals).isPresent()) { + final var certExpiry = instance.nextCertificateExpiresAt().orElse(Instant.MIN); + return loadedBinding(fingerprint, instance.instanceUid(), "next", certExpiry); + } + return null; + }); + if (binding.isEmpty()) { + LOG.debug("No instance binds fingerprint {}.", fingerprint); + } + return binding; + } + + private static CertBinding loadedBinding(String fingerprint, String instanceUid, String slot, Instant validUntil) { + LOG.debug("Loaded binding for fingerprint {}: instance {}, slot {}, valid until {}.", + fingerprint, instanceUid, slot, validUntil); + return new CertBinding(instanceUid, validUntil); + } + + /** + * Keeps the cache consistent with certificate changes by re-resolving each touched fingerprint that is + * currently cached. Absent fingerprints are left alone: they hold no stale binding and will resolve + * correctly on their next lookup, so proactively loading them would only add pointless work (e.g. for + * the many fingerprints of expired instances that no longer exist). + *

+ * A failed re-resolve retains the current value (serve-stale): the entry's write timestamp is not + * renewed, so {@code refreshAfterWrite} keeps re-attempting it on later accesses until a reload + * succeeds — see the class javadoc for the staleness bound this provides. + */ + @Subscribe + public void handleCertsChanged(CollectorInstanceCertsChangedEvent event) { + int refreshed = 0; + for (final var fingerprint : event.fingerprints()) { + if (cache.asMap().containsKey(fingerprint)) { + cache.refresh(fingerprint); + refreshed++; + } + } + LOG.debug("Certificates changed: refreshing {} cached of {} touched fingerprint(s).", + refreshed, event.fingerprints().size()); + } + + private Ticker clockTicker() { + return () -> clock.millis() * 1_000_000L; + } + + /** + * Loads each unexpired instance's active certificate fingerprint into the cache at startup (bounded by + * {@link #MAX_SIZE}), so a fleet reconnecting after a restart hits a warm cache instead of cold-loading + * on each handshake. Only the active fingerprint is prewarmed: it is what essentially every ingest + * connection presents in steady state (the hot set). The {@code next} and {@code previous} fingerprints + * are held only by the few instances mid-renewal or within the post-activation grace window, so they + * cold-load off the event loop on first use without risking a stampede. Best-effort: a failure leaves + * the cache to populate lazily. + */ + private void preWarm() { + try { + final var expirationThreshold = configService.getOrDefault().collectorExpirationThreshold(); + final var prewarmed = new AtomicLong(); + try (var stream = instanceService.streamAllUnexpired(expirationThreshold).limit(MAX_SIZE)) { + stream.forEach(instance -> { + cache.put(instance.activeCertificateFingerprint(), Optional.of( + new CertBinding(instance.instanceUid(), instance.activeCertificateExpiresAt()))); + prewarmed.incrementAndGet(); + }); + } + LOG.debug("Prewarmed {} active collector fingerprint(s).", prewarmed.get()); + } catch (Exception e) { + LOG.warn("Failed pre-warming collector fingerprint cache.", e); + } + } + + /** + * A certificate fingerprint's binding to a collector instance: the instance the certificate binds to + * and the instant the binding stops being honored on the ingest path — the certificate's own expiry, + * capped for a superseded (previous-slot) certificate by the rotation grace period. The binding is a + * pure fact about the instance's certificate slots; whether it is valid now is evaluated by + * {@link #resolve(String)} via {@link #isValidAt(Instant)}, so a loaded binding may already be + * expired. + */ + private record CertBinding(String instanceUid, Instant validUntil) { + private CertBinding { + Objects.requireNonNull(instanceUid, "instanceUid must not be null"); + Objects.requireNonNull(validUntil, "validUntil must not be null"); + } + + private boolean isValidAt(Instant instant) { + return instant.isBefore(validUntil()); + } + } +} diff --git a/graylog2-server/src/main/java/org/graylog/collectors/CollectorCaTrustManager.java b/graylog2-server/src/main/java/org/graylog/collectors/CollectorCaTrustManager.java index 648943e78614..3126ab5abf2a 100644 --- a/graylog2-server/src/main/java/org/graylog/collectors/CollectorCaTrustManager.java +++ b/graylog2-server/src/main/java/org/graylog/collectors/CollectorCaTrustManager.java @@ -16,6 +16,7 @@ */ package org.graylog.collectors; +import io.opentelemetry.instrumentation.annotations.WithSpan; import jakarta.inject.Inject; import jakarta.inject.Singleton; import org.bouncycastle.asn1.x509.KeyPurposeId; @@ -32,6 +33,8 @@ import java.time.Instant; import java.util.Date; +import static org.graylog2.shared.utilities.StringUtils.f; + /** * Custom trust manager that looks up the trust chain via authority key identifiers. This allows efficient certificate * lookup with multiple active signing certs. (e.g., cert renewal) @@ -45,14 +48,17 @@ public class CollectorCaTrustManager extends X509ExtendedTrustManager { private static final Logger LOG = LoggerFactory.getLogger(CollectorCaTrustManager.class); private final CollectorCaCache caCache; + private final CertBindingResolver certBindingResolver; private final Clock clock; @Inject - public CollectorCaTrustManager(CollectorCaCache caCache, Clock clock) { + public CollectorCaTrustManager(CollectorCaCache caCache, CertBindingResolver certBindingResolver, Clock clock) { this.caCache = caCache; + this.certBindingResolver = certBindingResolver; this.clock = clock; } + @WithSpan @Override public void checkClientTrusted(X509Certificate[] certs, String authType) throws CertificateException { if (certs == null || certs.length == 0) { @@ -72,6 +78,7 @@ public void checkClientTrusted(X509Certificate[] certs, String authType) throws verifySignatureAndValidity(clientCert, issuerEntry.cert()); verifyEndEntityCert(clientCert); verifyClientAuthEku(clientCert); + verifyClientCertCollectorBinding(clientCert); LOG.debug("Client certificate trusted: subject=<{}>, issuer=<{}>", clientCert.getSubjectX500Principal(), clientCert.getIssuerX500Principal()); @@ -134,6 +141,33 @@ private void verifyClientAuthEku(X509Certificate clientCert) throws CertificateE } } + private void verifyClientCertCollectorBinding(X509Certificate clientCert) throws CertificateException { + final String cn; + try { + cn = PemUtils.extractCn(clientCert); + } catch (Exception e) { + throw new CertificateException("Failed to extract CN from client certificate for collector binding verification", e); + } + + final String fingerprint; + try { + fingerprint = PemUtils.computeFingerprint(clientCert); + } catch (Exception e) { + throw new CertificateException("Failed to compute fingerprint for client certificate", e); + } + + final var boundInstanceUuid = certBindingResolver.resolve(fingerprint); + if (boundInstanceUuid.isEmpty()) { + throw new CertificateException(f("No collector instance found for certificate fingerprint %s", fingerprint)); + } + + if (!cn.equals(boundInstanceUuid.get())) { + throw new CertificateException("Certificate CN does not match bound collector instance"); + } + + LOG.debug("Verified collector binding for fingerprint {} -> instance {}.", fingerprint, boundInstanceUuid.get()); + } + @Override public void checkClientTrusted(X509Certificate[] certs, String authType, Socket socket) throws CertificateException { checkClientTrusted(certs, authType); diff --git a/graylog2-server/src/main/java/org/graylog/collectors/CollectorCertCacheRefreshExecutor.java b/graylog2-server/src/main/java/org/graylog/collectors/CollectorCertCacheRefreshExecutor.java new file mode 100644 index 000000000000..5deab9d1a617 --- /dev/null +++ b/graylog2-server/src/main/java/org/graylog/collectors/CollectorCertCacheRefreshExecutor.java @@ -0,0 +1,38 @@ +/* + * Copyright (C) 2020 Graylog, Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the Server Side Public License, version 1, + * as published by MongoDB, Inc. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * Server Side Public License for more details. + * + * You should have received a copy of the Server Side Public License + * along with this program. If not, see + * . + */ +package org.graylog.collectors; + +import com.google.inject.BindingAnnotation; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +/** + * Qualifies the {@code Executor} that runs the {@link CertBindingResolver}'s background cache work: + * event-driven and {@code refreshAfterWrite} reloads, and the startup prewarm. + *

+ * Deliberately distinct from {@link CollectorCertVerificationExecutor}: these tasks block on MongoDB + * with nobody waiting on the result, so they must not compete with the latency-critical TLS handshake + * work. The bound pool and its queueing behavior are defined where the executor is provided in + * {@code CollectorsModule}. + */ +@Target({ElementType.METHOD, ElementType.PARAMETER}) +@Retention(RetentionPolicy.RUNTIME) +@BindingAnnotation +public @interface CollectorCertCacheRefreshExecutor {} diff --git a/graylog2-server/src/main/java/org/graylog/collectors/CollectorCertVerificationExecutor.java b/graylog2-server/src/main/java/org/graylog/collectors/CollectorCertVerificationExecutor.java new file mode 100644 index 000000000000..e3305c361506 --- /dev/null +++ b/graylog2-server/src/main/java/org/graylog/collectors/CollectorCertVerificationExecutor.java @@ -0,0 +1,39 @@ +/* + * Copyright (C) 2020 Graylog, Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the Server Side Public License, version 1, + * as published by MongoDB, Inc. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * Server Side Public License for more details. + * + * You should have received a copy of the Server Side Public License + * along with this program. If not, see + * . + */ +package org.graylog.collectors; + +import com.google.inject.BindingAnnotation; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +/** + * Qualifies the {@code Executor} used to run collector mTLS certificate verification off the Netty + * event loop: it is the {@code SslHandler} delegated-task executor for the ingest TLS handshake, so + * the trust manager's {@code checkClientTrusted} (and its instance-binding resolution, which may + * block on MongoDB on a cold cache miss) never runs on an event-loop thread. + *

+ * The {@link CertBindingResolver}'s background cache work deliberately does not share this + * pool — see {@link CollectorCertCacheRefreshExecutor}. The bound pool and its overload (shedding) + * behavior are defined where the executor is provided in {@code CollectorsModule}. + */ +@Target({ElementType.METHOD, ElementType.PARAMETER}) +@Retention(RetentionPolicy.RUNTIME) +@BindingAnnotation +public @interface CollectorCertVerificationExecutor {} diff --git a/graylog2-server/src/main/java/org/graylog/collectors/CollectorInstanceService.java b/graylog2-server/src/main/java/org/graylog/collectors/CollectorInstanceService.java index a5ea1ef176a1..b31c01cdedb9 100644 --- a/graylog2-server/src/main/java/org/graylog/collectors/CollectorInstanceService.java +++ b/graylog2-server/src/main/java/org/graylog/collectors/CollectorInstanceService.java @@ -18,25 +18,33 @@ import com.fasterxml.jackson.annotation.JsonProperty; import com.google.common.base.Predicates; +import com.google.common.collect.Iterables; +import com.google.common.collect.Sets; +import com.google.errorprone.annotations.MustBeClosed; import com.mongodb.client.model.Accumulators; import com.mongodb.client.model.Aggregates; import com.mongodb.client.model.Filters; +import com.mongodb.client.model.FindOneAndDeleteOptions; import com.mongodb.client.model.FindOneAndUpdateOptions; import com.mongodb.client.model.IndexModel; import com.mongodb.client.model.IndexOptions; import com.mongodb.client.model.Indexes; import com.mongodb.client.model.Projections; import com.mongodb.client.model.ReturnDocument; +import com.mongodb.client.model.Sorts; +import com.mongodb.client.model.Updates; import com.mongodb.client.result.InsertOneResult; import jakarta.annotation.Nonnull; import jakarta.inject.Inject; import jakarta.inject.Singleton; import org.apache.commons.lang3.StringUtils; +import org.bson.BsonType; import org.bson.Document; import org.bson.conversions.Bson; import org.graylog.collectors.db.Attribute; import org.graylog.collectors.db.CollectorInstanceDTO; import org.graylog.collectors.db.CollectorInstanceReport; +import org.graylog.collectors.events.CollectorInstanceCertsChangedEvent; import org.graylog.collectors.opamp.IssuedCertificate; import org.graylog2.database.MongoCollection; import org.graylog2.database.MongoCollections; @@ -44,6 +52,7 @@ import org.graylog2.database.filtering.DbSortResolver; import org.graylog2.database.pagination.MongoPaginationHelper; import org.graylog2.database.utils.MongoUtils; +import org.graylog2.events.ClusterEventBus; import org.mongojack.Id; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -56,6 +65,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Optional; import java.util.Set; import java.util.function.Function; @@ -72,6 +82,7 @@ import static org.graylog.collectors.db.CollectorInstanceDTO.FIELD_ACTIVE_CERTIFICATE_FINGERPRINT; import static org.graylog.collectors.db.CollectorInstanceDTO.FIELD_ACTIVE_CERTIFICATE_PEM; import static org.graylog.collectors.db.CollectorInstanceDTO.FIELD_CAPABILITIES; +import static org.graylog.collectors.db.CollectorInstanceDTO.FIELD_CERTIFICATES_ROTATED_AT; import static org.graylog.collectors.db.CollectorInstanceDTO.FIELD_ENROLLMENT_TOKEN_ID; import static org.graylog.collectors.db.CollectorInstanceDTO.FIELD_FLEET_ID; import static org.graylog.collectors.db.CollectorInstanceDTO.FIELD_IDENTIFYING_ATTRIBUTES; @@ -84,6 +95,9 @@ import static org.graylog.collectors.db.CollectorInstanceDTO.FIELD_NEXT_CERTIFICATE_FINGERPRINT; import static org.graylog.collectors.db.CollectorInstanceDTO.FIELD_NEXT_CERTIFICATE_PEM; import static org.graylog.collectors.db.CollectorInstanceDTO.FIELD_NON_IDENTIFYING_ATTRIBUTES; +import static org.graylog.collectors.db.CollectorInstanceDTO.FIELD_PREVIOUS_CERTIFICATE_EXPIRES_AT; +import static org.graylog.collectors.db.CollectorInstanceDTO.FIELD_PREVIOUS_CERTIFICATE_FINGERPRINT; +import static org.graylog.collectors.db.CollectorInstanceDTO.FIELD_PREVIOUS_CERTIFICATE_PEM; import static org.graylog2.database.MongoEntity.FIELD_ID; import static org.graylog2.database.utils.MongoUtils.insertedIdAsString; import static org.graylog2.shared.utilities.StringUtils.f; @@ -92,19 +106,43 @@ public class CollectorInstanceService { private static final Logger LOG = LoggerFactory.getLogger(CollectorInstanceService.class); private static final String OS_TYPE_KEY = "os.type"; + public static final String COLLECTION_NAME = "collector_instances"; + + private static final int REVOCATION_EVENT_BATCH_SIZE = 1000; // not more than this many fingerprints per event + private static final long MAX_DELETIONS_PER_RUN = 100_000; // safety net for deleteExpired private final MongoCollection collection; private final MongoPaginationHelper paginationHelper; private final com.mongodb.client.MongoCollection projectedCollection; + + private final MongoCollections mongoCollections; + private final ClusterEventBus clusterEventBus; private final Clock clock; @Inject - public CollectorInstanceService(MongoCollections mongoCollections, Clock clock) { - collection = mongoCollections.collection("collector_instances", CollectorInstanceDTO.class); - projectedCollection = mongoCollections.nonEntityCollection("collector_instances", MinimalCollectorInstanceDTO.class); + public CollectorInstanceService(MongoCollections mongoCollections, ClusterEventBus clusterEventBus, Clock clock) { + collection = mongoCollections.collection(COLLECTION_NAME, CollectorInstanceDTO.class); + projectedCollection = mongoCollections.nonEntityCollection(COLLECTION_NAME, MinimalCollectorInstanceDTO.class); paginationHelper = mongoCollections.paginationHelper(collection); + this.mongoCollections = mongoCollections; + this.clusterEventBus = clusterEventBus; this.clock = clock; + // Clean up legacy null values which would prevent creating a unique index below + try { + final var updated = collection.updateMany(Filters.type(FIELD_NEXT_CERTIFICATE_FINGERPRINT, BsonType.NULL), + Updates.combine(Updates.unset(FIELD_NEXT_CERTIFICATE_FINGERPRINT), + Updates.unset(FIELD_NEXT_CERTIFICATE_PEM), + Updates.unset(FIELD_NEXT_CERTIFICATE_EXPIRES_AT) + ) + ); + if (updated.getModifiedCount() > 0) { + LOG.info("Cleaned up {} legacy null values for the \"next_certificate_*\" fields", updated.getModifiedCount()); + } + } catch (Exception e) { + LOG.error("Cleaning up legacy null values for the \"next_certificate_*\" fields failed", e); + } + try { collection.createIndexes(List.of( new IndexModel(Indexes.ascending(FIELD_INSTANCE_UID), new IndexOptions().unique(true)), @@ -112,7 +150,14 @@ public CollectorInstanceService(MongoCollections mongoCollections, Clock clock) FIELD_IDENTIFYING_ATTRIBUTES + ".value")), new IndexModel(Indexes.ascending(FIELD_NON_IDENTIFYING_ATTRIBUTES + ".key", FIELD_NON_IDENTIFYING_ATTRIBUTES + ".value")), - new IndexModel(Indexes.ascending(FIELD_ACTIVE_CERTIFICATE_FINGERPRINT), new IndexOptions().unique(true)), + new IndexModel(Indexes.ascending(FIELD_ACTIVE_CERTIFICATE_FINGERPRINT), + new IndexOptions().unique(true)), + new IndexModel(Indexes.ascending(FIELD_NEXT_CERTIFICATE_FINGERPRINT), + new IndexOptions().partialFilterExpression(Filters.exists(FIELD_NEXT_CERTIFICATE_FINGERPRINT)) + .unique(true)), + new IndexModel(Indexes.ascending(FIELD_PREVIOUS_CERTIFICATE_FINGERPRINT), + new IndexOptions().partialFilterExpression(Filters.exists(FIELD_PREVIOUS_CERTIFICATE_FINGERPRINT)) + .unique(true)), new IndexModel(Indexes.ascending(FIELD_LAST_SEEN)) )); } catch (Exception e) { @@ -180,6 +225,8 @@ public void updateCurrentFleet(@Nonnull String instanceUid, @Nonnull String newF *

* The {@code instance_uid} index is unique; concurrent first-time enrollments for the same UID * surface as {@link com.mongodb.MongoWriteException} with a duplicate-key error. + *

+ * Publishes a {@link CollectorInstanceCertsChangedEvent} for the new active fingerprint. * * @param instanceUid the agent's self-chosen OpAMP instance UID * @param fleetId the fleet the enrolling token belongs to @@ -208,6 +255,9 @@ public CollectorInstanceDTO enroll(String instanceUid, .enrollmentTokenId(enrollmentTokenId) .build(); final InsertOneResult insertOneResult = collection.insertOne(dto); + + clusterEventBus.post(new CollectorInstanceCertsChangedEvent(Set.of(issuedCert.fingerprint()))); + return dto.toBuilder() .id(insertedIdAsString(insertOneResult)) .build(); @@ -216,40 +266,45 @@ public CollectorInstanceDTO enroll(String instanceUid, /** * Atomically re-issues a collector's active certificate on an existing record. *

- * The update is a compare-and-swap: it only matches the record with the given Mongo - * {@code _id} and the given active certificate fingerprint. Callers must pass the id - * and fingerprint they observed when they verified the re-enrollment request (typically via - * {@link #findByInstanceUid}). This ensures the update only lands on the exact state the - * caller validated. If the record has since been deleted, replaced, or had its active - * certificate changed (e.g. by a concurrent renewal activation), the update matches nothing - * and this method throws. + * The update is a compare-and-swap: it only matches the record whose Mongo {@code _id}, active + * certificate fingerprint, and next certificate fingerprint equal those of the given + * {@code instance}. Callers must pass the instance they observed when they verified the + * re-enrollment request (typically via {@link #findByInstanceUid}). This ensures the update only + * lands on the exact state the caller validated. If the record has since been deleted, replaced, + * or had its certificates changed (e.g. by a concurrent renewal activation), the update matches + * nothing and this method throws. *

* The caller (typically {@code OpAmpService.handleEnrollment}) is responsible for enforcing * proof-of-possession (matching the CSR public key against the stored active certificate's * public key) before calling this method. This service method performs no such check — it * only enforces the compare-and-swap race guard. + *

+ * Publishes a {@link CollectorInstanceCertsChangedEvent} for every fingerprint this touches — the new + * active fingerprint plus the replaced active, next, and previous fingerprints — so subscribers + * re-resolve them all (the replaced ones resolve to nothing, since they were cleared). * - * @param id the Mongo {@code _id} of the record to update - * @param expectedActiveFingerprint the active certificate fingerprint the caller observed when - * it verified proof-of-possession - * @param issuedCert the freshly signed agent certificate - * @param enrollmentTokenId the id of the enrollment token that authorized this - * re-enrollment + * @param existingInstance the record state the caller validated (supplies the {@code _id} and the + * active and next fingerprints matched by the compare-and-swap) + * @param issuedCert the freshly signed agent certificate + * @param enrollmentTokenId the id of the enrollment token that authorized this re-enrollment * @return the updated DTO (post-update state) - * @throws IllegalStateException if no record with the given {@code _id} and active certificate - * fingerprint is found — the target was concurrently deleted, + * @throws IllegalStateException if no record matching the instance's {@code _id} and certificate + * fingerprints is found — the target was concurrently deleted, * replaced, or modified */ - public CollectorInstanceDTO reEnroll(String id, - String expectedActiveFingerprint, + public CollectorInstanceDTO reEnroll(CollectorInstanceDTO existingInstance, IssuedCertificate issuedCert, String enrollmentTokenId) { final var updates = combine( set(FIELD_LAST_SEEN, Date.from(clock.instant())), + unset(FIELD_PREVIOUS_CERTIFICATE_FINGERPRINT), + unset(FIELD_PREVIOUS_CERTIFICATE_PEM), + unset(FIELD_PREVIOUS_CERTIFICATE_EXPIRES_AT), unset(FIELD_NEXT_CERTIFICATE_FINGERPRINT), unset(FIELD_NEXT_CERTIFICATE_PEM), unset(FIELD_NEXT_CERTIFICATE_EXPIRES_AT), + unset(FIELD_CERTIFICATES_ROTATED_AT), set(FIELD_ACTIVE_CERTIFICATE_FINGERPRINT, issuedCert.fingerprint()), set(FIELD_ACTIVE_CERTIFICATE_PEM, issuedCert.certPem()), set(FIELD_ACTIVE_CERTIFICATE_EXPIRES_AT, Date.from(issuedCert.notAfter())), @@ -258,15 +313,27 @@ public CollectorInstanceDTO reEnroll(String id, ); final var updated = collection.findOneAndUpdate( - Filters.and(MongoUtils.idEq(id), Filters.eq(FIELD_ACTIVE_CERTIFICATE_FINGERPRINT, expectedActiveFingerprint)), + Filters.and( + MongoUtils.idEq( + Objects.requireNonNull(existingInstance.id(), "Instance ID must not be null for re-enrollment") + ), + Filters.eq(FIELD_ACTIVE_CERTIFICATE_FINGERPRINT, existingInstance.activeCertificateFingerprint()), + Filters.eq(FIELD_NEXT_CERTIFICATE_FINGERPRINT, existingInstance.nextCertificateFingerprint().orElse(null)), + Filters.eq(FIELD_PREVIOUS_CERTIFICATE_FINGERPRINT, existingInstance.previousCertificateFingerprint().orElse(null)) + ), updates, new FindOneAndUpdateOptions().upsert(false).returnDocument(ReturnDocument.AFTER) ); if (updated == null) { - throw new IllegalStateException(f("Cannot re-enroll. Collector instance with id %s doesn't exist or its active certificate changed concurrently.", id)); + throw new IllegalStateException(f("Cannot re-enroll. Collector instance with id %s doesn't exist or its active certificate changed concurrently.", existingInstance.id())); } + + clusterEventBus.post(new CollectorInstanceCertsChangedEvent( + Sets.union(existingInstance.allCertFingerprints(), updated.allCertFingerprints())) + ); + return updated; } @@ -286,29 +353,80 @@ public Optional findByActiveOrNextFingerprint(String finge } /** - * Sets the next certificate as active for the given instance. + * Finds the instance holding the given fingerprint in any certificate slot (active, next, or + * previous). Raw data access with no validity semantics — {@link CertBindingResolver} is the only + * intended caller and the authority on whether the certificate may actually be used. + */ + Optional findByActiveOrNextOrPreviousFingerprint(String fingerprint) { + return Optional.ofNullable( + collection.find(Filters.or( + Filters.eq(FIELD_ACTIVE_CERTIFICATE_FINGERPRINT, fingerprint), + Filters.eq(FIELD_NEXT_CERTIFICATE_FINGERPRINT, fingerprint), + Filters.eq(FIELD_PREVIOUS_CERTIFICATE_FINGERPRINT, fingerprint) + )).first() + ); + } + + /** + * Promotes the instance's next certificate to active. The superseded active certificate is demoted to + * the previous slot (kept resolvable on the ingest path for the grace window measured from + * {@code certificates_rotated_at}, which this stamps to now) and the next slot is cleared. + * + * Publishes a {@link CollectorInstanceCertsChangedEvent} for the rotated fingerprints (promoted, + * demoted, and any displaced previous) so subscribers re-resolve them. + * + * The update only applies while the document's certificate slots still match the given DTO + * (compare-and-swap): if a concurrent re-enrollment, renewal, or activation changed them after the + * caller's read, nothing is written and the caller may retry with a fresh read. * - * @param instance the instance DTO - * @return true if the activation succeeded, false otherwise + * @param instance the instance DTO whose slot state must still be current + * @return true if the activation succeeded, false if the instance doesn't exist or its certificate + * slots changed concurrently * @throws IllegalArgumentException if no next certificate exists for the instance */ public boolean activateNextCertificate(CollectorInstanceDTO instance) { final Supplier err = () -> new IllegalArgumentException("Instance missing next certificate data"); - final var result = collection.updateOne(Filters.eq(FIELD_INSTANCE_UID, instance.instanceUid()), combine( - set(FIELD_ACTIVE_CERTIFICATE_PEM, instance.nextCertificatePem().orElseThrow(err)), - set(FIELD_ACTIVE_CERTIFICATE_FINGERPRINT, instance.nextCertificateFingerprint().orElseThrow(err)), - set(FIELD_ACTIVE_CERTIFICATE_EXPIRES_AT, Date.from(instance.nextCertificateExpiresAt().orElseThrow(err))), - unset(FIELD_NEXT_CERTIFICATE_PEM), - unset(FIELD_NEXT_CERTIFICATE_FINGERPRINT), - unset(FIELD_NEXT_CERTIFICATE_EXPIRES_AT) - )); + final var orig = collection.findOneAndUpdate( + Filters.and( + Filters.eq(FIELD_INSTANCE_UID, instance.instanceUid()), + Filters.eq(FIELD_ACTIVE_CERTIFICATE_FINGERPRINT, instance.activeCertificateFingerprint()), + Filters.eq(FIELD_NEXT_CERTIFICATE_FINGERPRINT, instance.nextCertificateFingerprint().orElse(null)), + Filters.eq(FIELD_PREVIOUS_CERTIFICATE_FINGERPRINT, instance.previousCertificateFingerprint().orElse(null)) + ), + combine( + set(FIELD_PREVIOUS_CERTIFICATE_PEM, instance.activeCertificatePem()), + set(FIELD_PREVIOUS_CERTIFICATE_FINGERPRINT, instance.activeCertificateFingerprint()), + set(FIELD_PREVIOUS_CERTIFICATE_EXPIRES_AT, Date.from(instance.activeCertificateExpiresAt())), + set(FIELD_ACTIVE_CERTIFICATE_PEM, instance.nextCertificatePem().orElseThrow(err)), + set(FIELD_ACTIVE_CERTIFICATE_FINGERPRINT, instance.nextCertificateFingerprint().orElseThrow(err)), + set(FIELD_ACTIVE_CERTIFICATE_EXPIRES_AT, Date.from(instance.nextCertificateExpiresAt().orElseThrow(err))), + set(FIELD_CERTIFICATES_ROTATED_AT, Date.from(clock.instant())), + unset(FIELD_NEXT_CERTIFICATE_PEM), + unset(FIELD_NEXT_CERTIFICATE_FINGERPRINT), + unset(FIELD_NEXT_CERTIFICATE_EXPIRES_AT) + ), + new FindOneAndUpdateOptions().returnDocument(ReturnDocument.BEFORE) + ); + + if (orig != null) { + LOG.debug("Rotated certificates for instance {}: promoted next fingerprint {} to \"active\", demoted " + + "active fingerprint {} to \"previous\".", + instance.instanceUid(), instance.nextCertificateFingerprint().orElse(null), + instance.activeCertificateFingerprint()); + clusterEventBus.post(new CollectorInstanceCertsChangedEvent( + Sets.union(instance.allCertFingerprints(), orig.allCertFingerprints())) + ); + } - return result.getModifiedCount() > 0; + return orig != null; } /** * Inserts the next certificate data for the given instance UID. + *

+ * Publishes a {@link CollectorInstanceCertsChangedEvent} for the new next fingerprint and the + * fingerprints already on the instance, so subscribers re-resolve them. * * @param instanceUid the instance UID * @param fingerprint the next certificate fingerprint @@ -317,27 +435,118 @@ public boolean activateNextCertificate(CollectorInstanceDTO instance) { * @return true if the insert succeeded, false otherwise */ public boolean insertNextCertificate(String instanceUid, String fingerprint, String pem, Instant expiresAt) { - final var result = collection.updateOne(Filters.eq(FIELD_INSTANCE_UID, instanceUid), combine( + final var orig = collection.findOneAndUpdate(Filters.eq(FIELD_INSTANCE_UID, instanceUid), combine( set(FIELD_NEXT_CERTIFICATE_PEM, pem), set(FIELD_NEXT_CERTIFICATE_FINGERPRINT, fingerprint), set(FIELD_NEXT_CERTIFICATE_EXPIRES_AT, Date.from(expiresAt)) - )); - return result.getModifiedCount() > 0; + ), new FindOneAndUpdateOptions().returnDocument(ReturnDocument.BEFORE)); + + if (orig != null) { + clusterEventBus.post(new CollectorInstanceCertsChangedEvent( + Sets.union(orig.allCertFingerprints(), Set.of(fingerprint))) + ); + } + + return orig != null; } + /** + * Finds the collector instance with the given instance UID. + * + * @return the instance, or an empty optional if none exists + */ public Optional findByInstanceUid(String instanceUid) { return Optional.ofNullable( collection.find(Filters.eq(FIELD_INSTANCE_UID, instanceUid)).first() ); } + /** + * Deletes the collector instance with the given instance UID and, if one was deleted, publishes a + * {@link CollectorInstanceCertsChangedEvent} for all of its certificate fingerprints so subscribers + * re-resolve them (and drop them, since the instance is gone). + * + * @return true if an instance was deleted, false if none matched + */ public boolean deleteByInstanceUid(String instanceUid) { - return collection.deleteOne(Filters.eq(FIELD_INSTANCE_UID, instanceUid)).getDeletedCount() > 0; + final var deleted = collection.findOneAndDelete(Filters.eq(FIELD_INSTANCE_UID, instanceUid)); + if (deleted != null) { + clusterEventBus.post(new CollectorInstanceCertsChangedEvent(deleted.allCertFingerprints())); + } + + return deleted != null; } + /** + * Deletes collector instances whose {@code last_seen} is older than {@code expirationThreshold}, + * one at a time via {@link com.mongodb.client.MongoCollection#findOneAndDelete}, and publishes the + * deleted certificate fingerprints in {@link CollectorInstanceCertsChangedEvent}s, + * batched by {@link #REVOCATION_EVENT_BATCH_SIZE}. Deletes at most {@link #MAX_DELETIONS_PER_RUN} + * instances per call. + * + * @return the number of instances deleted + */ public long deleteExpired(Duration expirationThreshold) { final Date cutoff = Date.from(Instant.now(clock).minus(expirationThreshold)); - return collection.deleteMany(Filters.lt(FIELD_LAST_SEEN, cutoff)).getDeletedCount(); + + final var coll = mongoCollections.nonEntityCollection(COLLECTION_NAME, DeletedInstance.class); + + final var deleted = new ArrayList(); + + while (deleted.size() < MAX_DELETIONS_PER_RUN) { // safety net + final var deletedInstance = coll.findOneAndDelete(Filters.lt(FIELD_LAST_SEEN, cutoff), + new FindOneAndDeleteOptions().projection( + Projections.include( + FIELD_ID, + FIELD_PREVIOUS_CERTIFICATE_FINGERPRINT, + FIELD_ACTIVE_CERTIFICATE_FINGERPRINT, + FIELD_NEXT_CERTIFICATE_FINGERPRINT)) + ); + + if (deletedInstance == null) { + break; // We are done. There are no more expired instances. + } + + deleted.add(deletedInstance); + } + + if (deleted.size() == MAX_DELETIONS_PER_RUN) { + LOG.warn("Exceptionally high number of expired Collectors. Expiration cleanup might not be able to keep up."); + } + + final var revokedCertFingerprints = deleted.stream() + .flatMap(DeletedInstance::allCertFingerprints) + .collect(Collectors.toSet()); + + for (var batch : Iterables.partition(revokedCertFingerprints, REVOCATION_EVENT_BATCH_SIZE)) { + clusterEventBus.post(new CollectorInstanceCertsChangedEvent(Set.copyOf(batch))); + } + return deleted.size(); + } + + /** + * Streams collector instances last seen at or after {@code now - expirationThreshold} (i.e. not yet + * expired), most-recently-seen first. The caller must close the returned stream. + */ + @MustBeClosed + public Stream streamAllUnexpired(Duration expirationThreshold) { + final Date cutoff = Date.from(Instant.now(clock).minus(expirationThreshold)); + return MongoUtils.stream( + collection.find(Filters.gte(FIELD_LAST_SEEN, cutoff)) + .sort(Sorts.descending(FIELD_LAST_SEEN))); + } + + private record DeletedInstance(@Id @JsonProperty(FIELD_ID) String id, + @JsonProperty(FIELD_PREVIOUS_CERTIFICATE_FINGERPRINT) String previousFingerprint, + @JsonProperty(FIELD_ACTIVE_CERTIFICATE_FINGERPRINT) String activeFingerprint, + @JsonProperty(FIELD_NEXT_CERTIFICATE_FINGERPRINT) String nextFingerprint) { + public Stream allCertFingerprints() { + return Stream.of( + Optional.ofNullable(previousFingerprint()), + Optional.of(activeFingerprint()), + Optional.ofNullable(nextFingerprint()) + ).flatMap(Optional::stream); + } } public PaginatedList findPaginated(Bson query, DbSortResolver.ResolvedSort resolvedSort, @@ -437,4 +646,5 @@ public long offline() { return total() - online(); } } + } diff --git a/graylog2-server/src/main/java/org/graylog/collectors/CollectorTLSUtils.java b/graylog2-server/src/main/java/org/graylog/collectors/CollectorTLSUtils.java index 93e8e746d267..da934a3a478c 100644 --- a/graylog2-server/src/main/java/org/graylog/collectors/CollectorTLSUtils.java +++ b/graylog2-server/src/main/java/org/graylog/collectors/CollectorTLSUtils.java @@ -16,21 +16,30 @@ */ package org.graylog.collectors; +import io.netty.buffer.ByteBufAllocator; import io.netty.handler.ssl.ClientAuth; import io.netty.handler.ssl.SslContextBuilder; +import io.netty.handler.ssl.SslHandler; import io.netty.handler.ssl.SslProvider; import jakarta.inject.Inject; import jakarta.inject.Singleton; +import javax.net.ssl.SSLException; +import java.util.concurrent.Executor; + @Singleton public class CollectorTLSUtils { private final CollectorCaKeyManager keyManager; private final CollectorCaTrustManager trustManager; + private final Executor certVerificationExecutor; @Inject - public CollectorTLSUtils(CollectorCaKeyManager keyManager, CollectorCaTrustManager trustManager) { + public CollectorTLSUtils(CollectorCaKeyManager keyManager, + CollectorCaTrustManager trustManager, + @CollectorCertVerificationExecutor Executor certVerificationExecutor) { this.keyManager = keyManager; this.trustManager = trustManager; + this.certVerificationExecutor = certVerificationExecutor; } /** @@ -59,4 +68,21 @@ public SslContextBuilder newServerSslContextBuilder() { throw new RuntimeException("Failed to create OTLP server SSL context", e); } } + + /** + * Creates an {@link SslHandler} for the OTLP server endpoint whose TLS handshake delegated + * tasks run on the {@link CollectorCertVerificationExecutor} rather than the Netty event loop. + *

+ * The JDK {@code SSLEngine} performs client-certificate validation + * ({@link CollectorCaTrustManager#checkClientTrusted}) as a delegated task; passing the executor + * to {@code newHandler} makes Netty run those tasks — including any MongoDB-backed instance + * binding lookup — off the event loop. Prefer this over building the context and creating the + * handler directly, so the executor is never accidentally omitted. + * + * @param alloc the allocator for the new handler + * @return an {@link SslHandler} configured for mTLS with off-loop handshake task execution + */ + public SslHandler newServerSslHandler(ByteBufAllocator alloc) throws SSLException { + return newServerSslContextBuilder().build().newHandler(alloc, certVerificationExecutor); + } } diff --git a/graylog2-server/src/main/java/org/graylog/collectors/CollectorsConfig.java b/graylog2-server/src/main/java/org/graylog/collectors/CollectorsConfig.java index 2c719b54404f..cd93f9f4992c 100644 --- a/graylog2-server/src/main/java/org/graylog/collectors/CollectorsConfig.java +++ b/graylog2-server/src/main/java/org/graylog/collectors/CollectorsConfig.java @@ -39,6 +39,7 @@ public abstract class CollectorsConfig { private static final Duration DEFAULT_COLLECTOR_CERT_LIFETIME = Duration.ofDays(365); private static final Duration DEFAULT_COLLECTOR_HEARTBEAT_INTERVAL = Duration.ofSeconds(30); private static final int DEFAULT_OPAMP_MAX_REQUEST_BODY_SIZE_BYTES = Ints.saturatedCast(Size.megabytes(10).toBytes()); + private static final Duration DEFAULT_COLLECTOR_CERT_ROTATION_GRACE_PERIOD = Duration.ofMinutes(5); public static final String FIELD_CA_CERT_ID = "ca_cert_id"; public static final String FIELD_SIGNING_CERT_ID = "signing_cert_id"; @@ -51,6 +52,7 @@ public abstract class CollectorsConfig { public static final String FIELD_COLLECTOR_CERT_LIFETIME = "collector_cert_lifetime"; public static final String FIELD_COLLECTOR_HEARTBEAT_INTERVAL = "collector_heartbeat_interval"; public static final String FIELD_OPAMP_MAX_REQUEST_BODY_SIZE_BYTES = "opamp_max_request_body_size_bytes"; + public static final String FIELD_COLLECTOR_CERT_ROTATION_GRACE_PERIOD = "collector_cert_rotation_grace_period"; @JsonProperty(FIELD_CA_CERT_ID) @Nullable @@ -91,6 +93,9 @@ public abstract class CollectorsConfig { @JsonProperty(FIELD_OPAMP_MAX_REQUEST_BODY_SIZE_BYTES) public abstract int opampMaxRequestBodySizeBytes(); + @JsonProperty(FIELD_COLLECTOR_CERT_ROTATION_GRACE_PERIOD) + public abstract Duration collectorCertRotationGracePeriod(); + public static Builder createDefaultBuilder(String hostname) { requireNonBlank(hostname, "hostname can't be blank"); @@ -118,7 +123,8 @@ public static Builder create() { .collectorExpirationThreshold(DEFAULT_EXPIRATION_THRESHOLD) .collectorCertLifetime(DEFAULT_COLLECTOR_CERT_LIFETIME) .collectorHeartbeatInterval(DEFAULT_COLLECTOR_HEARTBEAT_INTERVAL) - .opampMaxRequestBodySizeBytes(DEFAULT_OPAMP_MAX_REQUEST_BODY_SIZE_BYTES); + .opampMaxRequestBodySizeBytes(DEFAULT_OPAMP_MAX_REQUEST_BODY_SIZE_BYTES) + .collectorCertRotationGracePeriod(DEFAULT_COLLECTOR_CERT_ROTATION_GRACE_PERIOD); } @JsonProperty(FIELD_CA_CERT_ID) @@ -154,6 +160,9 @@ public static Builder create() { @JsonProperty(FIELD_OPAMP_MAX_REQUEST_BODY_SIZE_BYTES) public abstract Builder opampMaxRequestBodySizeBytes(int opampMaxRequestBodySizeBytes); + @JsonProperty(FIELD_COLLECTOR_CERT_ROTATION_GRACE_PERIOD) + public abstract Builder collectorCertRotationGracePeriod(Duration collectorCertRotationGracePeriod); + public abstract CollectorsConfig build(); } } diff --git a/graylog2-server/src/main/java/org/graylog/collectors/CollectorsModule.java b/graylog2-server/src/main/java/org/graylog/collectors/CollectorsModule.java index efab68da0187..e6f4572d732b 100644 --- a/graylog2-server/src/main/java/org/graylog/collectors/CollectorsModule.java +++ b/graylog2-server/src/main/java/org/graylog/collectors/CollectorsModule.java @@ -16,9 +16,15 @@ */ package org.graylog.collectors; +import com.codahale.metrics.InstrumentedExecutorService; +import com.codahale.metrics.MetricRegistry; +import com.google.common.util.concurrent.ThreadFactoryBuilder; +import com.google.inject.Provides; import com.google.inject.Scopes; +import com.google.inject.assistedinject.FactoryModuleBuilder; import com.google.inject.multibindings.MapBinder; import com.google.inject.multibindings.Multibinder; +import jakarta.inject.Singleton; import org.graylog.collectors.cloud.CloudCollectorIngestService; import org.graylog.collectors.config.receiver.FilelogReceiverConfig; import org.graylog.collectors.config.receiver.JournaldReceiverConfig; @@ -38,6 +44,7 @@ import org.graylog.collectors.input.processor.JournaldRecordProcessor; import org.graylog.collectors.input.processor.LogRecordProcessor; import org.graylog.collectors.input.processor.WindowsEventLogRecordProcessor; +import org.graylog.collectors.input.transport.CollectorIngestHttpHandler; import org.graylog.collectors.input.transport.CollectorIngestHttpTransport; import org.graylog.collectors.opamp.OpAmpModule; import org.graylog.collectors.periodical.CollectorCaRenewalPeriodical; @@ -53,6 +60,15 @@ import org.graylog2.indexer.template.IndexTemplateProvider; import org.graylog2.plugin.PluginModule; +import java.util.concurrent.BlockingQueue; +import java.util.concurrent.Executor; +import java.util.concurrent.LinkedBlockingQueue; +import java.util.concurrent.ThreadFactory; +import java.util.concurrent.ThreadPoolExecutor; + +import static com.codahale.metrics.MetricRegistry.name; +import static java.util.concurrent.TimeUnit.SECONDS; + public class CollectorsModule extends PluginModule { private static final String OTLP_DUMP_FLAG = "collector_otlp_traffic_dump"; public static final String COLLECTORS_FLAG = "collectors"; @@ -87,6 +103,8 @@ protected void configure() { addMessageInput(CollectorIngestHttpInput.class); addTransport(CollectorIngestHttpTransport.NAME, CollectorIngestHttpTransport.class); addCodec(CollectorIngestCodec.NAME, CollectorIngestCodec.class); + install(new FactoryModuleBuilder().build(CollectorIngestHttpHandler.Factory.class)); + addInitializer(CertBindingResolver.class); if (isCloud) { serviceBinder().addBinding().to(CloudCollectorIngestService.class).in(Scopes.SINGLETON); @@ -151,4 +169,54 @@ protected void configure() { addTelemetryMetricProvider("Collector Metrics", CollectorMetricsSupplier.class); } + + /** + * Executor for collector mTLS certificate verification (see {@link CollectorCertVerificationExecutor}). + *

+ * Handshakes are latency-sensitive work someone waits on, so overload must shed: with pool + * and bounded queue full, {@code execute} rejects, failing the TLS handshake so the collector retries + * with backoff — bounded admission instead of latency-unbounded queueing. The fixed thread count also + * caps concurrent MongoDB lookups during a reconnect storm. + */ + @Provides + @Singleton + @CollectorCertVerificationExecutor + Executor collectorCertVerificationExecutor(MetricRegistry metricRegistry) { + final var maxThreads = Math.max(2, Runtime.getRuntime().availableProcessors() / 2); + final ThreadFactory threadFactory = new ThreadFactoryBuilder() + .setNameFormat("collector-cert-verification-%d") + .setDaemon(true) + .build(); + final BlockingQueue queue = new LinkedBlockingQueue<>(1024); + final ThreadPoolExecutor executor = new ThreadPoolExecutor(maxThreads, maxThreads, 60L, SECONDS, queue, + threadFactory); + executor.allowCoreThreadTimeOut(true); + return new InstrumentedExecutorService(executor, metricRegistry, + name("collector-cert-verification", "executor-service")); + } + + /** + * Executor for the {@link CertBindingResolver}'s background cache work — refreshes and prewarm + * (see {@link CollectorCertCacheRefreshExecutor} for why it must not share the verification pool). + *

+ * Nobody waits on these tasks, so the overload policy is the inverse of the verification pool's: + * queue instead of shed. The queue is unbounded but intrinsically capped by Caffeine's + * per-key reload coalescing, and the fixed two-thread pool is the throttle towards MongoDB. + */ + @Provides + @Singleton + @CollectorCertCacheRefreshExecutor + Executor collectorCertCacheRefreshExecutor(MetricRegistry metricRegistry) { + final var maxThreads = 2; + final ThreadFactory threadFactory = new ThreadFactoryBuilder() + .setNameFormat("collector-cert-refresh-%d") + .setDaemon(true) + .build(); + final BlockingQueue queue = new LinkedBlockingQueue<>(); + final ThreadPoolExecutor executor = new ThreadPoolExecutor(maxThreads, maxThreads, 60L, SECONDS, queue, + threadFactory); + executor.allowCoreThreadTimeOut(true); + return new InstrumentedExecutorService(executor, metricRegistry, + name("collector-cert-cache-refresh", "executor-service")); + } } diff --git a/graylog2-server/src/main/java/org/graylog/collectors/db/CollectorInstanceDTO.java b/graylog2-server/src/main/java/org/graylog/collectors/db/CollectorInstanceDTO.java index 4ce1159131e3..bb876d03f1b4 100644 --- a/graylog2-server/src/main/java/org/graylog/collectors/db/CollectorInstanceDTO.java +++ b/graylog2-server/src/main/java/org/graylog/collectors/db/CollectorInstanceDTO.java @@ -17,6 +17,7 @@ package org.graylog.collectors.db; import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonInclude; import com.fasterxml.jackson.annotation.JsonProperty; import com.fasterxml.jackson.databind.annotation.JsonDeserialize; import com.fasterxml.jackson.databind.annotation.JsonSerialize; @@ -29,9 +30,13 @@ import java.time.Instant; import java.util.List; import java.util.Optional; +import java.util.Set; +import java.util.stream.Collectors; +import java.util.stream.Stream; @AutoValue @JsonDeserialize(builder = CollectorInstanceDTO.Builder.class) +@JsonInclude(JsonInclude.Include.NON_ABSENT) public abstract class CollectorInstanceDTO implements BuildableMongoEntity { public static final String FIELD_INSTANCE_UID = "instance_uid"; public static final String FIELD_MESSAGE_SEQ_NUM = "message_seq_num"; @@ -44,6 +49,10 @@ public abstract class CollectorInstanceDTO implements BuildableMongoEntity nextCertificateExpiresAt(); + @JsonProperty(FIELD_PREVIOUS_CERTIFICATE_FINGERPRINT) + public abstract Optional previousCertificateFingerprint(); + + @JsonProperty(FIELD_PREVIOUS_CERTIFICATE_PEM) + public abstract Optional previousCertificatePem(); + + @JsonProperty(FIELD_PREVIOUS_CERTIFICATE_EXPIRES_AT) + @JsonSerialize(contentUsing = MongoInstantSerializer.class) + @JsonDeserialize(contentUsing = MongoInstantDeserializer.class) + public abstract Optional previousCertificateExpiresAt(); + + @JsonProperty(FIELD_CERTIFICATES_ROTATED_AT) + @JsonSerialize(contentUsing = MongoInstantSerializer.class) + @JsonDeserialize(contentUsing = MongoInstantDeserializer.class) + public abstract Optional certificatesRotatedAt(); + @JsonProperty(FIELD_ISSUING_CA_ID) public abstract String issuingCaId(); @@ -115,6 +140,14 @@ public static Builder builder() { return AutoValue_CollectorInstanceDTO.Builder.create(); } + public Set allCertFingerprints() { + return Stream.of( + previousCertificateFingerprint(), + Optional.of(activeCertificateFingerprint()), + nextCertificateFingerprint() + ).flatMap(Optional::stream).collect(Collectors.toSet()); + } + @AutoValue.Builder public abstract static class Builder implements BuildableMongoEntity.Builder { @@ -159,6 +192,18 @@ public static Builder create() { @JsonProperty(FIELD_NEXT_CERTIFICATE_EXPIRES_AT) public abstract Builder nextCertificateExpiresAt(@Nullable Instant nextCertificateExpiresAt); + @JsonProperty(FIELD_PREVIOUS_CERTIFICATE_FINGERPRINT) + public abstract Builder previousCertificateFingerprint(@Nullable String previousCertificateFingerprint); + + @JsonProperty(FIELD_PREVIOUS_CERTIFICATE_PEM) + public abstract Builder previousCertificatePem(@Nullable String previousCertificatePem); + + @JsonProperty(FIELD_PREVIOUS_CERTIFICATE_EXPIRES_AT) + public abstract Builder previousCertificateExpiresAt(@Nullable Instant previousCertificateExpiresAt); + + @JsonProperty(FIELD_CERTIFICATES_ROTATED_AT) + public abstract Builder certificatesRotatedAt(@Nullable Instant certificatesRotatedAt); + @JsonProperty(FIELD_ISSUING_CA_ID) public abstract Builder issuingCaId(String issuingCaId); diff --git a/graylog2-server/src/main/java/org/graylog/collectors/events/CollectorInstanceCertsChangedEvent.java b/graylog2-server/src/main/java/org/graylog/collectors/events/CollectorInstanceCertsChangedEvent.java new file mode 100644 index 000000000000..95625f3940c3 --- /dev/null +++ b/graylog2-server/src/main/java/org/graylog/collectors/events/CollectorInstanceCertsChangedEvent.java @@ -0,0 +1,36 @@ +/* + * Copyright (C) 2020 Graylog, Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the Server Side Public License, version 1, + * as published by MongoDB, Inc. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * Server Side Public License for more details. + * + * You should have received a copy of the Server Side Public License + * along with this program. If not, see + * . + */ +package org.graylog.collectors.events; + +import com.fasterxml.jackson.annotation.JsonProperty; + +import java.util.Objects; +import java.util.Set; + +/** + * Cluster event signalling that a collector instance operation touched a set of certificate fingerprints + * (enrollment, renewal insertion/activation, re-enrollment, or deletion). Subscribers re-resolve the + * affected fingerprints against current state. The event carries the fingerprints only, not their new + * binding — the resolver is the source of truth for what each fingerprint now maps to (or that it no + * longer maps to anything). + */ +public record CollectorInstanceCertsChangedEvent( + @JsonProperty("fingerprints") Set fingerprints) { + public CollectorInstanceCertsChangedEvent { + Objects.requireNonNull(fingerprints, "fingerprints must not be null"); + } +} diff --git a/graylog2-server/src/main/java/org/graylog/collectors/input/transport/AgentCertChannelHandler.java b/graylog2-server/src/main/java/org/graylog/collectors/input/transport/AgentCertChannelHandler.java index 94443502cf7d..607adc1b2761 100644 --- a/graylog2-server/src/main/java/org/graylog/collectors/input/transport/AgentCertChannelHandler.java +++ b/graylog2-server/src/main/java/org/graylog/collectors/input/transport/AgentCertChannelHandler.java @@ -29,18 +29,18 @@ import java.security.cert.X509Certificate; /** - * A Netty {@link ChannelInboundHandlerAdapter} that extracts the agent's instance_uid from the - * client certificate CN after TLS handshake and stores it as a channel attribute. + * A Netty {@link ChannelInboundHandlerAdapter} that computes the fingerprint of the agent's + * client certificate after TLS handshake and stores it as a channel attribute. *

* This handler listens for {@link SslHandshakeCompletionEvent} and, on success, retrieves the - * peer certificate from the {@link SslHandler}'s SSL session. The extracted CN is stored under - * {@link #AGENT_INSTANCE_UID} for later handlers to read on a per-request basis. + * peer certificate from the {@link SslHandler}'s SSL session. The fingerprint is stored under + * {@link #AGENT_CERT_FINGERPRINT} for later handlers to read on a per-request basis. */ public class AgentCertChannelHandler extends ChannelInboundHandlerAdapter { private static final Logger LOG = LoggerFactory.getLogger(AgentCertChannelHandler.class); - public static final AttributeKey AGENT_INSTANCE_UID = - AttributeKey.valueOf("agent-instance-uid"); + public static final AttributeKey AGENT_CERT_FINGERPRINT = + AttributeKey.valueOf("agent-cert-fingerprint"); @Override public void userEventTriggered(ChannelHandlerContext ctx, Object evt) throws Exception { @@ -49,9 +49,9 @@ public void userEventTriggered(ChannelHandlerContext ctx, Object evt) throws Exc final SSLSession session = sslHandler.engine().getSession(); try { final X509Certificate cert = (X509Certificate) session.getPeerCertificates()[0]; - ctx.channel().attr(AGENT_INSTANCE_UID).set(PemUtils.extractCn(cert)); + ctx.channel().attr(AGENT_CERT_FINGERPRINT).set(PemUtils.computeFingerprint(cert)); } catch (Exception e) { - LOG.warn("Failed to extract agent identity from client certificate", e); + LOG.warn("Failed to compute fingerprint for client certificate", e); } } ctx.fireUserEventTriggered(evt); diff --git a/graylog2-server/src/main/java/org/graylog/collectors/input/transport/CollectorIngestHttpHandler.java b/graylog2-server/src/main/java/org/graylog/collectors/input/transport/CollectorIngestHttpHandler.java index e37946eb5e8b..cc240d7c4bda 100644 --- a/graylog2-server/src/main/java/org/graylog/collectors/input/transport/CollectorIngestHttpHandler.java +++ b/graylog2-server/src/main/java/org/graylog/collectors/input/transport/CollectorIngestHttpHandler.java @@ -16,6 +16,8 @@ */ package org.graylog.collectors.input.transport; +import com.google.inject.assistedinject.Assisted; +import com.google.inject.assistedinject.AssistedInject; import io.netty.buffer.Unpooled; import io.netty.channel.ChannelFutureListener; import io.netty.channel.ChannelHandlerContext; @@ -29,11 +31,12 @@ import io.netty.handler.codec.http.HttpUtil; import io.netty.handler.codec.http.HttpVersion; import io.opentelemetry.proto.collector.logs.v1.ExportLogsServiceRequest; +import org.graylog.collectors.CertBindingResolver; import org.graylog.collectors.input.CollectorJournalRecordFactory; -import org.graylog2.shared.utilities.RecordSizeDistributingProcessor; import org.graylog.inputs.otel.transport.OtlpHttpUtils; import org.graylog2.plugin.inputs.MessageInput; import org.graylog2.plugin.journal.RawMessage; +import org.graylog2.shared.utilities.RecordSizeDistributingProcessor; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -55,9 +58,16 @@ public class CollectorIngestHttpHandler extends SimpleChannelInboundHandler r.getOtelRecord().getLog().getLogRecord().getSerializedSize(), createRawMessage, diff --git a/graylog2-server/src/main/java/org/graylog/collectors/input/transport/CollectorIngestHttpTransport.java b/graylog2-server/src/main/java/org/graylog/collectors/input/transport/CollectorIngestHttpTransport.java index a0824c1efb25..684b0f0e1b0a 100644 --- a/graylog2-server/src/main/java/org/graylog/collectors/input/transport/CollectorIngestHttpTransport.java +++ b/graylog2-server/src/main/java/org/graylog/collectors/input/transport/CollectorIngestHttpTransport.java @@ -21,7 +21,6 @@ import io.netty.buffer.PooledByteBufAllocator; import io.netty.channel.ChannelHandler; import io.netty.channel.EventLoopGroup; -import io.netty.handler.ssl.SslContext; import jakarta.inject.Inject; import jakarta.inject.Named; import org.graylog.collectors.CollectorCaService; @@ -34,7 +33,6 @@ import org.graylog2.inputs.transports.netty.EventLoopGroupFactory; import org.graylog2.plugin.LocalMetricRegistry; import org.graylog2.plugin.configuration.Configuration; -import org.graylog2.security.encryption.EncryptedValueService; import org.graylog2.plugin.configuration.ConfigurationRequest; import org.graylog2.plugin.configuration.fields.BooleanField; import org.graylog2.plugin.configuration.fields.ConfigurationField; @@ -44,6 +42,7 @@ import org.graylog2.plugin.inputs.annotations.FactoryClass; import org.graylog2.plugin.inputs.transports.Transport; import org.graylog2.plugin.inputs.util.ThroughputCounter; +import org.graylog2.security.encryption.EncryptedValueService; import org.graylog2.utilities.IpSubnet; import java.util.HashMap; @@ -66,6 +65,7 @@ public class CollectorIngestHttpTransport extends AbstractHttpTransport { static final int DEFAULT_HTTP_PORT = 14401; private final CollectorTLSUtils tlsUtils; + private final CollectorIngestHttpHandler.Factory httpHandlerFactory; @AssistedInject public CollectorIngestHttpTransport(@Assisted Configuration configuration, @@ -77,26 +77,28 @@ public CollectorIngestHttpTransport(@Assisted Configuration configuration, TLSProtocolsConfiguration tlsConfiguration, @Named("trusted_proxies") Set trustedProxies, CollectorTLSUtils tlsUtils, - EncryptedValueService encryptedValueService) { - super(withTlsDefaults(configuration), eventLoopGroup, eventLoopGroupFactory, + EncryptedValueService encryptedValueService, + CollectorIngestHttpHandler.Factory httpHandlerFactory) { + super(withCustomDefaults(configuration), eventLoopGroup, eventLoopGroupFactory, nettyTransportConfiguration, throughputCounter, localMetricRegistry, tlsConfiguration, encryptedValueService, trustedProxies, OtlpHttpUtils.LOGS_PATH); this.tlsUtils = tlsUtils; + this.httpHandlerFactory = httpHandlerFactory; } - private static Configuration withTlsDefaults(Configuration userConfig) { + private static Configuration withCustomDefaults(Configuration userConfig) { final var merged = Optional.ofNullable(userConfig.getSource()).map(HashMap::new).orElse(new HashMap<>()); merged.put(CK_TLS_ENABLE, true); merged.put(CK_TLS_CLIENT_AUTH, TLS_CLIENT_AUTH_REQUIRED); + // Using a fixed idle timeout < fingerprint cache expiry (see CertBindingResolver). This makes sure that + // cert lookups in the request path are cache hits, after the TLS handshake has populated the cache. + merged.put(CK_IDLE_WRITER_TIMEOUT, 60); // seconds return new Configuration(merged); } @Override protected Callable createSslHandler(MessageInput input) { - return () -> { - final SslContext sslContext = tlsUtils.newServerSslContextBuilder().build(); - return sslContext.newHandler(PooledByteBufAllocator.DEFAULT); - }; + return () -> tlsUtils.newServerSslHandler(PooledByteBufAllocator.DEFAULT); } @Override @@ -114,7 +116,7 @@ protected LinkedHashMap> getCustomChi // Note: rawmessage-handler, codec-aggregator, and exception-logger are added downstream // by AbstractTcpTransport and are unreachable because CollectorIngestHttpHandler does not // fire messages downstream. These cannot be removed without overriding getChildChannelHandlers. - handlers.replace("http-handler", () -> new CollectorIngestHttpHandler(input)); + handlers.replace("http-handler", () -> httpHandlerFactory.create(input)); return handlers; } @@ -138,7 +140,6 @@ public static class Config extends AbstractHttpTransport.Config { CK_RECV_BUFFER_SIZE, CK_NUMBER_WORKER_THREADS, CK_MAX_CHUNK_SIZE, - CK_IDLE_WRITER_TIMEOUT, CK_TCP_KEEPALIVE ); diff --git a/graylog2-server/src/main/java/org/graylog/collectors/opamp/OpAmpService.java b/graylog2-server/src/main/java/org/graylog/collectors/opamp/OpAmpService.java index fcf90a3540c9..d772cb7b0e2e 100644 --- a/graylog2-server/src/main/java/org/graylog/collectors/opamp/OpAmpService.java +++ b/graylog2-server/src/main/java/org/graylog/collectors/opamp/OpAmpService.java @@ -277,8 +277,7 @@ private ServerToAgent handleEnrollment(AgentToServer message, OpAmpAuthContext.E "Current assignment to fleet {} will be kept.", auth.token().fleetId(), instance.instanceUid(), instance.fleetId()); } - final var enrolled = collectorInstanceService.reEnroll(instance.id(), - instance.activeCertificateFingerprint(), issuedCert, auth.token().id()); + final var enrolled = collectorInstanceService.reEnroll(instance, issuedCert, auth.token().id()); LOG.info("Re-enrolled existing collector {}. Current fleet: {}", enrolled.instanceUid(), enrolled.fleetId()); // Don't count token usage for consecutive enrollments of the same collector, but diff --git a/graylog2-server/src/test/java/org/graylog/collectors/CertBindingResolverTest.java b/graylog2-server/src/test/java/org/graylog/collectors/CertBindingResolverTest.java new file mode 100644 index 000000000000..492672690503 --- /dev/null +++ b/graylog2-server/src/test/java/org/graylog/collectors/CertBindingResolverTest.java @@ -0,0 +1,417 @@ +/* + * Copyright (C) 2020 Graylog, Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the Server Side Public License, version 1, + * as published by MongoDB, Inc. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * Server Side Public License for more details. + * + * You should have received a copy of the Server Side Public License + * along with this program. If not, see + * . + */ +package org.graylog.collectors; + +import com.google.common.eventbus.EventBus; +import org.graylog.collectors.db.CollectorInstanceDTO; +import org.graylog.collectors.events.CollectorInstanceCertsChangedEvent; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; +import org.threeten.extra.MutableClock; + +import java.time.Duration; +import java.time.Instant; +import java.util.Optional; +import java.util.Set; +import java.util.stream.Stream; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +/** + * Tests the binding resolution (which certificate slot binds, until when — cert expiry, rotation grace) + * and the caching mechanics around it (what is cached, for how long, how events refresh it, and that + * reads enforce validity locally without a reload). The instance service is mocked at the raw document + * lookup, so no MongoDB is involved. + */ +@ExtendWith(MockitoExtension.class) +class CertBindingResolverTest { + + private static final Duration GRACE_PERIOD = Duration.ofMinutes(5); + + @Mock + private CollectorsConfigService configService; + @Mock + private CollectorInstanceService instanceService; + + private EventBus eventBus; + private MutableClock clock; + private CertBindingResolver resolver; + + @BeforeEach + void setUp() { + eventBus = new EventBus(); + clock = MutableClock.epochUTC(); + // Runnable::run makes Caffeine's refresh reloads (and preWarm) run inline for deterministic tests. + // The ticker is derived from this clock, so advancing it drives the idle expiry. + resolver = new CertBindingResolver(configService, instanceService, eventBus, clock, Runnable::run); + } + + // ----- Binding resolution: which slot binds, until when ----- + + @Test + void resolvesActiveFingerprintUntilCertExpiry() { + final var instance = activeInstance("uid-1", "fp-active", clock.instant().plus(Duration.ofDays(365))); + when(instanceService.findByActiveOrNextOrPreviousFingerprint("fp-active")) + .thenReturn(Optional.of(instance)); + + assertThat(resolver.resolve("fp-active")).contains("uid-1"); + } + + @Test + void rejectsActiveFingerprintOfExpiredCert() { + // An expired certificate must not resolve even though it still occupies the active slot — the + // per-request resolution is the only post-handshake enforcement of cert expiry (TLS never + // re-validates an established connection). + final var instance = activeInstance("uid-1", "fp-active", clock.instant().minus(Duration.ofDays(1))); + when(instanceService.findByActiveOrNextOrPreviousFingerprint("fp-active")) + .thenReturn(Optional.of(instance)); + + assertThat(resolver.resolve("fp-active")).isEmpty(); + } + + @Test + void resolvesNextFingerprintUntilCertExpiry() { + final var instance = nextInstance("uid-1", "fp-next", clock.instant().plus(Duration.ofDays(30))); + when(instanceService.findByActiveOrNextOrPreviousFingerprint("fp-next")) + .thenReturn(Optional.of(instance)); + + assertThat(resolver.resolve("fp-next")).contains("uid-1"); + } + + @Test + void resolvesPreviousFingerprintOnlyWithinGraceWindow() { + stubGracePeriod(GRACE_PERIOD); + final var instance = previousInstance("uid-1", "fp-prev", clock.instant(), + clock.instant().plus(Duration.ofDays(90))); + when(instanceService.findByActiveOrNextOrPreviousFingerprint("fp-prev")) + .thenReturn(Optional.of(instance)); + + assertThat(resolver.resolve("fp-prev")).contains("uid-1"); // within grace + + clock.add(GRACE_PERIOD.plus(Duration.ofMinutes(1))); + assertThat(resolver.resolve("fp-prev")).isEmpty(); // past grace + } + + @Test + void previousBindingIsCappedByCertExpiry() { + // The superseded cert expires before the grace window ends: the earlier instant wins. + stubGracePeriod(GRACE_PERIOD); + final var instance = previousInstance("uid-1", "fp-prev", clock.instant(), + clock.instant().plus(Duration.ofMinutes(1))); + when(instanceService.findByActiveOrNextOrPreviousFingerprint("fp-prev")) + .thenReturn(Optional.of(instance)); + + assertThat(resolver.resolve("fp-prev")).contains("uid-1"); + + clock.add(Duration.ofMinutes(2)); // past cert expiry, still within grace + assertThat(resolver.resolve("fp-prev")).isEmpty(); + } + + @Test + void zeroGracePeriodCutsSupersededCertImmediately() { + // grace = 0 restores prompt-cut behavior: the binding expires at the rotation instant. + stubGracePeriod(Duration.ZERO); + final var instance = previousInstance("uid-1", "fp-prev", clock.instant(), + clock.instant().plus(Duration.ofDays(90))); + when(instanceService.findByActiveOrNextOrPreviousFingerprint("fp-prev")) + .thenReturn(Optional.of(instance)); + + assertThat(resolver.resolve("fp-prev")).isEmpty(); + } + + @Test + void previousFingerprintWithoutRotationTimestampDoesNotResolve() { + // A missing rotation timestamp (shouldn't happen) must not grant an unbounded grace. + stubGracePeriod(GRACE_PERIOD); + final var instance = previousInstance("uid-1", "fp-prev", null, + clock.instant().plus(Duration.ofDays(90))); + when(instanceService.findByActiveOrNextOrPreviousFingerprint("fp-prev")) + .thenReturn(Optional.of(instance)); + + assertThat(resolver.resolve("fp-prev")).isEmpty(); + } + + @Test + void unknownFingerprintDoesNotResolve() { + when(instanceService.findByActiveOrNextOrPreviousFingerprint("fp-unknown")) + .thenReturn(Optional.empty()); + + assertThat(resolver.resolve("fp-unknown")).isEmpty(); + } + + // ----- Caching mechanics ----- + + @Test + void resolveCachesPositiveResult() { + final var instance = activeInstance("uid-1", "fp-1", clock.instant().plus(Duration.ofDays(365))); + when(instanceService.findByActiveOrNextOrPreviousFingerprint("fp-1")).thenReturn(Optional.of(instance)); + + assertThat(resolver.resolve("fp-1")).contains("uid-1"); + assertThat(resolver.resolve("fp-1")).contains("uid-1"); + + verify(instanceService, times(1)).findByActiveOrNextOrPreviousFingerprint("fp-1"); + } + + @Test + void resolveCachesNegativeResult() { + when(instanceService.findByActiveOrNextOrPreviousFingerprint("fp-unknown")).thenReturn(Optional.empty()); + + assertThat(resolver.resolve("fp-unknown")).isEmpty(); + assertThat(resolver.resolve("fp-unknown")).isEmpty(); + + verify(instanceService, times(1)).findByActiveOrNextOrPreviousFingerprint("fp-unknown"); + } + + @Test + void resolveFailsClosedWhenLoadThrows() { + when(instanceService.findByActiveOrNextOrPreviousFingerprint("fp-err")) + .thenThrow(new RuntimeException("mongo down")); + + assertThat(resolver.resolve("fp-err")).isEmpty(); + } + + @Test + void resolveRejectsExpiredBindingWithoutReload() { + stubGracePeriod(GRACE_PERIOD); + final var instance = previousInstance("uid-prev", "prev-fp", clock.instant(), + clock.instant().plus(Duration.ofDays(90))); + when(instanceService.findByActiveOrNextOrPreviousFingerprint("prev-fp")).thenReturn(Optional.of(instance)); + + assertThat(resolver.resolve("prev-fp")).contains("uid-prev"); + + clock.add(Duration.ofMinutes(4)); // still within grace + assertThat(resolver.resolve("prev-fp")).contains("uid-prev"); + + // Past the grace deadline the still-cached entry keeps serving local rejects — the cut must not + // cost a reload (this is what keeps the deadline cut off the Netty event loop). + clock.add(Duration.ofMinutes(2)); // now past the deadline + assertThat(resolver.resolve("prev-fp")).isEmpty(); + assertThat(resolver.resolve("prev-fp")).isEmpty(); + + verify(instanceService, times(1)).findByActiveOrNextOrPreviousFingerprint("prev-fp"); + } + + @Test + void idleEntryExpiresAndReloadsOnNextResolve() { + final var instance = activeInstance("uid-idle", "fp-idle", clock.instant().plus(Duration.ofDays(365))); + when(instanceService.findByActiveOrNextOrPreviousFingerprint("fp-idle")).thenReturn(Optional.of(instance)); + + assertThat(resolver.resolve("fp-idle")).contains("uid-idle"); + + clock.add(Duration.ofMinutes(61)); // exceeds IDLE_EXPIRY without any access + assertThat(resolver.resolve("fp-idle")).contains("uid-idle"); + + verify(instanceService, times(2)).findByActiveOrNextOrPreviousFingerprint("fp-idle"); + } + + @Test + void touchedFingerprintThatIsCachedIsReResolved() { + final var instance = activeInstance("uid-1", "fp-1", clock.instant().plus(Duration.ofDays(365))); + when(instanceService.findByActiveOrNextOrPreviousFingerprint("fp-1")).thenReturn(Optional.of(instance)); + assertThat(resolver.resolve("fp-1")).contains("uid-1"); + + // The instance is deleted: the fingerprint no longer binds. A certs-changed event touching the + // still-cached fingerprint re-resolves it (to unbound). + when(instanceService.findByActiveOrNextOrPreviousFingerprint("fp-1")).thenReturn(Optional.empty()); + resolver.handleCertsChanged(new CollectorInstanceCertsChangedEvent(Set.of("fp-1"))); + + assertThat(resolver.resolve("fp-1")).isEmpty(); + verify(instanceService, times(2)).findByActiveOrNextOrPreviousFingerprint("fp-1"); + } + + @Test + void refreshInstallsGraceStampOnLiveEntry() { + // Rotation scenario: the fingerprint is cached as a plain active binding, then rotation demotes it + // to the previous slot — the certs-changed refresh must re-resolve the live entry so it picks up + // the grace deadline, and the subsequent cut must again be reload-free. + final var active = activeInstance("uid-rot", "fp-rot", clock.instant().plus(Duration.ofDays(365))); + when(instanceService.findByActiveOrNextOrPreviousFingerprint("fp-rot")).thenReturn(Optional.of(active)); + assertThat(resolver.resolve("fp-rot")).contains("uid-rot"); + + stubGracePeriod(GRACE_PERIOD); + final var demoted = previousInstance("uid-rot", "fp-rot", clock.instant(), + clock.instant().plus(Duration.ofDays(90))); + when(instanceService.findByActiveOrNextOrPreviousFingerprint("fp-rot")).thenReturn(Optional.of(demoted)); + resolver.handleCertsChanged(new CollectorInstanceCertsChangedEvent(Set.of("fp-rot"))); + + assertThat(resolver.resolve("fp-rot")).contains("uid-rot"); // still within grace + + clock.add(GRACE_PERIOD.plus(Duration.ofMinutes(1))); + assertThat(resolver.resolve("fp-rot")).isEmpty(); + verify(instanceService, times(2)).findByActiveOrNextOrPreviousFingerprint("fp-rot"); // load + refresh + } + + @Test + void failedReResolveServesStaleUntilRefreshSucceeds() { + final var instance = activeInstance("uid-1", "fp-1", clock.instant().plus(Duration.ofDays(365))); + // Call sequence: initial load binds; the event-driven re-resolve hits a Mongo outage; the + // later refreshAfterWrite re-resolve succeeds against recovered Mongo (instance revoked). + when(instanceService.findByActiveOrNextOrPreviousFingerprint("fp-1")) + .thenReturn(Optional.of(instance)) + .thenThrow(new RuntimeException("mongo down")) + .thenReturn(Optional.empty()); + assertThat(resolver.resolve("fp-1")).contains("uid-1"); + + // The certs-changed event says this fingerprint's binding changed (the instance was revoked), + // but the re-resolve fails. Serve-stale: the last authoritative state keeps being served — the + // cut must not depend on Mongo being up at the moment of the event. + resolver.handleCertsChanged(new CollectorInstanceCertsChangedEvent(Set.of("fp-1"))); + assertThat(resolver.resolve("fp-1")).contains("uid-1"); + + // Mongo recovers. The failed refresh did not renew the entry's write timestamp, so once the + // entry is older than the refreshAfterWrite interval, an access re-resolves it and the missed + // revocation is applied — the bounded self-healing that makes serve-stale safe. (The value the + // triggering access itself returns depends on executor timing; the inline test executor + // completes the reload immediately, so only the post-trigger state is asserted.) + clock.add(Duration.ofMinutes(31)); // past refreshAfterWrite (30m), within idle expiry (60m) + resolver.resolve("fp-1"); // triggers the re-resolve + assertThat(resolver.resolve("fp-1")).isEmpty(); // revocation applied + + verify(instanceService, times(3)).findByActiveOrNextOrPreviousFingerprint("fp-1"); + } + + @Test + void staleEntryIsReResolvedOnAccessAfterRefreshInterval() { + final var instance = activeInstance("uid-1", "fp-1", clock.instant().plus(Duration.ofDays(365))); + when(instanceService.findByActiveOrNextOrPreviousFingerprint("fp-1")).thenReturn(Optional.of(instance)); + assertThat(resolver.resolve("fp-1")).contains("uid-1"); + + // The instance is revoked but the certs-changed event never arrives (lost cluster event). + // refreshAfterWrite is the backstop: an access past the interval re-resolves the entry. + when(instanceService.findByActiveOrNextOrPreviousFingerprint("fp-1")).thenReturn(Optional.empty()); + clock.add(Duration.ofMinutes(31)); // past refreshAfterWrite (30m), within idle expiry (60m) + resolver.resolve("fp-1"); // triggers the background re-resolve + + assertThat(resolver.resolve("fp-1")).isEmpty(); + verify(instanceService, times(2)).findByActiveOrNextOrPreviousFingerprint("fp-1"); + } + + @Test + void touchedFingerprintThatIsNotCachedIsNotLoaded() { + // fp-2 is not cached, so a certs-changed event must NOT proactively load it (avoids loading the + // many fingerprints of expired instances that no longer exist). + resolver.handleCertsChanged(new CollectorInstanceCertsChangedEvent(Set.of("fp-2"))); + verify(instanceService, never()).findByActiveOrNextOrPreviousFingerprint("fp-2"); + + // It cold-loads on first resolve instead. + final var instance = activeInstance("uid-2", "fp-2", clock.instant().plus(Duration.ofDays(365))); + when(instanceService.findByActiveOrNextOrPreviousFingerprint("fp-2")).thenReturn(Optional.of(instance)); + assertThat(resolver.resolve("fp-2")).contains("uid-2"); + verify(instanceService, times(1)).findByActiveOrNextOrPreviousFingerprint("fp-2"); + } + + @Test + void preWarmLoadsOnlyActiveFingerprints() { + final var config = CollectorsConfig.createDefault("test-host"); + when(configService.getOrDefault()).thenReturn(config); + final var instance = activeInstance("uid-pw", "active-fp", clock.instant().plus(Duration.ofDays(365))); + when(instanceService.streamAllUnexpired(config.collectorExpirationThreshold())).thenReturn(Stream.of(instance)); + + resolver.startAsync().awaitRunning(); + try { + // The active fingerprint is prewarmed: it resolves without a document lookup. + assertThat(resolver.resolve("active-fp")).contains("uid-pw"); + verify(instanceService, never()).findByActiveOrNextOrPreviousFingerprint(any()); + + // The next fingerprint is NOT prewarmed — it cold-loads on first resolve. + final var next = nextInstance("uid-pw", "next-fp", clock.instant().plus(Duration.ofDays(30))); + when(instanceService.findByActiveOrNextOrPreviousFingerprint("next-fp")).thenReturn(Optional.of(next)); + assertThat(resolver.resolve("next-fp")).contains("uid-pw"); + verify(instanceService).findByActiveOrNextOrPreviousFingerprint("next-fp"); + } finally { + resolver.stopAsync().awaitTerminated(); + } + } + + @Test + void certsChangedEventPostedToEventBusIsHandled() { + final var config = CollectorsConfig.createDefault("test-host"); + when(configService.getOrDefault()).thenReturn(config); + when(instanceService.streamAllUnexpired(any())).thenReturn(Stream.empty()); + + resolver.startAsync().awaitRunning(); + try { + final var instance = activeInstance("uid-1", "fp-1", clock.instant().plus(Duration.ofDays(365))); + when(instanceService.findByActiveOrNextOrPreviousFingerprint("fp-1")).thenReturn(Optional.of(instance)); + assertThat(resolver.resolve("fp-1")).contains("uid-1"); + + when(instanceService.findByActiveOrNextOrPreviousFingerprint("fp-1")).thenReturn(Optional.empty()); + eventBus.post(new CollectorInstanceCertsChangedEvent(Set.of("fp-1"))); + + assertThat(resolver.resolve("fp-1")).isEmpty(); + } finally { + resolver.stopAsync().awaitTerminated(); + } + } + + // ----- Fixtures ----- + + private void stubGracePeriod(Duration gracePeriod) { + when(configService.getOrDefault()).thenReturn(CollectorsConfig.createDefaultBuilder("test-host") + .collectorCertRotationGracePeriod(gracePeriod) + .build()); + } + + /** + * An instance matching the fingerprint in its active slot. Only the accessors the resolver touches on + * this code path are stubbed (Mockito strict stubbing). + */ + private CollectorInstanceDTO activeInstance(String instanceUid, String activeFp, Instant certExpiry) { + final var instance = mock(CollectorInstanceDTO.class); + when(instance.instanceUid()).thenReturn(instanceUid); + when(instance.activeCertificateFingerprint()).thenReturn(activeFp); + when(instance.activeCertificateExpiresAt()).thenReturn(certExpiry); + return instance; + } + + /** + * An instance matching the fingerprint in its previous slot ({@code rotatedAt} nullable to simulate a + * missing rotation timestamp). + */ + private CollectorInstanceDTO previousInstance(String instanceUid, String previousFp, Instant rotatedAt, + Instant certExpiry) { + final var instance = mock(CollectorInstanceDTO.class); + when(instance.instanceUid()).thenReturn(instanceUid); + when(instance.activeCertificateFingerprint()).thenReturn("fp-other-active"); + when(instance.previousCertificateFingerprint()).thenReturn(Optional.of(previousFp)); + when(instance.certificatesRotatedAt()).thenReturn(Optional.ofNullable(rotatedAt)); + when(instance.previousCertificateExpiresAt()).thenReturn(Optional.of(certExpiry)); + return instance; + } + + /** + * An instance matching the fingerprint in its next slot. + */ + private CollectorInstanceDTO nextInstance(String instanceUid, String nextFp, Instant certExpiry) { + final var instance = mock(CollectorInstanceDTO.class); + when(instance.instanceUid()).thenReturn(instanceUid); + when(instance.activeCertificateFingerprint()).thenReturn("fp-other-active"); + when(instance.previousCertificateFingerprint()).thenReturn(Optional.empty()); + when(instance.nextCertificateFingerprint()).thenReturn(Optional.of(nextFp)); + when(instance.nextCertificateExpiresAt()).thenReturn(Optional.of(certExpiry)); + return instance; + } +} diff --git a/graylog2-server/src/test/java/org/graylog/collectors/CollectorCaServiceTest.java b/graylog2-server/src/test/java/org/graylog/collectors/CollectorCaServiceTest.java index 53e5cf0dc77b..b12a38c832ec 100644 --- a/graylog2-server/src/test/java/org/graylog/collectors/CollectorCaServiceTest.java +++ b/graylog2-server/src/test/java/org/graylog/collectors/CollectorCaServiceTest.java @@ -18,6 +18,7 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.eventbus.EventBus; +import com.google.common.util.concurrent.MoreExecutors; import io.netty.handler.ssl.SslContext; import io.netty.handler.ssl.SslContextBuilder; import org.bouncycastle.asn1.ASN1OctetString; @@ -317,7 +318,8 @@ void renewCertificates_skipsWhenCaNotInitialized() { void newServerSslContextBuilder_returnsConfiguredBuilder() throws Exception { initConfig(); final var cache = new CollectorCaCache(collectorCaService, certificateService, encryptedValueService, new EventBus(), TestClocks.fixedEpoch()); - final var tlsUtils = new CollectorTLSUtils(new CollectorCaKeyManager(cache), new CollectorCaTrustManager(cache, clock)); + final var tlsUtils = new CollectorTLSUtils(new CollectorCaKeyManager(cache), + new CollectorCaTrustManager(cache, mock(CertBindingResolver.class), clock), MoreExecutors.directExecutor()); final SslContextBuilder builder = tlsUtils.newServerSslContextBuilder(); assertThat(builder).isNotNull(); diff --git a/graylog2-server/src/test/java/org/graylog/collectors/CollectorCaTrustManagerTest.java b/graylog2-server/src/test/java/org/graylog/collectors/CollectorCaTrustManagerTest.java index 93347b495501..def556075b85 100644 --- a/graylog2-server/src/test/java/org/graylog/collectors/CollectorCaTrustManagerTest.java +++ b/graylog2-server/src/test/java/org/graylog/collectors/CollectorCaTrustManagerTest.java @@ -49,6 +49,7 @@ class CollectorCaTrustManagerTest { private CertificateEntry signingCertEntry; private CollectorCaTrustManager trustManager; private CollectorCaCache caCache; + private CertBindingResolver certBindingResolver; @BeforeEach void setUp() throws Exception { @@ -64,7 +65,12 @@ void setUp() throws Exception { when(caCache.getSigning()).thenReturn(cacheEntry(signingCertEntry)); when(caCache.getCa()).thenReturn(cacheEntry(caCertEntry)); - trustManager = new CollectorCaTrustManager(caCache, TestClocks.fixedEpoch()); + // The binding check resolves the client cert fingerprint to its instance UID; the success-path + // tests use "test-agent" as the cert CN, so bind every fingerprint to that UID. + certBindingResolver = mock(CertBindingResolver.class); + when(certBindingResolver.resolve(any())).thenReturn(Optional.of("test-agent")); + + trustManager = new CollectorCaTrustManager(caCache, certBindingResolver, TestClocks.fixedEpoch()); } @Test @@ -147,7 +153,7 @@ void checkClientTrusted_rejectsExpiredCert() throws Exception { // Use a clock far in the future so the cert is expired final var futureClock = Clock.fixed(Instant.EPOCH.plus(Duration.ofDays(365)), ZoneOffset.UTC); - final var futureTrustManager = new CollectorCaTrustManager(caCache, futureClock); + final var futureTrustManager = new CollectorCaTrustManager(caCache, certBindingResolver, futureClock); assertThatThrownBy(() -> futureTrustManager.checkClientTrusted(new X509Certificate[]{expiredCert}, "Ed25519")) .isInstanceOf(CertificateException.class); diff --git a/graylog2-server/src/test/java/org/graylog/collectors/CollectorTLSUtilsIT.java b/graylog2-server/src/test/java/org/graylog/collectors/CollectorTLSUtilsIT.java deleted file mode 100644 index 55039a01fed5..000000000000 --- a/graylog2-server/src/test/java/org/graylog/collectors/CollectorTLSUtilsIT.java +++ /dev/null @@ -1,330 +0,0 @@ -/* - * Copyright (C) 2020 Graylog, Inc. - * - * This program is free software: you can redistribute it and/or modify - * it under the terms of the Server Side Public License, version 1, - * as published by MongoDB, Inc. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * Server Side Public License for more details. - * - * You should have received a copy of the Server Side Public License - * along with this program. If not, see - * . - */ -package org.graylog.collectors; - -import com.google.common.eventbus.EventBus; -import io.netty.bootstrap.ServerBootstrap; -import io.netty.buffer.Unpooled; -import io.netty.channel.Channel; -import io.netty.channel.ChannelFutureListener; -import io.netty.channel.ChannelHandlerContext; -import io.netty.channel.ChannelInitializer; -import io.netty.channel.ChannelPipeline; -import io.netty.channel.EventLoopGroup; -import io.netty.channel.MultiThreadIoEventLoopGroup; -import io.netty.channel.SimpleChannelInboundHandler; -import io.netty.channel.nio.NioIoHandler; -import io.netty.channel.socket.SocketChannel; -import io.netty.channel.socket.nio.NioServerSocketChannel; -import io.netty.handler.codec.http.DefaultFullHttpResponse; -import io.netty.handler.codec.http.FullHttpRequest; -import io.netty.handler.codec.http.HttpHeaderNames; -import io.netty.handler.codec.http.HttpObjectAggregator; -import io.netty.handler.codec.http.HttpResponseStatus; -import io.netty.handler.codec.http.HttpServerCodec; -import io.netty.handler.codec.http.HttpVersion; -import io.netty.handler.ssl.SslContext; -import org.bouncycastle.asn1.x509.KeyPurposeId; -import org.bouncycastle.asn1.x509.KeyUsage; -import org.graylog.collectors.input.transport.AgentCertChannelHandler; -import org.graylog.security.pki.CertificateBuilder; -import org.graylog.security.pki.CertificateEntry; -import org.graylog.security.pki.CertificateService; -import org.graylog.security.pki.PemUtils; -import org.graylog.testing.cluster.ClusterConfigServiceExtension; -import org.graylog.testing.mongodb.MongoDBExtension; -import org.graylog2.configuration.HttpConfiguration; -import org.graylog2.database.MongoCollections; -import org.graylog2.events.ClusterEventBus; -import org.graylog2.plugin.cluster.ClusterConfigService; -import org.graylog2.plugin.cluster.ClusterIdService; -import org.graylog2.security.TrustAllX509TrustManager; -import org.graylog2.security.encryption.EncryptedValueService; -import org.graylog2.web.customization.CustomizationConfig; -import org.junit.jupiter.api.AfterEach; -import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.extension.ExtendWith; - -import javax.net.ssl.KeyManager; -import javax.net.ssl.SSLContext; -import javax.net.ssl.SSLEngine; -import javax.net.ssl.SSLHandshakeException; -import javax.net.ssl.TrustManager; -import javax.net.ssl.X509ExtendedKeyManager; -import javax.net.ssl.X509ExtendedTrustManager; -import java.net.InetSocketAddress; -import java.net.Socket; -import java.net.URI; -import java.net.http.HttpClient; -import java.net.http.HttpRequest; -import java.net.http.HttpResponse; -import java.nio.charset.StandardCharsets; -import java.security.Principal; -import java.security.PrivateKey; -import java.security.cert.X509Certificate; -import java.time.Clock; -import java.time.Duration; -import java.util.UUID; - -import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.assertThatThrownBy; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; - -/** - * Integration test for {@link CollectorTLSUtils} with a real Netty server. - *

- * The test creates a three-level CA hierarchy (root CA → signing cert → server cert), - * wires it through mocked {@link CollectorCaService} into a real {@link CollectorCaKeyManager} and - * {@link CollectorCaTrustManager}, then verifies Ed25519 mTLS handshakes succeed end-to-end. - */ -@ExtendWith(MongoDBExtension.class) -@ExtendWith(ClusterConfigServiceExtension.class) -class CollectorTLSUtilsIT { - - private static final String AGENT_INSTANCE_UID = "test-agent-42"; - private static final Duration CERT_VALIDITY = Duration.ofDays(1); - - private PrivateKey agentKey; - private X509Certificate agentCert; - private X509Certificate signingCert; - - private final EncryptedValueService encryptedValueService = new EncryptedValueService("1234567890abcdef"); - - private CollectorCaCache caCache; - private CollectorTLSUtils tlsUtils; - private Channel serverChannel; - private EventLoopGroup bossGroup; - private EventLoopGroup workerGroup; - - @BeforeEach - void setUp(MongoCollections mongoCollections, ClusterConfigService clusterConfigService) throws Exception { - final var certBuilder = new CertificateBuilder(encryptedValueService, "Test", Clock.systemUTC()); - - final var certService = new CertificateService(mongoCollections, encryptedValueService, CustomizationConfig.empty(), Clock.systemUTC()); - final var clusterIdService = mock(ClusterIdService.class); - final var httpConfiguration = mock(HttpConfiguration.class); - when(httpConfiguration.getHttpExternalUri()).thenReturn(java.net.URI.create("https://localhost:443/")); - final var collectorsConfigService = new CollectorsConfigService(clusterConfigService, new ClusterEventBus(), httpConfiguration); - final var caService = new CollectorCaService(certService, clusterIdService, collectorsConfigService, Clock.systemUTC()); - - when(clusterIdService.getString()).thenReturn(UUID.randomUUID().toString()); - - final var hierarchy = caService.initializeCa(); - - collectorsConfigService.save(CollectorsConfig.createDefaultBuilder("localhost") - .caCertId(hierarchy.caCert().id()) - .signingCertId(hierarchy.signingCert().id()) - .otlpServerCertId(hierarchy.otlpServerCert().id()) - .build()); - - final CertificateEntry agentCertEntry = certBuilder.createEndEntityCert( - AGENT_INSTANCE_UID, - hierarchy.signingCert(), - KeyUsage.digitalSignature, - KeyPurposeId.id_kp_clientAuth, - CERT_VALIDITY - ); - - signingCert = PemUtils.parseCertificate(hierarchy.signingCert().certificate()); - agentKey = PemUtils.parsePrivateKey(encryptedValueService.decrypt(agentCertEntry.privateKey())); - agentCert = PemUtils.parseCertificate(agentCertEntry.certificate()); - - caCache = new CollectorCaCache(caService, certService, encryptedValueService, new EventBus(), Clock.systemUTC()); - caCache.startAsync().awaitRunning(); - final var keyManager = new CollectorCaKeyManager(caCache); - final var trustManager = new CollectorCaTrustManager(caCache, Clock.systemUTC()); - tlsUtils = new CollectorTLSUtils(keyManager, trustManager); - - bossGroup = new MultiThreadIoEventLoopGroup(1, NioIoHandler.newFactory()); - workerGroup = new MultiThreadIoEventLoopGroup(2, NioIoHandler.newFactory()); - } - - @AfterEach - void tearDown() throws Exception { - caCache.stopAsync().awaitTerminated(); - if (serverChannel != null) { - serverChannel.close().sync(); - } - bossGroup.shutdownGracefully().sync(); - workerGroup.shutdownGracefully().sync(); - } - - @Test - void mtlsHandshakeSucceedsWithKeyManagerProvidedCerts() throws Exception { - final int port = startServer(); - final HttpClient client = createMtlsClient(agentKey, agentCert); - - final HttpResponse response = client.send( - HttpRequest.newBuilder() - .uri(URI.create("https://127.0.0.1:" + port + "/test")) - .GET() - .build(), - HttpResponse.BodyHandlers.ofString() - ); - - assertThat(response.statusCode()).isEqualTo(200); - // AgentCertChannelHandler extracts the CN from the client cert - assertThat(response.body()).isEqualTo(AGENT_INSTANCE_UID); - } - - @Test - void mtlsRejectsConnectionWithoutClientCert() throws Exception { - final int port = startServer(); - - final SSLContext sslContext = SSLContext.getInstance("TLS"); - sslContext.init(null, new TrustManager[]{new TrustAllX509TrustManager()}, null); - final HttpClient client = HttpClient.newBuilder().sslContext(sslContext).build(); - - assertThatThrownBy(() -> client.send( - HttpRequest.newBuilder() - .uri(URI.create("https://127.0.0.1:" + port + "/test")) - .GET() - .build(), - HttpResponse.BodyHandlers.ofString() - )).hasCauseInstanceOf(SSLHandshakeException.class); - } - - private int startServer() throws Exception { - final SslContext sslContext = tlsUtils.newServerSslContextBuilder().build(); - - final ServerBootstrap bootstrap = new ServerBootstrap() - .group(bossGroup, workerGroup) - .channel(NioServerSocketChannel.class) - .childHandler(new ChannelInitializer() { - @Override - protected void initChannel(SocketChannel ch) { - final ChannelPipeline pipeline = ch.pipeline(); - pipeline.addLast("ssl", sslContext.newHandler(ch.alloc())); - pipeline.addLast("agent-cert-handler", new AgentCertChannelHandler()); - pipeline.addLast("http-codec", new HttpServerCodec()); - pipeline.addLast("http-aggregator", new HttpObjectAggregator(64 * 1024)); - pipeline.addLast("handler", new EchoAgentUidHandler()); - } - }); - - serverChannel = bootstrap.bind("127.0.0.1", 0).sync().channel(); - return ((InetSocketAddress) serverChannel.localAddress()).getPort(); - } - - private HttpClient createMtlsClient(PrivateKey clientKey, X509Certificate clientCert) throws Exception { - final X509ExtendedKeyManager km = new SimpleKeyManager(clientKey, clientCert, signingCert); - final SSLContext sslContext = SSLContext.getInstance("TLS"); - sslContext.init(new KeyManager[]{km}, new TrustManager[]{new TrustAllManager()}, null); - return HttpClient.newBuilder().sslContext(sslContext).build(); - } - - /** - * Simple HTTP handler that echoes the agent instance UID extracted by {@link AgentCertChannelHandler}, - * or "ok" if no UID path is requested at {@code /test}. - */ - private static class EchoAgentUidHandler extends SimpleChannelInboundHandler { - @Override - protected void channelRead0(ChannelHandlerContext ctx, FullHttpRequest request) { - final String uid = ctx.channel().attr(AgentCertChannelHandler.AGENT_INSTANCE_UID).get(); - final byte[] body = (uid != null ? uid : "ok").getBytes(StandardCharsets.UTF_8); - - final DefaultFullHttpResponse response = new DefaultFullHttpResponse( - HttpVersion.HTTP_1_1, HttpResponseStatus.OK, Unpooled.wrappedBuffer(body)); - response.headers().set(HttpHeaderNames.CONTENT_LENGTH, body.length); - ctx.writeAndFlush(response).addListener(ChannelFutureListener.CLOSE); - } - } - - private static class SimpleKeyManager extends X509ExtendedKeyManager { - private final PrivateKey privateKey; - private final X509Certificate[] certChain; - - SimpleKeyManager(PrivateKey privateKey, X509Certificate clientCert, X509Certificate issuerCert) { - this.privateKey = privateKey; - this.certChain = new X509Certificate[]{clientCert, issuerCert}; - } - - @Override - public String[] getClientAliases(String keyType, Principal[] issuers) { - return new String[]{"agent"}; - } - - @Override - public String chooseClientAlias(String[] keyType, Principal[] issuers, Socket socket) { - return "agent"; - } - - @Override - public String chooseEngineClientAlias(String[] keyType, Principal[] issuers, SSLEngine engine) { - return "agent"; - } - - @Override - public String[] getServerAliases(String keyType, Principal[] issuers) { - return null; - } - - @Override - public String chooseServerAlias(String keyType, Principal[] issuers, Socket socket) { - return null; - } - - @Override - public X509Certificate[] getCertificateChain(String alias) { - return certChain; - } - - @Override - public PrivateKey getPrivateKey(String alias) { - return privateKey; - } - } - - /** - * Trust manager that accepts all certificates and skips hostname verification. - *

- * Must extend {@link X509ExtendedTrustManager} (not just {@link javax.net.ssl.X509TrustManager}) - * because the JDK wraps plain X509TrustManager in AbstractTrustManagerWrapper which adds - * hostname/IP identity checks. X509ExtendedTrustManager is used directly, bypassing the wrapper. - */ - private static class TrustAllManager extends X509ExtendedTrustManager { - @Override - public void checkClientTrusted(X509Certificate[] chain, String authType) { - } - - public void checkServerTrusted(X509Certificate[] chain, String authType) { - } - - @Override - public void checkClientTrusted(X509Certificate[] chain, String authType, Socket socket) { - } - - @Override - public void checkServerTrusted(X509Certificate[] chain, String authType, Socket socket) { - } - - @Override - public void checkClientTrusted(X509Certificate[] chain, String authType, SSLEngine engine) { - } - - @Override - public void checkServerTrusted(X509Certificate[] chain, String authType, SSLEngine engine) { - } - - @Override - public X509Certificate[] getAcceptedIssuers() { - return new X509Certificate[0]; - } - } -} diff --git a/graylog2-server/src/test/java/org/graylog/collectors/input/CollectorIngestMtlsIT.java b/graylog2-server/src/test/java/org/graylog/collectors/input/CollectorIngestMtlsIT.java index a780d1b0d826..b2de5063ee4b 100644 --- a/graylog2-server/src/test/java/org/graylog/collectors/input/CollectorIngestMtlsIT.java +++ b/graylog2-server/src/test/java/org/graylog/collectors/input/CollectorIngestMtlsIT.java @@ -16,300 +16,357 @@ */ package org.graylog.collectors.input; +import com.google.common.eventbus.EventBus; +import com.google.common.util.concurrent.MoreExecutors; import io.netty.bootstrap.ServerBootstrap; import io.netty.channel.Channel; import io.netty.channel.ChannelInitializer; import io.netty.channel.ChannelPipeline; import io.netty.channel.EventLoopGroup; -import io.netty.channel.nio.NioEventLoopGroup; +import io.netty.channel.MultiThreadIoEventLoopGroup; +import io.netty.channel.nio.NioIoHandler; import io.netty.channel.socket.SocketChannel; import io.netty.channel.socket.nio.NioServerSocketChannel; import io.netty.handler.codec.http.HttpObjectAggregator; import io.netty.handler.codec.http.HttpServerCodec; -import io.netty.handler.ssl.ClientAuth; import io.netty.handler.ssl.SslContext; -import io.netty.handler.ssl.SslContextBuilder; -import io.netty.handler.ssl.SslProvider; -import io.netty.handler.ssl.util.InsecureTrustManagerFactory; import io.opentelemetry.proto.collector.logs.v1.ExportLogsServiceRequest; import io.opentelemetry.proto.common.v1.AnyValue; import io.opentelemetry.proto.logs.v1.LogRecord; import io.opentelemetry.proto.logs.v1.ResourceLogs; import io.opentelemetry.proto.logs.v1.ScopeLogs; -import org.bouncycastle.asn1.x500.X500Name; -import org.bouncycastle.asn1.x500.X500NameBuilder; -import org.bouncycastle.asn1.x500.style.BCStyle; -import org.bouncycastle.asn1.x509.BasicConstraints; -import org.bouncycastle.asn1.x509.ExtendedKeyUsage; -import org.bouncycastle.asn1.x509.Extension; -import org.bouncycastle.asn1.x509.GeneralName; -import org.bouncycastle.asn1.x509.GeneralNames; import org.bouncycastle.asn1.x509.KeyPurposeId; import org.bouncycastle.asn1.x509.KeyUsage; -import org.bouncycastle.cert.jcajce.JcaX509CertificateConverter; -import org.bouncycastle.cert.jcajce.JcaX509v3CertificateBuilder; -import org.bouncycastle.jce.provider.BouncyCastleProvider; -import org.bouncycastle.operator.ContentSigner; -import org.bouncycastle.operator.jcajce.JcaContentSignerBuilder; +import org.graylog.collectors.CollectorCaCache; +import org.graylog.collectors.CollectorCaKeyManager; +import org.graylog.collectors.CollectorCaService; +import org.graylog.collectors.CollectorCaTrustManager; +import org.graylog.collectors.CertBindingResolver; +import org.graylog.collectors.CollectorInstanceService; import org.graylog.collectors.CollectorJournal; +import org.graylog.collectors.CollectorTLSUtils; +import org.graylog.collectors.CollectorsConfig; +import org.graylog.collectors.CollectorsConfigService; import org.graylog.collectors.input.transport.AgentCertChannelHandler; import org.graylog.collectors.input.transport.CollectorIngestHttpHandler; +import org.graylog.collectors.opamp.IssuedCertificate; import org.graylog.security.pki.Algorithm; import org.graylog.security.pki.CertificateBuilder; import org.graylog.security.pki.CertificateEntry; +import org.graylog.security.pki.CertificateService; import org.graylog.security.pki.PemUtils; -import org.graylog.testing.TestClocks; +import org.graylog.testing.cluster.ClusterConfigServiceExtension; +import org.graylog.testing.mongodb.MongoDBExtension; +import org.graylog2.configuration.HttpConfiguration; +import org.graylog2.database.MongoCollections; +import org.graylog2.events.ClusterEventBus; +import org.graylog2.plugin.cluster.ClusterConfigService; +import org.graylog2.plugin.cluster.ClusterIdService; import org.graylog2.plugin.inputs.MessageInput; import org.graylog2.plugin.journal.RawMessage; import org.graylog2.security.encryption.EncryptedValueService; +import org.graylog2.web.customization.CustomizationConfig; import org.junit.jupiter.api.AfterEach; -import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.ArgumentCaptor; -import org.mockito.Mock; -import org.mockito.junit.jupiter.MockitoExtension; +import org.threeten.extra.MutableClock; import javax.net.ssl.KeyManager; import javax.net.ssl.SSLContext; import javax.net.ssl.SSLEngine; +import javax.net.ssl.SSLException; import javax.net.ssl.SSLHandshakeException; import javax.net.ssl.TrustManager; import javax.net.ssl.X509ExtendedKeyManager; -import javax.net.ssl.X509TrustManager; -import java.math.BigInteger; +import javax.net.ssl.X509ExtendedTrustManager; import java.net.InetSocketAddress; import java.net.Socket; import java.net.URI; import java.net.http.HttpClient; import java.net.http.HttpRequest; import java.net.http.HttpResponse; -import java.security.KeyPair; -import java.security.KeyPairGenerator; import java.security.Principal; import java.security.PrivateKey; -import java.security.Security; import java.security.cert.X509Certificate; +import java.time.Clock; import java.time.Duration; -import java.time.Instant; -import java.util.Date; +import java.util.UUID; +import java.util.concurrent.TimeUnit; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; /** - * Integration tests for Ed25519 mTLS over the managed HTTP ingest path. + * End-to-end integration test for the collector ingest mTLS path against the real production stack. *

- * These tests verify the remaining technical risk after removing the unpublished gRPC path: - * Ed25519 mTLS still works end-to-end for the HTTP OTLP transport. Each test starts a real - * server with TLS, creates client connections using Ed25519 certificates, and validates - * authentication and data flow. + * It initializes the collectors CA hierarchy, enrolls an agent, and wires the real + * {@link CollectorCaKeyManager}, {@link CollectorCaTrustManager} (including its active-instance + * binding), {@link CertBindingResolver}, and {@link CollectorIngestHttpHandler} into a Netty + * server. Client certificates are minted with the production {@link CertificateBuilder} + * (clientAuth EKU, signed by the real signing cert), so they are cryptographically valid and only + * the instance binding decides whether a handshake is trusted. *

- * BouncyCastle is used to generate Ed25519 certificates (JDK does not provide a - * certificate builder API). All keys and certs are parsed through {@link PemUtils} - * (same code path as production) which returns JDK-native types via SunEC. + * It verifies the security fix for the ingest mTLS path: a certificate is trusted only when it binds + * to an active, non-deleted collector instance — not merely because it was signed by the CA. It also + * covers the renewal model (the {@code next} certificate is accepted before activation; the superseded + * certificate stays accepted for a grace window after activation, then loses access) and that a + * foreign-CA certificate is rejected at the crypto gate. A trusted connection propagates the resolved + * instance UID to the journal record. *

- * Both client and server use {@code InsecureTrustManagerFactory} to bypass PKIX cert path - * validation. The server still enforces {@code ClientAuth.REQUIRE}, ensuring the client - * must present a certificate. The focus of these tests is on the mTLS handshake mechanics - * and agent identity propagation, not on cert chain validation. + * The test client uses an {@link X509ExtendedTrustManager} that accepts any server certificate, which + * also bypasses the JDK's hostname-verification wrapper — so the real OTLP server certificate is used + * as-is without needing an IP SAN for {@code 127.0.0.1}. */ -@ExtendWith(MockitoExtension.class) +@ExtendWith(MongoDBExtension.class) +@ExtendWith(ClusterConfigServiceExtension.class) class CollectorIngestMtlsIT { - private static final String AGENT_INSTANCE_UID = "test-agent-instance-123"; + private static final String AGENT_INSTANCE_UID = "test-agent-42"; + private static final String UNENROLLED_INSTANCE_UID = "unenrolled-agent-99"; private static final Duration CERT_VALIDITY = Duration.ofDays(1); - // Server cert with SAN for 127.0.0.1 - private static PrivateKey serverKey; - private static X509Certificate serverCert; + private final EncryptedValueService encryptedValueService = new EncryptedValueService("1234567890abcdef"); - // Agent cert signed by the CA - private static PrivateKey agentKey; - private static X509Certificate agentCert; + private CertificateBuilder certBuilder; + private CertificateEntry signingCertEntry; + private X509Certificate signingCert; - // CA cert (self-signed root) - private static X509Certificate caCert; + // Enrolled agent: its certificate binds to an active instance. + private PrivateKey agentKey; + private X509Certificate agentCert; - @Mock + private MutableClock clock; + private CollectorInstanceService instanceService; + private CertBindingResolver certBindingResolver; + private CollectorTLSUtils tlsUtils; + private CollectorCaCache caCache; private MessageInput input; - private Channel httpServerChannel; + private Channel serverChannel; private EventLoopGroup bossGroup; private EventLoopGroup workerGroup; - - @BeforeAll - static void setupCerts() throws Exception { - // Register BC for cert generation (JDK has no certificate builder API). - if (Security.getProvider("BC") == null) { - Security.addProvider(new BouncyCastleProvider()); - } - - final EncryptedValueService encryptedValueService = new EncryptedValueService("1234567890abcdef"); - final CertificateBuilder certificateBuilder = new CertificateBuilder(encryptedValueService, "Test", TestClocks.fixedEpoch()); - - // Create a flat CA hierarchy: self-signed root CA signs all end-entity certs - final CertificateEntry caEntry = certificateBuilder.createRootCa("Test CA", Algorithm.ED25519, CERT_VALIDITY); - final CertificateEntry agentCertEntry = certificateBuilder.createEndEntityCert(AGENT_INSTANCE_UID, caEntry, - KeyUsage.digitalSignature, KeyPurposeId.id_kp_clientAuth, CERT_VALIDITY); - - // Parse CA and agent certs via PemUtils — same code path as production - // (OpAmpCaService.newServerSslContextBuilder()) - caCert = PemUtils.parseCertificate(caEntry.certificate()); - agentKey = PemUtils.parsePrivateKey(encryptedValueService.decrypt(agentCertEntry.privateKey())); - agentCert = PemUtils.parseCertificate(agentCertEntry.certificate()); - - // Create server cert with SAN for IP 127.0.0.1 (required by hostname verification). - // CertificateBuilder only supports DNS SANs, so we build this one manually with BC. - final PrivateKey issuerKey = PemUtils.parsePrivateKey(encryptedValueService.decrypt(caEntry.privateKey())); - final KeyPair serverKeyPair = KeyPairGenerator.getInstance("Ed25519").generateKeyPair(); - final X500Name serverSubject = new X500NameBuilder(BCStyle.INSTANCE) - .addRDN(BCStyle.CN, "Test Server").addRDN(BCStyle.O, "Test").build(); - final X500Name issuerDn = new X500Name(caCert.getSubjectX500Principal().getName()); - final Instant now = Instant.now(); - final JcaX509v3CertificateBuilder certBuilder = new JcaX509v3CertificateBuilder( - issuerDn, BigInteger.valueOf(System.currentTimeMillis()), - Date.from(now), Date.from(now.plus(CERT_VALIDITY)), - serverSubject, serverKeyPair.getPublic()); - certBuilder.addExtension(Extension.basicConstraints, true, new BasicConstraints(false)); - certBuilder.addExtension(Extension.keyUsage, true, - new KeyUsage(KeyUsage.digitalSignature | KeyUsage.keyEncipherment)); - certBuilder.addExtension(Extension.extendedKeyUsage, false, - new ExtendedKeyUsage(KeyPurposeId.id_kp_serverAuth)); - certBuilder.addExtension(Extension.subjectAlternativeName, false, - new GeneralNames(new GeneralName(GeneralName.iPAddress, "127.0.0.1"))); - final ContentSigner signer = new JcaContentSignerBuilder("Ed25519") - .setProvider("BC").build(issuerKey); - serverCert = new JcaX509CertificateConverter() - .getCertificate(certBuilder.build(signer)); - serverKey = serverKeyPair.getPrivate(); - } - @BeforeEach - void setUp() { - bossGroup = new NioEventLoopGroup(1); - workerGroup = new NioEventLoopGroup(2); + void setUp(MongoCollections mongoCollections, ClusterConfigService clusterConfigService) throws Exception { + // The instance service and the fingerprint cache share a MutableClock so the renewal grace window + // can be advanced deterministically; certificate crypto validity uses the real clock. + clock = MutableClock.epochUTC(); + certBuilder = new CertificateBuilder(encryptedValueService, "Test", Clock.systemUTC()); + final var certService = new CertificateService(mongoCollections, encryptedValueService, CustomizationConfig.empty(), Clock.systemUTC()); + + final var clusterIdService = mock(ClusterIdService.class); + when(clusterIdService.getString()).thenReturn(UUID.randomUUID().toString()); + final var httpConfiguration = mock(HttpConfiguration.class); + when(httpConfiguration.getHttpExternalUri()).thenReturn(URI.create("https://localhost:443/")); + + final var collectorsConfigService = new CollectorsConfigService(clusterConfigService, new ClusterEventBus(), httpConfiguration); + final var caService = new CollectorCaService(certService, clusterIdService, collectorsConfigService, Clock.systemUTC()); + + final var hierarchy = caService.initializeCa(); + collectorsConfigService.save(CollectorsConfig.createDefaultBuilder("localhost") + .caCertId(hierarchy.caCert().id()) + .signingCertId(hierarchy.signingCert().id()) + .otlpServerCertId(hierarchy.otlpServerCert().id()) + .build()); + signingCertEntry = hierarchy.signingCert(); + signingCert = PemUtils.parseCertificate(signingCertEntry.certificate()); + + // Mint and enroll the agent so its (active) fingerprint resolves to an active instance. + final AgentCert agent = mintClientCert(AGENT_INSTANCE_UID, signingCertEntry); + agentKey = agent.key(); + agentCert = agent.cert(); + + instanceService = new CollectorInstanceService(mongoCollections, new ClusterEventBus(), clock); + instanceService.enroll(AGENT_INSTANCE_UID, "000000000000000000000000", + new IssuedCertificate(agent.entry().fingerprint(), agent.entry().certificate(), + agent.entry().notAfter(), signingCertEntry.id()), + "000000000000000000000000"); + + certBindingResolver = new CertBindingResolver(collectorsConfigService, instanceService, + new EventBus(), clock, MoreExecutors.directExecutor()); + + caCache = new CollectorCaCache(caService, certService, encryptedValueService, new EventBus(), Clock.systemUTC()); + caCache.startAsync().awaitRunning(); + final var keyManager = new CollectorCaKeyManager(caCache); + final var trustManager = new CollectorCaTrustManager(caCache, certBindingResolver, Clock.systemUTC()); + tlsUtils = new CollectorTLSUtils(keyManager, trustManager, MoreExecutors.directExecutor()); + + input = mock(MessageInput.class); + + bossGroup = new MultiThreadIoEventLoopGroup(1, NioIoHandler.newFactory()); + workerGroup = new MultiThreadIoEventLoopGroup(2, NioIoHandler.newFactory()); } @AfterEach void tearDown() throws Exception { - if (httpServerChannel != null) { - httpServerChannel.close().sync(); + caCache.stopAsync().awaitTerminated(); + if (serverChannel != null) { + serverChannel.close().sync(); } - bossGroup.shutdownGracefully().sync(); - workerGroup.shutdownGracefully().sync(); + // Zero quiet period: the no-arg default waits 2s per group for "no new tasks", which only adds + // dead time here — the server channel is already closed, nothing is in flight. + bossGroup.shutdownGracefully(0, 15, TimeUnit.SECONDS).sync(); + workerGroup.shutdownGracefully(0, 15, TimeUnit.SECONDS).sync(); } - // ----- HTTP mTLS Tests ----- - @Test - void httpMtlsSuccessWithAgentCertSignedByOpAmpCa() throws Exception { - final int port = startHttpServer(); - final HttpClient client = createHttpClient(agentKey, agentCert); + void enrolledAgentCompletesMtlsAndPropagatesInstanceUidToJournal() throws Exception { + final int port = startServer(); + final HttpClient client = createMtlsClient(agentKey, agentCert); - final ExportLogsServiceRequest request = createTestRequest(); - final HttpResponse response = client.send( - HttpRequest.newBuilder() - .uri(URI.create("https://127.0.0.1:" + port + "/v1/logs")) - .header("Content-Type", "application/x-protobuf") - .POST(HttpRequest.BodyPublishers.ofByteArray(request.toByteArray())) - .build(), - HttpResponse.BodyHandlers.ofByteArray() - ); + final HttpResponse response = postLogs(client, port); assertThat(response.statusCode()).isEqualTo(200); + + // The handshake completion event must fire before the HTTP request so AgentCertChannelHandler + // can set the fingerprint attribute; reaching the journal with the right UID proves both the + // ordering and the end-to-end binding fingerprint -> active instance UID. + assertThat(capturedJournalRecord().getCollectorInstanceUid()).isEqualTo(AGENT_INSTANCE_UID); } @Test - void httpMtlsRejectsConnectionWithNoClientCert() throws Exception { - final int port = startHttpServer(); + void rejectsConnectionWithoutClientCert() throws Exception { + final int port = startServer(); - // Build a Java SSLContext with no client cert - trust all servers final SSLContext sslContext = SSLContext.getInstance("TLS"); sslContext.init(null, new TrustManager[]{new TrustAllManager()}, null); + final HttpClient client = HttpClient.newBuilder().sslContext(sslContext).build(); - final HttpClient client = HttpClient.newBuilder() - .sslContext(sslContext) - .build(); + // The server requires client auth, so the handshake fails before any HTTP request is processed. + assertThatThrownBy(() -> postLogs(client, port)) + .hasCauseInstanceOf(SSLHandshakeException.class); + } - final ExportLogsServiceRequest request = createTestRequest(); + @Test + void rejectsCaSignedCertOfUnenrolledInstance() throws Exception { + // The cert is signed by the CA and otherwise valid, but its instance was never enrolled, so the + // trust manager's binding lookup finds no active instance and aborts the handshake. This is the + // core of the fix: a CA signature alone is not enough to be trusted. + final AgentCert unenrolled = mintClientCert(UNENROLLED_INSTANCE_UID, signingCertEntry); - // The server requires client auth so the TLS handshake should fail - assertThatThrownBy(() -> client.send( - HttpRequest.newBuilder() - .uri(URI.create("https://127.0.0.1:" + port + "/v1/logs")) - .header("Content-Type", "application/x-protobuf") - .POST(HttpRequest.BodyPublishers.ofByteArray(request.toByteArray())) - .build(), - HttpResponse.BodyHandlers.ofByteArray() - )).hasCauseInstanceOf(SSLHandshakeException.class); + final int port = startServer(); + final HttpClient client = createMtlsClient(unenrolled.key(), unenrolled.cert()); + + assertThatThrownBy(() -> postLogs(client, port)) + .hasCauseInstanceOf(SSLException.class); } @Test - void httpMtlsSetsCollectorInstanceUidInJournalRecord() throws Exception { - final int port = startHttpServer(); - final HttpClient client = createHttpClient(agentKey, agentCert); + void rejectsCertOfDeletedInstance() throws Exception { + // Delete the enrolled instance before the first handshake: its fingerprint was never cached, so + // the binding lookup resolves straight from MongoDB and finds nothing — the cert is no longer + // trusted. This is the revocation half of the fix (a removed instance loses ingest access). + assertThat(instanceService.deleteByInstanceUid(AGENT_INSTANCE_UID)).isTrue(); - final ExportLogsServiceRequest request = createTestRequest(); - final HttpResponse response = client.send( - HttpRequest.newBuilder() - .uri(URI.create("https://127.0.0.1:" + port + "/v1/logs")) - .header("Content-Type", "application/x-protobuf") - .POST(HttpRequest.BodyPublishers.ofByteArray(request.toByteArray())) - .build(), - HttpResponse.BodyHandlers.ofByteArray() - ); + final int port = startServer(); + final HttpClient client = createMtlsClient(agentKey, agentCert); - assertThat(response.statusCode()).isEqualTo(200); + assertThatThrownBy(() -> postLogs(client, port)) + .hasCauseInstanceOf(SSLException.class); + } - final ArgumentCaptor captor = ArgumentCaptor.forClass(RawMessage.class); - verify(input).processRawMessage(captor.capture()); + @Test + void acceptsNextCertificateDuringRenewal() throws Exception { + // During renewal the agent may present its freshly issued "next" certificate before it is + // activated. the binding resolver resolves the next fingerprint to the same instance, so the + // handshake is trusted and the same instance UID reaches the journal. + final AgentCert renewed = mintClientCert(AGENT_INSTANCE_UID, signingCertEntry); + assertThat(instanceService.insertNextCertificate(AGENT_INSTANCE_UID, renewed.entry().fingerprint(), + renewed.entry().certificate(), renewed.entry().notAfter())).isTrue(); + + final int port = startServer(); + final HttpClient client = createMtlsClient(renewed.key(), renewed.cert()); + + final HttpResponse response = postLogs(client, port); - final CollectorJournal.Record record = CollectorJournal.Record.parseFrom(captor.getValue().getPayload()); - assertThat(record.getCollectorInstanceUid()).isEqualTo(AGENT_INSTANCE_UID); + assertThat(response.statusCode()).isEqualTo(200); + assertThat(capturedJournalRecord().getCollectorInstanceUid()).isEqualTo(AGENT_INSTANCE_UID); } @Test - void httpSslHandshakeCompletionEventFiresBeforeHttpRequest() throws Exception { - // This test verifies that SslHandshakeCompletionEvent fires before HTTP requests, - // allowing AgentCertChannelHandler to extract the CN before CollectorIngestHttpHandler processes the request. - // If the handshake event did NOT fire, CollectorIngestHttpHandler would return 401. - final int port = startHttpServer(); - final HttpClient client = createHttpClient(agentKey, agentCert); + void acceptsSupersededCertificateWithinRenewalGraceWindow() throws Exception { + // After activation the old certificate is demoted to the previous slot and stays accepted for the + // grace window, so the collector's in-flight ingest connection isn't cut before its exporter has + // switched to the new certificate. + activateRenewedCertificate(); - final ExportLogsServiceRequest request = createTestRequest(); - final HttpResponse response = client.send( - HttpRequest.newBuilder() - .uri(URI.create("https://127.0.0.1:" + port + "/v1/logs")) - .header("Content-Type", "application/x-protobuf") - .POST(HttpRequest.BodyPublishers.ofByteArray(request.toByteArray())) - .build(), - HttpResponse.BodyHandlers.ofByteArray() - ); + final int port = startServer(); + final HttpClient client = createMtlsClient(agentKey, agentCert); // the now-superseded cert + + final HttpResponse response = postLogs(client, port); - // 200 means the handler found the agent UID, proving the handshake event fired first assertThat(response.statusCode()).isEqualTo(200); + assertThat(capturedJournalRecord().getCollectorInstanceUid()).isEqualTo(AGENT_INSTANCE_UID); + } - // Verify that collector_instance_uid was propagated all the way to the journal record - final ArgumentCaptor captor = ArgumentCaptor.forClass(RawMessage.class); - verify(input).processRawMessage(captor.capture()); - final CollectorJournal.Record record = CollectorJournal.Record.parseFrom(captor.getValue().getPayload()); - assertThat(record.getCollectorInstanceUid()).isEqualTo(AGENT_INSTANCE_UID); + @Test + void rejectsSupersededCertificateAfterGraceWindowElapses() throws Exception { + // Once the grace window has elapsed, the superseded certificate's binding is expired and a new + // connection is rejected at the handshake — renewal rotates ingest access off the old cert. + activateRenewedCertificate(); + clock.add(Duration.ofHours(1)); // well past the grace window + + final int port = startServer(); + final HttpClient client = createMtlsClient(agentKey, agentCert); + + assertThatThrownBy(() -> postLogs(client, port)) + .hasCauseInstanceOf(SSLException.class); } - // ----- Helper methods ----- + @Test + void cutsEstablishedConnectionWhenGraceWindowElapses() throws Exception { + // TLS validates the client certificate only at the handshake — an established connection is never + // re-checked by the TLS layer. So a connection opened within the grace window must be cut by the + // per-request binding check once the grace deadline passes: same connection (the HttpClient reuses + // its pooled keep-alive connection, no new handshake), but now 401 instead of 200. + activateRenewedCertificate(); - private int startHttpServer() throws Exception { - // InsecureTrustManagerFactory accepts any client cert. ClientAuth.REQUIRE - // still enforces that a cert MUST be presented. - final SslContext sslContext = SslContextBuilder.forServer(serverKey, serverCert) - .sslProvider(SslProvider.JDK) - .clientAuth(ClientAuth.REQUIRE) - .trustManager(InsecureTrustManagerFactory.INSTANCE) - .build(); + final int port = startServer(); + final HttpClient client = createMtlsClient(agentKey, agentCert); // the now-superseded cert + + assertThat(postLogs(client, port).statusCode()).isEqualTo(200); // within grace + + clock.add(Duration.ofMinutes(10)); // past the grace window, well within the cache's idle expiry + + assertThat(postLogs(client, port).statusCode()).isEqualTo(401); + } + + /** + * Stages a fresh next certificate for the enrolled agent and activates it, demoting the old active cert. + */ + private void activateRenewedCertificate() throws Exception { + final AgentCert renewed = mintClientCert(AGENT_INSTANCE_UID, signingCertEntry); + instanceService.insertNextCertificate(AGENT_INSTANCE_UID, renewed.entry().fingerprint(), + renewed.entry().certificate(), renewed.entry().notAfter()); + final var instance = instanceService.findByInstanceUid(AGENT_INSTANCE_UID).orElseThrow(); + assertThat(instanceService.activateNextCertificate(instance)).isTrue(); + } + + @Test + void rejectsCertSignedByForeignCa() throws Exception { + // An attacker who knows the instance UID but signs with their own CA: the cert carries the right + // CN but does not chain to the collectors root, so the trust manager rejects it at the crypto + // gate before the binding is even consulted. + final CertificateEntry foreignCa = certBuilder.createRootCa("Foreign CA", Algorithm.ED25519, CERT_VALIDITY); + final AgentCert foreign = mintClientCert(AGENT_INSTANCE_UID, foreignCa); + final X509Certificate foreignCaCert = PemUtils.parseCertificate(foreignCa.certificate()); + + final int port = startServer(); + final HttpClient client = createMtlsClient(foreign.key(), foreign.cert(), foreignCaCert); + + assertThatThrownBy(() -> postLogs(client, port)) + .hasCauseInstanceOf(SSLException.class); + } + + // ----- Helpers ----- + + private int startServer() throws Exception { + final SslContext sslContext = tlsUtils.newServerSslContextBuilder().build(); final ServerBootstrap bootstrap = new ServerBootstrap() .group(bossGroup, workerGroup) @@ -322,29 +379,40 @@ protected void initChannel(SocketChannel ch) { pipeline.addLast("agent-cert-handler", new AgentCertChannelHandler()); pipeline.addLast("http-codec", new HttpServerCodec()); pipeline.addLast("http-aggregator", new HttpObjectAggregator(1024 * 1024)); - pipeline.addLast("http-handler", new CollectorIngestHttpHandler(input)); + pipeline.addLast("http-handler", new CollectorIngestHttpHandler(input, certBindingResolver)); } }); - httpServerChannel = bootstrap.bind("127.0.0.1", 0).sync().channel(); - return ((InetSocketAddress) httpServerChannel.localAddress()).getPort(); + serverChannel = bootstrap.bind("127.0.0.1", 0).sync().channel(); + return ((InetSocketAddress) serverChannel.localAddress()).getPort(); } - /** - * Creates a Java HttpClient with mTLS using a custom KeyManager and trust-all TrustManager. - *

- * The custom {@link SimpleKeyManager} avoids PKCS12 keystore chain validation issues - * with Ed25519 certs (JDK's {@code KeyStore.setKeyEntry()} rejects Ed25519 cert chains). - */ - private HttpClient createHttpClient(PrivateKey clientKey, X509Certificate clientCert) throws Exception { - final X509ExtendedKeyManager km = new SimpleKeyManager(clientKey, clientCert, caCert); + private HttpResponse postLogs(HttpClient client, int port) throws Exception { + final ExportLogsServiceRequest request = createTestRequest(); + return client.send( + HttpRequest.newBuilder() + .uri(URI.create("https://127.0.0.1:" + port + "/v1/logs")) + .header("Content-Type", "application/x-protobuf") + .POST(HttpRequest.BodyPublishers.ofByteArray(request.toByteArray())) + .build(), + HttpResponse.BodyHandlers.ofByteArray()); + } + + private CollectorJournal.Record capturedJournalRecord() throws Exception { + final ArgumentCaptor captor = ArgumentCaptor.forClass(RawMessage.class); + verify(input).processRawMessage(captor.capture()); + return CollectorJournal.Record.parseFrom(captor.getValue().getPayload()); + } + private HttpClient createMtlsClient(PrivateKey clientKey, X509Certificate clientCert) throws Exception { + return createMtlsClient(clientKey, clientCert, signingCert); + } + + private HttpClient createMtlsClient(PrivateKey clientKey, X509Certificate clientCert, X509Certificate issuerCert) throws Exception { + final X509ExtendedKeyManager km = new SimpleKeyManager(clientKey, clientCert, issuerCert); final SSLContext sslContext = SSLContext.getInstance("TLS"); sslContext.init(new KeyManager[]{km}, new TrustManager[]{new TrustAllManager()}, null); - - return HttpClient.newBuilder() - .sslContext(sslContext) - .build(); + return HttpClient.newBuilder().sslContext(sslContext).build(); } private ExportLogsServiceRequest createTestRequest() { @@ -354,14 +422,28 @@ private ExportLogsServiceRequest createTestRequest() { .addLogRecords(LogRecord.newBuilder() .setBody(AnyValue.newBuilder().setStringValue("test log message")) .setTimeUnixNano(System.nanoTime()) - .setSeverityText("INFO") - ))) + .setSeverityText("INFO")))) .build(); } /** - * A simple X509ExtendedKeyManager that returns a fixed client cert and key. - * This avoids the PKCS12 keystore chain validation that fails for Ed25519 certs. + * Mints an Ed25519 client (clientAuth) end-entity certificate with the given CN, signed by the given + * issuer, using the production {@link CertificateBuilder}. + */ + private AgentCert mintClientCert(String cn, CertificateEntry issuer) throws Exception { + final CertificateEntry entry = certBuilder.createEndEntityCert( + cn, issuer, KeyUsage.digitalSignature, KeyPurposeId.id_kp_clientAuth, CERT_VALIDITY); + return new AgentCert( + PemUtils.parsePrivateKey(encryptedValueService.decrypt(entry.privateKey())), + PemUtils.parseCertificate(entry.certificate()), + entry); + } + + private record AgentCert(PrivateKey key, X509Certificate cert, CertificateEntry entry) {} + + /** + * A simple {@link X509ExtendedKeyManager} that returns a fixed client cert and key. This avoids the + * PKCS12 keystore chain validation that fails for Ed25519 certs. */ private static class SimpleKeyManager extends X509ExtendedKeyManager { private final PrivateKey privateKey; @@ -409,19 +491,35 @@ public PrivateKey getPrivateKey(String alias) { } /** - * A TrustManager that accepts all certificates. Used only in tests to bypass - * client-side server cert validation. Server-side mTLS validation (the focus - * of these tests) is unaffected. + * Trust manager that accepts all server certificates and skips hostname verification. + *

+ * Must extend {@link X509ExtendedTrustManager} (not just {@link javax.net.ssl.X509TrustManager}) + * because the JDK wraps a plain X509TrustManager in AbstractTrustManagerWrapper which adds + * hostname/IP identity checks; X509ExtendedTrustManager is used directly, bypassing the wrapper. */ - private static class TrustAllManager implements X509TrustManager { + private static class TrustAllManager extends X509ExtendedTrustManager { @Override public void checkClientTrusted(X509Certificate[] chain, String authType) { - // Accept all } @Override public void checkServerTrusted(X509Certificate[] chain, String authType) { - // Accept all + } + + @Override + public void checkClientTrusted(X509Certificate[] chain, String authType, Socket socket) { + } + + @Override + public void checkServerTrusted(X509Certificate[] chain, String authType, Socket socket) { + } + + @Override + public void checkClientTrusted(X509Certificate[] chain, String authType, SSLEngine engine) { + } + + @Override + public void checkServerTrusted(X509Certificate[] chain, String authType, SSLEngine engine) { } @Override diff --git a/graylog2-server/src/test/java/org/graylog/collectors/input/transport/AgentCertChannelHandlerTest.java b/graylog2-server/src/test/java/org/graylog/collectors/input/transport/AgentCertChannelHandlerTest.java index d9b1394bb00b..68f96bbb0ac8 100644 --- a/graylog2-server/src/test/java/org/graylog/collectors/input/transport/AgentCertChannelHandlerTest.java +++ b/graylog2-server/src/test/java/org/graylog/collectors/input/transport/AgentCertChannelHandlerTest.java @@ -75,8 +75,8 @@ void setsChannelAttributeOnSuccessfulHandshake() throws Exception { channel.pipeline().fireUserEventTriggered(SslHandshakeCompletionEvent.SUCCESS); - final String uid = channel.attr(AgentCertChannelHandler.AGENT_INSTANCE_UID).get(); - assertThat(uid).isEqualTo("my-agent-uid"); + final String uid = channel.attr(AgentCertChannelHandler.AGENT_CERT_FINGERPRINT).get(); + assertThat(uid).isEqualTo(PemUtils.computeFingerprint(x509)); channel.close(); } @@ -92,7 +92,7 @@ void doesNotSetAttributeOnFailedHandshake() { new SslHandshakeCompletionEvent(new RuntimeException("handshake failed")); channel.pipeline().fireUserEventTriggered(failEvent); - final String uid = channel.attr(AgentCertChannelHandler.AGENT_INSTANCE_UID).get(); + final String uid = channel.attr(AgentCertChannelHandler.AGENT_CERT_FINGERPRINT).get(); assertThat(uid).isNull(); channel.close(); diff --git a/graylog2-server/src/test/java/org/graylog/collectors/input/transport/CollectorIngestHttpHandlerTest.java b/graylog2-server/src/test/java/org/graylog/collectors/input/transport/CollectorIngestHttpHandlerTest.java index cf0bfd41de01..4e13a9e4808d 100644 --- a/graylog2-server/src/test/java/org/graylog/collectors/input/transport/CollectorIngestHttpHandlerTest.java +++ b/graylog2-server/src/test/java/org/graylog/collectors/input/transport/CollectorIngestHttpHandlerTest.java @@ -31,6 +31,7 @@ import io.opentelemetry.proto.logs.v1.LogRecord; import io.opentelemetry.proto.logs.v1.ResourceLogs; import io.opentelemetry.proto.logs.v1.ScopeLogs; +import org.graylog.collectors.CertBindingResolver; import org.graylog.collectors.CollectorJournal; import org.graylog2.plugin.inputs.MessageInput; import org.graylog2.plugin.journal.RawMessage; @@ -41,13 +42,16 @@ import org.mockito.junit.jupiter.MockitoExtension; import java.nio.charset.StandardCharsets; +import java.util.Optional; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; +import static org.mockito.Mockito.when; @ExtendWith(MockitoExtension.class) class CollectorIngestHttpHandlerTest { @@ -55,6 +59,9 @@ class CollectorIngestHttpHandlerTest { @Mock private MessageInput input; + @Mock + private CertBindingResolver certBindingResolver; + @Test void postWithValidIdentityReturns200() throws Exception { final EmbeddedChannel channel = createChannel("test-uid"); @@ -70,13 +77,29 @@ void postWithValidIdentityReturns200() throws Exception { } @Test - void postWithoutIdentityReturns401() { + void postWithMissingFingerprintReturns500() { final EmbeddedChannel channel = createChannel(null); final ExportLogsServiceRequest request = createTestRequest(); final FullHttpRequest httpRequest = createProtobufRequest("/v1/logs", request.toByteArray()); channel.writeInbound(httpRequest); + final FullHttpResponse response = channel.readOutbound(); + assertThat(response.status()).isEqualTo(HttpResponseStatus.INTERNAL_SERVER_ERROR); + verifyNoInteractions(input); + response.release(); + } + + @Test + void postWithUnboundFingerprintReturns401() { + final EmbeddedChannel channel = new EmbeddedChannel(new CollectorIngestHttpHandler(input, certBindingResolver)); + channel.attr(AgentCertChannelHandler.AGENT_CERT_FINGERPRINT).set("sha256:unbound"); + when(certBindingResolver.resolve("sha256:unbound")).thenReturn(Optional.empty()); + + final ExportLogsServiceRequest request = createTestRequest(); + final FullHttpRequest httpRequest = createProtobufRequest("/v1/logs", request.toByteArray()); + channel.writeInbound(httpRequest); + final FullHttpResponse response = channel.readOutbound(); assertThat(response.status()).isEqualTo(HttpResponseStatus.UNAUTHORIZED); verifyNoInteractions(input); @@ -262,9 +285,12 @@ void inputMessageSizeIsDistributedProportionally() { private EmbeddedChannel createChannel(String agentInstanceUid) { final EmbeddedChannel channel = new EmbeddedChannel( - new CollectorIngestHttpHandler(input)); + new CollectorIngestHttpHandler(input, certBindingResolver)); if (agentInstanceUid != null) { - channel.attr(AgentCertChannelHandler.AGENT_INSTANCE_UID).set(agentInstanceUid); + // The handler reads the fingerprint from the channel attribute and resolves it to an + // instance UID via the cache; here the test uses the UID itself as the fingerprint. + channel.attr(AgentCertChannelHandler.AGENT_CERT_FINGERPRINT).set(agentInstanceUid); + lenient().when(certBindingResolver.resolve(agentInstanceUid)).thenReturn(Optional.of(agentInstanceUid)); } return channel; } diff --git a/graylog2-server/src/test/java/org/graylog/collectors/input/transport/CollectorIngestHttpTransportConfigTest.java b/graylog2-server/src/test/java/org/graylog/collectors/input/transport/CollectorIngestHttpTransportConfigTest.java index ca88ebb417a3..97923c070022 100644 --- a/graylog2-server/src/test/java/org/graylog/collectors/input/transport/CollectorIngestHttpTransportConfigTest.java +++ b/graylog2-server/src/test/java/org/graylog/collectors/input/transport/CollectorIngestHttpTransportConfigTest.java @@ -19,6 +19,7 @@ import org.graylog.collectors.CollectorsConfig; import org.graylog.collectors.CollectorsConfigService; import org.graylog.collectors.IngestEndpointConfig; +import org.graylog2.inputs.transports.AbstractHttpTransport; import org.graylog2.plugin.inputs.transports.AbstractTcpTransport; import org.graylog2.plugin.inputs.transports.NettyTransport; import org.junit.jupiter.api.Test; @@ -72,6 +73,18 @@ void portFieldHasPortNumberAttribute() { assertThat(portField.getAttributes()).contains("is_port_number"); } + @Test + void idleWriterTimeoutIsNotExposed() { + when(collectorsConfigService.get()).thenReturn(Optional.empty()); + + final var transportConfig = new CollectorIngestHttpTransport.Config(collectorsConfigService); + + // The idle connection timeout is forced by the transport (it backs the fingerprint-cache hit + // guarantee), so it must not be offered as a user-configurable field. + assertThat(transportConfig.getRequestedConfiguration().getFields()) + .doesNotContainKey(AbstractHttpTransport.CK_IDLE_WRITER_TIMEOUT); + } + @Test void keepAliveIsEnabledByDefault() { when(collectorsConfigService.get()).thenReturn(Optional.empty()); diff --git a/graylog2-server/src/test/java/org/graylog/collectors/input/transport/CollectorIngestHttpTransportTest.java b/graylog2-server/src/test/java/org/graylog/collectors/input/transport/CollectorIngestHttpTransportTest.java new file mode 100644 index 000000000000..628f32f5059c --- /dev/null +++ b/graylog2-server/src/test/java/org/graylog/collectors/input/transport/CollectorIngestHttpTransportTest.java @@ -0,0 +1,90 @@ +/* + * Copyright (C) 2020 Graylog, Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the Server Side Public License, version 1, + * as published by MongoDB, Inc. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * Server Side Public License for more details. + * + * You should have received a copy of the Server Side Public License + * along with this program. If not, see + * . + */ +package org.graylog.collectors.input.transport; + +import io.netty.channel.EventLoopGroup; +import io.netty.handler.timeout.IdleStateHandler; +import org.graylog.collectors.CollectorTLSUtils; +import org.graylog2.configuration.TLSProtocolsConfiguration; +import org.graylog2.inputs.transports.AbstractHttpTransport; +import org.graylog2.inputs.transports.NettyTransportConfiguration; +import org.graylog2.inputs.transports.netty.EventLoopGroupFactory; +import org.graylog2.plugin.LocalMetricRegistry; +import org.graylog2.plugin.configuration.Configuration; +import org.graylog2.plugin.inputs.MessageInput; +import org.graylog2.plugin.inputs.util.ThroughputCounter; +import org.graylog2.security.encryption.EncryptedValueService; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +import java.time.Duration; +import java.util.Map; +import java.util.Set; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.when; + +@ExtendWith(MockitoExtension.class) +class CollectorIngestHttpTransportTest { + + @Mock + private EventLoopGroup eventLoopGroup; + @Mock + private EventLoopGroupFactory eventLoopGroupFactory; + @Mock + private NettyTransportConfiguration nettyTransportConfiguration; + @Mock + private ThroughputCounter throughputCounter; + @Mock + private TLSProtocolsConfiguration tlsConfiguration; + @Mock + private CollectorTLSUtils tlsUtils; + @Mock + private EncryptedValueService encryptedValueService; + @Mock + private CollectorIngestHttpHandler.Factory httpHandlerFactory; + @Mock + private MessageInput input; + + /** + * The idle connection timeout is part of the transport's forced, non-configurable envelope (like + * mTLS): connections must be closed well before their fingerprint cache entry can idle out, so that + * per-request binding lookups are always cache hits (see {@code CertBindingResolver}). A user + * config of {@code 0} would disable the read-timeout handler entirely and void that guarantee, so it + * must be overridden. + */ + @Test + void idleConnectionTimeoutIsForcedEvenWhenConfiguredOff() throws Exception { + final var transport = buildTransport(Map.of(AbstractHttpTransport.CK_IDLE_WRITER_TIMEOUT, 0)); + + final var handlers = transport.getCustomChildChannelHandlers(input); + + assertThat(handlers).containsKey("read-timeout-handler"); + final var handler = (IdleStateHandler) handlers.get("read-timeout-handler").call(); + assertThat(handler.getReaderIdleTimeInMillis()).isEqualTo(Duration.ofSeconds(60).toMillis()); + } + + private CollectorIngestHttpTransport buildTransport(Map userConfig) { + when(throughputCounter.gauges()).thenReturn(Map.of()); + return new CollectorIngestHttpTransport(new Configuration(userConfig), eventLoopGroup, + eventLoopGroupFactory, nettyTransportConfiguration, throughputCounter, + new LocalMetricRegistry(), tlsConfiguration, Set.of(), tlsUtils, encryptedValueService, + httpHandlerFactory); + } +} diff --git a/graylog2-server/src/test/java/org/graylog/collectors/opamp/CollectorInstanceServiceTest.java b/graylog2-server/src/test/java/org/graylog/collectors/opamp/CollectorInstanceServiceTest.java index fc457826660e..812a9c7a7c10 100644 --- a/graylog2-server/src/test/java/org/graylog/collectors/opamp/CollectorInstanceServiceTest.java +++ b/graylog2-server/src/test/java/org/graylog/collectors/opamp/CollectorInstanceServiceTest.java @@ -16,6 +16,7 @@ */ package org.graylog.collectors.opamp; +import com.google.common.eventbus.Subscribe; import com.mongodb.client.model.Filters; import org.bouncycastle.asn1.x509.KeyUsage; import org.bson.Document; @@ -24,12 +25,14 @@ import org.graylog.collectors.db.Attribute; import org.graylog.collectors.db.CollectorInstanceDTO; import org.graylog.collectors.db.CollectorInstanceReport; +import org.graylog.collectors.events.CollectorInstanceCertsChangedEvent; import org.graylog.security.pki.Algorithm; import org.graylog.security.pki.CertificateBuilder; import org.graylog.security.pki.CertificateEntry; import org.graylog.testing.TestClocks; import org.graylog.testing.mongodb.MongoDBExtension; import org.graylog2.database.MongoCollections; +import org.graylog2.events.ClusterEventBus; import org.graylog2.security.encryption.EncryptedValueService; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.BeforeEach; @@ -39,7 +42,9 @@ import java.time.Duration; import java.time.Instant; +import java.util.ArrayList; import java.util.Date; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Optional; @@ -62,6 +67,7 @@ class CollectorInstanceServiceTest { private CollectorInstanceService collectorInstanceService; private MongoCollections mongoCollections; private MutableClock clock; + private List capturedEvents; @BeforeAll static void beforeAll() throws Exception { @@ -74,7 +80,23 @@ static void beforeAll() throws Exception { void setUp(MongoCollections coll) { mongoCollections = coll; clock = TestClocks.mutableFixedEpoch(); - collectorInstanceService = new CollectorInstanceService(coll, clock); + capturedEvents = new ArrayList<>(); + + // directExecutor() makes posts synchronous, so events are captured inline. + final var clusterEventBus = new ClusterEventBus(); + clusterEventBus.registerClusterEventSubscriber(new Object() { + @Subscribe + public void on(CollectorInstanceCertsChangedEvent event) { + capturedEvents.add(event); + } + }); + + collectorInstanceService = new CollectorInstanceService(coll, clusterEventBus, clock); + } + + private CollectorInstanceCertsChangedEvent lastEvent() { + assertThat(capturedEvents).isNotEmpty(); + return capturedEvents.get(capturedEvents.size() - 1); } @Test @@ -302,7 +324,7 @@ void reEnrollUpdatesCertTokenAndLastSeen() throws Exception { final var newCert = certBuilder.createEndEntityCert("uid-re", issuerCert, KeyUsage.digitalSignature, Duration.ofDays(1)); final var newIssued = new IssuedCertificate(newCert.fingerprint(), newCert.certificate(), newCert.notAfter(), issuerCert.id()); - final var updated = collectorInstanceService.reEnroll(original.id(), original.activeCertificateFingerprint(), newIssued, "new-token-id"); + final var updated = collectorInstanceService.reEnroll(original, newIssued, "new-token-id"); assertThat(updated.activeCertificateFingerprint()).isEqualTo(newIssued.fingerprint()); assertThat(updated.activeCertificatePem()).isEqualTo(newIssued.certPem()); @@ -321,7 +343,7 @@ void reEnrollPreservesExistingFields() throws Exception { final var newCert = certBuilder.createEndEntityCert("uid-preserve", issuerCert, KeyUsage.digitalSignature, Duration.ofDays(1)); final var newIssued = new IssuedCertificate(newCert.fingerprint(), newCert.certificate(), newCert.notAfter(), issuerCert.id()); - final var updated = collectorInstanceService.reEnroll(original.id(), original.activeCertificateFingerprint(), newIssued, "new-token-id"); + final var updated = collectorInstanceService.reEnroll(original, newIssued, "new-token-id"); assertThat(updated.instanceUid()).isEqualTo(original.instanceUid()); assertThat(updated.enrolledAt()).isEqualTo(enrollTime); @@ -339,7 +361,8 @@ void reEnrollClearsNextCertificateFields() throws Exception { final var newCert = certBuilder.createEndEntityCert("uid-pending-renewal", issuerCert, KeyUsage.digitalSignature, Duration.ofDays(1)); final var newIssued = new IssuedCertificate(newCert.fingerprint(), newCert.certificate(), newCert.notAfter(), issuerCert.id()); - final var updated = collectorInstanceService.reEnroll(original.id(), original.activeCertificateFingerprint(), newIssued, "token-x"); + final var withNext = collectorInstanceService.findByInstanceUid("uid-pending-renewal").orElseThrow(); + final var updated = collectorInstanceService.reEnroll(withNext, newIssued, "token-x"); assertThat(updated.nextCertificateFingerprint()).isEmpty(); assertThat(updated.nextCertificatePem()).isEmpty(); @@ -363,7 +386,7 @@ void enrollAndReEnrollRoundTripThroughMongo() throws Exception { clock.setInstant(reEnrollTime); final var newCert = certBuilder.createEndEntityCert("uid-roundtrip", issuerCert, KeyUsage.digitalSignature, Duration.ofDays(1)); final var newIssued = new IssuedCertificate(newCert.fingerprint(), newCert.certificate(), newCert.notAfter(), issuerCert.id()); - collectorInstanceService.reEnroll(enrolled.id(), enrolled.activeCertificateFingerprint(), newIssued, "token-2"); + collectorInstanceService.reEnroll(enrolled, newIssued, "token-2"); // Fresh read after re-enroll — exercises the post-update deserialize path. final var afterReEnroll = collectorInstanceService.findByInstanceUid("uid-roundtrip").orElseThrow(); @@ -384,18 +407,119 @@ void reEnrollStoresTemporalFieldsAsBsonDate() throws Exception { final var newCert = certBuilder.createEndEntityCert(uid, issuerCert, KeyUsage.digitalSignature, Duration.ofDays(1)); final var newIssued = new IssuedCertificate(newCert.fingerprint(), newCert.certificate(), newCert.notAfter(), issuerCert.id()); - collectorInstanceService.reEnroll(original.id(), original.activeCertificateFingerprint(), newIssued, "token-x"); + collectorInstanceService.reEnroll(original, newIssued, "token-x"); assertFieldIsDate(uid, CollectorInstanceDTO.FIELD_LAST_SEEN); assertFieldIsDate(uid, CollectorInstanceDTO.FIELD_ACTIVE_CERTIFICATE_EXPIRES_AT); } + @Test + void enrollDoesNotPersistNextCertificateFingerprintAsNull() throws Exception { + // The partial index on next_certificate_fingerprint filters on {$exists: true}, which in MongoDB + // also matches a field present with an explicit null. Persisting the field as null (instead of + // omitting it) would pull every non-renewing instance into the index, defeating its purpose. A + // freshly enrolled instance has no next certificate, so the field must be absent from the document. + enroll("uid-no-next-field"); + + final var doc = findRawDocument("uid-no-next-field").orElseThrow(); + assertThat(doc.containsKey(CollectorInstanceDTO.FIELD_NEXT_CERTIFICATE_FINGERPRINT)).isFalse(); + } + + @Test + void enrollDoesNotPersistPreviousCertificateFieldsAsNull() throws Exception { + // A freshly enrolled instance has never rotated, so none of the previous-certificate fields nor + // certificates_rotated_at exist yet. They must be ABSENT, not present-with-null: the partial index + // on previous_certificate_fingerprint is UNIQUE, and {$exists: true} matches an explicit null, so a + // second instance persisting a null previous fingerprint would collide and fail the write. + enroll("uid-no-previous-fields"); + + final var doc = findRawDocument("uid-no-previous-fields").orElseThrow(); + assertThat(doc.containsKey(CollectorInstanceDTO.FIELD_PREVIOUS_CERTIFICATE_FINGERPRINT)).isFalse(); + assertThat(doc.containsKey(CollectorInstanceDTO.FIELD_PREVIOUS_CERTIFICATE_PEM)).isFalse(); + assertThat(doc.containsKey(CollectorInstanceDTO.FIELD_PREVIOUS_CERTIFICATE_EXPIRES_AT)).isFalse(); + assertThat(doc.containsKey(CollectorInstanceDTO.FIELD_CERTIFICATES_ROTATED_AT)).isFalse(); + } + + @Test + void enrollingMultipleInstancesWithoutPreviousDoesNotCollideOnUniqueIndex() throws Exception { + // Guards the unique partial index on previous_certificate_fingerprint: instances that have never + // rotated omit the field entirely, so the partial {$exists: true} filter excludes them and many + // such instances can coexist. If any write path ever wrote an explicit null instead, the second + // enroll here would fail with a duplicate-key error. + final var first = enroll("uid-no-previous-1"); + final var second = enroll("uid-no-previous-2"); + + assertThat(first.id()).isNotNull(); + assertThat(second.id()).isNotNull(); + assertThat(collectorInstanceService.findByInstanceUid("uid-no-previous-1")).isPresent(); + assertThat(collectorInstanceService.findByInstanceUid("uid-no-previous-2")).isPresent(); + } + + @Test + void constructionClearsLegacyNullNextCertificateValuesSoTheUniqueIndexCanBuild() throws Exception { + // Released versions persisted next_certificate_fingerprint as an explicit null (the field predates + // @JsonInclude(NON_ABSENT)). Reproduce that pre-migration state: drop the unique next index so + // multiple null documents can be seeded (the live unique index would reject the second null), then + // seed several. On construction the service must clear the nulls and successfully (re)build the index. + final var raw = mongoCollections.nonEntityCollection(CollectorInstanceService.COLLECTION_NAME, Document.class); + raw.dropIndex("next_certificate_fingerprint_1"); + for (int i = 0; i < 3; i++) { + enroll("uid-legacy-null-" + i); + raw.updateOne(Filters.eq(CollectorInstanceDTO.FIELD_INSTANCE_UID, "uid-legacy-null-" + i), + new Document("$set", new Document() + .append(CollectorInstanceDTO.FIELD_NEXT_CERTIFICATE_FINGERPRINT, null) + .append(CollectorInstanceDTO.FIELD_NEXT_CERTIFICATE_PEM, null) + .append(CollectorInstanceDTO.FIELD_NEXT_CERTIFICATE_EXPIRES_AT, null))); + } + + // Constructing the service runs the cleanup, then rebuilds the indexes over the now-clean data. + new CollectorInstanceService(mongoCollections, new ClusterEventBus(), clock); + + for (int i = 0; i < 3; i++) { + final var doc = findRawDocument("uid-legacy-null-" + i).orElseThrow(); + assertThat(doc.containsKey(CollectorInstanceDTO.FIELD_NEXT_CERTIFICATE_FINGERPRINT)).isFalse(); + assertThat(doc.containsKey(CollectorInstanceDTO.FIELD_NEXT_CERTIFICATE_PEM)).isFalse(); + assertThat(doc.containsKey(CollectorInstanceDTO.FIELD_NEXT_CERTIFICATE_EXPIRES_AT)).isFalse(); + } + + // The unique next index must exist again — if the cleanup had failed, createIndexes would have + // collided on the nulls (swallowed by its catch) and the index would be missing. + boolean uniqueNextIndexPresent = false; + for (Document index : raw.listIndexes()) { + final Document key = index.get("key", Document.class); + if (key != null && key.containsKey(CollectorInstanceDTO.FIELD_NEXT_CERTIFICATE_FINGERPRINT) + && index.getBoolean("unique", false)) { + uniqueNextIndexPresent = true; + break; + } + } + assertThat(uniqueNextIndexPresent).isTrue(); + } + + @Test + void activateNextCertificateRemovesNextCertificateFingerprintField() throws Exception { + // Activation promotes next -> active and must remove the next fingerprint field entirely (not set + // it to null), so the instance drops back out of the partial index once it is no longer mid-renewal. + enroll("uid-activate-clears-next"); + collectorInstanceService.insertNextCertificate("uid-activate-clears-next", "sha256:next-fp", "next-pem", + Instant.ofEpochMilli(0).plus(Duration.ofDays(2))); + final var withNext = collectorInstanceService.findByInstanceUid("uid-activate-clears-next").orElseThrow(); + + collectorInstanceService.activateNextCertificate(withNext); + + final var doc = findRawDocument("uid-activate-clears-next").orElseThrow(); + assertThat(doc.containsKey(CollectorInstanceDTO.FIELD_NEXT_CERTIFICATE_FINGERPRINT)).isFalse(); + } + @Test void reEnrollThrowsWhenRecordDoesNotExist() throws Exception { final var cert = certBuilder.createEndEntityCert("ghost", issuerCert, KeyUsage.digitalSignature, Duration.ofDays(1)); final var issued = new IssuedCertificate(cert.fingerprint(), cert.certificate(), cert.notAfter(), issuerCert.id()); - assertThatThrownBy(() -> collectorInstanceService.reEnroll("507f1f77bcf86cd799439999", "sha256:whatever", issued, "token")) + // An instance whose id matches no stored record. + final var ghost = enroll("ghost").toBuilder().id("507f1f77bcf86cd799439999").build(); + + assertThatThrownBy(() -> collectorInstanceService.reEnroll(ghost, issued, "token")) .isInstanceOf(IllegalStateException.class) .hasMessageContaining("doesn't exist"); } @@ -409,7 +533,8 @@ void reEnrollThrowsWhenActiveCertificateChangedConcurrently() throws Exception { // Stale fingerprint: simulates the active cert being swapped (e.g. by a concurrent renewal // activation) between the caller's read and the update. - assertThatThrownBy(() -> collectorInstanceService.reEnroll(original.id(), "sha256:stale-fingerprint", newIssued, "token")) + final var stale = original.toBuilder().activeCertificateFingerprint("sha256:stale-fingerprint").build(); + assertThatThrownBy(() -> collectorInstanceService.reEnroll(stale, newIssued, "token")) .isInstanceOf(IllegalStateException.class); // The record must be untouched — same cert, token, and next_* state as before. @@ -484,6 +609,46 @@ void activateNextCertificateThrowsWhenNextFieldsMissing() throws Exception { .isInstanceOf(IllegalArgumentException.class); } + @Test + void activateNextCertificateIsNoOpWhenSlotsChangedConcurrently() throws Exception { + final var enrolled = enroll("uid-cas"); + setNextCertificateFields("uid-cas", "sha256:cas-next", "next-pem", clock.instant().plus(Duration.ofDays(30))); + final var stale = collectorInstanceService.findByInstanceUid("uid-cas").orElseThrow(); + // A concurrent renewal stages a different next certificate after our read: the activation must + // not promote the outdated one it read. + setNextCertificateFields("uid-cas", "sha256:cas-next-2", "next-pem-2", clock.instant().plus(Duration.ofDays(60))); + capturedEvents.clear(); + + assertThat(collectorInstanceService.activateNextCertificate(stale)).isFalse(); + + // The concurrently written state stays intact: no promotion, no rotation stamp, no event. + final var current = collectorInstanceService.findByInstanceUid("uid-cas").orElseThrow(); + assertThat(current.activeCertificateFingerprint()).isEqualTo(enrolled.activeCertificateFingerprint()); + assertThat(current.nextCertificateFingerprint()).contains("sha256:cas-next-2"); + assertThat(current.previousCertificateFingerprint()).isEmpty(); + assertThat(current.certificatesRotatedAt()).isEmpty(); + assertThat(capturedEvents).isEmpty(); + } + + @Test + void activateNextCertificateRepeatedActivationWithStaleStateIsNoOp() throws Exception { + enroll("uid-cas-double"); + setNextCertificateFields("uid-cas-double", "sha256:cas-d-next", "next-pem", clock.instant().plus(Duration.ofDays(30))); + final var withNext = collectorInstanceService.findByInstanceUid("uid-cas-double").orElseThrow(); + assertThat(collectorInstanceService.activateNextCertificate(withNext)).isTrue(); + final var rotatedAt = collectorInstanceService.findByInstanceUid("uid-cas-double").orElseThrow() + .certificatesRotatedAt().orElseThrow(); + + // A second activation with the same stale DTO (e.g. two concurrent requests both presenting the + // next thumbprint) must be a no-op — re-stamping certificates_rotated_at would silently extend + // the rotation grace window. + clock.add(Duration.ofMinutes(1)); + assertThat(collectorInstanceService.activateNextCertificate(withNext)).isFalse(); + + assertThat(collectorInstanceService.findByInstanceUid("uid-cas-double").orElseThrow() + .certificatesRotatedAt()).contains(rotatedAt); + } + @Test void insertNextCertificateSetsNextFields() throws Exception { final var instance = enroll("uid-insert-next"); @@ -536,6 +701,150 @@ void insertNextCertificateOverwritesPreviousNextFields() throws Exception { assertThat(updated.nextCertificateExpiresAt()).hasValue(Date.from(secondExpiresAt).toInstant()); } + // ----- CollectorInstanceCertsChangedEvent publishing ----- + + @Test + void enrollPublishesActiveFingerprint() throws Exception { + final var instance = enroll("uid-evt-enroll"); + + assertThat(lastEvent().fingerprints()).containsExactly(instance.activeCertificateFingerprint()); + } + + @Test + void insertNextCertificatePublishesActiveAndNextFingerprints() throws Exception { + final var enrolled = enroll("uid-evt-next"); + capturedEvents.clear(); + + collectorInstanceService.insertNextCertificate("uid-evt-next", "sha256:next-fp", "pem", + Instant.now().plus(Duration.ofDays(30))); + + assertThat(lastEvent().fingerprints()) + .containsExactlyInAnyOrder(enrolled.activeCertificateFingerprint(), "sha256:next-fp"); + } + + @Test + void insertNextCertificatePublishesReplacedAndNewNextFingerprints() throws Exception { + final var enrolled = enroll("uid-evt-next-replace"); + collectorInstanceService.insertNextCertificate("uid-evt-next-replace", "sha256:first-next", "pem1", + Instant.now().plus(Duration.ofDays(10))); + capturedEvents.clear(); + + collectorInstanceService.insertNextCertificate("uid-evt-next-replace", "sha256:second-next", "pem2", + Instant.now().plus(Duration.ofDays(20))); + + assertThat(lastEvent().fingerprints()).containsExactlyInAnyOrder( + enrolled.activeCertificateFingerprint(), "sha256:first-next", "sha256:second-next"); + } + + @Test + void activateNextCertificatePublishesRotatedFingerprints() throws Exception { + final var enrolled = enroll("uid-evt-activate"); + setNextCertificateFields("uid-evt-activate", "sha256:new-active", "new-pem", + Instant.now().plus(Duration.ofDays(30))); + final var withNext = collectorInstanceService.findByInstanceUid("uid-evt-activate").orElseThrow(); + capturedEvents.clear(); + + collectorInstanceService.activateNextCertificate(withNext); + + // Both rotated fingerprints (demoted old active + promoted next) are touched and re-resolved. + assertThat(lastEvent().fingerprints()) + .containsExactlyInAnyOrder(enrolled.activeCertificateFingerprint(), "sha256:new-active"); + } + + @Test + void activateNextCertificateDemotesActiveToPreviousAndStampsRotationTimestamp() throws Exception { + final var enrolled = enroll("uid-rotate-fields"); + setNextCertificateFields("uid-rotate-fields", "sha256:new-active", "new-pem", + Instant.now().plus(Duration.ofDays(30))); + final var withNext = collectorInstanceService.findByInstanceUid("uid-rotate-fields").orElseThrow(); + + collectorInstanceService.activateNextCertificate(withNext); + + // The old active cert is demoted into the previous slot, and the rotation is timestamped so the + // ingest grace window can be measured from it. Temporal fields must be stored as BSON dates. + final var doc = findRawDocument("uid-rotate-fields").orElseThrow(); + assertThat(doc.getString(CollectorInstanceDTO.FIELD_PREVIOUS_CERTIFICATE_FINGERPRINT)) + .isEqualTo(enrolled.activeCertificateFingerprint()); + assertFieldIsDate("uid-rotate-fields", CollectorInstanceDTO.FIELD_PREVIOUS_CERTIFICATE_EXPIRES_AT); + assertFieldIsDate("uid-rotate-fields", CollectorInstanceDTO.FIELD_CERTIFICATES_ROTATED_AT); + } + + @Test + void reEnrollPublishesOldAndNewActiveFingerprints() throws Exception { + final var original = enroll("uid-evt-reenroll"); + final var newCert = certBuilder.createEndEntityCert("uid-evt-reenroll", issuerCert, KeyUsage.digitalSignature, Duration.ofDays(1)); + final var newIssued = new IssuedCertificate(newCert.fingerprint(), newCert.certificate(), newCert.notAfter(), issuerCert.id()); + capturedEvents.clear(); + + collectorInstanceService.reEnroll(original, newIssued, "token-evt"); + + assertThat(lastEvent().fingerprints()) + .containsExactlyInAnyOrder(original.activeCertificateFingerprint(), newIssued.fingerprint()); + } + + @Test + void deleteByInstanceUidPublishesAllCertFingerprints() throws Exception { + final var instance = enroll("uid-evt-delete"); + setNextCertificateFields("uid-evt-delete", "sha256:pending", "pending-pem", + Instant.now().plus(Duration.ofDays(30))); + capturedEvents.clear(); + + collectorInstanceService.deleteByInstanceUid("uid-evt-delete"); + + assertThat(lastEvent().fingerprints()) + .containsExactlyInAnyOrder(instance.activeCertificateFingerprint(), "sha256:pending"); + } + + @Test + void deleteExpiredPublishesFingerprintsForPurgedInstances() throws Exception { + final Instant reference = Instant.parse("2025-01-01T00:00:00Z"); + final var expiredA = enrollWithFleetAndLastSeen("uid-exp-a", "507f1f77bcf86cd799439012", reference.minus(Duration.ofDays(8))); + final var expiredB = enrollWithFleetAndLastSeen("uid-exp-b", "507f1f77bcf86cd799439012", reference.minus(Duration.ofDays(8))); + enrollWithFleetAndLastSeen("uid-exp-fresh", "507f1f77bcf86cd799439012", reference); + capturedEvents.clear(); + + clock.setInstant(reference); + collectorInstanceService.deleteExpired(Duration.ofDays(7)); + + final var fingerprints = new ArrayList(); + capturedEvents.forEach(event -> fingerprints.addAll(event.fingerprints())); + assertThat(fingerprints).containsExactlyInAnyOrder( + expiredA.activeCertificateFingerprint(), expiredB.activeCertificateFingerprint()); + } + + @Test + void deleteExpiredBatchesRevocationEvents() { + // Enough expired instances that the purge spans multiple revocation-event batches + // (REVOCATION_EVENT_BATCH_SIZE is 1000). Inserted as raw documents — deleteExpired only reads + // last_seen and the fingerprint fields through its projection, so full enrollment is unnecessary. + final int instanceCount = 1100; + final Instant reference = Instant.parse("2025-01-01T00:00:00Z"); + final var expectedFingerprints = new HashSet(); + final var documents = new ArrayList(instanceCount); + for (int i = 0; i < instanceCount; i++) { + final var fingerprint = "sha256:batch-" + i; + expectedFingerprints.add(fingerprint); + documents.add(new Document(CollectorInstanceDTO.FIELD_INSTANCE_UID, "uid-batch-" + i) + .append(CollectorInstanceDTO.FIELD_LAST_SEEN, Date.from(reference.minus(Duration.ofDays(8)))) + .append(CollectorInstanceDTO.FIELD_ACTIVE_CERTIFICATE_FINGERPRINT, fingerprint)); + } + mongoCollections.nonEntityCollection(CollectorInstanceService.COLLECTION_NAME, Document.class) + .insertMany(documents); + capturedEvents.clear(); + + clock.setInstant(reference); + assertThat(collectorInstanceService.deleteExpired(Duration.ofDays(7))).isEqualTo(instanceCount); + + // Each event must respect the batch limit, and together the events must cover every revoked + // fingerprint exactly once. + assertThat(capturedEvents).allSatisfy(event -> + assertThat(event.fingerprints()).hasSizeLessThanOrEqualTo(1000)); + final var posted = new ArrayList(); + capturedEvents.forEach(event -> posted.addAll(event.fingerprints())); + assertThat(posted).hasSize(instanceCount); // no fingerprint posted twice + assertThat(Set.copyOf(posted)).isEqualTo(expectedFingerprints); + } + @Test void updateFromReportThrowsForNonExistentInstance() { final var report = CollectorInstanceReport.builder() diff --git a/graylog2-server/src/test/java/org/graylog/collectors/opamp/OpAmpServiceHandleEnrollmentTest.java b/graylog2-server/src/test/java/org/graylog/collectors/opamp/OpAmpServiceHandleEnrollmentTest.java index 15e8df6b62b1..12fa26e2637f 100644 --- a/graylog2-server/src/test/java/org/graylog/collectors/opamp/OpAmpServiceHandleEnrollmentTest.java +++ b/graylog2-server/src/test/java/org/graylog/collectors/opamp/OpAmpServiceHandleEnrollmentTest.java @@ -131,7 +131,7 @@ void enrollsNewCollectorWhenNoExistingRecord() throws Exception { assertThat(response.hasErrorResponse()).isFalse(); assertThat(response.hasConnectionSettings()).isTrue(); verify(collectorInstanceService).enroll(eq(instanceUuid.toString()), eq("new-fleet-id"), any(IssuedCertificate.class), eq("new-token-id")); - verify(collectorInstanceService, never()).reEnroll(any(), any(), any(), any()); + verify(collectorInstanceService, never()).reEnroll(any(), any(), any()); verify(enrollmentTokenService).incrementUsage("new-token-id"); verify(enrollmentTokenService, never()).markUsed(any()); } @@ -146,14 +146,13 @@ void reEnrollsWhenExistingRecordKeyMatchesAndDifferentTokenIncrements() throws E final CollectorInstanceDTO existing = existingRecordFor(instanceUuid.toString(), keyPair, "original-token-id", "original-fleet-id"); when(collectorInstanceService.findByInstanceUid(instanceUuid.toString())).thenReturn(Optional.of(existing)); - when(collectorInstanceService.reEnroll(any(), any(), any(), any())).thenReturn(existing); + when(collectorInstanceService.reEnroll(any(), any(), any())).thenReturn(existing); final var response = opAmpService.handleMessage(message, auth); assertThat(response.hasErrorResponse()).isFalse(); assertThat(response.hasConnectionSettings()).isTrue(); - verify(collectorInstanceService).reEnroll(eq(existing.id()), eq(existing.activeCertificateFingerprint()), - any(IssuedCertificate.class), eq("new-token-id")); + verify(collectorInstanceService).reEnroll(eq(existing), any(IssuedCertificate.class), eq("new-token-id")); verify(collectorInstanceService, never()).enroll(any(), any(), any(), any()); verify(enrollmentTokenService).incrementUsage("new-token-id"); verify(enrollmentTokenService, never()).markUsed(any()); @@ -168,13 +167,12 @@ void reEnrollsWhenExistingRecordKeyMatchesAndSameTokenSkipsIncrement() throws Ex final CollectorInstanceDTO existing = existingRecordFor(instanceUuid.toString(), keyPair, "same-token-id", "same-fleet-id"); when(collectorInstanceService.findByInstanceUid(instanceUuid.toString())).thenReturn(Optional.of(existing)); - when(collectorInstanceService.reEnroll(any(), any(), any(), any())).thenReturn(existing); + when(collectorInstanceService.reEnroll(any(), any(), any())).thenReturn(existing); final var response = opAmpService.handleMessage(message, auth); assertThat(response.hasErrorResponse()).isFalse(); - verify(collectorInstanceService).reEnroll(eq(existing.id()), eq(existing.activeCertificateFingerprint()), - any(IssuedCertificate.class), eq("same-token-id")); + verify(collectorInstanceService).reEnroll(eq(existing), any(IssuedCertificate.class), eq("same-token-id")); verify(enrollmentTokenService, never()).incrementUsage(any()); // The skipped increment must not freeze the token's last_used_at — it would look dormant // to operators even though a collector still relies on it for recovery. @@ -198,7 +196,7 @@ void rejectsReEnrollmentWhenKeyDoesNotMatchStoredCertificate() throws Exception assertThat(response.hasErrorResponse()).isTrue(); assertThat(response.getErrorResponse().getErrorMessage()).isEqualTo("Enrollment rejected."); verify(collectorInstanceService, never()).enroll(any(), any(), any(), any()); - verify(collectorInstanceService, never()).reEnroll(any(), any(), any(), any()); + verify(collectorInstanceService, never()).reEnroll(any(), any(), any()); verify(enrollmentTokenService, never()).incrementUsage(any()); verify(enrollmentTokenService, never()).markUsed(any()); } @@ -215,7 +213,7 @@ void returnsErrorWhenCsrIsMissing() { assertThat(response.hasErrorResponse()).isTrue(); assertThat(response.getErrorResponse().getErrorMessage()).contains("Missing CSR"); verify(collectorInstanceService, never()).enroll(any(), any(), any(), any()); - verify(collectorInstanceService, never()).reEnroll(any(), any(), any(), any()); + verify(collectorInstanceService, never()).reEnroll(any(), any(), any()); verify(enrollmentTokenService, never()).incrementUsage(any()); verify(enrollmentTokenService, never()).markUsed(any()); } diff --git a/graylog2-server/src/test/java/org/graylog/collectors/opamp/auth/AgentTokenServiceTest.java b/graylog2-server/src/test/java/org/graylog/collectors/opamp/auth/AgentTokenServiceTest.java index 5ff10f0e2ae6..2df4f77cf828 100644 --- a/graylog2-server/src/test/java/org/graylog/collectors/opamp/auth/AgentTokenServiceTest.java +++ b/graylog2-server/src/test/java/org/graylog/collectors/opamp/auth/AgentTokenServiceTest.java @@ -101,7 +101,7 @@ void setUp(MongoDBTestService mongodb, ClusterConfigService clusterConfigService certificateService = new CertificateService(mongoCollections, encryptedValueService, CustomizationConfig.empty(), clock); final var clusterIdService = mock(ClusterIdService.class); when(clusterIdService.getString()).thenReturn(TEST_CLUSTER_ID); - collectorInstanceService = new CollectorInstanceService(mongoCollections, clock); + collectorInstanceService = new CollectorInstanceService(mongoCollections, new ClusterEventBus(), clock); final var httpConfiguration = mock(HttpConfiguration.class); when(httpConfiguration.getHttpExternalUri()).thenReturn(java.net.URI.create("https://localhost:443/")); collectorsConfigService = new CollectorsConfigService(clusterConfigService, mock(ClusterEventBus.class), httpConfiguration);