Git http authentication #216

dennari · 2017-06-07T19:10:52Z

Description

Add a proxy and authentication scheme for git http connections. Allow charles to accept git http requests with Auth0 credentials, authenticate against Auth0, rewrite the authorization header and pass forward to GitLab.

The GitLab account passwords are derived from the usernames with HMAC-SHA1, requiring a secret key. The key can be set with the environment variable GITLAB_PASSWORD_SECRET.

This PR also includes a new admin endpoint GET /operations/regenerate-gitlab-passwords. It lets you set the passwords for all the users in GitLab to the derived ones. This is needed for instance if the password secret changes.

Testing locally

First you need to reset the passwords. I suggest doing this by temporarily changing line https://github.com/lucified/minard-backend/blob/git-http-authentication/src/operations/operations-hapi-plugin.ts#L58 to

      config: {...config, auth: false },

restarting minard-backend and requesting

curl -v localtest.me:8000/operations/regenerate-gitlab-passwords

The integration test configuration files will also need to be updated by removing all the gitPassword definitions. After these steps the integration tests should pass.

TODO

merge Move to Node 8 / npm 5, update packages and cleanup #215 and change the base to master
ensure the git requests are routed correctly in lucify-infra

vsaarinen

Here are some initial comments. Haven't had a chance to test it yet.

If it's ok, I want to hand over the review to @juhoojala.

vsaarinen · 2017-06-09T13:23:15Z

src/authentication/git-auth-scheme.ts

+}
+
+export class GitAuthScheme {
+  private readonly _getSigningKey: (arg1: string) => Promise<jwksRsa.Key>;


Instead of having these underscore functions, I'd prefer it if the name describes what it exactly is. The underscore gives no indication into why the underscore is there or why it's different from a non-underscore version.

(This comment applies generally.)

I think that if there are clearly two versions of a function or a method, let's say a private and a public one, it's not worth the effort to think of two different names. That would also make it less clear if the two methods are related or not. We could come up with another convention, but this is widely used.

Ok, I guess it's fine for private/public differentiation.

vsaarinen · 2017-06-09T13:28:36Z

src/authentication/git-auth-scheme.ts

+        scope: 'openid email',
+      });
+    }
+    return this.clientLogin(username, password);


When is userLogin used and when is clientLogin used?

Added comments to code. The difference is between signed-up regular users and non-interactive clients.

vsaarinen · 2017-06-09T13:40:15Z

src/authentication/git-auth-scheme.ts

+import { sanitizeSubClaim } from './authentication-hapi-plugin';
+import { AccessToken } from './types';
+
+interface Auth0LoginResponse {


Shouldn't this be Auth0DecodedHash from the 'auth0-js' package?

vsaarinen · 2017-06-09T13:54:06Z

src/shared/gitlab-client.ts

+  public async getUsers() {
+    const users = await this.fetchJson<User[]>(
+      `users`,
+      true,


What's this boolean for? Shouldn't the second parameter be an object?

Same thing below.

vsaarinen · 2017-06-09T13:55:13Z

src/shared/gitlab-client.ts

-    const groups = await this.fetchJson<Group[]>(`groups?${qs.stringify({ search })}`, true);
+    const groups = await this.fetchJson<Group[]>(
+      `groups?${qs.stringify({ search })}`,
+      true,


Same thing here

vsaarinen · 2017-06-09T13:55:25Z

src/shared/gitlab-client.ts

-      group = await this.fetchJson<Group>(`groups/${groupIdOrPath}?${qs.stringify(sudo)}`, true);
+      group = await this.fetchJson<Group>(
+        `groups/${groupIdOrPath}?${qs.stringify(sudo)}`,
+        true,


vsaarinen · 2017-06-09T13:55:33Z

src/shared/gitlab-client.ts

-      project = await this.fetchJson<Project>(`projects/${projectId}?${qs.stringify(sudo)}`, true);
+      project = await this.fetchJson<Project>(
+        `projects/${projectId}?${qs.stringify(sudo)}`,
+        true,


This is actually pretty surprising. After looking at what's happening here, I found: microsoft/TypeScript#7485. So TS 2.4 would've caught this. Anyway, fixed.

Refactor the auth0 configuration parameters

…ntication

juhoojala · 2017-06-15T10:36:52Z

src/config/config-production.ts

@@ -142,6 +148,12 @@ const EXTERNAL_BASEURL = env.EXTERNAL_BASEURL || `http://localhost:${PORT}`;
 // External baseUrl for git clone urls
 const EXTERNAL_GIT_BASEURL = env.EXTERNAL_GIT_BASEURL || `http://localhost:${GITLAB_PORT}`;

+// External baseUrl for git clone urls
+const GIT_VHOST = env.GIT_VHOST || parseUrl(EXTERNAL_GIT_BASEURL).hostname;


What is the difference between EXTERNAL_GIT_BASEURL and GIT_VHOST?

Fixed comments

juhoojala · 2017-06-15T10:41:17Z

src/integration-test/git-authentication-tests.ts

+      const invalidSecret = secret + '9';
+
+      it('should be able to get the accessToken with correct credentials', async () => {
+        // Arrange


Arrange is empty? Maybe remove the titles altogether?

juhoojala · 2017-06-15T10:43:35Z

src/integration-test/git-authentication-tests.ts

@@ -0,0 +1,120 @@
+import { expect } from 'chai';


Are these integration tests or unit tests?

Added comment

juhoojala · 2017-06-15T13:59:36Z

👍 This can now be merged, but we need to check for infra changes.

Avoid using `import * as`

Add more methods to CharlesClient Allow defining the config as an S3 url

vsaarinen · 2017-06-15T16:23:52Z

docs/testing.md

-copy it to the `gitPassword` field of the corresponding team in the `auth0` block
-of the configuration file.
+is the id of the corresponding Auth0 client. Additionally, create one more new user, which
+you don't need add to any group. This will be admin user.


...need to add...
...will be the admin...

vsaarinen · 2017-06-15T16:24:13Z

docs/testing.md

+is the id of the corresponding Auth0 client. Additionally, create one more new user, which
+you don't need add to any group. This will be admin user.
+
+After setting up these configuration, update the passwords for all users


After setting this up, update...

vsaarinen · 2017-06-20T08:46:48Z

src/project/cached-project-module-spec.ts

      store: 'memory',
      ttl: 1000,
-    }) as Cache;
+    }) as {} as Cache;


caching returns its own Cache type which is different than the Cache type returned here. Is that by accident? The casting hides this incompatibility.

This is by design, but admittedly suboptimal. We have just rewritten the Cache interface with promises.

juhoojala · 2017-06-20T11:44:27Z

👍

vsaarinen reviewed Jun 9, 2017

View reviewed changes

vsaarinen force-pushed the update-packages-node branch from ea3446a to 30399fa Compare June 9, 2017 14:09

dennari and others added 6 commits June 9, 2017 17:21

Add a proxy and authentication scheme for git http connections

d367d18

Refactor the auth0 configuration parameters

Update existing integration tests and add new ones for git http authe…

db262bd

…ntication

Generate deterministic GitLab passwords

06079e1

Regenerate GitLab passwords

0953088

Cleanup and formatting

b93fc5b

Clean up code

dfe498e

vsaarinen changed the base branch from update-packages-node to master June 9, 2017 14:27

vsaarinen force-pushed the git-http-authentication branch from 582e1f4 to dfe498e Compare June 9, 2017 14:28

vsaarinen added 3 commits June 9, 2017 17:29

Add Auth0 dependency

66973e6

Avoid using import * as

2bf0cae

Don't import Boom as *

9c6cb5f

vsaarinen mentioned this pull request Jun 9, 2017

Avoid using import * as #217

Merged

vsaarinen and others added 6 commits June 9, 2017 18:12

Fix util-promisify

539e248

Add comments

b91047f

Fix calls to fetchJson

9b2a5ec

Merge branch 'git-http-authentication' into avoid-importing-all

d3f9da9

Fix getUsers in gitlab-client

aec584d

Don’t change password for root user

b14c98b

juhoojala reviewed Jun 15, 2017

View reviewed changes

juhoojala added 3 commits June 15, 2017 16:52

Fix some documentation

3a8f876

Sligthly cleanup git-authentication-tests

0a1f6d6

Improve comments in config-production

1491565

juhoojala and others added 5 commits June 15, 2017 17:05

Merge pull request #217 from lucified/avoid-importing-all

a999a0f

Avoid using `import * as`

Have a single admin user instead of an admin team

030a82e

Add the 'charles-client' cli

005ea58

Add more methods to CharlesClient Allow defining the config as an S3 url

Fix the tests

0925a2a

Improve documentation

b6ec778

vsaarinen reviewed Jun 15, 2017

View reviewed changes

dennari added 5 commits June 16, 2017 10:36

Remove git password related code from AuthenticationHapiPlugin

7aa2a7f

Throw errors in intra-team tests but not in the inter-team ones

90f6ffe

Use node 8.1

36caae8

Update docs

e2ef9ec

Minor package updates

9955de7

lucify-deployments bot deployed to staging June 16, 2017 10:55 View deployment

Also update the production Dockerfile to Node 8

46acc2e

lucify-deployments bot had a problem deploying to staging June 16, 2017 11:18 Failure

Include package-lock.json

b9ed9e0

lucify-deployments bot had a problem deploying to staging June 16, 2017 11:34 Failure

lucify-deployments bot had a problem deploying to staging June 16, 2017 11:45 Failure

Don't clone the headers in mapUri

b56fb3a

lucify-deployments bot had a problem deploying to staging June 19, 2017 14:16 Failure

Prevent writing to package-lock.json on npm install

d047286

lucify-deployments bot had a problem deploying to staging June 19, 2017 15:04 Failure

Don't use package-lock.json

8c2c79f

lucify-deployments bot had a problem deploying to staging June 19, 2017 15:12 Failure

dennari added 2 commits June 19, 2017 19:04

Just use Yarn

6f352e1

Fix typing errors

755edcb

lucify-deployments bot had a problem deploying to staging June 19, 2017 16:14 Failure

dennari added 2 commits June 19, 2017 19:33

Use the included Yarn

4af2fba

Don't transpile in the production container

e1bf5d6

lucify-deployments bot deployed to staging June 19, 2017 16:53 View deployment

Fix typo and increase timeout

dfb207e

vsaarinen reviewed Jun 20, 2017

View reviewed changes

Minor casting tweaks

49b20cc

juhoojala merged commit 5b827da into master Jun 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Git http authentication #216

Git http authentication #216

dennari commented Jun 7, 2017 •

edited by vsaarinen

Loading

vsaarinen left a comment

vsaarinen Jun 9, 2017

vsaarinen Jun 9, 2017

dennari Jun 12, 2017

vsaarinen Jun 12, 2017

vsaarinen Jun 9, 2017

dennari Jun 12, 2017

vsaarinen Jun 9, 2017

vsaarinen Jun 9, 2017

vsaarinen Jun 9, 2017 •

edited

Loading

vsaarinen Jun 9, 2017

vsaarinen Jun 9, 2017

vsaarinen Jun 9, 2017

dennari Jun 12, 2017

juhoojala Jun 15, 2017

juhoojala Jun 15, 2017

juhoojala Jun 15, 2017

juhoojala Jun 15, 2017

juhoojala Jun 15, 2017

juhoojala Jun 15, 2017

juhoojala commented Jun 15, 2017

vsaarinen Jun 15, 2017

vsaarinen Jun 15, 2017

vsaarinen Jun 20, 2017

dennari Jun 20, 2017

juhoojala commented Jun 20, 2017

Git http authentication #216

Git http authentication #216

Conversation

dennari commented Jun 7, 2017 • edited by vsaarinen Loading

Description

Testing locally

vsaarinen 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

vsaarinen Jun 9, 2017 • edited Loading

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

juhoojala commented Jun 15, 2017

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

juhoojala commented Jun 20, 2017

dennari commented Jun 7, 2017 •

edited by vsaarinen

Loading

vsaarinen Jun 9, 2017 •

edited

Loading