-
Notifications
You must be signed in to change notification settings - Fork 463
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
[WFCORE-3963] Fix of WFCORE-3826 breaks plain authentic… #3391
[WFCORE-3963] Fix of WFCORE-3826 breaks plain authentic… #3391
Conversation
@JiriOndrusek [WFCORE-3963] is mentioned twice in commit and PR title messages |
@xstefank Bad ctrl+c, I'll fix it, thanks |
0c68bab
to
ef02212
Compare
@@ -590,7 +590,7 @@ public SaslAuthenticationFactory getSaslAuthenticationFactory(final String[] mec | |||
|
|||
for(Iterator<String> iter = requestedMechanisms.iterator();iter.hasNext();) { | |||
AuthMechanism authMechanism = toAuthMechanism("SASL", iter.next()); | |||
if(authMechanism != null) { | |||
if(authMechanism != null && (registeredServices.containsKey(authMechanism) || registeredServices.containsKey(AuthMechanism.DIGEST))) { |
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.
@JiriOndrusek Why is there a special case for DIGEST here and below? Could you explain the reason for these changes a bit? Thanks!
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.
@fjuma I admit that I'm not sure with this particular piece of code. I was trying to add new mechanisms defined by SASL_MECHANISMS. Originally I wanted use mechanism names as realm names for all types. But it wasn't possible. This is a workaround to make it working. But as I'm thinking more about it, it seems wrong.
My fix would work only if elytron is defined with digest mechanism - which may not be true always.
I have to probably rework this solution.
@darranl May I ask for your opinion?
Currently e.g. if user wants PLAIN mechanism and there is no plain mechanism defined in elytron, how should it work? (goal of this issue is to don't do any elytron configuration at all and work only with legacy one)
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 think going back to the original issue the main problem was that users previously could use this property to activate ANONYMOUS authentication.
If they are using something like LDAP they will never be able to override the config and enable DIGEST, the two are just completely incompatible.
On the other hand, I am not sure if it has been used in the past but a realm that supports DIGEST should always be compatible to support PLAIN, this should be east to verify with a test.
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.
Now I understand it correctly, (I hope):
- fix should allow anonymous - no problem
- fix should allow only these mechanisms, which are currently enabled by elytron, with exception for PLAIN which could use DIGEST (this substitution works, I've tested it)
@darranl Is my understanding correct?
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.
IMO that clarification sounds good, this is adding a property to be used with a legacy configuration - even if we could go further we should not as users would not have been able to do it previously and if a more advanced config is required nowadays they should be switching to an Elytron based configuration where they get full control of the mechanism configurations.
@fjuma I'll change fix according to result of discussion:
|
… using legacy configuration
ef02212
to
f8f8c69
Compare
@fjuma changes are commited |
…ation for ejbs using legacy configuration
Issue: https://issues.jboss.org/browse/WFCORE-3963
Test: wildfly/wildfly#11432