Add Support For JWT-Secured Authorization Requests in OIDC Connector#56990
Add Support For JWT-Secured Authorization Requests in OIDC Connector#56990rhammonds-teleport merged 15 commits intomasterfrom
Conversation
| // match for identifier-first login. | ||
| GetUserMatchers() []string | ||
| // GetEnableRequestObjects returns true if the connector should use JWT-Secured Authorization Requests when making auth requests | ||
| GetEnableRequestObjects() bool |
There was a problem hiding this comment.
Instead of a bool, should we have a "mode" like we did for PKCEMode? This might make it easier for us to change defaults or support more than 2 options in the future. (For example, maybe the options are never, always, and auto-detect)
There was a problem hiding this comment.
Not a bad idea. Maybe we should also account for the possibility that we'll want to support "pass by reference" (request_uri) later on as well.
There was a problem hiding this comment.
+1, In the future I could imagine supporting at least the following:
signedsigned_pushed(request_uri)encrypted(also signed by definition)encrypted_pushed
There was a problem hiding this comment.
I've replaced EnableRequestObjects with RequestObjectMode and added associated constants. For now, we support none and signed, but could certainly add more (auto, encrypted, etc) as needed.
The default behavior is that of none (do not use request objects).
There was a problem hiding this comment.
@zmb3 you recommended following string PKCEMode as an example, but should we use a proto enum here, with the same custom marshaling logic used for RequireMFAType, SecondFactorType, etc? Or are we trying to move away from this pattern?
There was a problem hiding this comment.
My first thought was to use an enum as well. Though, I noticed that we have a lot of existing enums defined like so:
enum MySetting {
OFF = 0;
ON = 1;
}
// Will conflict with:
enum OtherSetting {
OFF = 0;
ON = 1;
}
Which causes conflicts in generated C++ code. Buf normally complains about this during linting but maybe we have it disabled. I would've had to get somewhat verbose with my enum values in order to achieve decent namespacing, so I opted to follow suit with PKCEMode.
Still, I'm open to changing it. Maybe there are good arguments for instead using enums that I'm missing.
There was a problem hiding this comment.
@zmb3 you recommended following
string PKCEModeas an example, but should we use a proto enum here, with the same custom marshaling logic used forRequireMFAType,SecondFactorType, etc? Or are we trying to move away from this pattern?
Sure, if we can get the marshalling correct then a proto enum is probably the most semantically correct.
We often fail to get the marshalling correct though, which results in hard-to-interpret configuration and audit events (they show up as integers instead of strings).
There was a problem hiding this comment.
Does the custom marshaling route allow users to specify "signed" instead of 2 if they are managing the connector via the terraform provider? If not, then I would suggest proceeding with a string as that's a major pain point for any terraform users.
There was a problem hiding this comment.
Ah you're right Tim, we should move away from the enum+custom marshaling approach for the terraform provider and other external consumers of the protobuf spec.
| return k.verify(token, expectedClaims) | ||
| } | ||
|
|
||
| // SignArbitrary creates a signed JWT with an arbitrary claims set |
There was a problem hiding this comment.
| // SignArbitrary creates a signed JWT with an arbitrary claims set | |
| // SignAny creates a signed JWT with an arbitrary claims set |
| var kid string | ||
| var err error | ||
| if kid, err = KeyID(k.config.PublicKey); err != nil { |
There was a problem hiding this comment.
| var kid string | |
| var err error | |
| if kid, err = KeyID(k.config.PublicKey); err != nil { | |
| kid, err := KeyID(k.config.PublicKey) | |
| if err != nil { |
This is probably personal preference, but I think the suggestion here better fits with the Teleport-style.
| // SignArbitrary creates a signed JWT with an arbitrary claims set | ||
| func (k *Key) SignAny(claims map[string]any) (string, error) { |
There was a problem hiding this comment.
comment is SignArbitrary, signature is SignAny
But, is there any minimal set of claims that we can make sure are set before signing a JWT? Does it not need at least an issuer and audience or something like that? If so do you think it would be appropriate to have a SignOAuthRequestObject and VerifyOAuthRequestObject method instead of SignAny/ValidateAny? That seems to be what we've done for other types of JWTs
There was a problem hiding this comment.
issuer and audience aren't strictly required per RFC 9101, but you're right that there are a handful of required parameters for authorization requests that should always be included in the JAR object.
I think I can take inspiration from SignParamsJWTSVID which has a PrivateClaims field that can be used to include additional (optional?) claims.
There was a problem hiding this comment.
I've removed SignAny/ValidateAny methods and replaced them with more opinionated SignOIDCAuthRequest/VerifyOIDCAuthRequestToken alternatives. Hopefully these are less likely to be abused/misused. Open to discussing this further if I've missed the mark or if you have any other concerns.
I do feel a bit like I've pushed more OIDC/OAuth domain logic into this package than I would have liked, but I suppose it's just a bit of validation around required claims/parameters.
There was a problem hiding this comment.
from RFC 9101
If signed, the Authorization Request Object SHOULD contain the Claims iss (issuer) and aud (audience) as members with their semantics being the same as defined in the JWT [RFC7519] specification. The value of aud should be the value of the authorization server (AS) issuer, as defined in RFC 8414 [RFC8414].
I guess it's only a SHOULD but since we are the ones issuing these we should probably go ahead and include those
There was a problem hiding this comment.
I do feel a bit like I've pushed more OIDC/OAuth domain logic into this package than I would have liked
I'm aware of this guidance from the RFC, and I was on the fence about whether or not to include these claims. Typically I would say that OIDC client implementation itself is in a better position to make this decision rather than this package. Consider the off chance that we encounter a fussy IdP that dislikes these claims for whatever reason... but that's pretty unlikely. I'll go ahead and add them.
There was a problem hiding this comment.
Added more standard claims (including audience and issuer) in the companion PR.
There was a problem hiding this comment.
Add new configuration parameter to OIDC Connector enable_request_objects which forces the connector to use JWT-Secured Authorization Requests (RFC 9101) when invoking the IdP's authorization endpoint.
What CA will be used to sign the JWT token on Teleport OIDC Relying Party side ?
|
|
||
| var alg jose.SignatureAlgorithm | ||
| if alg, err = AlgorithmForPublicKey(k.config.PublicKey); err != nil { | ||
| return "", err |
There was a problem hiding this comment.
| return "", err | |
| return "", trace.Wrap(err) |
| } | ||
|
|
||
| return k.sign(claims, &jose.SignerOptions{ | ||
| ExtraHeaders: map[jose.HeaderKey]interface{}{ |
There was a problem hiding this comment.
| ExtraHeaders: map[jose.HeaderKey]interface{}{ | |
| ExtraHeaders: map[jose.HeaderKey]any{ |
| var kid string | ||
| var err error | ||
| if kid, err = KeyID(k.config.PublicKey); err != nil { | ||
| return "", err |
There was a problem hiding this comment.
| return "", err | |
| return "", trace.Wrap(err) |
| func (k *Key) validate(rawToken string) (map[string]any, error) { | ||
| tok, err := jwt.ParseSigned(rawToken) | ||
| if err != nil { | ||
| return nil, trace.Errorf("failed to parse jwt") |
There was a problem hiding this comment.
| return nil, trace.Errorf("failed to parse jwt") | |
| return nil, trace.Errorf("failed to parse JWT") |
|
|
||
| return k.sign(claims, &jose.SignerOptions{ | ||
| ExtraHeaders: map[jose.HeaderKey]interface{}{ | ||
| "alg": alg, |
There was a problem hiding this comment.
Does manual setting "alg" in the header is needed ? I thought that this is done by jose library internals.
8eda678 to
35c690d
Compare
The plan is to use the JWT CA for signing these request objects. We expect that the provider can be configured to retrieve public keys from our well-known JWKS endpoint. |
The |
Would you suggest adding a new CA specifically for this purpose then? It might be a little more apparent to administrators the ramifications of rotating their "OIDC" key. edit: I just noticed that we have an |
|
Amplify deployment status
|
| } | ||
|
|
||
| // OIDCAuthRequestClaims defines the required parameters for a JWT-Secured Authorization Request object. | ||
| type OIDCAuthRequestClaims struct { |
There was a problem hiding this comment.
do you think we should embed jwt.Claims like other custom claims types do to include some standard JWT claims? I'm pretty sure we should be including at least an issuer, audience, and expiry
There was a problem hiding this comment.
It seems that my use case doesn't really match up with the intended purpose of this package, so I've removed my additions. There's at least one other package, boundkeypair, that handles JWT signing/serialization itself when it dealing with custom claims, so I'll follow this pattern in the OIDC implementation.
| // VerifyOIDCAuthRequestToken parses and validates a JWT that is to be interpreted as | ||
| // a JWT-Secured Authorization Request (JAR) object. | ||
| func (k *Key) VerifyOIDCAuthRequestToken(rawToken string) (OIDCAuthRequestClaims, error) { | ||
| claims, err := k.verifyOnly(rawToken) | ||
| if err != nil { | ||
| return OIDCAuthRequestClaims{}, trace.Wrap(err, "error verifying authorization request object signature") | ||
| } | ||
|
|
||
| request, err := oidcAuthRequestFromClaims(claims) | ||
| if err != nil { | ||
| return OIDCAuthRequestClaims{}, trace.Wrap(err, "error validating authorization request object parameters") | ||
| } | ||
|
|
||
| return request, nil | ||
| } |
There was a problem hiding this comment.
if this is only used in the test maybe we should define it only in the test file rather than exporting it. Otherwise I think it should be checking the standard JWT claims like expiry, issuedAt, subject, audience, etc.
1f94f7e to
77b2b18
Compare
…nnector to enable the use of JWT-Secured Authorization Requests * Include key id and algorithm in JWT header when signing JARs
* Add a simple 'Validate' function to jwt.Key object allowing users to simply validate a signature and retrieve claims (registered and unregistered)
* Remove inconsistent go-jose/jwt/v5 dependency * Use consistent map type for input/output of 'SignAny' and 'ValidateAny' methods
…ionated OIDC SignOIDCAuthRequestToken and VerifyOIDCAuthRequestToken alternatives
…string 'RequestObjectMode' which gives us more flexibility to support additional request object settings in the future.
… OIDC Auth Request related JWT methods.
…ast 'audience' and 'issuer' claims are set, and perform validation of claims during parsing/verification. * Implement json.Marshaler and json.Unmarshaler interfaces on 'OIDCAuthRequestClaims' so that it can passed to go-jose utilities without pre/post transformation steps.`
77b2b18 to
1c36ad6
Compare
…56990) * POC make signer usable with jwt signing. * Add SignOAuthRequest method. * Update TODO; Add e ref. * * Add new configuration parameter 'enable_request_objects' to oidc connector to enable the use of JWT-Secured Authorization Requests * Include key id and algorithm in JWT header when signing JARs * * Add setter for enabling/disabling JARs on an oidc connector * Add a simple 'Validate' function to jwt.Key object allowing users to simply validate a signature and retrieve claims (registered and unregistered) * Add comment on OIDCConnectorV3 setter * * Rename 'SignOauthRequest' method to more generic 'SignAny' * Remove inconsistent go-jose/jwt/v5 dependency * Use consistent map type for input/output of 'SignAny' and 'ValidateAny' methods * Remove generic JWT signing/verification methods and replace with opinionated OIDC SignOIDCAuthRequestToken and VerifyOIDCAuthRequestToken alternatives * Replace 'EnableRequestObjects' setting on OIDCConnectorSpecV3 with a string 'RequestObjectMode' which gives us more flexibility to support additional request object settings in the future. * Clean up tests, fix formatting, and pick (hopefully) better names for OIDC Auth Request related JWT methods. * Periods at end of godoc comments * Regenerate configuration docs/examples * * Embed jwt.Claims within 'OIDCAuthRequestClaims', require that at least 'audience' and 'issuer' claims are set, and perform validation of claims during parsing/verification. * Implement json.Marshaler and json.Unmarshaler interfaces on 'OIDCAuthRequestClaims' so that it can passed to go-jose utilities without pre/post transformation steps.` * Remove OIDC Auth Request utilities from 'lib/jwt' package. * Regenerate terraform docs --------- Co-authored-by: joerger <bjoerger@goteleport.com>
…56990) * POC make signer usable with jwt signing. * Add SignOAuthRequest method. * Update TODO; Add e ref. * * Add new configuration parameter 'enable_request_objects' to oidc connector to enable the use of JWT-Secured Authorization Requests * Include key id and algorithm in JWT header when signing JARs * * Add setter for enabling/disabling JARs on an oidc connector * Add a simple 'Validate' function to jwt.Key object allowing users to simply validate a signature and retrieve claims (registered and unregistered) * Add comment on OIDCConnectorV3 setter * * Rename 'SignOauthRequest' method to more generic 'SignAny' * Remove inconsistent go-jose/jwt/v5 dependency * Use consistent map type for input/output of 'SignAny' and 'ValidateAny' methods * Remove generic JWT signing/verification methods and replace with opinionated OIDC SignOIDCAuthRequestToken and VerifyOIDCAuthRequestToken alternatives * Replace 'EnableRequestObjects' setting on OIDCConnectorSpecV3 with a string 'RequestObjectMode' which gives us more flexibility to support additional request object settings in the future. * Clean up tests, fix formatting, and pick (hopefully) better names for OIDC Auth Request related JWT methods. * Periods at end of godoc comments * Regenerate configuration docs/examples * * Embed jwt.Claims within 'OIDCAuthRequestClaims', require that at least 'audience' and 'issuer' claims are set, and perform validation of claims during parsing/verification. * Implement json.Marshaler and json.Unmarshaler interfaces on 'OIDCAuthRequestClaims' so that it can passed to go-jose utilities without pre/post transformation steps.` * Remove OIDC Auth Request utilities from 'lib/jwt' package. * Regenerate terraform docs --------- Co-authored-by: joerger <bjoerger@goteleport.com>
…ector (#58063) * Add Support For JWT-Secured Authorization Requests in OIDC Connector (#56990) * POC make signer usable with jwt signing. * Add SignOAuthRequest method. * Update TODO; Add e ref. * * Add new configuration parameter 'enable_request_objects' to oidc connector to enable the use of JWT-Secured Authorization Requests * Include key id and algorithm in JWT header when signing JARs * * Add setter for enabling/disabling JARs on an oidc connector * Add a simple 'Validate' function to jwt.Key object allowing users to simply validate a signature and retrieve claims (registered and unregistered) * Add comment on OIDCConnectorV3 setter * * Rename 'SignOauthRequest' method to more generic 'SignAny' * Remove inconsistent go-jose/jwt/v5 dependency * Use consistent map type for input/output of 'SignAny' and 'ValidateAny' methods * Remove generic JWT signing/verification methods and replace with opinionated OIDC SignOIDCAuthRequestToken and VerifyOIDCAuthRequestToken alternatives * Replace 'EnableRequestObjects' setting on OIDCConnectorSpecV3 with a string 'RequestObjectMode' which gives us more flexibility to support additional request object settings in the future. * Clean up tests, fix formatting, and pick (hopefully) better names for OIDC Auth Request related JWT methods. * Periods at end of godoc comments * Regenerate configuration docs/examples * * Embed jwt.Claims within 'OIDCAuthRequestClaims', require that at least 'audience' and 'issuer' claims are set, and perform validation of claims during parsing/verification. * Implement json.Marshaler and json.Unmarshaler interfaces on 'OIDCAuthRequestClaims' so that it can passed to go-jose utilities without pre/post transformation steps.` * Remove OIDC Auth Request utilities from 'lib/jwt' package. * Regenerate terraform docs --------- Co-authored-by: joerger <bjoerger@goteleport.com> * Allow OIDC Connector to Omit 'max_age' Parameter From Auth Requests (#57950) * Check for 'TELEPORT_OIDC_OMIT_MFA_MAX_AGE' environment variable when adding MFA settings to OIDC connector config. This option allows users to configure teleport to omit the 'max_age' parameter from authorization requests to the IdP in rare cases where this behavior is necessary. * More robust parsing of 'TELEPORT_OIDC_OMIT_MFA_MAX_AGE' value * Need to explicitly set 'MaxAge' to nil when 'TELEPORT_OIDC_OMIT_MFA_MAX_AGE' is enabled, otherwise the base connector's 'MaxAge' setting will persist. --------- Co-authored-by: joerger <bjoerger@goteleport.com>
Add new configuration parameter to OIDC Connector
request_object_modewhich forces the connector to use JWT-Secured Authorization Requests (RFC 9101) when invoking the IdP's authorization endpoint.Changelog: Add support for JWT-Secured Authorization Requests to OIDC Connector