[Feat] API Key and custom headers as an alternative method of authorisation#1062
Conversation
|
How do I get this PR reviewed? |
|
|
||
| // Set authentication - API key takes precedence if both are provided | ||
| if hasAPIKey { | ||
| apiClient.AddDefaultHeader("X-Ntnx-Api-Key", credentials.APIKey) |
There was a problem hiding this comment.
We have a specific method defined in the go sdk to set the API key. Can we use that instead of adding it in DefaultHeader?
example: apiClient.SetApiKey(credentials.APIKey)
https://developers.nutanix.com/sdk-reference?namespace=vmm&version=v4.2&language=golang
There was a problem hiding this comment.
Unfortunately, SetApiKey() doesn't actually work in the current SDK (v4.2.1). All generated API methods are hardcoded to use authNames := []string{"basicAuthScheme"} (see categories_api.go:71), which
means the API key authentication code in prepareRequest() never executes. The AddDefaultHeader() approach works because default headers are applied after the authentication loop, regardless of the authNames list.
There was a problem hiding this comment.
Can you refer to https://github.com/nutanix/ntnx-api-golang-clients/blob/main/vmm-go-client/api/images_api.go?? I guess SetApiKey() doesn't work for Prism Client https://github.com/nutanix/ntnx-api-golang-clients/blob/main/prism-go-client/api , rest all API client work with SetApiKey(), please cross check.
There was a problem hiding this comment.
I had Claude look over that, too much code for me! It's finding reflect what I experienced, but to be fair I didn't delve deeper after it failed on nutanix_category_v2 data source, so I didn't realize that it worked for some.
Claude's summary:
After checking the upstream SDK source:
vmm-go-client:authNames := []string{"apiKeyAuthScheme", "basicAuthScheme"}—SetApiKey()worksprism-go-client:authNames := []string{"basicAuthScheme"}—SetApiKey()does not work
This is consistent across all files in prism-go-client/api (categories, tasks, domain_manager, etc.), and all other clients (networking, iam, clusters, vmm, volumes, etc.) do include "apiKeyAuthScheme".
However, AddDefaultHeader("X-Ntnx-Api-Key", ...) remains the correct approach for this provider for two reasons:
1. Prism v2 resources break with SetApiKey()
The following resources and data sources all go through prism-go-client and would fail to authenticate with API key if we switched to SetApiKey():
| Resources | Data Sources |
|---|---|
| nutanix_category_v2 | nutanix_category_v2, nutanix_categories_v2 |
| nutanix_pc_deploy_v2 | nutanix_pc_v2, nutanix_pcs_v2 |
| nutanix_pc_backup_target_v2 | nutanix_pc_backup_target_v2, nutanix_pc_backup_targets_v2 |
| nutanix_pc_restore_source_v2 | nutanix_pc_restore_source_v2 |
| nutanix_pc_restore_v2 | nutanix_pc_restore_point_v2, nutanix_pc_restore_points_v2 |
| nutanix_pc_registration_v2 | nutanix_restorable_pcs_v2 |
| nutanix_pc_unregistration_v2 |
2. v3 resources don't use the SDK at all
All v3 resources (VMs, subnets, images, projects, Karbon, NDB, Foundation, Self Service, etc.) use a
custom HTTP client with an applyAuthHeaders() method that sets the X-Ntnx-Api-Key header directly.
SetApiKey() is not applicable to these.
Since AddDefaultHeader() is applied after the authNames authentication loop it works uniformly
across all clients including Prism, making it the only approach that covers the full surface area of
the provider.
There was a problem hiding this comment.
If it wasn't for the nutanix_category_v2 being affected by this, I'd definitely make the change.
I could implement calling SetApiKey like below.
if hasAPIKey {
apiClient.SetApiKey(credentials.APIKey) // works for all clients except prism
apiClient.AddDefaultHeader("X-Ntnx-Api-Key", credentials.APIKey) // fallback, covers prism
}Is it worth making this change, or is AddDefaultHeader is sufficient is enough for now and could be revisited when/if the SDK is updated?
@Haroon-Dweikat-Ntx tagging you for feedback as well.
There was a problem hiding this comment.
Hi @andrew-sumner ,
This is the list of namespaces which have issue with the predefined method for setting API key:
Cluster Management
Object Storage Management
Prism
Security
So, for these we have to add a fallback workaround, we checked after calling this method that is the api key set in headers or not. If it is not set then we should use this workaround. We did the same in Ansible, this is a small snippet from Ansible code:
# Workaround as set_api_key not working as expected for clusters mgmt api
if api_key:
default_headers = getattr(client, "_ApiClient__default_headers", {})
if "X-ntnx-api-key" not in default_headers:
client.add_default_header(
header_name="X-ntnx-api-key", header_value=api_key
)
There was a problem hiding this comment.
@abhinavbansal29, I tried these approaches:
-
Call both
_ = apiClient.SetApiKey(credentials.APIKey) apiClient.AddDefaultHeader("X-Ntnx-Api-Key", credentials.APIKey)
Result: error while fetching subnets : {"message": "Invalid cookies present in the request"}
Likely cause: The difference in case between this SetApiKey and AddDefaultHeader resulted in 2 headers
-
Set if failed
if apiClient.SetApiKey(credentials.APIKey) != nil { // Client has no apiKeyAuthScheme; inject the header directly as a fallback. apiClient.AddDefaultHeader("X-Ntnx-Api-Key", credentials.APIKey) }
Result: error while fetching categories : Get "/api/iam/authn/v1/oidc/auth?%24filter=key+eq+%27Organisation%27...": unsupported protocol scheme ""
Likely cause: the != nil fallback to AddDefaultHeader never fires.
-
Call both but use same case for header as SetApiKey
_ = apiClient.SetApiKey(credentials.APIKey) apiClient.AddDefaultHeader("X-ntnx-api-key", credentials.APIKey)
Result: Worked
Conclusion
The problem with SetApiKey, is that whether the stored key is actually applied to a request is decided per-call in prepareRequest, which loops over the method-level authNames slice — and that's only known at call time, not at configuration time. There's no way to inspect it from the outside.
While Option 3 worked, it's very fragile in that if the case of the header ever changes, we get the result from Option 1.
The only approach that reliably works for all clients is AddDefaultHeader — the SDK's scheme-based injection is too inconsistent to depend on.
While I'll do whatever you want to get this PR in, my recommendation is to stick with current implementation.
There was a problem hiding this comment.
We have raised this issue internally with the API team, you can keep the current approach for now. We will update once the new SDKs with the fix is available.
|
@andrew-sumner Can you please run all the tests, authentication method: api_key. This helps us to get the required confidence to take this in. + @Haroon-Dweikat-Ntx |
|
@andrew-sumner This is an excellent improvement—we’d love to bring it in. If possible, could you start working on it soon so we can aim to include it in the next release? |
|
Yep, aiming to have this out today. |
|
@GullapalliAkhil I think the tests show the PR is fully functional, the failures are environment/permission-specific, not code issues. Unit Tests$env:NUTANIX_API_KEY = "***"
$env:NUTANIX_HEADER_CF_ACCESS_CLIENT_ID = "***"
$env:NUTANIX_HEADER_CF_ACCESS_CLIENT_SECRET = "***"
$env:NUTANIX_ENDPOINT = "***"
$env:NUTANIX_PORT = "443"
$env:NUTANIX_INSECURE = "true"
go test ./... -v -timeout 500m | Tee-Object -FilePath unit-test-results.txtSummary:
Acceptance TestsAs I am in a locked down mulit-tennant environment I've only run a subset of DataSource tests - this did highlight two issues that I had to fix:
$env:TF_ACC = "1"
$env:NUTANIX_STORAGE_CONTAINER = "0"
go test ./nutanix/services/clusters/... -v -run "DataSource" -timeout 30m 2>&1 | Tee-Object -FilePath test-results-readonly.txt
go test ./nutanix/services/clustersv2/... -v -run "DataSource" -timeout 30m 2>&1 | Tee-Object -FilePath test-results-readonly.txt -AppendTest failures are:
|
|
@GullapalliAkhil Is that enough evidence or do you need more? |
|
LGTM, @Haroon-Dweikat-Ntx Please take a look at this. |
|
|
||
| // Check authentication - either username/password OR api_key must be set | ||
| hasBasicAuth := os.Getenv("NUTANIX_USERNAME") != "" && os.Getenv("NUTANIX_PASSWORD") != "" | ||
| hasAPIKey := os.Getenv("NUTANIX_API_KEY") != "" |
There was a problem hiding this comment.
What if user set both username, Password and API key, what's the behaviour? Which one gets the precedence?
There was a problem hiding this comment.
API key will take precedence I'm updating the code to display a warning.
| } | ||
|
|
||
| // Set authentication - API key takes precedence if both are provided | ||
| if hasAPIKey { |
There was a problem hiding this comment.
We also need to handle a scenario where user set username, password and API key... We need to show a warning like provided username, password and API key, using API key and proceeding further
There was a problem hiding this comment.
Have update the code to show an error if username or password is provided and not the other, and a warning if both username and password and api key are provided.
| * `NDB` : For Nutanix Database Service (NDB) related resources and data sources. | ||
| Going from 1.8.0-beta release of nutanix provider, fields inside provider configuration would be mandatory as per the usecase : | ||
|
|
||
| * `Prism Central & Karbon` : For prism central and karbon related resources and data sources, `endpoint` is mandatory plus either (`username` and `password`) or `api_key` for authentication. |
There was a problem hiding this comment.
We need to make changes at README.md, contributing.md, env.example files as well... please take a look at them and do the needful.
Haroon-Dweikat-Ntx
left a comment
There was a problem hiding this comment.
LGTM. As @GullapalliAkhil noted, what is the expected behavior if both username/password and an API key are provided? Which credential has higher priority?
|
@andrew-sumner please fix the lint issues, and make sure to merge the master to get the latest changes. |
|
Hi @andrew-sumner, can you please address this ASAP, we will take it in the next release |
|
Created: #1090, pipeline changes to support test execution using API KEY. |
API key will take precedence I'm updating the code to display a warning. |
b5692f8 to
f99f961
Compare
I haven't been able to find any linting errors can you send me a link? |
try to run the |
Thanks for that, fixed. |
|
@andrew-sumner, honestly this is the first time we are doing this, it'll be helpful you provide the steps for us in doing it. This'll hep us in qualifying the custom headers and make the PR merge in master.
|
|
@GullapalliAkhil I've attached a small demo project - readme with instructions for settings this up, a script for the proxy that will log headers, and sample terraform. As the proxy just logs and forwards, you don't need to use API key, you could use user/password auth. The Cloudflare headers should be ignored by Prism as you don't have a Cloudflare fronted api. Setting this up did show a small issue where a header set both as environment variable and in terraform where both being added to the header - latest commit fixed that. I've also rebased from main as this PR was showing a conflict.
|
|
@abhinavbansal29 Any chance you can approve the workflow on the ansible PR? |
04bc8c7 to
c78b051
Compare
…risation to user credentials - Either API Key or credentials are required - Custom headers (e.g. for Cloudflare) are supported - API Key and custom headers can be specified by environment variables - Logic for configuring provider for v4 APIs centralised
…re set Env var-derived header names (via NUTANIX_HEADER_ prefix) were stored under their canonical cased key (e.g. Cf-Access-Client-Id), while config-defined headers used the user's original casing (e.g. CF-Access-Client-Id). As these are different map keys the config value did not overwrite the env var value, causing both to be sent on every request. Fix by doing a case-insensitive delete of any existing env var entry before inserting the config value, preserving the user's intended casing exactly. Also replaces the custom toTitleCase helper with http.CanonicalHeaderKey in the env var path, and removes the now-redundant TestToTitleCase test.
c78b051 to
1c9d414
Compare
We are testing this in Ansible. We will get back on that. |
Manually tested this by setting up reverse proxy. Thanks @andrew-sumner |
|
@Haroon-Dweikat-Ntx Let's merge this in to master and take this in to release branch. All our Iris.1 regressions should be run with this changes. |
|
@andrew-sumner Can we fix lints asap? |
I can have a look in about 30 mins |
|
Formatting issue fixed, workflow needs approval |
|
Two approvals and a green PR, must be getting close😉 |
|
@Haroon-Dweikat-Ntx , @GullapalliAkhil , @abhinavbansal29 Thank you all for your support in getting this PR merged, it was much appreciated! |
Closes #1057
Closes #1040