OIDC module for contour-authserver #13

robinfoe · 2021-01-16T16:51:10Z

OIDC module for contour-authserver

jpeach · 2021-01-17T21:51:58Z

@robinfoe for the DCO check to pass you need to sign your commits with git commit -s. Use your vmware email for this. It's fine do squash and rewrite the commit message to give an editorial overview of the change at this point.

jpeach

Making good progress. I think after this round we can probably land this. Let's make sure we have a TODO marking where we think additional config may be needed and any other items that can be filled in later.

jpeach · 2021-02-07T04:23:38Z

config/oidc/configmap.yaml

+  auth-svr-config.yaml: |
+    address: ":9443"
+    issuerURL: "http://dex.auth.app.192.168.10.134.nip.io:9080"
+    redirectURL: "https://echo.oidc.app.local:9443"


This is global config for an instance of contour-authserver, but it probably only makes sense for a single vhost. Could we use the vhost metadata to pass down this sort of configuration? WDYT?

Feels like we could TODO this one, get this in, then come back and fix. We're still in experimental territory.

I think this is okay for the first version, but it will need a TODO issue. I'll log one and update here. Once I do, @robinfoe, can you please drop a comment in here and in the docs for the command?

This is #17.

config/oidc/configmap.yaml

pkg/auth/oidc_connect.go

jpeach · 2021-02-07T05:02:49Z

pkg/auth/oidc_connect.go

+		return false
+	}
+
+	return true


Leave a TODO (and GitHub issue) for propagating the claims? I would guess that many apps would want to see the verified claims?

This one needs an issue for tracking as well.

pkg/cli/oicd_connect.go

jpeach · 2021-02-07T05:36:34Z

pkg/cli/oicd_connect.go

+				return ExitError{EX_CONFIG, err}
+			}
+
+			if cfg == nil {


This should be an error since a nil will make it panic later. Even better wold be for the config to be guaranteer non-nil when there is no error.

I think that in the version I'm reviewing it's not possible for this to be nil here? It will always have at least the defaults? If so, this check will never be used.

pkg/config/config.go

pkg/auth/oidc_connect.go

Signed-off-by: James Peach <[email protected]> Signed-off-by: robinfoe <[email protected]>

…ojectcontour#3) The makefile task `make lint` did not work if the `golangci-lint` binary is not found locally on the machine. This fixes the run command for `docker` builds as well as upgrade to latest since issues were found in the older version. Signed-off-by: Steve Sloka <[email protected]> Signed-off-by: robinfoe <[email protected]>

Add standard `ProjectContour` copyright information to all source code files. Signed-off-by: Steve Sloka <[email protected]> Signed-off-by: robinfoe <[email protected]>

Internally, the check response heaers use maps, so the header slice ordering is not stable. Add a test helper to sort this for stable test comparisons. Signed-off-by: James Peach <[email protected]> Signed-off-by: robinfoe <[email protected]>

Signed-off-by: robinfoe <[email protected]>

Labels have more restrictive value validation than annotations, so switch to annotations for specifying the basic authentication realm and type. To make up for the use of annotations, add a label selector flag so that operators can filter which Secrets to actually use. This fixes projectcontour#4. Signed-off-by: James Peach <[email protected]> Signed-off-by: robinfoe <[email protected]>

Signed-off-by: robinfoe <[email protected]>

This fixes projectcontour#5. Signed-off-by: James Peach <[email protected]> Signed-off-by: robinfoe <[email protected]>

Signed-off-by: robinfoe <[email protected]>

Move the common gRPC server initialization code into a helper function. Signed-off-by: James Peach <[email protected]> Signed-off-by: robinfoe <[email protected]>

The `testserver` subcommand was running the `Htpasswd` backend rather than the `Testserver` backend. Signed-off-by: James Peach <[email protected]> Signed-off-by: robinfoe <[email protected]>

Signed-off-by: James Peach <[email protected]> Signed-off-by: robinfoe <[email protected]>

Signed-off-by: robinfoe <[email protected]>

Signed-off-by: robin foe <[email protected]> Signed-off-by: robinfoe <[email protected]>

youngnick

Quick edit pass over the initial docs, I need to review the oauth flow before I can review properly, sorry.

README.md

youngnick · 2021-02-11T00:10:26Z

config/oidc/configmap.yaml

+  auth-svr-config.yaml: |
+    address: ":9443"
+    issuerURL: "http://dex.auth.app.192.168.10.134.nip.io:9080"
+    redirectURL: "https://echo.oidc.app.local:9443"


Feels like we could TODO this one, get this in, then come back and fix. We're still in experimental territory.

Co-authored-by: Nick Young <[email protected]>

youngnick

Sorry, hit approve by mistake. Still need to review the OIDC flow stuff.

Sorry again @robinfoe.

sunjayBhatia

@robinfoe could you rebase on main so the set of changed files is smaller?

The pod was OOMKilled with 30 Mi, so increase the limit to 90 Mi. Signed-off-by: Suraj Deshmukh <[email protected]>

Signed-off-by: Steve Sloka <[email protected]>

stevesloka · 2021-05-24T19:51:54Z

Hey, @robinfoe do you think you'd like to finish this PR up? Happy to help out getting this pushed through, or if not, could we pick up where you left off to get this complete?

Signed-off-by: robinfoe <[email protected]>

Signed-off-by: robin foe <[email protected]> Signed-off-by: robinfoe <[email protected]>

Co-authored-by: Nick Young <[email protected]>

robinfoe · 2021-08-30T04:58:01Z

hi @stevesloka I did the rebase on this, do you mind checking and see if it's good to go?

youngnick

Sorry to take so long to review this @robinfoe.

I think this is looking pretty good, but needs a couple of tracking issues for TODOs (which I will add).

For those, can you put a comment that looks like:

// TODO(robinfoe) #issuenumber : description

That tells us what issue to check, who has the most context, and a rough idea of what to do.

youngnick · 2021-09-15T05:18:35Z

config/oidc/configmap.yaml

+  auth-svr-config.yaml: |
+    address: ":9443"
+    issuerURL: "http://dex.auth.app.192.168.10.134.nip.io:9080"
+    redirectURL: "https://echo.oidc.app.local:9443"


I think this is okay for the first version, but it will need a TODO issue. I'll log one and update here. Once I do, @robinfoe, can you please drop a comment in here and in the docs for the command?

pkg/auth/oidc_connect.go

youngnick · 2021-09-15T05:23:30Z

pkg/auth/oidc_connect.go

+		return false
+	}
+
+	return true


This one needs an issue for tracking as well.

youngnick · 2021-09-15T05:25:42Z

pkg/cli/oicd_connect.go

+				return ExitError{EX_CONFIG, err}
+			}
+
+			if cfg == nil {


I think that in the version I'm reviewing it's not possible for this to be nil here? It will always have at least the defaults? If so, this check will never be used.

youngnick · 2021-09-15T05:28:12Z

pkg/store/state.go

+	StatusTokenReady
+)
+
+// NewState .... create new state to store token for OIDC


Can you please update these comments to be of the following form?

// XFunc does the thing that it does.

I agree that a bunch of them are pretty simple, but it doesn't hurt to explain what they do and are used for regardless.

stevesloka · 2021-11-03T14:14:48Z

Hey @robin-mbg thanks for the work on this! Going to merge and add some more PRs to follow-up, but this a great start! 🎉

robin-mbg · 2021-11-03T15:11:37Z

@stevesloka I think the thanks belongs to @robinfoe as I have honestly never heard of this project, nor have I actively contributed, despite the commit history. It seems like @robinfoe is using an email address for his commits that GitHub attributes to my account. Looks quite interesting though and will definitely take a closer look at this project from now on.

stevesloka · 2021-11-03T15:32:11Z

Huh? Sorry for the mixup @robin-mbg. I assumed @robinfoe starting using a new github handle for some reason. 🙃

robinfoe · 2022-02-24T03:30:14Z

@stevesloka , thx for the merge, not too sure what's going on with my Github account but thx for spotting that , will get it fixed :)

jpeach requested review from jpeach and youngnick January 17, 2021 21:52

jpeach suggested changes Feb 7, 2021

View reviewed changes

jpeach and others added 16 commits February 11, 2021 10:55

Add a Github pull request check action. (projectcontour#1)

3e7156e

Signed-off-by: James Peach <[email protected]> Signed-off-by: robinfoe <[email protected]>

add copyright information (projectcontour#2)

22a94e2

Add standard `ProjectContour` copyright information to all source code files. Signed-off-by: Steve Sloka <[email protected]> Signed-off-by: robinfoe <[email protected]>

initial checkin

98d1c45

Signed-off-by: robinfoe <[email protected]>

fix error handling flow

154e1ff

Signed-off-by: robinfoe <[email protected]>

Add release tooling. (projectcontour#8)

35713cc

This fixes projectcontour#5. Signed-off-by: James Peach <[email protected]> Signed-off-by: robinfoe <[email protected]>

fix handler flow and comments

c94897d

Signed-off-by: robinfoe <[email protected]>

Add testcase

3ec8d04

Signed-off-by: robinfoe <[email protected]>

Move common gRPC server code. (projectcontour#9)

ff47c07

Move the common gRPC server initialization code into a helper function. Signed-off-by: James Peach <[email protected]> Signed-off-by: robinfoe <[email protected]>

Fix the testserver backend. (projectcontour#10)

7e89e52

The `testserver` subcommand was running the `Htpasswd` backend rather than the `Testserver` backend. Signed-off-by: James Peach <[email protected]> Signed-off-by: robinfoe <[email protected]>

Fix secrets key name.

5bcca54

Signed-off-by: James Peach <[email protected]> Signed-off-by: robinfoe <[email protected]>

add readme and code cleanup

679bf29

Signed-off-by: robinfoe <[email protected]>

remote vscode entry

a09b65c

Signed-off-by: robinfoe <[email protected]>

Various fixes and code restructuring

d18e0a2

Signed-off-by: robin foe <[email protected]> Signed-off-by: robinfoe <[email protected]>

robinfoe force-pushed the contour-ext-auth branch from 61a966a to d18e0a2 Compare February 11, 2021 02:57

youngnick reviewed Feb 11, 2021

View reviewed changes

youngnick mentioned this pull request Feb 17, 2021

OIDC support in contour and contour-authserver #12

Open

5 tasks

robinfoe and others added 2 commits March 2, 2021 10:16

Update README.md

6825aaf

Co-authored-by: Nick Young <[email protected]>

Update README.md

65a5eab

Co-authored-by: Nick Young <[email protected]>

youngnick approved these changes Mar 2, 2021

View reviewed changes

youngnick changed the title ~~Contour ext auth~~ OIDC module for contour-authserver Mar 2, 2021

youngnick self-requested a review March 2, 2021 03:06

youngnick suggested changes Mar 2, 2021

View reviewed changes

sunjayBhatia reviewed Mar 2, 2021

View reviewed changes

surajssd and others added 2 commits April 6, 2021 10:30

htpasswd-deployment: Increase mem limit to 90 Mi (projectcontour#15)

2f6ab4c

The pod was OOMKilled with 30 Mi, so increase the limit to 90 Mi. Signed-off-by: Suraj Deshmukh <[email protected]>

enable arm64 binaries/images (projectcontour#16)

ec18211

Signed-off-by: Steve Sloka <[email protected]>

robinfoe and others added 10 commits August 20, 2021 23:14

initial checkin

21b76be

Signed-off-by: robinfoe <[email protected]>

fix error handling flow

d9c46ee

Signed-off-by: robinfoe <[email protected]>

fix handler flow and comments

929d046

Signed-off-by: robinfoe <[email protected]>

Add testcase

be80bfe

Signed-off-by: robinfoe <[email protected]>

add readme and code cleanup

bc62091

Signed-off-by: robinfoe <[email protected]>

remote vscode entry

9493ece

Signed-off-by: robinfoe <[email protected]>

Various fixes and code restructuring

ca62306

Signed-off-by: robin foe <[email protected]> Signed-off-by: robinfoe <[email protected]>

Update README.md

17b6d03

Co-authored-by: Nick Young <[email protected]>

Update README.md

35f6d3e

Co-authored-by: Nick Young <[email protected]>

rebasing

5ec59ef

youngnick suggested changes Sep 15, 2021

View reviewed changes

youngnick mentioned this pull request Sep 15, 2021

OIDC support should be on a per-vhost basis #17

Open

robin added 2 commits October 1, 2021 10:43

add comments

4ff9b08

add comments

3c1d5bb

stevesloka merged commit 1ee7415 into projectcontour:main Nov 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OIDC module for contour-authserver #13

OIDC module for contour-authserver #13

robinfoe commented Jan 16, 2021

jpeach commented Jan 17, 2021

jpeach left a comment

jpeach Feb 7, 2021

youngnick Feb 11, 2021

youngnick Sep 15, 2021

youngnick Sep 15, 2021

jpeach Feb 7, 2021

youngnick Sep 15, 2021

youngnick Sep 15, 2021

jpeach Feb 7, 2021

youngnick Sep 15, 2021

youngnick left a comment

youngnick Feb 11, 2021

youngnick left a comment

sunjayBhatia left a comment

stevesloka commented May 24, 2021

robinfoe commented Aug 30, 2021

youngnick left a comment

youngnick Sep 15, 2021

youngnick Sep 15, 2021

youngnick Sep 15, 2021

youngnick Sep 15, 2021

stevesloka commented Nov 3, 2021

robin-mbg commented Nov 3, 2021

stevesloka commented Nov 3, 2021

robinfoe commented Feb 24, 2022

OIDC module for contour-authserver #13

OIDC module for contour-authserver #13

Conversation

robinfoe commented Jan 16, 2021

jpeach commented Jan 17, 2021

jpeach 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

youngnick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youngnick left a comment

Choose a reason for hiding this comment

sunjayBhatia left a comment

Choose a reason for hiding this comment

stevesloka commented May 24, 2021

robinfoe commented Aug 30, 2021

youngnick 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

stevesloka commented Nov 3, 2021

robin-mbg commented Nov 3, 2021

stevesloka commented Nov 3, 2021

robinfoe commented Feb 24, 2022