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.

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

Resetting/rotating client secrets not behaving as expected #1769

Open
ChrisAtHashiCorp opened this issue Oct 17, 2023 · 10 comments
Open

Resetting/rotating client secrets not behaving as expected #1769

ChrisAtHashiCorp opened this issue Oct 17, 2023 · 10 comments
Assignees
Labels
bug triaged Triaged into internal Jira

Comments

@ChrisAtHashiCorp
Copy link

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

Terraform Version

Terraform v1.4.6
on linux_amd64

  • provider registry.terraform.io/okta/okta v4.4.3

Affected Resource(s)

  • okta_app_oauth

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
Copy link
Contributor

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 duytiennguyen-okta added bug triaged Triaged into internal Jira labels Oct 17, 2023
@duytiennguyen-okta
Copy link
Contributor

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

@ChrisAtHashiCorp
Copy link
Author

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 monde self-assigned this Nov 1, 2023
@monde
Copy link
Collaborator

monde commented Nov 1, 2023

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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: #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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug triaged Triaged into internal Jira
Projects
None yet
Development

No branches or pull requests

5 participants