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

Exposing Beans for defaultMethodExpressionHandler can prevent Method Security #4020

Closed
bitsofinfo opened this issue Aug 9, 2016 · 30 comments · Fixed by #4022
Closed

Exposing Beans for defaultMethodExpressionHandler can prevent Method Security #4020

bitsofinfo opened this issue Aug 9, 2016 · 30 comments · Fixed by #4022
Assignees
Labels
in: config An issue in spring-security-config type: bug A general bug
Milestone

Comments

@bitsofinfo
Copy link

bitsofinfo commented Aug 9, 2016

Updated Summary

If a @Configuration provides a @Bean that is used to default GlobalMethodSecurityConfiguration's defaultMethodExpressionHandlers defaultMethodExpressionHandler it will prevent any @Bean that is @Autowired into the same @Configuration from having method security enabled. For example:

@Configuration
@EnableGlobalMethodSecurity(prePostEnabled = true)
public class SecurityConfig {
    // any one of the following @Bean will prevent DenyAllService from being
    // secured since DenyAllService is also Autowired into this same Configuration
    @Bean
    PermissionEvaluator permissionEvaluator() {
        return mock(PermissionEvaluator.class);
    }

    @Bean
    RoleHierarchy RoleHierarchy() {
        return mock(RoleHierarchy.class);
    }

    @Bean
    AuthenticationTrustResolver trustResolver() {
        return mock(AuthenticationTrustResolver.class);
    }

    @Autowired
    DenyAllService denyAll;
}

@Configuration
public class ServiceConfig {
    @Bean
    DenyAllService denyAllService() {
        return new DenyAllService();
    }
}

@PreAuthorize("denyAll")
public class DenyAllService {
    void denyAll() {
    }
}

Summary

spring-data-rest @PreAuthorize annotated methods on a @RepositoryRestResource annotated PagingAndSortingRepository interface fail to be evaluated on invocation if the resulting repository bean instance is @Autowired into a @Configuration annotated class.

Actual Behavior

See this project for a runnable example:
https://github.com/bitsofinfo/spring-boot-pre-authorize-issue-01

Expected Behavior

@PreAuthorize expressions should be evaluated on requests that hit the repository

Configuration

See this project for a runnable example:
https://github.com/bitsofinfo/spring-boot-pre-authorize-issue-01

Version

All latest spring-boot components

See this project for a runnable example:
https://github.com/bitsofinfo/spring-boot-pre-authorize-issue-01

Sample

https://github.com/bitsofinfo/spring-boot-pre-authorize-issue-01

@jgrandja jgrandja changed the title @PreAuthorize expressions fail to be evaluated on invocation if spring-data repository bean instance is @Autowired into a @Configuration annotated class @PreAuthorize not evaluated on spring-data repository bean Aug 10, 2016
@jgrandja jgrandja self-assigned this Aug 10, 2016
@rwinch
Copy link
Member

rwinch commented Aug 10, 2016

@bitsofinfo Thanks for the report and the sample! I am unable to reproduce (what I understand as) the problem.

If I uncomment the autowired and the repository in Application https://github.com/bitsofinfo/spring-boot-pre-authorize-issue-01/blob/d6a23214f1b62183e7e971e7d33d8ce5ac888d58/src/main/java/my/test/Application.java#L37-L38

Then invoke

curl http://localhost:8080/testrecords/search/findByFirstname?fn=1

I still see the following in the logs

hasPermission() org.springframework.security.authentication.AnonymousAuthenticationToken@9055c2bc: Principal: anonymousUser; Credentials: [PROTECTED]; Authenticated: true; Details: org.springframework.security.web.authentication.WebAuthenticationDetails@b364: RemoteIpAddress: 0:0:0:0:0:0:0:1; SessionId: null; Granted Authorities: ROLE_ANONYMOUS target: 1 perm:READ

From what I can tell everything is working just fine. Can you help me understand what the problem is? Perhaps this is an example of…works for me and not for you?

cc @jgrandja

@bitsofinfo
Copy link
Author

bitsofinfo commented Aug 10, 2016

Thats odd, myself and an associate did the following and it fails everytime as expected.

git clone https://github.com/bitsofinfo/spring-boot-pre-authorize-issue-01.git
cd spring-boot-pre-authorize-issue-01
./gradlew bootRun
curl http://localhost:8080/testrecords/search/findByFirstname?fn=1

See the expected entries in STDOUT

Stop the process:, edit Application.java and uncomment the noted section.

./gradlew clean
./gradlew bootRun
curl http://localhost:8080/testrecords/search/findByFirstname?fn=1

The expected entries no longer show up.

@rwinch
Copy link
Member

rwinch commented Aug 10, 2016

When the entries don't show up what happens? Do you get access denied or access permitted? What OS and what JDK (vendor and exact version) are you using?

@rwinch
Copy link
Member

rwinch commented Aug 10, 2016

PS: You can run the following to get the OS and JDK information I need

$ ./gradlew --version

@bitsofinfo
Copy link
Author

Thats the point, when those system prints from the PermissionEvaluator don't show up in stdout, it is evidence that the @PreAuthorize annotated methods are not even being evaluated or proxied or delegating to the custom evaluator.

------------------------------------------------------------
Gradle 2.14
------------------------------------------------------------

Build time:   2016-06-14 07:16:37 UTC
Revision:     cba5fea19f1e0c6a00cc904828a6ec4e11739abc

Groovy:       2.4.4
Ant:          Apache Ant(TM) version 1.9.6 compiled on June 29 2015
JVM:          1.8.0_45 (Oracle Corporation 25.45-b02)
OS:           Mac OS X 10.11.5 x86_64

@rwinch
Copy link
Member

rwinch commented Aug 10, 2016

Thats the point, when those system prints from the PermissionEvaluator don't show up in stdout, it is evidence that the @PreAuthorize annotated methods are not even being evaluated or proxied or delegating to the custom evaluator.

This makes sense. I understand you are not getting the desired behavior ( the custom PermissionEvaluator is not invoked). However, I'm trying to determine what is happening instead. This might help me diagnose the issue. For example, "Is the repository getting proxied in the first place?" (in which case the repository should be deny all). If you are getting a permission granted, it might imply the repository is not getting proxied at all.

@bitsofinfo
Copy link
Author

Yes I suspect the latter, that the repo is not getting proxied at all.

@rwinch
Copy link
Member

rwinch commented Aug 10, 2016

Can you provide a System.out.println of TestRepository. For example, add the following to Application.java:

@Autowired
public void see(TestRepository test) {
    System.out.println("This is my value ====================== " + test);
}

@bitsofinfo
Copy link
Author

added to Application.java

@bitsofinfo
Copy link
Author

bitsofinfo commented Aug 10, 2016

In both instances (commented and uncommented) its this:

This is my value ====================== org.springframework.data.keyvalue.repository.support.SimpleKeyValueRepository@78b612c6

This is my value ====================== org.springframework.data.keyvalue.repository.support.SimpleKeyValueRepository@376c7d7d

@rwinch
Copy link
Member

rwinch commented Aug 10, 2016

Thanks!

In order to figure out what is triggering this behaviour, can you try
moving the PermissionEvaluator Bean to another Configuration and tell me
what happens. If that is still broke move @surprise to another configuration

On Aug 10, 2016 11:58 AM, "bitsofinfo" notifications@github.com wrote:

This is my value ====================== org.springframework.data.keyvalue.repository.support.SimpleKeyValueRepository@78b612c6


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#4020 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAWIB4ky-RTR68gbsi3SHPDzYiVIBATKks5qegMogaJpZM4Jgi1k
.

@bitsofinfo
Copy link
Author

bitsofinfo commented Aug 10, 2016

I'm already tied up w/ another task and can't spend any more time on this today. Feel free to play w/ it however you want, fork or pr, or just locally to experiment with it. I just needed to point this out and document it as I spent about 10 hours yesterday trying to widdle down a very complex app to this to reproduce this maddening result :)

To confirm, yes I worked around this by moving that repo bean to another config class. But my point is that this could be a giant time waster for folks, do to the lack of info on the cause. I'm not sure if this simply a chicken-before-the-egg kind of dependency issue w/ the proxying or what.

But either way it just silently causes odd behavior that is very hard to pinpoint the cause of (i.e. from the spring user's perspective just trying to wire a bean to use)

@rwinch
Copy link
Member

rwinch commented Aug 10, 2016

But my point is that this could be a giant time waster for folks, do to the lack of info on the cause. I'm not sure if this simply a chicken-before-the-egg kind of dependency issue w/ the proxying or what.

Agreed. This is a workaround, but should not be required. Mostly trying to figure out what is happening so I can fix it (difficult since I cannot reproduce it though).

@rwinch
Copy link
Member

rwinch commented Aug 10, 2016

One more thing. Are you importing it into an IDE at all or do you only run from the command line? If you do import into an IDE which one?

@bitsofinfo
Copy link
Author

So you have run this sample project, and no matter what the state of the @Autowired bean in Application.java, you ALWAYS have the System print from PermissionEvaluator outputting to the console?

I am only running this from the cmd line.

@rwinch
Copy link
Member

rwinch commented Aug 10, 2016

Correct. I always see the print statement from the custom PermissionEvaluator. I am running from the command line using your instructions.

The only difference is that I have a newer version of the JDK. I have also tried with jdk1.8.0_40.jdk and that works for me as well.

@bitsofinfo
Copy link
Author

great, so what does that mean, something w/ my local .m2 or gradle repos is messed up?

@bitsofinfo
Copy link
Author

Going to have my colleagues run it as well and chime in

@peloncano
Copy link

peloncano commented Aug 10, 2016

@rwinch I am seeing the same result as @bitsofinfo on my local environment.

Essentially when the autowiring is commented my output for multiple curl requests is

hasPermission() org.springframework.security.authentication.AnonymousAuthenticationToken@9055c2bc: Principal: anonymousUser; Credentials: [PROTECTED]; Authenticated: true; Details: org.springframework.security.web.authentication.WebAuthenticationDetails@b364: RemoteIpAddress: 0:0:0:0:0:0:0:1; SessionId: null; Granted Authorities: ROLE_ANONYMOUS target: 2 perm:READ

when I uncomment for my initial curl request i get the following

2016-08-10 16:32:40.886  INFO 89858 --- [nio-8080-exec-1] o.a.c.c.C.[Tomcat].[localhost].[/]       : Initializing Spring FrameworkServlet 'dispatcherServlet'
2016-08-10 16:32:40.887  INFO 89858 --- [nio-8080-exec-1] o.s.web.servlet.DispatcherServlet        : FrameworkServlet 'dispatcherServlet': initialization started
2016-08-10 16:32:40.900  INFO 89858 --- [nio-8080-exec-1] o.s.web.servlet.DispatcherServlet        : FrameworkServlet 'dispatcherServlet': initialization completed in 13 ms

And no more output on other curl requests...

Here is my ./gradlew --version

------------------------------------------------------------
Gradle 2.14
------------------------------------------------------------

Build time:   2016-06-14 07:16:37 UTC
Revision:     cba5fea19f1e0c6a00cc904828a6ec4e11739abc

Groovy:       2.4.4
Ant:          Apache Ant(TM) version 1.9.6 compiled on June 29 2015
JVM:          1.8.0_51 (Oracle Corporation 25.51-b03)
OS:           Mac OS X 10.10.5 x86_64

@rwinch
Copy link
Member

rwinch commented Aug 11, 2016

@peloncano Thanks for the response.

@bitsofinfo

great, so what does that mean, something w/ my local .m2 or gradle repos is messed up?

I'm not sure what is causing this, but my best guess is to do with ordering of classpath scanning which can at times cause weird issues like this.

@jgrandja Can you give the sample a try to see if you can reproduce this?

@rwinch
Copy link
Member

rwinch commented Aug 11, 2016

@bitsofinfo and @peloncano I just tried it again and I can reproduce the issue.

@rwinch
Copy link
Member

rwinch commented Aug 11, 2016

I have figured out the issue. The problem is that:

  • All the BeanPostProcessors are registered. Specifically embeddedServletContainerCustomizerBeanPostProcessor is created
  • This triggers AbstractAutoProxyCreator attempting to determine if an AOP proxy should be created for embeddedServletContainerCustomizerBeanPostProcessor
  • The metaDataSourceAdvisor is created in order to perform AOP
  • This triggers methodSecurityMetadataSource to be created
  • This triggers GlobalMethodSecurityConfiguration to be created
  • This triggers aclPermissionEvaluator to be created (because GlobalMethodSecurityConfiguration autowires all PermissionEvaluators)
  • This triggers application to be created
  • This triggers testRecordRepository to be created

Then in BeanFactoryAdvisorRetrievalHelper the testRecordRepository is not proxied because metaDataSourceAdvisor is currently in creation.

To get around this I think we need to delay looking up any Beans in GlobalMethodSecurityConfiguration until SmartInitializingSingleton.afterSingletonsInstantiated is invoked. I will try and put together a fix tomorrow.

@rwinch rwinch assigned rwinch and unassigned jgrandja Aug 11, 2016
@rwinch rwinch added in: config An issue in spring-security-config type: bug A general bug and removed investigating labels Aug 11, 2016
@rwinch rwinch modified the milestones: 4.1.2, 4.2.0 M1 Aug 11, 2016
@rwinch rwinch changed the title @PreAuthorize not evaluated on spring-data repository bean Exposing Beans for defaultMethodExpressionHandler can prevent Method Security Aug 11, 2016
rwinch pushed a commit to rwinch/spring-security that referenced this issue Aug 11, 2016
Previously if a Bean for GlobalMethodSecurityConfiguration's
defaultMethodExpressionHandler was found on a Configuration that also
@Autowired a Bean that enabled method security, the Bean that was
@Autowired would not have security enabled.

This fixes the issue by delaying the lookup of Beans populated on
GlobalMethodSecurityConfiguration's defaultMethodExpressionHandler.

Fixes spring-projectsgh-4020
rwinch pushed a commit to rwinch/spring-security that referenced this issue Aug 11, 2016
Previously if a Bean for GlobalMethodSecurityConfiguration's
defaultMethodExpressionHandler was found on a Configuration that also
@Autowired a Bean that enabled method security, the Bean that was
@Autowired would not have security enabled.

This fixes the issue by delaying the lookup of Beans populated on
GlobalMethodSecurityConfiguration's defaultMethodExpressionHandler.

Fixes spring-projectsgh-4020
@bitsofinfo
Copy link
Author

Awesome, great work @rwinch thanks for looking into it!

@rwinch
Copy link
Member

rwinch commented Aug 11, 2016

Thank you @bitsofinfo and @peloncano for all your help reporting & isolating the issue!

@rwinch rwinch closed this as completed Aug 11, 2016
@rwinch
Copy link
Member

rwinch commented Aug 11, 2016

Closed via #4021

@rwinch
Copy link
Member

rwinch commented Aug 12, 2016

@bitsofinfo / @peloncano I wanted to thank you again for all your efforts in helping isolate this issue. Spring Security 4.1.2 is now released with fixes for the issue you were encountering. See https://spring.io/blog/2016/08/12/spring-security-4-1-2-released for the announcement

@bitsofinfo
Copy link
Author

No problem glad to help, any thoughts on this one @rwinch? https://github.com/bitsofinfo/spring-boot-cloud-eureka-path-variable-issue

@rwinch
Copy link
Member

rwinch commented Aug 12, 2016

@bitsofinfo Probably best for the Cloud team to look at.

@spolito
Copy link

spolito commented Oct 21, 2020

Hello, I have found that this issue is back. I assume I should start a new issue since this thread is closed.

I am running the following versions:
<spring-boot.version>2.2.5.RELEASE</spring-boot.version>
<hibernate.version>5.4.12.Final</hibernate.version>

The LendingTransactionPermissionEvaluator is in the same Config as my Repo(s). For most Repos there is no issue. But when I use the Repo of the same entity type that the LendingTransactionPermissionEvaluator acts on... then the LendingTransactionPermissionEvaluator will silently not get invoked and permissions fly as if there was no security.

Thanks very much, and I will start a new Post as well.

@spolito
Copy link

spolito commented Oct 21, 2020

Sorry, I meant to post "TargetedPermissionEvaluator" instead of my custom name "LendingTransactionPermissionEvaluator"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: config An issue in spring-security-config type: bug A general bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants