Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolve: Add option to configure key caching duration (#24) #25

Merged
merged 3 commits into from
Jan 30, 2020

Conversation

foxcool
Copy link
Contributor

@foxcool foxcool commented Jan 29, 2020

This PR is just adds cacheDuration parameter. When specified, the default value (15 minutes) is replaced.

@foxcool foxcool requested a review from kpacha January 29, 2020 18:39
@foxcool
Copy link
Contributor Author

foxcool commented Jan 29, 2020

Also 1 test in master is failed

=== RUN TestJWK
--- FAIL: TestJWK (0.00s)
jwk_test.go:15: open cert.pem: no such file or directory

is it ok?

jws.go Outdated Show resolved Hide resolved
@kpacha
Copy link
Member

kpacha commented Jan 29, 2020

@foxcool I think this is a great idea and I'm loving you make it backward compatible. On the other side, I think the TTL should not be too long in order to properly react to a rotation of the keys

regarding your comment, there is a test that requires a cert and key. They are generated with this line from the jwk_test.go:

//go:generate go run $GOROOT/src/crypto/tls/generate_cert.go --rsa-bits 1024 --host 127.0.0.1,::1,localhost --ca --start-date "Jan 1 00:00:00 1970" --duration=1000000h

the CI config calls go generate before launching the test suite

https://github.com/devopsfaith/krakend-jose/blob/master/.travis.yml#L11-L13

@foxcool
Copy link
Contributor Author

foxcool commented Jan 30, 2020

I accept a duration value of less than a second. Maybe I need to increase the step to a second?

@kpacha
Copy link
Member

kpacha commented Jan 30, 2020

I'd change the type of the new property and instead of using a time.Duration I'd go with a plain int representing minutes.

otherwise, the duration will be defined in nanoseconds (https://play.golang.org/p/Hw0QJ7XDgdE)

@foxcool
Copy link
Contributor Author

foxcool commented Jan 30, 2020

I'd change the type of the new property and instead of using a time.Duration I'd go with a plain int representing minutes.

otherwise, the duration will be defined in nanoseconds (https://play.golang.org/p/Hw0QJ7XDgdE)

Yes, this is what I meant in the previous comment. A nanosecond is too small a step. But the minute, it seems to me, is too large. Maybe a second is better?

@kpacha
Copy link
Member

kpacha commented Jan 30, 2020

ouch, I did not understand you... 😄

I think it's ok to use seconds instead of minutes.

@kpacha kpacha merged commit 73e704c into krakend:master Jan 30, 2020
@kpacha
Copy link
Member

kpacha commented Jan 30, 2020

thanks for the contribution!

@github-actions
Copy link

github-actions bot commented Apr 7, 2022

This pull request was marked as resolved a long time ago and now has been automatically locked as there has not been any recent activity after it. You can still open a new issue and reference this link.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants