Skip to content
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

ARTEMIS-33 Generic integration with SASL Frameworks #3432

Closed
wants to merge 4 commits into from

Conversation

laeubi
Copy link

@laeubi laeubi commented Feb 5, 2021

This adds the opportunity to register new SASL schemes via the default
java service-loader mechanism.
Implementors have to provide an implementation of the ServerSASLFactory
that is responsible for providing instances of the actual scheme.

This adds the opportunity to register new SASL schemes via the default
java service-loader mechanism.
Implementors have to provide an implementation of the ServerSASLFactory
that is responsible for providing instances of the actual scheme.
@laeubi
Copy link
Author

laeubi commented Feb 5, 2021

I have prepared an example that builds up this new feature to support SASL-SCRAM.
Would be really helpful if this could go into the release next week.

Copy link
Contributor

@gtully gtully left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is great.
could you add a little test for this such that it is exercised without any external dependencies, that will protect it into the future.
Not sure how much work is in that, but maybe even verifying the mechanism is loaded via something like: org.apache.activemq.artemis.tests.integration.amqp.AmqpInboundConnectionTest

@gtully
Copy link
Contributor

gtully commented Feb 5, 2021

It can be any dummy SASL thing, just that it completes the round trip from being requested by a client to being found and loaded by the server.

@laeubi
Copy link
Author

laeubi commented Feb 5, 2021

@gtully what do you think about transforming the both defaults (PLAIN, ANONYMOUS) to the new way of providing a SASL-Mechanism? Would this suffice?

laeubi pushed a commit to laeubi/scram-sasl that referenced this pull request Feb 5, 2021
- Use the ServerSASLFactory API for default implementations also
- provide ServerSASLFactory with a precedence to order mechanisms
@laeubi
Copy link
Author

laeubi commented Feb 5, 2021

I have updated the PR but can't see why the build fails any idea?

@gemmellr
Copy link
Member

gemmellr commented Feb 5, 2021

I have updated the PR but can't see why the build fails any idea?

It seems to be unrelated, I see similar sporadically failures in a lot in other recent prior builds. Perhaps the test failure to look at before release @clebertsuconic (perhaps its even the thing you already noted looking at).

only store the value with highest precedence
cache computed array of default mechanisms
@laeubi
Copy link
Author

laeubi commented Feb 6, 2021

@gemmellr @gtully I think this can be used as a first step, just one thing I noticed (but that could be changed later also):

Even though I removed the storage of multiple providers for the same scheme it might still be valuable to restore this. I noticed that e.g. the java sasl API do it the way that they (in case of multiple ones) ask them in order and return the first one returning a non-null client/server object.
We could do a similar thing here, MechanismFinder#getFactory would then simply return a list of factories.

@asfgit asfgit closed this in 76e3592 Feb 7, 2021
@clebertsuconic
Copy link
Contributor

nice PR... ran the whole test suite with this commit pulled and it worked fine

I squashed all 4 commits into one before I merged it.

@laeubi
Copy link
Author

laeubi commented Feb 7, 2021

Thanks for all the reviews and of course the merge 👍

@jbertram
Copy link
Contributor

jbertram commented Feb 7, 2021

Thanks much for the contribution, @laeubi.

@clebertsuconic
Copy link
Contributor

yes.. thanks a lot @laeubi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants