OIDC middleware

[simongottschlag]

Hi!

I'm working on a library that exposes an OAuth2 authorization server / OpenID Provider meant for tests. While working on it, I created an initial middleware (roughly based on JWT) to work with OIDC.

You can see the initial middleware here: https://github.com/XenitAB/pkg/blob/main/echo-v4-middleware/oidc/oidc.go

I still have some work left as well as adding tests, but you can see how it's used here: https://github.com/XenitAB/dispans/blob/main/example/main_test.go#L33-L62

Would this be something you would like to be contributed directly to the echo project? I'd be willing to do a PR and help out with future issues around it (best effort).

Best regards,
Simon

[aldas]

Hi!
From scrolling through that file it seems to be only part that is different is ParseTokenFunc implementation. Would it not be easier if OIDC parts is encapsulated only in ParseTokenFunc. In that way it is easier to maintain as it has less code.

type OIDCConfig struct {
	AllowedTokenDrift time.Duration
}

func OIDC(config OIDCConfig) func(auth string, c echo.Context) (interface{}, error) {
	if config.AllowedTokenDrift == 0 {
		config.AllowedTokenDrift = 10 * time.Second
	}
	// ...
	return func(auth string, c echo.Context) (interface{}, error) {
		// implementation
		return nil, nil
	}
}

and you would use it with JTW middleware

func main() {
	e := echo.New()

	e.Use(middleware.JWTWithConfig(middleware.JWTConfig{
		ParseTokenFunc: OIDC(OIDCConfig{}),
	}))

	s := http.Server{Addr: ":8082", Handler: e}
	if err := s.ListenAndServe(); err != http.ErrServerClosed {
		log.Fatal(err)
	}
}

I would also suggest to send PR to https://github.com/labstack/echo-contrib as it has external dependency to https://github.com/lestrrat-go/jwx and we have already situation with current JWT token parser library it would be easier in future to have updates in that way. And we can include OIDC in https://github.com/labstack/echox (echo site) as other middlewares from echo-contrib are there.

p.s. is OIDC correct term? Would something contanting JWT and/or JWK be more precise? For example for me OIDC name relates more with the flows and less with token extraction/validation that is small detail of flows.

[simongottschlag]

From scrolling through that file it seems to be only part that is different is ParseTokenFunc implementation. Would it not be easier if OIDC parts is encapsulated only in ParseTokenFunc. In that way it is easier to maintain as it has less code.

This was only the initial test to validate the inner workings, I've started to refactor it and I do think it would be better if it was completely separate - making sure it can be changed without having to think about the JWT implementation.

I would also suggest to send PR to https://github.com/labstack/echo-contrib as it has external dependency to https://github.com/lestrrat-go/jwx and we have already situation with current JWT token parser library it would be easier in future to have updates in that way. And we can include OIDC in https://github.com/labstack/echox (echo site) as other middlewares from echo-contrib are there.

I completely agree! I will finish up the middleware and make sure it's fully tested, then create a PR for contrib and the site. Any help in making sure the PRs meet the expected quality would be highly appreciated!

p.s. is OIDC correct term? Would something contanting JWT and/or JWK be more precise? For example for me OIDC name relates more with the flows and less with token extraction/validation that is small detail of flows.

My initial thought was that OIDC was the correct name, since it's using the discovery model and is quite opinionated. As a reference, seems like others call middlewares like this the same: https://github.com/auth0/express-openid-connect

[aldas]

making sure it can be changed without having to think about the JWT implementation

Currently Echo has two middlewares KeyAuth and JWT which are pretty much same - they both extract some string from request and run it though some "check" and return error on failure (and some value). As JWT is used more often the extraction part is more evolved than KeyAuth but essentially they are pretty much the same. Now proposed OIDC token is just variation of JWT middleware in sense that is JWT+JWK. It does not make sense to introduce third set of extractors to do same things. It is just maintenance burden. This will change soon in v5 or maybe in v4 as I have done background work to merge extraction part of those middlewares.

I would still suggest having at least word token in name. I have implemented OIDC flows before and when I saw notice email from github about "OIDC" I thought - cool, someone made life easier for others and implemented some easy way to registers OICD provider flow handlers with echo.

p.s. you might want to make newKeyHandler lazy. I have had problems with applications with KeyCloak as oauth2 provider and when during startup you cant connect immediately to KeyCloak it will not start which will render application completely unusable and not web/API requests that need that part.

[simongottschlag]

Thank you for the responses! 🥇

Currently Echo has two middlewares KeyAuth and JWT which are pretty much same - they both extract some string from request and run it though some "check" and return error on failure (and some value). As JWT is used more often the extraction part is more evolved than KeyAuth but essentially they are pretty much the same. Now proposed OIDC token is just variation of JWT middleware in sense that is JWT+JWK. It does not make sense to introduce third set of extractors to do same things. It is just maintenance burden. This will change soon in v5 or maybe in v4 as I have done background work to merge extraction part of those middlewares.

With the above in mind, maybe the most maintainable way forward would be for me to create a PR for this repository (not contrib) and add oidc_discovery.go and oidc_discover_test.go in the middleware folder, and document how to use it together with the JWT middleware as you proposed with ParseTokenFunc? I could call it OIDCDiscovery and OIDCDiscoveryConfig{}?

I would still suggest having at least word token in name. I have implemented OIDC flows before and when I saw notice email from github about "OIDC" I thought - cool, someone made life easier for others and implemented some easy way to registers OICD provider flow handlers with echo.

Haha, that's another project I'm working on. Maybe not specific for echo, but this work comes out of me creating a library that makes it easy to create an OpenID Provider that can be used in tests. Both of these are yak shaving from me wanting to build something that needs that OpenID Provider for tests.

I don't really mind what name it has and will leave it up to you, making sure that it fits with the project as a whole. I myself wouldn't name it OIDCToken but rather OIDCDiscovery - but as always, naming things is one of the harder things. Perhaps we can resolve it when everything else is done and we both think the PR is ready?

p.s. you might want to make newKeyHandler lazy. I have had problems with applications with KeyCloak as oauth2 provider and when during startup you cant connect immediately to KeyCloak it will not start which will render application completely unusable and not web/API requests that need that part.

I am open for this, but usually prefer the application to crash if things like this isn't available and making sure that the orchestration system restarts it and the monitoring picks up that it's not starting. Wouldn't be much better having it up without the keys, no requests would work but it could be "hidden" from the orchestration system that there's an issue. As above, I'm willing to do this as you want - but maybe something we should decide on in the PR.

With all the above, would you want me to create a PR for this repository and create something that can be used together with the JWT middleware as a ParseTokenFunc?

[simongottschlag]

@aldas I've created the (draft) PR #1930 - let's continue over there. I've added subtasks based on the questions that haven't been answered in this issue.

[simongottschlag]

Opened a new PR over at contrib: labstack/echo-contrib#59

[aldas]

Closing. end result is https://github.com/XenitAB/go-oidc-middleware