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

add provider ca support for approle auth-method #16259

Merged
merged 2 commits into from Mar 3, 2023

Conversation

eikenb
Copy link
Contributor

@eikenb eikenb commented Feb 14, 2023

Adds support for the approle auth-method. Only handles using the approle role/secret to auth and it doesn't support the agent's extra management configuration options (wrap and delete after read) as they are not required as part of the auth (ie. they are vault agent things).

PR Checklist

@eikenb eikenb added type/enhancement Proposed improvement or new feature theme/connect Anything related to Consul Connect, Service Mesh, Side Car Proxies pr/dependencies PR specifically updates dependencies of project theme/consul-vault Relating to Consul & Vault interactions labels Feb 14, 2023
@github-actions github-actions bot removed the pr/dependencies PR specifically updates dependencies of project label Feb 14, 2023
@eikenb eikenb force-pushed the eikenb/approle-vault-provider-auth branch 2 times, most recently from c7e924e to da46edc Compare February 15, 2023 00:19
@eikenb eikenb changed the title add provider ca support for approle app-method add provider ca support for approle auth-method Feb 15, 2023
@eikenb eikenb force-pushed the eikenb/approle-vault-provider-auth branch from da46edc to 56ac990 Compare February 15, 2023 06:01
@eikenb
Copy link
Contributor Author

eikenb commented Feb 15, 2023

Note that there is a failing test that doesn't test what it seems like it should and makes me unsure about the fields that are required as part of this.

Our docs say the role_id_file_path is a require parameter but we have a test that has the role_id and secret_id set directly in the params.

I'm going to look at this more tomorrow, but wanted to drop a note in case a reviewer drops by.

@eikenb eikenb force-pushed the eikenb/approle-vault-provider-auth branch from 7a14976 to e186242 Compare February 15, 2023 21:38
@eikenb
Copy link
Contributor Author

eikenb commented Feb 15, 2023

Figured out what is going on with that test. Here is the comment I wrote (on the K8s support PR, #16262) to explain what is going on in regard to this...

// Note the authMethod's parameters (Params) is populated from a free form map
// in the configuration where they could hardcode values to be passed directly
// to the auth/*/login endpoint. So each auth method's authentication code
// needs to handle both these cases. The legacy case (which should be
// deprecated) where the user has hardcoded login values directly (eg. a jwt
// string) and the case where they use the configuration option used in the
// vault agent's auth methods.

@kisunji kisunji requested review from a team, rboyer and cthain and removed request for a team February 23, 2023 21:03
@eikenb eikenb force-pushed the eikenb/approle-vault-provider-auth branch 2 times, most recently from 84e26a5 to 99b8aef Compare March 1, 2023 00:57
agent/connect/ca/provider_vault_auth_approle.go Outdated Show resolved Hide resolved
agent/connect/ca/provider_vault_auth_approle.go Outdated Show resolved Hide resolved
agent/connect/ca/provider_vault_auth_approle.go Outdated Show resolved Hide resolved
agent/connect/ca/provider_vault_auth_test.go Show resolved Hide resolved
agent/connect/ca/provider_vault_auth_approle.go Outdated Show resolved Hide resolved
@eikenb eikenb force-pushed the eikenb/approle-vault-provider-auth branch 2 times, most recently from b3b3229 to 4574684 Compare March 2, 2023 19:24
Adds support for the approle auth-method. Only handles using the approle
role/secret to auth and it doesn't support the agent's extra management
configuration options (wrap and delete after read) as they are not
required as part of the auth (ie. they are vault agent things).
@eikenb eikenb force-pushed the eikenb/approle-vault-provider-auth branch from 4574684 to 7cc4d2c Compare March 2, 2023 22:19
Copy link
Contributor

@cthain cthain left a comment

Choose a reason for hiding this comment

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

👍

@eikenb eikenb merged commit 56ffee6 into main Mar 3, 2023
@eikenb eikenb deleted the eikenb/approle-vault-provider-auth branch March 3, 2023 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/connect Anything related to Consul Connect, Service Mesh, Side Car Proxies theme/consul-vault Relating to Consul & Vault interactions type/enhancement Proposed improvement or new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants