Skip to content

Allow OIDC Connector to Omit 'max_age' Parameter From Auth Requests#57950

Merged
rhammonds-teleport merged 3 commits intomasterfrom
rhammonds/oidc-mfa-enhancements
Aug 15, 2025
Merged

Allow OIDC Connector to Omit 'max_age' Parameter From Auth Requests#57950
rhammonds-teleport merged 3 commits intomasterfrom
rhammonds/oidc-mfa-enhancements

Conversation

@rhammonds-teleport
Copy link
Copy Markdown
Contributor

By default, max_age is set to 0 when a connector is configured for MFA. Some users have requested the ability to omit this parameter entirely as their IdP will reject the request when present.

Comment thread api/types/oidc.go Outdated
Copy link
Copy Markdown
Collaborator

@zmb3 zmb3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One final suggestion.

Comment thread api/types/oidc.go Outdated
Comment on lines +582 to +585
omitMaxAge, err := strconv.ParseBool(os.Getenv("TELEPORT_OIDC_OMIT_MFA_MAX_AGE"))
if err != nil {
return trace.Wrap(err, "Invalid value for 'TELEPORT_OIDC_OMIT_MFA_MAX_AGE'")
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
omitMaxAge, err := strconv.ParseBool(os.Getenv("TELEPORT_OIDC_OMIT_MFA_MAX_AGE"))
if err != nil {
return trace.Wrap(err, "Invalid value for 'TELEPORT_OIDC_OMIT_MFA_MAX_AGE'")
}
omitMaxAge, _ := strconv.ParseBool(os.Getenv("TELEPORT_OIDC_OMIT_MFA_MAX_AGE"))

No need for an error check here. An invalid value returns a non-nil error, but also returns false, which is exactly the behavior we want.

@rhammonds-teleport rhammonds-teleport marked this pull request as ready for review August 15, 2025 17:16
@github-actions github-actions bot requested review from GavinFrazar and Tener August 15, 2025 17:17
@rhammonds-teleport
Copy link
Copy Markdown
Contributor Author

One last change - I was doing some final validation and realized that if the base connector already has MaxAge configured, then this code path needs to explicitly set it back to nil, otherwise the base MaxAge will persist.

@Joerger @zmb3 could you just give me a thumbs up/down before I hit merge? I don't want to sneak commits in after review.

@rhammonds-teleport rhammonds-teleport added the no-changelog Indicates that a PR does not require a changelog entry label Aug 15, 2025
Copy link
Copy Markdown
Contributor

@Joerger Joerger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, LGTM

…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.
…AX_AGE' is enabled, otherwise the base connector's 'MaxAge' setting will persist.
@rhammonds-teleport rhammonds-teleport force-pushed the rhammonds/oidc-mfa-enhancements branch from 7b341a1 to 413489c Compare August 15, 2025 17:34
@rhammonds-teleport rhammonds-teleport added this pull request to the merge queue Aug 15, 2025
Merged via the queue into master with commit 2c8a61c Aug 15, 2025
40 checks passed
@rhammonds-teleport rhammonds-teleport deleted the rhammonds/oidc-mfa-enhancements branch August 15, 2025 18:24
Copy link
Copy Markdown
Contributor

@Tener Tener left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rhammonds-teleport is there a doc PR open for this?

Comment thread api/types/oidc.go
Value: o.Spec.MFASettings.MaxAge,
// In rare cases, some providers will complain about the presence of the 'max_age'
// parameter in auth requests. Provide users with a workaround to omit it.
omitMaxAge, _ := strconv.ParseBool(os.Getenv("TELEPORT_OIDC_OMIT_MFA_MAX_AGE"))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to make sure we don't lose that workaround, can you make sure we document the correct way to use this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that our intention is to avoid advertising this option in the docs. While some users really do need this option, it's a potentially significant security footgun for most.

I can (and probably should) add a corresponding test case to the OIDC implementation that will squawk if this behavior is accidentally removed or broken though.

rhammonds-teleport added a commit that referenced this pull request Aug 18, 2025
…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.
rhammonds-teleport added a commit that referenced this pull request Aug 18, 2025
…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.
github-merge-queue bot pushed a commit that referenced this pull request Aug 18, 2025
…ector (#58013)

* Add 'RequestObjectMode' to' OIDCConnectorSpecV3' allowing us to configure the connector to use signed JWT-Secured Authorization Requests during authentication.

* Bring in 'SigningKeyFromPrivateKey' required by 'rhammonds/backport-6931-branch/v17'

* 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.

* regenerate docs
rhammonds-teleport added a commit that referenced this pull request Aug 19, 2025
…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.
github-merge-queue bot pushed a commit that referenced this pull request Aug 19, 2025
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog Indicates that a PR does not require a changelog entry size/sm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants