Merge pull request #820 from roeezis/split/03-servicediscovery-secretsmanager

fix(agent): resolve secrets_manager alongside identity_administration

Author: mladen-rusev-cyberark

internal/cyberark/servicediscovery/discovery.go

@@ -28,6 +28,13 @@ const (
 	// in responses from the Service Discovery API.
 	DiscoveryContextServiceName = "discoverycontext"
 
+	// SecretsManagerServiceName is the name of the Secrets Manager (Conjur
+	// Cloud) API in responses from the Service Discovery API. This is the host
+	// that serves `authn-jwt/<service-id>/<account>/authenticate` — NOT the
+	// identity_administration host. The server that validates the resulting
+	// token resolves this same service name.
+	SecretsManagerServiceName = "secrets_manager"
+
 	// maxDiscoverBodySize is the maximum allowed size for a response body from the CyberArk Service Discovery subdomain endpoint
 	// As of 2025-04-16, a response from the integration environment is ~4kB
 	maxDiscoverBodySize = 2 * 1024 * 1024
@@ -47,6 +54,17 @@ type Client struct {
 	cachedResponseMutex sync.Mutex
 }
 
+// mainActiveAPI returns the API URL of the first active "main" endpoint in
+// eps, or "" if there isn't one.
+func mainActiveAPI(eps []ServiceEndpoint) string {
+	for _, ep := range eps {
+		if ep.Type == "main" && ep.IsActive && ep.API != "" {
+			return ep.API
+		}
+	}
+	return ""
+}
+
 // New creates a new CyberArk Service Discovery client. If the ARK_DISCOVERY_API
 // environment variable is set, it is used as the base URL for the service
 // discovery API. Otherwise, the production URL is used.
@@ -101,11 +119,13 @@ type ServiceEndpoint struct {
 	API      string `json:"api"`
 }
 
-// This is a convenience struct to hold the two ServiceEndpoints we care about.
-// Currently, we only care about the Identity API and the Discovery Context API.
+// This is a convenience struct to hold the ServiceEndpoints we care about:
+// the Identity API, the Discovery Context API, and the Secrets Manager
+// (Conjur Cloud) API used for the authn-jwt token exchange.
 type Services struct {
 	Identity         ServiceEndpoint
 	DiscoveryContext ServiceEndpoint
+	SecretsManager   ServiceEndpoint
 }
 
 // DiscoverServices fetches from the service discovery service for the configured subdomain
@@ -163,35 +183,41 @@ func (c *Client) DiscoverServices(ctx context.Context) (*Services, string, error
 		}
 		return nil, "", fmt.Errorf("failed to parse JSON from otherwise successful request to service discovery endpoint: %s", err)
 	}
-	var identityAPI, discoveryContextAPI string
+	var identityAPI, discoveryContextAPI, secretsManagerAPI string
 	for _, svc := range discoveryResp.Services {
 		switch svc.ServiceName {
 		case IdentityServiceName:
-			for _, ep := range svc.Endpoints {
-				if ep.Type == "main" && ep.IsActive && ep.API != "" {
-					identityAPI = ep.API
-					break
-				}
-			}
+			identityAPI = mainActiveAPI(svc.Endpoints)
 		case DiscoveryContextServiceName:
-			for _, ep := range svc.Endpoints {
-				if ep.Type == "main" && ep.IsActive && ep.API != "" {
-					discoveryContextAPI = ep.API
-					break
-				}
-			}
+			discoveryContextAPI = mainActiveAPI(svc.Endpoints)
+		case SecretsManagerServiceName:
+			secretsManagerAPI = mainActiveAPI(svc.Endpoints)
 		}
 	}
 
+	// identityAPI is required unconditionally, unlike discoveryContextAPI and
+	// secretsManagerAPI below: it's present and active for every healthy
+	// tenant, so callers may rely on it being non-empty without checking it
+	// themselves again.
 	if identityAPI == "" {
 		return nil, "", fmt.Errorf("didn't find %s in service discovery response, "+
 			"which may indicate a suspended tenant; unable to detect CyberArk Identity API URL", IdentityServiceName)
 	}
-	//TODO: Should add a check for discoveryContextAPI too?
+	// discoveryContextAPI and secretsManagerAPI are deliberately not required
+	// here, unlike identityAPI above: not every caller needs both, and
+	// requiring secretsManagerAPI would break every existing
+	// username/password install on a tenant not yet onboarded to Conjur.
+	// Each caller that needs one validates it itself — e.g.
+	// cyberark.NewDatauploadClient rejects an empty discoveryContextAPI, and
+	// cyberark.selectAuthenticator rejects an empty secretsManagerAPI on the
+	// Conjur JWT path. Not every caller does this yet (keyfetch's client
+	// doesn't check discoveryContextAPI), so an empty value can still surface
+	// downstream as a less obvious error.
 
 	services := &Services{
 		Identity:         ServiceEndpoint{API: identityAPI},
 		DiscoveryContext: ServiceEndpoint{API: discoveryContextAPI},
+		SecretsManager:   ServiceEndpoint{API: secretsManagerAPI},
 	}
 
 	c.cachedResponse = services

internal/cyberark/servicediscovery/discovery_test.go

@@ -62,6 +62,9 @@ func Test_DiscoverIdentityAPIURL(t *testing.T) {
 				DiscoveryContext: ServiceEndpoint{
 					API: mockDiscoveryContextAPIURL,
 				},
+				SecretsManager: ServiceEndpoint{
+					API: mockSecretsManagerAPIURL,
+				},
 			})
 
 			client := New(httpClient, testSpec.subdomain)
@@ -76,6 +79,13 @@ func Test_DiscoverIdentityAPIURL(t *testing.T) {
 			if services.Identity.API != testSpec.expectedURL {
 				t.Errorf("expected API URL=%s\nobserved API URL=%s", testSpec.expectedURL, services.Identity.API)
 			}
+			// The Conjur authn-jwt exchange is served by secrets_manager, not
+			// by identity_administration. Parsing it into the wrong field means
+			// every live token exchange 404s/401s, which the Conjur unit tests
+			// cannot catch because they point their mock at whichever field the
+			// code reads.
+			assert.Equal(t, mockSecretsManagerAPIURL, services.SecretsManager.API)
+			assert.NotEqual(t, services.Identity.API, services.SecretsManager.API)
 		})
 	}
 }

internal/cyberark/servicediscovery/mock.go

@@ -25,6 +25,7 @@ const (
 
 	mockIdentityAPIURL         = "https://ajp5871.id.integration-cyberark.cloud"
 	mockDiscoveryContextAPIURL = "https://venafi-test.inventory.integration-cyberark.cloud/"
+	mockSecretsManagerAPIURL   = "https://venafi-test.secretsmgr.integration-cyberark.cloud/api"
 	prefix                     = "/api/public/tenant-discovery?bySubdomain="
 )
 

internal/cyberark/servicediscovery/testdata/README.md

@@ -9,6 +9,7 @@ NOTE: This API is not implemented yet as of 02.09.2025 but is expected to be fin
 curl -fsSL "${ARK_DISCOVERY_API}?bySubdomain=${ARK_SUBDOMAIN}" | jq
 ```
 
-Then replace `identity_administration.api` with `{{ .Identity.API }}` and
-`discoverycontext.api` with `{{ .DiscoveryContext.API }}`. Those Go template
+Then replace `identity_administration.api` with `{{ .Identity.API }}`,
+`discoverycontext.api` with `{{ .DiscoveryContext.API }}`, and
+`secrets_manager.api` with `{{ .SecretsManager.API }}`. Those Go template
 fields will be substituted in the tests.

internal/cyberark/servicediscovery/testdata/discovery_success.json.template

@@ -31,7 +31,7 @@
           "is_active": true,
           "type": "main",
           "ui": "https://ui.test-conjur.cloud",
-          "api": "https://venafi-test.secretsmgr.integration-cyberark.cloud/api"
+          "api": "{{ .SecretsManager.API }}"
         }
       ]
     },