Add authentication mechanism for OpenID Connect

[arjantijms]

[rdebusscher]

I want to get started on this topic since Payara has already an integration of OpenIdConnect with Soteria. You can read more about the supported functionality on this documentation page https://docs.payara.fish/community/docs/documentation/payara-server/public-api/openid-connect-support.html

Do we first need an agreement on some items before we start putting something in the spec and Impl project?

like

• Agree on the properties of @OpenIdAuthenticationDefinition
• Do we define specialized annotations for Google, Azure, ...?
• What will be in OpenIdContext
• Do we support automatic redirect to original page (only with GET or with any method)?
• ...

[keilw]

That sounds very specific. We don't have anything for OAuth either, so what about OAuth2? https://auth0.com/docs/protocols/openid-connect-protocol and similar sites clearly state, that OpenIDConnect is just a layer and extension to OAuth 2.0. So why add something for the OpenIDConnect extension instead, something along the lines of jakarta.security.enterprise.authentication.mechanism.oauth? Matching the jakarta.security.enterprise.authentication.mechanism.http we already got.

[arjantijms]

OpenIDConnect sounds like a very good start. OAuth2 itself is actually more of an authorisation protocol, where authentication is just a side-effect of being authorised for something (say, reading tweets when used with Twitter).

In that sense, OIC is more "pure" and more suitable for use as an authentication mechanism.

Following the "code first" approach that we used to develop Jakarta Security 1.0, I'd say, let's make master the 3.x branch and "just put it in".

From a working mechanism we can then start to setup the specified parts, collect feedback about that, and based on it update the implementation again.

Ps big thanks to Payara for considering this!

[keilw]

As long as they are properly separated and isolated from each other, I don't see a problem with an "oidc" or "oic" package similar to what we got with "http" before, but please as part of upgrades we should do this properly similar to NoSQL. There under communication every type of DB is properly separated and you may (no JPMS modules yet, but by the time NoSQL gets ready they shall) use each of them independently:

• Column
• Document
• Key-Value

So we must have similar modularized approch and refactoring a bit like what CDI is facing or Jakarta REST (with Server vs. Client or more) and offer something like

• HTTP
• OpenID Connect
• ...

under authentication/mechanism in a similar way.

[arjantijms]

The OIC mechanism can be fully implemented as an HttpAuthenticationMechanism.

In fact, that's also exactly what Payara has been doing. See:

https://github.com/payara/ecosystem-security-connectors/blob/master/openid/src/main/java/fish/payara/security/openid/OpenIdAuthenticationMechanism.java#L136

[keilw]

Then what's the benefit of adding something new to the spec/API that maybe not every implementor wishes to support? There was a discussion about Optional Features today in the Spec Committee and while the current use of the term "Optionality" is much more about avoiding the burden and effort to implement "soon to be deprecated" features I would argue this is no different. It may be much smaller for this API but take the NoSQL example where it seems to have a much greater impact, if say MongoDB wanted to implement Jakata NoSQL but only for document-based systems or Neo4J only for Graph-DBs.

[arjantijms]

Then what's the benefit of adding something new to the spec/API that maybe not every implementor wishes to support?

The idea would be for everyone to support it ;)

The list of default available authentication mechanisms would then be:

• BASIC
• FORM
• FORM+
• OIC
• JWT

Hopefully we can extend that with:

• DIGEST
• CLIENT-CERT

[keilw]

Even if there may not be any optionality on this level (e.g. in the TCK) we should get a little more modular and break the API into modules like jakarta.security.enterprise.authentication.mechanism.http (ideally dropping the "enterprise" to shorten it to jakarta.security.authentication.mechanism.http) and others like jakarta.security.authentication.mechanism.basic, jakarta.security.authentication.mechanism.form, jakarta.security.authentication.mechanism.oic, etc.

[arjantijms]

Just a small ping here, we have about a month left to get this in ;)

[keilw]

Where would the list be by now? And what about https://github.com/eclipse-ee4j/security-api/pull/185 ?

[keilw]

How does this correlate with https://github.com/eclipse-ee4j/soteria/pull/313, will that still make it into Soteria 3 or not?

[rdebusscher]

The Soteria PR for OpenId Connect is ready for some months now but no one seems interested to review it to get it included. So I merged the API part already to put on some pressure.

[keilw]

Thanks @rdebusscher, I also mentioned to @arjantijms, that currently our book together with @teobais https://github.com/Apress/definitive-guide-jakarta-ee-security makes a pretty bad impression despite a vague Jakarta EE 10 outlook if we keep saying it "may include OIDC" because all the other solutions like Spring Security, Keycloak, etc. long do.

[arjantijms]

The Soteria PR for OpenId Connect is ready for some months now but no one seems interested to review it

I just noticed a previous reply seemingly never appeared here. From memory I said it looked good, but a small thing was that the logout functionality didn't need an explicit logout method in the API. I discussed this at the time with Gaurav se well.

The current functionality for it should be moved to cleanSubject in the HttpAuthenticationMechanism.

Additionally my review then mentioned that some types should not be directly in one of the main packages, but should go to a sub package, as they are really specific to this Authentication Mechanism.

[rdebusscher]

Since I don't have the time to finish the TCK, the OpenId Connect need to be skipped (and the entire Security API/Soteria 3.0 is not included in Jakarta EE 10)

• There was never any proper discussion about the OpenId Connect topic.
• No review from anyone on the PRs (from community nor Vendors)
• No TCK so there will not be any minimal check if an implementation follows the spec.
• Only last-minute some vague comments.

When there are no requests or interests in the Security changes, we should not do them. And especially not last minute as that will introduce some features that aren't well thought out.

[rdebusscher]

I will revert https://github.com/jakartaee/security/commit/7e2ab468a1fbebf33565696e95d01f86ec71427c soon.

[arjantijms]

@rdebusscher I don't think it's needed to revert. See my comment from 5 days back, I reviewed, and it looks quite good. I only had the few comments about the logout functionality being in the wrong location, and code needing to be moved to sub-packages.

[arjantijms]

and the entire Security API/Soteria 3.0 is not included in Jakarta EE 10

That's another point to consider. There were a few small features added before, that absolutely do make sense and have been often requested. For instance, I recorded some 30+ requests (email, forum, SO) for making CallerPrincipal Serializable (https://github.com/jakartaee/security/pull/198).

That and a few other features absolutely make sense.

The question is then, do we call it 3.0 still? Maybe that will make less sense. A big driving feature was https://www.eclipse.org/lists/es-dev/msg00137.html, but there too nobody responded (Rudy sent a reply, but it was mainly about something related, not directly about the authorization modules themselves).

Another big driving feature was indeed OpenID Connect.

The reality in Jakarta EE however is that there are simply not that many people involved anymore, and from the people involved even less so at the spec level. Security has always been a topic which many people want, but few people actually contribute to. Frankly, when you go really way back to the days when Jakarta Authorization was first designed, there was very little feedback as well.

[rdebusscher]

But since there is no TCK by next week, we can't include OpenidConnect in the release, simply as that.

If you want to ship the small changes that you have done and call it 2.1, that makes sense.

[keilw]

@rdebusscher Who told you there must be a TCK now? I was in the platform call this week and also think I saw @arjantijms and while the API/spec work must be done by the end of Feb, the TCK may even be done at a later point. Are you saying you won't expect a TCK even by say April?

[rdebusscher]

Is this documented that TCK can be done at a later time? Not that familiar with Jakarta EE specifications but with MicroProfile API, Impl and TCK must be delivered together. How else can you verify the API and Impl is good and WorkingGroup members can do the vote?

[keilw]

Forget about MicroProfile, that took over a year to fix 4.x problems in the next release ;-) And of course Jakarta EE is much bigger and whenever the platform is ready (which is after the individual specs) those TCKs also should be ready, but not by the end of February which is the deadline for the individual projects.

[m-reza-rahman]

@arjantijms, would you be able to be specific about what kind of engagement, feedback and contribution you are looking for? I can try with the community and vendor folks I know. I think pretty much all the work in the Security specification is long awaited.

[arjantijms]

To start with, some engagement from people who contributed before to Jakarta Security would be good. Specifically for the authorization modules, it would be good to brainstorm a little about the best API design, and how they best correspond to the lower level modules from Jakarta Authorization.

Maybe @ggam has some opinions here? We talked about this before.

OpenID Connect is perhaps a matter of trying it out with different authentication providers and see if it covers people's API expectations well. It has a long history, as the initial version of OpenID Connect that's contributed here was created under my supervision while I was still at Payara. This was in part inspired by the needs of the ZEEF web app I worked on before. In Payara the code was updated a bit after I left, so I wasn't aware of all details anymore, but at least one actual application I know of has used that version.

So the code is in various degrees battle tested over a period of about 5 years and didn't just come falling out of thin air. Hence why I commented above that I reviewed on some of the technical specifics, and less on the feasibility of the idea (as I know, given the history, it's kinda tested already). But I didn't make this very explicit above, so my apologies for that.

[arjantijms]

But since there is no TCK by next week, we can't include OpenidConnect in the release, simply as that.

Adding to Werner's comment, there's also no deadline next week. There is a go/no go deadline at the end of February, but nothing specifically next week.

I personally have a large number of projects to look at (not the least of which is GlassFish 7), but I'll try to see if I can make some extra time free to dedicate to the Security APIs for next week.

[keilw]

@arjantijms @rdebusscher @m-reza-rahman My comment was primarily about MicroProfile being IBM's own Spring because no other company really bothers about the TCKs anymore after MP 3.5, so if the only one or two vendors that still care about MP as a strategic part of their products have everything in place that is no big surprise and all other vendors may use a few bits of it but do not care about the whole MP platform or passing the TCK. With Jakarta EE that is obviously very different given over a dozen compatible implementations from all over the world, sometimes even smaller vendors or creators of "Glassfish Distributions".

[teddyjtorres]

Greetings,

This is a request for clarifications and perhaps get an update on this spec draft.

Since the section https://github.com/jakartaee/security/blob/master/spec/src/main/asciidoc/openid.adoc#openidauthenticationmechanism refers to the HttpAuthenticationMechanism implementation, the following responses should be in terms of AuthenticationStatus instead of CredentialValidationResult,

Current:

If the call does not match the redirectURI, it must reply with a CredentialValidationResult.NOT_VALIDATED_RESULT value.

If there is no State value stored, it must reply with a CredentialValidationResult.NOT_VALIDATED_RESULT value.

If the State value on the request does not match the State value stored, it must reply with a CredentialValidationResult.INVALID_RESULT value.

If the request contains a parameter error, the authentication by the Provider failed and the authentication must reply with a CredentialValidationResult.INVALID_RESULT value.

Proposed:

If the call does not match the redirectURI, it must reply with a AuthenticationStatus.NOT_DONE value.

If there is no State value stored, it must reply with a AuthenticationStatus.NOT_DONE value.

If the State value on the request does not match the State value stored, it must reply with a AuthenticationStatus.SEND_FAILURE value.

If the request contains a parameter error, the authentication by the Provider failed and the authentication must reply with a AuthenticationStatus.SEND_FAILURE value.

What are your thoughts?

Regards, Teddy

[teddyjtorres]

Greetings,

I am reviewing the draft for implementation and the following section is problematic,

Step (3) in the OpenID Connect diagram depicted above, that is, when the OpenID Connect Provider calls us back, is detected by the authentication mechanism when a request contains a state request parameter. When that initial condition is satisfied, the following investigation and actions must be done by the authentication mechanism:

A protected resource may already use the "state" parameter for other purposes and by itself does not indicate that we are at step 3 in the OIDC flow. For example, mybank?state=NY.

The presence of a stored State would seem to indicate that we are at step 3. Whereas the lack of a stored State would indicate we are at step 1, regardless that the request uses the state request parameter.

[arjantijms]

A protected resource may already use the "state" parameter for other purposes and by itself does not indicate that we are at step 3 in the OIDC flow.

Yes, that is correct. Perhaps the wording needs to be changed a little there. The state request parameter itself is an "initial condition" for the authentication mechanism to start investigating further. Only when the other conditions are satisfied, is it really determined that we are at step 3.

Do you have a proposal for an alternative wording there?

[teddyjtorres]

Hi, Yes. Also since notifying the container with something other than CredentialValidation.VALID results in AuthenticationStatus.FAILURE per https://github.com/jakartaee/security/blob/7572f4e002b0fbc308706593c67bfa9e61f12e5d/api/src/main/java/jakarta/security/enterprise/authentication/mechanism/http/HttpMessageContext.java#:~:text=AuthenticationStatus-,notifyContainerAboutLogin,-(CredentialValidationResult%20result)%3B, and the section is about authentication mechanism's return values, the wording is simplified to use AuthenticationStatus,

Step (3) in the OpenID Connect diagram depicted above, that is, when the OpenID Connect Provider calls us back, is detected by the authentication mechanism when there is a State value stored. When that initial condition is satisfied, the following investigation and actions must be done by the authentication mechanism:

• If the request (without request parameters) does not match the redirectURI, or does not match the stored original URL (without request parameters) in case AuthenticationMechanismDefinition.redirectToOriginalResource is set to 'true', it must reply with a AuthenticationStatus.SEND_FAILURE value.
• If the State value in the request does not match the State value stored, it must reply with a AuthenticationStatus.SEND_FAILURE value.
• If the request contains a parameter error, the authentication by the OpenID Connect Provider has failed and the authentication mechanism must reply with a AuthenticationStatus.SEND_FAILURE value.