spring-projects / spring-security

Spring Security
http://spring.io/projects/spring-security
Apache License 2.0
8.48k stars 5.77k forks source link

Exposing Beans for defaultMethodExpressionHandler can prevent Method Security #4020

Closed bitsofinfo closed 7 years ago

bitsofinfo commented 7 years ago

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

rwinch commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

$ ./gradlew --version
bitsofinfo commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

rwinch commented 7 years ago

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 commented 7 years ago

added to Application.java

bitsofinfo commented 7 years ago

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 commented 7 years ago

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 https://github.com/spring-projects/spring-security/issues/4020#issuecomment-238931750, or mute the thread https://github.com/notifications/unsubscribe-auth/AAWIB4ky-RTR68gbsi3SHPDzYiVIBATKks5qegMogaJpZM4Jgi1k .

bitsofinfo commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

bitsofinfo commented 7 years ago

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

peloncano commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

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

rwinch commented 7 years ago

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

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.

bitsofinfo commented 7 years ago

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

rwinch commented 7 years ago

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

rwinch commented 7 years ago

Closed via #4021

rwinch commented 7 years ago

@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 commented 7 years ago

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

rwinch commented 7 years ago

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

spolito commented 3 years ago

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:

2.2.5.RELEASE 5.4.12.Final 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 commented 3 years ago

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