Hystrix shareSecurityContext is not working

[dennyh89]

Hi all.
i am using the following libs and versions:

compile('org.springframework.boot:spring-boot-starter-actuator')
    compile('org.springframework.cloud:spring-cloud-starter-config')
    compile('org.springframework.cloud:spring-cloud-starter-eureka')
    compile('org.springframework.cloud:spring-cloud-starter-ribbon')
    compile('org.springframework.cloud:spring-cloud-starter-hystrix')
    compile('org.springframework.cloud:spring-cloud-netflix-hystrix-stream')
    compile('org.springframework.cloud:spring-cloud-starter-stream-rabbit')
    compile('org.springframework.cloud:spring-cloud-starter-feign')
    compile('org.springframework.boot:spring-boot-starter-web')
    compile('org.springframework.cloud:spring-cloud-starter-oauth2')
mavenBom 'org.springframework.cloud:spring-cloud-dependencies:Camden.BUILD-SNAPSHOT'

i try to get hystrix working in the THREAD isolation strategy mode relaying AccessTokens, therefore i tried both, Oauth2RestTemplate and Feign.

// Defining Beans
    @LoadBalanced
    @Bean
    public OAuth2RestTemplate loadBalancedOauth2RestTemplate(OAuth2ProtectedResourceDetails resource,
            OAuth2ClientContext oauth2Context) {
        return new OAuth2RestTemplate(resource, oauth2Context);
    }

@Bean
        public RequestInterceptor oAuth2RequestInterceptor(OAuth2ProtectedResourceDetails resource,
                OAuth2ClientContext oauth2Context) {
            return new OAuth2FeignRequestInterceptor(oauth2Context, resource);
        }

I also set the feign.hystrix.enabled property to true and also hystrix.shareSecurityContext to true but still getting the "no thread bound" exception when making the service to service call. it is just working with setting the SEMAPHORE strategy.
Any ideas? @daniellavoie

[daniellavoie]

Can you share a project where I can have a look ?

[daniellavoie]

BTW, I don't see Spring Security Starter in your gradle. The hystrix.shareSecurityContext will be effective if Spring Security is present in the classpath. Share a sample project and I'll see if there is any means to adapt the mechanism specially for Spring Cloud OAuth without depending on Spring Security.

[dennyh89]

hi. i have some effort to share the project on github, therefore i will needs some hours, but would like to answer your question. org.springframework.cloud:spring-cloud-starter-oauth2 depends on spring-cloud-starter-security and spring-security-core and so on, therefore i think i have all dependencies in. i can also debug through the autoconfiguration HystrixSecurityAutoConfiguration where the "existingConcurrencyStrategy" is null and gets wrapped into the SecurityContextConcurrencyStrategy. but then the exception occurs in the called OAuth2FeignRequestInterceptor at OAuth2AccessToken accessToken = oAuth2ClientContext.getAccessToken();

[bilak]

@daniellavoie I think this is the same issue as mine. The problem is that RequestContextHolder doesn't hold the attributes for child threads.

[daniellavoie]

I fear that the DelegatingSecurityContextCallable does not transfer the attributes you are expecting. The hystrix.shareSecurityContext was implemented to transfer the Principal holder used by classic Spring Security. A custom one might be needed for the specific requirements of Spring Cloud OAuth. I'll dig into it and see what's what.

[dennyh89]

you can see my code within this project and/or the other Org Projects there: https://github.com/dennyh89-spring-cloud-ref/microservice-two

the config is a bit mixed between config-repo and bootstrap.yml as i have tried around a little bit but it should be easy to follow.

[daniellavoie]

This project does not even run. I am not sure you understand how OAuth2FeignRequestInterceptor works. The OAuth2ProtectedResourceDetails is not something that is available in the container on startup that can be injected on bootstrap. Please provide a unit test that reproduce the exact problem you are encountering.

[dennyh89]

it is now runnable (i forgot to checkin gradle directory with it - gradle bootRun)
AFAIK the OAuth2ProtectedResourceDetails are able to be injected as they are always the same since bootstrap in this context. they are just filled with the clientId that is somehow mandatory for generating this Object and it is not needed at all for forwarding the Oauth2 Tokens. The requestInterceptor is using the oAuth2ClientContext to get the Access Token out of the request context and there the problem occurs when using the THREAD strategy which i was expecting to be eliminated with your commit on the HystrixConcurrencyStrategy and the SecurityContextCallable!?

[s4s0l]

My small input maybe you don't realize it. I tried to solve it myself, and maybe I don't understand it fully but there is few places where HystrixConcurrencyStrategy is tweaked: user can inject a bean - it will be picked by HystrixSecurityAutoConfiguration and wrap it with something that wraps callable. There is also SleuthHystrixConcurrencyStrategy which takes HystrixConcurrencyStrategy from HystrixPlugins.getInstance().getConcurrencyStrategy(). If it executes after HystrixSecurityAutoConfiguration will wrap already wrapped strategy, but if order will be different then Sleuth will replace plugin and then will be overriden by HystrixSecurityAutoConfiguration (sorry for syntax, I don't know how to explain it in polish even). I'm not sure what the order will be, will test it maybe later but my suggestion:

• because all these strategies do is wrap a callable
• there is a obvious need for another wrapper for oauth token propagation
• probably users will also want to wrap this callable to fulfill their needs
• HystrixConcurrencyStrategy has 2 responsibilities: concurency itself and means to wrap call in callables (it realizes 2 different needs i think)

Maybe it would be nice to introduce Strategy that could be a general wrapper for some 'callablewrapperproviders' that could be chained. It would simplify what has to be done to extend it. Code in HystrixSecurityAutoConfiguration and SleuthHystrixConcurrencyStrategy it quite complicated (and similar) and this way it would be as simple as registering new 'histrixcallablewrapperprovider' bean or something. Making this strategy as default would not break existing code even. Because currently to do it by myself I Have to almost copy code from one of the above classes:/

[alwaysastudent]

There is a fundamental flaw on the implementation of org.springframework.cloud.security.oauth2.client.feign.OAuth2FeignRequestInterceptor.getToken() https://github.com/spring-cloud/spring-cloud-security/blob/v1.2.1.RELEASE/spring-cloud-security/src/main/java/org/springframework/cloud/security/oauth2/client/feign/OAuth2FeignRequestInterceptor.java#L124

The OAuth2FeignRequestInterceptor should be getting the token value from the

((OAuth2AuthenticationDetails) org.springframework.security.core.context.SecurityContextHolder
				.getContext().getAuthentication().getDetails()).getTokenValue()

instead of going after the

org.springframework.security.oauth2.client.OAuth2ClientContext.getAccessToken()

This should be the case, because we are dealing with the SecurityContext which gets associated to the current thread of execution on org.springframework.security.concurrent.DelegatingSecurityContextCallable.call(), which gets wrapped from org.springframework.cloud.netflix.hystrix.security.SecurityContextConcurrencyStrategy.wrapCallable()

cc. @dsyer; @miguelsauza; @joaoevangelista

[alwaysastudent]

I was able to work around this by doing the below

	@Bean
	public OAuth2ClientContext oAuth2ClientContext() {
		return new DefaultOAuth2ClientContext() {

			@Override
			public OAuth2AccessToken getAccessToken() {
				return Optional.ofNullable(super.getAccessToken())
						.orElse(new DefaultOAuth2AccessToken(
								((OAuth2AuthenticationDetails) SecurityContextHolder
										.getContext().getAuthentication().getDetails())
												.getTokenValue()));
			}

		};
	}

	@Bean
	public RequestInterceptor oauth2FeignRequestInterceptor(
			OAuth2ClientContext oAuth2ClientContext,
			OAuth2ProtectedResourceDetails resource) {
		return new OAuth2FeignRequestInterceptor(oAuth2ClientContext, resource);
	}

hystrix:
  shareSecurityContext: true
feign:
  hystrix:
    enabled: true

cc. @bilak @dennyh89

[alwaysastudent]

I can issue a PR, if you agree with this approach.

[dsyer]

Actually, I don't think that is the right approach. The OAuth2ClientContext knows about the refresh token and how to use it, but the SecurityContext doesn't. So what you need to do is transfer the OAuth context to the hystrix thread, not the generic security context.

Also, the code you linked to is in a different project. Maybe you should open an issue or pull request there instead?

[alwaysastudent]

@dsyer - I can create another issue, give you the github project and work with you for a PR

But if I understand you correctly, you are acknowledging that there is an issue and you are saying it would be better to change the implementation of SecurityContextConcurrencyStrategy.wrapCallable(Callable<T>) - to wrap the commands with a custom delegator similar to DelegatingSecurityContextCallable which would transfer an OAuth2ClientContext instead of SecurityContext on some ThreadLocal carrier. Did I get you right ?

---

Currently the issue happens within this method, OAuth2FeignRequestInterceptor.getToken() , who craps out trying to weave the oAuth2ClientContext.

And if I step through with my debugger I can grab the detailed message as

org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'scopedTarget.oauth2ClientContext': Scope 'request' is not active for the current thread; consider defining a scoped proxy for this bean if you intend to refer to it from a singleton; nested exception is java.lang.IllegalStateException: No thread-bound request found: Are you referring to request attributes outside of an actual web request, or processing a request outside of the originally receiving thread? If you are actually operating within a web request and still receive this message, your code is probably running outside of DispatcherServlet/DispatcherPortlet: In this case, use RequestContextListener or RequestContextFilter to expose the current request.

Stacktrace

java.lang.IllegalStateException: No thread-bound request found: Are you referring to request attributes outside of an actual web request, or processing a request outside of the originally receiving thread? If you are actually operating within a web request and still receive this message, your code is probably running outside of DispatcherServlet/DispatcherPortlet: In this case, use RequestContextListener or RequestContextFilter to expose the current request.
	at org.springframework.web.context.request.RequestContextHolder.currentRequestAttributes(RequestContextHolder.java:131) ~[spring-web-4.3.9.RELEASE.jar:4.3.9.RELEASE]
	at org.springframework.web.context.request.SessionScope.get(SessionScope.java:91) ~[spring-web-4.3.9.RELEASE.jar:4.3.9.RELEASE]
	at org.springframework.beans.factory.support.AbstractBeanFactory.doGetBean(AbstractBeanFactory.java:340) ~[spring-beans-4.3.9.RELEASE.jar:4.3.9.RELEASE]
	at org.springframework.beans.factory.support.AbstractBeanFactory.getBean(AbstractBeanFactory.java:197) ~[spring-beans-4.3.9.RELEASE.jar:4.3.9.RELEASE]
	at org.springframework.aop.target.SimpleBeanTargetSource.getTarget(SimpleBeanTargetSource.java:35) ~[spring-aop-4.3.9.RELEASE.jar:4.3.9.RELEASE]
	at org.springframework.aop.framework.JdkDynamicAopProxy.invoke(JdkDynamicAopProxy.java:192) ~[spring-aop-4.3.9.RELEASE.jar:4.3.9.RELEASE]
	at com.sun.proxy.$Proxy103.getAccessToken(Unknown Source) ~[na:na]
	at org.springframework.cloud.security.oauth2.client.feign.OAuth2FeignRequestInterceptor.getToken(OAuth2FeignRequestInterceptor.java:124) ~[spring-cloud-security-1.2.0.RELEASE.jar:1.2.0.RELEASE]

[dsyer]

Your debugging probably suggests that the solution could be in spring-cloud-netflix (supporting @Scope("request") in a HystrixCommand - i.e. nothing to do with the SecurityContext).

[alwaysastudent]

@dsyer - Thank you for clarifying. I got your coordinates now. And I was able to get the request attributes propagated to the OAuth2FeignRequestInterceptor using a custom HystrixConcurrencyStrategy which mimics the same pattern as SecurityContextConcurrencyStrategy. So that solved the weaving issue within the OAuth2FeignRequestInterceptor.getToken() and I am able to get the oauth2ClientContext resolved there along with the token.

I have an auto configuration HystrixRequestScopeAutoConfiguration* that is placed after the HystrixSecurityAutoConfiguration. Currently it pivots on the same conditions as the later, but will set up a RequestScopeConcurrencyStrategy* that is decorated with any existing strategy if found.

class RequestScopeConcurrencyStrategy{
...

	@Override
	public <T> Callable<T> wrapCallable(Callable<T> callable) {
		return delegate != null
				? delegate.wrapCallable(new DelegatingRequestContextCallable<T>(callable))
				: super.wrapCallable(new DelegatingRequestContextCallable<T>(callable));
	}
...
}

The DelegatingRequestContextCallable* which is wrapping the original Callable as a delegate, and would set the request attributes before invoking the delegate similar to the DelegatingSecurityContextCallable

If you like this approach, I can issue a PR.

[alwaysastudent]

I think it is better to collapse the logic, while enabling hystrix.shareSecurityContext, to transfer both the SecurityContext and the RequestAttributes at one place, since they complement each other.

Also should not be looking for the presence of feign.hystrix.enabled, to dictate these transfers to happen, as this issue is not in particular to feign alone and can happen even if you wrap the OAuth2RestTemplate call within a @HystrixCommand

[michaeltecourt]

We had already written our token relay implementation based on the SecurityContext, and we found out that we only needed to propagate the security context in threads spawned from the request thread :
SecurityContextHolder.setStrategyName(SecurityContextHolder.MODE_INHERITABLETHREADLOCAL);
We put that in a @PostConstruct method somewhere in our @Configuration classes.

It works with stuff wrapped in HystrixCommands like @FeignClient calls, just like @Async methods.

@daniellavoie do you see any pitfall in this approach ? It seems far too simple compared to what you did in 5b0f601.

@dsyer would it be a bad idea to suggest that Spring Boot could auto-configure the SecurityContextHolder strategy through a property like security.context.strategy=MODE_INHERITABLETHREADLOCAL ?
I wonder why it's not the default strategy ?

[dsyer]

That won't work with refresh tokens, but neither will anything else in this project (that's more of a spring-cloud-security thing) so it seems like a reasonable solution if it works for you. I'm kind of surprised it works though because InheritableThreadLocal doesn't usually have any impact on work done in a thread pool. I guess it depends on how the thread pool is constructed.

[dsyer]

@alwaysastudent I don't think we want to link security and request scope together in a single class (they come from different dependencies). I think Spring Cloud Netflix allows you to stack Hystrix strategies together (but I don't recall if that applies to the concurrency strategy - if not we'd have to see if we can extend something). If you are still working on a PR, please try and split it up so it works when Spring Security is not in use.

[michaeltecourt]

OK that's the catch. I was using a FeignClient within an @Async method, which seems to change everything and I'm not sure why :

• Within an @Async method, I only need to set the security context holder strategy to MODE_INHERITABLETHREADLOCAL . The SecurityContext is available even if hystrix.shareSecurityContext is false. Is it because the default TaskExecutor configured by @EnableAsync spawns new threads instead of using a thread pool ?
• In a normal setup, I only need hystrix.shareSecurityContext: true. MODE_INHERITABLETHREADLOCAL has no effect like @dsyer predicted

[dsyer]

That's because @Async by default doesn't use a thread pool. OK, so we are all on the same page. Good.

[alwaysastudent]

@alwaysastudent I don't think we want to link security and request scope together in a single class (they come from different dependencies). I think Spring Cloud Netflix allows you to stack Hystrix strategies together (but I don't recall if that applies to the concurrency strategy - if not we'd have to see if we can extend something). If you are still working on a PR, please try and split it up so it works when Spring Security is not in use.

@dsyer - Makes sense. I am thinking to have a RequestScopeConcurrencyStrategy that can be independently configurable using hystrix.shareRequestContext or if you have the hystrix.shareSecurityContext turned on, since it complements the entire request cycle. I have been busy, but hope I'll be ready to issue a pull this weekend or worse next week.

[OlgaMaciaszek]

Closing as Hystrix is no longer supported within Spring Cloud Netflix.