Elytron-security support for Database Identity Store #3654

danielpetisme · 2019-08-22T19:28:08Z

A proposal for #3586

danielpetisme · 2019-08-22T19:29:05Z

@loicmathieu I started something (only the structure for now).
Feel free to comment if you see errors.

loicmathieu · 2019-08-23T08:17:55Z

You should also add documentation to the existing guide: docs/src/main/asciidoc/security-guide.adoc

sberyozkin · 2019-08-23T11:22:39Z

extensions/elytron-security/deployment/pom.xml

+        <dependency>
+          <groupId>io.quarkus</groupId>
+          <artifactId>quarkus-agroal-deployment</artifactId>
+        </dependency>


Why is the agroal dependency needed ? Should the user application pull the quarkus-agroal instead ?

🤔 I used the flyway extension as inspiration source.... At this point, I don't know yet if elytron extension will need to manipulate the Agroal Datasource object (or if everything will be done at runtime).

You're right I should don't couple the 2 extensions).

I hope so, at runtime :-), my understanding the other DB related extensions depend on Agroal

...oyment/src/main/java/io/quarkus/elytron/security/deployment/SecurityDeploymentProcessor.java

danielpetisme · 2019-08-23T15:04:26Z

I continued the structure.

I used org.wildfly.security.auth.realm.jdbc.JdbcSecurityRealm from wildfly-elytron-realm-jdbc. So far my concern is that the builder requires a javax.sql.DataSource instance so I have to retrieve the one produced by Agroal.
I'm not sure how to do that. I tried the Producer pattern to use @Inject... I did an attemps to retrieve it directly from Arc but it seems is not the proper way (I'm still learning how Quarkus works 😄 ).

Regarding the tests I will use Flyway to init a h2 instance to test the Jdbc realm (at some point I would like to test with hibernate generating a user table based an entity).

loicmathieu · 2019-08-23T08:00:47Z

extensions/elytron-security/deployment/pom.xml

+        <dependency>
+          <groupId>io.quarkus</groupId>
+          <artifactId>quarkus-agroal-deployment</artifactId>
+        </dependency>


the dependency should be optional and you should document that for using this identity store a datasource is needed

The deployment module has a dependency to the runtime module so this dependency should not be needed here

Should it? I wouldn't mind it being opinionated.

At the moment, I flagged optional.

IMHO, in real life there are more application using DBs as Identity Store rather than files.
From a developer point of view, he/she uses a DB for authentication, I'm almost sure, It will use hibernate/ flyway somewhere in the application. These dependencies actually includes Agroal but to make authentication work the dev will have to re-declare explicitly Agroal.

I understand the reason but the dependency organization looks a bit weird to me.

For me the 2 options are:

Include Agroal by default by the the security extension
Have a dedicated extension for JDBC realm, which makes sens since I had to import the dedicated wildfly-elytron-realm-jdbc.

loicmathieu · 2019-08-23T08:02:54Z

extensions/elytron-security/runtime/pom.xml

+        <dependency>
+          <groupId>io.quarkus</groupId>
+          <artifactId>quarkus-agroal</artifactId>
+        </dependency>


the dependency should be optional and you should document that for using this identity store a datasource is needed

...tron-security/runtime/src/main/java/io/quarkus/elytron/security/runtime/JdbcRealmConfig.java

...oyment/src/main/java/io/quarkus/elytron/security/deployment/SecurityDeploymentProcessor.java

loicmathieu · 2019-08-23T15:34:53Z

@danielpetisme for retreiving the datasource, I would retrieve it from Arc.

Why do you said it's not the correct way? As your builditem is at static init time Arc should be available.

Or you should do the same that is done inside the flyway extension ...

sberyozkin · 2019-08-23T15:43:30Z

So far my concern is that the builder requires a javax.sql.DataSource instance so I have to retrieve the one produced by Agroal. I'm not sure how to do that. I tried the Producer pattern to use @Inject...

I'd expect it work as well. One thing that may have to be done is to add the Agroal producer similarly to the way it is done here but using a String option.
Something like that, otherwise please ping the forum/zulip

sberyozkin · 2019-08-23T15:47:20Z

@loicmathieu, thanks for the flyway reference :-), yea, except that I would not link it to Agroal in the code, but you are right, the optional dependency should be just enough to get that producer analyzed

danielpetisme · 2019-08-23T15:53:28Z

Thank you all for the quick review.

@loic Arc is indeed present during the build time but it does found any implementation of Datasource. I didn't investigate further, I guess the Agroal DataSource should be available at during this step.
I'll give it another try in the coming days.

danielpetisme · 2019-08-24T20:08:41Z

I found the solution: DataSourceInitializedBuildItem is a marker BuildItem to indicate the DataSource is... initialized. By consuming this build item I can retrieve a DataSource instance from the BeanContainer.

Finally I didn't used Flyway to init the database, h2 automatically loads automatically SQL resources which permits to create the basic users.

The tests are ✅ for a basic version,.
It's time to scope this PR.

I think clear passwords storage are not acceptable even for a v1. The extension must be compatible with password encryptions
It's ok to only use the default datasource.
It's ok to have only one principal-query
Attribute mapping can wait too

Note: Documentation and quickstarts are of course mandatory elements 😃
WDYT?

loicmathieu · 2019-08-25T10:08:39Z

Elytron security is used by all other security extension. So if you add the agroal extension as a mandatory extension a rest service that uses oauth and mongo will pull the agroal extension (that may need a driver! Not checked).
So two solutions. Add Agroal as mandatory or add only the needed library (jpa? ) but this will anyway add unneeded dependency...

sberyozkin · 2019-08-26T09:07:59Z

...oyment/src/main/java/io/quarkus/elytron/security/deployment/SecurityDeploymentProcessor.java

+            BuildProducer<SecurityRealmBuildItem> securityRealm,
+            BuildProducer<PasswordRealmBuildItem> passwordRealm,
+            BeanContainerBuildItem beanContainer,
+            DataSourceInitializedBuildItem dataSourceInitialized) throws Exception {


Hi, I'm not sure the agroal code has to be referenced in. What about the idea discussed earlier where you have DataSource produced by Agroal injected ? Only the integration tests module should include a non-optional agroal dependency, this module should not have any agroal dependency (or only in the test scope if the tests will be collocated), unless I'm missing something :-)

🤔 Looking at the Flyway extension which relies use a CDI injection to get the Datasource instance, the deployment module as a dependency on DataSourceInitializedBuildItem dataSourceInitialized.
https://github.com/quarkusio/quarkus/blob/master/extensions/flyway/deployment/src/main/java/io/quarkus/flyway/FlywayProcessor.java

My comprehension is this build item ensures you have an instance of a DataSource before invoking the Producer

I'm not sure to understand how you want to get rid off the agroal-deployment dependency...

@danielpetisme If I understand it correctly, in order to be able to access an instance of DataSource inside the SecurityRecorder you must add the DataSourceInitializedBuildItem dataSourceInitialized or there will not be any datasource yet in the bean factory.

So this is a build order issue ... and I'm afraid I don't know if there is a way to give a hint to the current build algorithm to execute this step after an other one.

If nobody find better solutions, I found three that all have some caveats:

Separate this in a new extension to be able to have a hard dependency to Agroal

Provide the DataSource lazily (with some kind of provider or proxy): maybe difficult to build

Move the DataSourceInitializedBuildItem to quarkus-core-deployment: the simplest one :)

Maybe you can use the third one for the moment to be able to easily finish your first implementation and discuss this point later on ?

Oh, sorry, I missed that this is how the flyway processor does it. So please continue as you agreed with Loic.
My current concern is that this security module is not only about the DB, while Flyway is, so I was hoping that simply adding the agroal dependency to the user application only which needs a DB security storage would do, but I do not know now how else to enforce that the agroal is initialized first :-), let me actually ask, I think it can be of general interest. Some work around io.quarkus.deployment.Capabilities might need to be done. But in meantime, please continue as you plan, thanks

Update I think the way to do it may be to check Capabilities, see section 2.3.1. So one would check if the Agroal capability is available, and if yes, then use Arc somehow to fetch the DataSource produced by Agroal. This option can be reviewed later though...

I propose to stick with the Agroal dependency set in the elytron-security extension to move on.
IMHO, a dedicated elytron-security-jdbc extension would be the easiest and the more explicit. The dev wouldn't have to do any further configuration.

I propose to discuss this point on zulip?

@danielpetisme yes, maybe some people will have different solution on Zulip :)
Please include my three proposals on your message on Zulip as I don't go there very often.

https://quarkusio.zulipchat.com/#narrow/stream/187030-users/topic/quarkus-elytron-security/near/174217865

danielpetisme · 2019-08-27T08:56:44Z

I noticed that I was wrong in my previous comment when I said the test are greens. It was loading the wrong application.properties.

Anyway, now I succeed to use the JdbcSecurityRealm to retrieve the user password. But when I do the evidence verification
https://github.com/wildfly-security/wildfly-elytron/blob/de462529f5c5f49e752fd70b5a72ade360fc4b24/auth/realm/jdbc/src/main/java/org/wildfly/security/auth/realm/jdbc/JdbcSecurityRealm.java the realm does not provided the providerSupliers (ie. WildflyElytronProvider).

The PasswordCredential will use the installed providers (the one provided by the JVM/OS) rather than the one provided and for instance the clear algorithm can not be found...

danielpetisme · 2019-08-28T22:31:23Z

I still have the Agroal dependency issue.
In order to ensure the readiness of a DataSource I use the DataSourceInitializedBuildItem coming from Agroal, that means the elytron-security deployment module must know Agroal dependency module... and it's useless to flag the it as optional because the elytron-security deployment module bytecode as a trace of DataSourceInitializedBuildItem, so we need it...

I tried to create the elystron-security-jdbc integration test project to test the maven dependency setup and with the above issue, elystron-security-jdbc works perfectly but elystron-security-oauth2 fails asking for Agroal... 😢

I don't know what can I test ?

Anyway, regarding the feature itself, Covering the scope of jdbc-ream was not that hard. The extension permit to use many principal-queries and user different datasources

quarkus.datasource.url=jdbc:h2:tcp://localhost/mem:multiple-data-sources-users;DB_CLOSE_DELAY=-1;DB_CLOSE_DELAY=-1;INIT=RUNSCRIPT FROM 'classpath:jdbc-realm/multiple-data-sources/users.sql'
quarkus.datasource.driver=org.h2.Driver
quarkus.datasource.username=sa
quarkus.datasource.password=sa

quarkus.datasource.permissions.url=jdbc:h2:tcp://localhost/mem:multiple-data-sources-permissions;DB_CLOSE_DELAY=-1;DB_CLOSE_DELAY=-1;INIT=RUNSCRIPT FROM 'classpath:jdbc-realm/multiple-data-sources/permissions.sql'
quarkus.datasource.permissions.driver=org.h2.Driver
quarkus.datasource.permissions.username=sa
quarkus.datasource.permissions.password=sa

quarkus.security.jdbc.enabled=true
quarkus.security.jdbc.principal-query.sql=SELECT u.password FROM test_user u WHERE u.username=?
quarkus.security.jdbc.principal-query.clear-password-mapper.enabled=true
quarkus.security.jdbc.principal-query.clear-password-mapper.password-index=1

quarkus.security.jdbc.principal-query.roles.sql=SELECT r.role_name FROM test_role r, test_user_role ur WHERE ur.username=? AND ur.role_id = r.id
quarkus.security.jdbc.principal-query.roles.datasource=permissions
quarkus.security.jdbc.principal-query.roles.attribute-mappings.0.index=1
quarkus.security.jdbc.principal-query.roles.attribute-mappings.0.to=groups

I use (and abused?) the Configuration Map. A dev must provide 1 principal-query (at least to get the password) but he/she can define named principal-queries to retrieve the user's roles for instance.

As discussed above, only proposing the "clear" password mapper was not acceptable, so I did the brcypt-password-mapper. We could easily cover them all I'll try to do it before the PR's merge.

WDYT?

loicmathieu · 2019-08-29T08:02:33Z

@danielpetisme as I understand it, build dependency is based on procuding/consuming builditem, an extension that consumes a build item will be build after one that procudes it.
So the correct use case is to consume the DataSourceInitializedBuildItem (so having it as parameter of your build method).

But, as this one lies in the agroal extension, this cause a hard dependency in it. So I suggest to move the DataSourceInitializedBuildItem to quarkus-core-deployment modue (in the io.quarkus.deployment.builditem package), this should fix the issue.

Maybe during the review of this PR someone will have a better idea but meanwhile this should works :)

danielpetisme · 2019-08-29T14:46:09Z

Since the majority of the discussion regarding the Agroal dependency inclusion has been made on Zulip. Here you have the main points:

With the current design the Capability feature is not the good option to ensure the Data Source is ready to be injected (we should rely on a the Producer/Consumer model)
The BuildItems which can be consumed by a 3rd party extension should be moved to a spi module, here you have a proposal:

extensions/
  agroal/
    dependency/ (depends on spi)
    runtime
    spi/ (provides the BuildItem definitions including`DataSourceInitializedBuildItem`)

  elyrton-security
    dependency/ (depends on agroal-spi)
    runtime
    spi/ (provides the BuildItem defintions)

The spi will be part of the current PR (in a dedicated commit) but I will focus first on the "feature" part of the PR.

danielpetisme · 2019-08-29T14:48:49Z

@loicmathieu @sberyozkin genltemens, could you review the current code (focusing on the feature provided, the configuration properties and the tests), please.

loicmathieu

Without more documentation of it I found hard to understand the configuration part that seems very complicated.
Maybe you should simplify the configuration to only what is necessary, even if wildfly propose more options.

Having to configure the index of the password and the group seems complex, maybe you can retrive them by name rather than index ?

What is the purpose of the attribute-mapping. ?

You should provides sensible default so a user can just use the extension with

quarkus.security.jdbc.enabled=true

Maybe with the query ... maybe not if you can create automatically the table ...

loicmathieu · 2019-08-29T14:59:31Z

...oyment/src/main/java/io/quarkus/elytron/security/deployment/SecurityDeploymentProcessor.java

-import java.util.Arrays;
-import java.util.List;
-import java.util.Set;
+import java.util.*;


no widlcard imports

Either eclipse-formatter or the maven formatter plugin do this optimization

loicmathieu · 2019-08-29T15:03:15Z

...n-security/deployment/src/test/java/io/quarkus/security/test/jdbc/CustomRoleDecoderTest.java

+
+import io.quarkus.test.QuarkusUnitTest;
+
+public class CustomRoleDecoderTest extends JdbcSecurityRealmTest {


CustomRole is a cross-functionality of elytron-security, not sure it's useful to test it in with the jdbc auth mechanism as it should works for all mechanism.

loicmathieu · 2019-08-29T15:05:53Z

...curity/deployment/src/test/java/io/quarkus/security/test/jdbc/ParametrizedPathsResource.java

+import javax.ws.rs.PathParam;
+
+/**
+ * @author Michal Szynkiewicz, [email protected]


Is this author tag comes from a wrong copy/paste ?

loicmathieu · 2019-08-29T15:07:45Z

...curity/deployment/src/test/java/io/quarkus/security/test/jdbc/ParametrizedPathsResource.java

+ *         Date: 6/13/18
+ */
+@Path("/parameterized-paths")
+public class ParametrizedPathsResource {


Why don't you reuse the existing io.quarkus.security.test.ParametrizedPathsResource ?

loicmathieu · 2019-08-29T15:08:03Z

...security/deployment/src/test/java/io/quarkus/security/test/jdbc/RolesEndpointClassLevel.java

+ */
+@Path("/roles-class")
+@RolesAllowed("user")
+public class RolesEndpointClassLevel {


Why don't you reuse the existing io.quarkus.security.test.RolesEndpointClassLevel ?

loicmathieu · 2019-08-29T15:09:27Z

...security/deployment/src/test/java/io/quarkus/security/test/jdbc/SubjectExposingResource.java

+ *         Date: 8/8/18
+ */
+@Path("subject")
+public class SubjectExposingResource {


Why don't you reuse the existing io.quarkus.security.test.SubjectExposingResource ?

loicmathieu · 2019-08-29T15:10:31Z

...elytron-security/deployment/src/test/java/io/quarkus/security/test/jdbc/TestApplication.java

+
+@ApplicationScoped
+@ApplicationPath("/jaxrs-secured")
+public class TestApplication extends Application {


Why don't you reuse the existing io.quarkus.security.test.TestApplication ?

loicmathieu · 2019-08-29T15:18:32Z

...ron-security/runtime/src/main/java/io/quarkus/elytron/security/runtime/SecurityRecorder.java

+
+        QueryBuilder queryBuilder = builder.principalQuery(principalQuery.sql).from(dataSource);
+
+        AttributeMapper[] mappers = principalQuery.attributeMappings.entrySet()


This seems over complicated. The only attribute that we need is the group one no?
Or do we enable a developer to load additional user information in the table and add them as attributes ?

danielpetisme · 2019-08-29T18:35:09Z

@loicmathieu providing defaults values for the principal query is tricky...

Elytron jdbc configuration is not very sexy... the configuration I propose is a 1-1 mapping
For instance, the config you found here https://docs.wildfly.org/14/WildFly_Elytron_Security.html#configure-authentication-with-a-database-identity-store

/subsystem=elytron/jdbc-realm=exampleDbRealm:add(principal-query=[{sql="SELECT password,roles FROM wildfly_users WHERE username=?",data-source=examplePostgresDS,clear-password-mapper={password-index=1},attribute-mapping=[{index=2,to=groups}]}])

would be mapped to

quarkus.security.jdbc.enabled=true
quarkus.security.jdbc.principal-query.sql=SELECT password,roles FROM wildfly_users WHERE username=?
quarkus.security.jdbc.principal-query.clear-password-mapper.enabled=true
quarkus.security.jdbc.principal-query.clear-password-mapper.password-index=1
quarkus.security.jdbc.principal-query.attribute-mappings.0.index=2
quarkus.security.jdbc.principal-query.attribute-mappings.0.to=groups

And since Elytron permits to define multiple queries, I provide the same feature.

The options I see to ease the config could be to hardcode some attribute index and names (like is made for "groups") but IMO it's would require more documentation. The other one would be to provide a intermediate layer the dev could override (define a QuarkusIdentity and a DatabaseIdentityStore#loadIdentity(String username)).

I'm open to discussion for this point

danielpetisme · 2019-08-29T18:48:58Z

Regarding the test, I think having a test case that can be run against the multiple provider is interesting so consider the jdbc test as a proposal of test refactoring.

Currently a user can have multiple roles even if in the servlet/jaxrs is not used (mon role) + users are not event used.
Finally with JDBC I can't have
INSERT INTO test_user (id, username, password, roles) VALUES (1, 'admin', 'admin', 'admin, user');
admin,user is loaded as 1 value and to create 2 roles I have to provide a CustomRoleDecoder which is over-engineering for a basic test.

loicmathieu · 2019-08-30T07:44:17Z

@danielpetisme still thinking that the configuration is complex, it's not an issue to not cover all the capabilities of an underlying library when building an extension.

In fact, the extension uses a library to ease its implementation, that's all, maybe some day we will switch to something else that elytron and when it happens having too much configuration options could complexify this migration.

For example, multiple principal queries didn't seems necessary to me at this point, even spring secyrity didn't support it !
But I would love to see a default schema being created automatically (but this could be complex so it can be done on a followup PR).

danielpetisme · 2019-08-30T08:31:55Z

I do understand.
For me, if I have to provide a default architecture:

Bcrypt mapper by default with salt=Quarkus, iteration_count=10, encoding: BASE64
The extension wait for at least 2 tables (should create them automatically?):
- QUARKUS_USER (id, username, password);
- QUARKUS_ROLE (id, username, role);
The authentication principal-query would be:
- sql: SELECT password FROM QUARKUS_USER where username=?
- bcrypt-password-mapper: index=1
The autorisation principal-query:
- sql: SELECT role FROM QUARKUS_ROLE where username=?
- attribute-mapping: index=1, to=groups

WDYT? and if the dev don't want the default behavior, he/she explicits via properties as explained above?

loicmathieu · 2019-08-30T09:46:46Z

@danielpetisme what you propose it OK for me.

For the creation of the default tables, you can open a followup issue if you don't want to implement it now.

Again, not worth it to support multiple queries now, so you should simplify the proposed configuration.

danielpetisme · 2019-09-02T20:13:56Z

@loicmathieu I have several concerns with the default properties

Given the 2 table described above, I have to do multiple queries:

1 for authentication
1 for authorization (because 1 user can have many roles)

I could do in 1 query if the user has only 1 role:

Given a table QUARKUS_USER (id, username, password, role); the query would be SELECT password, role FROM QUARKUS_USER where username=?.

IMHO, is not a realistic configuration but I can provide it by default. Quickly, dev will need to do a one-to-many join (to load many roles) and this require an additional query (Is not clean to do SELECT u.password, r.role from Users u, Roles r where u.username = r.username and u.username=?if it returns many lines).

Another point, regarding password. IMHO, Bcrypt is the norm, but the JDBC Security Realm password mappers are looking for columns index, so I can provide any default salt or iteration_count 😢 that means the password-mapper config is made by the attribute-mapping configuration.
The best I can do is to set a query to:
SELECT password_hash, salt, iteration_count FROM QUARKUS_USER where username=? with
the default values set to

quarkus.security.jdbc.principal-query.bcrypt-password-mapper.password-index=1
quarkus.security.jdbc.principal-query.bcrypt-password-mapper.salt-index=2
quarkus.security.jdbc.principal-query.bcrypt-password-mapper.iteration-count-index=3

Note: Just to compare, with Spring the password mapping is a Bean (BCryptPasswordEncoder) and the mapper configuration is not persisted in the DB (properties based with default values). In the scope of this PR, I'm not confortable with providing an intermediate Elytron with Panache layer.

Trust me, just as you I want to simplifies dev's life with a cool DX but the Elytron JDBC Realm config is not very dev friendly. Even in the doc I don't find any simple and realistic configurations...

Maybe I spent to much time on it and can't see the balance between simplicity and efficiency... At the moment I would prefer to release something and see how it's adopted. Then based on facts we could simplify the config.

WDYT?

loicmathieu · 2019-09-03T09:42:50Z

@danielpetisme if the Elytron JDBC Realm is complex, you should create some kind of abstraction layer, there is multiple customization point that can be used (in the identity manager, using a role decoder, ...).

But if you think that you made all that you think is feasible so let's stop here ;)

Maybe others will have some ideas to simplify it.

sberyozkin · 2019-09-03T10:58:01Z

@danielpetisme sorry for a delay, I was actually under impression you'd go with enhancing elytron-security only, given that the agroal spi will be available, if you have elytron-security-jdbc then the idea of spi is less interesting IMHO for the purposes of this PR, elytron-security-jdbc could just depend directly on agroal. Can you clarify please why you decided not to continue with the elytron-security path only ?

danielpetisme · 2019-09-03T12:01:58Z

@sberyozkin Sorry I don't understand. I didn't created any elytron-security-jdbc extension I worked on the current elytron-security (I did create dedicated integration test just to include/exclude agroal when the spi would be done). So not sure to understand your point.

@loicmathieu I think indeed an abstraction layer would bring some Panache 😄 but I do agree I should stop here the current PR:

provide the agroal/elytron spi
provide the technical JDBC realm integration with a 1-1 configuration mapping
the documentation

Other persons/PRs should cover the defaults values/abstraction layer.

sberyozkin · 2019-09-03T14:49:28Z

@danielpetisme you referred to elytron-security-jdbc in one of your last Zulip messages :-).
But I see now it was just a typo

gsmet · 2019-09-06T13:14:29Z

Hi,

I haven't read the code but as far as dependencies/extensions are concerned, I would avoid using optional dependencies because it's confusing for the user: he adds an extension and then has to figure out which optional dependencies he might need to get the feature working.

I think I would largely prefer if we had separate extensions for the various security realms if they rely on various dependencies to provide very specific feature e.g. have a JDBC realm extension for JDBC, a Panache extension if we want to support Panache at some point.

OK, it's a bit more verbose and requires to create new extensions but at least it's very clear for the users and just adding the extension makes it work properly.

danielpetisme · 2019-09-06T16:26:00Z

@gsmet request makes sense to me. Plus, it solves without the dependency issue without refactoring the current external extensions.

danielpetisme · 2019-09-09T19:29:46Z

Up? Ok for everyone I can create a dedicated extension?

sberyozkin · 2019-09-09T21:25:48Z

Hi @danielpetisme, @gsmet,
But with the spi idea we all discussed at Zulip the optional dependencies are not needed unless I'm missing something ? My understanding elytron-security would link with the agroal-spi for the DataSourceInitializedBuildItem be available, while DataSource will be injected.

elytron-security-jdbc - it would still have to depend on elytron-security and thus the same file and properties providers will be available with elytron-security-jdbc unless elytron-security-common is introduced, may be it should be done if elytron-security-jdbc were to go ahead to have a clean separation indeed ?

thanks

sberyozkin · 2019-09-10T16:10:46Z

Or do you mean that the users would have to add the agroal extension for the elytron-security be able to offer support the JDBC store ? Yeah, it can be of some concern, but I suppose it will be documented.
We are discussing the same or very similar problem with Vincent who is doing the Vault PR, agroal can use the Vault to fetch the DB passwords. The PR has agroal tightly coupled with vault. So the idea we are discussing is that a light weight vault-spi is introduced and agroal depends on this vault-spi only and the users would have to add a vault dependency if they want agroal be able to use vault. The other option is to introduce an agroal-vault module, but I'm not sure about it.
As far as this PR is concerned, elytron-security-jdbc may work well, but I guess for it to work really well, elytron-security-common can be introduced too, so that smallrye-jwt, elytron-security-oauth2 and a new elytron-security-jdbc all start depending on it, and do not have an implicit support for the file and properties stores which they will never need :-).
Loic suggested to have elytron-security-jdbc earlier, similarly to elytron-security-oauth2, I thought initially it could be squeezed into the existing elytron-security because -jdbc does not introduce a new type of flow, compared to -oauth2

Thanks, Sergey

danielpetisme · 2019-09-10T16:36:48Z

A -common + flavors (jdbc, oauth2, etc) makes totally sense for me and would be the more explicit way for a dev to express what he/she wants to do. Plus, it allows each module to have its own release roadmap.
Now that you have my opinion, you are the ones who will support all these crazy ideas in the future (I'm an external contrib) so I do prefer to have your approval on one direction or another.

sberyozkin · 2019-09-11T11:18:25Z

Hi @danielpetisme, as far as I'm concerned, I'll support either the original option or the elytron-security-jdbc way.
I believe the latter option is preferred by Loic and Guillaume as well. So please go ahead with the dedicated module approach - the elytron-security-common can be created at the last moment once everything else is ready to go otherwise or may be even as a follow up PR. There will be no need for the agroal-spi in this case though if I understand it correctly.
Thanks

sberyozkin · 2019-09-17T11:26:39Z

Hi @danielpetisme how are you, I CC-ed to you on Zulip, the users are asking, so the DB store support would be welcome and quite timely :-). Let us know please how it goes.
P.S. And let me know if the symmetric signature support is needed in smallrye-jwt for your Quarkus migration project work as expected, I'll do it anyway in smallrye-jwt but not sure how urgent it should be.

danielpetisme · 2019-09-17T21:35:07Z

Awesome if someone else feel the need.

Regarding the refactoring per-se, I'll try to propose something before the end of this week (struggling with time).

Regarding, smallrye-jwt I'll wait to close this PR before working again on the Quarkus migration project (I like to finish stuff rather working in too much topics in //)
Anyway, thanks for your support

sberyozkin · 2019-09-19T15:45:19Z

@danielpetisme sounds good, thanks

danielpetisme · 2019-09-26T21:59:30Z

As discussed I made a dedicate elytron-security-jdbc extension.
The main concern I have is with BeanContainerBuildItem. I used to consume it to be sure the CDI was ready before using Arc in the Recorder.
With the dedicated extension, a cycle looks to appear

[ERROR] Errors: 
[ERROR]   java.lang.RuntimeException: io.quarkus.builder.ChainBuildException: Cycle detected:
                   io.quarkus.arc.deployment.ArcProcessor#generateResources produced class io.quarkus.arc.deployment.BeanContainerBuildItem
                to io.quarkus.elytron.security.jdbc.deployment.ElytronSecurityJdbcProcessor#configureJdbcRealmAuthConfig produced class io.quarkus.elytron.security.deployment.SecurityRealmBuildItem
                to io.quarkus.elytron.security.deployment.SecurityDeploymentProcessor#build produced class io.quarkus.arc.deployment.AdditionalBeanBuildItem
                to io.quarkus.arc.deployment.BeanArchiveProcessor#build produced class io.quarkus.arc.deployment.BeanArchiveIndexBuildItem
                to io.quarkus.arc.deployment.AnnotatedUnremovableClassesProcessor#findAnnotatedClasses produced class io.quarkus.arc.deployment.UnremovableBeanBuildItem
                to io.quarkus.arc.deployment.ArcProcessor#initialize produced class io.quarkus.arc.deployment.ContextRegistrationPhaseBuildItem
                to io.quarkus.arc.deployment.ArcProcessor#registerBeans produced class io.quarkus.arc.deployment.BeanRegistrationPhaseBuildItem
                to io.quarkus.arc.deployment.ArcProcessor#validate produced class io.quarkus.arc.deployment.ValidationPhaseBuildItem
                to io.quarkus.arc.deployment.ArcProcessor#generateResources
[ERROR]   CustomRoleDecoderTest » Runtime java.lang.RuntimeException: Unable to create t...
[ERROR]   java.lang.RuntimeException: io.quarkus.builder.ChainBuildException: Cycle detected:
                   io.quarkus.arc.deployment.BeanArchiveProcessor#build produced class io.quarkus.arc.deployment.BeanArchiveIndexBuildItem
                to io.quarkus.arc.deployment.AnnotatedUnremovableClassesProcessor#findAnnotatedClasses produced class io.quarkus.arc.deployment.UnremovableBeanBuildItem
                to io.quarkus.arc.deployment.ArcProcessor#initialize produced class io.quarkus.arc.deployment.ContextRegistrationPhaseBuildItem
                to io.quarkus.arc.deployment.ArcProcessor#registerBeans produced class io.quarkus.arc.deployment.BeanRegistrationPhaseBuildItem
                to io.quarkus.arc.deployment.ConfigBuildStep#analyzeConfigPropertyInjectionPoints produced class io.quarkus.arc.deployment.BeanRegistrationPhaseBuildItem$BeanConfiguratorBuildItem
                to io.quarkus.arc.deployment.ArcProcessor#validate produced class io.quarkus.arc.deployment.ValidationPhaseBuildItem
                to io.quarkus.arc.deployment.ArcProcessor#generateResources produced class io.quarkus.arc.deployment.BeanContainerBuildItem
                to io.quarkus.elytron.security.jdbc.deployment.ElytronSecurityJdbcProcessor#configureJdbcRealmAuthConfig produced class io.quarkus.elytron.security.deployment.SecurityRealmBuildItem
                to io.quarkus.elytron.security.deployment.SecurityDeploymentProcessor#build produced class io.quarkus.arc.deployment.AdditionalBeanBuildItem
                to io.quarkus.arc.deployment.BeanArchiveProcessor#build
[ERROR]   java.lang.RuntimeException: io.quarkus.builder.ChainBuildException: Cycle detected:
                   io.quarkus.elytron.security.deployment.SecurityDeploymentProcessor#build produced class io.quarkus.arc.deployment.AdditionalBeanBuildItem
                to io.quarkus.arc.deployment.ArcProcessor#initialize produced class io.quarkus.arc.deployment.ContextRegistrationPhaseBuildItem
                to io.quarkus.arc.deployment.ArcProcessor#registerBeans produced class io.quarkus.arc.deployment.BeanRegistrationPhaseBuildItem
                to io.quarkus.arc.deployment.ArcProcessor#validate produced class io.quarkus.arc.deployment.ValidationPhaseBuildItem
                to io.quarkus.arc.deployment.ArcProcessor#generateResources produced class io.quarkus.arc.deployment.BeanContainerBuildItem
                to io.quarkus.elytron.security.jdbc.deployment.ElytronSecurityJdbcProcessor#configureJdbcRealmAuthConfig produced class io.quarkus.elytron.security.deployment.SecurityRealmBuildItem
                to io.quarkus.elytron.security.deployment.SecurityDeploymentProcessor#build
[ERROR]   java.lang.RuntimeException: io.quarkus.builder.ChainBuildException: Cycle detected:
                   io.quarkus.elytron.security.jdbc.deployment.ElytronSecurityJdbcProcessor#configureJdbcRealmAuthConfig produced class io.quarkus.elytron.security.deployment.SecurityRealmBuildItem
                to io.quarkus.elytron.security.deployment.SecurityDeploymentProcessor#build produced class io.quarkus.arc.deployment.AdditionalBeanBuildItem
                to io.quarkus.arc.deployment.BeanArchiveProcessor#build produced class io.quarkus.arc.deployment.BeanArchiveIndexBuildItem
                to io.quarkus.arc.deployment.ArcProcessor#initialize produced class io.quarkus.arc.deployment.ContextRegistrationPhaseBuildItem
                to io.quarkus.arc.deployment.ArcProcessor#registerBeans produced class io.quarkus.arc.deployment.BeanRegistrationPhaseBuildItem
                to io.quarkus.arc.deployment.ArcProcessor#validate produced class io.quarkus.arc.deployment.ValidationPhaseBuildItem
                to io.quarkus.arc.deployment.ArcProcessor#generateResources produced class io.quarkus.arc.deployment.BeanContainerBuildItem
                to io.quarkus.elytron.security.jdbc.deployment.ElytronSecurityJdbcProcessor#configureJdbcRealmAuthConfig

To workaround . the problem, I remove the build item and I've added an explicit Arc init
https://github.com/quarkusio/quarkus/pull/3654/files#diff-ca355d8390b3ab8b6d5613f0de12e972R42
But I'm not confortable with this solution. WDYT?

So far the tests fail but I think it's just a matter of time before spotting the problem.
Stay tuned.

sberyozkin · 2019-09-27T14:08:55Z

@danielpetisme thanks a lot; FYI, I think in some cases Arc is used directly depending on how early the bean is required.
@stuartwdouglas FYI

danielpetisme · 2019-09-27T16:18:49Z

If I explicitly ask for a BeanContainerBuildItem I get a cycle.

If I don't ask for it, I got an NPE when invoking Arc.container().
My workaround is no BuildContainterBuildItem + Arc.initialize

I don't have a clue of what is the best option

sberyozkin · 2019-09-27T20:05:01Z

@mkouba, @gsmet Hi Martin, Guillaume, can you please advise @danielpetisme how to address it ?

stuartwdouglas · 2019-09-30T05:23:09Z

This needs to be formatted. The runtime module also needs to depend on quarkus-elytron-security (and the moment this dependency is only in the deployment module). Not having this dep will cause all kinds of problems.

stuartwdouglas · 2019-09-30T08:08:28Z

I have taken this PR and have attempted to get everything passing at #4273

danielpetisme · 2019-09-30T15:03:29Z

@stuartwdouglas since you took control of the PR, should we close this one?
For my understanding and learnings, except the dependency issue do you saw any other big concerns I missed? Thanks

stuartwdouglas · 2019-10-01T04:02:42Z

The main issues were:

Dependencies were incorrect, you need to make sure that the deployment and runtime artifacts depend on the same extensions or else you can get inconsistent behaviour.
I had removed the RoleDecoder stuff in an earlier PR as nothing appeared to be using it, so I needed to add it back
There was a bug in how the executor was being handled for blocking auth tasks

I need to spent a little bit more time going through it today, but on the whole it looks really good. I only took control because we have a very tight schedule to get security in for the 1.0 release so anything security related is likely going to be fast tracked.

sberyozkin reviewed Aug 23, 2019

View reviewed changes

...oyment/src/main/java/io/quarkus/elytron/security/deployment/SecurityDeploymentProcessor.java Outdated Show resolved Hide resolved

danielpetisme force-pushed the feat/db-identity-store-for-elytron-security branch from 0ec6217 to ca03d15 Compare August 23, 2019 14:57

loicmathieu reviewed Aug 23, 2019

View reviewed changes

danielpetisme force-pushed the feat/db-identity-store-for-elytron-security branch from ca03d15 to ef9f581 Compare August 24, 2019 18:38

sberyozkin reviewed Aug 26, 2019

View reviewed changes

danielpetisme force-pushed the feat/db-identity-store-for-elytron-security branch 2 times, most recently from 9d70a0c to a24c3f7 Compare August 28, 2019 22:13

loicmathieu requested changes Aug 29, 2019

View reviewed changes

Refactoring

580e51d

danielpetisme force-pushed the feat/db-identity-store-for-elytron-security branch 3 times, most recently from c7a354f to 580e51d Compare September 26, 2019 21:53

machi1990 mentioned this pull request Sep 30, 2019

Add Elytron security JDBC extension #4273

Merged

stuartwdouglas closed this Oct 1, 2019

gsmet added the triage/invalid This doesn't seem right label Oct 8, 2019


		import io.quarkus.test.QuarkusUnitTest;

		public class CustomRoleDecoderTest extends JdbcSecurityRealmTest {


		QueryBuilder queryBuilder = builder.principalQuery(principalQuery.sql).from(dataSource);

		AttributeMapper[] mappers = principalQuery.attributeMappings.entrySet()

Elytron-security support for Database Identity Store #3654

Elytron-security support for Database Identity Store #3654

Conversation

danielpetisme commented Aug 22, 2019

danielpetisme commented Aug 22, 2019

loicmathieu commented Aug 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielpetisme commented Aug 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loicmathieu commented Aug 23, 2019

sberyozkin commented Aug 23, 2019

sberyozkin commented Aug 23, 2019 • edited Loading

danielpetisme commented Aug 23, 2019

danielpetisme commented Aug 24, 2019 • edited Loading

loicmathieu commented Aug 25, 2019

sberyozkin Aug 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sberyozkin Aug 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielpetisme commented Aug 27, 2019 • edited Loading

danielpetisme commented Aug 28, 2019

loicmathieu commented Aug 29, 2019

danielpetisme commented Aug 29, 2019

danielpetisme commented Aug 29, 2019

loicmathieu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielpetisme commented Aug 29, 2019

danielpetisme commented Aug 29, 2019

loicmathieu commented Aug 30, 2019

danielpetisme commented Aug 30, 2019

loicmathieu commented Aug 30, 2019

danielpetisme commented Sep 2, 2019

loicmathieu commented Sep 3, 2019

sberyozkin commented Sep 3, 2019

danielpetisme commented Sep 3, 2019

sberyozkin commented Sep 3, 2019 • edited Loading

gsmet commented Sep 6, 2019

danielpetisme commented Sep 6, 2019

danielpetisme commented Sep 9, 2019

sberyozkin commented Sep 9, 2019

sberyozkin commented Sep 10, 2019

danielpetisme commented Sep 10, 2019

sberyozkin commented Sep 11, 2019

sberyozkin commented Sep 17, 2019

danielpetisme commented Sep 17, 2019

sberyozkin commented Sep 19, 2019

danielpetisme commented Sep 26, 2019

sberyozkin commented Sep 27, 2019

danielpetisme commented Sep 27, 2019

sberyozkin commented Sep 27, 2019

stuartwdouglas commented Sep 30, 2019

stuartwdouglas commented Sep 30, 2019

danielpetisme commented Sep 30, 2019

stuartwdouglas commented Oct 1, 2019

sberyozkin commented Aug 23, 2019 •

edited

Loading

danielpetisme commented Aug 24, 2019 •

edited

Loading

sberyozkin Aug 26, 2019 •

edited

Loading

sberyozkin Aug 27, 2019 •

edited

Loading

danielpetisme commented Aug 27, 2019 •

edited

Loading

sberyozkin commented Sep 3, 2019 •

edited

Loading