Conversation
|
/run-security-scan |
alexcos20
left a comment
There was a problem hiding this comment.
AI automated code review (Gemini 3).
Overall risk: medium
Summary:
This pull request introduces the capability to authenticate with private Docker/OCI registries when validating and pulling images for Compute-to-Data (C2D) operations. It defines a new environment variable DOCKER_REGISTRY_AUTHS to store registry credentials (username/password or auth token). The changes include updating data types and configuration schemas, modifying the C2DEngineDocker to integrate authentication for getDockerManifest and docker.pull methods, and converting relevant static methods to instance methods. Comprehensive documentation for the new configuration is added, along with new integration tests to verify the authentication logic.
Comments:
• [ERROR][bug] The dockerRegistryAuth interface defines username, password, and auth as required string properties. However, the documentation (in docs/env.md) explicitly states that 'Each registry entry must provide either username+password or auth.' This implies that these fields should be optional (e.g., username?: string). Please update the interface to reflect this optionality to avoid type mismatches and allow flexible configuration where not all three fields are present.
• [WARNING][style] The check if (!this.dockerRegistryAuths) is redundant. According to the OceanNodeConfigSchema, dockerRegistrysAuth defaults to an empty object {} if not explicitly provided. This means this.dockerRegistryAuths will always be an object and never null or undefined. You can safely remove this check.
• [WARNING][bug] The pullOptions.authconfig object is constructed to always include username, password, and auth, even if some of these fields are empty strings or undefined from the dockerRegistryAuth object. It's generally better practice to only include the fields that are actually provided and necessary for dockerode's authconfig. For example, if auth is present, username and password might not be needed by dockerode, or vice-versa. Consider conditionally adding username and password if they exist and auth is not used, to avoid sending redundant or empty fields to the Docker daemon. A more precise construction might look like:
pullOptions.authconfig = {
serveraddress,
...(dockerRegistryAuth.auth ? { auth: authString } : { username: dockerRegistryAuth.username, password: dockerRegistryAuth.password })
};(assuming username and password are guaranteed to be present if auth is not).
• [INFO][style] The private dockerRegistryAuths: dockerRegistrysAuth property in C2DEngines appears to be redundant. The dockerRegistrysAuth is already part of the OceanNodeConfig and is directly passed to the C2DEngineDocker constructor when engines are created. C2DEngines acts as a factory and doesn't seem to store or use this property directly itself after initialization. Consider removing it.
• [ERROR][bug] In the imageCleanup.test.ts file, null is passed for dockerRegistryAuths in the C2DEngineDocker constructor. This is inconsistent with the OceanNodeConfigSchema which defaults dockerRegistrysAuth to an empty object {}. The C2DEngineDocker constructor also expects dockerRegistrysAuth: dockerRegistrysAuth. Passing null could lead to runtime type errors if methods attempt to access properties of dockerRegistryAuths assuming it's an object. Please change null to {} or an appropriate mock dockerRegistrysAuth object to align with the schema and type expectations.
• [ERROR][bug] The DockerRegistryAuthSchema defines username, password, and auth as required z.string() properties. This directly conflicts with the documentation in docs/env.md which states that a registry entry must provide 'either username+password or auth'. This schema will cause validation to fail if users configure only auth or only username/password. Please adjust the schema to correctly reflect this 'either/or' condition, for example, by making username, password, and auth optional (z.string().optional()) and then using z.refine to enforce the 'at least one' or 'either/or' logic.
|
/run-security-scan |
alexcos20
left a comment
There was a problem hiding this comment.
AI automated code review (Gemini 3).
Overall risk: low
Summary:
This pull request introduces support for authenticating with private Docker registries when pulling images for C2D compute jobs. It achieves this by adding a new configuration option DOCKER_REGISTRY_AUTHS which accepts a JSON object mapping registry URLs to authentication credentials (username/password or a pre-encoded auth token). The changes include: updating type definitions, modifying the C2D engine base and Docker implementations to consume and apply these credentials during manifest fetching and image pulling, adding detailed documentation for the new configuration, and introducing robust Zod schemas for validation. A comprehensive integration test suite has been added to cover various authentication scenarios and schema validation.
Comments:
• [INFO][style] The // eslint-disable-next-line require-await comment is added for public abstract checkDockerImage. While it's common to disable this rule for abstract methods where the implementation decides async status, it's worth a quick thought if there's a reason the abstract method must return a Promise but doesn't always require await in its implementation. In this specific case, it seems reasonable given the Promise<ValidateParams> return type, implying potential asynchronous operations.
• [INFO][performance] Changing static parseImage to private parseImage is a good refactoring, making it an instance method and allowing access to this.dockerRegistryAuths indirectly. This is a positive change for the design, aligning image parsing with the engine's specific configuration rather than a global static utility.
• [WARNING][security] When constructing the Basic authentication string Buffer.from(${dockerRegistryAuth.username}:${dockerRegistryAuth.password}).toString('base64'), ensure that dockerRegistryAuth.username and dockerRegistryAuth.password are guaranteed to be non-empty strings if present. The Zod schema (DockerRegistryAuthSchema in src/utils/config/schemas.ts) already handles this by requiring both to be non-empty if auth is not provided. This is good, but it's a critical point for security that these values are validated before reaching this point to avoid encoding empty credentials.
• [INFO][performance] Similar to the getDockerManifest method, the serveraddress extraction for pullOptions.authconfig is well-handled by parsing the registry URL. This correctly accounts for potential ports in the registry URL, which is important for more complex registry setups.
• [INFO][testing] The integration tests for DockerRegistryAuthSchema are excellent. They cover all critical validation paths, including required combinations (auth OR username+password) and rejection of incomplete or empty credentials. This robust testing of the schema itself significantly increases confidence in the overall feature's stability and security implications.
• [INFO][security] The refine logic in DockerRegistryAuthSchema is very well implemented. It accurately enforces the rule that either auth must be present and non-empty, OR both username and password must be present and non-empty. This is crucial for preventing misconfigurations that could lead to failed authentication attempts or security vulnerabilities from partially supplied credentials.
|
Do not merge, because #1197 will be merged here before |
Fixes #1180
Add Docker Registry Authentication Support
Summary
This PR adds support for authenticating with private Docker/OCI registries when validating and pulling Docker images. The implementation allows nodes to access private container registries (Docker Hub, GHCR, GitLab Container Registry, etc.) by configuring registry credentials.
Changes
Configuration
DOCKER_REGISTRY_AUTHS- JSON object mapping registry URLs to authentication credentialsdockerRegistryAuth- Parsed fromDOCKER_REGISTRY_AUTHSenvironment variableDockerRegistryAuthSchemaandDockerRegistryAuthsSchemato validate auth configurationCore Implementation
Config Schema (
src/utils/config/schemas.ts):DockerRegistryAuthSchemawithusername,password, andauthfieldsDockerRegistryAuthsSchemaas a record mapping registry URLs to auth objectsOceanNodeConfigSchemawith JSON parsing supportBase Class (
src/components/c2d/compute_engine_base.ts):getDockerRegistryAuth(registry: string)method to retrieve auth credentials for a registryc2dEnginesInstancereference for static method accessDocker Engine (
src/components/c2d/compute_engine_docker.ts):getDockerManifest(): Updated to use registry authenticationusername/passwordand pre-encodedauthstring formatspullImage(): Updated to use registry authenticationauthconfigobject with credentialsauthconfigtodocker.pull()for authenticated image pullsC2D Engines (
src/components/c2d/compute_engines.ts):getDockerRegistryAuth()public methodc2dEnginesInstancereference when creating enginesdockerRegistryAuthfrom configDocumentation
docs/env.md: Added comprehensive documentation forDOCKER_REGISTRY_AUTHSenvironment variableTesting
src/test/integration/dockerRegistryAuth.test.ts: New integration test suiteConfiguration Example
Or in
config.json:{ "dockerRegistryAuth": { "https://registry-1.docker.io": { "username": "myuser", "password": "mypassword" } } }Authentication Methods Supported
usernameandpasswordfieldsauthstring (alternative to username/password)Backward Compatibility
Testing
Related Issues
Fixes: [Add support for private Docker registry authentication]