cmd/dex: expose IDTokensValidFor and RotateKeysAfter server options in config #665
Conversation
|
@ericchiang I know we discussed that I should ParseDuration before the gRPC code, but since there is a possibility that both the durations might not be provided, I thought it would be easier to do it after defining serverConfig. Let me know if you want me to change it. |
|
Travis caught some issues with the struct tags. Please take a look. |
| @@ -215,3 +216,12 @@ func (c *Connector) UnmarshalYAML(unmarshal func(interface{}) error) error { | |||
| } | |||
| return err | |||
| } | |||
|
|
|||
| // Expiry holds various expiration time durations | |||
There was a problem hiding this comment.
Maybe "Expiry holds configuration for the validity period of components."
| if c.Expiry.SigningKeys != "" { | ||
| signingKeys, err := time.ParseDuration(c.Expiry.SigningKeys) | ||
| if err != nil { | ||
| return fmt.Errorf("parsing SigningKeys expiry: %v", err) |
There was a problem hiding this comment.
nit: signingKeys (since that's the config field).
| if c.Expiry.IDTokens != "" { | ||
| idTokens, err := time.ParseDuration(c.Expiry.IDTokens) | ||
| if err != nil { | ||
| return fmt.Errorf("parsing IdTokens expiry: %v", err) |
There was a problem hiding this comment.
same nit: lowercase idTokens to correspond to config field
|
Sorry, I appear to have caused a merge conflict :/ |
|
lgtm |
|
Has there been any effort to add a webhook for this? The SP doesn't know exactly when Dex refreshes its signing keys, so I was wondering whether there was any interest in making Dex push to its static clients a HTTP request to say its keys have been rotated. |
|
chrisdone@ key endpoints have expiry data in a Cache-Control HTTP header. You can just use that :) you definitely don't need a webhook for this. |
|
@ericchiang Thanks. Upon reflection, neither the webhook nor the expiry data are sufficient to avoid race conditions:
In this situation, the expiry datetime is essentially advisory, to reduce cache misses. A webhook would mitigate the time skew and restarts, causing fewer cache misses, but is also advisory. I will have to implement an on-demand system in SP which requests an update from Dex on cache misses (whenever JWKs fail to decode a JWT in an attempted authentication). The only missing piece of the puzzle is to avoid DoS attacks causing intentional cache misses. Some kind of rate limit of e.g. 5 minutes will probably have to do. 🤔 |
|
I don't maintain dex anymore, so you probably want to open up a new issue.
You probably want to figure out if there's a bug before jumping to
solutions (and again a webhook is overkill). For example, dex's key
rotation is based on its database, not an in-memory value, so restarting
won't impact its rotation schedule. You can also look at the go-oidc client
to figure out how it details with caching https://github.com/coreos/go-oidc.
We haven't had any reported issues with that logic so far, so this might be
a bug in your OpenID Connect client that needs fixing.
…On Thu, Jun 18, 2020 at 8:50 AM Chris Done ***@***.***> wrote:
@ericchiang <https://github.com/ericchiang> Thanks.
Upon reflection, neither the webhook nor the expiry data are sufficient to
avoid race conditions:
- Due to time skew where the SP and Dex's times become out of sync, SP
can jump the gun.
- Dex could be restarted or re-provisioned during the expiry interval,
meaning that the JWKs would be rotated earlier than expected.
In this situation, the expiry datetime is essentially *advisory*, to
reduce cache misses. A webhook would mitigate the time skew and restarts,
causing fewer cache misses, but is also *advisory*.
I will have to implement an on-demand system in SP which requests an
update from Dex on cache misses (whenever JWKs fail to decode a JWT in an
attempted authentication).
The only missing piece of the puzzle is to avoid DoS attacks causing
intentional cache misses. Some kind of rate limit of e.g. 5 minutes will
probably have to do. 🤔
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#665 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAR36XNQLHWCPBQCAHCHBOTRXIZSVANCNFSM4CU75MKQ>
.
|
closes #660.
@ericchiang