Add vault extra audiences

[andrey-dubnik]

enable audiences example

Example of the spec

spec:
  vault:
    path: pki/sign/sandbox
    server: https://vault.net
    auth:
      kubernetes:
        role: sandbox-issuer
        mountPath: /v1/auth/kubernetes
        serviceAccountRef:
          name: sandbox-issuer
          audiences:
          - vault

---

Pull Request Motivation

Motivation is elaborated in #6666

Kind

/feature

Release Note

Added option to provide additional audiences in the service account auth section for vault

[jetstack-bot]

Hi @andrey-dubnik. Thanks for your PR.

I'm waiting for a cert-manager member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[andrey-dubnik]

@SgtCoDFish here is a new PR with corrected DCO, I had to abort another one as it was past saving...

[SgtCoDFish]

/kind feature
/ok-to-test

Linked to #6686

[SgtCoDFish]

This looks good, just a couple of bits that need looking at!

If it helps, with rebasing / DCO signoff, I like to do:

git rebase -i --signoff master

[SgtCoDFish]

/lgtm
/approve

Thanks very much, this is really great 😁

[SgtCoDFish]

/test pull-cert-manager-master-e2e-v1-28

[jetstack-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: SgtCoDFish

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [SgtCoDFish]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[maelvls]

Hey, sorry for the delay, I see that the PR has already been merged. 😅 It took me a while to test everything manually. I described how I manually tested this in this Hackmd page.

As Ash explained, I went "too deep" in the risk analysis of serviceAccountRef. I think it is OK to assume that the Issuer and ClusterIssuer resources are "safe". I think it would have been OK to have a default audience vault, or even to simply let people set their own audiences from the start without any audiences (which would default to the Kubernetes API's default audience).

I have a few remarks:

1. audiences should be under serviceAccountRef since setting the audiences only applies to the serviceAccountRef field. Example:

   spec:
     vault:
       path: pki/sign/sandbox
       server: https://vault.net
       auth:
         kubernetes:
           role: sandbox-issuer
           mountPath: /v1/auth/kubernetes
           serviceAccountRef:
             name: sandbox-issuer
             audiences:
             - vault

2. I'm OK with the plural "audiences" since it matches the projected token API. But is there a reason for setting multiple audiences here? I struggle finding a good reason.

3. When someone sets something in audiences, is the default audience (e.g., vault://default/issuer overridden, or is the default audience added to the list of audiences?

4. In your example, the only audience is vault. Does this example work in the case of an external Vault? In my case, I had to use the following issuer:

   apiVersion: cert-manager.io/v1
   kind: Issuer
   metadata:
     name: vault-issuer
     namespace: default
   spec:
     vault:
       path: pki/sign/vault-issuer
       server: http://vault.default.svc.cluster.local:8200
       auth:
         kubernetes:
           role: vault-issuer
           mountPath: /v1/auth/kubernetes
           serviceAccountRef:
             name: vault-issuer
           audiences: [https://kubernetes.default.svc.cluster.local]

Note: Just before you submitted your PR, I was working with @SpectralHiss on documenting using the JWT authentication method in cert-manager/website#1397 (see preview). We thought the JWT auth would be easier to handle... but we found some hurdles (mainly when folks are using OpenShift), and we were happy to see your PR; it made us realize that giving the possibility of configuring the audiences isn't that bad after all.

[maelvls]

This should be moved under serviceAccountRef IMO. audiences and secretRef wouldn't make sense for example:

kind: Issuer
spec:
  auth:
    kubernetes:
      secretRef:
        name: vault
        key: token
      audiences:
      - https://kubernetes.default.svc.cluster.local

[andrey-dubnik]

Thanks for commenting. Here is a line of thinking.

This change should not be a breaking change so if and when the audiences are supplied they are merged with the default audience. This way current users won't experience a failure during a version upgrade.

Since it is possible to add audiences now there is no reason to limit it to only one. E.g. default k8s token in aks have quite a few which include the issuer and the host. It is up to user to decide which audiences to provide if any.

For my use case I would put k8s (AKS) public url into the audiences list as this is what is expected on the vault side. But can be more expressive and mention multiple audiences if on vault end I need to explicitly configure the audience validation. Not sure if I'm going to need more than one and need to think on the possible use case. Either way having multiple options is better than having only one.

Reg. the placement of the audiences, I thought the same way you did initially but end of the day mimicked the hashicorp spec where they have it on the same level with SA property so there is a level of consistency between the cert-manager and vault operator.

[andrey-dubnik]

One of the theoretical use cases for having multiple audiences would be in decoupling the vault role config from the cert-manager config as if I am to change vault role aud requirement I can register new aud as additional audiences list item in the cert-manager config and make changes on the vault side without breaking things.

[SgtCoDFish]

Reg. the placement of the audiences, I thought the same way you did initially but end of the day mimicked the hashicorp spec where they have it on the same level with SA property so there is a level of consistency between the cert-manager and vault operator.

I think I agree with Maël's suggestion here to be honest, I think it would be better to have it under serviceAccountRef. I think that design is cleaner and better matches what I'd expect. Would you maybe be open to creating a PR with that change? 😇

[maelvls]

Other thought: since 99% of folks who will be using the audiences field will because they want to use an external Vault, why not do something better? Something like:

kind: Issuer
spec:
  auth:
    kubernetes:
      serviceAccountRef:
        enableKubeAudience: true

The cluster's issuer URL is automatically added by cert-manager without having to figure out what is the issuer URL of the cluster, since each cluster has its own issuer URL

[andrey-dubnik]

Other thought: since 99% of folks who will be using the audiences field will because they want to use an external Vault, why not do something better? Something like:

kind: Issuer
spec:
  auth:
    kubernetes:
      serviceAccountRef:
        enableKubeAudience: true

The cluster's issuer URL is automatically added by cert-manager without having to figure out what is the issuer URL of the cluster, since each cluster has its own issuer URL

Probably not as I'd like to keep the option open for the audiences, at least the proposal won't meet our potential use scenarios. Cluster issuer in AUD is not what vault expects by default from what I've seen as it is looking for the kube API server as aud instead, which may change depending on what Vault or K8s teams do. If aud is flexible then user can coordinate the aud and the vault role.

I will make another PR to change the aud to be within the SA scope