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.

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

Implement a KubernetesSecretsRepository #4462

Merged
merged 1 commit into from
Jun 18, 2019
Merged

Implement a KubernetesSecretsRepository #4462

merged 1 commit into from
Jun 18, 2019

Conversation

ahmelsayed
Copy link
Contributor

@ahmelsayed ahmelsayed commented May 17, 2019

This adds a KubernetesSecretsRepository implementation of ISecretsRepository. I'm still working on adding some tests, but the functionality is done. I have few questions that I'll add as comments.

With this PR the user can set AzureWebJobsSecretStorageType=kubernetes

Then optionally you can set AzureWebJobsKubernetesSecretName to a kubernetes secret name in the same namespace as the deployment. If you don't set that, then we expect a secret to be volumeMounted into /run/secrets/functions-keys

Note: when using volumeMount, the secrets are readonly and can't be updated by the runtime APIs. When using a AzureWebJobsKubernetesSecretName with a kubernetes secret (or configMap), it's read/write.

Note: To use AzureWebJobsKubernetesSecretName, you need to allow the functions pod identity to be able to read and modify that secret.

the secret in kubernetes

apiVersion: v1
kind: Secret
metadata:
  name: function-keys
type: Opaque
data:
  host.master: <base64 of masterkey>
  function.HttpTrigger.default: <base64 of default key for HttpTrigger function>

if you set AzureWebJobsKubernetesSecretName to configmaps/{name} it'll use configmaps instead which doesn't require base64 enoding

2019-05-16_22-01-38

If you don't want to create an identity for the function pod, you can mount the sam secret or configmap using this

    spec:
      containers:
      - name: app
        volumeMounts:
        - mountPath: /run/secrets/functions-keys
          name: functions-keys
          readOnly: true
      volumes:
      - name: functions-keys
        secret:
          secretName: func-keys

and don't set AzureWebJobsKubernetesSecretName.

/cc @jeffhollan @paulbatum @mattchenderson

@jeffhollan
Copy link

This looks good. Only question (not urgent or blocking, more for docs) is clarifcation on the two notes:

Note: when using volumeMount, the secrets are readonly and can't be updated by the runtime APIs. When using a AzureWebJobsKubernetesSecretName with a kubernetes secret (or configMap), it's read/write.

I imagine most people likely aren't or won't be accessing runtime APIs here so imagine for vast majority this is more an FYI? I'm not even sure we document the key APIs for the runtime in Azure but wanted to make sure

Note: To use AzureWebJobsKubernetesSecretName, you need to allow the functions pod identity to be able to read and modify that secret.

Do you have an example of what allowing the pod identity to read and modify a secret is? Is this a Kubernetes RBAC identity thing?

@ahmelsayed
Copy link
Contributor Author

I imagine most people likely aren't or won't be accessing runtime APIs here so imagine for vast majority this is more an FYI? I'm not even sure we document the key APIs for the runtime in Azure but wanted to make sure

Yes, I'd imagine most kubernetes users would be generating their keys and creating that secret on deployment or something like that, then just use volumeMount. Though all of the runtime APIs assume that key management will go through the runtime APIs itself. So there is a bit of a disconnect there. We sorta document the keys APIs but not too much I guess. It's hard to judge their usage though because the UX uses them heavily.

Do you have an example of what allowing the pod identity to read and modify a secret is? Is this a Kubernetes RBAC identity thing?

Yes, exactly.

# Create an identity to use for your pod
# Alternativly you can just set the RoleBinding below for
# the `default` identity for the namespace, though all pods
# in this namespace will have access to the secret.
apiVersion: v1
kind: ServiceAccount
metadata:
  name: function-identity
---
# Create a secret-manager role
# Change the resource to `configmaps` if you wanna use
# ConfigMap instead of a Secret
apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
  name: secrets-manager
rules:
- apiGroups: [""]
  resources: ["secrets"]
  verbs: ["get", "list", "watch", "create", "update", "patch", "delete"]
---
# Create a RoleBinding between secrets-manager <=> function-identity
apiVersion: rbac.authorization.k8s.io/v1
kind: RoleBinding
metadata:
  name: function-identity-role-binding
roleRef:
  apiGroup: rbac.authorization.k8s.io
  kind: Role
  name: secrets-manager
subjects:
- kind: ServiceAccount
  name: function-identity
---

Then you can give that identity to your pod using this in your deployment object

apiVersion: apps/v1
kind: Deployment
metadata:
  ...
spec:
    ...
    spec:
+      serviceAccountName: function-identity
      containers:
      ....

/cc @anirudhgarg for FYI as well.

}
}

return hostSecrets?.MasterKey == null ? null : hostSecrets;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think there might be a bug in KeyVaultSecretsRepository here as I was looking at it as an example to make this one.

SecretsManager assumes that if ReadAsync returned a non-null object for HostSecrets, then a _master key must exist. If this is the first time to use this repository on a brand new Secret (and I would assume KeyVault too, though I didn't test KeyVault) and you return a non-null HostSecrets without a _master key in it, all Admin operations will fail with a 500

/cc @alrod

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this does look like a bug.

Copy link
Member

@fabiocav fabiocav left a comment

Choose a reason for hiding this comment

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

Took a quick look and this looks good! Mostly nit comments and a couple of questions at this point.


private static async Task<IDictionary<string, string>> GetFromFiles(string path)
{
var files = await FileUtility.GetFilesAsync(path, "*");
Copy link
Member

Choose a reason for hiding this comment

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

nit: var -> string (for files)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what is the rule you use for var vs typeName?

Copy link
Member

Choose a reason for hiding this comment

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

The key thing (for me) is whether or not the type is evident.

If the variable is being assigned from a method call, (e.g. var foo = SomeMethod()), I opt to use the type instead of var as that significantly improves readability.

@ahmelsayed
Copy link
Contributor Author

Copy link
Member

@fabiocav fabiocav left a comment

Choose a reason for hiding this comment

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

Didn't spend a lot of time this time around, but didn't have any major concerns from the first review.

@ahmelsayed ahmelsayed merged commit 30fa09f into dev Jun 18, 2019
@ahmelsayed ahmelsayed deleted the ahmels-k8s-secret branch June 18, 2019 22:08
@cwoolum
Copy link

cwoolum commented Jun 21, 2019

Was this included in the 2.0.12543 release?

@mathewc
Copy link
Member

mathewc commented Jun 21, 2019

Yes

@cwoolum
Copy link

cwoolum commented Jun 22, 2019

Hey @ahmelsayed, should these kubernetes keys also work for authorizing a specific function call? I'm getting a 401 no matter how I try to configure this. Here is what I have:

I've configured a secret basically based on the examplke above

apiVersion: v1
kind: Secret
metadata:
  name: function-keys
type: Opaque
data:
  host.master: TXlTdXBlclNlY3JldEtleQ==         #base64 encoded version of 'MySuperSecretKey'

I know that my my pod is able to access this secret because my host api calls work and the repository is able to generate a new code for my specific function.

Using the api to get the key for the function works correctly with the host.master key I have defined in the secret. If I send a get request to https://my-api-url/admin/functions/UpdateUserLocation/keys?code=MySuperSecretKey, I get a response of

{
    "keys": [
        {
            "name": "default",
            "value": "1eHaHrL3Wc1nTyAGSXDqMTUACLEBtal5fmxtAaGnl5d3o84IhqvMTA=="
        }
    ],
    "links": [
        {
            "rel": "self",
            "href": "https://my-api-url/admin/functions/UpdateUserLocation/keys"
        }
    ]
}

I looked in the secret in kubernetes and the value above does match. Next I try to make a call to my function using https://my-api-url/runtime/webhooks/EventGrid?functionName=UpdateUserLocation&code=1eHaHrL3Wc1nTyAGSXDqMTUACLEBtal5fmxtAaGnl5d3o84IhqvMTA== but always get a 401 back. Am I missing something here?

I apologize for adding this comment here instead of an issue but I thought this would be the best place since I was testing this specific functionality.

Thanks for adding this functionality!! I was looking for something like this for a few weeks

@ahmelsayed
Copy link
Contributor Author

I think for eventgrid you need a systemkey (see this)

You'll need to create it first though. You can either add it to your Kubernetes Secret by adding

  host.systemkey.eventgrid_extension: # base64 encoded value

or if you wanna use the Host APIs, then try:

# First create the key using:
# POST https://my-api-url/admin/host/systemkeys/eventgrid_extension?code={masterkey} 
curl -d '' https://my-api-url/admin/host/systemkeys/eventgrid_extension?code={masterkey}

# The create action above will return the key. But you can do a GET on it 

Then try using that key. If it still doesn't work, I'll need to cc someone more familiar with eventgrid than me.

@ahmelsayed
Copy link
Contributor Author

actually I don't think you have to name it eventgrid_extension either. That's probably the default name the portal picks, but I'm not 100% sure. @fabiocav who knows about eventgrid trigger?

@ahmelsayed
Copy link
Contributor Author

Also thank you for giving it a try. I still need to update the core-tools to allow for creating and managing keys for kubernetes deployment, so it's a bit easier to use. But it's great to see someone put the pieces together and use it already :)

@cwoolum
Copy link

cwoolum commented Jun 22, 2019

I was able to create the system key and get a value returned but unfortunately that did not work either.

If I try to make a GET to https://my-api-url/admin/host/systemkeys/eventgrid_extension?code={masterkey}, I just get a 404. Looking in the Kubernetes secret, I do see the key populated as host.systemKey.eventgrid_extension with the value returned from the POST request.

It's also interesting because if I make a GET request to https://my-api-url/admin/host/systemkeys?code=MySuperSecretKey, it returns an empty array.

{
    "keys": [],
    "links": [
        {
            "rel": "self",
            "href": "https://my-api-url/admin/host/systemkeys"
        }
    ]
}

@cwoolum
Copy link

cwoolum commented Jun 22, 2019

Got it to work! There seems to be a small bug after the systemKey is created where even though it is added to the kubernetes secret, the collection of systemKeys in memory is not updated. Killing the pod and restarting it fixes the issue.

@ahmelsayed
Copy link
Contributor Author

Just to double check, are you creating a service account and using a read/write secret or mounting the secret as a volume?

@cwoolum
Copy link

cwoolum commented Jun 22, 2019

I'm using a service account with a read/write secret.

@v1ferrarij
Copy link

Is there anyway to tell the function app to look at another path other than /run/secrets/function-keys?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants