BookStackApp
ES256 Support in OIDC
Describe the feature you'd like
ES256 implementation for OIDC
Describe the benefits this would bring to existing BookStack users
It would allow the use of the far more secure and up-to-date signing format which is preferable for security especially as RS256 is generally on the way out. It would also work better for people who use Lets Encrypt for signing certs as that is what they typically provide.
Can the goal of this request already be achieved via other means?
No
Have you searched for an existing open/closed issue?
- [X] I have searched for existing issues and none cover my fundamental request
How long have you been using BookStack?
Under 3 months
Additional context
No response
ES256, following the spec, is ECDSA using P-256 and SHA-256. Looks like it should be supported by the lib we're already using to verify signatures: https://phpseclib.com/ Would need to check/validate the process/format for certs provided via config, as well as autodiscovery.
Tricky to find any useful information out there regarding widespread use/plans/changes in ES256 use for OIDC.
The JWA spec does mark it as recommended+, hinting at being required in future, so may be a good indicator at specifically supporting ES256 over any other potential algorithm, but not sure about timings around that or realistic use in the OIDC landscape.
ES256, following the spec, is ECDSA using P-256 and SHA-256. Looks like it should be supported by the lib we're already using to verify signatures: https://phpseclib.com/ Would need to check/validate the process/format for certs provided via config, as well as autodiscovery.
Tricky to find any useful information out there regarding widespread use/plans/changes in ES256 use for OIDC. The JWA spec does mark it as
recommended+, hinting at being required in future, so may be a good indicator at specifically supporting ES256 over any other potential algorithm, but not sure about timings around that or realistic use in the OIDC landscape.
Hi, I've come across this issue as its something id like to see implemented as well. Be it that its marked as recommended+ i guess its not going to take too long for support to come about. In our use case we just need to be able to sign our own auth correctly for security - and unfortunately this is the only thing keeping us away from BookStack.
Although once this is implemented we'll probably be switching straight over from whichever alternative we decide on.
I'll keep an eye on this for any updates on your end as I would love to be able to use BookStack (seems like the best option by far).
Be it that its marked as recommended+ i guess its not going to take too long for support to come about.
That is from a 2015 document though, so things aren't moving too fast there.
In our use case we just need to be able to sign our own auth correctly for security
Is there a specific reason that can't be done using RS256?
Be it that its marked as recommended+ i guess its not going to take too long for support to come about.
That is from a 2015 document though, so things aren't moving too fast there.
In our use case we just need to be able to sign our own auth correctly for security
Is there a specific reason that can't be done using RS256?
So, we generate all our certs through letsencrypt and the standard it uses is ECDSA. Whilst im sure theres a way to change this, the organisation this is involved with requires higher security wherever possible.
i.e. We cannot use RSA where ECDSA is available.
@CL107 Is there a reason why letsencrypt is used to gain certs for OIDC token signing?
Just seems a bit odd to me to reuse keys from TLS certs to use for another purpose, but don't know if I'm missing or misunderstanding something.
Sorry for the long time to respond. No specific reason @ssddanbrown, we just intend to use our own signing keys wherever possible.
I managed with minimal changes (4 lines to be exact) to allow RS512 signing algorithms.
From what I can read out of that discussion, @ssddanbrown you are not sure whether you should add this feature from a perspective different from the sake of implementing it? If you think it is useful to have other algorithms supported, I can open a PR and we could discuss an implementation there?
I will provide the lines changed quickly, but I don't have the opportunity to test this against other signings, and currently I am not convinced that I have sufficient knowledge or enough time to logically validate and test whether those changes are 100% correct.
Maybe this only works because RS256 and RS512 are so similar, and other signing algorithms would require similar changes.
- Change to
RS512:- https://github.com/BookStackApp/BookStack/blob/1ac74099ca843c4141f1a442b18f97a0b57a4266/app/Access/Oidc/OidcJwtSigningKey.php#L66
- https://github.com/BookStackApp/BookStack/blob/1ac74099ca843c4141f1a442b18f97a0b57a4266/app/Access/Oidc/OidcJwtWithClaims.php#L122
- https://github.com/BookStackApp/BookStack/blob/1ac74099ca843c4141f1a442b18f97a0b57a4266/app/Access/Oidc/OidcProviderSettings.php#L164
- https://github.com/BookStackApp/BookStack/blob/1ac74099ca843c4141f1a442b18f97a0b57a4266/app/Access/Oidc/OidcJwtSigningKey.php#L100
here I made this change:
$this->key = $key->withHash('sha512')->withPadding(RSA::SIGNATURE_PKCS1);
Just a quick overview, I could be plain wrong with those changes/this approach. If those changes are acceptable, I would like to think that I have enough skill to draft up something that works for more than one algorithm in general in a PR.
Thanks @McTom234. Generally my concerns come down to testing & maintenance rather than implementation ease, so I just want to make sure that if we're adding extra options they're suited for a wide audience and forward-looking (algorithms which have, and will gain wider adoption), rather than looking to add algorithms just to suit edge-cases.
Feel free to open a new request specifically for that algorithm if there's a case to be made for it to be added.
Please add this support =) Sometimes we don’t have the ability to choose between RS512 and RS256, and we need to use whatever is provided, and in my case - it is RS512
@VitalyOmelch Please open a new issue, specifically for RS512, and in that please explain the environment/context/systems in use (where enforcement is set) where possible.
The support for ES256 (and possibly 384/512) would be a really nice security improvement.
@ssddanbrown Sorry it took a bit longer; life carries on 😉
I opened #5912 and hope that I understood your concerns (regarding maintenance mainly) correctly. Testing would need to be extended, but I'd say the maintenance is doable, and now it is up to you (the maintainers and community) to decide which algorithms should be implemented; the approach presented should be suitable to implement any preferred.
Thanks @McTom234. To be honest though, implementation/code effort was not really my worry/concern. It comes down to manual testing needs and user support.
Ultimately, to help move things forward, it'd probably help to come up with some kind of criteria for how algorithms are assessed. Here's an attempt at coming up with that:
Algorithm Criteria (Draft)
- At least 2 unique BookStack user requests that are based on using different auth platforms/software.
- This helps indicate some wide industry support, and a wider audience need.
- This helps avoid us making support decisions based upon one-off decisions of other platforms/services.
- Support for the algorithm in one of the auth platforms we commonly test on (Entra, Okta, Jumpcloud, Auth0).
- Allows for relatively easy testing, and helps indicate wider industry support.
- Algorithm is forward looking, without signs it's due to be deprecated.
- Don't want to go to effort to add options we'd have to soon remove as a breaking change.
- Algorithm can be supported in our codebase without extra dependency/requirement changes.
- Could be flexible on this one if there's very wide industry support, like RS256 now.