feat: Add rotation support for client secrets#608
Conversation
aeneasr
left a comment
There was a problem hiding this comment.
Awesome, thank you for your contribution! This looks very good. Just one or two minor things :)
client_authentication_test.go
Outdated
| } | ||
| } | ||
|
|
||
| // func TestCheckClientSecret(t *testing.T) { |
There was a problem hiding this comment.
Do you want to enable this test or remove it? :)
There was a problem hiding this comment.
Tried to clean commit history and somewhat ended up with this there :)
Removed it now, but it relates to my last question.
add new interface and implement it for
|
Thank you! I think this looks good. Documentation could be added in the README, otherwise we don't really have docs. Regarding the tests, I think the current set up is sufficient! Sorry for the long review time, I missed the force push in my notifications :/ |
|
@aeneasr It's all right, just let me know if there's something else needed. Haven't found a great section to put it into, but it's in the example that is in the readme at least. |
|
Maybe you could do a follow up PR in https://github.com/ory/fosite-example/ ? With rotation enabled there. I think that's probably the best option. Merging this one :) |
|
I'm confuse what does the RotatedSecrets do? Anyone who can help me explain it? Thanks. |
|
@seamory You can set Have a look here ory/fosite-example#33 (it's using changes in |
Thanks, i'm clear now. |
Related issue
Issue: #590
Discussed with: @aeneasr
Proposed changes
The aim of this PR is to introduce support for rotation of client's secrets.
New interface is created that extends
fosite.Clientso as not to break compatibility of custom clients. Thefosite.DefaultClientimplements this interface by introducingRotatedSecretsfield. When not set (nil), the functionality stays the same as before.Checklist
and signed the CLA.
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got green light (please contact
security@ory.sh) from the maintainers to push
the changes.
works.
appropriate).
Further comments
Detail of how the change above is utilized in the fosite code:
GetHashedSecretmethodfosite.ClientWithSecretRotationGetRotatedHashesand loop over provided hashes to compare the secretSome questions I have:
checkClientSecretmethod be private, I'm unable to test it directly, so only a few basic scenarios were added testing it via the methods that use it. Should I opt for public method just in order to test it better? Any other preference?