Deployment should fail if annotations contain invalid attributes

[glassfishrobot]

As confirmed by Will (https://javaee.groups.io/g/javaee-security-spec/message/341), identity stores with invalid values should fail the deployment. The same applies for authentication mechanisms.

I'll start to work on this later today.

[glassfishrobot]

• Issue Imported From: https://github.com/javaee/security-soteria/issues/104
• Original Issue Raised By:@ggam
• Original Issue Assigned To: Unassigned

[glassfishrobot]

@rdebusscher Commented Hi Guillermo,

That is unfortunately not always possible.

For example, when you use the LDAP option with direct bind, you can only verify if the LDAP url is valid by making a call with user credentials. Which, of course, you don't have at deploy time.

So when we specify that values are always validated, expect in case X, Y and Z it just confused the developer.

best regards Rudy

On 10 July 2017 at 15:12, Guillermo González de Agüero < [email protected]> wrote:

As confirmed by Will (https://javaee.groups.io/g/ javaee-security-spec/message/341), identity stores with invalid values should fail the deployment. The same applies for authentication mechanisms.

I'll start to work on this later today.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/javaee-security-spec/soteria/issues/104, or mute the thread https://github.com/notifications/unsubscribe-auth/ABK96Yh4dmSF8ndegFhr6bH7AynTPXroks5sMiMlgaJpZM4OS0Pd .

[glassfishrobot]

@ggam Commented Hi Rudy, interesting point. I don't really have experience with LDAP to be honest.

My idea, as I suggested on the mailing list (https://javaee.groups.io/g/javaee-security-spec/topic/validation_of_identity_store/5457689?p=,,,20,0,0,0::recentpostdate%2Fsticky,,,20,2,0,5457689) is that the deployment should fail only when there are no posibilities for the identity store/authentication mechanism to work.

A database might be unavailable at deployment time, but if the JNDI lookup succeeds, it should be valid for the container. But if the callerQuery attribute is empty and validationType is set to VALIDATE, then we know for sure that it won't work at runtime and as a user, I'd expect to get an error.

That's specially important since there are a lot of attributes that are only required on certain scenarios (callerQuery for example).

[glassfishrobot]

@rdebusscher Commented Hi Guillermo,

Ok, that are more basic checks then I had in mind.

Yes, they can help to identify some mistakes or forgotten configurations.

Thanks for taking care of it.

Best regards Rudy

On 10 July 2017 at 16:10, Guillermo González de Agüero < [email protected]> wrote:

Hi Rudy, interesting point. I don't really have experience with LDAP to be honest.

My idea, as I suggested on the mailing list (https://javaee.groups.io/g/ javaee-security-spec/topic/validation_of_identity_store/ 5457689?p=,,,20,0,0,0::recentpostdate%2Fsticky,,,20,2,0,5457689) is that the deployment should fail only when there are no posibilities for the identity store/authentication mechanism to work.

A database might be unavailable at deployment time, but if the JNDI lookup succeeds, it should be valid for the container. But if the callerQuery attribute is empty and validationType is set to VALIDATE, then we know for sure that it won't work at runtime and as a user, I'd expect to get an error.

That's specially important since there are a lot of attributes that are only required on certain scenarios (callerQuery for example).

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/javaee-security-spec/soteria/issues/104#issuecomment-314117228, or mute the thread https://github.com/notifications/unsubscribe-auth/ABK96SRARdGI4e6zYGiOyeZc3BsnquXfks5sMjDSgaJpZM4OS0Pd .

[glassfishrobot]

@wmhopkins Commented That's a good point. Implementations should probably try to fail as soon as they can on bad config, but that's probably just good programming practice, not something we can/should require by spec.

On 07/10/2017 09:26 AM, Rudy De Busscher wrote:

Hi Guillermo,

That is unfortunately not always possible.

For example, when you use the LDAP option with direct bind, you can only verify if the LDAP url is valid by making a call with user credentials. Which, of course, you don't have at deploy time.

So when we specify that values are always validated, expect in case X, Y and Z it just confused the developer.

best regards Rudy

On 10 July 2017 at 15:12, Guillermo González de Agüero < [email protected]> wrote:

As confirmed by Will (https://javaee.groups.io/g/ javaee-security-spec/message/341), identity stores with invalid values should fail the deployment. The same applies for authentication mechanisms.

I'll start to work on this later today.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/javaee-security-spec/soteria/issues/104, or mute the thread

https://github.com/notifications/unsubscribe-auth/ABK96Yh4dmSF8ndegFhr6bH7AynTPXroks5sMiMlgaJpZM4OS0Pd

.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/javaee-security-spec/soteria/issues/104#issuecomment-314104653, or mute the thread https://github.com/notifications/unsubscribe-auth/AKfOXs4sQVEbg_gw2OY1e8HUwEHfLg13ks5sMiZ-gaJpZM4OS0Pd.

-- Will Hopkins | WebLogic Security Architect | +1.781.442.0310 Oracle Application Development 35 Network Drive, Burlington, MA 01803

[glassfishrobot]

@ggam Commented Deployments fail by spec when a Servlet has an invalid url pattern, or when a bean dependency is unsatisfied. That's (at least for me) the main benefit of the deployments procress, the application is validated before runtime.

If we don't mandate these checks to be done at deployment time, users may face situations where applications deploy correctly but are unusable in practice.

The situation here is more difficult than on Servlets and CDI beans due to its dynamic nature, as Rudy pointed out. That's why I proposed a somewhat vague wording, leaving to interpretation what cases should fail the deployment. But mandating the deployment to fail it the container knows for sure that it won't work.

El lun., 10 de julio de 2017 18:01, Will Hopkins [email protected] escribió:

That's a good point. Implementations should probably try to fail as soon as they can on bad config, but that's probably just good programming practice, not something we can/should require by spec.

On 07/10/2017 09:26 AM, Rudy De Busscher wrote:

Hi Guillermo,

That is unfortunately not always possible.

For example, when you use the LDAP option with direct bind, you can only verify if the LDAP url is valid by making a call with user credentials. Which, of course, you don't have at deploy time.

So when we specify that values are always validated, expect in case X, Y and Z it just confused the developer.

best regards Rudy

On 10 July 2017 at 15:12, Guillermo González de Agüero < [email protected]> wrote:

As confirmed by Will (https://javaee.groups.io/g/ javaee-security-spec/message/341), identity stores with invalid values should fail the deployment. The same applies for authentication mechanisms.

I'll start to work on this later today.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/javaee-security-spec/soteria/issues/104, or mute the thread

< https://github.com/notifications/unsubscribe-auth/ABK96Yh4dmSF8ndegFhr6bH7AynTPXroks5sMiMlgaJpZM4OS0Pd

.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub < https://github.com/javaee-security-spec/soteria/issues/104#issuecomment-314104653>,

or mute the thread < https://github.com/notifications/unsubscribe-auth/AKfOXs4sQVEbg_gw2OY1e8HUwEHfLg13ks5sMiZ-gaJpZM4OS0Pd .

-- Will Hopkins | WebLogic Security Architect | +1.781.442.0310 Oracle Application Development 35 Network Drive, Burlington, MA 01803

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/javaee-security-spec/soteria/issues/104#issuecomment-314152048, or mute the thread https://github.com/notifications/unsubscribe-auth/ACAucDHB4fPa9hnHMCRKB_cZSdT8vsh1ks5sMkrmgaJpZM4OS0Pd .

[glassfishrobot]

@arjantijms Commented Slightly offtopic, but this in a way is an argument for annotated annotation attributes, so e.g. bean validation can be used on them.

E.g.

@Retention(RUNTIME)
@Target(TYPE)
public @interface DatabaseIdentityStoreDefinition {

    @NonEmpty
    @ValidELExpression
    String dataSourceLookup() default "java:comp/DefaultDataSource"; // default data source

    @NonEmpty
    @ValidELExpression
    String callerQuery() default "";

    @NonEmpty
    @ValidELExpression
    String groupsQuery() default "";

For non-empty one can of course start with removing the default empty value, making the attribute mandatory.

[glassfishrobot]

@wmhopkins Commented It might be reasonable to specify what a bean should do during initialization if it determines that it's incorrectly configured or otherwise unable to perform its function.

Any thoughts on what's reasonable hear, given that the beans are all instantiated via CDI? What's the correct, idiomatic CDI way to fail?

On 07/10/2017 01:47 PM, Guillermo González de Agüero wrote:

Deployments fail by spec when a Servlet has an invalid url pattern, or when a bean dependency is unsatisfied. That's (at least for me) the main benefit of the deployments procress, the application is validated before runtime.

If we don't mandate these checks to be done at deployment time, users may face situations where applications deploy correctly but are unusable in practice.

The situation here is more difficult than on Servlets and CDI beans due to its dynamic nature, as Rudy pointed out. That's why I proposed a somewhat vague wording, leaving to interpretation what cases should fail the deployment. But mandating the deployment to fail it the container knows for sure that it won't work.

El lun., 10 de julio de 2017 18:01, Will Hopkins [email protected] escribió:

That's a good point. Implementations should probably try to fail as soon as they can on bad config, but that's probably just good programming practice, not something we can/should require by spec.

On 07/10/2017 09:26 AM, Rudy De Busscher wrote:

Hi Guillermo,

That is unfortunately not always possible.

For example, when you use the LDAP option with direct bind, you can only verify if the LDAP url is valid by making a call with user credentials. Which, of course, you don't have at deploy time.

So when we specify that values are always validated, expect in case X, Y and Z it just confused the developer.

best regards Rudy

On 10 July 2017 at 15:12, Guillermo González de Agüero < [email protected]> wrote:

As confirmed by Will (https://javaee.groups.io/g/ javaee-security-spec/message/341), identity stores with invalid values should fail the deployment. The same applies for authentication mechanisms.

I'll start to work on this later today.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/javaee-security-spec/soteria/issues/104, or mute the thread

<

https://github.com/notifications/unsubscribe-auth/ABK96Yh4dmSF8ndegFhr6bH7AynTPXroks5sMiMlgaJpZM4OS0Pd

.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <

https://github.com/javaee-security-spec/soteria/issues/104#issuecomment-314104653>,

or mute the thread <

https://github.com/notifications/unsubscribe-auth/AKfOXs4sQVEbg_gw2OY1e8HUwEHfLg13ks5sMiZ-gaJpZM4OS0Pd

.

-- Will Hopkins | WebLogic Security Architect | +1.781.442.0310 Oracle Application Development 35 Network Drive, Burlington, MA 01803

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub

https://github.com/javaee-security-spec/soteria/issues/104#issuecomment-314152048,

or mute the thread

https://github.com/notifications/unsubscribe-auth/ACAucDHB4fPa9hnHMCRKB_cZSdT8vsh1ks5sMkrmgaJpZM4OS0Pd

.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/javaee-security-spec/soteria/issues/104#issuecomment-314182098, or mute the thread https://github.com/notifications/unsubscribe-auth/AKfOXsx7JrWlrs8-DOeLFuk19B5hXG_nks5sMmPMgaJpZM4OS0Pd.

-- Will Hopkins | WebLogic Security Architect | +1.781.442.0310 Oracle Application Development 35 Network Drive, Burlington, MA 01803

[glassfishrobot]

@rdebusscher Commented CDI beans can perform some initialization in the method annotated with @PostConstruct. That seems the natural place to perform such actions.

Only thing is that this happens when the bean is instantiated, and thus not during deploy time.

When it needs to be done during deploy time, it must be performed within the CDI extension.

What is sensible? A situation like the callerQuery attribute is empty and validationType is set to VALIDATE, (like given as example) seems very reasonable. But I'm afraid we need to specify those validation sets within the spec or JavaDoc. Otherwise the different implementations don't know what is reasonable for us.

On 10 July 2017 at 20:34, Will Hopkins [email protected] wrote:

It might be reasonable to specify what a bean should do during initialization if it determines that it's incorrectly configured or otherwise unable to perform its function.

Any thoughts on what's reasonable hear, given that the beans are all instantiated via CDI? What's the correct, idiomatic CDI way to fail?

On 07/10/2017 01:47 PM, Guillermo González de Agüero wrote:

Deployments fail by spec when a Servlet has an invalid url pattern, or when a bean dependency is unsatisfied. That's (at least for me) the main benefit of the deployments procress, the application is validated before runtime.

If we don't mandate these checks to be done at deployment time, users may face situations where applications deploy correctly but are unusable in practice.

The situation here is more difficult than on Servlets and CDI beans due to its dynamic nature, as Rudy pointed out. That's why I proposed a somewhat vague wording, leaving to interpretation what cases should fail the deployment. But mandating the deployment to fail it the container knows for sure that it won't work.

El lun., 10 de julio de 2017 18:01, Will Hopkins [email protected] escribió:

That's a good point. Implementations should probably try to fail as soon as they can on bad config, but that's probably just good programming practice, not something we can/should require by spec.

On 07/10/2017 09:26 AM, Rudy De Busscher wrote:

Hi Guillermo,

That is unfortunately not always possible.

For example, when you use the LDAP option with direct bind, you can only verify if the LDAP url is valid by making a call with user credentials. Which, of course, you don't have at deploy time.

So when we specify that values are always validated, expect in case X, Y and Z it just confused the developer.

best regards Rudy

On 10 July 2017 at 15:12, Guillermo González de Agüero < [email protected]> wrote:

As confirmed by Will (https://javaee.groups.io/g/ javaee-security-spec/message/341), identity stores with invalid values should fail the deployment. The same applies for authentication mechanisms.

I'll start to work on this later today.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/javaee-security-spec/soteria/issues/104, or mute the thread

<

https://github.com/notifications/unsubscribe-auth/ ABK96Yh4dmSF8ndegFhr6bH7AynTPXroks5sMiMlgaJpZM4OS0Pd

.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <

https://github.com/javaee-security-spec/soteria/issues/ 104#issuecomment-314104653>,

or mute the thread <

https://github.com/notifications/unsubscribe-auth/AKfOXs4sQVEbg_ gw2OY1e8HUwEHfLg13ks5sMiZ-gaJpZM4OS0Pd

.

-- Will Hopkins | WebLogic Security Architect | +1.781.442.0310 Oracle Application Development 35 Network Drive, Burlington, MA 01803

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub

<https://github.com/javaee-security-spec/soteria/issues/ 104#issuecomment-314152048>,

or mute the thread

<https://github.com/notifications/unsubscribe- auth/ACAucDHB4fPa9hnHMCRKB_cZSdT8vsh1ks5sMkrmgaJpZM4OS0Pd>

.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub <https://github.com/javaee-security-spec/soteria/issues/ 104#issuecomment-314182098>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKfOXsx7JrWlrs8- DOeLFuk19B5hXG_nks5sMmPMgaJpZM4OS0Pd>.

-- Will Hopkins | WebLogic Security Architect | +1.781.442.0310 Oracle Application Development 35 Network Drive, Burlington, MA 01803

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/javaee-security-spec/soteria/issues/104#issuecomment-314195423, or mute the thread https://github.com/notifications/unsubscribe-auth/ABK96UpJShfD_qwbAE2HQgat6dyUBjsoks5sMm6kgaJpZM4OS0Pd .

[glassfishrobot]

@wmhopkins Commented Is that something that exists, or something you're suggesting should exist?

On 07/10/2017 01:52 PM, Arjan Tijms wrote:

Slightly offtopic, but this in a way is an argument for annotated annotation attributes, so e.g. bean validation can be used on them.

E.g.

@Retention(RUNTIME) @Target(TYPE) public @interface DatabaseIdentityStoreDefinition {

 @NonEmpty
 @ValidELExpression
 String  dataSourceLookup() default "java:comp/DefaultDataSource";// default data source

 @NonEmpty
 @ValidELExpression
 String  callerQuery() default "";

 @NonEmpty
 @ValidELExpression
 String  groupsQuery() default "";

For non-empty one can of course start with removing the default empty value, making the attribute mandatory.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/javaee-security-spec/soteria/issues/104#issuecomment-314183327, or mute the thread https://github.com/notifications/unsubscribe-auth/AKfOXtcjf8QVfVPgjXoaxal7UAVfWKylks5sMmTVgaJpZM4OS0Pd.

-- Will Hopkins | WebLogic Security Architect | +1.781.442.0310 Oracle Application Development 35 Network Drive, Burlington, MA 01803

[glassfishrobot]

@arjantijms Commented

Any thoughts on what's reasonable hear, given that the beans are all instantiated via CDI? What's the correct, idiomatic CDI way to fail?

Unresolvable or ambiguous injections cause a deployment exception to be thrown.

In case of validating EL expressions, it's not so easy really. The only sure way to validate them is by evaluating them fully, but evaluating has side-effects (beans get created, application code runs, etc).

In think the most realistic thing to validate is during AfterDeploymentValidation to see if expressions are syntactical correct (can be parsed), and IF there are one of more bases, check if named beans exists for those (but don't try to actually instantiate them).

As an examplewhy this is not trivial, consider an expression like:

#{bean1.bean2.foo('abc.contact('xyz')).bar or bean3.bar(123 * 456)}

Checking for a valid syntax could be done. E.g. the following is invalid (unknown operator "orr"):

#{bean1.bean2.foo('abc.contact('xyz')).bar orr bean3.bar(123 * 456)}

[glassfishrobot]

@arjantijms Commented

Is that something that exists, or something you're suggesting should exist?

I'm not 100% sure what you're asking here, but @PostConstruct is an existing annotation and obviously exists, but I think you're asking somewhat else?

Beans can do an amount of validation there, but in our case we're often calling a setter on a bean even after @PostConstruct, so even that would not work really.

E.g.

authenticationMechanismBean = new CdiProducer<HttpAuthenticationMechanism>()
                    .scope(ApplicationScoped.class)
                    .beanClass(HttpAuthenticationMechanism.class)
                    .types(Object.class, HttpAuthenticationMechanism.class)
                    .addToId(FormAuthenticationMechanismDefinition.class)
                    .create(e -> 
                             CDI.current()
                                .select(FormAuthenticationMechanism.class)
                                .get()
                                .loginToContinue(
                                    LoginToContinueAnnotationLiteral.eval(
                                        formAuthenticationMechanismDefinition.loginToContinue()))
                    );

[glassfishrobot]

@arjantijms Commented

When it needs to be done during deploy time, it must be performed within the CDI extension.

Indeed. The CDI extension does call eval on all the annotation literals corresponding to the annotations used for beans (not interceptors), so that may be a good place to put validation checks.

[glassfishrobot]

@ggam Commented Evaluating expressions surely have side effects, but it's up to the user what he puts on the expression IMO.

If I use a deployment time expression, I expect it to be evaluated at deployment time (via CDI extension) anyway, so the problem is the same as I see it. For instance, request scoped beans won't be available. And that's logical, as it's still deploying.

But the current implementation you are working on evaluates ${} expressions at first invocation time (not blaming you, just stating what I see). Imagine then a ${} expression that uses a request scoped bean that is bound to the current HTTP request.

The expression will work... But will use a request scoped bean for an "application scoped" expression. That hardly matches what the developer expected. And it will lead to strange problems.

In that case, I prefer the deployment to fail saying that the request context is not avaible. The error would be clearer I think.

El lun., 10 de julio de 2017 20:55, Arjan Tijms [email protected] escribió:

Any thoughts on what's reasonable hear, given that the beans are all instantiated via CDI? What's the correct, idiomatic CDI way to fail?

Unresolvable or ambiguous injections cause a deployment exception to be thrown.

In case of validating EL expressions, it's not so easy really. The only sure way to validate them is by evaluating them fully, but evaluating has side-effects (beans get created, application code runs, etc).

In think the most realistic thing to validate is during AfterDeploymentValidation to see if expressions are syntactical correct (can be parsed), and IF there are one of more bases, check if named beans exists for those (but don't try to actually instantiate them).

As an examplewhy this is not trivial, consider an expression like:

#{bean1.bean2.foo('abc.contact('xyz')).bar or bean3.bar(123 * 456)}

Checking for a valid syntax could be done. E.g. the following is invalid (unknown operator "orr"):

#{bean1.bean2.foo('abc.contact('xyz')).bar orr bean3.bar(123 * 456)}

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/javaee-security-spec/soteria/issues/104#issuecomment-314201254, or mute the thread https://github.com/notifications/unsubscribe-auth/ACAucA48qT2Jutz9VAIrMqz6DUaXB21dks5sMnOagaJpZM4OS0Pd .

[glassfishrobot]

@ggam Commented After chatting with Arjan, we came to the conclusion that there is not enough consensus on my approach to spec anything around it givem the time constraints we have.

So I propose to leave the spec as is, and then add some wording on it when we have enough experience from the RI.

For Soteria, and as a propietary feature, I would:

• Validate literal attributes (when possible)
• All expressions: validate that they are at least synyactically correct
• (Maybe) evaluate deployment time expressions in order to validate it. This would probably need some more discussion.

Leaving the spec silent on this topic seems smarter than trying to amemd a wrong wording later. What do you think?

[glassfishrobot]

@wmhopkins Commented This seems reasonable to me, if there is consensus from the other folks in this thread.

What's left here that's actionable? Is there work to do in Soteria for this, and, if so, is anyone signed up to do it?

[glassfishrobot]

@wmhopkins Commented Deferring this to the "Futures" milestone, as the current proposal only handles explicit Expression attributes, and thus leads to inconsistent treatment/validation between Expression and non-Expression attributes. It seems better to wait until we have a more comprehensive approach.