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 gosaml2 so it supports AES256GCM #11272

Merged
merged 4 commits into from
Mar 21, 2022
Merged

Conversation

probakowski
Copy link
Contributor

@probakowski probakowski commented Mar 18, 2022

This change updates gosaml2 to our fork with added support for AES256GCM and AES192GCM for encrypted assertions.
I also run go mod tidy to cleanup go.mod file

Closes #10909

Note to reviewers: I'd ask for review of 2 things, this change and gravitational/gosaml2#4.
I moved master in fork to the commit we used before in go.mod (gravitational/gosaml2@757d23f) and started from there, you can check history at https://github.com/gravitational/gosaml2/commits/master

@github-actions github-actions bot requested review from smallinsky and Tener March 18, 2022 22:53
@Tener
Copy link
Contributor

Tener commented Mar 21, 2022

The change looks fine on its own, but I have questions:

  • why do we need support for AES256GCM and AES192GCM? This PR "closes Stale DynamoDB Auth Service Items #10019", but it is already closed and doesn't mention either of the two...?
  • our fork of gosaml2 is behind several commits from the original. perhaps we want to move it to the tip of the main branch?

@zmb3
Copy link
Collaborator

zmb3 commented Mar 21, 2022

The change looks fine on its own, but I have questions:

  • why do we need support for AES256GCM and AES192GCM? This PR "closes Stale DynamoDB Auth Service Items #10019", but it is already closed and doesn't mention either of the two...?
  • our fork of gosaml2 is behind several commits from the original. perhaps we want to move it to the tip of the main branch?

@Tener that was a typo. This closes #10909

Copy link
Contributor

@Tener Tener left a comment

Choose a reason for hiding this comment

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

Thanks to @zmb3 for clarification regarding the original issue.

Nit: as mentioned, perhaps worth moving our fork to include latest additions to https://github.com/russellhaering/gosaml2.

Other than that, LGTM.

Copy link
Collaborator

@zmb3 zmb3 left a comment

Choose a reason for hiding this comment

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

LGTM.

If we want to update gosaml2 to the latest upstream commit, let's do that as a separate PR in case something breaks and we need to roll it back.

@probakowski
Copy link
Contributor Author

@Tener @zmb3 thanks for review!
I'll merge it as is and test updating fork to upstream in separate PR.

@probakowski probakowski enabled auto-merge (squash) March 21, 2022 16:35
@probakowski probakowski merged commit 01b8aeb into master Mar 21, 2022
@probakowski probakowski deleted the probakowski/update-gosaml2 branch March 21, 2022 20:18
probakowski added a commit that referenced this pull request Mar 21, 2022
#11272 removed some required dependencies, this change brings them back
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.

Unable to decrypt SAML assertion
3 participants