Skip to content

Allow using API tokens for Proxmox authentication#10797

Merged
sylviamoss merged 8 commits intohashicorp:masterfrom
mraerino:feat/proxmox-token-auth
Mar 22, 2021
Merged

Allow using API tokens for Proxmox authentication#10797
sylviamoss merged 8 commits intohashicorp:masterfrom
mraerino:feat/proxmox-token-auth

Conversation

@mraerino
Copy link
Copy Markdown
Contributor

@mraerino mraerino commented Mar 20, 2021

Proxmox allows people to create API tokens with limited privileges that can be rotated without deleting a user account. It is usually useful for giving automated systems specific access to parts of the Proxmox API, e.g. a single VM resource pool.

API tokens allow stateless access to most parts of the REST API by another system, software or API client. Tokens can be generated for individual users and can be given separate permissions and expiration dates to limit the scope and duration of the access. Should the API token get compromised it can be revoked without disabling the user itself.
https://pve.proxmox.com/wiki/User_Management#pveum_tokens

The current authentication flow used for the Proxmox client is not compatible though:

POST /api2/json/access/ticket
This API endpoint is not available for API tokens.
https://pve.proxmox.com/pve-docs/api-viewer/#/access/ticket (POST tab)

Instead the required auth flow for API tokens is described here:

To use an API token, set the HTTP header Authorization to the displayed value of the form PVEAPIToken=USER@REALM!TOKENID=UUID when making API requests
https://pve.proxmox.com/wiki/User_Management#pveum_tokens


This PR introduces a new parameter token for the Proxmox builder which allows using the alternative API token flow.
It will take precedence over password if both are given.

Tests are provided, docs have been updated.
I tested a development build with a proxmox setup locally.

@mraerino mraerino requested review from a team and carlpett as code owners March 20, 2021 03:56
@hashicorp-cla
Copy link
Copy Markdown

hashicorp-cla commented Mar 20, 2021

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 20, 2021

Codecov Report

Merging #10797 (331e603) into master (667f930) will increase coverage by 0.01%.
The diff coverage is 74.07%.

Impacted Files Coverage Δ
builder/proxmox/common/builder.go 0.00% <0.00%> (ø)
builder/proxmox/common/client.go 71.42% <71.42%> (ø)
builder/proxmox/common/config.go 46.00% <100.00%> (+0.82%) ⬆️

@mraerino
Copy link
Copy Markdown
Contributor Author

Upstream PR is open, let's see which one wins: Telmate/proxmox-api-go#108

@mraerino
Copy link
Copy Markdown
Contributor Author

now using the method from the client lib

Copy link
Copy Markdown
Contributor

@carlpett carlpett left a comment

Choose a reason for hiding this comment

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

Thanks @mraerino! This all looks really good (getting the token auth into the upstream library was the best way to go IMO), just a linter failure to take care of

Comment thread builder/proxmox/common/client.go Outdated
Comment on lines +15 to +29
type authenticatedTransport struct {
rt http.RoundTripper
user string
token string
}

func newAuthenticatedTransport(rt http.RoundTripper, user, token string) *authenticatedTransport {
return &authenticatedTransport{rt, user, token}
}

func (t *authenticatedTransport) RoundTrip(req *http.Request) (*http.Response, error) {
auth := fmt.Sprintf("PVEAPIToken=%s=%s", t.user, t.token)
req.Header.Set("Authorization", auth)
return t.rt.RoundTrip(req)
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There's a linter failure because these are unused. I suppose they are from before moving the bits upstream?

Copy link
Copy Markdown
Contributor Author

@mraerino mraerino Mar 22, 2021

Choose a reason for hiding this comment

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

removed (2162e7d)

Comment thread builder/proxmox/common/client.go Outdated
return t.rt.RoundTrip(req)
}

func NewProxmoxClient(config Config) (*proxmox.Client, error) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: This strictly doesn't need to be exported, right?

Copy link
Copy Markdown
Contributor Author

@mraerino mraerino Mar 22, 2021

Choose a reason for hiding this comment

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

fixed (331e603)

Copy link
Copy Markdown
Contributor

@sylviamoss sylviamoss left a comment

Choose a reason for hiding this comment

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

Nice! I'll merge it once @carlpett's comments are addressed.

@mraerino mraerino requested a review from carlpett March 22, 2021 10:00
@sylviamoss sylviamoss merged commit 4d9fb62 into hashicorp:master Mar 22, 2021
@mraerino mraerino deleted the feat/proxmox-token-auth branch March 22, 2021 12:56
@mraerino
Copy link
Copy Markdown
Contributor Author

is there something that will let me know once this lands in a stable release?

@nywilken
Copy link
Copy Markdown
Contributor

Hey 👋 @mraerino, there is no automatic notification. This feature will be available for use in our next nightly release which is a glimpse of what you can expect in the next v1.7.1 release which is set to go out next week.

As far as notifications go we announce Packer releases to the Packer email distribution list. You can also subscribe to the GitHub RSS feeds so that you can see when new releases drop https://github.com/hashicorp/packer/releases.atom

Thanks again for making this change.

@ghost
Copy link
Copy Markdown

ghost commented Apr 22, 2021

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked as resolved and limited conversation to collaborators Apr 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants