diff --git a/backend/openapi.json b/backend/openapi.json index 8ebd6722..14c241d5 100644 --- a/backend/openapi.json +++ b/backend/openapi.json @@ -40729,6 +40729,13 @@ }, "provider": { "type": "string", + "enum": [ + "aws", + "azure", + "gcp", + "ibm", + "ssh" + ], "title": "Provider" }, "aws_auth_method": { @@ -41531,7 +41538,14 @@ "provider": { "anyOf": [ { - "type": "string" + "type": "string", + "enum": [ + "aws", + "azure", + "gcp", + "ibm", + "ssh" + ] }, { "type": "null" diff --git a/backend/routes/credential_templates.py b/backend/routes/credential_templates.py index 64be9be6..19aeaa3c 100644 --- a/backend/routes/credential_templates.py +++ b/backend/routes/credential_templates.py @@ -5,6 +5,7 @@ """ import logging from datetime import datetime +from typing import Literal from fastapi import APIRouter, Depends from fastapi.responses import JSONResponse @@ -29,7 +30,7 @@ class CredentialTemplateBase(BaseModel): name: str description: str | None = None - provider: str + provider: Literal["aws", "azure", "gcp", "ibm", "ssh"] aws_auth_method: str | None = None aws_profile: str | None = None region: str | None = None @@ -77,7 +78,7 @@ class CredentialTemplateCreate(CredentialTemplateBase): class CredentialTemplateUpdate(BaseModel): name: str | None = None description: str | None = None - provider: str | None = None + provider: Literal["aws", "azure", "gcp", "ibm", "ssh"] | None = None aws_auth_method: str | None = None aws_profile: str | None = None region: str | None = None diff --git a/backend/services/credential_template_service.py b/backend/services/credential_template_service.py index 7fb2d4ef..9d01893c 100644 --- a/backend/services/credential_template_service.py +++ b/backend/services/credential_template_service.py @@ -25,6 +25,43 @@ logger = logging.getLogger(__name__) +# Canonical set of providers a credential template may declare. +# +# This is the single source of truth for provider validation. It must stay in +# lock-step with every consumer that branches on ``template.provider`` to inject +# or resolve credentials, otherwise a template can be created that looks healthy +# in the API yet contributes no credentials at deploy time (see issue #191): +# - ``aws`` -> AWS_* env + TF_VAR_* mirror (credentials_service, terraform-env injection) +# - ``ibm`` -> IC_API_KEY / IBMCLOUD_API_KEY (credentials_service, terraform-env injection) +# - ``gcp`` -> GKE kubeconfig token (credentials_service.get_gcp_service_account_info; post-provision cluster access, not terraform env) +# - ``azure`` -> AKS kubeconfig token (execution.engine_router; post-provision cluster access, not terraform env) +# - ``ssh`` -> SSH tunnel / on-prem (credential test + tunnel manager) +# These are exactly the four cloud providers the UI offers plus the legacy +# ``ssh`` on-prem provider. Adding a new provider here without wiring its +# consumer (or vice-versa) is the bug this constant exists to prevent. NOTE: +# ``aws``/``ibm`` inject credentials into the terraform provisioning env, so +# #191's "looks healthy, injects nothing" class is fully closed for them; +# ``azure``/``gcp`` are consumed only for post-provision cluster access, so a +# mis-set provider is still rejected here but the underlying #191 class for the +# terraform-env path only ever applied to aws/ibm. +SUPPORTED_PROVIDERS: frozenset[str] = frozenset({"aws", "gcp", "azure", "ibm", "ssh"}) + + +def validate_provider(provider: Any) -> str: + """Return ``provider`` if it is a supported credential-template provider. + + Raises ``BadRequestError`` with a clear, enumerated message otherwise so a + misspelled or unknown value (e.g. ``"ibmcloud"``) is rejected at the point + of the mistake instead of silently injecting nothing later. + """ + if provider not in SUPPORTED_PROVIDERS: + supported = ", ".join(sorted(SUPPORTED_PROVIDERS)) + raise BadRequestError( + f"Unsupported credential-template provider '{provider}'. " + f"Must be one of: {supported}." + ) + return provider + class _AwsTestError(Exception): """Carrier object mimicking botocore ClientError shape for cloud observation. @@ -215,6 +252,11 @@ def get_template(self, template_id: int) -> dict: def create_template(self, template_data) -> dict: """Create a new credential template.""" + # Reject unknown/misspelled providers before persisting: a template whose + # provider matches no injection path is created "successfully" yet + # contributes no credentials at deploy time (issue #191). + validate_provider(template_data.provider) + # Duplicate name check existing = self.db.query(CloudCredentialTemplate).filter( CloudCredentialTemplate.name == template_data.name @@ -305,6 +347,17 @@ def update_template(self, template_id: int, template_data) -> dict: update_data = template_data.model_dump(exclude_unset=True) + # If the caller is changing the provider, hold it to the same canonical + # set as create so an update can't move a template onto a value that + # injects nothing (issue #191). An explicit ``null`` (or an absent + # field) leaves the stored provider unchanged: drop it here so the + # generic assignment loop below can't write None into the + # ``nullable=False`` column and 500 on flush. + if update_data.get("provider") is not None: + validate_provider(update_data["provider"]) + else: + update_data.pop("provider", None) + # Handle encrypted fields encrypted_map = { "aws_secret_access_key": "aws_secret_access_key_encrypted", diff --git a/backend/tests/component/test_credential_template_service.py b/backend/tests/component/test_credential_template_service.py index a639ae87..f5d59b60 100644 --- a/backend/tests/component/test_credential_template_service.py +++ b/backend/tests/component/test_credential_template_service.py @@ -237,6 +237,93 @@ def test_ibm_resource_group_defaults_to_default(self, db): assert result["ibmcloud_resource_group"] == "default" +# --------------------------------------------------------------------------- +# provider validation (issue #191) +# --------------------------------------------------------------------------- + +class TestProviderValidation: + """A credential template's provider must match a real injection path. + + Regression guard for issue #191: ``provider="ibmcloud"`` (a misspelling of + the canonical ``ibm``) used to be stored happily and then inject NO + credentials at deploy time — silently. Every provider the service accepts + must be one a credential-resolution consumer actually handles. + """ + + @patch("services.credential_template_service.get_default", return_value="us-east-1") + def test_create_rejects_misspelled_ibmcloud_provider(self, mock_get_default, db): + """The exact bug from #191: 'ibmcloud' looks right but injects nothing.""" + svc = CredentialTemplateService(db) + with pytest.raises(BadRequestError, match="Unsupported credential-template provider 'ibmcloud'"): + svc.create_template(_make_template_data(name="ibm-roks", provider="ibmcloud")) + # And nothing was persisted. + assert db.query(CloudCredentialTemplate).filter( + CloudCredentialTemplate.name == "ibm-roks" + ).first() is None + + @patch("services.credential_template_service.get_default", return_value="us-east-1") + def test_create_rejects_unknown_provider_with_enumerated_message(self, mock_get_default, db): + svc = CredentialTemplateService(db) + with pytest.raises(BadRequestError) as exc: + svc.create_template(_make_template_data(name="bogus", provider="digitalocean")) + msg = str(exc.value) + # Message names the offender and enumerates the supported set. + assert "digitalocean" in msg + for supported in ("aws", "azure", "gcp", "ibm", "ssh"): + assert supported in msg + + @patch("services.credential_template_service.get_default", return_value="us-east-1") + def test_create_rejects_empty_provider(self, mock_get_default, db): + svc = CredentialTemplateService(db) + with pytest.raises(BadRequestError): + svc.create_template(_make_template_data(name="empty-prov", provider="")) + + @pytest.mark.parametrize("provider", ["aws", "gcp", "azure", "ibm", "ssh"]) + @patch("services.credential_template_service.get_default", return_value="us-east-1") + def test_create_accepts_every_supported_provider(self, mock_get_default, provider, db): + """Each canonical provider is accepted — validation matches the injection set.""" + svc = CredentialTemplateService(db) + result = svc.create_template(_make_template_data( + name=f"tpl-{provider}", + provider=provider, + # give IBM its required key; other providers don't need extra fields here + ibmcloud_api_key="ibm-api-key-value" if provider == "ibm" else None, + )) + assert result["provider"] == provider + + def test_update_rejects_switch_to_unknown_provider(self, db): + """An update can't move a healthy template onto a no-op provider.""" + t = _create_template_in_db(db, name="aws-live", provider="aws") + svc = CredentialTemplateService(db) + with pytest.raises(BadRequestError, match="Unsupported credential-template provider 'ibmcloud'"): + svc.update_template(t.id, _make_update_data(provider="ibmcloud")) + # Provider unchanged. + db.refresh(t) + assert t.provider == "aws" + + def test_update_without_provider_change_is_allowed(self, db): + """Omitting provider on update leaves it untouched (no false rejection).""" + t = _create_template_in_db(db, name="aws-keep", provider="aws") + svc = CredentialTemplateService(db) + result = svc.update_template(t.id, _make_update_data(description="just a note")) + assert result["provider"] == "aws" + assert result["description"] == "just a note" + + def test_update_with_explicit_null_provider_leaves_it_unchanged(self, db): + """Minor 3: ``PUT {"provider": null}`` must leave the provider unchanged, + not assign None into the ``nullable=False`` column and 500 on flush. + + ``model_dump(exclude_unset=True)`` includes ``{"provider": None}`` for an + explicit null, so the presence-not-non-nullness guard must drop it.""" + t = _create_template_in_db(db, name="aws-null-prov", provider="aws") + svc = CredentialTemplateService(db) + result = svc.update_template(t.id, _make_update_data(provider=None, description="edit")) + assert result["provider"] == "aws" + assert result["description"] == "edit" + db.refresh(t) + assert t.provider == "aws" + + # --------------------------------------------------------------------------- # list_templates / get_template # --------------------------------------------------------------------------- diff --git a/backend/tests/component/test_credentials_service.py b/backend/tests/component/test_credentials_service.py index b06e474e..28a2a818 100644 --- a/backend/tests/component/test_credentials_service.py +++ b/backend/tests/component/test_credentials_service.py @@ -127,6 +127,40 @@ def test_ibm_template(self, mock_dec, db): assert env["IBMCLOUD_API_KEY"] == "ibm-api-key" assert env["IBMCLOUD_REGION"] == "us-south" + @patch("services.credentials_service.decrypt_value", return_value="ibm-api-key") + def test_misspelled_ibmcloud_provider_injects_nothing(self, mock_dec, db): + """Issue #191 reproduction at the injection layer. + + A template stored with provider='ibmcloud' (the natural misspelling of + the canonical 'ibm', matching every adjacent ibmcloud_* field name) + matches NO branch in the resolver, so it silently contributes no IBM + credentials — the deploy then fails far away with a BearerToken error. + The create/update validation (issue #191) now prevents such a row from + being created; this locks the resolver contract that made it dangerous. + """ + template = _make_template( + db, + name="IBM ROKs Testing", + provider="ibmcloud", # unknown to the resolver -> no-op + region="us-east", + aws_auth_method=None, + aws_access_key_id=None, + aws_secret_access_key_encrypted=None, + ibmcloud_api_key_encrypted="enc_ibm_key", + ) + project = _make_project( + db, + credential_template_id=template.id, + project_type="cloud-aws", + cloud_provider="ibm", + ) + + env = get_cloud_credentials_env(project, db=db) + + # The stored API key never reaches the deployment environment. + assert "IC_API_KEY" not in env + assert "IBMCLOUD_API_KEY" not in env + @patch("services.credentials_service.decrypt_value", side_effect=Exception("Bad key")) def test_ibm_template_decrypt_failure_omits_api_key(self, mock_dec, db): template = _make_template( diff --git a/backend/tests/integration/test_routes_credential_templates.py b/backend/tests/integration/test_routes_credential_templates.py index 09dea4d8..75eed377 100644 --- a/backend/tests/integration/test_routes_credential_templates.py +++ b/backend/tests/integration/test_routes_credential_templates.py @@ -121,6 +121,37 @@ def test_create_ibm_template(self, mock_svc_cls, client, operator_headers, all_t assert data["provider"] == "ibm" assert data["has_ibmcloud_api_key"] is True + @patch("routes.credential_templates.CredentialTemplateService") + def test_create_rejects_misspelled_provider_with_422(self, mock_svc_cls, client, operator_headers, all_test_users): + """Issue #191: provider='ibmcloud' is a 422 at the API boundary, not a + silently-empty template — and the service is never reached. + + The provider is a ``Literal`` field, so the 422 is field-scoped: the + error enumerates the supported set (proving the message is real, not + vacuous — ``azure``/``ssh`` are not substrings of the offender) and + must NOT echo the request body, which carries the plaintext secret.""" + mock_svc = MagicMock() + mock_svc_cls.return_value = mock_svc + + secret = "pltxt-ibm-api-key-DO-NOT-LEAK-9f3a7c" + response = client.post( + "/api/credential-templates", + json={"name": "IBM ROKs Testing", "provider": "ibmcloud", + "region": "us-east", "ibmcloud_api_key": secret}, + headers=operator_headers, + ) + assert response.status_code == 422 + body = response.json() + detail = str(body["detail"]) + # Field-scoped: the offending input is echoed, the supported set named. + assert "ibmcloud" in detail + assert "azure" in detail # non-vacuous: not a substring of "ibmcloud" + assert "ssh" in detail + # Body-leak guard (finding 1): the plaintext credential must never + # appear anywhere in the error response — no whole-body echo. + assert secret not in response.text + mock_svc.create_template.assert_not_called() + class TestGetCredentialTemplate: """GET /api/credential-templates/{id}.""" diff --git a/frontend-v2/src/components/settings/CredentialTemplates.tsx b/frontend-v2/src/components/settings/CredentialTemplates.tsx index 52c5c1dc..5e9e42cf 100644 --- a/frontend-v2/src/components/settings/CredentialTemplates.tsx +++ b/frontend-v2/src/components/settings/CredentialTemplates.tsx @@ -59,6 +59,7 @@ import { notify, notifyError } from '@/lib/notify'; import { api } from '@/lib/api'; import { loadIbmRegionsFromApiKey } from '@/lib/ibm-cloud'; import type { CloudCredentialTemplate, CloudCredentialTemplateCreate, CloudRegionOption, IBCosInstanceOption } from '@/types'; +import type { ApiCredentialTemplateCreate } from '@/types/api-schemas'; import { RegionSelector } from '@/components/aws/RegionSelector'; import { CloudRegionSelector } from '@/components/cloud/CloudRegionSelector'; import { SSOAuthDialog } from './SSOAuthDialog'; @@ -66,12 +67,17 @@ import { Tooltip, TooltipContent, TooltipProvider, TooltipTrigger } from '@/comp import { resolveCredStatus } from './resolveCredStatus'; import { useAppMutation } from '@/hooks/lib/useAppMutation'; +// Typed against the generated `provider` union so an invalid entry (or one +// that drifts from the backend Literal) is a compile error. `ssh` is a valid +// provider but intentionally not offered in this create UI; the type still +// permits it, so adding it later is a one-line change with no cast. +type TemplateProvider = ApiCredentialTemplateCreate['provider']; const TEMPLATE_PROVIDER_OPTIONS = [ { value: 'aws', label: 'Amazon Web Services (AWS)' }, { value: 'gcp', label: 'Google Cloud Platform (GCP)' }, { value: 'azure', label: 'Microsoft Azure' }, { value: 'ibm', label: 'IBM Cloud' }, -] as const; +] as const satisfies ReadonlyArray<{ value: TemplateProvider; label: string }>; /** * Single authoritative AWS credential status badge. diff --git a/frontend-v2/src/types/api-generated.ts b/frontend-v2/src/types/api-generated.ts index 44a03e85..80a79840 100644 --- a/frontend-v2/src/types/api-generated.ts +++ b/frontend-v2/src/types/api-generated.ts @@ -15621,8 +15621,11 @@ export interface components { name: string; /** Description */ description?: string | null; - /** Provider */ - provider: string; + /** + * Provider + * @enum {string} + */ + provider: "aws" | "azure" | "gcp" | "ibm" | "ssh"; /** Aws Auth Method */ aws_auth_method?: string | null; /** Aws Profile */ @@ -15819,7 +15822,7 @@ export interface components { /** Description */ description?: string | null; /** Provider */ - provider?: string | null; + provider?: ("aws" | "azure" | "gcp" | "ibm" | "ssh") | null; /** Aws Auth Method */ aws_auth_method?: string | null; /** Aws Profile */