Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add pod identity support for namespaces and per-resource scoped auth #3187

Merged
merged 14 commits into from
Aug 30, 2023
Merged

Add pod identity support for namespaces and per-resource scoped auth #3187

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
072db44
Add pod identity support for namespaces and per-resource scoped auth
super-harsh Aug 13, 2023
5fbfce6
Add comment and update ID var
super-harsh Aug 14, 2023
14b8427
Update documentation
super-harsh Aug 14, 2023
a2a3dfc
Add enum for Identity mode
super-harsh Aug 25, 2023
e92c5c8
Update IdentityAuthMode flag to AuthMode
super-harsh Aug 28, 2023
dd4e353
Merge branch 'main' into feature/scoped-pod-identity
theunrepentantgeek Aug 28, 2023
092d6d6
Update enum values; Update auth mode logic to return error
super-harsh Aug 29, 2023
3b7bdb9
minor refactor
super-harsh Aug 29, 2023
ae7b436
Refactor naming
super-harsh Aug 29, 2023
8119eb6
Default to workload identity if auth_mode==''
super-harsh Aug 29, 2023
4a7bf9e
Export auth mode options constants
super-harsh Aug 29, 2023
8ad87aa
Merge branch 'main' into feature/scoped-pod-identity
super-harsh Aug 29, 2023
9ee8b18
make the authmode check case sensitive
super-harsh Aug 30, 2023
163508b
Merge branch 'main' into feature/scoped-pod-identity
super-harsh Aug 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
159 changes: 141 additions & 18 deletions docs/hugo/content/guide/authentication/credential-format.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,8 +139,9 @@ metadata:
namespace: my-namespace
stringData:
AZURE_SUBSCRIPTION_ID: "$AZURE_SUBSCRIPTION_ID"
AZURE_TENANT_ID: "$AZURE_TENANT_ID"
AZURE_CLIENT_ID: "$AZURE_CLIENT_ID"
AZURE_TENANT_ID: "$AZURE_TENANT_ID"
AZURE_CLIENT_ID: "$AZURE_CLIENT_ID"
AUTH_MODE: "workload"
EOF
```

Expand All @@ -160,8 +161,9 @@ metadata:
namespace: my-namespace
stringData:
AZURE_SUBSCRIPTION_ID: "$AZURE_SUBSCRIPTION_ID"
AZURE_TENANT_ID: "$AZURE_TENANT_ID"
AZURE_CLIENT_ID: "$AZURE_CLIENT_ID"
AZURE_TENANT_ID: "$AZURE_TENANT_ID"
AZURE_CLIENT_ID: "$AZURE_CLIENT_ID"
AUTH_MODE: "workload"
EOF
```

Expand Down Expand Up @@ -448,7 +450,27 @@ export IDENTITY_CLIENT_ID="$(az identity show -g ${IDENTITY_RESOURCE_GROUP} -n $
export IDENTITY_RESOURCE_ID="$(az identity show -g ${IDENTITY_RESOURCE_GROUP} -n ${IDENTITY_NAME} --query id -otsv)"
```

#### Manual Deploy
### Create the secret

{{< tabpane text=true left=true >}}
{{% tab header="**Scope**:" disabled=true /%}}
{{% tab header="Global" %}}

If installing ASO for the first time, you can pass these values via Helm arguments:
```bash
helm upgrade --install --devel aso2 aso2/azure-service-operator \
--create-namespace \
--namespace=azureserviceoperator-system \
--set azureSubscriptionID=$AZURE_SUBSCRIPTION_ID \
--set aadPodIdentity.enable=true \
--set aadPodIdentity.azureManagedIdentityResourceId=${IDENTITY_RESOURCE_ID} \
--set azureClientID=${IDENTITY_CLIENT_ID} \
--set crdPattern='resources.azure.com/*;containerservice.azure.com/*;keyvault.azure.com/*;managedidentity.azure.com/*;eventhub.azure.com/*'
```

See [CRD management]( {{< relref "crd-management" >}} ) for more details about `crdPattern`.
matthchr marked this conversation as resolved.
Show resolved Hide resolved

Otherwise, if deploying manually:

Deploy an `AzureIdentity`:
```bash
Expand Down Expand Up @@ -479,7 +501,7 @@ spec:
EOF
```

Deploy the `aso-controller-settings` secret, configured to use the identity:
Create or update the `aso-controller-settings` secret:
```bash
cat <<EOF | kubectl apply -f -
apiVersion: v1
Expand All @@ -494,20 +516,121 @@ stringData:
EOF
```

#### Helm Chart Deploy
**Note:** The `aso-controller-settings` secret contains more configuration than just the global credential.
If ASO was already installed on your cluster and you are updating the `aso-controller-settings` secret, ensure that
[other values]( {{< relref "aso-controller-settings-options" >}} ) in that secret are not being overwritten.

{{% /tab %}}
{{% tab header="Namespace" %}}

Deploy an `AzureIdentity`:
```bash
helm repo add aso2 https://raw.githubusercontent.com/Azure/azure-service-operator/main/v2/charts
helm repo update
cat <<EOF | kubectl apply -f -
apiVersion: "aadpodidentity.k8s.io/v1"
kind: AzureIdentity
metadata:
name: aso-identity
namespace: azureserviceoperator-system
spec:
type: 0
resourceID: ${IDENTITY_RESOURCE_ID}
clientID: ${IDENTITY_CLIENT_ID}
EOF
```

helm upgrade --install --devel aso2 aso2/azure-service-operator \
--create-namespace \
--namespace=azureserviceoperator-system \
--set azureSubscriptionID=$AZURE_SUBSCRIPTION_ID \
--set aadPodIdentity.enable=true \
--set aadPodIdentity.azureManagedIdentityResourceId=${IDENTITY_RESOURCE_ID} \
--set azureClientID=${IDENTITY_CLIENT_ID} \
--set crdPattern='resources.azure.com/*;containerservice.azure.com/*;keyvault.azure.com/*;managedidentity.azure.com/*;eventhub.azure.com/*'
Comment on lines -503 to -510
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see this helm command present below this point - should it have been removed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have moved it above on line 455

Deploy an `AzureIdentityBinding` to bind this identity to the Azure Service Operator manager pod:
```bash
cat <<EOF | kubectl apply -f -
apiVersion: "aadpodidentity.k8s.io/v1"
kind: AzureIdentityBinding
metadata:
name: aso-identity-binding
namespace: azureserviceoperator-system
spec:
azureIdentity: aso-identity
selector: aso-manager-binding
EOF
```

See [CRD management]( {{< relref "crd-management" >}} ) for more details about `crdPattern`.
Create the `aso-credential` secret in your namespace:
```bash
cat <<EOF | kubectl apply -f -
apiVersion: v1
kind: Secret
metadata:
name: aso-credential
namespace: my-namespace
stringData:
AZURE_SUBSCRIPTION_ID: "$AZURE_SUBSCRIPTION_ID"
AZURE_TENANT_ID: "$AZURE_TENANT_ID"
AZURE_CLIENT_ID: "$IDENTITY_CLIENT_ID"
AUTH_MODE: "pod"
EOF
```

{{% /tab %}}
{{% tab header="Resource" %}}

Deploy an `AzureIdentity`:
```bash
cat <<EOF | kubectl apply -f -
apiVersion: "aadpodidentity.k8s.io/v1"
kind: AzureIdentity
metadata:
name: aso-identity
namespace: azureserviceoperator-system
spec:
type: 0
resourceID: ${IDENTITY_RESOURCE_ID}
clientID: ${IDENTITY_CLIENT_ID}
EOF
```

Deploy an `AzureIdentityBinding` to bind this identity to the Azure Service Operator manager pod:
```bash
cat <<EOF | kubectl apply -f -
apiVersion: "aadpodidentity.k8s.io/v1"
kind: AzureIdentityBinding
metadata:
name: aso-identity-binding
namespace: azureserviceoperator-system
spec:
azureIdentity: aso-identity
selector: aso-manager-binding
EOF
```

Create a per-resource secret. We'll use `my-resource-secret`:
```bash
cat <<EOF | kubectl apply -f -
apiVersion: v1
kind: Secret
metadata:
name: my-resource-secret
namespace: my-namespace
stringData:
AZURE_SUBSCRIPTION_ID: "$AZURE_SUBSCRIPTION_ID"
AZURE_TENANT_ID: "$AZURE_TENANT_ID"
AZURE_CLIENT_ID: "$IDENTITY_CLIENT_ID"
AUTH_MODE: "pod"
EOF
```

Create the ASO resource referring to `my-resource-secret`. We show a `ResourceGroup` here, but any ASO resource will work.

```bash
cat <<EOF | kubectl apply -f -
apiVersion: resources.azure.com/v1api20200601
kind: ResourceGroup
metadata:
name: aso-sample-rg
namespace: default
annotations:
serviceoperator.azure.com/credential-from: my-resource-secret
spec:
location: westcentralus
EOF
```

{{% /tab %}}
{{< /tabpane >}}
41 changes: 41 additions & 0 deletions v2/internal/identity/credential_provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,17 @@ const (
FederatedTokenFilePath = "/var/run/secrets/tokens/azure-identity"
)

type AuthModeOption string

const (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could these constants be made available to import like #3171?

podIdentity AuthModeOption = "pod"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better to call it podidentity and workloadidentity?

workloadIdentity AuthModeOption = "workload"

// AuthMode enum is used to determine if we're using Pod Identity or Workload Identity
//authentication for namespace and per-resource scoped credentials
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//authentication for namespace and per-resource scoped credentials
// authentication for namespace and per-resource scoped credentials

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not actually updated?

AuthMode = "AUTH_MODE"
)

// Credential describes a credential used to connect to Azure
type Credential struct {
tokenCredential azcore.TokenCredential
Expand Down Expand Up @@ -258,6 +269,29 @@ func (c *credentialProvider) newCredentialFromSecret(secret *v1.Secret) (*Creden
}, nil
}

var authMode AuthModeOption
if value, hasUsePodIdentity := secret.Data[AuthMode]; hasUsePodIdentity {
authMode = authModeOrDefault(string(value))
}

if authMode == podIdentity {
tokenCredential, err := azidentity.NewManagedIdentityCredential(&azidentity.ManagedIdentityCredentialOptions{
ClientOptions: azcore.ClientOptions{},
ID: azidentity.ClientID(clientID),
})

if err != nil {
return nil, errors.Wrap(err, errors.Errorf("invalid Managed Identity for %q encountered", nsName).Error())
}

return &Credential{
tokenCredential: tokenCredential,
subscriptionID: string(subscriptionID),
credentialFrom: nsName,
secretData: secret.Data,
}, nil
}

// Here we check for workload identity if client secret is not provided.
tokenCredential, err := azidentity.NewWorkloadIdentityCredential(&azidentity.WorkloadIdentityCredentialOptions{
ClientID: string(clientID),
Expand Down Expand Up @@ -300,3 +334,10 @@ func (c *credentialProvider) getSecret(ctx context.Context, namespace string, se
func getSecretNameFromAnnotation(credentialFrom string, resourceNamespace string) types.NamespacedName {
return types.NamespacedName{Namespace: resourceNamespace, Name: credentialFrom}
}

func authModeOrDefault(mode string) AuthModeOption {
if mode == string(podIdentity) {
return podIdentity
}
return workloadIdentity
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This treats anything other than podIdentity as workloadIdentity - including foo, bang, and oh-oh.

A misconfigured value should at least trigger a log warning, if not an actual error.

I'm also a fan of being case insensitive for configuration values, though @matthchr doesn't always agree.

Suggested change
func authModeOrDefault(mode string) AuthModeOption {
if mode == string(podIdentity) {
return podIdentity
}
return workloadIdentity
}
func authModeOrDefault(mode string) (AuthModeOption, error) {
if strings.EqualFold(mode, string(podIdentity)) {
return podIdentity, nil
}
if strings.EqualFold(mode, string(workloadIdentity)) {
return workloadIdentity, nil
}
return "", errors.Errorf("authorization mode %q not valid", mode)
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also a fan of being case insensitive for configuration values, though @matthchr doesn't always agree.

I don't have a philosophical objection here one way or the other. I do think that @theunrepentantgeek is correct we should have a warning or error if the user gets it wrong. I think from a consistency perspective, being case-sensitive here might make sense given other fields (such as enums in the CRDs themselves) are also case-sensitive, and case-sensitivity seems to be "the standard" in the Kubernetes world (look at labels which are case-sensitive, or names which only allow lowercase and thus are case-sensitive).

My dislike of case-insensitivity comes primarily from REST, where attempting to be case-insensitive is awkward because JSON keys are stereotypically case-sensitive (so the expectation is not necessarily case-insensitivity) and REST entities are both set by the user and returned to the user. In the "returned to the user" scenario, case insensitivity gets awkward because you want to preserve what they've sent you, but internally you want to store it in a single canonical form. Accomplishing this ends up being:

  1. More work in your backend (need to remember their case)
  2. Confusing. Which of their cases do you remember? The latest PUT? Or the first PUT? Or something else?

Postel's law ends up not really applying because usually in REST you have to return exactly what you accept (the expectation is that what you PUT is what you GET, basically), so you can't actually be liberal in what you accept but conservative in what you return. They are one and the same. You have to pick a winner and I tend to favor picking the easier to implement (and simpler to explain) approach and just give an error in all other cases.