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

Update sdk and fix login #43

Merged
merged 19 commits into from
Nov 17, 2022
Merged

Update sdk and fix login #43

merged 19 commits into from
Nov 17, 2022

Conversation

fairclothjm
Copy link
Contributor

@fairclothjm fairclothjm commented Nov 11, 2022

Update dependencies and fix login.

Login was likely broken since #37 which added the pathLoginResolveRole method. pathLoginResolveRole gets called before the call to pathLoginUpdate. Since we are capturing the signed GetCallerIdentity request here both pathLoginResolveRole and pathLoginUpdate use the same nonce that was generated once on the vault login call.

Some notes (updated):

  • the role is now a required field by the login path
  • previously the code allowed an empty role and parsed it from the GetCallerIdentity request, but the documentation said it was required
  • we require the role to avoid nonce reuse with the benefit of simplifying pathLoginResolveRole

Copy link
Contributor

@VioletHynes VioletHynes left a comment

Choose a reason for hiding this comment

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

Looks good! I'm sorry if I broke this, but thanks for fixing it.

The approach broadly makes sense to me, but admittedly I don't claim to be an AliCloud expert.

path_login.go Outdated Show resolved Hide resolved
Copy link

@austingebauer austingebauer left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for diving into this one. I have a couple of questions around resolving the role related to the nonce reuse issue and rework of the CLI login params.

path_login.go Outdated Show resolved Hide resolved
cli.go Outdated Show resolved Hide resolved
path_login.go Show resolved Hide resolved
Copy link

@austingebauer austingebauer left a comment

Choose a reason for hiding this comment

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

👍

@fairclothjm fairclothjm merged commit 722c59c into main Nov 17, 2022
@fairclothjm fairclothjm deleted the update-sdk-fix-login branch November 17, 2022 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants