[SPARK-26194][k8s] Auto generate auth secret for k8s apps.

[vanzin]

This change modifies the logic in the SecurityManager to do two
things:

• generate unique app secrets also when k8s is being used
• only store the secret in the user's UGI on YARN

The latter is needed so that k8s won't unnecessarily create
k8s secrets for the UGI credentials when only the auth token
is stored there.

On the k8s side, the secret is propagated to executors using
an environment variable instead. This ensures it works in both
client and cluster mode.

Security doc was updated to mention the feature and clarify that
proper access control in k8s should be enabled for it to be secure.

[SparkQA]

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/5476/

[SparkQA]

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/5476/

[SparkQA]

Test build #99402 has finished for PR 23174 at commit 0e36a4b.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[mccheah]

Would it be possible to also provide support for passing this via a mounted file? Some users would prefer to avoid propagating sensitive information via environment variables. Also the user should be able to specify their own mounted file; spark-submit shouldn't always mount an auto-generated secret for the user.

[vanzin]

via a mounted file
Also the user should be able to specify their own mounted file

The point is that the user shouldn't need to set this at all. You enable auth, Spark takes care of it. There's no point in specifying a pre-defined file with the secret - in fact that makes things less secure, because you'd be reusing the secret in different apps.

Some users would prefer to avoid propagating sensitive information via environment variables

Why? And how are mounted files better?

[mccheah]

Why? And how are mounted files better?

Environment variables leak far more easily than file contents. One can accidentally printenv in a shell attached to the container and get the secret contents. System.getenv has a similar effect within the application code itself. For what it's worth I'm also not sure if the secret would be listed under the environment variables in the Spark UI (would have to test this).

[vanzin]

if the secret would be listed under the environment variables in the Spark UI

Secrets are redacted in the UI and event logs. We already use env variables in other contexts (e.g. standalone with auth enabled).

Environment variables don't leak unless you leak them. If you do, it's a security problem in your code, since the env is generally considered "sensitive information". They're not written to disk, unlike files, which some people have problems with (really paranoid orgs don't want sensitive information in unencrypted files on disk).

This could be stashed in a k8s secret, but then how does the client mode driver get it? More user configuration? That's exactly what this is trying to avoid.

[vanzin]

(In fact, env variables don't even show up in the UI or event logs, as far as I can see. Other configs - Spark config, system properties, e.g. - do show up, and are redacted to mask secrets.)

[gdearment]

The issue with requiring the use of secrets is quite a bit of work must be done in order to secure a cluster to ensure that the secrets are themselves secured. Most of the high level concerns are outlined here, but you then need to understand the implications of this in the context of how Spark is used. Its my understanding that Spark's implementation on Kubernetes doesn’t dictate how spark jobs should be run. As an example of where this can quickly break down:

• Imagine a scenario where someone runs two drivers in the same namespace, and rely on the default svc token to allow the driver to create its executors.
• The driver token will have GET access on the secret for its driver, but also other secrets within the same namespace – such as the secret for driver B.
• If the executors in job B can use the same service token as their driver, then they have the ability of launching executors that can use the secret from job A to connect an executor at which point you have a compromise.

There are lots of things that can be done to prevent this kind of attack – run in different namespaces, don’t allow executors to have k8s permissions, etc., but it feels risky for spark to require the use of secrets while leaving it an exercise for the reader to understand how to properly run spark such that the secrets are actually secured.

At Palantir, we rely heavily on an external secret storage and bootstrapping system, built around Vault, to securely pass secrets into pods. Using external systems for these kinds of things, especially those built around Vault, is a common practice.

So really, what we'd like here is to avoid coupling the API for how spark containers read secrets so that alternative implementations can be used that some might consider more secure.

[vanzin]

while leaving it an exercise for the reader to understand how to properly run spark such that the secrets are actually secured.

I don't think that's an exercise for the user, but for the admin. If the admin has configured things properly, the user will be able to deploy secure applications without issue. This is not just the case in kubernetes. It's the case in any deployment.

Even your example of using Vault requires that. There's work needed to use it properly and securely.

avoid coupling the API for how spark containers read secrets

There doesn't need to be a single solution. This patch going in does not preclude adding more features later, one of which might be reading this from a pre-defined secret.

And in your example I don't see how Vault would actually solve the client-mode driver case. There's no kubernetes involved in the driver, which may be running anywhere, and it needs to know the secret.

[mccheah]

There doesn't need to be a single solution. This patch going in does not preclude adding more features later, one of which might be reading this from a pre-defined secret.

The way it's written now, if the user opts-in to using spark.authenticate with Kubernetes, the application will always automatically generate the secret and use that as the security mechanism. I think we'd prefer to see the various options that are available up front and this patch should probably introduce both the automatic secret creation version (if we agree that this is secure) and the manual provision version. If this change is merged into 3.x without any other changes, users will be forced to use the K8s secret based SASL scheme and this feature will be unusable for other users such as with the vault case discussed above.

[vanzin]

The way it's written now

Code can change after it's written...

If this change is merged into 3.x without any other changes, users will be forced to use the K8s secret based

If this change is not merged, users have no way to use authentication at all. So I don't see your point.

It will prevent you from using the Vault approach in the sense that support for that won't be implemented yet. But this change, again, does not put a block on adding support for what you're saying later. If you think that's important and want to do that in 3.x, then you're free to. But I don't see why this approach needs to be blocked because it doesn't cover the Vault use case.

[mccheah]

The trouble is the API proposed here and how it would have to change for future features. If we wanted to add the optionality to support authentication via mounted files later, then what's the API for that, and how would that change the API for users that were relying on this authentication mechanism? That's why it's important to see the optionality now, so it can be clear to us that are our options, and this is how we are going to use them.

A proposed scheme is to have spark.authenticate.k8s.secret.provider=autok8ssecret, then document what that does. Perhaps that's the default mode. Then add another scheme, say spark.authenticate.k8s.secret.provider=files and then further options for specifying where that file is located on both the driver and the executors.

It's helpful to put this patch in the context of where we want to go for authentication in general moving forward. Otherwise this feature taken in isolation will make it appear that Spark is being opinionated about using Kubernetes secrets and environment variables for authentication.

if it's not introduced in this patch, then at least we should file JIRA tickets and reference them as future add-ons to this and have a roadmap for what SASL on K8s will look like in the bigger picture for 3.x.

[vanzin]

A proposed scheme is to have spark.authenticate.k8s.secret.provider=autok8ssecret

If you're going to add a different way to get the auth secret later, then you can introduce that option with a default value. It does not mean it needs to be done in this change, which is my point.

The only real change being introduced here form the Spark status quo is that you don't need to provide your own auth secret in the configuration (i.e. it would work like YARN), which doesn't even work in k8s today because that is not propagated to the executors in any way. And if you think that is a problem I can gate the generation of the secret based on whether one is already defined in the config.

if it's not introduced in this patch, then at least we should file JIRA tickets

That is fine with me.

[vanzin]

I filed SPARK-26239.

[vanzin]

So, can we move forward with this and let any future new feature be handled in SPARK-26239?

[SparkQA]

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/5687/

[mccheah]

I think as long as we have one alternate mechanism proposed in SPARK-26239 this is ok to merge. I proposed one in this comment.

[vanzin]

As I suggested before, any alternative method can be added later. I don't see why does it need to block this PR.

[mccheah]

It's just to have the assurance that we will have some way to bypass this for auth at least for 3.x. I'd like to concretely determine this before merging if possible. But I hope that the suggestion proposed in SPARK-26239 could be agreed upon fairly quickly?

[vanzin]

I don't understand what you want.

Without this change, auth does not work, period.

With this, users at least have one choice.

If you want to add another choice, you're free to. But I don't see why the lack of another choice has any effect on this PR.

[mccheah]

Ok that's fine, with the caveat that we merge the subsequent optionality soon. I'll work on the file-based secret authentication and encryption this week. I'm very concerned that we'll ship with this but with no other security options if we're not rigorously moving SPARK-26239 forward.

Merging to master in a few hours, letting it stay open for a bit for any other commentary. @gdearment for SA.

[SparkQA]

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/5687/

[vanzin]

with the caveat that we merge the subsequent optionality soon

Again, and sorry for pounding on that key, but why does that matter? It has zero effect on the feature being added here. If the code added here is not good enough for your use case, you're in the exact same situation as if this change did not go in. But for those that can leverage the auth feature as added in this change, they're in a much, much better place.

[mccheah]

It matters because we're discussing direction - that is, what opinion Spark wants to take regarding how to set up security on K8s. It's not obvious from our discussion on SPARK-26239 that we agree that we should allow such optionality for other authentication schemes. In other words, if we just merge this PR without further discussion and consensus on SPARK-26239, we're effectively communicating that Spark is locked in to the authentication backed by K8s secrets. I want to emphasize that it's important to agree on the direction for the bigger picture early on, and then we say that this patch still fits into the bigger vision.

I also want to intend to take this patch and check that work on SPARK-26239 would work nicely with it, but to the best of my knowledge the additional options should layer on top of this default one just fine. Would like some concrete prototyping to confirm this though.

[vanzin]

It matters because we're discussing direction

I'm not, you guys are. I'm adding a missing feature with one particular implementation. If you want to add other implementations that enable different use cases, great.

we're effectively communicating that Spark is locked in to the authentication backed by K8s secrets

We're not locking into anything, and that's basically where I strongly disagree with you. You're free to add new ways, and when that's done, you're not "locked in" anymore.

Locked in would mean that pushing this PR means you cannot make changes to it later, and that's just not true.

Right now you're "locked in" to no auth at all, but somehow that's ok?

check that work on SPARK-26239 would work nicely with it

Anything needed to implement that feature is just code changes. Whether it "works nicely" is just a matter of not breaking this when that feature is implemented.

[mccheah]

Ok that's fine. Will merge to master if there are no further comments in the near future.

[vanzin]

I looked at the test failure, but the logs weren't super useful. This passed locally, but let me retrigger here.

[vanzin]

retest this please

[SparkQA]

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/5691/

[SparkQA]

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/5691/

[SparkQA]

Test build #99629 has finished for PR 23174 at commit 791b5ee.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #99634 has finished for PR 23174 at commit 791b5ee.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[mccheah]

Ah I thought about this a bit more and realized that this is more insecure than I originally read it to be.

If the secret is put directly in the environment variable field itself, then anyone who has permission to get the pod metadata from the Kubernetes API server can now read the secret generated by this application. In practice permissioning on pod specs is often far looser than permissioning on Kubernetes secret objects. (Edit: For example a system administrator who performs maintenance and debugs issues should be able to read the pod spec object but often can't access sensitive information in Kubernetes secrets in the same namespace.) In this solution the administrator has to restrict access to pod specs to only the user.

I think at the very least we want this to be configured via creating a Kubernetes secret object, then loading the environment variable to point to the secret object.

In the meantime I'm going to push the PR that allows secrets to be specified as file paths directly. I will also file a Spark ticket to avoid putting the environment variable directly in the pod spec object itself.

[mccheah]

I filed https://issues.apache.org/jira/browse/SPARK-26301 to suggest the alternative scheme. Unlike SPARK-26139 this would change the functionality that was merged here.

[vanzin]

If the secret is put directly in the environment variable field itself, then anyone who has permission to get the pod metadata from the Kubernetes API server can now read the secret generated by this application.

Yes, and it's extremely annoying that k8s allows anybody with access to the pods to read env variables, instead of just the pod owner. In fact, it doesn't even seem to have the concept of who owns the pod.

Anyway, this isn't different from someone else being able to read secrets in the same namespace as the pod.

As I said before, it all depends on how you configure your cluster for security, and in k8s there seems to be a lot of different options.

[mccheah]

Anyway, this isn't different from someone else being able to read secrets in the same namespace as the pod.

It isn't in theory, but in practice my understanding is that secrets are often permissioned more strictly than pod objects in the cluster. We should be optimizing for the more common use case, which will work out of the box for more users and also is more secure in the context of more common configurations.

[vanzin]

the more common use case,

Which is?

There's a lot to think about when you give permissions like "users can view, create and delete pods". If you do that, for example, you can delete other people's pods. That is also considered a security issue, since you can DoS other users.

Anyway, my point is that we should give people the choice of how they deploy things, and set up security according to their own constraints. This was just one way of doing it, and was not meant to be the only way.