CORE-427: Azure AKS OIDC federation / Azure VM user-assigned managed identity support #17157

andijcr · 2024-03-18T09:14:33Z

AKS

ASK managed identities are implemented with cloud_role/refresh_credentials implementations
azure_aks_refresh_impl.cc

new credentials implementation apply_abs_oauth_credentials.cc

azure_aks_refresh_impl.cc reads these environment variables

AZURE_CLIENT_ID
AZURE_TENANT_ID
AZURE_FEDERATED_TOKEN_FILE
AZURE_AUTHORITY_HOST

to compose a POST request to retrieve an authorization token that azure_aks_refresh_impl.cc will use to authorize calls to ABS.

The env variables are set up directly by AKS.

Azure VM

User-assigned managed identity via IMDS on a azure VM is implemented in azure_vm_refresh_impl
it requires setting cloud_storage_azure_managed_identity_id in configs to the user-assigned managed identity client_id.
The implementation will perform a GET request to the instance metadata service to retrieve an authorization token

Fixes https://github.com/redpanda-data/core-internal/issues/1125
Fixes https://github.com/redpanda-data/core-internal/issues/1126

Backports Required

Release Notes

Features

AKS OIDC Federation support
Azure VM user-assigned managed identity support

vbotbuildovich · 2024-03-19T00:42:37Z

new failures in https://buildkite.com/redpanda/redpanda/builds/46403#018e53e6-7781-4791-9788-c5539a80b1d2:

"rptest.tests.e2e_iam_role_test.ASKRoleFetchTests.test_write"

new failures in https://buildkite.com/redpanda/redpanda/builds/46403#018e540d-e5d6-4582-be55-33ad5036bac0:

"rptest.tests.e2e_iam_role_test.ASKRoleFetchTests.test_write"

new failures in https://buildkite.com/redpanda/redpanda/builds/46435#018e567e-7793-426c-96d4-1343812ca47a:

"rptest.tests.e2e_iam_role_test.ASKRoleFetchTests.test_write"

new failures in https://buildkite.com/redpanda/redpanda/builds/46435#018e5690-3c51-4ff3-8163-8a71a3864772:

"rptest.tests.e2e_iam_role_test.ASKRoleFetchTests.test_write"

new failures in https://buildkite.com/redpanda/redpanda/builds/46733#018e77cf-c715-4835-87a5-2fe6002e31d3:

"rptest.tests.partition_move_interruption_test.PartitionMoveInterruption.test_cancelling_partition_move.replication_factor=3.unclean_abort=False.recovery=no_recovery.compacted=False"

dotnwat

looking good.

should we avoid the trailing return type?
pointed out a few things that looks like they need unit tests
do you have a good sense for how well the code coverage is for the tests you've added?
is there a test plan written down somewhere for testing on azure?

src/v/cloud_roles/types.cc

src/v/cloud_storage/remote.cc

src/v/cloud_roles/request_response_helpers.cc

src/v/cloud_roles/request_response_helpers.h

src/v/cloud_roles/types.h

src/v/redpanda/admin/server.cc

src/v/cloud_storage/remote.cc

src/v/cloud_storage/types.cc

src/v/http/tests/http_imposter.cc

src/v/cloud_roles/azure_aks_refresh_impl.cc

src/v/cloud_roles/azure_vm_refresh_impl.cc

tests/rptest/tests/e2e_iam_role_test.py

vbotbuildovich · 2024-03-19T13:00:43Z

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46435#018e567e-778d-41a7-90d5-e1ae098ff741

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46624#018e671b-8488-4d12-90f1-939431136853

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46624#018e672d-51e5-474a-9723-c8b0a3148913

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46733#018e77cf-c71b-4120-b84e-b0cec4eaabf6

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46733#018e77e2-6d2a-4314-a587-e4112d3ac567

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46733#018e77e2-6d26-4dd8-9db5-1acb42f55243

dotnwat

looks really good. just a few questions and things to maybe clean up.

src/v/cloud_storage/remote.cc

src/v/cloud_roles/request_response_helpers.cc

src/v/cloud_roles/tests/schema_parsing_tests.cc

src/v/cloud_roles/types.h

src/v/config/configuration.cc

src/v/redpanda/admin/server.cc

src/v/cloud_storage/remote.cc

src/v/cloud_storage/types.cc

src/v/cloud_roles/signature.cc

andijcr · 2024-03-25T19:45:09Z

as discussed, followup with ducktape testing is moved here #17374

not an actual problem, but calling `<< credentials{abs_credentials{}}` would cause infinite recursion, because the implementation for credential would not find an operator<< abs_credential, and instead would call itself since credentials is constructible from abs_credentials. the fix is to use a concept to ensure that the actual calling type is credential and not an implicit conversion

parse_json_response_and_validate acceps a schema string_view, performs schema validation on the json::Document read from the iobuf parameters, and returns the document or an error in case of validation failures.

requests urls are saved as is, url params are not removed from the url, failing simple queries via has_calls and get_latests_request to not break existing behavior, these calls accepts ignore_url_params to perform a search only on the part before '?'

oauth on ABS requires an access token and the header x-ms-version. this commit incroduces apply_abs_oauth_credentials

AKS OpenID Credential federations places a jwt in the pod that can be used to get an access token with a limited lifespan. this commit introduces an implmentation of refresh credentials for this usecase

azure_vm_refresh_impl calls the vm IMDS endpoint to retrieve an access_token, and converts it to an abs_oauth_credentials configuration: cloud_storage_azure_managed_identity_id to be used with cloud_credentials_source::azure_vm_instance_metadata

in config_multi_property_validation small refactor for the next commit, when support is added for new cloud_credentials_source

extended validation to ensure that cloud_storage_azure_managed_identity_id is set if cloud_storage_credentials_source is azure_vm_instance_metadata

for readability, less lines and clutter

adapted get_abs_config to work with credential_source equal to either config_file, azure_aks_oidc_federation, azure_vm_instance_metadata. removed the strict requirement to have a shared_key. for managed idendities the autorization is done with an access_token.

andijcr · 2024-03-26T20:44:02Z

fixed merge conflict, addressed comments, trailing return type fix. previous ci failure was #16324 (it's failing a lot in dev)

dotnwat · 2024-03-27T18:12:16Z

src/v/cloud_roles/azure_aks_refresh_impl.cc

+
+    return ss::visit(
+      maybe_jresp,
+      [](auto err_resp) -> api_response_parse_result {


why doesn't this auto case match everything? will the more specific case (e.g. Document&) be chosen when possible?

ye it's the rule of template overloading having lower precedence than exact type match

dotnwat · 2024-03-27T18:21:06Z

src/v/cloud_roles/azure_vm_refresh_impl.cc

+      "&resource=https%3A%2F%2Fstorage.azure.com%2F"
+      "&client_id={}",
+      client_id_opt.value()));
+    req.set("Metadata", "true");


what is Metadata?

https://learn.microsoft.com/en-us/azure/virtual-machines/instance-metadata-service?tabs=linux#security-and-authentication
it's part of a mitigation to prevent redirection of requests.
since it's a get request, this header (required) make sure that the request originated from the app and it's not unintended/malicious

dotnwat · 2024-03-27T18:22:24Z

src/v/cloud_roles/azure_vm_refresh_impl.cc

+    // to the vm-local IMDS
+    auto req = http::client::request_header{};
+    req.method(boost::beast::http::verb::get);
+    // TODO fix deps and use boost::url


do you need a follow issue for this? the comment isn't clear what is needed.

don't think so, it's a comment i forgot to remove. boost::url has a nice interface to compose get requests, but it's not part of our dependencies. probably not really worth it.

dotnwat · 2024-03-27T18:22:53Z

src/v/cloud_roles/azure_vm_refresh_impl.cc

+    return std::visit(
+      ss::make_visitor(


why did you use ss::visit previous, but std::visit here?

just missed it

andijcr added the doc-needed label Mar 18, 2024

github-actions bot added the area/redpanda label Mar 18, 2024

andijcr force-pushed the feat/aks_oidc_support branch from 23500fc to fc45043 Compare March 18, 2024 15:13

andijcr changed the title ~~Azure AKS OIDC Federation support~~ Azure AKS OIDC federation / Azure VM user-assigned managed identity support Mar 18, 2024

andijcr force-pushed the feat/aks_oidc_support branch 2 times, most recently from 2390890 to d11ce73 Compare March 18, 2024 22:37

andijcr requested review from abhijat, andrwng and dotnwat March 18, 2024 22:38

dotnwat reviewed Mar 19, 2024

View reviewed changes

abhijat reviewed Mar 19, 2024

View reviewed changes

src/v/http/tests/http_imposter.cc Outdated Show resolved Hide resolved

abhijat reviewed Mar 19, 2024

View reviewed changes

src/v/cloud_roles/azure_aks_refresh_impl.cc Show resolved Hide resolved

abhijat reviewed Mar 19, 2024

View reviewed changes

src/v/cloud_roles/azure_vm_refresh_impl.cc Outdated Show resolved Hide resolved

abhijat reviewed Mar 19, 2024

View reviewed changes

tests/rptest/tests/e2e_iam_role_test.py Outdated Show resolved Hide resolved

andijcr force-pushed the feat/aks_oidc_support branch from d11ce73 to 0312938 Compare March 19, 2024 10:42

andijcr force-pushed the feat/aks_oidc_support branch from 0312938 to a3badbf Compare March 22, 2024 15:42

andijcr requested review from abhijat and dotnwat March 22, 2024 18:46

dotnwat reviewed Mar 23, 2024

View reviewed changes

This was referenced Mar 25, 2024

ducktake/azurite: enable https and oauth #17373

Open

CORE-2163: tests/e2e_iam_role_test: mock AKS and AzureVM credential test #17374

Draft

andijcr force-pushed the feat/aks_oidc_support branch from a3badbf to cad8307 Compare March 25, 2024 21:34

andijcr added 5 commits March 26, 2024 21:38

cloud_storage/remote: small refactor

adce15c

cloud_roles/uri_encode: move function to request_response_helpers

afcbfe4

cloud_roles/request_response_helpers: schema validation

7a3bcb0

parse_json_response_and_validate acceps a schema string_view, performs schema validation on the json::Document read from the iobuf parameters, and returns the document or an error in case of validation failures.

andijcr added 10 commits March 26, 2024 21:38

cloud_roles/abs_oauth_credentials: new credential type for abs oauth

6eac05e

oauth on ABS requires an access token and the header x-ms-version. this commit incroduces apply_abs_oauth_credentials

cloud_roles/azure_aks_refresh_impl: implementation for AKS OIDC

5a18bbb

AKS OpenID Credential federations places a jwt in the pod that can be used to get an access token with a limited lifespan. this commit introduces an implmentation of refresh credentials for this usecase

config/cloud_storage_credentials_source: azure_aks_oidc_federation

d472826

admin/server: refactor if-else to switch

7b91f3b

in config_multi_property_validation small refactor for the next commit, when support is added for new cloud_credentials_source

admin/server: validation for cloud_storage_credentials_source

008aadd

extended validation to ensure that cloud_storage_azure_managed_identity_id is set if cloud_storage_credentials_source is azure_vm_instance_metadata

cloud_storage/remote: refactor std::visit to ss::visit

a5aa8cf

for readability, less lines and clutter

cloud_storage/remote: remove asserts preventing refresher for ABS

5a86672

cloud_storage/abs_test_client: update for OIDC and user-managed identity

8ea1ee4

andijcr force-pushed the feat/aks_oidc_support branch from cad8307 to 8ea1ee4 Compare March 26, 2024 20:39

andijcr requested a review from dotnwat March 26, 2024 20:44

dotnwat approved these changes Mar 27, 2024

View reviewed changes

dotnwat merged commit 22776d1 into redpanda-data:dev Mar 27, 2024
17 checks passed

dotnwat changed the title ~~Azure AKS OIDC federation / Azure VM user-assigned managed identity support~~ CORE-427: Azure AKS OIDC federation / Azure VM user-assigned managed identity support Apr 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CORE-427: Azure AKS OIDC federation / Azure VM user-assigned managed identity support #17157

CORE-427: Azure AKS OIDC federation / Azure VM user-assigned managed identity support #17157

andijcr commented Mar 18, 2024 •

edited

vbotbuildovich commented Mar 19, 2024 •

edited

dotnwat left a comment

vbotbuildovich commented Mar 19, 2024 •

edited

dotnwat left a comment

andijcr commented Mar 25, 2024

andijcr commented Mar 26, 2024 •

edited

dotnwat Mar 27, 2024

andijcr Mar 27, 2024

dotnwat Mar 27, 2024

andijcr Mar 27, 2024

dotnwat Mar 27, 2024

andijcr Mar 27, 2024

dotnwat Mar 27, 2024

andijcr Mar 27, 2024

CORE-427: Azure AKS OIDC federation / Azure VM user-assigned managed identity support #17157

CORE-427: Azure AKS OIDC federation / Azure VM user-assigned managed identity support #17157

Conversation

andijcr commented Mar 18, 2024 • edited

AKS

Azure VM

Backports Required

Release Notes

Features

vbotbuildovich commented Mar 19, 2024 • edited

dotnwat left a comment

Choose a reason for hiding this comment

vbotbuildovich commented Mar 19, 2024 • edited

dotnwat left a comment

Choose a reason for hiding this comment

andijcr commented Mar 25, 2024

andijcr commented Mar 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andijcr commented Mar 18, 2024 •

edited

vbotbuildovich commented Mar 19, 2024 •

edited

vbotbuildovich commented Mar 19, 2024 •

edited

andijcr commented Mar 26, 2024 •

edited