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

Clean up Proxmox API token handling by stripping whitespace and forma… #9228

Merged
merged 5 commits into from
Dec 20, 2024

Conversation

xilmen
Copy link
Contributor

@xilmen xilmen commented Dec 8, 2024

SUMMARY

Fix header issue for proxmox dynamic inventory with vault

#9227

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

proxmox inventory

ADDITIONAL INFORMATION

#9227

@ansibullbot
Copy link
Collaborator

@ansibullbot ansibullbot added bug This issue/PR relates to a bug has_issue inventory inventory plugin plugins plugin (any type) labels Dec 8, 2024
@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-9 Automatically create a backport for the stable-9 branch backport-10 Automatically create a backport for the stable-10 branch labels Dec 8, 2024
Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!

plugins/inventory/proxmox.py Outdated Show resolved Hide resolved
plugins/inventory/proxmox.py Outdated Show resolved Hide resolved
changelogs/fragments/9228-fix-issue-header.yml Outdated Show resolved Hide resolved
# Clean and format token components
user = self.proxmox_user.strip()
token_id = self.proxmox_token_id.strip()
token_secret = self.proxmox_token_secret.strip()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm wondering whether this is a good idea. Is token_secret a user-selected string that can potentially have valid whitespace in it, that would get incorrectly removed by this change? (Most likely spaces, and not newlines.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Normally, pve tokens should never have spaces. If this is the case the token will be invalid in all cases.
https://pve.proxmox.com/wiki/Proxmox_VE_API#Example:_Use_API_Token
Exemple with token format

curl -H "Authorization: PVEAPIToken=root@pam!monitoring=aaaaaaaaa-bbb-cccc-dddd-ef0123456789" https://10.0.0.1:8006/api2/json/

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, what about token IDs and usernames? Can they have spaces?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i try to create local user with space. It's not possible.

userid: invalid format - value 'user name@pam' does not look like a valid user name

(i cant try with other type (LDAP/AD/OpenID))

we can remove strip to self.proxmox_user if you prefer.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you also test token ID? If that doesn't work either, everything should be fine.

Copy link
Contributor Author

@xilmen xilmen Dec 9, 2024

Choose a reason for hiding this comment

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

I try for token ID space is not possible :

Method 'POST /access/users/ansible@pam/token/user space' not implemented (501)

changelogs/fragments/9228-fix-issue-header.yml Outdated Show resolved Hide resolved
@felixfontein felixfontein removed the backport-9 Automatically create a backport for the stable-9 branch label Dec 8, 2024
xilmen and others added 4 commits December 8, 2024 18:06
Co-authored-by: Felix Fontein <felix@fontein.de>
Co-authored-by: Felix Fontein <felix@fontein.de>
Co-authored-by: Felix Fontein <felix@fontein.de>
Co-authored-by: Felix Fontein <felix@fontein.de>
@felixfontein
Copy link
Collaborator

If nobody objects, I'll merge this at the end of next week.

Copy link
Collaborator

@russoz russoz left a comment

Choose a reason for hiding this comment

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

LGTM

@felixfontein felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Dec 20, 2024
@felixfontein felixfontein merged commit c5855d1 into ansible-collections:main Dec 20, 2024
129 checks passed
Copy link

patchback bot commented Dec 20, 2024

Backport to stable-10: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-10/c5855d1a58d91994105d7ea07e23fa8554c7fa38/pr-9228

Backported as #9288

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Dec 20, 2024
#9228)

* Clean up Proxmox API token handling by stripping whitespace and formatting the token string

* Update plugins/inventory/proxmox.py

Co-authored-by: Felix Fontein <felix@fontein.de>

* Update plugins/inventory/proxmox.py

Co-authored-by: Felix Fontein <felix@fontein.de>

* Update changelogs/fragments/9228-fix-issue-header.yml

Co-authored-by: Felix Fontein <felix@fontein.de>

* Update changelogs/fragments/9228-fix-issue-header.yml

Co-authored-by: Felix Fontein <felix@fontein.de>

---------

Co-authored-by: Felix Fontein <felix@fontein.de>
(cherry picked from commit c5855d1)
@felixfontein
Copy link
Collaborator

@xilmen thanks for your contribution!
@russoz thanks for reviewing!

felixfontein pushed a commit that referenced this pull request Dec 20, 2024
…ndling by stripping whitespace and forma… (#9288)

Clean up Proxmox API token handling by stripping whitespace and forma… (#9228)

* Clean up Proxmox API token handling by stripping whitespace and formatting the token string

* Update plugins/inventory/proxmox.py

Co-authored-by: Felix Fontein <felix@fontein.de>

* Update plugins/inventory/proxmox.py

Co-authored-by: Felix Fontein <felix@fontein.de>

* Update changelogs/fragments/9228-fix-issue-header.yml

Co-authored-by: Felix Fontein <felix@fontein.de>

* Update changelogs/fragments/9228-fix-issue-header.yml

Co-authored-by: Felix Fontein <felix@fontein.de>

---------

Co-authored-by: Felix Fontein <felix@fontein.de>
(cherry picked from commit c5855d1)

Co-authored-by: xilmen <romain.scha@gmail.com>
erichoog pushed a commit to erichoog/community.general that referenced this pull request Dec 23, 2024
ansible-collections#9228)

* Clean up Proxmox API token handling by stripping whitespace and formatting the token string

* Update plugins/inventory/proxmox.py

Co-authored-by: Felix Fontein <felix@fontein.de>

* Update plugins/inventory/proxmox.py

Co-authored-by: Felix Fontein <felix@fontein.de>

* Update changelogs/fragments/9228-fix-issue-header.yml

Co-authored-by: Felix Fontein <felix@fontein.de>

* Update changelogs/fragments/9228-fix-issue-header.yml

Co-authored-by: Felix Fontein <felix@fontein.de>

---------

Co-authored-by: Felix Fontein <felix@fontein.de>
@xilmen xilmen deleted the fix_issue_9227 branch January 1, 2025 19:49
Massl123 pushed a commit to Massl123/community.general that referenced this pull request Feb 7, 2025
ansible-collections#9228)

* Clean up Proxmox API token handling by stripping whitespace and formatting the token string

* Update plugins/inventory/proxmox.py

Co-authored-by: Felix Fontein <felix@fontein.de>

* Update plugins/inventory/proxmox.py

Co-authored-by: Felix Fontein <felix@fontein.de>

* Update changelogs/fragments/9228-fix-issue-header.yml

Co-authored-by: Felix Fontein <felix@fontein.de>

* Update changelogs/fragments/9228-fix-issue-header.yml

Co-authored-by: Felix Fontein <felix@fontein.de>

---------

Co-authored-by: Felix Fontein <felix@fontein.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-10 Automatically create a backport for the stable-10 branch bug This issue/PR relates to a bug has_issue inventory inventory plugin plugins plugin (any type)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants