korotovsky
RedirectResponse issue
Hello,
I've been trying to implement your bundle but I think there is something wrong with the AuthenticationEntryPoint. After some debugging I found that it tries to create external redirect uri using symfony's HttpUtils::createRedirectResponse() method. The problem is - this method doesnt allow creating external redirects.
Please take a look at this: https://github.com/korotovsky/SingleSignOnServiceProviderBundle/blob/61ba4fc903ea89e50704eef988f7211fb96fcd0a/src/Krtv/Bundle/SingleSignOnServiceProviderBundle/EntryPoint/SingleSignOnAuthenticationEntryPoint.php#L132
The fix is simple - create RedirectResponse w/o using HttpUtils class.
Hi, what do you mean with "external"? AFAIK it worked well at least 2 years ago :) Maybe Symfony team has changed something?
Yeah that's what I'm worried about. By external I mean generating a route to "idp.example.com" from "sp.example.com".
Woah, that's definitely worked on 2.6 versions, I don't know to be honest how it is on 4 version line. Maybe you could make a PR and apply all necessary changes for Symfony 4? I'd love to merge it!
Truth to be said I'm using symfony 3.x right now :) I can post a PR after I fix it for my project but it requires documentation changes too.
And btw indeed - the HttpUtils files changed in symfony 2.7 making a BC imho. Check it out:
https://github.com/symfony/symfony/blob/240e9648af3daa5ed19580fdec74d768e30692a6/src/Symfony/Component/Security/Http/HttpUtils.php#L61
vs
https://github.com/symfony/symfony/blob/34d6116e2b80351c5a6f591285a4e9381b0dd127/src/Symfony/Component/Security/Http/HttpUtils.php#L62
The changes will be needed also on IdP end.
OK, I see. We either could properly configure SSO bundle and set correct $domainRegexp and a set of IdP and all SPs domains, or just fix it somehow else...
Well perhaps we can manage to do it with oneliner like this:
return new RedirectResponse($this->httpUtils->generateUri($request, $redirectUri), 302);
instead of
return $this->httpUtils->createRedirectResponse($request, $redirectUri);
Concerning security I'd rather set correct domainRegex... in the end it's SSO bundle :) I'd limit the amount of domain where you could be redirected in theory
Well, that's certainly a point and probably a reason why Symfony entered this. Therefore there's nothing to include in a bundle except for a change in docs perhaps. The httpUtils can have domainRegex set up but it cannot be changed globally because potentially you can break the site.
The only reasonable way to do that would be factoring a new HttpUtils instance with proper domainRegex just to inject it onto SSO bundle. Although I think it would be easier to set our own Regex to simply match the value in config?
Yes, factoring is a great idea. Maybe we could extend DI Configuration and ask developers to provide a so called white list of the domains and then build a regex out of it
Hmmm, isn't it a potential security breach if they add a method like 'setDomainRegex" or simply 'setHttpUtils' to authenticator class?
I think the best approach would be to abandon "createRedirectResponse" method from symfony HttpUtils class and create it internally in UriSigner or even in a new class that has HttpUtils injected (and use generateUri from there).
Something like this:
class SsoHttpUtils {
/**
* Native symfony http utils. This class is a helper and a bypass checker method for valid RedirectResponse generation.
*
* @var \Symfony\Component\Http\HttpUtils $httpUtils
*/
protected $httpUtils;
/**
* @var array
*/
protected $whitelistDomains;
public function __construct(HttpUtils $httpUtils, $whitelistDomains = [])
{
$this->httpUtils = $httpUtils;
$this->whitelistDomains = $whitelistDomains;
}
public function createRedirectResponse(Request $request, $path, $status = 302)
{
if(!$this->isValidDomain($path)) {
throw new \InvalidArgumentException('Domain is not whitelisted');
}
return new RedirectResponse($this->generateUri($request, $path), $status);
}
protected function isValidDomain($url) {
// validation of domains here
foreach($this->whitelistDomains as $domain) {
$domainRegexp = '#%s#i';
if (null !== $url && preg_match('#^https?://[^/]++#i', $url, $host) && !preg_match(sprintf($domainRegexp, preg_quote($domain)), $host[0])) {
return false;
}
}
return true;
}
}