Set Github token through workspace master Rest API. Added a force act… #4444

sunix · 2017-03-16T12:24:33Z

Set Github token through workspace master Rest API. Added a force activation property variable to register Github Oauth provider even without client id/secret

Signed-off-by: Sun Seng David Tan sutan@redhat.com

What does this PR do?

add a new REST operation to set a Oauth token to an existing provider
add a property che.oauth.github.forceactivation to force registration of Github Oauth provider, even without client id/secret (actually set NULL string for these value)

What issues does this PR fix or reference?

https://issues.jboss.org/browse/CHE-151

Changelog

Set Github token through workspace master Rest API. Added a force activation property variable to register Github Oauth provider even without client id/secret

Release Notes

Set Github token through workspace master Rest API. Added a force activation property variable to register Github Oauth provider even without client id/secret

Docs PR

…ivation property variable to register Github Oauth provider even without client id/secret Signed-off-by: Sun Seng David Tan <sutan@redhat.com>

skabashnyuk

Conceptually -1
Explained opinion here #4438

garagatyi · 2017-03-16T12:42:13Z

...bly-wsagent-war/src/main/webapp/WEB-INF/classes/codenvy/che-machine-configuration.properties

@@ -50,6 +50,8 @@ oauth.github.authuri= https://github.com/login/oauth/authorize
 oauth.github.tokenuri= https://github.com/login/oauth/access_token
 #redirected uris
 oauth.github.redirecturis= http://localhost:${SERVER_PORT}/che/api/oauth/callback
+# register github even without client id and secret
+oauth.github.forceactivation=false


I don't see the place in code that uses this property. Can you point to it?

right, will remove it

garagatyi · 2017-03-16T12:45:22Z

assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/codenvy/che.properties

@@ -142,7 +142,8 @@ che.oauth.github.clientsecret=NULL
 che.oauth.github.authuri= https://github.com/login/oauth/authorize
 che.oauth.github.tokenuri= https://github.com/login/oauth/access_token
 che.oauth.github.redirecturis= http://localhost:${SERVER_PORT}/wsmaster/api/oauth/callback
-
+# register github even without client id and secret
+che.oauth.github.forceactivation=false


Can you elaborate on what will happen if token is not injected but this property is set to true and client secret/id are unset.
What will happen on clicking OAuth button in IDE?
What will happen on an operation with github?

These video shows an attempt to perform a git operation or github operation (would be similar to clicking in OAuth button):

attempt with default che.oauth.github.forceactivation false

attempt with che.oauth.github.forceactivation true without setToken

attempt with che.oauth.github.forceactivation true and setting the token

Github operation (upload ssh key - old demo, but will show what happen) https://youtu.be/y9cux67RQHY It shows a 404 in a pop in one case + notification error of Oauth failure

Git operation (push) https://youtu.be/5D6XZlDyPKE . It shows a notification error

garagatyi · 2017-03-16T12:46:05Z

...gin-github-oauth2/src/main/java/org/eclipse/che/security/oauth/GitHubOAuthAuthenticator.java

-            && !isNullOrEmpty(clientSecret)
-            && !isNullOrEmpty(authUri)
-            && !isNullOrEmpty(tokenUri)
+    public GitHubOAuthAuthenticator(@Nullable @Named("che.oauth.github.clientid") String clientId, //


It is uncommon for existing codebase to use trailing slashes, can you elaborate on why they are needed?

I use the default formatter in Che. So these were to keep the current formatting

garagatyi · 2017-03-16T12:47:30Z

...gin-github-oauth2/src/main/java/org/eclipse/che/security/oauth/GitHubOAuthAuthenticator.java

+            return;
+        }
+
+        if (forceActivation != null && forceActivation) {


If you make forceActivation primitive then nullness check can be avoided.

Can it be a default null value (so it's false by default) ?

garagatyi · 2017-03-16T12:47:40Z

...gin-github-oauth2/src/main/java/org/eclipse/che/security/oauth/GitHubOAuthAuthenticator.java

+
+        if (forceActivation != null && forceActivation) {
+            configure("NULL", "NULL", redirectUris, authUri, tokenUri, new MemoryDataStoreFactory());
+            return;


Return is not needed here

It's for readability. But I can remove it ...

garagatyi · 2017-03-16T12:48:55Z

...gin-github-oauth2/src/main/java/org/eclipse/che/security/oauth/GitHubOAuthAuthenticator.java

+        }
+
+        if (forceActivation != null && forceActivation) {
+            configure("NULL", "NULL", redirectUris, authUri, tokenUri, new MemoryDataStoreFactory());


If client id/secret are not configured how other params (redirectUris, etc) can be used? Should they be unset too?

garagatyi · 2017-03-16T13:00:14Z

...e-core-api-auth/src/main/java/org/eclipse/che/security/oauth/OAuthAuthenticationService.java

+    @POST
+    @Path("token")
+    @Consumes(MediaType.APPLICATION_JSON)
+    public void setToken(@Required @QueryParam("oauth_provider") String oauthProvider, //


Looks like one of map's entry can growth infinitely because of this API, and it is not restricted so any user of the product can cause OOM

It means that EnvironmentContext.getCurrent().getSubject().getUserId(); can return any userId ? or be dynamically changed ? are you sure of that ?
If you are sure of that, so getToken shouldn't be available because anyone could still others' token ...

codenvy-ci · 2017-03-16T15:09:53Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/2194/

skabashnyuk · 2017-03-16T15:17:43Z

@sunix are you 100% sure that there is no way(even theoretical) to hack the system like Che or Codenvy with this method? How confident you are?

@POST @Path("token"

@l0rd @benoitf wdyt? Is this only my paranoia :)?

sunix · 2017-03-16T15:41:25Z

@skabashnyuk I think that User's data and access to other system is more important than anything.
But if I were a hacker, I would try to get the token and have access to the users Github ( or whatever) and steal informations or change rights if I can. Setting the token ... ? I really don't see how this can be used.

In my opinion, setting the token and use another system to do the oauth stuff is actually more secured than letting Codenvy/Che doing the whole thing: the system responsible for oauth would focus on security ... where as we (Che) want to focus our effort on providing the best development tool.

skabashnyuk · 2017-03-16T15:47:23Z

Can you comment
https://oauth.net/articles/authentication/
Injection of access tokens
An additional (and very dangerous) threat occurs when clients accept access tokens from sources other than the return call from the token endpoint. This can occur for a client that uses the implicit flow (where the token is passed directly as a parameter in the URL hash) and don't properly use the OAuth state parameter. This issue can also occur if different parts of an application pass the access token between components in order to "share" access among them. This is problematic because it opens up a place for access tokens to potentially be injected into an application by an outside party (and potentially leak outside of the application). If the client application does not validate the access token through some mechanism, it has no way of differentiating between a valid token and an attack token.

This can be mitigated by using the authorization code flow and only accepting tokens directly from the authorization server's token enpdoint, and by using a state value that is unguessable by an attacker.

Are you sure it's not related to what you are proposing?

sunix · 2017-03-16T17:56:40Z

Yes OK. Thanks for this. The plan is: we will add validation of token.
@skabashnyuk how does it sounds ? or you still think we shouldn't add a setToken at all ?

…rce activation property variable to register Github Oauth provider even without client id/secret Signed-off-by: Sun Seng David Tan <sutan@redhat.com>

codenvy-ci · 2017-03-17T12:34:58Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/2205/

TylerJewell · 2017-04-17T14:44:42Z

@sunix - I think your plan sounds appropriate. What is the plan to revise and then get this merged?

vparfonov · 2017-07-12T22:26:59Z

@sunix @skabashnyuk any update here, look like not actual for now?

sunix · 2017-07-17T08:13:04Z

not needed anymore thanks

Set Github token through workspace master Rest API. Added a force act…

6244cbe

…ivation property variable to register Github Oauth provider even without client id/secret Signed-off-by: Sun Seng David Tan <sutan@redhat.com>

sunix mentioned this pull request Mar 16, 2017

Set Github token through workspace master Rest API. Added a force act… #4438

Merged

skabashnyuk requested changes Mar 16, 2017

View reviewed changes

garagatyi reviewed Mar 16, 2017

View reviewed changes

skabashnyuk requested a review from benoitf March 16, 2017 15:20

fixup! Set Github token through workspace master Rest API. Added a fo…

b8cea72

…rce activation property variable to register Github Oauth provider even without client id/secret Signed-off-by: Sun Seng David Tan <sutan@redhat.com>

This was referenced Jun 7, 2017

CHE-5169: Add Git configuration agent #5285

Merged

CHE-272: Making 'AuthorizationCodeFlow' field from OAuthAuthenticator protected #5289

Merged

sunix closed this Jul 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set Github token through workspace master Rest API. Added a force act… #4444

Set Github token through workspace master Rest API. Added a force act… #4444

sunix commented Mar 16, 2017

skabashnyuk left a comment

garagatyi Mar 16, 2017

sunix Mar 16, 2017

garagatyi Mar 16, 2017

sunix Mar 16, 2017

garagatyi Mar 16, 2017

sunix Mar 16, 2017

garagatyi Mar 16, 2017

sunix Mar 16, 2017 •

edited

garagatyi Mar 16, 2017

sunix Mar 16, 2017

garagatyi Mar 16, 2017

garagatyi Mar 16, 2017

sunix Mar 16, 2017 •

edited

codenvy-ci commented Mar 16, 2017

skabashnyuk commented Mar 16, 2017

sunix commented Mar 16, 2017 •

edited

skabashnyuk commented Mar 16, 2017

sunix commented Mar 16, 2017 •

edited

codenvy-ci commented Mar 17, 2017

TylerJewell commented Apr 17, 2017

vparfonov commented Jul 12, 2017

sunix commented Jul 17, 2017

Set Github token through workspace master Rest API. Added a force act… #4444

Set Github token through workspace master Rest API. Added a force act… #4444

Conversation

sunix commented Mar 16, 2017

What does this PR do?

What issues does this PR fix or reference?

Changelog

Release Notes

Docs PR

skabashnyuk 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

sunix Mar 16, 2017 • edited

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

sunix Mar 16, 2017 • edited

Choose a reason for hiding this comment

codenvy-ci commented Mar 16, 2017

skabashnyuk commented Mar 16, 2017

sunix commented Mar 16, 2017 • edited

skabashnyuk commented Mar 16, 2017

sunix commented Mar 16, 2017 • edited

codenvy-ci commented Mar 17, 2017

TylerJewell commented Apr 17, 2017

vparfonov commented Jul 12, 2017

sunix commented Jul 17, 2017

sunix Mar 16, 2017 •

edited

sunix Mar 16, 2017 •

edited

sunix commented Mar 16, 2017 •

edited

sunix commented Mar 16, 2017 •

edited