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

feat(provider): add support for pre(external) auth'd session tokens #1441

Conversation

vanillaSprinkles
Copy link
Contributor

@vanillaSprinkles vanillaSprinkles commented Jul 15, 2024

adds provider config inputs:

  • env vars: PROXMOX_VE_AUTH_PAYLOAD; PROXMOX_VE_AUTH_TICKET with PROXMOX_VE_CSRF_PREVENTION_TOKEN
  • provider-config: auth_payload; auth_ticket with csrf_prevention_token

Contributor's Note

  • I have added / updated documentation in /docs for any user-facing features or additions.
  • I have added / updated acceptance tests in /fwprovider/tests for any new or updated resources / data sources.
  • I have ran make example to verify that the change works as expected.
    • i did not run the make example but i have ran this in my own env for testing the new provider config inputs

Proof of Work

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment 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 pull request followers and do not help prioritize the request

Closes #0000 | Relates #0000

@vanillaSprinkles vanillaSprinkles force-pushed the vs/main/feat_provider_add_support_forpre_auth_session_tokens branch 13 times, most recently from 640ebc8 to 16f474d Compare July 18, 2024 08:08
@bpg
Copy link
Owner

bpg commented Jul 18, 2024

Hi @vanillaSprinkles! 👋🏼

Would you mind to sign-off your commits? You can find instructions here

@vanillaSprinkles
Copy link
Contributor Author

vanillaSprinkles commented Jul 19, 2024

Hi @vanillaSprinkles! 👋🏼

Would you mind to sign-off your commits? You can find instructions here

yep will do - was hoping to get all the lint'ing fun done, remove the ancient comments, and re-run the binary first

is the final lint-error due to the commented lines?

edit:
[ fixed ] well im getting a weird segfault from my lint-fix commit so i'm gonna drill into the problem

edit2:
getting close. sign-off added

got one issue where env-var PROXMOX_VE_API_TOKEN is not being injested (does not even give me an error on an invalid input). and gotta cleanup my comments.

@vanillaSprinkles vanillaSprinkles force-pushed the vs/main/feat_provider_add_support_forpre_auth_session_tokens branch 4 times, most recently from 2be1719 to 60e53ca Compare July 20, 2024 21:15
@vanillaSprinkles vanillaSprinkles force-pushed the vs/main/feat_provider_add_support_forpre_auth_session_tokens branch 6 times, most recently from 59b931c to 4105e17 Compare July 23, 2024 21:48
@bpg
Copy link
Owner

bpg commented Sep 6, 2024

Hey @vanillaSprinkles 👋🏼

I did a second pass over the code, and made a few changes, mostly to untangle credentials management. Everything works fine in my environment, but would you mind run a few tests in yours, to make sure all works as expected for you? And feel free to comment on my code as well :)

@vanillaSprinkles
Copy link
Contributor Author

hey @bpg ! it has a stacktrace on the auth-ticket and csrf (via env-var, untested via tf-file); though i didnt capture it. I am really diggin the class refactor (turns my hack into a thing of beauty!) .. but sadly, i keep getting distracted, so hoping friday i'll get to sit and tinker a bit more (and hopefully be able to commit something for testing the cred-paths)

btw do you use a class object visualizer/mapper for golang?

@vanillaSprinkles
Copy link
Contributor Author

vanillaSprinkles commented Sep 26, 2024

here's the stack trace details; i have not dug into the code yet - built from b3fcb2a (not updated the branch in a while)
i hope to dive deeper into the code soon(tm) - will edit this thread if i find anything provided you dont comment after..

i've whipped up a shell loop to pass in several variations of credentials (pass + fail) via env-vars; going to have it write dynamically to a .tf file for static provider testing next, then will push this shell-monster up as well.

panic: interface conversion: interface {} is nil, not string

goroutine 12 [running]:
github.com/bpg/terraform-provider-proxmox/proxmoxtf/provider.providerConfigure({0x102073d?, 0x6?}, 0xc000149600)
        /repo/forks/bpg__terraform-provider-proxmox/proxmoxtf/provider/provider.go:183 +0x25a5
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.(*Provider).Configure(0xc00037ce00, {0x1230ae8, 0xc000242450}, 0xc0004929b0)
        /repo/go/pkg/mod/github.com/hashicorp/terraform-plugin-sdk/v2@v2.34.0/helper/schema/provider.go:369 +0x228
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.(*GRPCProviderServer).ConfigureProvider(0xc0002a4e28, {0x1230ae8?, 0xc000235ad0?}, 0xc00005a800)
        /repo/go/pkg/mod/github.com/hashicorp/terraform-plugin-sdk/v2@v2.34.0/helper/schema/grpc_provider.go:616 +0x3c5
github.com/hashicorp/terraform-plugin-mux/tf5to6server.v5tov6Server.ConfigureProvider({{0x123d658?, 0xc0002a4e28?}}, {0x1230ae8?, 0xc000235ad0?}, 0xc00005a220?)
        /repo/go/pkg/mod/github.com/hashicorp/terraform-plugin-mux@v0.16.0/tf5to6server/tf5to6server.go:60 +0x1e9
github.com/hashicorp/terraform-plugin-mux/tf6muxserver.(*muxServer).ConfigureProvider(0xc00037ce70, {0x1230ae8?, 0xc00048ad50?}, 0xc00005a220)
        /repo/go/pkg/mod/github.com/hashicorp/terraform-plugin-mux@v0.16.0/tf6muxserver/mux_server_ConfigureProvider.go:28 +0x134
github.com/hashicorp/terraform-plugin-go/tfprotov6/tf6server.(*server).ConfigureProvider(0xc00037e280, {0x1230ae8?, 0xc00048a4b0?}, 0xc000492140)
        /repo/go/pkg/mod/github.com/hashicorp/terraform-plugin-go@v0.23.0/tfprotov6/tf6server/server.go:559 +0x342
github.com/hashicorp/terraform-plugin-go/tfprotov6/internal/tfplugin6._Provider_ConfigureProvider_Handler({0xfef660, 0xc00037e280}, {0x1230ae8, 0xc00048a4b0}, 0xc000148000, 0x0)
        /repo/go/pkg/mod/github.com/hashicorp/terraform-plugin-go@v0.23.0/tfprotov6/internal/tfplugin6/tfplugin6_grpc.pb.go:464 +0x1a6
google.golang.org/grpc.(*Server).processUnaryRPC(0xc000170a00, {0x1230ae8, 0xc00048a420}, {0x1238320, 0xc0004fa000}, 0xc00021c000, 0xc00043e0c0, 0x191d710, 0x0)
        /repo/go/pkg/mod/google.golang.org/grpc@v1.63.2/server.go:1369 +0xdf8
google.golang.org/grpc.(*Server).handleStream(0xc000170a00, {0x1238320, 0xc0004fa000}, 0xc00021c000)
        /repo/go/pkg/mod/google.golang.org/grpc@v1.63.2/server.go:1780 +0xe8b
google.golang.org/grpc.(*Server).serveStreams.func2.1()
        /repo/go/pkg/mod/google.golang.org/grpc@v1.63.2/server.go:1019 +0x7f
created by google.golang.org/grpc.(*Server).serveStreams.func2 in goroutine 11
        /repo/go/pkg/mod/google.golang.org/grpc@v1.63.2/server.go:1030 +0x125

edit: i dont think you edited the flow of the PROXMOX_VE_OTP being sent in; if im not mistaken this should be the totp digits right?
that is failing (set via env-vars) with (test is a single tf file with a single data-object of the vms):
Error: error retrieving VMs: failed to authenticate HTTP GET request (path: nodes/peemoxve/qemu) - Reason: failed to authenticate: failed to authenticate: received an HTTP 401 response - Reason: authentication failure

@bpg
Copy link
Owner

bpg commented Sep 27, 2024

hey @vanillaSprinkles
speaking about the panic, I think the error is coming from here:

if v, ok := sshConf[mkProviderSSHUsername]; !ok || v.(string) == "" {
if sshUsername != "" {
sshConf[mkProviderSSHUsername] = sshUsername
} else if creds.UserCredentials != nil {
sshConf[mkProviderSSHUsername] = strings.Split(creds.UserCredentials.Username, "@")[0]
}
}

It looks like your provider's config does not have ssh.username neither username defined, but we need some username for the SSH connection. I guess we can set sshConf[mkProviderSSHUsername] = "" and move on if both conditions there are false.

OTP is part of the UserCredentials now, and will be initialized only if neither token nor ticket credentials are provided:

tok, err := newTokenCredentials(apiToken)
if err == nil {
return Credentials{TokenCredentials: &tok}, nil
}
tic, err := newTicketCredentials(authTicket, csrfPreventionToken)
if err == nil {
return Credentials{TicketCredentials: &tic}, nil
}
usr, err := newUserCredentials(username, password, otp)
if err == nil {
return Credentials{UserCredentials: &usr}, nil
}

I guess that's not your use case? 😅 Could you post an example of the config you're testing with?

@bpg
Copy link
Owner

bpg commented Sep 27, 2024

btw do you use a class object visualizer/mapper for golang?

no, not sure there is such thing for golang, as it doesn't really have objects / inheritance it the traditional OOP way.

@vanillaSprinkles
Copy link
Contributor Author

vanillaSprinkles commented Sep 27, 2024

hey @bpg
(interesting re my ask on the class grapher - thanks much appreciated!)

(test setup only has a single data-object just to get the provider kicked in) - config is simply 100% api (no ssh), blank provider config (nothing in there except insecure = true , version in required-providers to toggle stuff but will simplify that soon); using env-vars for everything else (hope you didnt mean for me to paste it all up here, but can do)

i know some items require ssh'ing in but in my later-setup i plan to have a particular workspace for that (and hope proxmox-api's expand to not require ssh for some features, and instead to use api)

env-vars:

  • PROXMOX_VE_ENDPOINT

with one of:

  • PROXMOX_VE_AUTH_TICKET PROXMOX_VE_CSRF_PREVENTION_TOKEN
  • PROXMOX_VE_PASSWORD PROXMOX_VE_USERNAME (and added the PROXMOX_VE_OTP for some tests - i was thinking that was the 6 digits from the totp but i dont have a full grasp of what proxmox offers via the OTP part - i think i have my understanding wrong)

my cred-tester shell-loop only runs through env-vars at the moment vs dropping into the provider block; once i nail that part out i'll send it up.

was able to see the stracktrace with PROXMOX_VE_CSRF_PREVENTION_TOKEN='19870727:abc' PROXMOX_VE_AUTH_TICKET='PVE:user@pve:19870727::def' tofu plan -> i'll pull your latest down and give it a run through in a bit.

Edit;

  • so i find this interesting: ive begun to move my cred-tester into the repo, with a very basic setup (using the makefile's args for tf work with it) .. and i get no stack trace with the auth-ticket + csrf env-vars.
  • copied this exact folder over to my live setup (into a sub-dir) and ran it similarly.. and no stack trace
  • i do get the stack trace (not rebased with your latest from tonight) when i inject the local-built into a .terraform/providers/.../bla file, named appropriatly, and also with the files h1 sha injected into the .terraform.lock.hcl - werid - i dont quite understand the why yet
  • and pulling in the latest that you sent, and running it with the above setup and the stack trace is gone; w00t

i'll re-test again tomorrow after some sleep - thanks again !

@vanillaSprinkles
Copy link
Contributor Author

@bpg heya!
tl;dr: all testing ran fine on my end; code looks good to me as well (maybe remove OTP in the authorization request but probably best for its own PR) - only a minor easy to fix merge conflict at this point

overkill details:

im used to full local rebases and as such i did not want to re-write history since your class refactor is your awesomness - i rebased locally and can force-push if you want (with the merge conflict using upstream fixes)

test cred looper shell-fun:
its almost ready for pushing - at this point might be better as a separate PR - currently a few of my over-extensive tests overlap in results so i want to revisit the inputs and hopefully have a few less tests (turned into way more logic than i imagined, including unlocking the mfa constantly lol)
cred combination are as follows, env-vars and provider-config vars for each ( valid and invalid combinations ) :

  • user+pass + (otp)
  • api_token
  • [pre-auth] ticket+csrf

regarding your code wizardry; the class re-work is great - nothing too much to criticise of (esp with me still ramping up to golang).
i dont think the OTP form field coupled with username and password results in a valid auth for proxmox 8 ? i wouldnt mind doing the busy work there and removing it in another PR if its helpful at all

and lastly, just ran a test using the repo's makefile example flags (what i hope to cleanup more and commit) and with an over-complicated check with manually tweaked .terraform/providers and .terraform.locl.hcl files and all seems well (off latest main pull+rebase)

thanks again!

@bpg
Copy link
Owner

bpg commented Sep 30, 2024

Hey @vanillaSprinkles 👋🏼
Thanks for testing! I don't mind if you rewrite the git history on the PR, please rebase whatever way works better for you. At the end, all commits will be squashed at the PR merge, so it doesn't really matter.
Let's deal with OTP in a separate PR though.

vanillaSprinkles and others added 5 commits October 1, 2024 23:15
adds provider config inputs:
  - env vars: PROXMOX_VE_AUTH_PAYLOAD; PROXMOX_VE_AUTH_TICKET with PROXMOX_VE_CSRF_PREVENTION_TOKEN
  - provider-config: auth_payload; auth_ticket with csrf_prevention_token

Signed-off-by: vanillaSprinkles <vanillaSprinkles@users.noreply.github.com>
…add flags to terraform-plugin-docs

Signed-off-by: vanillaSprinkles <vanillaSprinkles@users.noreply.github.com>
…ex.md

Signed-off-by: vanillaSprinkles <vanillaSprinkles@users.noreply.github.com>
Signed-off-by: Pavel Boldyrev <627562+bpg@users.noreply.github.com>
Signed-off-by: Pavel Boldyrev <627562+bpg@users.noreply.github.com>
@vanillaSprinkles vanillaSprinkles force-pushed the vs/main/feat_provider_add_support_forpre_auth_session_tokens branch from 28ab486 to e39d152 Compare October 2, 2024 06:18
Signed-off-by: vanillaSprinkles <vanillaSprinkles@users.noreply.github.com>
@vanillaSprinkles vanillaSprinkles force-pushed the vs/main/feat_provider_add_support_forpre_auth_session_tokens branch from d29129f to fbac792 Compare October 2, 2024 07:16
@vanillaSprinkles
Copy link
Contributor Author

vanillaSprinkles commented Oct 2, 2024

@bpg heya! force pushed - not sure about these build steps stating tar is not available (im hoping its a github thing at the moment)

make docs seems to give the builds an issue: terraform not found -> last commit explicitly adds something for the .github action, but wondering if we can pass-in tofu or terraform dymaically via the make (but i wouldnt know how to make it 'dynamic' in the .gitlab area)

i'll place my api-auth-tester shell-script monster in a separate PR and can decide there if its wanted

Copy link
Owner

@bpg bpg left a comment

Choose a reason for hiding this comment

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

Nice work @vanillaSprinkles!

LGTM! 🚀

@bpg bpg merged commit eb2f36b into bpg:main Oct 3, 2024
8 checks passed
@bpg
Copy link
Owner

bpg commented Oct 3, 2024

@all-contributors please add @vanillaSprinkles for code, idea

Copy link
Contributor

@bpg

I've put up a pull request to add @vanillaSprinkles! 🎉

@camaeel
Copy link

camaeel commented Oct 4, 2024

Hi
Can we use this somehow to get credentials from oidc?
Something like what is done by https://github.com/int128/kubelogin

@vanillaSprinkles
Copy link
Contributor Author

hi @camaeel - i imagine it will work (with oidc, idp, ect); if it can be done via the webui then it should be curl'able and also TF'able - i dont have that stuff all wired up locally to get a full api-flow yet [ i plan to but it is low on my list at the moment ]

the api-flow for proxmox user+pass+totp maybe very similar, might be worth seeing how it works with your setup - highlighted in the docs index.md at https://github.com/bpg/terraform-provider-proxmox/blob/main/docs/index.md#pre-authentication-or-passing-an-authentication-ticket-into-the-provider

@vanillaSprinkles vanillaSprinkles deleted the vs/main/feat_provider_add_support_forpre_auth_session_tokens branch October 5, 2024 19:34
@camaeel
Copy link

camaeel commented Oct 19, 2024

@vanillaSprinkles @bpg I implemented a little helper app that uses OIDC to obtain credentials and expose them as export commands to execute in the shell (or alternatively json output).
Here is my repo: https://github.com/camaeel/proxmox-oidc-credential-helper

Maybe it can be better integrated with this terraform provider. Currently I managed to achieve seamless integration with terragrunt.

@vanillaSprinkles
Copy link
Contributor Author

@camaeel thats pretty nice indeed - thanks !
we should probably add a note to the readme at least to that; only suggestion i have is to have a flag/env-var to pass in the browser to open (if it does not exist and i missed it)

it maybe a bit hard to integrate with the provider directly (esp with my skillset at the moment) - would be bpg doing his wizardry...
but personally i like to have the credential management outside of the provider itself (when MFA/extra-steps are involved other than user+pass) - just due level of involvment credential management involves. it allows another app to interface with security components (os level of keeping a credential to re-use, time to live on a credential, ect); but if it gets incorpated to the provider direclty at some point, more power! (but also more complexity)

(my own setup for testing out things has been slow going but cant wait to try out the idp components, so will use that at some point down the road)

@camaeel
Copy link

camaeel commented Oct 20, 2024

Every time the OS default browser will be open. I could expose a flag to output URL to be opened on the terminal, in cases where default local browser is not convenient.

IMHO it might not work well if it is directly integrated with provider. But it can be called by the provider and the result can be consumed by the provider to populate credentials fields. I'm thinking about something like exec plugins in kubernetes provider: https://registry.terraform.io/providers/hashicorp/kubernetes/latest/docs#exec-plugins (they are also supported by kubectl).

EDIT: I just released v0.2.0 with support for outputting URL to STDOUT instead of opening default browser

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants