[WFLY-12537] Incoming RunAsPrincipal is not being propagated to an unsecured EJB #12626

Skyllarr · 2019-09-11T13:56:16Z

https://issues.jboss.org/browse/WFLY-12537

Skyllarr · 2019-09-11T14:43:33Z

ejb3/src/main/java/org/jboss/as/ejb3/component/EJBComponent.java

@@ -643,14 +641,7 @@ private boolean checkCallerSecurityIdentityRole(String roleName) {
    }

    private SecurityIdentity getCallerSecurityIdentity() {
-        if (incomingRunAsIdentity != null) {


This just reverts the fix for JBEAP-9744. This is now handled by RunAsPrincipalInterceptor.

Skyllarr · 2019-09-11T15:12:23Z

ejb3/src/main/java/org/jboss/as/ejb3/component/EJBComponentDescription.java

+            interceptorFactories.put(InterceptorOrder.View.RUN_AS_PRINCIPAL, new ImmediateInterceptorFactory(new RunAsPrincipalInterceptor(RunAsPrincipalInterceptor.ANONYMOUS_PRINCIPAL, false)));
+        } else {
+            // Switch users (set new incoming)
+            interceptorFactories.put(InterceptorOrder.View.RUN_AS_PRINCIPAL, new ImmediateInterceptorFactory(new RunAsPrincipalInterceptor(runAsPrincipal, isSecurityRequired())));


@darranl We have to add RunAsPrincipalInterceptor to beans that do not have RunAsPrincipal annotation and propagate security. This is to be able to propagate incoming runas identity further. It should behave the same as legacy.

Skyllarr · 2019-09-11T15:15:24Z

ejb3/src/main/java/org/jboss/as/ejb3/security/RunAsPrincipalInterceptor.java

+        this.securityRequired = securityRequired;
+    }
+
+    private boolean remotingContextIsSet (){


@darranl Not sure if I should leave this method here or I should create for example SecurityActions class helper for this package or reuse some existing ones (i.e. make them public)?

Skyllarr · 2019-09-11T15:22:39Z

ejb3/src/main/java/org/jboss/as/ejb3/security/RunAsPrincipalInterceptor.java

-            // run as a user with the given name or if the caller has sufficient permission
-            if (runAsPrincipal.equals(ANONYMOUS_PRINCIPAL)) {
+            boolean remotingContextIsSet = remotingContextIsSet();
+            boolean isRemoteStatelessSecurityDisabled = !(ejbComponent instanceof StatefulSessionComponent) && remotingContextIsSet && !securityRequired;


make principal anonymous and incoming identity null if isRemoteStatelessSecurityDisabled

not sure if this is the best way to identify stateless bean

Hello @Skyllarr, you can use a instanceof check against org.jboss.as.ejb3.component.stateless.StatelessSessionComponent to be sure if it's a stateless EJB. In its current form, the above code in the PR, even singleton session bean component will pass this check. I don't know if you want that to happen.
Having said that, I'm just curious - is there a reason why this (security) logic is only applicable for stateless beans?

@jaikiran Hello, thanks for review! I changed it to StatelessSessionComponent. This way the identity will not be propagated if the bean is a stateless unsecured remote bean. I did not find any reference for this, but failing tests lead me this way and it also made sense to me that I should not propagate identity for such a bean.

Skyllarr · 2019-09-11T15:24:48Z

.../src/test/java/org/jboss/as/test/integration/ejb/remote/security/RemoteIdentityTestCase.java

@@ -99,12 +99,4 @@ public void testSwitched() throws Exception {
        assertEquals("user1", targetBean.getPrincipalName("user1", "password1"));
    }

-    @Test


@darranl I don't think this test has valid objective. EntryBean is unsecured stateless remote bean, so its principal is anonymous. This bean should not propagate guest identity to the injected SecurityInformation EJB in my opinion. The returned identity should be anonymous and not guest IMO. I do not think this test should be present for Elytron if the test objective is invalid.

@Skyllarr I think it is correct that the injected bean is making use of the identity from the Remoting connection since it is a secured bean.

@fjuma I see, after reading #12626 (comment) it probably makes sense to keep it here.

Skyllarr · 2019-09-11T15:42:30Z

...ic/src/test/java/org/wildfly/test/integration/weld/builtinBeans/InjectPrincipalTestCase.java

+    @Test
+    public void testNonAnonymousPrincipalInjected(CallerWithIdentity callerWithIdentity) throws Exception {
+        try {
+            // Assert.assertEquals(NON_ANONYMOUS_PRINCIPAL, callerWithIdentity.getCallerPrincipalInjected()); TODO see issue WFLY-12538


@nziakova I added your test cases from issue WFLY-11604. I commented out this one because it still needs to be addressed (see https://issues.jboss.org/browse/WFLY-12538)

darranl · 2019-12-19T13:27:38Z

Retest this please

darranl · 2019-12-19T13:28:48Z

@tadamski / @chengfang / @fjuma Looks like this one could use some additional review, I am not overly familiar myself with the EJB integration to answer Diana's questions.

Skyllarr · 2019-12-20T12:45:15Z

Retest this please

fjuma · 2019-12-20T18:31:35Z

ejb3/src/main/java/org/jboss/as/ejb3/security/RunAsPrincipalInterceptor.java

+        if(WildFlySecurityManager.isChecking()) {
+            remoteConnection = WildFlySecurityManager.doUnchecked((PrivilegedAction<RemoteConnection>) () -> RemotingContext.getRemoteConnection());
+        } else {
+            remoteConnection = RemotingContext.getRemoteConnection();


The RemotingContext class is meant to be used for legacy security, I don't think we should be using it for Elytron.

Ok it might not be needed

@fjuma Ok it might not be needed

fjuma · 2019-12-20T20:27:47Z

ejb3/src/main/java/org/jboss/as/ejb3/security/EJBSecurityViewConfigurator.java

-        if (securityRequired) {
-            ejbComponentDescription.setSecurityRequired(securityRequired);
-        }
+        ejbComponentDescription.setSecurityRequired(securityRequired);


setSecurityRequired should only be called if securityRequired is true (see https://issues.redhat.com/browse/WFLY-12301).

@fjuma Ok reverted

fjuma · 2019-12-20T21:24:02Z

@Skyllarr I think we should take a step back and see if there is another way to solve this problem. One thing to note is that the meaning of the incomingRunAsIdentity in RunAsPrincipalInterceptor is slightly different for the Elytron case than it was for legacy security. For Elytron, if there is a RunAsPrincipal annotation specified on an EJB, the RunAsPrincipalInterceptor will use securityDomain.createAdHocIdentity(runAsPrincipal) or currentIdentity.createRunAsIdentity(runAsPrincipal) to get the new identity to run as. The interceptor will then call newIdentity.runAs to switch users so this new identity will be used for outgoing calls. The purpose of the incomingRunAsIdentity variable is only to make sure that the RunAsPrincipal annotation doesn't affect the caller's identity (see wildfly-security-incubator@08b5940).

Taking a look at InjectPrincipalTestCase#testNonAnonymousPrincipalInjected:

We have a bean with a RunAsPrincipal annotation that has a method that invokes an unsecured bean.
When callerWithIdentity.getCallerPrincipalFromEJBContext() is called, we get to the RunAsPrincipalInterceptor, where a newIdentity is correctly created using the "non-anonymous" principal from the RunAsPrincipal annotation. The "non-anonmyous" identity will be used for outgoing calls.
The incomingRunAsIdentity is just set to the anonymous identity prior to switching users since that was the current identity when callerWithIdentity.getCallerPrincipalFromEJBContext() was called.

When we eventually end up in EJBComponent.getCallerSecurityIdentity, securityDomain.getCurrentSecurityIdentity() will return the "non-anonymous" user. The problem is that because beanB is an unsecured bean, securityRequired evaluates to false so we just return the anonymous identity (see

wildfly/ejb3/src/main/java/org/jboss/as/ejb3/component/EJBComponent.java

Lines 648 to 653 in 2f14ebd

    
           } else if (securityRequired) { 
        
               return securityDomain.getCurrentSecurityIdentity(); 
        
           } else { 
        
               // unsecured EJB 
        
               return securityDomain.getAnonymousSecurityIdentity(); 
        
           }

).

The tricky part is that we do want the anonymous identity to be returned for the Remoting -> unsecured EJB case (as per https://issues.redhat.com/browse/WFLY-8414). So it seems like we only want the "non-anonymous" identity to be returned for the EJB -> unsecured EJB case. Perhaps you could try to see if there is a way to differentiate between these two cases in EJBComponent#getCallerPrincipal so that securityDomain.getCurrentSecurityIdentity() gets returned in the latter case.

Skyllarr · 2020-01-28T10:16:53Z

The tricky part is that we do want the anonymous identity to be returned for the Remoting -> unsecured EJB case (as per https://issues.redhat.com/browse/WFLY-8414).

Is it okay that this would not match the behaviour of legacy? If I understand it correctly legacy would reauthenticate from remoting to unsecured bean and return non-anonymous:

https://github.com/wildfly/wildfly/blob/master/security/subsystem/src/main/java/org/jboss/as/security/service/SimpleSecurityManager.java#L321

So it seems like we only want the "non-anonymous" identity to be returned for the EJB -> unsecured EJB case. Perhaps you could try to see if there is a way to differentiate between these two cases in EJBComponent#getCallerPrincipal so that securityDomain.getCurrentSecurityIdentity() gets returned in the latter case.

So it is important to distinguish if the caller was from EJB or not. I do not see a way to lookup this information in the current code. So maybe we could add it to the security identity (something like boolean isEJB ) and set it in RunAsPrincipalInterceptor to have the appropriate behaviour.

fjuma · 2020-01-28T16:47:30Z

The tricky part is that we do want the anonymous identity to be returned for the Remoting -> unsecured EJB case (as per https://issues.redhat.com/browse/WFLY-8414).

Is it okay that this would not match the behaviour of legacy? If I understand it correctly legacy would reauthenticate from remoting to unsecured bean and return non-anonymous:

https://github.com/wildfly/wildfly/blob/master/security/subsystem/src/main/java/org/jboss/as/security/service/SimpleSecurityManager.java#L321

For the Remoting -> unsecured EJB case, the anonymous identity was returned when using legacy security. There's a description of why the anonymous identity gets returned here:

https://issues.redhat.com/browse/JBEAP-9744?focusedCommentId=13399451&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-13399451

Skyllarr · 2020-01-29T12:55:20Z

For the Remoting -> unsecured EJB case, the anonymous identity was returned when using legacy security. There's a description of why the anonymous identity gets returned here:

https://issues.redhat.com/browse/JBEAP-9744?focusedCommentId=13399451&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-13399451

@fjuma Yes sorry you're right.

So it is important to distinguish if the caller was from EJB or not. I do not see a way to lookup this information in the current code. So maybe we could add it to the security identity (something like boolean isEJB ) and set it in RunAsPrincipalInterceptor to have the appropriate behaviour.

@fjuma Do you think it would be good to do it this way?

fjuma · 2020-01-29T21:01:35Z

For the Remoting -> unsecured EJB case, the anonymous identity was returned when using legacy security. There's a description of why the anonymous identity gets returned here:
https://issues.redhat.com/browse/JBEAP-9744?focusedCommentId=13399451&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-13399451

@fjuma Yes sorry you're right.

So it is important to distinguish if the caller was from EJB or not. I do not see a way to lookup this information in the current code. So maybe we could add it to the security identity (something like boolean isEJB ) and set it in RunAsPrincipalInterceptor to have the appropriate behaviour.

@fjuma Do you think it would be good to do it this way?

I don't think this should be set in RunAsPrincipalInterceptor as this interceptor is already correctly making sure the correct identity is used for outgoing calls. I think we should take a closer look at EJBComponent#getCallerPrincipal to try to distinguish between the two cases there. It looks like CurrentInvocationContext.get() could be used there to get the interceptor context. I wonder if we could possibly make use of InterceptorContext.getPrivateData() to help us distinguish between the two cases by either trying to make use of existing data or perhaps by adding something new that could be used?

Skyllarr · 2020-01-30T20:12:47Z

@fjuma I have updated the PR accordingly. Thanks!

fjuma · 2020-02-10T19:43:16Z

Thanks @Skyllarr, this fix looks good to me. I think it would be good to check if the new tests being added here along with the existing test cases now cover the different scenarios (i.e., Remoting -> unsecured EJB, Remoting -> secured EJB, EJB -> unsecured EJB, EJB -> secured EJB) to make sure these are all working properly.

Skyllarr · 2020-02-18T15:55:05Z

Thanks @Skyllarr, this fix looks good to me. I think it would be good to check if the new tests being added here along with the existing test cases now cover the different scenarios (i.e., Remoting -> unsecured EJB, Remoting -> secured EJB, EJB -> unsecured EJB, EJB -> secured EJB) to make sure these are all working properly.

@fjuma Remoting -> secured or unsecured EJB are covered in org.jboss.as.test.integration.ejb.remote.security.RemoteIdentityTestCase. I added test that covers EJB -> secured EJB just now. Thanks

fjuma · 2020-02-18T21:14:22Z

Thanks @Skyllarr.

…secured EJB

Skyllarr · 2020-05-21T12:03:52Z

rebased

bstansberry · 2020-06-26T20:30:16Z

Thanks @Skyllarr

wildfly-ci added the deps-ok Dependencies have been checked, and there are no significant changes label Sep 11, 2019

Skyllarr commented Sep 11, 2019

View reviewed changes

Skyllarr force-pushed the WFLY-12537 branch from 0e2545c to 94e1667 Compare September 13, 2019 17:04

Skyllarr force-pushed the WFLY-12537 branch from 94e1667 to e6444ae Compare December 20, 2019 12:44

fjuma reviewed Dec 20, 2019

View reviewed changes

Skyllarr force-pushed the WFLY-12537 branch from e6444ae to a7f4221 Compare January 28, 2020 16:19

Skyllarr force-pushed the WFLY-12537 branch 3 times, most recently from c03642b to 971855c Compare January 30, 2020 20:05

Skyllarr force-pushed the WFLY-12537 branch from 971855c to 2f7ac9a Compare January 30, 2020 20:24

Skyllarr force-pushed the WFLY-12537 branch from 2f7ac9a to 0cb09b1 Compare February 18, 2020 15:52

fjuma approved these changes Feb 18, 2020

View reviewed changes

wildfly-ci added deps-changed Dependencies have been checked, and there are changes highlighted in a comment and removed deps-ok Dependencies have been checked, and there are no significant changes labels Mar 20, 2020

Skyllarr and others added 2 commits May 21, 2020 13:41

[WFLY-12537] Incoming RunAsPrincipal is not being propagated to an un…

95a24af

…secured EJB

[WFLY-11587] Added test for @Inject Principal

a31c5b0

Skyllarr force-pushed the WFLY-12537 branch from 0cb09b1 to a31c5b0 Compare May 21, 2020 12:02

wildfly-ci added deps-ok Dependencies have been checked, and there are no significant changes and removed deps-changed Dependencies have been checked, and there are changes highlighted in a comment labels May 21, 2020

darranl added the ready-for-merge Only for use by those with merge permissions! label Jun 18, 2020

bstansberry mentioned this pull request Jun 26, 2020

Pulls #13386

Closed

bstansberry merged commit 0a96300 into wildfly:master Jun 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WFLY-12537] Incoming RunAsPrincipal is not being propagated to an unsecured EJB #12626

[WFLY-12537] Incoming RunAsPrincipal is not being propagated to an unsecured EJB #12626

Skyllarr commented Sep 11, 2019

Skyllarr Sep 11, 2019

Skyllarr Sep 11, 2019

Skyllarr Sep 11, 2019

Skyllarr Sep 11, 2019

Skyllarr Sep 11, 2019

jaikiran Sep 12, 2019

Skyllarr Sep 13, 2019

Skyllarr Sep 11, 2019

fjuma Dec 20, 2019 •

edited

Loading

Skyllarr Jan 28, 2020

Skyllarr Jan 28, 2020

Skyllarr Sep 11, 2019

darranl commented Dec 19, 2019

darranl commented Dec 19, 2019

Skyllarr commented Dec 20, 2019

fjuma Dec 20, 2019

Skyllarr Jan 28, 2020

Skyllarr Jan 28, 2020

fjuma Dec 20, 2019

Skyllarr Jan 28, 2020

fjuma commented Dec 20, 2019

Skyllarr commented Jan 28, 2020

fjuma commented Jan 28, 2020

Skyllarr commented Jan 29, 2020

fjuma commented Jan 29, 2020

Skyllarr commented Jan 30, 2020

fjuma commented Feb 10, 2020

Skyllarr commented Feb 18, 2020

fjuma commented Feb 18, 2020

Skyllarr commented May 21, 2020

bstansberry commented Jun 26, 2020

[WFLY-12537] Incoming RunAsPrincipal is not being propagated to an unsecured EJB #12626

[WFLY-12537] Incoming RunAsPrincipal is not being propagated to an unsecured EJB #12626

Conversation

Skyllarr commented Sep 11, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjuma Dec 20, 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

darranl commented Dec 19, 2019

darranl commented Dec 19, 2019

Skyllarr commented Dec 20, 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

fjuma commented Dec 20, 2019

Skyllarr commented Jan 28, 2020

fjuma commented Jan 28, 2020

Skyllarr commented Jan 29, 2020

fjuma commented Jan 29, 2020

Skyllarr commented Jan 30, 2020

fjuma commented Feb 10, 2020

Skyllarr commented Feb 18, 2020

fjuma commented Feb 18, 2020

Skyllarr commented May 21, 2020

bstansberry commented Jun 26, 2020

fjuma Dec 20, 2019 •

edited

Loading