Add Sentry JWT and OIDC Proposal#84
Conversation
Signed-off-by: Jonathan Collinge <jonathancollinge@live.com>
Signed-off-by: Jonathan Collinge <jonathancollinge@live.com>
There was a problem hiding this comment.
Please can you add a bit about private key generation, I.e, sentry does it on first boot. If multiple sentry's boot at the same time, how is first write wins managed? Is it possible to have multiple public keys in the file path that are advertised to enable key rotation?
| - `--jwks-filename` (string): JWKS (JSON Web Key Set) filename | ||
| - `--jwt-issuer` (string): Issuer value for JWT tokens (no issuer if empty) | ||
| - `--jwt-signing-algorithm` (string): Algorithm for JWT signing, must be supported by signing key | ||
| - `--jwt-key-id` (string): Key ID (kid) for JWT signing |
There was a problem hiding this comment.
What’s a key id/what does this mean/do?
Why isn't this managed/set by Sentry internally, rather than configured.
There was a problem hiding this comment.
This is the kid of the signing key. It's in the token and used by clients to uniquely look up the key in the JWKS during validation. I'll use a base64 encoded SHA 256 thumbprint of the key as the default but a user must be able to set it in case they have generated their key/jwks using a different kid.
| - `--jwt-key-id` (string): Key ID (kid) for JWT signing | ||
|
|
||
| **OIDC-related flags:** | ||
| - `--oidc-http-port` (int): Port for the OIDC HTTP server (disabled if 0) |
There was a problem hiding this comment.
Please allow 0 as a valid port (random port provided by the operating system). Default should be unset (nil), meaning disabled.
| **OIDC-related flags:** | ||
| - `--oidc-http-port` (int): Port for the OIDC HTTP server (disabled if 0) | ||
| - `--oidc-jwks-uri` (string): Custom URI for external JWKS access | ||
| - `--oidc-path-prefix` (string): Path prefix for all OIDC HTTP endpoints |
There was a problem hiding this comment.
“All”? Why is there more than one? If I understand this is for serving the oidc discovery endpoint?
Rather than prefix, please can we have explicit url path with sane well known defaults. Prefix style configuration is always confusing/finicky.
There was a problem hiding this comment.
We currently serve the discovery and issuer JWKS endpoints. To be compliant we also have to advertise the authorize endpoint via the discovery doc even though it's not implemented. This is for routing - you need the discovery document endpoints to match the route you used to expose it. I'd rather keep this as path prefix as the combination of issuer and this allow for domain/path routing correctly.
| - `--oidc-http-port` (int): Port for the OIDC HTTP server (disabled if 0) | ||
| - `--oidc-jwks-uri` (string): Custom URI for external JWKS access | ||
| - `--oidc-path-prefix` (string): Path prefix for all OIDC HTTP endpoints | ||
| - `--oidc-domains` (string slice): Allowed domains for OIDC HTTP endpoint requests |
There was a problem hiding this comment.
What does this do? Check the SNI headers for incoming well known http requests? 🤔
There was a problem hiding this comment.
TLS is terminated here, but because you're exposing an unauthenticated HTTP server this just allows you to restrict the hostname in the request to ensure the requests are coming via the expected route e.g. oidc.myissuer.net and not via some internal or alternative domain.
| - `--jwt-enabled` (bool): Enable JWT token issuance by Sentry | ||
| - `--jwt-key-filename` (string): JWT signing key filename | ||
| - `--jwks-filename` (string): JWKS (JSON Web Key Set) filename | ||
| - `--jwt-issuer` (string): Issuer value for JWT tokens (no issuer if empty) |
There was a problem hiding this comment.
Surely this is always the Dapr control plane trust domain, and cannot be configured differently?
There was a problem hiding this comment.
No, this has to match the domain you are exposing your OIDC server on for federation. If third parties are going to oidc.mydomain.net then they expect that to be the issuer in the token. Dapr doesn't use these tokens internally so there's not really a use case where we can assume a value for this as a default but equally it's not necessarily required for all use cases so I've defaulted to omitted.
Co-authored-by: Josh van Leeuwen <me@joshvanl.dev> Signed-off-by: Joni Collinge <jonathancollinge@live.com>
Added a bit on "Propose Key Management". I'm just extending the current Sentry mechanism when generating the keys - I don't see any leadership election etc. so I'm assuming we just accept LWW currently anyway? |
Signed-off-by: Joni Collinge <jonathancollinge@live.com>
|
+1 binding |
1 similar comment
|
+1 binding |
3 participants
Signed-off-by: Jonathan Collinge jonathancollinge@live.com