The discussion is about supporting multiple SSO secrets with Discourse as the provider. supermathie suggests two options to implement this feature: signing the SSO payload with both a primary key and a hashed key based on the consumer’s hostname, or building explicit support for multiple keys per hostname. mpalmer advises against the first approach due to potential security risks and inefficiencies.
sam proposes a more comprehensive solution by introducing a sso_provider_secrets site setting, which would contain a list of pairs with hostnames and corresponding secrets. This approach would provide extra security and encourage users to audit their SSO configurations. joffreyjaffeux confirms that implementing this feature would be relatively easy, especially with the recent introduction of “list_type” in yml settings.
Work on this feature is underway, with zogstrip referencing the relevant pull request (https://github.com/discourse/discourse/pull/6431). joffreyjaffeux later confirms that the feature has been merged. Some minor improvements are still needed for the mobile admin interface, but this is not a high-priority task.
I am quite a fan of key agility, and this provides a mechanism to do that (at least the “do it properly” approach). I think the quick hack method is a really bad idea, for several reasons:
You’d presumably have to be sending out both signatures all the time, which is wasteful for all those clients not requiring the extra signature;
k+consumer.hostname is equivalent to k in any circumstance in which the attacker can influence the value of consumer.hostname.
Conversely, even getting agreement between consumer and provider on what consumer.hostname even is can be… an adventure, and the ways that are typically easier to do are also more amenable to attacker influence.
The construction hash(k+consumer.hostname) would have to be carefully evaluated against possible attacks, which is a non-trivial exercise. There are extremely good reasons why HMAC(K, m) is defined as H((K' xor opad) || H((K' xor ipad) || m)), and not just H(K || m). Cryptography Is Hard.
I think it is a bit overdue to make sso_secret single use vs “multi use” where it works both for incoming and outgoing SSO.
If we are cleaning this up I think we should probably clean it up all the way. Add a site setting for sso_provider_secrets which contains a list of pairs:
Etc… @j.jaffeux new control makes creating settings like this super easy but we would need a special mode where it hides the secrets in the UX like we do for other passwordish fields in site settings.
Then we also get extra security here cause even if a key leaks out it can only be used on the “domain” you allowed it so it will simply (post login) redirect to a non-hijacked URL which would be confused about the whole thing cause a NONCE would not check out.
Overall there is not too much work in this change and I totally support it cause it makes SSO provider support way cleaner and will force people using this feature to do a nice audit.
@maja was looking to it on mobile and it could use some love. Nothing urgent and critical as we don’t aim for perfection on mobile admin. But it should be fairly easy to fix.
but we would need a special mode where it hides the secrets in the UX like we do for other passwordish fields in site settings.