Repository: external-secrets/external-secrets (go)

Issue:
[Infisical] Sync doesn't fail if projectSlug is not available
**Is your feature request related to a problem? Please describe.**
I've created a SecretStore with a self-hosted Infisical server as provider. I first entered a wrong projectSlug. Both the SecretStore as well as the ExternalSecret looked fine, they both said that the secret was successfully synced.
Looking at the secret however shows that the Keys have an empty value.
See also https://github.com/external-secrets/external-secrets/issues/3686.

**Describe the solution you'd like**
If the External-Secrets-Operator is not able to retrieve a secret (project doesn't exist or missing permissions or similar), the Secret-Store should fail to validate.

**Describe alternatives you've considered**
If the SecretStore validation cannot check this, at least the ExternalSecret should fail to sync.

**Additional context**
I've tested the wrong request using an api client against the Infisical-Server and it returned a 500:
```
{"statusCode": 500,"message": "Something went wrong","error": "Find project by slug"}
```
So from Infisical's side the behaviour seems correct.

Manifest misconfigured SecretStore (retrieved from Kubernetes, but slightly cleaned up):
```
apiVersion: external-secrets.io/v1beta1
kind: SecretStore
metadata:
  name: infisical-foo
  namespace: external-secrets
spec:
  provider:
    infisical:
      auth:
        universalAuthCredentials:
          clientId:
            key: clientId
            name: infisical-creds
            namespace: external-secrets
          clientSecret:
            key: clientSecret
            name: infisical-creds
            namespace: external-secrets
      hostAPI: https://infisical.change.me # changed for data protection
      secretsScope:
        environmentSlug: dev
        projectSlug: foobar # project doesn't exist
        secretsPath: /
status:
  capabilities: ReadOnly
  conditions:
  - lastTransitionTime: "2024-10-29T14:30:24Z"
    message: store validated
    reason: Valid
    status: "True"
    type: Ready
```
Manifest ExternalSecret (retrieved from Kubernetes, but slightly cleaned up):
```
apiVersion: external-secrets.io/v1beta1
kind: ExternalSecret
metadata:
  name: es-foo
  namespace: external-secrets
spec:
  data:
  - remoteRef:
      conversionStrategy: Default
      decodingStrategy: None
      key: PASSWORD
      metadataPolicy: None
    secretKey: ADMIN_PASS
  refreshInterval: 1h
  secretStoreRef:
    kind: SecretStore
    name: infisical-foo
  target:
    creationPolicy: Owner
    deletionPolicy: Retain
    name: secret-foo
status:
  binding:
    name: secret-foo
  conditions:
  - lastTransitionTime: "2024-10-29T14:33:58Z"
    message: Secret was synced
    reason: SecretSynced
    status: "True"
    type: Ready
  refreshTime: "2024-10-29T14:33:58Z"
  syncedResourceVersion: 1-4fe8a58fdd8d1c1dbe5e34de2c39af9a
```
Manifest Secret (retrieved from Kubernetes, but slightly cleaned up):
```
apiVersion: v1
data:
  ADMIN_PASS: ""
immutable: false
kind: Secret
metadata:
  annotations:
    reconcile.external-secrets.io/data-hash: 2f268487e7f9303c1fe8888de1ee50af
  labels:
    reconcile.external-secrets.io/created-by: 512949fc898ec69665a6157fa1ac81e1
  name: secret-foo
  namespace: external-secrets
type: Opaque
```

Pull request description:
infisical: fix error handling which previously failed silently (missing secrets, incorrect auth, etc.)
(cc @akhilmhdh)

## Problem Statement

Previously, all of the HTTP requests in the Infisical provider did not handle HTTP errors. This resulted in plenty of mysterious problems where the provider seemed to be working, but would do things like return empty secrets due to empty values.

## Related Issue

Fixes #4059 

## Proposed Changes

To address the problem, I made a number of changes:
- Added handling for any non-200 response on all 4 of the API calls which was missing from 3 of them (and, only a 404 check on the remaining 1)
- Added a bit more invariants/error handling for a few scenarios like incorrect unmarshaled data (e.g. auth token retrieval and invocation)
- Coalesced the `api.go` methods to be simpler, and push most of the complexity into a common `do(...)` for making API requests
- Added a handful of unit testing against `api.go` 

I did some manual e2e testing with my Infisical instance:
- [x] Used an invalid client ID / client secret
- :x: `SecretStore` had error: `failed to authenticate via universal auth API error (404): error="No identity with specified client ID was found" message="NotFound`
- [x] Used a machine identity without access to the project
- :x: `SecretStore` had error: `cannot read secrets with provided project scope project:homelab-p-zl-t environment:prod secret-path:/ recursive:true, API error (403): error="You are not allowed to access th
│ is resource" message="Identity is not a member of the specified project with ID '46b1cf70-bd3c-4677-8b04-1cfcb803edcf'"`
- [x] Added a role to the machine identity without access to secrets
- :x: `SecretStore` had error: `cannot read secrets with provided project scope project:homelab-p-zl-t environment:prod secret-path:/ recursive:true, API error (403): error="You are not allowed to read on s
│ ecrets" message="PermissionDenied", details=[]`
- [x] Added permission to access secrets, but not the secret being requested
- :x: `ExternalSecret` had error: `(key: SECRET2), err: API error (403): error="You are not allowed to read on secrets" message="PermissionDenied", details=[{"action":["read"],"conditions":{"secretName":{"$eq":"SECRET1"}},"inverted":false,"subject":["secrets"]}]`
  - Note, the contents of `details` are actuall a list of the policies for the API key's role (not the request). It may be helpful to use a different name on the error logs (e.g. `role_policies`) to clarify that, but honestly I don't see where Infisical has documentation about this. It's not included in their golang SDK.
- [x] Added permission to access the secret being used
- ✅ `ExternalSecret` synced

## Checklist

- [x] I have read the [contribution guidelines](https://external-secrets.io/latest/contributing/process/#submitting-a-pull-request)
- [x] All commits are signed with `git commit --signoff`
- [x] My changes have reasonable test coverage
- [x] All tests pass with `make test`
- [ ] I ensured my PR is ready for review with `make reviewable`


Patch:
diff --git a/pkg/provider/infisical/api/api.go b/pkg/provider/infisical/api/api.go
index 706fd9a19..f1586f451 100644
--- a/pkg/provider/infisical/api/api.go
+++ b/pkg/provider/infisical/api/api.go
@@ -19,10 +19,10 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"io"
 	"net/http"
 	"net/url"
 	"strconv"
-	"time"
 
 	esv1beta1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1beta1"
 	"github.com/external-secrets/external-secrets/pkg/metrics"
@@ -42,10 +42,71 @@ type InfisicalApis interface {
 	RevokeAccessToken() error
 }
 
+const (
+	machineIdentityLoginViaUniversalAuth = "MachineIdentityLoginViaUniversalAuth"
+	getSecretsV3                         = "GetSecretsV3"
+	getSecretByKeyV3                     = "GetSecretByKeyV3"
+	revokeAccessToken                    = "RevokeAccessToken"
+)
+
 const UserAgentName = "k8-external-secrets-operator"
-const errJSONSecretUnmarshal = "unable to unmarshal secret: %w"
 
-func NewAPIClient(baseURL string) (*InfisicalClient, error) {
+var errJSONUnmarshal = errors.New("unable to unmarshal API response")
+var errNoAccessToken = errors.New("unexpected error: no access token available to revoke")
+var errAccessTokenAlreadyRetrieved = errors.New("unexpected error: access token was already retrieved")
+
+type InfisicalAPIError struct {
+	StatusCode int
+	Err        any
+	Message    any
+	Details    any
+}
+
+func (e *InfisicalAPIError) Error() string {
+	if e.Details != nil {
+		detailsJSON, _ := json.Marshal(e.Details)
+		return fmt.Sprintf("API error (%d): error=%v message=%v, details=%s", e.StatusCode, e.Err, e.Message, string(detailsJSON))
+	} else {
+		return fmt.Sprintf("API error (%d): error=%v message=%v", e.StatusCode, e.Err, e.Message)
+	}
+}
+
+// checkError checks for an error on the http response and generates an appropriate error if one is
+// found.
+func checkError(resp *http.Response) error {
+	if resp.StatusCode >= 200 && resp.StatusCode < 400 {
+		return nil
+	}
+
+	var buf bytes.Buffer
+	_, err := buf.ReadFrom(resp.Body)
+	if err != nil {
+		return fmt.Errorf("API error (%d) and failed to read response body: %w", resp.StatusCode, err)
+	}
+
+	// Attempt to unmarshal the response body into an InfisicalAPIErrorResponse.
+	var errRes InfisicalAPIErrorResponse
+	err = json.Unmarshal(buf.Bytes(), &errRes)
+	// Non-200 errors that cannot be unmarshaled must be handled, as errors could come from outside of
+	// Infisical.
+	if err != nil {
+		return fmt.Errorf("API error (%d), could not unmarshal error response: %w", resp.StatusCode, err)
+	} else if errRes.StatusCode == 0 {
+		// When the InfisicalResponse has a zero-value status code, then the
+		// response was either malformed or not from Infisical. Instead, just return
+		// the error string from the response.
+		return fmt.Errorf("API error (%d): %s", resp.StatusCode, buf.String())
+	} else {
+		return &InfisicalAPIError{
+			StatusCode: resp.StatusCode,
+			Message:    errRes.Message,
+			Err:        errRes.Error,
+			Details:    errRes.Details,
+		}
+	}
+}
+
+func NewAPIClient(baseURL string, client *http.Client) (*InfisicalClient, error) {
 	baseParsedURL, err := url.Parse(baseURL)
 	if err != nil {
 		return nil, err
@@ -53,9 +114,7 @@ func NewAPIClient(baseURL string) (*InfisicalClient, error) {
 
 	api := &InfisicalClient{
 		BaseURL: baseParsedURL,
-		client: &http.Client{
-			Timeout: time.Second * 15,
-		},
+		client:  client,
 	}
 
 	return api, nil
@@ -63,13 +122,22 @@ func NewAPIClient(baseURL string) (*InfisicalClient, error) {
 
 func (a *InfisicalClient) SetTokenViaMachineIdentity(clientID, clientSecret string) error {
 	if a.token != "" {
-		return nil
+		return errAccessTokenAlreadyRetrieved
 	}
 
-	loginResponse, err := a.MachineIdentityLoginViaUniversalAuth(MachineIdentityUniversalAuthLoginRequest{
-		ClientID:     clientID,
-		ClientSecret: clientSecret,
-	})
+	var loginResponse MachineIdentityDetailsResponse
+	err := a.do(
+		"api/v1/auth/universal-auth/login",
+		http.MethodPost,
+		map[string]string{},
+		MachineIdentityUniversalAuthLoginRequest{
+			ClientID:     clientID,
+			ClientSecret: clientSecret,
+		},
+		&loginResponse,
+	)
+	metrics.ObserveAPICall(constants.ProviderName, machineIdentityLoginViaUniversalAuth, err)
+
 	if err != nil {
 		return err
 	}
@@ -80,9 +148,20 @@ func (a *InfisicalClient) SetTokenViaMachineIdentity(clientID, clientSecret stri
 
 func (a *InfisicalClient) RevokeAccessToken() error {
 	if a.token == "" {
-		return nil
+		return errNoAccessToken
 	}
-	if _, err := a.RevokeMachineIdentityAccessToken(RevokeMachineIdentityAccessTokenRequest{AccessToken: a.token}); err != nil {
+
+	var revokeResponse RevokeMachineIdentityAccessTokenResponse
+	err := a.do(
+		"api/v1/auth/token/revoke",
+		http.MethodPost,
+		map[string]string{},
+		RevokeMachineIdentityAccessTokenRequest{AccessToken: a.token},
+		&revokeResponse,
+	)
+	metrics.ObserveAPICall(constants.ProviderName, revokeAccessToken, err)
+
+	if err != nil {
 		return err
 	}
 
@@ -94,97 +173,85 @@ func (a *InfisicalClient) resolveEndpoint(path string) string {
 	return a.BaseURL.ResolveReference(&url.URL{Path: path}).String()
 }
 
-func (a *InfisicalClient) do(r *http.Request) (*http.Response, error) {
+func (a *InfisicalClient) addHeaders(r *http.Request) {
 	if a.token != "" {
 		r.Header.Add("Authorization", "Bearer "+a.token)
 	}
 	r.Header.Add("User-Agent", UserAgentName)
 	r.Header.Add("Content-Type", "application/json")
-
-	return a.client.Do(r)
 }
 
-func (a *InfisicalClient) MachineIdentityLoginViaUniversalAuth(data MachineIdentityUniversalAuthLoginRequest) (*MachineIdentityDetailsResponse, error) {
-	endpointURL := a.resolveEndpoint("api/v1/auth/universal-auth/login")
-	body, err := MarshalReqBody(data)
-	if err != nil {
-		return nil, err
-	}
+// do is a generic function that makes an API call to the Infisical API, and handle the response
+// (including if an API error is returned).
+func (a *InfisicalClient) do(endpoint, method string, params map[string]string, body, response any) error {
+	endpointURL := a.resolveEndpoint(endpoint)
 
-	req, err := http.NewRequest(http.MethodPost, endpointURL, body)
-	metrics.ObserveAPICall(constants.ProviderName, "MachineIdentityLoginViaUniversalAuth", err)
+	bodyReader, err := MarshalReqBody(body)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
-	rawRes, err := a.do(req)
+	r, err := http.NewRequest(method, endpointURL, bodyReader)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
-	var res MachineIdentityDetailsResponse
-	err = ReadAndUnmarshal(rawRes, &res)
-	if err != nil {
-		return nil, fmt.Errorf(errJSONSecretUnmarshal, err)
+	a.addHeaders(r)
+
+	q := r.URL.Query()
+	for key, value := range params {
+		q.Add(key, value)
 	}
-	return &res, nil
-}
+	r.URL.RawQuery = q.Encode()
 
-func (a *InfisicalClient) RevokeMachineIdentityAccessToken(data RevokeMachineIdentityAccessTokenRequest) (*RevokeMachineIdentityAccessTokenResponse, error) {
-	endpointURL := a.resolveEndpoint("api/v1/auth/token/revoke")
-	body, err := MarshalReqBody(data)
+	resp, err := a.client.Do(r)
 	if err != nil {
-		return nil, err
+		return err
 	}
+	defer resp.Body.Close()
 
-	req, err := http.NewRequest(http.MethodPost, endpointURL, body)
-	metrics.ObserveAPICall(constants.ProviderName, "RevokeMachineIdentityAccessToken", err)
-	if err != nil {
-		return nil, err
+	if err := checkError(resp); err != nil {
+		return err
 	}
 
-	rawRes, err := a.do(req)
+	bodyBytes, err := io.ReadAll(resp.Body)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
-	var res RevokeMachineIdentityAccessTokenResponse
-	err = ReadAndUnmarshal(rawRes, &res)
+	err = json.Unmarshal(bodyBytes, response)
 	if err != nil {
-		return nil, fmt.Errorf(errJSONSecretUnmarshal, err)
+		// Importantly, we do not include the response in the actual error to avoid
+		// leaking anything sensitive.
+		return errJSONUnmarshal
 	}
-	return &res, nil
+
+	return nil
 }
 
 func (a *InfisicalClient) GetSecretsV3(data GetSecretsV3Request) (map[string]string, error) {
-	endpointURL := a.resolveEndpoint("api/v3/secrets/raw")
-
-	req, err := http.NewRequest(http.MethodGet, endpointURL, http.NoBody)
-	metrics.ObserveAPICall(constants.ProviderName, "GetSecretsV3", err)
-	if err != nil {
-		return nil, err
+	params := map[string]string{
+		"workspaceSlug":          data.ProjectSlug,
+		"environment":            data.EnvironmentSlug,
+		"secretPath":             data.SecretPath,
+		"include_imports":        "true",
+		"expandSecretReferences": "true",
+		"recursive":              strconv.FormatBool(data.Recursive),
 	}
 
-	q := req.URL.Query()
-	q.Add("workspaceSlug", data.ProjectSlug)
-	q.Add("environment", data.EnvironmentSlug)
-	q.Add("secretPath", data.SecretPath)
-	q.Add("include_imports", "true")
-	q.Add("expandSecretReferences", "true")
-	q.Add("recursive", strconv.FormatBool(data.Recursive))
-	req.URL.RawQuery = q.Encode()
-
-	rawRes, err := a.do(req)
+	res := GetSecretsV3Response{}
+	err := a.do(
+		"api/v3/secrets/raw",
+		http.MethodGet,
+		params,
+		http.NoBody,
+		&res,
+	)
+	metrics.ObserveAPICall(constants.ProviderName, getSecretsV3, err)
 	if err != nil {
 		return nil, err
 	}
 
-	var res GetSecretsV3Response
-	err = ReadAndUnmarshal(rawRes, &res)
-	if err != nil {
-		return nil, fmt.Errorf(errJSONSecretUnmarshal, err)
-	}
-
 	secrets := make(map[string]string)
 	for _, s := range res.ImportedSecrets {
 		for _, el := range s.Secrets {
@@ -199,42 +266,30 @@ func (a *InfisicalClient) GetSecretsV3(data GetSecretsV3Request) (map[string]str
 }
 
 func (a *InfisicalClient) GetSecretByKeyV3(data GetSecretByKeyV3Request) (string, error) {
-	endpointURL := a.resolveEndpoint(fmt.Sprintf("api/v3/secrets/raw/%s", data.SecretKey))
-
-	req, err := http.NewRequest(http.MethodGet, endpointURL, http.NoBody)
-	metrics.ObserveAPICall(constants.ProviderName, "GetSecretByKeyV3", err)
-	if err != nil {
-		return "", err
+	params := map[string]string{
+		"workspaceSlug":   data.ProjectSlug,
+		"environment":     data.EnvironmentSlug,
+		"secretPath":      data.SecretPath,
+		"include_imports": "true",
 	}
 
-	q := req.URL.Query()
-	q.Add("workspaceSlug", data.ProjectSlug)
-	q.Add("environment", data.EnvironmentSlug)
-	q.Add("secretPath", data.SecretPath)
-	q.Add("include_imports", "true")
-	req.URL.RawQuery = q.Encode()
-
-	rawRes, err := a.do(req)
+	endpointURL := fmt.Sprintf("api/v3/secrets/raw/%s", data.SecretKey)
+
+	res := GetSecretByKeyV3Response{}
+	err := a.do(
+		endpointURL,
+		http.MethodGet,
+		params,
+		http.NoBody,
+		&res,
+	)
+	metrics.ObserveAPICall(constants.ProviderName, getSecretByKeyV3, err)
 	if err != nil {
-		return "", err
-	}
-	if rawRes.StatusCode == 400 {
-		var errRes InfisicalAPIErrorResponse
-		err = ReadAndUnmarshal(rawRes, &errRes)
-		if err != nil {
-			return "", fmt.Errorf(errJSONSecretUnmarshal, err)
-		}
-
-		if errRes.Message == "Secret not found" {
+		var apiErr *InfisicalAPIError
+		if errors.As(err, &apiErr) && apiErr.StatusCode == 404 {
 			return "", esv1beta1.NoSecretError{}
 		}
-		return "", errors.New(errRes.Message)
-	}
-
-	var res GetSecretByKeyV3Response
-	err = ReadAndUnmarshal(rawRes, &res)
-	if err != nil {
-		return "", fmt.Errorf(errJSONSecretUnmarshal, err)
+		return "", err
 	}
 
 	return res.Secret.SecretValue, nil
@@ -247,13 +302,3 @@ func MarshalReqBody(data any) (*bytes.Reader, error) {
 	}
 	return bytes.NewReader(body), nil
 }
-
-func ReadAndUnmarshal(resp *http.Response, target any) error {
-	var buf bytes.Buffer
-	defer resp.Body.Close()
-	_, err := buf.ReadFrom(resp.Body)
-	if err != nil {
-		return err
-	}
-	return json.Unmarshal(buf.Bytes(), target)
-}
diff --git a/pkg/provider/infisical/api/api_fake.go b/pkg/provider/infisical/api/api_fake.go
new file mode 100644
index 000000000..0eefc4055
--- /dev/null
+++ b/pkg/provider/infisical/api/api_fake.go
@@ -0,0 +1,47 @@
+/*
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package api
+
+import (
+	"encoding/json"
+	"net/http"
+	"net/http/httptest"
+)
+
+func newMockServer(status int, data any) *httptest.Server {
+	body, err := json.Marshal(data)
+	if err != nil {
+		panic(err)
+	}
+
+	return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(status)
+		_, err := w.Write(body)
+		if err != nil {
+			panic(err)
+		}
+	}))
+}
+
+// NewMockClient creates an InfisicalClient with a mocked HTTP client that has a
+// fixed response.
+func NewMockClient(status int, data any) (*InfisicalClient, func()) {
+	server := newMockServer(status, data)
+	client, err := NewAPIClient(server.URL, server.Client())
+	if err != nil {
+		panic(err)
+	}
+	return client, server.Close
+}
diff --git a/pkg/provider/infisical/api/api_models.go b/pkg/provider/infisical/api/api_models.go
index b1f2be2c0..55cbe26f8 100644
--- a/pkg/provider/infisical/api/api_models.go
+++ b/pkg/provider/infisical/api/api_models.go
@@ -84,5 +84,7 @@ type ImportedSecretV3 struct {
 type InfisicalAPIErrorResponse struct {
 	StatusCode int    `json:"statusCode"`
 	Message    string `json:"message"`
-	Error      any    `json:"error"`
+	Error      string `json:"error"`
+	// According to Infisical's API docs, `details` are only returned for 403 errors.
+	Details any `json:"details,omitempty"`
 }
diff --git a/pkg/provider/infisical/fake/fake.go b/pkg/provider/infisical/fake/fake.go
deleted file mode 100644
index 888f8c74b..000000000
--- a/pkg/provider/infisical/fake/fake.go
+++ /dev/null
@@ -1,58 +0,0 @@
-/*
-Licensed under the Apache License, Version 2.0 (the "License");
-you may not use this file except in compliance with the License.
-You may obtain a copy of the License at
-
-	http://www.apache.org/licenses/LICENSE-2.0
-
-Unless required by applicable law or agreed to in writing, software
-distributed under the License is distributed on an "AS IS" BASIS,
-WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or impliec.
-See the License for the specific language governing permissions and
-limitations under the License.
-*/
-package fake
-
-import (
-	"errors"
-	"time"
-
-	"github.com/external-secrets/external-secrets/pkg/provider/infisical/api"
-)
-
-var (
-	ErrMissingMockImplementation = errors.New("missing mock implmentation")
-)
-
-type MockInfisicalClient struct {
-	MockedGetSecretV3      func(data api.GetSecretsV3Request) (map[string]string, error)
-	MockedGetSecretByKeyV3 func(data api.GetSecretByKeyV3Request) (string, error)
-}
-
-func (a *MockInfisicalClient) MachineIdentityLoginViaUniversalAuth(data api.MachineIdentityUniversalAuthLoginRequest) (*api.MachineIdentityDetailsResponse, error) {
-	return &api.MachineIdentityDetailsResponse{
-		AccessToken:       "test-access-token",
-		ExpiresIn:         int(time.Hour * 24),
-		TokenType:         "bearer",
-		AccessTokenMaxTTL: int(time.Hour * 24 * 2),
-	}, nil
-}
-
-func (a *MockInfisicalClient) GetSecretsV3(data api.GetSecretsV3Request) (map[string]string, error) {
-	if a.MockedGetSecretV3 == nil {
-		return nil, ErrMissingMockImplementation
-	}
-
-	return a.MockedGetSecretV3(data)
-}
-
-func (a *MockInfisicalClient) GetSecretByKeyV3(data api.GetSecretByKeyV3Request) (string, error) {
-	if a.MockedGetSecretByKeyV3 == nil {
-		return "", ErrMissingMockImplementation
-	}
-	return a.MockedGetSecretByKeyV3(data)
-}
-
-func (a *MockInfisicalClient) RevokeAccessToken() error {
-	return nil
-}
diff --git a/pkg/provider/infisical/provider.go b/pkg/provider/infisical/provider.go
index 28490284f..fbb9b7985 100644
--- a/pkg/provider/infisical/provider.go
+++ b/pkg/provider/infisical/provider.go
@@ -18,6 +18,8 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"net/http"
+	"time"
 
 	ctrl "sigs.k8s.io/controller-runtime"
 	kclient "sigs.k8s.io/controller-runtime/pkg/client"
@@ -36,7 +38,7 @@ var (
 )
 
 type Provider struct {
-	apiClient api.InfisicalApis
+	apiClient *api.InfisicalClient
 	apiScope  *InfisicalClientScope
 }
 
@@ -70,7 +72,9 @@ func (p *Provider) NewClient(ctx context.Context, store esv1beta1.GenericStore,
 
 	infisicalSpec := storeSpec.Provider.Infisical
 
-	apiClient, err := api.NewAPIClient(infisicalSpec.HostAPI)
+	apiClient, err := api.NewAPIClient(infisicalSpec.HostAPI, &http.Client{
+		Timeout: time.Second * 15,
+	})
 	if err != nil {
 		return nil, err
 	}


Test patch:
diff --git a/pkg/provider/infisical/api/api_test.go b/pkg/provider/infisical/api/api_test.go
new file mode 100644
index 000000000..8547b549d
--- /dev/null
+++ b/pkg/provider/infisical/api/api_test.go
@@ -0,0 +1,317 @@
+/*
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package api
+
+import (
+	"errors"
+	"reflect"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+
+	esv1beta1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1beta1"
+)
+
+const (
+	fakeClientID        = "client-id"
+	fakeClientSecret    = "client-secret"
+	fakeToken           = "token"
+	fakeProjectSlug     = "first-project"
+	fakeEnvironmentSlug = "dev"
+)
+
+func TestAPIClientDo(t *testing.T) {
+	apiURL := "foo"
+	httpMethod := "bar"
+
+	testCases := []struct {
+		Name             string
+		MockStatusCode   int
+		MockResponse     any
+		ExpectedResponse any
+		ExpectedError    error
+	}{
+		{
+			Name:           "Success",
+			MockStatusCode: 200,
+			MockResponse: MachineIdentityDetailsResponse{
+				AccessToken: "foobar",
+			},
+			ExpectedResponse: MachineIdentityDetailsResponse{
+				AccessToken: "foobar",
+			},
+			ExpectedError: nil,
+		},
+		{
+			Name:           "Error when response cannot be unmarshalled",
+			MockStatusCode: 500,
+			MockResponse:   []byte("not-json"),
+			ExpectedError:  errors.New("API error (500), could not unmarshal error response: json: cannot unmarshal string into Go value of type api.InfisicalAPIErrorResponse"),
+		},
+		{
+			Name:           "Error when non-Infisical error response received",
+			MockStatusCode: 500,
+			MockResponse:   map[string]string{"foo": "bar"},
+			ExpectedError:  errors.New("API error (500): {\"foo\":\"bar\"}"),
+		},
+		{
+			Name:           "Do: Error when non-200 response received",
+			MockStatusCode: 401,
+			MockResponse: InfisicalAPIErrorResponse{
+				StatusCode: 401,
+				Error:      "Unauthorized",
+			},
+			ExpectedError: &InfisicalAPIError{StatusCode: 401, Err: "Unauthorized", Message: ""},
+		},
+		{
+			Name:           "Error when arbitrary details are returned",
+			MockStatusCode: 401,
+			MockResponse: InfisicalAPIErrorResponse{
+				StatusCode: 401,
+				Error:      "Unauthorized",
+				Details:    map[string]string{"foo": "details"},
+			},
+			ExpectedError: &InfisicalAPIError{StatusCode: 401, Err: "Unauthorized", Message: "", Details: map[string]string{"foo": "details"}},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.Name, func(t *testing.T) {
+			apiClient, closeFunc := NewMockClient(tc.MockStatusCode, tc.MockResponse)
+			defer closeFunc()
+
+			// Automatically pluck out the expected response type using reflection to create a new empty value for unmarshalling.
+			var actualResponse any
+			if tc.ExpectedResponse != nil {
+				actualResponse = reflect.New(reflect.TypeOf(tc.ExpectedResponse)).Interface()
+			}
+
+			err := apiClient.do(apiURL, httpMethod, nil, nil, actualResponse)
+			if tc.ExpectedError != nil {
+				assert.Error(t, err)
+				assert.Equal(t, tc.ExpectedError.Error(), err.Error())
+			} else {
+				assert.NoError(t, err)
+				assert.Equal(t, tc.ExpectedResponse, reflect.ValueOf(actualResponse).Elem().Interface())
+			}
+		})
+	}
+}
+
+// TestAPIClientDoInvalidResponse tests the case where the response is a 200 but does not unmarshal
+// correctly.
+func TestAPIClientDoInvalidResponse(t *testing.T) {
+	apiClient, closeFunc := NewMockClient(200, []byte("not-json"))
+	defer closeFunc()
+
+	err := apiClient.do("foo", "bar", nil, nil, nil)
+	assert.ErrorIs(t, err, errJSONUnmarshal)
+}
+
+func TestSetTokenViaMachineIdentity(t *testing.T) {
+	t.Run("Success", func(t *testing.T) {
+		apiClient, closeFunc := NewMockClient(200, MachineIdentityDetailsResponse{
+			AccessToken: "foobar",
+		})
+		defer closeFunc()
+
+		err := apiClient.SetTokenViaMachineIdentity(fakeClientID, fakeClientSecret)
+		assert.NoError(t, err)
+		assert.Equal(t, apiClient.token, "foobar")
+	})
+
+	t.Run("SetTokenViaMachineIdentity: Error when non-200 response received", func(t *testing.T) {
+		apiClient, closeFunc := NewMockClient(401, InfisicalAPIErrorResponse{
+			StatusCode: 401,
+			Error:      "Unauthorized",
+		})
+		defer closeFunc()
+
+		err := apiClient.SetTokenViaMachineIdentity(fakeClientID, fakeClientSecret)
+		assert.Error(t, err)
+		var apiErr *InfisicalAPIError
+		assert.True(t, errors.As(err, &apiErr))
+		assert.Equal(t, 401, apiErr.StatusCode)
+		assert.Equal(t, "Unauthorized", apiErr.Err)
+	})
+
+	t.Run("Error when token already set", func(t *testing.T) {
+		apiClient, closeFunc := NewMockClient(401, nil)
+		defer closeFunc()
+
+		apiClient.token = fakeToken
+
+		err := apiClient.SetTokenViaMachineIdentity(fakeClientID, fakeClientSecret)
+		assert.ErrorIs(t, err, errAccessTokenAlreadyRetrieved)
+	})
+}
+
+func TestRevokeAccessToken(t *testing.T) {
+	t.Run("Success", func(t *testing.T) {
+		apiClient, closeFunc := NewMockClient(200, RevokeMachineIdentityAccessTokenResponse{
+			Message: "Success",
+		})
+		defer closeFunc()
+
+		apiClient.token = fakeToken
+
+		err := apiClient.RevokeAccessToken()
+		assert.NoError(t, err)
+		// Verify that the access token was unset.
+		assert.Equal(t, apiClient.token, "")
+	})
+
+	t.Run("RevokeAccessToken: Error when non-200 response received", func(t *testing.T) {
+		apiClient, closeFunc := NewMockClient(401, InfisicalAPIErrorResponse{
+			StatusCode: 401,
+			Error:      "Unauthorized",
+		})
+		defer closeFunc()
+
+		apiClient.token = fakeToken
+
+		err := apiClient.RevokeAccessToken()
+		assert.Error(t, err)
+		var apiErr *InfisicalAPIError
+		assert.True(t, errors.As(err, &apiErr))
+		assert.Equal(t, 401, apiErr.StatusCode)
+		assert.Equal(t, "Unauthorized", apiErr.Err)
+	})
+
+	t.Run("Error when no access token is set", func(t *testing.T) {
+		apiClient, closeFunc := NewMockClient(401, nil)
+		defer closeFunc()
+
+		err := apiClient.RevokeAccessToken()
+		assert.ErrorIs(t, err, errNoAccessToken)
+	})
+}
+
+func TestGetSecretsV3(t *testing.T) {
+	t.Run("Works with secrets", func(t *testing.T) {
+		apiClient, closeFunc := NewMockClient(200, GetSecretsV3Response{
+			Secrets: []SecretsV3{
+				{SecretKey: "foo", SecretValue: "bar"},
+			},
+		})
+		defer closeFunc()
+
+		secrets, err := apiClient.GetSecretsV3(GetSecretsV3Request{
+			ProjectSlug:     fakeProjectSlug,
+			EnvironmentSlug: fakeEnvironmentSlug,
+			SecretPath:      "/",
+			Recursive:       true,
+		})
+		assert.NoError(t, err)
+		assert.Equal(t, secrets, map[string]string{"foo": "bar"})
+	})
+
+	t.Run("Works with imported secrets", func(t *testing.T) {
+		apiClient, closeFunc := NewMockClient(200, GetSecretsV3Response{
+			ImportedSecrets: []ImportedSecretV3{{
+				Secrets: []SecretsV3{{SecretKey: "foo", SecretValue: "bar"}},
+			}},
+		})
+		defer closeFunc()
+
+		secrets, err := apiClient.GetSecretsV3(GetSecretsV3Request{
+			ProjectSlug:     fakeProjectSlug,
+			EnvironmentSlug: fakeEnvironmentSlug,
+			SecretPath:      "/",
+			Recursive:       true,
+		})
+		assert.NoError(t, err)
+		assert.Equal(t, secrets, map[string]string{"foo": "bar"})
+	})
+
+	t.Run("GetSecretsV3: Error when non-200 response received", func(t *testing.T) {
+		apiClient, closeFunc := NewMockClient(401, InfisicalAPIErrorResponse{
+			StatusCode: 401,
+			Error:      "Unauthorized",
+		})
+		defer closeFunc()
+
+		_, err := apiClient.GetSecretsV3(GetSecretsV3Request{
+			ProjectSlug:     fakeProjectSlug,
+			EnvironmentSlug: fakeEnvironmentSlug,
+			SecretPath:      "/",
+			Recursive:       true,
+		})
+		assert.Error(t, err)
+		var apiErr *InfisicalAPIError
+		assert.True(t, errors.As(err, &apiErr))
+		assert.Equal(t, 401, apiErr.StatusCode)
+		assert.Equal(t, "Unauthorized", apiErr.Err)
+	})
+}
+func TestGetSecretByKeyV3(t *testing.T) {
+	t.Run("Works", func(t *testing.T) {
+		apiClient, closeFunc := NewMockClient(200, GetSecretByKeyV3Response{
+			Secret: SecretsV3{
+				SecretKey:   "foo",
+				SecretValue: "bar",
+			},
+		})
+		defer closeFunc()
+
+		secret, err := apiClient.GetSecretByKeyV3(GetSecretByKeyV3Request{
+			ProjectSlug:     fakeProjectSlug,
+			EnvironmentSlug: fakeEnvironmentSlug,
+			SecretPath:      "/",
+			SecretKey:       "foo",
+		})
+		assert.NoError(t, err)
+		assert.Equal(t, "bar", secret)
+	})
+
+	t.Run("Error when secret is not found", func(t *testing.T) {
+		apiClient, closeFunc := NewMockClient(404, InfisicalAPIErrorResponse{
+			StatusCode: 404,
+			Error:      "Not Found",
+		})
+		defer closeFunc()
+
+		_, err := apiClient.GetSecretByKeyV3(GetSecretByKeyV3Request{
+			ProjectSlug:     fakeProjectSlug,
+			EnvironmentSlug: fakeEnvironmentSlug,
+			SecretPath:      "/",
+			SecretKey:       "foo",
+		})
+		assert.Error(t, err)
+		// Importantly, we return the standard error for no secrets found.
+		assert.ErrorIs(t, err, esv1beta1.NoSecretError{})
+	})
+
+	// Test case where the request is unauthorized
+	t.Run("ErrorHandlingUnauthorized", func(t *testing.T) {
+		apiClient, closeFunc := NewMockClient(401, InfisicalAPIErrorResponse{
+			StatusCode: 401,
+			Error:      "Unauthorized",
+		})
+		defer closeFunc()
+
+		_, err := apiClient.GetSecretByKeyV3(GetSecretByKeyV3Request{
+			ProjectSlug:     fakeProjectSlug,
+			EnvironmentSlug: fakeEnvironmentSlug,
+			SecretPath:      "/",
+			SecretKey:       "foo",
+		})
+		assert.Error(t, err)
+		var apiErr *InfisicalAPIError
+		assert.True(t, errors.As(err, &apiErr))
+		assert.Equal(t, 401, apiErr.StatusCode)
+		assert.Equal(t, "Unauthorized", apiErr.Err)
+	})
+}
diff --git a/pkg/provider/infisical/provider_test.go b/pkg/provider/infisical/provider_test.go
index 24fb34502..6f68206c6 100644
--- a/pkg/provider/infisical/provider_test.go
+++ b/pkg/provider/infisical/provider_test.go
@@ -25,7 +25,6 @@ import (
 	esv1beta1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1beta1"
 	esv1meta "github.com/external-secrets/external-secrets/apis/meta/v1"
 	"github.com/external-secrets/external-secrets/pkg/provider/infisical/api"
-	"github.com/external-secrets/external-secrets/pkg/provider/infisical/fake"
 )
 
 type storeModifier func(*esv1beta1.SecretStore) *esv1beta1.SecretStore
@@ -38,61 +37,65 @@ var apiScope = InfisicalClientScope{
 
 type TestCases struct {
 	Name           string
-	MockClient     *fake.MockInfisicalClient
-	PropertyAccess string
+	MockStatusCode int
+	MockResponse   any
+	Property       string
 	Error          error
 	Output         any
 }
 
 func TestGetSecret(t *testing.T) {
+	key := "foo"
+
 	testCases := []TestCases{
 		{
-			Name: "Get_valid_key",
-			MockClient: &fake.MockInfisicalClient{
-				MockedGetSecretByKeyV3: func(data api.GetSecretByKeyV3Request) (string, error) {
-					return "value", nil
+			Name:           "Get_valid_key",
+			MockStatusCode: 200,
+			MockResponse: api.GetSecretByKeyV3Response{
+				Secret: api.SecretsV3{
+					SecretKey:   key,
+					SecretValue: "bar",
 				},
 			},
-			Error:  nil,
-			Output: []byte("value"),
+			Output: []byte("bar"),
 		},
 		{
-			Name: "Get_property_key",
-			MockClient: &fake.MockInfisicalClient{
-				MockedGetSecretByKeyV3: func(data api.GetSecretByKeyV3Request) (string, error) {
-					return `{"key":"value"}`, nil
+			Name:           "Get_property_key",
+			MockStatusCode: 200,
+			MockResponse: api.GetSecretByKeyV3Response{
+				Secret: api.SecretsV3{
+					SecretKey:   key,
+					SecretValue: `{"bar": "value"}`,
 				},
 			},
-			Error:  nil,
-			Output: []byte("value"),
+			Property: "bar",
+			Output:   []byte("value"),
 		},
 		{
-			Name: "Key_not_found",
-			MockClient: &fake.MockInfisicalClient{
-				MockedGetSecretByKeyV3: func(data api.GetSecretByKeyV3Request) (string, error) {
-					// from server
-					return "", errors.New("Secret not found")
-				},
+			Name:           "Key_not_found",
+			MockStatusCode: 404,
+			MockResponse: api.InfisicalAPIError{
+				StatusCode: 404,
+				Err:        "Not Found",
+				Message:    "Secret not found",
 			},
-			Error:  errors.New("Secret not found"),
+			Error:  esv1beta1.NoSecretError{},
 			Output: "",
 		},
 	}
 
 	for _, tc := range testCases {
 		t.Run(tc.Name, func(t *testing.T) {
+			apiClient, closeFunc := api.NewMockClient(tc.MockStatusCode, tc.MockResponse)
+			defer closeFunc()
 			p := &Provider{
-				apiClient: tc.MockClient,
+				apiClient: apiClient,
 				apiScope:  &apiScope,
 			}
-			var property string
-			if tc.Name == "Get_property_key" {
-				property = "key"
-			}
 
 			output, err := p.GetSecret(context.Background(), esv1beta1.ExternalSecretDataRemoteRef{
-				Key:      "key",
-				Property: property,
+				Key:      key,
+				Property: tc.Property,
 			})
 
 			if tc.Error == nil {
@@ -106,39 +109,39 @@ func TestGetSecret(t *testing.T) {
 }
 
 func TestGetSecretMap(t *testing.T) {
+	key := "foo"
 	testCases := []TestCases{
 		{
-			Name: "Get_valid_key_map",
-			MockClient: &fake.MockInfisicalClient{
-				MockedGetSecretByKeyV3: func(data api.GetSecretByKeyV3Request) (string, error) {
-					return `{"key":"value"}`, nil
+			Name:           "Get_valid_key_map",
+			MockStatusCode: 200,
+			MockResponse: api.GetSecretByKeyV3Response{
+				Secret: api.SecretsV3{
+					SecretKey:   key,
+					SecretValue: `{"bar": "value"}`,
 				},
 			},
-			Error: nil,
 			Output: map[string][]byte{
-				"key": []byte("value"),
+				"bar": []byte("value"),
 			},
 		},
 		{
-			Name: "Get_invalid_map",
-			MockClient: &fake.MockInfisicalClient{
-				MockedGetSecretByKeyV3: func(data api.GetSecretByKeyV3Request) (string, error) {
-					return ``, nil
-				},
-			},
-			Error:  errors.New("unexpected end of JSON input"),
-			Output: nil,
+			Name:           "Get_invalid_map",
+			MockStatusCode: 200,
+			MockResponse:   []byte(``),
+			Error:          errors.New("unable to unmarshal secret foo"),
 		},
 	}
 
 	for _, tc := range testCases {
 		t.Run(tc.Name, func(t *testing.T) {
+			apiClient, closeFunc := api.NewMockClient(tc.MockStatusCode, tc.MockResponse)
+			defer closeFunc()
 			p := &Provider{
-				apiClient: tc.MockClient,
+				apiClient: apiClient,
 				apiScope:  &apiScope,
 			}
 			output, err := p.GetSecretMap(context.Background(), esv1beta1.ExternalSecretDataRemoteRef{
-				Key: "key",
+				Key: key,
 			})
 			if tc.Error == nil {
 				assert.NoError(t, err)