Skip to content

Comments

feat(pam): windows discovery base#5517

Open
x032205 wants to merge 5 commits intomainfrom
pam/windows/discovery
Open

feat(pam): windows discovery base#5517
x032205 wants to merge 5 commits intomainfrom
pam/windows/discovery

Conversation

@x032205
Copy link
Member

@x032205 x032205 commented Feb 19, 2026

Context

Adds Windows Discovery with:

  • AD scanning for machines (resources) & AD user/service accounts

Not yet supported:

  • Local machine account scanning & import (WinRM)
  • Dependency scanning & import (WinRM)

Screenshots

CleanShot 2026-02-22 at 00 29 42@2x CleanShot 2026-02-22 at 00 30 10@2x

Type

  • Fix
  • Feature
  • Improvement
  • Breaking
  • Docs
  • Chore

Checklist

  • Title follows the conventional commit format: type(scope): short description (scope is optional, e.g., fix: prevent crash on sync or fix(api): handle null response).
  • Tested locally
  • Updated docs (if needed)
  • Read the contributing guide

@maidul98
Copy link
Collaborator

maidul98 commented Feb 19, 2026

Snyk checks have passed. No issues have been found so far.

Status Scanner Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@gitguardian
Copy link

gitguardian bot commented Feb 20, 2026

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secret in your pull request
GitGuardian id GitGuardian status Secret Commit Filename
27411356 Triggered Generic Database Assignment 3a65a82 backend/src/ee/services/pam-discovery/active-directory/active-directory-discovery-factory.ts View secret
🛠 Guidelines to remediate hardcoded secrets
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secret safely. Learn here the best practices.
  3. Revoke and rotate this secret.
  4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

@x032205 x032205 marked this pull request as ready for review February 22, 2026 03:30
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 22, 2026

Greptile Summary

This PR implements Windows Discovery for PAM with Active Directory scanning capabilities. The implementation adds comprehensive backend infrastructure including database tables, DALs, services, queue handlers, and API endpoints for discovering AD machines (resources) and AD user/service accounts. The frontend includes new pages and forms for managing discovery sources and viewing discovered resources/accounts.

Key Changes:

  • Added 6 new database tables for discovery sources, runs, and junction tables linking discovered resources/accounts to sources
  • Implemented AD LDAP enumeration via gateway proxy for discovering Windows Servers and domain accounts
  • Service accounts are auto-detected using SPN and naming patterns (svc-, -service, -svc)
  • Scheduled discovery runs daily at 3:00 AM UTC, with manual trigger support
  • Comprehensive permission system with Read, Create, Edit, Delete, and RunScan actions
  • Full audit logging for all discovery operations

Issues Found:

  • Critical: Migration has conflicting constraint - gatewayId is notNullable() but foreign key uses onDelete("SET NULL") (line 21)
  • LDAP filter construction uses user-provided domainFQDN - verify validation prevents LDAP injection
  • No documentation found in /docs folder for this new discovery feature

Not Yet Supported (per PR description):

  • Local machine account scanning via WinRM
  • Dependency scanning via WinRM

Confidence Score: 3/5

  • This PR has one critical database constraint issue that will cause runtime failures, but otherwise implements a well-structured feature with proper security controls
  • Score reflects the critical migration bug (gatewayId constraint conflict) that must be fixed before merging. The implementation is otherwise solid with proper permissions, audit logging, encryption, and error handling. The LDAP enumeration appears safe but needs validation confirmation. Missing documentation is a concern for feature discoverability.
  • Pay close attention to backend/src/db/migrations/20260219070359_pam-discovery.ts (line 21) - the gatewayId constraint conflict must be resolved

Important Files Changed

Filename Overview
backend/src/db/migrations/20260219070359_pam-discovery.ts Creates 6 new PAM discovery tables with proper foreign keys, indices, and idempotency checks
backend/src/ee/services/pam-discovery/active-directory/active-directory-discovery-factory.ts Active Directory LDAP discovery implementation with connection validation and auto-import of resources and accounts
backend/src/ee/services/pam-discovery/pam-discovery-source-service.ts Service layer with proper permission checks for all PAM discovery operations including CRUD and scan triggering
backend/src/ee/services/permission/project-permission.ts Adds PamDiscovery permission subject with Read, Create, Edit, Delete, and RunScan actions
backend/src/ee/routes/v1/pam-discovery-routers/pam-discovery-endpoints.ts Generic endpoint factory for PAM discovery sources with comprehensive audit logging
frontend/src/pages/pam/PamDiscoveryPage/components/PamDiscoverySourceForm/ActiveDirectoryDiscoveryForm.tsx Frontend form for AD discovery with proper validation and password handling using UNCHANGED_PASSWORD_SENTINEL
backend/src/ee/services/pam-discovery/pam-discovery-queue.ts Queue handler for PAM discovery scans with manual and scheduled triggers, runs daily at 3 AM UTC

Last reviewed commit: 6b712df

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

71 files reviewed, 3 comments

Edit Code Review Agent Settings | Greptile

t.string("discoveryType").notNullable();

t.uuid("gatewayId").notNullable();
t.foreign("gatewayId").references("id").inTable(TableName.GatewayV2).onDelete("SET NULL");
Copy link
Contributor

Choose a reason for hiding this comment

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

Column gatewayId is marked as notNullable() on line 20 but the foreign key uses onDelete("SET NULL"). This creates a constraint conflict - if the referenced gateway is deleted, the database will try to set this field to NULL but the NOT NULL constraint will prevent it, causing the delete operation to fail.

Suggested change
t.foreign("gatewayId").references("id").inTable(TableName.GatewayV2).onDelete("SET NULL");
t.foreign("gatewayId").references("id").inTable(TableName.GatewayV2).onDelete("CASCADE");

Choose a reason for hiding this comment

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

valid ^^

we should make it so that deleting the gateway throws an error if it's selected in one of the discovery sources

Comment on lines +73 to +78
const toSlugName = (name: string): string =>
name
.toLowerCase()
.replace(/[^a-z0-9-]/g, "-")
.replace(/-+/g, "-")
.replace(/^-|-$/g, "");
Copy link
Contributor

Choose a reason for hiding this comment

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

The regex patterns in toSlugName use simple character classes without nested quantifiers, so they're safe from ReDoS. However, consider using the re2 package for consistency with the codebase's security policy (Rule 1), even though these specific patterns are not vulnerable.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Choose a reason for hiding this comment

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

valid ^^

Redos!

Comment on lines +234 to +239
const computerEntries = await ldapSearch(
client,
baseDN,
"(&(objectClass=computer)(operatingSystem=*Server*))",
["cn", "dNSHostName", "operatingSystem", "operatingSystemVersion", "objectGUID", "whenChanged"]
);
Copy link
Contributor

Choose a reason for hiding this comment

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

LDAP filters are hardcoded here, which is safe. User-controllable input (configuration.domainFQDN, credentials.username) is used in buildDomainDN and bind operations but not in the filter strings themselves. The baseDN is constructed from the domain FQDN, which could theoretically be manipulated. Verify that configuration.domainFQDN is validated on the frontend and backend to prevent LDAP injection (e.g., no special LDAP characters like *, (, ), \, etc.).

import { TLicenseServiceFactory } from "@app/ee/services/license/license-service";
import { TOidcConfigServiceFactory } from "@app/ee/services/oidc/oidc-config-service";
import { TPamAccountServiceFactory } from "@app/ee/services/pam-account/pam-account-service";
import { TPamDiscoverySourceServiceFactory } from "@app/ee/services/pam-discovery/pam-discovery-source-service";

Choose a reason for hiding this comment

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

IGNORE PLACEMENT OF COMMENT

we might not need the "New" and "Stale" columns altogether if you just do (+1) or (-1) beside the account/resource count?

Image

Comment on lines +1539 to +1542
[TableName.PamDiscoveryRun]: KnexOriginal.CompositeTableType<
TPamDiscoveryRuns,
TPamDiscoveryRunsInsert,
TPamDiscoveryRunsUpdate

Choose a reason for hiding this comment

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

this should be PamDiscoverySourceRun right

Comment on lines +51 to +52
t.integer("newSinceLastRun").defaultTo(0);
t.integer("staleSinceLastRun").defaultTo(0);
Copy link
Member

@sheensantoscapadngan sheensantoscapadngan Feb 22, 2026

Choose a reason for hiding this comment

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

we might not need to track this since these are just values you can derive if needed

Comment on lines +48 to +51
t.integer("resourcesDiscovered").defaultTo(0);
t.integer("accountsDiscovered").defaultTo(0);
t.integer("dependenciesDiscovered").defaultTo(0);
t.integer("newSinceLastRun").defaultTo(0);

Choose a reason for hiding this comment

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

for numeric columns, it would make more sense if it had count prefix or something better

t.string("state").nullable();
t.jsonb("data").notNullable();

t.string("source").notNullable();

Choose a reason for hiding this comment

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

what does this refer to?

t.foreign("resourceId").references("id").inTable(TableName.PamResource).onDelete("CASCADE");
t.index("resourceId");

t.string("dependencyType").notNullable();

Choose a reason for hiding this comment

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

nit - this can just be type

Comment on lines +44 to +48
const discoveryTypeId = discoveryType
.split("-")
.map((word) => word.charAt(0).toUpperCase() + word.slice(1))
.join("");

Choose a reason for hiding this comment

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

maybe just use an enum for this

Comment on lines +213 to +216
metadata: {
sourceId: req.params.discoverySourceId,
discoveryType
}
Copy link
Member

@sheensantoscapadngan sheensantoscapadngan Feb 22, 2026

Choose a reason for hiding this comment

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

let's make all audit logs for this have an identifier that's easily recognizable by users.. like discovery source name

)
);
},
{ prefix: "/discovery" }

Choose a reason for hiding this comment

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

let's do discovery-sources?


const LDAP_TIMEOUT = 30 * 1000;
const LDAP_PAGE_SIZE = 500;
const SERVICE_ACCOUNT_PATTERNS = [/^svc[_-]/i, /[_-]service$/i, /[_-]svc$/i];

Choose a reason for hiding this comment

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

same here

Comment on lines +337 to +373
const existing = await pamResourceDAL.find({
projectId,
resourceType: PamResource.Windows,
name: resourceName
});

if (existing.length > 0) return { resource: existing[0], isNew: false };

const [encryptedConnectionDetails, encryptedResourceMetadata] = await Promise.all([
encryptResourceConnectionDetails({
projectId,
connectionDetails: {
protocol: WindowsProtocol.RDP,
hostname,
port: 3389
} as TWindowsResourceConnectionDetails,
kmsService
}),
encryptResourceMetadata({
projectId,
metadata: {
osVersion: computer.operatingSystem || undefined,
osVersionDetail: computer.operatingSystemVersion || undefined
} as TWindowsResourceMetadata,
kmsService
})
]);

const resource = await pamResourceDAL.create({
projectId,
name: resourceName,
resourceType: PamResource.Windows,
gatewayId,
encryptedConnectionDetails,
encryptedResourceMetadata,
adServerResourceId
});

Choose a reason for hiding this comment

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

we should be doing this inside a transaction right because otherwise the read request might be served by a replica so it would be invalid

Comment on lines +337 to +341
const existing = await pamResourceDAL.find({
projectId,
resourceType: PamResource.Windows,
name: resourceName
});
Copy link
Member

@sheensantoscapadngan sheensantoscapadngan Feb 22, 2026

Choose a reason for hiding this comment

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

don't we want to match against something more specific/definite like an active directory ID? Existing resources might clash with resources discovered so we could add a suffix to the name. Imagine if I had two domains each with a windows machine of the same name

Comment on lines +388 to +392
const existing = await pamAccountDAL.find({
resourceId: adServerResourceId,
name: accountName
});

Copy link
Member

@sheensantoscapadngan sheensantoscapadngan Feb 22, 2026

Choose a reason for hiding this comment

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

same concern here about matching and account name duplicates...

also shouldn't we be checking accountType as well?

Comment on lines +422 to +428
const account = await pamAccountDAL.create({
projectId,
resourceId: adServerResourceId,
name: accountName,
encryptedCredentials,
metadata
});

Choose a reason for hiding this comment

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

same concern about using transactions

triggeredBy,
startedAt: new Date(),
progress: {
adEnumeration: { status: "running" }

Choose a reason for hiding this comment

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

let's use enums for the status

Comment on lines +520 to +532
const { resource: adServerResource, isNew: isAdServerNew } = await upsertAdServerResource(
projectId,
configuration,
gatewayId,
kmsService,
pamResourceDAL
);

await pamDiscoverySourceResourcesDAL.upsertJunction({
discoverySourceId,
resourceId: adServerResource.id,
lastDiscoveredRunId: run.id
});

Choose a reason for hiding this comment

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

this should be in a txn right?

Comment on lines +539 to +552
const { resource, isNew } = await upsertWindowsServerResource(
projectId,
computer,
adServerResource.id,
gatewayId,
kmsService,
pamResourceDAL
);

await pamDiscoverySourceResourcesDAL.upsertJunction({
discoverySourceId,
resourceId: resource.id,
lastDiscoveredRunId: run.id
});

Choose a reason for hiding this comment

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

same here.. it should likely be a txn per computer so that we don't hold up a txn for too long

Comment on lines +564 to +576
const { account, isNew } = await upsertDomainAccount(
projectId,
user,
adServerResource.id,
kmsService,
pamAccountDAL
);

await pamDiscoverySourceAccountsDAL.upsertJunction({
discoverySourceId,
accountId: account.id,
lastDiscoveredRunId: run.id
});

Choose a reason for hiding this comment

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

same remark here

logger.warn({ err, computer: computer.dNSHostName }, "Failed to import Windows Server resource");
}
}

Choose a reason for hiding this comment

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

shouldn't we be updating the discovered resource count right away in case the account processing part fails?

}
};

const scan = async (discoverySourceId: string, triggeredBy: PamDiscoveryRunTrigger, deps: TPamDiscoveryScanDeps) => {

Choose a reason for hiding this comment

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

would it make sense to add a check to ensure that only one scan can happen at a time?

Comment on lines +623 to +625
await pamDiscoverySourceDAL.updateById(discoverySourceId, {
lastRunAt: new Date()
});

Choose a reason for hiding this comment

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

nit - could just be added to the finally block

.optional(),
dependencyScan: z
.object({
status: z.enum(["running", "completed", "failed", "skipped"]),

Choose a reason for hiding this comment

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

enum

totalMachines: z.number().optional(),
scannedMachines: z.number().optional(),
failedMachines: z.number().optional(),
reason: z.string().optional()
Copy link
Member

@sheensantoscapadngan sheensantoscapadngan Feb 22, 2026

Choose a reason for hiding this comment

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

what reason? is this for failure? let's be more specific

Comment on lines +11 to +12
export type TActiveDirectoryDiscoveryCredentials = z.infer<typeof ActiveDirectoryDiscoveryCredentialsSchema>;
export type TActiveDirectoryDiscoveryConfiguration = z.infer<typeof ActiveDirectoryDiscoveryConfigurationSchema>;

Choose a reason for hiding this comment

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

nit - DiscoverySourceCredentials and same for configuration

Comment on lines +35 to +39
export enum PamAccountDependencyType {
WindowsService = "windows-service",
ScheduledTask = "scheduled-task",
IisAppPool = "iis-app-pool"
}

Choose a reason for hiding this comment

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

should we prefix these so that it's specific to windows? windows-scheduled-task as an example

Comment on lines +69 to +75
if (!discoverySource.gatewayId) {
logger.error(
{ discoverySourceId, triggeredBy },
"PAM Discovery Source has no gateway configured, skipping scan"
);
return;
}

Choose a reason for hiding this comment

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

is this something we should to the user in the UI?

Comment on lines +23 to +26
void query.select(selectAllTableCols(TableName.PamDiscoveryRun));

void query.orderBy("createdAt", "desc");

Choose a reason for hiding this comment

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

nit but we can just combine this right?

resourceName: z.string(),
resourceId: z.string().uuid(),
accountName: z.string(),
metadata: z.union([ActiveDirectoryAccountMetadataSchema, WindowsAccountMetadataSchema])
Copy link
Member

@sheensantoscapadngan sheensantoscapadngan Feb 22, 2026

Choose a reason for hiding this comment

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

should we be using discriminatedUnion here or is that not a concern for this use-case?

export const DiscoveredResourceSchema = PamDiscoverySourceResourcesSchema.extend({
resourceName: z.string(),
resourceType: z.nativeEnum(PamResource),
resourceMetadata: z.union([SSHResourceMetadataSchema, WindowsResourceMetadataSchema]).optional()

Choose a reason for hiding this comment

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

same Q here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants