Added security to the API server

[blublinsky]

Why are these changes needed?

Right now API server access is completely opened. This defines the approach to secure API server and provides a very simple implementation.

Securing API server endpoint

Related issue number

Closes #1376

Checks

• [ x] I've made sure the tests are passing.
• Testing Strategy
  • [ x] Unit tests
  • [ x] Manual tests
  • This PR is not tested :(

[blublinsky]

@astefanutti , @z103cb , please take a look

cc: @kevin85421 , @architkulkarni

[z103cb]

I believe that we should be covering the newly added code with some unit tests.

[blublinsky]

I believe that we should be covering the newly added code with some unit tests.

These are available in this project https://github.com/blublinsky/auth-reverse-proxy

[blublinsky]

@tedhtchang can you take a look

[tedhtchang]

@blublinsky The instruction worked w/ some changes. In addition, is it possible to have helm optionally deployed the sidecar with --set security.enable=true ?

[blublinsky]

@kevin85421 I think its ready for you

[astefanutti]

LGTM overall, once we find an appropriate place to land the reverse proxy parts (in the security directory and the associated workflows) instead to have them in-tree.

@kevin85421 we'd like to have your advice / guidance where you think this could be maintained. Could this be in a dedicated repository in the ray-project organisation? I could also propose we create a ray-contrib organisation, if the former is not an option, to hosts incubating or extension projects.

[kevin85421]

@kevin85421 we'd like to have your advice / guidance where you think this could be maintained. Could this be in a dedicated repository in the ray-project organisation? I could also propose we create a ray-contrib organisation, if the former is not an option, to hosts incubating or extension projects.

cc @architkulkarni do you have any suggestions?

[architkulkarni]

I don't have any special insight here but I can check with the Ray maintainers. What's the main advantage of having the security directory in a separate repository? Sorry if I missed some of the previous discussion around this.

[blublinsky]

@kevin85421 @architkulkarni The reason for the question is that security directory contains a very simple implementation, that is easily breakable and as such should never be used in production. The point of PR is to show how the security fits in the overall API server implementation. So provided implementation is really for people to experiment with and then roll out their own implementation. We discussed that on one hand we wanted to give people something to try security with but wanted to make sure that they understand, that this is a very simple "toy" example.

[architkulkarni]

Gotcha! That makes sense, but couldn't we convey that by putting it in an experimental/ folder and adding a prominent warning in a code comment or readme?

[blublinsky]

This will work for me. We were looking for your suggestion on the matter

[astefanutti]

@kevin85421 @architkulkarni @blublinsky besides the maturity level of this reverse-proxy component, it's also that it's completely independent from KubeRay. Even if it would become mature enough (which is not the goal as @blublinsky stressed), it would still be as if a project like oauth2-proxy would be pull in-tree into KubeRay.

That being said, that was more to raise awareness so you guys can decide, and if you don't have any objections to having it in-tree, moving that component into an experimental folder sounds like a good compromise as @architkulkarni suggested, and this can still be revisited in the future anyway.

[z103cb]

@kevin85421 @architkulkarni @blublinsky besides the maturity level of this reverse-proxy component, it's also that it's completely independent from KubeRay. Even if it would become mature enough (which is not the goal as @blublinsky stressed), it would still be as if a project like oauth2-proxy would be pull in-tree into KubeRay.

That being said, that was more to raise awareness so you guys can decide, and if you don't have any objections to having it in-tree, moving that component into an experimental folder sounds like a good compromise as @architkulkarni suggested, and this can still be revisited in the future anyway.

I agree with @astefanutti's comments. Using the experimental folder name is slightly better than using the security folder name. I will be fine with your decision @architkulkarni and @kevin85421.

[blublinsky]

@kevin85421 @architkulkarni, please let me know final decision and I will update the PR, which is otherwise ready to merge

[architkulkarni]

Thanks for the clarification! We're okay with keeping it in tree, so let's go with the experimental folder for now. Once that's done, we can merge the PR.

[blublinsky]

@kevin85421 @architkulkarni, you can merge it now

[architkulkarni]

Stamp