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

Added support for jwt_supported_algs #345

Merged
merged 1 commit into from
Apr 3, 2019

Conversation

shupp
Copy link
Contributor

@shupp shupp commented Mar 9, 2019

Overview

As of vault 1.0.3, you can configure the list of supported algorithms in a jwt auth backend. This PR adds this support to terraform-provider-vault.

Manual Testing

Config not set:

resource "vault_jwt_auth_backend" "my_jwt_auth" {
  path               = "jwt"
  oidc_discovery_url = "https://myserver.com"
  bound_issuer       = "https://myserver.com"
}
$ vault read auth/jwt/config
Key                       Value
---                       -----
bound_issuer              https://myserver.com
jwt_supported_algs        []
jwt_validation_pubkeys    []
oidc_discovery_ca_pem     n/a
oidc_discovery_url        https://myserver.com

Config with one option:

resource "vault_jwt_auth_backend" "my_jwt_auth" {
  path               = "jwt"
  oidc_discovery_url = "https://myserver.com"
  bound_issuer       = "https://myserver.com"
  jwt_supported_algs = ["RS512"]
}
$ vault read auth/jwt/config
Key                       Value
---                       -----
bound_issuer              https://myserver.com
jwt_supported_algs        [RS512]
jwt_validation_pubkeys    []
oidc_discovery_ca_pem     n/a
oidc_discovery_url        https://myserver.com

Config with two options:

resource "vault_jwt_auth_backend" "my_jwt_auth" {
  path               = "jwt"
  oidc_discovery_url = "https://myserver.com"
  bound_issuer       = "https://myserver.com"
  jwt_supported_algs = ["RS256","RS512"]
}
$ vault read auth/jwt/config
Key                       Value
---                       -----
bound_issuer              https://myserver.com
jwt_supported_algs        [RS256 RS512]
jwt_validation_pubkeys    []
oidc_discovery_ca_pem     n/a
oidc_discovery_url        https://myserver.com

Unit Testing

$ make test
==> Checking that code complies with gofmt requirements...
go test -i $(go list ./... |grep -v 'vendor') || exit 1
echo $(go list ./... |grep -v 'vendor') | \
                xargs -t -n4 go test  -timeout=30s -parallel=4
go test -timeout=30s -parallel=4 github.com/terraform-providers/terraform-provider-vault github.com/terraform-providers/terraform-provider-vault/util github.com/terraform-providers/terraform-provider-vault/vault
?       github.com/terraform-providers/terraform-provider-vault [no test files]
ok      github.com/terraform-providers/terraform-provider-vault/util    0.022s
ok      github.com/terraform-providers/terraform-provider-vault/vault   0.044s

Copy link
Contributor

@tyrannosaurus-becks tyrannosaurus-becks left a comment

Choose a reason for hiding this comment

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

This looks very good! Would it be possible to get just one sunny path test displaying how this value should be populated? I see you have it in the PR commentary, which is great, but once it's merged it'll essentially "disappear" in the code itself; yet it's very useful. A test would help the example be evident on an ongoing basis.

@@ -38,6 +38,8 @@ The following arguments are supported:

* `jwt_validation_pubkeys` - (Optional) A list of PEM-encoded public keys to use to authenticate signatures locally. Cannot be used in combination with `oidc_discovery_url`

* `jwt_supported_algs` - (Optional) A list of supported signing algorithms. Defaults to [RS256]
Copy link
Contributor

Choose a reason for hiding this comment

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

Since it's being defaulted by Vault rather than by the Terraform Vault Provider, could we note that more explicitly here? Something like Vault 1.1.0 currently defaults to RS256 but future or past versions of Vault may differ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tyrannosaurus-becks it's actually the default of the oidc dependency that go-oidc package. The vault-plugin-jwt-auth package depends on that and uses its defaults by using an empty string slice. Not sure if that changes your preference on language, but wanted to be clear here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Totally! Yes, I'm aware of that. I was thinking it'd be good to note that it ties to Vault because of how most people will be using the version of the plugin that's included in the Vault catalog at the time of a particular release. However, if you think it makes more sense to have the language refer to the plugin instead, that's fine with me.

@tyrannosaurus-becks tyrannosaurus-becks self-assigned this Mar 29, 2019
@shupp
Copy link
Contributor Author

shupp commented Mar 29, 2019

@tyrannosaurus-becks Sure, I can turn the PR contents into a test.

@ghost ghost added size/S and removed size/XS labels Apr 2, 2019
@shupp
Copy link
Contributor Author

shupp commented Apr 2, 2019

Was not using make to test, will fix this shortly.

@shupp
Copy link
Contributor Author

shupp commented Apr 3, 2019

@tyrannosaurus-becks tests fixed up, ready for another look when you have a chance.

Copy link
Contributor

@tyrannosaurus-becks tyrannosaurus-becks left a comment

Choose a reason for hiding this comment

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

@shupp awesome! Thank you!

@tyrannosaurus-becks tyrannosaurus-becks merged commit 53add2d into hashicorp:master Apr 3, 2019
@shupp shupp deleted the jwt-config branch April 3, 2019 16:36
dandandy pushed a commit to dandandy/terraform-provider-vault that referenced this pull request Jun 17, 2021
Added support for jwt_supported_algs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants