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.

Sign up for GitHub

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

Jump to bottom

ansible-galaxy collection - validate and refresh KeycloakToken #79145

Closed
wants to merge 4 commits into from
Closed

ansible-galaxy collection - validate and refresh KeycloakToken #79145

wants to merge 4 commits into from

Conversation

s-hertel
Copy link
Contributor

SUMMARY

This should fix refreshing KeycloakTokens while waiting for import results. I'm not sure how to test it though...

Fixes #70019

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

ansible-galaxy collection publish

@ansibot ansibot added affects_2.15 bug This issue/PR relates to a bug. needs_triage Needs a first human triage before being processed. labels Oct 14, 2022
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Oct 22, 2022
@s-hertel s-hertel removed the needs_triage Needs a first human triage before being processed. label Oct 25, 2022
s-hertel
s-hertel commented Oct 25, 2022
View reviewed changes
lib/ansible/galaxy/api.py Outdated Show resolved Hide resolved
@s-hertel s-hertel marked this pull request as draft October 25, 2022 15:04
jborean93
jborean93 approved these changes Oct 25, 2022
View reviewed changes
Copy link
Contributor

@jborean93 jborean93 left a comment

Choose a reason for hiding this comment

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

+1 to the warning, the rest looks good.

@ansibot ansibot added the WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. label Oct 25, 2022
webknjaz
webknjaz reviewed Oct 25, 2022
View reviewed changes
lib/ansible/galaxy/api.py Outdated Show resolved Hide resolved
webknjaz
webknjaz reviewed Oct 25, 2022
View reviewed changes
lib/ansible/galaxy/api.py Outdated Show resolved Hide resolved
webknjaz
webknjaz reviewed Oct 25, 2022
View reviewed changes
lib/ansible/galaxy/api.py Outdated Show resolved Hide resolved
@ansibot ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Nov 2, 2022
@s-hertel s-hertel force-pushed the a-g-fix-refreshing-token branch 2 times, most recently from 6973c5b to 647efb3 Compare November 2, 2022 20:35
s-hertel
s-hertel commented Nov 2, 2022
View reviewed changes
lib/ansible/galaxy/token.py Outdated Show resolved Hide resolved
webknjaz
webknjaz reviewed Nov 3, 2022
View reviewed changes
lib/ansible/galaxy/api.py Outdated Show resolved Hide resolved
webknjaz
webknjaz reviewed Nov 3, 2022
View reviewed changes
lib/ansible/galaxy/api.py Outdated Show resolved Hide resolved
webknjaz
webknjaz reviewed Nov 3, 2022
View reviewed changes
lib/ansible/galaxy/api.py Outdated Show resolved Hide resolved
webknjaz
webknjaz reviewed Nov 3, 2022
View reviewed changes
lib/ansible/galaxy/api.py Outdated Show resolved Hide resolved
@s-hertel s-hertel marked this pull request as ready for review November 8, 2022 00:02
@ansibot ansibot removed the WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. label Nov 8, 2022
webknjaz
webknjaz approved these changes Nov 8, 2022
View reviewed changes
Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

Approving, but there's room for improvement if you're up to it :)

lib/ansible/galaxy/token.py Outdated Show resolved Hide resolved
lib/ansible/galaxy/token.py Outdated
Comment on lines 100 to 103
data = json.loads(to_text(resp.read(), errors='surrogate_or_strict'))
token_data = json.loads(
base64.b64decode(data['id_token'].split('.')[1] + "==")
)
Copy link
Member

Choose a reason for hiding this comment

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

This incantation looks out of place. It leaks into a low-level abstraction of decoding and parsing the internals.

I'd move at least the last instruction into a pure function so it'd look something like

bearer_token_struct = parse_bearer_token(resp.read())

It could return a data structure with both the access token and the id token, if it replaces both instructions.

I don't have visibility into what the HTTP response structure it, but often this data also includes some checksum/signature. Such a function could also verify the integrity and raise an error if it's corrupted somehow.

Maybe it could also normalize the time fields into datetime objects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestions, thanks. I implemented signature checking. I'm not sure I can assume the JSON web key sets url is relative to the auth URL, so I added a new server config option to achieve that in case the auth_url does not match AH - maybe not backportable now, but it's better I think.

I also fixed normalizing the datetime fields in parse_bearer_token.

Need to figure out how to get sanity tests passing, but I think this is close to done now.

@ansibot ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Feb 23, 2023
@s-hertel s-hertel force-pushed the a-g-fix-refreshing-token branch 2 times, most recently from b53e503 to 2033f3d Compare February 23, 2023 15:43
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Feb 23, 2023
webknjaz
webknjaz reviewed Feb 23, 2023
View reviewed changes
Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

Got a few more comments...

lib/ansible/galaxy/token.py Outdated Show resolved Hide resolved
lib/ansible/galaxy/token.py Outdated Show resolved Hide resolved
test/units/galaxy/test_token.py Outdated Show resolved Hide resolved
test/units/galaxy/test_token.py Outdated Show resolved Hide resolved
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Mar 3, 2023
@ansibot ansibot added docs This issue/PR relates to or includes documentation. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Apr 17, 2023
@s-hertel s-hertel changed the title ansible-galaxy collection publish - fix expired token when waiting for import ansible-galaxy collection - validate and refresh KeycloakToken Apr 17, 2023
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Apr 25, 2023
@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels May 3, 2023
@ansibot ansibot added stale_review Updates were made after the last review and the last review is more than 7 days old. has_issue and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jul 12, 2023
s-hertel and others added 3 commits August 9, 2023 13:35
@s-hertel
Approach 1 (naive)
6d91543 
add expiration property to KeycloakToken

warn when token expires

handle expired token when waiting for imports to complete after publishing to AH

changelog
@s-hertel @webknjaz
Approach 2 (naive, proactive)
82f37f0 
Handle refreshing in KeycloakToken.get() before tokens expire

Make the warning a debug msg since there is no error

Use global constant for early KeycloakToken expiration

Co-authored-by: Sviatoslav Sydorenko <[email protected]>
@s-hertel @webknjaz
Approach 3 (validate according to https://www.rfc-editor.org/rfc/rfc7519
57517b9 
)

Add an openid_configuration galaxy server option. Since we don't want to
hardcode the AH URL, the bugfix is now opt-in only. On the plus side,
now the auth_url can be derived from the OpenID config, so users only
need to configure 1 of these options.

validate bearer token before using expiration data from it

normalize datetime fields before returning token data

add cryptograph to unit test requirements for ansible-galaxy

tweak user_agent string so getting AH OpenID config works (comment order
seems to have requirements?)

Co-authored-by: Sviatoslav Sydorenko <[email protected]>
@ansibot ansibot removed needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Aug 9, 2023
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Aug 23, 2023
@s-hertel s-hertel closed this Sep 13, 2023
@ansible ansible locked and limited conversation to collaborators Oct 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@webknjaz webknjaz webknjaz approved these changes

@jborean93 jborean93 Awaiting requested review from jborean93

Assignees
No one assigned
Labels
affects_2.15 bug This issue/PR relates to a bug. docs This issue/PR relates to or includes documentation. has_issue stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_review Updates were made after the last review and the last review is more than 7 days old.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Long import wait times in ansible-galaxy collection publish break on token expiration
4 participants
@s-hertel @ansibot @webknjaz @jborean93