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.

Sign up for GitHub

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

feat: SSO MFA - support for OIDC MaxAge #47292

Merged
merged 5 commits into from
Oct 16, 2024
Merged

feat: SSO MFA - support for OIDC MaxAge #47292

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
100a811
Add max age to oidc mfa settings.
Joerger Oct 7, 2024
bb927c2
Validate oidc max age.
Joerger Oct 7, 2024
ad15a58
Update error message.
Joerger Oct 8, 2024
9899204
Upate terraform schema and docs and crds.
Joerger Oct 8, 2024
b044103
Merge branch 'master' into joerger/oidc-mfa-max-age
Joerger Oct 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions api/proto/teleport/legacy/types/types.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4600,6 +4600,10 @@ message OIDCConnectorMFASettings {
// Prompt is an optional OIDC prompt. An empty string omits prompt.
// If not specified, it defaults to select_account for backwards compatibility.
string prompt = 5;
// MaxAge is the amount of time in nanoseconds that an IdP session is valid for. Defaults to
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make this a proper duration instead of requiring the value be defined in nanoseconds?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems to cause issues - #29815 (comment)

I'll give it a try though.

Copy link
Contributor Author

@Joerger Joerger Oct 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

google.protobuf.Duration max_age = 6 [(gogoproto.stdduration) = true];

Resulted in it not parsing from a string like 10s.

google.protobuf.Duration max_age = 6;

Would not parse from an int or a string like 10s, seems completely broken. Looks like we map - Mgoogle/protobuf/duration.proto=github.com/gogo/protobuf/types, and gogoproto ofc has issues so I'm guessing changing this would be a huge pain.

google.protobuf.Duration max_age = 6 [
    (gogoproto.stdduration) = true,
    (gogoproto.casttype) = "Duration"
  ];

This works, though I don't think it's any better than the int64 as it still relies on gogoproto to overrule the actual type. I think it's essentially the same, but we can go with this.

Edit: nevermind, this breaks the protobuf gen file, might work without the (gogoproto.stdduration) = true

Edit2: Nope that breaks too. I don't see a way forward other than int64.

@codingllama any opinion on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

codingllama any opinion on this?

Yes, gogo was a terrible idea. :P

Why does it not parse? In what way does it break?

No strong opinions on my part about using int64 here - I would prefer the stronger type but I know how much of a pain it can be on public-facing types (and on legacy/ to boot).

// 0 to always force re-authentication for MFA checks. This should only be set to a non-zero
// value if the IdP is setup to perform MFA checks on top of active user sessions.
int64 max_age = 6 [(gogoproto.casttype) = "Duration"];
}

// OIDCAuthRequest is a request to authenticate with OIDC
Expand Down
29 changes: 19 additions & 10 deletions api/types/oidc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ type OIDCConnector interface {
// GetClientRedirectSettings returns the client redirect settings.
GetClientRedirectSettings() *SSOClientRedirectSettings
// GetMFASettings returns the connector's MFA settings.
GetMFASettings() OIDCConnectorMFASettings
GetMFASettings() *OIDCConnectorMFASettings
// IsMFAEnabled returns whether the connector has MFA enabled.
IsMFAEnabled() bool
// WithMFASettings returns the connector will some settings overwritten set from MFA settings.
Expand Down Expand Up @@ -465,7 +465,17 @@ func (o *OIDCConnectorV3) CheckAndSetDefaults() error {
return trace.BadParameter("max_age cannot be negative")
}
if maxAge.Round(time.Second) != maxAge {
return trace.BadParameter("max_age must be a multiple of seconds")
return trace.BadParameter("max_age %q is invalid, cannot have sub-second units", maxAge.String())
}
}

if o.Spec.MFASettings != nil {
maxAge := o.Spec.MFASettings.MaxAge.Duration()
if maxAge < 0 {
return trace.BadParameter("max_age cannot be negative")
}
if maxAge.Round(time.Second) != maxAge {
return trace.BadParameter("max_age %q invalid, cannot have sub-second units", maxAge.String())
}
}

Expand Down Expand Up @@ -514,18 +524,14 @@ func (o *OIDCConnectorV3) GetClientRedirectSettings() *SSOClientRedirectSettings
}

// GetMFASettings returns the connector's MFA settings.
func (o *OIDCConnectorV3) GetMFASettings() OIDCConnectorMFASettings {
if o.Spec.MFASettings == nil {
return OIDCConnectorMFASettings{
Enabled: false,
}
}
return *o.Spec.MFASettings
func (o *OIDCConnectorV3) GetMFASettings() *OIDCConnectorMFASettings {
return o.Spec.MFASettings
}

// IsMFAEnabled returns whether the connector has MFA enabled.
func (o *OIDCConnectorV3) IsMFAEnabled() bool {
return o.GetMFASettings().Enabled
mfa := o.GetMFASettings()
return mfa != nil && mfa.Enabled
}

// WithMFASettings returns the connector will some settings overwritten set from MFA settings.
Expand All @@ -538,6 +544,9 @@ func (o *OIDCConnectorV3) WithMFASettings() error {
o.Spec.ClientSecret = o.Spec.MFASettings.ClientSecret
o.Spec.ACR = o.Spec.MFASettings.AcrValues
o.Spec.Prompt = o.Spec.MFASettings.Prompt
o.Spec.MaxAge = &MaxAge{
Value: o.Spec.MFASettings.MaxAge,
}
return nil
}

Expand Down
3,462 changes: 1,747 additions & 1,715 deletions api/types/types.pb.go
View file
Open in desktop

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions docs/pages/reference/operator-resources/resources.teleport.dev_oidcconnectors.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,5 +68,6 @@ resource, which you can apply after installing the Teleport Kubernetes operator.
|client_id|string|ClientID is the OIDC OAuth app client ID.|
|client_secret|string|ClientSecret is the OIDC OAuth app client secret.|
|enabled|boolean|Enabled specified whether this OIDC connector supports MFA checks. Defaults to false.|
|max_age|string|MaxAge is the amount of time in nanoseconds that an IdP session is valid for. Defaults to 0 to always force re-authentication for MFA checks. This should only be set to a non-zero value if the IdP is setup to perform MFA checks on top of active user sessions.|
|prompt|string|Prompt is an optional OIDC prompt. An empty string omits prompt. If not specified, it defaults to select_account for backwards compatibility.|

1 change: 1 addition & 0 deletions docs/pages/reference/terraform-provider/data-sources/oidc_connector.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ Optional:
- `client_id` (String) ClientID is the OIDC OAuth app client ID.
- `client_secret` (String) ClientSecret is the OIDC OAuth app client secret.
- `enabled` (Boolean) Enabled specified whether this OIDC connector supports MFA checks. Defaults to false.
- `max_age` (String) MaxAge is the amount of time in nanoseconds that an IdP session is valid for. Defaults to 0 to always force re-authentication for MFA checks. This should only be set to a non-zero value if the IdP is setup to perform MFA checks on top of active user sessions.
- `prompt` (String) Prompt is an optional OIDC prompt. An empty string omits prompt. If not specified, it defaults to select_account for backwards compatibility.


Expand Down
1 change: 1 addition & 0 deletions docs/pages/reference/terraform-provider/resources/oidc_connector.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ Optional:
- `client_id` (String) ClientID is the OIDC OAuth app client ID.
- `client_secret` (String) ClientSecret is the OIDC OAuth app client secret.
- `enabled` (Boolean) Enabled specified whether this OIDC connector supports MFA checks. Defaults to false.
- `max_age` (String) MaxAge is the amount of time in nanoseconds that an IdP session is valid for. Defaults to 0 to always force re-authentication for MFA checks. This should only be set to a non-zero value if the IdP is setup to perform MFA checks on top of active user sessions.
- `prompt` (String) Prompt is an optional OIDC prompt. An empty string omits prompt. If not specified, it defaults to select_account for backwards compatibility.


Expand Down
8 changes: 8 additions & 0 deletions ...cluster/charts/teleport-operator/operator-crds/resources.teleport.dev_oidcconnectors.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,14 @@ spec:
description: Enabled specified whether this OIDC connector supports
MFA checks. Defaults to false.
type: boolean
max_age:
description: MaxAge is the amount of time in nanoseconds that
an IdP session is valid for. Defaults to 0 to always force re-authentication
for MFA checks. This should only be set to a non-zero value
if the IdP is setup to perform MFA checks on top of active user
sessions.
format: duration
type: string
prompt:
description: Prompt is an optional OIDC prompt. An empty string
omits prompt. If not specified, it defaults to select_account
Expand Down
8 changes: 8 additions & 0 deletions integrations/operator/config/crd/bases/resources.teleport.dev_oidcconnectors.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,14 @@ spec:
description: Enabled specified whether this OIDC connector supports
MFA checks. Defaults to false.
type: boolean
max_age:
description: MaxAge is the amount of time in nanoseconds that
an IdP session is valid for. Defaults to 0 to always force re-authentication
for MFA checks. This should only be set to a non-zero value
if the IdP is setup to perform MFA checks on top of active user
sessions.
format: duration
type: string
prompt:
description: Prompt is an optional OIDC prompt. An empty string
omits prompt. If not specified, it defaults to select_account
Expand Down
44 changes: 44 additions & 0 deletions integrations/terraform/tfschema/types_terraform.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading