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

Fix #59 add encryption key config option #60

Merged
merged 7 commits into from
Nov 28, 2022
Merged

Fix #59 add encryption key config option #60

merged 7 commits into from
Nov 28, 2022

Conversation

fangpenlin
Copy link
Contributor

@fangpenlin fangpenlin commented Nov 6, 2022 •
edited

PR checklist

  • An issue/feature request has been created for this PR
  • Pull Request title clearly describes the work in the pull request and the Pull Request description provides details about how to validate the work. Missing information here may result in a delayed response.
  • File the PR against the main branch
  • The code in this PR is covered by unit tests

Link to issue/feature request: #59

Description

Provide alternative option to pass in PublicKey instead of Certificate for request payload encryption.

@danny-gallagher
Copy link
Contributor

Thanks @fangpenlin for creating the PR!

After some testing it looks like the encryption key fingerprint is no longer generated when the public encryption key is used.
We'll need to update the computeEncryptionKeyFingerprintWhenNeeded function to account for the new key type.

@fangpenlin fangpenlin force-pushed the add-encryption-key-config-option branch from 6f97c65 to 0f0e0a3 Compare November 22, 2022 10:04
@fangpenlin
Copy link
Contributor Author

Hi @danny-gallagher, thank you so much for pointing out the issue.

I have addressed it and also added test cases for that. Could you please review it again? Thanks 🙏

@danny-gallagher danny-gallagher added the safe The PR has been reviewed by Mastercard and has been validated to be safe label Nov 28, 2022
danny-gallagher
danny-gallagher approved these changes Nov 28, 2022
View reviewed changes
Copy link
Contributor

@danny-gallagher danny-gallagher left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@danny-gallagher danny-gallagher merged commit 8fb481a into Mastercard:main Nov 28, 2022
@fangpenlin fangpenlin deleted the add-encryption-key-config-option branch November 28, 2022 15:43
@jcweber10 jcweber10 mentioned this pull request Oct 6, 2023
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danny-gallagher danny-gallagher danny-gallagher approved these changes

Assignees
No one assigned
Labels
safe The PR has been reviewed by Mastercard and has been validated to be safe
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@fangpenlin @danny-gallagher