okta / terraform-provider-okta

A Terraform provider to manage Okta resources, enabling infrastructure-as-code provisioning and management of users, groups, applications, and other Okta objects.
https://registry.terraform.io/providers/okta/okta
Mozilla Public License 2.0
255 stars 207 forks source link

Resetting/rotating client secrets not behaving as expected #1769

Open ChrisAtHashiCorp opened 1 year ago

ChrisAtHashiCorp commented 1 year ago

Community Note

Terraform Version

Terraform v1.4.6 on linux_amd64

Affected Resource(s)

Terraform Configuration Files

resource "okta_app_oauth" "example" {
  label                      = "Terraform"
  type                       = "web"
  grant_types                = ["authorization_code"]
  redirect_uris              = ["https://example.com/"]
  response_types             = ["code"]
#  omit_secret               = true
}

Expected Behavior

Client secrets should be rotated by following the documentation located here: https://registry.terraform.io/providers/okta/okta/latest/docs/resources/app_oauth#resetting-client-secret

Can this be done in the Admin UI?

Yes

Can this be done in the actual API call?

Yes

Actual Behavior

The resource gets tainted and is destroyed and rebuilt.

Steps to Reproduce

  1. Follow the documentation located here: https://registry.terraform.io/providers/okta/okta/latest/docs/resources/app_oauth#resetting-client-secret

  2. terraform apply

  3. The results:

alpine318:~/repos/random/okta-pass-rotate$ terraform apply
okta_app_oauth.example: Refreshing state... [id=0oacspkmvfpYIwBdr5d7]

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following
symbols:
-/+ destroy and then create replacement

Terraform will perform the following actions:

  # okta_app_oauth.example must be replaced
-/+ resource "okta_app_oauth" "example" {
      - app_links_json             = jsonencode(
            {
              - oidc_client_link = true
            }
        ) -> null
      - app_settings_json          = jsonencode({})
      ~ client_id                  = "0oacspkmvfpYIwBdr5d7" -> (known after apply)
      + client_secret              = (sensitive value)
      ~ id                         = "0oacspkmvfpYIwBdr5d7" -> (known after apply)
      - implicit_assignment        = false -> null
      - login_scopes               = [] -> null
      ~ logo_url                   = "https://ok12static.oktacdn.com/assets/img/logos/default.6770228fb0dab49a1695ef440a5279bb.png" -> (known after apply)
      ~ name                       = "oidc_client" -> (known after apply)
      ~ omit_secret                = true -> false # forces replacement
      - pkce_required              = false -> null
      - post_logout_redirect_uris  = [] -> null
      ~ sign_on_mode               = "OPENID_CONNECT" -> (known after apply)
        # (20 unchanged attributes hidden)
    }

Plan: 1 to add, 0 to change, 1 to destroy.

Request

There should be a way to create and rotate application client secrets from Terraform (that doesn't involve destroying and rebuilding the resource). It would be nice to possibly be able to create two client secrets for an OAuth app so there can be a grace period while downstream applications are updated with the new client secret: https://developer.okta.com/docs/guides/client-secret-rotation-key/main/#rotate-a-client-secret

exitcode0 commented 1 year ago

Looking at the API docs - I think this would require a new resource

perhaps okta_app_oauth_secret, okta_app_oauth_key, or okta_app_oauth_credential ¯\(ツ)Add New Client Secret

It'd make sense to me to add this for SAML apps as well perhaps okta_app_saml_key to match okta_idp_saml_key ¯\(ツ)Generate Application Key Credential

duytiennguyen-okta commented 1 year ago

OKTA internal reference https://oktainc.atlassian.net/browse/OKTA-658715

ChrisAtHashiCorp commented 1 year ago

Thanks for reviewing this @exitcode0 and @duytiennguyen-okta!

I gave this some thought last night after I submitted it. I agree that breaking out client secrets into a new resource may be the most "correct" way to do this. I also thought adding a new number argument to the okta_app_oauth resource, client_secret_count, that can only be 1 or 2, and then exposing a list[] attribute with the client_secrets may be another way. To rotate them this way you'd increment client_secret_count from 1 to 2, update your downstream applications with the new client secret, then decrement client_secret_count from 2 to 1. Decrementing will then remove the oldest client secret.

Putting that logic in a new resource that allows tainting and better management makes sense though!

monde commented 11 months ago

I think allowing two secrets on the resource as @ChrisAtHashiCorp suggests is the way to go. It's only additive behavior and and we can retain the default behavior so the change is seamless on existing configs during provider upgrades.

@duytiennguyen-okta @exitcode0 what do you think?

exitcode0 commented 11 months ago

I think this approach makes sense and is likely the quickest way to solve this right now

I think that I'd like to eventually see us use a separate resource for app credentials as I think this better aligns with the Terraform best practice of one resource, one api endpoint

tgoodsell-tempus commented 11 months ago

I think this approach makes sense and is likely the quickest way to solve this right now

I think that I'd like to eventually see us use a separate resource for app credentials as I think this better aligns with the Terraform best practice of one resource, one api endpoint

I agree here, based on the API docs: https://developer.okta.com/docs/reference/api/apps/#application-client-secret-management-operations

This definitely should be split out into its own resource, since these do now have things like a unique ID, a active/inactive status, and other things which means the behavior has more nuance that what is currently supported.

Additionally, we should have the next "breaking release" remove the ability to manage the secrets inside of the okta_app_oauth directly, since trying to support all this behavior inside of that resource is a bad idea, and we should avoid having multiple places the same secret/value can be managed.

ChrisAtHashiCorp commented 11 months ago

Great work here team, I truly appreciate it! This request came from a shared customer of ours. Not sure what the best way to name drop them is. I can send one of y'all an email with some details around this request if you'd like so you can tag them in your internal tracker. What's the best way to ship y'all this information?

tgoodsell-tempus commented 11 months ago

I think allowing two secrets on the resource as @ChrisAtHashiCorp suggests is the way to go. It's only additive behavior and and we can retain the default behavior so the change is seamless on existing configs during provider upgrades.

@duytiennguyen-okta @exitcode0 what do you think?

@monde The only thing I'd warn against here is I'm unsure that the standard CRUD API call for apps even understands it's possible to have 2 secrets, and everything else which comes with it. As the okta_app_oauth call is just using the potential client_secret attribute in the base app object, which predates any of this new API / multiple secret stuff.

tgoodsell-tempus commented 11 months ago

Evidence for the above:

  1. I created a okta_app_oauth using client_secret_basic mode and omit_secret set to false.
  2. In the Web UI, I manually "generated a new secret", which adding a second client secret to the created app
  3. Tried to apply a random config change to the app and got the following error:
failed to update OAuth application: the API returned an error: Api validation failed: App Instance. Causes: errorSummary: Cannot update ''client_secret'' when the OAuth2 client has multiple client secrets

Since the GET request to the apps endpoint never returns any secret values or anything, and this isn't some sort of value you can set on the app body (as far as I can tell), you're gonna have to rewrite everything to use the new APIs anyways to support this.

Therefore, it seems more prudent to just go the "new resource" route and start deprecating the direct secret management on the okta_app_oauth resource itself.

tgoodsell-tempus commented 11 months ago

FYI, I created this PR which hopefully helps improve the use of omit_secret in the short term and makes it more clear the function of these different arguments/attributes: https://github.com/okta/terraform-provider-okta/pull/1815

Long term, for me, the only real answer is removing all of this and making that new secrets API the only way to manage these things, as the current way client_secret is handled by the API means that this cannot support the full Terraform lifecycle for an attribute, such as drift detection.