Make OIDC end_session_endpoint optional

[oneonestar]

Description

Make OIDC end_session_endpoint optional.
Fix #19844

Although this field is mandatory in the spec OpenID Connect RP-Initiated Logout 1.0 , the spec itself is not mandatory.
Some providers do not implement it:

• https://accounts.google.com/.well-known/openid-configuration
• End Session Endpoint Support in Dex dexidp/dex#1697

https://openid.net/developers/how-connect-works/

Release notes

(x) This is not user-visible or is docs only, and no release notes are required.

[oneonestar]

Looks like a bug to me.
It works with StaticConfigurationProvider (http-server.authentication.oauth2.oidc.discovery=false).

2024-02-20T09:24:53.681+0900	INFO	main	io.airlift.log.Logging	Logging to stderr
2024-02-20T09:24:53.682+0900	INFO	main	Bootstrap	Loading configuration
2024-02-20T09:24:53.723+0900	INFO	main	org.hibernate.validator.internal.util.Version	HV000001: Hibernate Validator 8.0.1.Final
2024-02-20T09:24:53.793+0900	INFO	main	Bootstrap	Initializing logging
2024-02-20T09:24:54.200+0900	ERROR	main	io.trino.server.Server	Configuration is invalid
==========

Errors:

1) Configuration property 'http-server.authentication.oauth2.end-session-url' was not used

==========

#20754

[mosabua]

So should we update the docs .. I kinda feel like it would be better to merge this PR.

[oneonestar]

Currently, there are 3 ways to config a OAuth2 property:

1. Static configuration (http-server.authentication.oauth2.oidc.discovery=false)
2. Obtain from OpenID provider using OIDC discovery (http-server.authentication.oauth2.oidc.discovery=true)
3. Obtain from OpenID provider using OIDC discovery, but overrides it in config file

Not all properties support all 3 ways.

This PR #20753 allows end_session_endpoint to be optional in the OIDC discovery.
This means we can turn on OIDC discovery feature even if OpenID provider doesn't support end_session_endpoint.
Without this PR, static configuration must be used if the provider doesn't support end_session_endpoint.

The issue (#20754) is about end_session_endpoint doesn't support the third
way but haven't been documented properly.

[github-actions]

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

[mosabua]

Does this look good now @Praveen2112 ?

[Praveen2112]

I'll take a look

[github-actions]

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

[mosabua]

Reminder @Praveen2112 and @oneonestar

[Praveen2112]

@oneonestar Can we rebase it out.

Although this field is mandatory in the spec OpenID Connect RP-Initiated Logout 1.0 , the spec itself is not mandatory.

The spec mentions this mandatory and I'm not sure if we need to make it optional.

I would rely on @dain / @lukasz-walkiewicz opinion here.

[whg517]

I'm trying to use dexidp on top of k8s to build automatically enhanced security authentication for projects using operator. When I was validating trino, I ran into the same problem. I would like to know how the current community views this issue, including how to deal with it in the future, and whether there is a roadmap?

[Praveen2112]

Would like to have @lukasz-walkiewicz review here.

[lukasz-walkiewicz]

Are these mixed cases any useful? Aren't two tests cases enough as previously?

[Praveen2112]

Previously we weren't covering these cases right ? So aren't they good ?

[Praveen2112]

@oneonestar Can we address this comment - Then I guess we could merge this PR

[oneonestar]

This PR makes endSessionPath optional.
This way ensures that the previous tests work with or without setting endSessionPath.

[Praveen2112]

Thanks for fixing this issue