-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Jetty 12.0.x core security #9405
Conversation
@janbartel @sbordet @lorban I've taken over @lachlan-roberts's core jetty-security work. This is still draft, but I would really appreciate some early feedback, specially on names and APIs. The plan is in this PR to get a cleaned up mechanism working with all the common login & identity services, plus the authenticators and authentications. This will be able to be used stand alone. Currently it is tested to BASIC auth and data constraints. Best place to start looking is in @janbartel yeah I know there is no javadoc. I will write it once the basic API is agreed apon. |
...ty-security/src/main/java/org/eclipse/jetty/security/authentication/DigestAuthenticator.java
Show resolved
Hide resolved
Signed-off-by: gregw <[email protected]>
Signed-off-by: gregw <[email protected]>
Signed-off-by: gregw <[email protected]>
Note I have a parallel branch jetty-12.0.x-core-security-ee9, where I am applying this to ee9, very much more a WIP... but it preserves the git history of the moved files. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gregw perhaps we need to discuss my comments, I'm sure I'm missing important details.
jetty-core/jetty-security/src/main/java/org/eclipse/jetty/security/SecurityHandler.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-ee/src/main/java/org/eclipse/jetty/ee/security/ConstraintAware.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-security/src/test/java/org/eclipse/jetty/security/SecurityHandlerTest.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-security/src/main/java/org/eclipse/jetty/security/Constraint.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-security/src/main/java/org/eclipse/jetty/security/Constraint.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-security/src/main/java/org/eclipse/jetty/security/Authenticator.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-security/src/main/java/org/eclipse/jetty/security/SecurityHandler.java
Show resolved
Hide resolved
jetty-core/jetty-security/src/main/java/org/eclipse/jetty/security/SecurityHandler.java
Show resolved
Hide resolved
jetty-core/jetty-security/src/main/java/org/eclipse/jetty/security/Authentication.java
Outdated
Show resolved
Hide resolved
@sbordet your comments are mostly clear. I agree on most of the renamings, however I might delay some until this is merged with eeX implementations, so the refactor will catch them..... or perhaps I won't and we can keep old names in the ee facades over the new names... except for ee10 which will expose the new names. I'll ponder the timing. Some of your comments are correctly pointing out complications inherited from the original implementation. I'll work to simplify to just what is needed; move to a common ee; or just remove if they really are legacy. I think I'll change the Constraint to a true Builder pattern as the fluent-like API almost works, but is surprising with regards to naming. So thanks for the review, it's pointed out more areas that need to be de-legacied and simplified. Lots to do on the plane trip :) |
jetty-core/jetty-ee/src/main/java/org/eclipse/jetty/ee/security/ConstraintAware.java
Show resolved
Hide resolved
jetty-core/jetty-ee/src/main/java/org/eclipse/jetty/ee/security/ConstraintAware.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-security/src/main/java/org/eclipse/jetty/security/Authenticator.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-security/src/main/java/org/eclipse/jetty/security/SecurityHandler.java
Show resolved
Hide resolved
jetty-core/jetty-security/src/main/java/org/eclipse/jetty/security/Authenticator.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-security/src/main/java/org/eclipse/jetty/security/AbstractLoginService.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-security/src/main/java/org/eclipse/jetty/security/AuthenticationState.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-security/src/main/java/org/eclipse/jetty/security/Authenticator.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-security/src/main/java/org/eclipse/jetty/security/SecurityHandler.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-session/src/main/java/org/eclipse/jetty/session/SessionManager.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IO.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IO.java
Outdated
Show resolved
Hide resolved
...jetty-ee10-jaspi/src/main/java/org/eclipse/jetty/ee10/security/jaspi/JaspiAuthenticator.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more nits/renaming, almost there.
*/ | ||
interface AuthConfiguration | ||
{ | ||
String getAuthMethod(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bump.
jetty-core/jetty-security/src/main/java/org/eclipse/jetty/security/Constraint.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-security/src/main/java/org/eclipse/jetty/security/Constraint.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-security/src/main/java/org/eclipse/jetty/security/IdentityService.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-security/src/main/java/org/eclipse/jetty/security/SecurityHandler.java
Show resolved
Hide resolved
public static void close(Closeable closeable) | ||
{ | ||
close((AutoCloseable)closeable); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cannot be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be able to be, but broke CI big time.
Jenkins is a mystery to me, so simplest to just leave it in:)
…rity/IdentityService.java Co-authored-by: Simone Bordet <[email protected]>
@sbordet thanks for all the reviews. I have fixed the last round and gone through and resolved the conversations that I think are fixed. There are still a few left (boms, TODOs, etc.) but I think most of those are problems already in HEAD, so I would like to go for a merge and create new issues to fix the TODOs, boms etc. So have a look at the remaining unresolved conversations and see which ones you'd like fixed before merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gregw I need one last rename, and then I'm good: Authorization.Configuration.getAuthMethod()
-> getAuthenticationName()
or similar, as long as it does not contain abbreviated "auth".
Also, there are now CI failures?
<Arg name="issuer"><Property name="jetty.openid.provider" deprecated="jetty.openid.openIdProvider"/></Arg> | ||
<Arg name="authorizationEndpoint"><Property name="jetty.openid.provider.authorizationEndpoint"/></Arg> | ||
<Arg name="tokenEndpoint"><Property name="jetty.openid.provider.tokenEndpoint"/></Arg> | ||
<Arg name="clientId"><Property name="jetty.openid.clientId"/></Arg> | ||
<Arg name="clientSecret"><Property name="jetty.openid.clientSecret"/></Arg> | ||
<Arg name="authMethod"><Property name="jetty.openid.authMethod" default="client_secret_post"/></Arg> | ||
<Arg name="authenticatorName"><Property name="jetty.openid.authenticatorName" deprecated="jetty.openid.authMethod" default="client_secret_post"/></Arg> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is right.
Elsewhere the authenticatorName
is OPENID
, while here is client_secret_post
.
This should really be the client-side mechanism to use, so it should have a different name.
Perhaps authenticationMethod
or clientAuthentication[Method|Mode]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lachlan-roberts HELP!!! what is "client_secret_post" about? Why can the openid authenticator change it's method name?
@sbordet I agree it looks strange.... but it started strange so maybe I open and issue, assign to Lachlan and we move on?
## What authenticator to use with the Token Endpoint (client_secret_post, client_secret_basic). | ||
# jetty.openid.authenticatorName=client_secret_post |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lachlan-roberts ditto
@@ -50,7 +50,7 @@ public class OpenIdConfiguration extends ContainerLifeCycle | |||
private final String clientId; | |||
private final String clientSecret; | |||
private final List<String> scopes = new ArrayList<>(); | |||
private final String authMethod; | |||
private final String authenticatorName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
This file has numerous occurrences of this to fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lachlan-roberts ditto ditto ditto ditto
@@ -11,15 +11,13 @@ | |||
// ======================================================================== | |||
// | |||
|
|||
package org.eclipse.jetty.ee9.security.openid; | |||
package org.eclipse.jetty.security.openid; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rename this file to OpenIdRealmNameTest
(not the missing l
in Ream
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
glad you pointed out the missing l. I read it 10 times and thought it was the same :)
public boolean isAuthMandatory() | ||
{ | ||
return _map.isAuthMandatory(); | ||
return _map.getAuthenticatorName(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is never used.
_map.getAuthenticatorName()
is only ever used here.
I think we can totally remove the field (and the logic) from MIMap
, and that should also allow to remove the delegation by removing MIMap
entirely, which in turn fixes the TODO present in the MIMap
class (and hopefully also fixes the use of generics for the Map?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed the unused method, but for any other jaspi cleanup, I'd prefer to punt that to a new issue/PR. I think there is a lot that needs careful thought about JASPI, so I'd prefer not to delay this PR whilst I page in what the hell JASPI is again and do a clean up.
@gregw just a reminder:
Almost there! |
This is a core security mechanism, which will ultimately be the base of each of the EEn security handlers.