Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
| 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
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- 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
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 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.
Greptile SummaryThis 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:
Issues Found:
Not Yet Supported (per PR description):
Confidence Score: 3/5
Important Files Changed
Last reviewed commit: 6b712df |
| t.string("discoveryType").notNullable(); | ||
|
|
||
| t.uuid("gatewayId").notNullable(); | ||
| t.foreign("gatewayId").references("id").inTable(TableName.GatewayV2).onDelete("SET NULL"); |
There was a problem hiding this comment.
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.
| t.foreign("gatewayId").references("id").inTable(TableName.GatewayV2).onDelete("SET NULL"); | |
| t.foreign("gatewayId").references("id").inTable(TableName.GatewayV2).onDelete("CASCADE"); |
There was a problem hiding this comment.
valid ^^
we should make it so that deleting the gateway throws an error if it's selected in one of the discovery sources
| const toSlugName = (name: string): string => | ||
| name | ||
| .toLowerCase() | ||
| .replace(/[^a-z0-9-]/g, "-") | ||
| .replace(/-+/g, "-") | ||
| .replace(/^-|-$/g, ""); |
There was a problem hiding this comment.
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!
| const computerEntries = await ldapSearch( | ||
| client, | ||
| baseDN, | ||
| "(&(objectClass=computer)(operatingSystem=*Server*))", | ||
| ["cn", "dNSHostName", "operatingSystem", "operatingSystemVersion", "objectGUID", "whenChanged"] | ||
| ); |
There was a problem hiding this comment.
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"; |
| [TableName.PamDiscoveryRun]: KnexOriginal.CompositeTableType< | ||
| TPamDiscoveryRuns, | ||
| TPamDiscoveryRunsInsert, | ||
| TPamDiscoveryRunsUpdate |
There was a problem hiding this comment.
this should be PamDiscoverySourceRun right
| t.integer("newSinceLastRun").defaultTo(0); | ||
| t.integer("staleSinceLastRun").defaultTo(0); |
There was a problem hiding this comment.
we might not need to track this since these are just values you can derive if needed
| t.integer("resourcesDiscovered").defaultTo(0); | ||
| t.integer("accountsDiscovered").defaultTo(0); | ||
| t.integer("dependenciesDiscovered").defaultTo(0); | ||
| t.integer("newSinceLastRun").defaultTo(0); |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
what does this refer to?
| t.foreign("resourceId").references("id").inTable(TableName.PamResource).onDelete("CASCADE"); | ||
| t.index("resourceId"); | ||
|
|
||
| t.string("dependencyType").notNullable(); |
There was a problem hiding this comment.
nit - this can just be type
| const discoveryTypeId = discoveryType | ||
| .split("-") | ||
| .map((word) => word.charAt(0).toUpperCase() + word.slice(1)) | ||
| .join(""); | ||
|
|
There was a problem hiding this comment.
maybe just use an enum for this
| metadata: { | ||
| sourceId: req.params.discoverySourceId, | ||
| discoveryType | ||
| } |
There was a problem hiding this comment.
let's make all audit logs for this have an identifier that's easily recognizable by users.. like discovery source name
| ) | ||
| ); | ||
| }, | ||
| { prefix: "/discovery" } |
There was a problem hiding this comment.
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]; |
| 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 | ||
| }); |
There was a problem hiding this comment.
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
| const existing = await pamResourceDAL.find({ | ||
| projectId, | ||
| resourceType: PamResource.Windows, | ||
| name: resourceName | ||
| }); |
There was a problem hiding this comment.
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
| const existing = await pamAccountDAL.find({ | ||
| resourceId: adServerResourceId, | ||
| name: accountName | ||
| }); | ||
|
|
There was a problem hiding this comment.
same concern here about matching and account name duplicates...
also shouldn't we be checking accountType as well?
| const account = await pamAccountDAL.create({ | ||
| projectId, | ||
| resourceId: adServerResourceId, | ||
| name: accountName, | ||
| encryptedCredentials, | ||
| metadata | ||
| }); |
There was a problem hiding this comment.
same concern about using transactions
| triggeredBy, | ||
| startedAt: new Date(), | ||
| progress: { | ||
| adEnumeration: { status: "running" } |
There was a problem hiding this comment.
let's use enums for the status
| const { resource: adServerResource, isNew: isAdServerNew } = await upsertAdServerResource( | ||
| projectId, | ||
| configuration, | ||
| gatewayId, | ||
| kmsService, | ||
| pamResourceDAL | ||
| ); | ||
|
|
||
| await pamDiscoverySourceResourcesDAL.upsertJunction({ | ||
| discoverySourceId, | ||
| resourceId: adServerResource.id, | ||
| lastDiscoveredRunId: run.id | ||
| }); |
There was a problem hiding this comment.
this should be in a txn right?
| const { resource, isNew } = await upsertWindowsServerResource( | ||
| projectId, | ||
| computer, | ||
| adServerResource.id, | ||
| gatewayId, | ||
| kmsService, | ||
| pamResourceDAL | ||
| ); | ||
|
|
||
| await pamDiscoverySourceResourcesDAL.upsertJunction({ | ||
| discoverySourceId, | ||
| resourceId: resource.id, | ||
| lastDiscoveredRunId: run.id | ||
| }); |
There was a problem hiding this comment.
same here.. it should likely be a txn per computer so that we don't hold up a txn for too long
| const { account, isNew } = await upsertDomainAccount( | ||
| projectId, | ||
| user, | ||
| adServerResource.id, | ||
| kmsService, | ||
| pamAccountDAL | ||
| ); | ||
|
|
||
| await pamDiscoverySourceAccountsDAL.upsertJunction({ | ||
| discoverySourceId, | ||
| accountId: account.id, | ||
| lastDiscoveredRunId: run.id | ||
| }); |
| logger.warn({ err, computer: computer.dNSHostName }, "Failed to import Windows Server resource"); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
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) => { |
There was a problem hiding this comment.
would it make sense to add a check to ensure that only one scan can happen at a time?
| await pamDiscoverySourceDAL.updateById(discoverySourceId, { | ||
| lastRunAt: new Date() | ||
| }); |
There was a problem hiding this comment.
nit - could just be added to the finally block
| .optional(), | ||
| dependencyScan: z | ||
| .object({ | ||
| status: z.enum(["running", "completed", "failed", "skipped"]), |
| totalMachines: z.number().optional(), | ||
| scannedMachines: z.number().optional(), | ||
| failedMachines: z.number().optional(), | ||
| reason: z.string().optional() |
There was a problem hiding this comment.
what reason? is this for failure? let's be more specific
| export type TActiveDirectoryDiscoveryCredentials = z.infer<typeof ActiveDirectoryDiscoveryCredentialsSchema>; | ||
| export type TActiveDirectoryDiscoveryConfiguration = z.infer<typeof ActiveDirectoryDiscoveryConfigurationSchema>; |
There was a problem hiding this comment.
nit - DiscoverySourceCredentials and same for configuration
| export enum PamAccountDependencyType { | ||
| WindowsService = "windows-service", | ||
| ScheduledTask = "scheduled-task", | ||
| IisAppPool = "iis-app-pool" | ||
| } |
There was a problem hiding this comment.
should we prefix these so that it's specific to windows? windows-scheduled-task as an example
| if (!discoverySource.gatewayId) { | ||
| logger.error( | ||
| { discoverySourceId, triggeredBy }, | ||
| "PAM Discovery Source has no gateway configured, skipping scan" | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
is this something we should to the user in the UI?
| void query.select(selectAllTableCols(TableName.PamDiscoveryRun)); | ||
|
|
||
| void query.orderBy("createdAt", "desc"); | ||
|
|
There was a problem hiding this comment.
nit but we can just combine this right?
| resourceName: z.string(), | ||
| resourceId: z.string().uuid(), | ||
| accountName: z.string(), | ||
| metadata: z.union([ActiveDirectoryAccountMetadataSchema, WindowsAccountMetadataSchema]) |
There was a problem hiding this comment.
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() |

Context
Adds Windows Discovery with:
Not yet supported:
Screenshots
Type
Checklist
type(scope): short description(scope is optional, e.g.,fix: prevent crash on syncorfix(api): handle null response).