OpenConext / OpenConext-manage

Stores and publishes metadata of all entities known to OpenConext
Apache License 2.0
7 stars 10 forks source link

Weaker strength for Resource Servers / Relaying parties secrets #87

Closed oharsta closed 2 years ago

oharsta commented 2 years ago

The default BCrypt strength of 10 decreases the performance of the Introspection endpoint with ~60ms. Under heavy load this is becoming a bottle-neck. The Hook to encrypt plain text secrets now used a strength of 5 for Resource Server secrets.

codecov[bot] commented 2 years ago

Codecov Report

Merging #87 (e377107) into master (c7706e8) will decrease coverage by 0.00%. The diff coverage is 90.90%.

@@             Coverage Diff              @@
##             master      #87      +/-   ##
============================================
- Coverage     82.92%   82.91%   -0.01%     
- Complexity      924      925       +1     
============================================
  Files            95       95              
  Lines          2887     2892       +5     
  Branches        273      274       +1     
============================================
+ Hits           2394     2398       +4     
  Misses          341      341              
- Partials        152      153       +1     
Impacted Files Coverage Δ
...e-server/src/main/java/manage/hook/SecretHook.java 89.47% <90.00%> (-3.39%) :arrow_down:
...in/java/manage/hook/MetaDataHookConfiguration.java 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

thijskh commented 2 years ago

Good idea.

Which other secrets are there (that remain at strength 10)? Is this necessary or can we just always make it 5?

The generation of the secrets themselves should be sufficiently long and/or complex (length or complexity does not matter for usability since users will be able to copy paste them), then the strength matters not so much. Is this guaranteed somewhere? A minimum length perhaps? Or in the code that generates a new secret upon secret reset?

oharsta commented 2 years ago

Currently Manage does a (strong) suggestion for the secret for new RP's and RS's, but the manage users can override this with anything they want. I don't know how this is implemented in SP dashboard. The only secret being hashed is the secret for Relying Parties / Resource Servers. The former strength stays at 10, the latter strength is now 5. We can add a weak-password check very easily in Manage, but as the users are power-users it might not be necessary?

thijskh commented 2 years ago

I would make all secrets the same hashing strength. A simple check could be to require any user/api supplied secret to be at least say 12 chars?

oharsta commented 2 years ago

Implemented minimal length of 12 characters for secrets. See https://github.com/OpenConext/OpenConext-manage/commit/e377107a8eef5ca30b69b4bf9d518eae1d6b176d

tvdijen commented 2 years ago

Shouldn't the secret follow some kind of best practice? Length is not the only variable here. What about complexity? 'aaaaaaaaaaaa' is not a strong secret. i.e. our passwords for privileged accounts must be at least 14 characters long and follow certain complexity rules..

This could be weakened depending on refresh-strategy.. i.e. in Azure an OIDC-secret expires after 6 months by default. In OC Manage I don't think they ever expire, so that would certify some additional complexity rules

thijskh commented 2 years ago

Additional length quickly trumps any additional character set complexity. So having a minimum length is by far the best guarantee.

However, best would be if passwords are always generated since machines are way better at this than humans and you don't need to check whether they're doing it right. Just have the button that generates a secret and do not provide a manual text box can be a simple solution for this. SP Dashboard already only generates the secret, does not allow to set one by the user.