feat: Add ConnectionRef to DataSource for pluggable external credential resolution - #6642
feat: Add ConnectionRef to DataSource for pluggable external credential resolution#6642ntkathole wants to merge 1 commit into
Conversation
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6642 +/- ##
==========================================
+ Coverage 46.79% 46.81% +0.02%
==========================================
Files 415 416 +1
Lines 50395 50676 +281
Branches 7215 7261 +46
==========================================
+ Hits 23581 23724 +143
- Misses 25162 25296 +134
- Partials 1652 1656 +4
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
4f3809d to
948b262
Compare
ntkathole
changed the title
ed8fece to
35e9afc
Compare
|
@jyejare @patelchaitany @aniketpalu @Vperiodt Please review |
jyejare
left a comment
jyejare
left a comment
There was a problem hiding this comment.
This PR adds ConnectionRef to DataSource for pluggable external credential resolution, enabling per-datasource credentials via external providers like Kubernetes Secrets, HashiCorp Vault, and cloud secret managers. The implementation is well-architected with comprehensive documentation, but has some critical security concerns and missing integration points that need addressing.
| addr: str = field(default_factory=lambda: os.environ.get("VAULT_ADDR", "")) | ||
| token: str = field(default_factory=lambda: os.environ.get("VAULT_TOKEN", "")) | ||
| role: str = field(default_factory=lambda: os.environ.get("VAULT_ROLE", "")) | ||
| auth_method: str = field( | ||
| default_factory=lambda: os.environ.get("VAULT_AUTH_METHOD", "token") | ||
| ) | ||
|
|
||
|
|
||
| class VaultProvider(CredentialProvider): | ||
| """Reads credentials from HashiCorp Vault KV v2 secrets engine. | ||
|
|
||
| ``ref.name`` is the Vault secret path (e.g. ``"secret/data/feast/my-conn"``). | ||
| ``ref.namespace`` is the Vault mount point (defaults to ``"secret"``). | ||
|
|
There was a problem hiding this comment.
[Critical] Unsafe credentials returned from Kubernetes secrets without authentication validation
The KubernetesSecretProvider base64-decodes and returns credentials without validating the service account's permissions. This could lead to privilege escalation if a malicious actor can modify the Secret or if the Pod has broader access than intended. The provider should validate that the current service account has explicit read access to the specific Secret before returning credentials.
Suggested:
| addr: str = field(default_factory=lambda: os.environ.get("VAULT_ADDR", "")) | |
| token: str = field(default_factory=lambda: os.environ.get("VAULT_TOKEN", "")) | |
| role: str = field(default_factory=lambda: os.environ.get("VAULT_ROLE", "")) | |
| auth_method: str = field( | |
| default_factory=lambda: os.environ.get("VAULT_AUTH_METHOD", "token") | |
| ) | |
| class VaultProvider(CredentialProvider): | |
| """Reads credentials from HashiCorp Vault KV v2 secrets engine. | |
| ``ref.name`` is the Vault secret path (e.g. ``"secret/data/feast/my-conn"``). | |
| ``ref.namespace`` is the Vault mount point (defaults to ``"secret"``). | |
| # Validate service account has read access to this secret | |
| try: | |
| v1.read_namespaced_secret(name=ref.name, namespace=namespace, exact=True) | |
| except client.exceptions.ApiException as exc: | |
| if exc.status == 403: | |
| raise CredentialResolutionError( | |
| f"Service account lacks read access to Secret '{ref.name}' " | |
| f"in namespace '{namespace}'" | |
| ) from exc | |
| raise | |
| return { | |
| key: base64.b64decode(value).decode("utf-8") | |
| for key, value in (secret.data or {}).items() | |
| } |
There was a problem hiding this comment.
It delegates authentication and authorization entirely to the Kubernetes API server, if the service account lacks get permission on secrets in the target namespace, the call returns 403.
| except ImportError as exc: | ||
| raise CredentialResolutionError( | ||
| "hvac package is required for the 'vault' credential provider. " | ||
| "Install it with: pip install hvac" | ||
| ) from exc | ||
|
|
||
| vault_client = hvac.Client(url=self._config.addr, token=self._config.token) | ||
| if not vault_client.is_authenticated(): | ||
| raise CredentialResolutionError( | ||
| "Vault client is not authenticated. " | ||
| "Set VAULT_ADDR and VAULT_TOKEN, or configure auth_method." | ||
| ) | ||
|
|
||
| mount_point = ref.namespace or "secret" | ||
| try: |
There was a problem hiding this comment.
[Critical] VaultProvider exposes all secret data without field-level access control
The VaultProvider returns all key-value pairs from a Vault secret path without any field-level filtering or access validation. This violates the principle of least privilege and could expose unintended credentials. The provider should either accept a list of required fields or implement field-level access validation.
Suggested:
| except ImportError as exc: | |
| raise CredentialResolutionError( | |
| "hvac package is required for the 'vault' credential provider. " | |
| "Install it with: pip install hvac" | |
| ) from exc | |
| vault_client = hvac.Client(url=self._config.addr, token=self._config.token) | |
| if not vault_client.is_authenticated(): | |
| raise CredentialResolutionError( | |
| "Vault client is not authenticated. " | |
| "Set VAULT_ADDR and VAULT_TOKEN, or configure auth_method." | |
| ) | |
| mount_point = ref.namespace or "secret" | |
| try: | |
| data = response.get("data", {}).get("data", {}) | |
| # Only return explicitly requested fields if specified | |
| if ref.params and "fields" in ref.params: | |
| requested_fields = ref.params["fields"].split(",") | |
| data = {k: v for k, v in data.items() if k in requested_fields} | |
| return {k: str(v) for k, v in data.items()} |
There was a problem hiding this comment.
I think this can be nice to have enhancement. This is consistent with how every Vault client operates, it's not a security gap - it's a usability improvement
| self.connection_ref = connection_ref | ||
| now = _utc_now() | ||
| self.created_timestamp = now | ||
| self.last_updated_timestamp = now | ||
|
|
||
| def resolve_credentials(self) -> Optional[Dict[str, str]]: | ||
| """Resolve credentials from the ``connection_ref`` if set. | ||
|
|
||
| Returns ``None`` when no ``connection_ref`` is configured (ambient | ||
| credentials are used instead). | ||
| """ | ||
| if self.connection_ref is None: | ||
| return None | ||
| from feast.credentials import resolve_credentials | ||
|
|
||
| return resolve_credentials(self.connection_ref) |
There was a problem hiding this comment.
[Warning] Missing credential validation in resolve_credentials method
The resolve_credentials method directly calls the credential provider without any validation or error handling for malformed or missing credentials. This could lead to runtime errors that are difficult to debug. The method should validate the resolved credentials and provide informative error messages.
Suggested:
| self.connection_ref = connection_ref | |
| now = _utc_now() | |
| self.created_timestamp = now | |
| self.last_updated_timestamp = now | |
| def resolve_credentials(self) -> Optional[Dict[str, str]]: | |
| """Resolve credentials from the ``connection_ref`` if set. | |
| Returns ``None`` when no ``connection_ref`` is configured (ambient | |
| credentials are used instead). | |
| """ | |
| if self.connection_ref is None: | |
| return None | |
| from feast.credentials import resolve_credentials | |
| return resolve_credentials(self.connection_ref) | |
| def resolve_credentials(self) -> Optional[Dict[str, str]]: | |
| """Resolve credentials from the ``connection_ref`` if set. | |
| Returns ``None`` when no ``connection_ref`` is configured (ambient | |
| credentials are used instead). | |
| Raises: | |
| CredentialResolutionError: If credentials cannot be resolved. | |
| """ | |
| if self.connection_ref is None: | |
| return None | |
| from feast.credentials import resolve_credentials, CredentialResolutionError | |
| try: | |
| creds = resolve_credentials(self.connection_ref) | |
| if not creds: | |
| raise CredentialResolutionError( | |
| f"No credentials returned from provider '{self.connection_ref.provider}' " | |
| f"for name '{self.connection_ref.name}'" | |
| ) | |
| return creds | |
| except Exception as e: | |
| raise CredentialResolutionError( | |
| f"Failed to resolve credentials for DataSource '{self.name}': {e}" | |
| ) from e |
There was a problem hiding this comment.
resolve_credentials function is intentionally a thin wrapper - it delegates to providers that already handle their own error cases.
| message ConnectionRef { | ||
| // Credential provider type: "kubernetes", "vault", "aws-secrets-manager", | ||
| // "gcp-secret-manager", "azure-key-vault", "env". | ||
| string provider = 1; | ||
|
|
||
| // Provider-specific name: K8s Secret name, Vault path, env var prefix, etc. | ||
| string name = 2; | ||
|
|
||
| // Optional scope qualifier: K8s namespace, Vault mount, AWS region, etc. |
There was a problem hiding this comment.
[Suggestion] ConnectionRef proto should include validation constraints
The ConnectionRef proto definition lacks validation constraints that could prevent common misconfigurations. Adding field validation (e.g., required provider, valid provider types, namespace format) would catch configuration errors early rather than at runtime. This could be part of next improvement PR though.
Optional Comment!
Suggested:
| message ConnectionRef { | |
| // Credential provider type: "kubernetes", "vault", "aws-secrets-manager", | |
| // "gcp-secret-manager", "azure-key-vault", "env". | |
| string provider = 1; | |
| // Provider-specific name: K8s Secret name, Vault path, env var prefix, etc. | |
| string name = 2; | |
| // Optional scope qualifier: K8s namespace, Vault mount, AWS region, etc. | |
| message ConnectionRef { | |
| // Credential provider type: "kubernetes", "vault", "aws-secrets-manager", | |
| // "gcp-secret-manager", "azure-key-vault", "env". | |
| // Required field. | |
| string provider = 1 [(validate.rules).string.min_len = 1]; | |
| // Provider-specific name: K8s Secret name, Vault path, env var prefix, etc. | |
| // Required field. | |
| string name = 2 [(validate.rules).string.min_len = 1]; |
| f"{TAG_PREFIX}provider": self.provider, | ||
| f"{TAG_PREFIX}name": self.name, | ||
| } | ||
| if self.namespace: | ||
| tags[f"{TAG_PREFIX}namespace"] = self.namespace | ||
| if self.connection_type: | ||
| tags[f"{TAG_PREFIX}connection-type"] = self.connection_type | ||
| if self.auth_type and self.auth_type != "secret": | ||
| tags[f"{TAG_PREFIX}auth-type"] = self.auth_type | ||
| for key, value in self.params.items(): | ||
| tags[f"{TAG_PREFIX}param.{key}"] = value | ||
| return tags | ||
|
|
||
| @classmethod | ||
| def from_tags(cls, tags: Dict[str, str]) -> Optional["ConnectionRef"]: | ||
| """Deserialize from DataSource ``tags``. Returns *None* when no | ||
| connection-ref tags are present.""" | ||
| provider = tags.get(f"{TAG_PREFIX}provider") |
There was a problem hiding this comment.
[Suggestion] ConnectionRef to_tags serialization should validate tag key limits
The to_tags method doesn't validate that the generated tag keys don't exceed storage limits or contain invalid characters. Some data source backends may have restrictions on tag key length or character sets that could cause silent failures during serialization.
Suggested:
| f"{TAG_PREFIX}provider": self.provider, | |
| f"{TAG_PREFIX}name": self.name, | |
| } | |
| if self.namespace: | |
| tags[f"{TAG_PREFIX}namespace"] = self.namespace | |
| if self.connection_type: | |
| tags[f"{TAG_PREFIX}connection-type"] = self.connection_type | |
| if self.auth_type and self.auth_type != "secret": | |
| tags[f"{TAG_PREFIX}auth-type"] = self.auth_type | |
| for key, value in self.params.items(): | |
| tags[f"{TAG_PREFIX}param.{key}"] = value | |
| return tags | |
| @classmethod | |
| def from_tags(cls, tags: Dict[str, str]) -> Optional["ConnectionRef"]: | |
| """Deserialize from DataSource ``tags``. Returns *None* when no | |
| connection-ref tags are present.""" | |
| provider = tags.get(f"{TAG_PREFIX}provider") | |
| def to_tags(self) -> Dict[str, str]: | |
| """Serialize into DataSource ``tags`` dict entries.""" | |
| tags: Dict[str, str] = { | |
| f"{TAG_PREFIX}provider": self.provider, | |
| f"{TAG_PREFIX}name": self.name, | |
| } | |
| if self.namespace: | |
| tags[f"{TAG_PREFIX}namespace"] = self.namespace | |
| # Validate tag key lengths (common limit is 128 chars) | |
| for key in tags.keys(): | |
| if len(key) > 128: | |
| raise ValueError(f"Tag key too long: {key} (max 128 chars)") | |
| return tags |
There was a problem hiding this comment.
The same type used across all Feast objects (FeatureView.tags, Entity.tags, etc.), none of which validate tag keys.
|
|
||
| @staticmethod | ||
| def create_filesystem_and_path( | ||
| path: str, s3_endpoint_override: str | ||
| path: str, | ||
| s3_endpoint_override: str, | ||
| resolved_credentials: Optional[Dict[str, str]] = None, | ||
| ) -> Tuple[Optional[FileSystem], str]: | ||
| if path.startswith("s3://"): | ||
| s3fs = S3FileSystem( | ||
| endpoint_override=s3_endpoint_override if s3_endpoint_override else None | ||
| ) | ||
| kwargs: Dict[str, Optional[str]] = {} | ||
| if s3_endpoint_override: | ||
| kwargs["endpoint_override"] = s3_endpoint_override | ||
|
|
||
| if resolved_credentials: | ||
| access_key = resolved_credentials.get("AWS_ACCESS_KEY_ID", "") | ||
| secret_key = resolved_credentials.get("AWS_SECRET_ACCESS_KEY", "") | ||
| session_token = resolved_credentials.get("AWS_SESSION_TOKEN") | ||
| region = resolved_credentials.get("AWS_DEFAULT_REGION") | ||
| if access_key and secret_key: | ||
| kwargs["access_key"] = access_key | ||
| kwargs["secret_key"] = secret_key | ||
| if session_token: | ||
| kwargs["session_token"] = session_token | ||
| if region: | ||
| kwargs["region"] = region | ||
|
|
||
| s3fs = S3FileSystem(**kwargs) |
There was a problem hiding this comment.
[Suggestion] S3 credential handling should support all AWS credential types
The S3FileSystem credential handling only supports access_key/secret_key authentication but AWS supports many other credential types (IAM roles, STS, OIDC, etc.). The implementation should be more flexible to support the full range of AWS authentication methods.
Again this could be the part of future improvements. Or we should at least document that only access_key / secret_key type auth.
Suggested:
| @staticmethod | |
| def create_filesystem_and_path( | |
| path: str, s3_endpoint_override: str | |
| path: str, | |
| s3_endpoint_override: str, | |
| resolved_credentials: Optional[Dict[str, str]] = None, | |
| ) -> Tuple[Optional[FileSystem], str]: | |
| if path.startswith("s3://"): | |
| s3fs = S3FileSystem( | |
| endpoint_override=s3_endpoint_override if s3_endpoint_override else None | |
| ) | |
| kwargs: Dict[str, Optional[str]] = {} | |
| if s3_endpoint_override: | |
| kwargs["endpoint_override"] = s3_endpoint_override | |
| if resolved_credentials: | |
| access_key = resolved_credentials.get("AWS_ACCESS_KEY_ID", "") | |
| secret_key = resolved_credentials.get("AWS_SECRET_ACCESS_KEY", "") | |
| session_token = resolved_credentials.get("AWS_SESSION_TOKEN") | |
| region = resolved_credentials.get("AWS_DEFAULT_REGION") | |
| if access_key and secret_key: | |
| kwargs["access_key"] = access_key | |
| kwargs["secret_key"] = secret_key | |
| if session_token: | |
| kwargs["session_token"] = session_token | |
| if region: | |
| kwargs["region"] = region | |
| s3fs = S3FileSystem(**kwargs) | |
| if resolved_credentials: | |
| # Support multiple AWS credential formats | |
| access_key = resolved_credentials.get("AWS_ACCESS_KEY_ID") or resolved_credentials.get("access_key_id") | |
| secret_key = resolved_credentials.get("AWS_SECRET_ACCESS_KEY") or resolved_credentials.get("secret_access_key") | |
| session_token = resolved_credentials.get("AWS_SESSION_TOKEN") or resolved_credentials.get("session_token") | |
| region = resolved_credentials.get("AWS_DEFAULT_REGION") or resolved_credentials.get("region") | |
| if access_key and secret_key: | |
| kwargs["access_key"] = access_key | |
| kwargs["secret_key"] = secret_key | |
| # Support role-based authentication | |
| elif "role_arn" in resolved_credentials: | |
| kwargs["role_arn"] = resolved_credentials["role_arn"] | |
| if "external_id" in resolved_credentials: | |
| kwargs["external_id"] = resolved_credentials["external_id"] |
jyejare
left a comment
jyejare
left a comment
There was a problem hiding this comment.
This PR introduces a well-architected ConnectionRef feature for pluggable external credential resolution in Feast DataSources. The implementation follows good patterns with comprehensive documentation, built-in providers for Kubernetes/Vault/environment variables, and proper integration across all data source types. However, there are several critical security concerns around credential handling and some missing test coverage.
…al resolution Signed-off-by: ntkathole <nikhilkathole2683@gmail.com>
4 participants
What this PR does / why we need it:
Feast DataSources today have no mechanism to reference external credentials - authentication relies on ambient environment variables or a single global
offline_storeconfig infeature_store.yaml.This PR introduces
ConnectionRef- which backend to use, how to authenticate, and where to connect - an optional reference onDataSourcethat points to an external credential store (Kubernetes Secrets, HashiCorp Vault, cloud secret managers, or environment variables). Credentials are resolvedat runtime by a pluggable
CredentialProviderinterface.This enables:
connection_refspecifyingconnection_type, credentials, and connection params - no server restart needed.