Added an ability to authenticate requests to servers with jwtproxy #10252

sleshchenko · 2018-07-03T12:18:02Z

What does this PR do?

In this PR secure servers are introduced. If server supposed to be secure it should contain secure: true attribute, for example ws-agent server configuration:

    "wsagent/http": {
      "port": "4401/tcp",
      "protocol": "http",
      "path" : "/api",
      "attributes": {
        "secure": "true"
      }
    }

It is needed to make servers authentication independent.

Then infrastructure should cover server with authentication and server will receive only authenticated requests. Requests to secure servers must contain machine token in cookies, Authorization header or token query parameter.

This PR contains secure servers supporting of Kubernetes and OpenShift infrastructures.

For some time, it is not enabled by default, so secure servers still should authenticate requests themselves. It may be enabled by setting che.agents.auth.secure_server_exposer=jwtproxy.

Kubernetes and OpenShift infrastructures implentation notes

It is done with JWTProxy, that is add in each Workspace as separated Pod. Original servers endpoints won't be accessible externally, instead JWTProxy port will be exposed and authenticated requests will be proxied to original server.
The same components works for Kubernetes and OpenShift infrasturcture since they reuse ExternalExposeServerStrategy (for K8s - ingress based, for OS - route based) for exposing JWTProxy service's ports.

What issues does this PR fix or reference?

#10081

Docs PR

I think Docs should be added when this feature won't be beta

skabashnyuk · 2018-07-05T10:52:37Z

...java/org/eclipse/che/workspace/infrastructure/kubernetes/server/KubernetesServerExposer.java

            internalServers.put(key, value);
          } else {
-            externalServers.put(key, value);
+            // Server is external. Check if it should be secure or not
+            if ("true".equals(value.getAttributes().get(ServerConfig.SECURE_SERVER_ATTRIBUTE))) {


Boolean.parseBoolean(value.getAttributes().get(ServerConfig.SECURE_SERVER_ATTRIBUTE)) ?

Thanks for pointing. And it was implemented in this way for internal servers. I think it is done in this way because comparing with true obviously shows that only this value is suitable here. Also, it is documented here that only true will be considered as right value for internal/secure servers. While parseBoolean checks value with equalsIgnoreCase. I'd leave it as it is, at least for now.

sleshchenko · 2018-07-09T07:54:44Z

@garagatyi @mshaposhnik @skabashnyuk Now PR is ready to review, please take a look.

sleshchenko · 2018-07-10T06:55:46Z

ci-test

codenvy-ci · 2018-07-10T07:03:19Z

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:10252
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

sleshchenko · 2018-07-10T07:24:54Z

ci-test

codenvy-ci · 2018-07-10T12:37:24Z

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:10252
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

garagatyi · 2018-07-10T15:24:35Z

Is this PR working or just a part of working new security system?

garagatyi

Overall looks good! There are inlined comments about the things I would love to understand more/address in the PR to provide a meaningful review.

garagatyi · 2018-07-10T13:53:29Z

...clipse/che/workspace/infrastructure/kubernetes/server/secure/SecureServerExposerFactory.java

+
+  @Inject
+  public SecureServerExposerFactory(
+      @Named("che.agents.auth_enabled") boolean agentsAuthEnabled,


Can you elaborate why do we use agents notion here instead of servers?

Thank, Good point! It makes more sense to use servers word there instead of agents. I will change it.

@garagatyi Do you think it may be che.server.secure_exposer property which is Che System scope or it would be better to make it K8s/OS scope like che.infra.kubernetes.server_secure_exposer?

interesting question) On one hand it would be more complex to configure Che if we had several properties for several infras. On the other hand, we don't have anything like this for the Docker infra.
I think it is not so critical, and I would opt for the 1st option.

OK, I will rename it to che.server.secure_exposer and we'll rename it to infraspecific if it will be needed to have different authentication proxy backends for different infrastructures.

I thought you are talking only about che.agents.secure_server_exposer property.
As about che.agents.auth_enabled it is not new but an existing one, it is used configuration CHE_AUTH_ENABLED environment variable that is injected into each machine. I used it here as a flag if authentication is enabled or disabled, is Che launched in single user mode or multiuser. Are you OK to use it here, or do you think it makes sense to introduce new property?

I'm OK to leave the old name. I missed that it is old.

che.agents.auth_enabled leaved as it is and che.agents.secure_server_exposer is renamed to che.server.secure_exposer

garagatyi · 2018-07-10T13:55:43Z

...clipse/che/workspace/infrastructure/kubernetes/server/secure/SecureServerExposerFactory.java

+    }
+
+    switch (serverExposer) {
+      case "jwtproxy":


With this approach, this class has to know all the implementations. maybe it worth to rework the code in a way it is possible to bind other auth implementations?

Makes sense, I'll rework it with Map bindings.

Ouch. I missed that JwtProxySecureServerExposer is created with factory that uses assisted inject. So, SecureServerExposerFactory could inject something like Map<String, SecureServerExposerImplFactory> where SecureServerExposerImplFactory is a custom interface

interface SecureServerExposerImplFactory { SecureServerExpose create(RuntimeIdentity identity); }

And then there should be corresponding SecureServerExposerImplFactory implementaion of each secure server (now JwtProxy and Default).

Looks like a bit overkill for now.

Do you think I should rework it or it is OK to leave it as it is for now?

Maybe we can use a single interface of a factory of a factory instead of the map) This would allow using another implementation instead of JWT one (it can be even customized version of JWT one, depending on the ISV needs).
But yes, if there is no elegant and easy to implement and support way or if it will delay merging of the PR we can totally return to this issue when there would be a need for that.
And it looks like we definitely need to leave the approach as it is implemented now.

Reworked in the suggested way.

garagatyi · 2018-07-10T13:56:35Z

...clipse/che/workspace/infrastructure/kubernetes/server/secure/SecureServerExposerFactory.java

+import org.slf4j.LoggerFactory;
+
+/** @author Sergii Leshchenko */
+public class SecureServerExposerFactory<T extends KubernetesEnvironment> {


Consider adding unit tests and javadocs

garagatyi · 2018-07-10T14:00:19Z

...se/che/workspace/infrastructure/kubernetes/server/secure/jwtproxy/JwtProxyConfigBuilder.java

+    for (VerifierProxy verifierProxy : verifierProxies) {
+      configBuilder.append(
+          String.format(
+              "  - listen_addr: :%s\n" // :4471


I think that the usage of YAML serialization would improve readability and supportability of this code.

I've looked at this approach but it did not make code easier since I need to create 4 or even more additionally classes. I'll try again, maybe it would be better now =)

garagatyi · 2018-07-10T15:19:40Z

...se/che/workspace/infrastructure/kubernetes/server/secure/jwtproxy/JwtProxyConfigBuilder.java

+ *
+ * @author Sergii Leshchenko
+ */
+public class JwtProxyConfigBuilder {


This doesn't seem related to k8s. Maybe it worth to move it to another module?

Good point, but I have no idea which module will be good to place for this class. I would leave it here while it is used by Kubernetes infrastructure. And move it to a new separated module or existing one when there will be a case where JwtProxyConfigBuilder will be used in another module, like Docker infrastructure. Is it OK for you?

garagatyi · 2018-07-10T15:26:23Z

...ipse/che/workspace/infrastructure/kubernetes/server/secure/jwtproxy/JwtProxyProvisioner.java

+import org.eclipse.che.workspace.infrastructure.kubernetes.server.ServerServiceBuilder;
+
+/** @author Sergii Leshchenko */
+public class JwtProxyProvisioner {


Please, add javadocs

garagatyi · 2018-07-10T15:29:05Z

.../workspace/infrastructure/kubernetes/server/secure/jwtproxy/JwtProxySecureServerExposer.java

+import org.eclipse.che.workspace.infrastructure.kubernetes.server.secure.SecureServerExposer;
+
+/**
+ * Expose secure servers with JWTProxy.


Can you add more details into the javadocs? It would be helpful to understand what the class does and what for.

garagatyi · 2018-07-10T15:51:29Z

...ipse/che/workspace/infrastructure/kubernetes/server/secure/jwtproxy/JwtProxyProvisioner.java

+      throws InfrastructureException {
+    ensureJwtProxyInjected(k8sEnv);
+
+    int listenPort = availablePort++;


Didn't get some use-cases. Would you be willing to explain them to me?

What if 2 workspace masters are running at the same time?

What if workspaces get deployed to the same single k8s namespace?

Is there any guarantee that this class won't be created multiple times since it is not a singleton and is not created automatically by DI container?

What if 2 workspace masters are running at the same time?

There is no issue with that because of a restriction that only 1 Che Workspace Master is able to start a particular workspace at the same time.

What if workspaces get deployed to the same single k8s namespace?

There are no any issues with that while it is a port of a jwtproxy pod for a particular workspace. Jwtproxy pod will have a unique name within k8s namespace that is set by UniqueNamesProvisioner class.

Is there any guarantee that this class won't be created multiple times since it is not a singleton and is not created automatically by DI container?

Actually, this class MUST BE created once for one STARTING workspace, I don't see any possible ways in which we can guarantee that class is used correctly. What we can do - describe in java docs how it should be used.

One more thing that we could implement to prevent conflicts while incorrect usage of this class - use unique objects name - like machine name, pod name, etc. Then if this class will be created two or more times then the only thing that happens - there are more jwtproxy pods that are needed but not ports, objects conflicts should be there and everything would work as expected.
But I think it may be a bit overkill while this class is supposed to use in one place.

One more note, about unique names - do you think should we use a unique machine name instead of hardcoded? Now we always use jwtproxy machine name. It would be better to use generated name with prefix like jwtproxy9sjXnw6y to avoid conflicts when user have jwtproxy name?

ok, I got it. Yeah, the current solution is good enough, overcomplicating it is not needed. Thanks for the clarification!

sleshchenko · 2018-07-11T14:23:21Z

Is this PR working or just a part of working new security system?

Actually, it requires a few more things to be working:

excludes will be implemented soon in jwtproxy otherwise liveness probes fail because of token leak. For testing the following workaround was used fe8ff5f
now jwtproxy requires some claims to be present in JWT while they are documented as optional in a specification. So, jwt proxy will be improved not to require optional claims and some of claims will be added to machine token soon. Temporary commit for testing is here 8f7c975

So, when these two steps will be merged, authentication secure servers with JWT proxy will be working in master branch (but disabled by default).

garagatyi · 2018-07-11T14:26:07Z

Then I think we should not have release notes in this PR as it would not get into the product as something users can use.

garagatyi

Looks good!

sleshchenko · 2018-07-11T14:29:01Z

Then I think we should not have release notes in this PR as it would not get into the product as something users can use.

It makes sense. I'll remove release notes section

sleshchenko · 2018-07-11T15:39:48Z

ci-test

riuvshin · 2018-07-11T20:58:39Z

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:10252
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

sleshchenko · 2018-07-12T05:16:10Z

@eclipse/eclipse-che-qa Could you please review test report?

Ohrimenko1988 · 2018-07-12T07:55:01Z

@sleshchenko In the functionality which is covered by selenium tests - new regression was not found.

sleshchenko · 2018-07-12T08:47:14Z

During manual testing on minikube found the following error:

I'll prepare the corresponding changes to reduce service port name.

It allows infrastructure to save configuration into ConfigMap and mount it as files to Pods. JwtProxy config will be mounted to container in this way

sleshchenko added kind/enhancement A feature request - must adhere to the feature request template. status/in-progress This issue has been taken by an engineer and is under active development. labels Jul 3, 2018

sleshchenko self-assigned this Jul 3, 2018

sleshchenko requested review from skabashnyuk, mshaposhnik and garagatyi July 3, 2018 12:18

sleshchenko requested review from riuvshin and tolusha as code owners July 3, 2018 12:18

sleshchenko force-pushed the jwtproxy-refactor branch 2 times, most recently from 8dcb57e to 2dc14d3 Compare July 4, 2018 11:21

sleshchenko mentioned this pull request Jul 4, 2018

Refactored external servers exposition in K8s/OS infrastructures #10266

Merged

sleshchenko force-pushed the jwtproxy-refactor branch 2 times, most recently from 113344a to b9ef2ef Compare July 5, 2018 08:52

skabashnyuk approved these changes Jul 5, 2018

View reviewed changes

sleshchenko force-pushed the jwtproxy-refactor branch from b9ef2ef to a7ccfcb Compare July 5, 2018 14:40

sleshchenko requested a review from a user July 5, 2018 14:40

sleshchenko requested a review from l0rd as a code owner July 5, 2018 14:40

ghost approved these changes Jul 5, 2018

View reviewed changes

sleshchenko force-pushed the jwtproxy-refactor branch 2 times, most recently from 7d5cf5d to 9bd42b4 Compare July 6, 2018 07:41

sleshchenko changed the title ~~[WIP] Added an ability to authenticate requests to servers with jwtproxy~~ Added an ability to authenticate requests to servers with jwtproxy Jul 6, 2018

sleshchenko added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. and removed status/in-progress This issue has been taken by an engineer and is under active development. labels Jul 6, 2018

sleshchenko force-pushed the jwtproxy-refactor branch 2 times, most recently from e7f237e to a7438c4 Compare July 9, 2018 07:43

sleshchenko force-pushed the jwtproxy-refactor branch 2 times, most recently from 4b53f31 to ab14de5 Compare July 10, 2018 06:48

sleshchenko force-pushed the jwtproxy-refactor branch from ab14de5 to 11a7d39 Compare July 10, 2018 07:24

garagatyi reviewed Jul 10, 2018

View reviewed changes

garagatyi approved these changes Jul 11, 2018

View reviewed changes

sleshchenko force-pushed the jwtproxy-refactor branch from 5a40729 to b8e01f9 Compare July 11, 2018 15:39

sleshchenko added status/pending-merge and removed status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. labels Jul 12, 2018

vkuznyetsov mentioned this pull request Jul 12, 2018

QE-2018-07-31 (Sprint: 152) #10311

Closed

39 tasks

sleshchenko added 4 commits July 12, 2018 14:50

Add ConfigMaps into K8s/OS environments

3ad17d6

It allows infrastructure to save configuration into ConfigMap and mount it as files to Pods. JwtProxy config will be mounted to container in this way

Introduce secure servers and add SecureServerExposer

efaef7e

Mark exec-agent, terminal, wsmaster servers secure

e142d37

Add JWTProxy based implementation for SecureServerExposer

d6a913b

sleshchenko force-pushed the jwtproxy-refactor branch from b8e01f9 to d6a913b Compare July 12, 2018 11:51

sleshchenko merged commit cb562b9 into eclipse-che:master Jul 12, 2018

sleshchenko deleted the jwtproxy-refactor branch July 12, 2018 11:52

benoitf added this to the 6.9.0 milestone Jul 12, 2018

sonntag-dev mentioned this pull request Mar 31, 2020

Question: "No JWT found" for tomcat service in workspace #16503

Closed

Added an ability to authenticate requests to servers with jwtproxy #10252

Added an ability to authenticate requests to servers with jwtproxy #10252

Conversation

sleshchenko commented Jul 3, 2018 • edited Loading

What does this PR do?

Kubernetes and OpenShift infrastructures implentation notes

What issues does this PR fix or reference?

Docs PR

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sleshchenko commented Jul 9, 2018

sleshchenko commented Jul 10, 2018

codenvy-ci commented Jul 10, 2018

sleshchenko commented Jul 10, 2018

codenvy-ci commented Jul 10, 2018

garagatyi commented Jul 10, 2018

garagatyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sleshchenko commented Jul 11, 2018

garagatyi commented Jul 11, 2018

garagatyi left a comment

Choose a reason for hiding this comment

sleshchenko commented Jul 11, 2018

sleshchenko commented Jul 11, 2018

riuvshin commented Jul 11, 2018

sleshchenko commented Jul 12, 2018

Ohrimenko1988 commented Jul 12, 2018

sleshchenko commented Jul 12, 2018

sleshchenko commented Jul 3, 2018 •

edited

Loading