-
Notifications
You must be signed in to change notification settings - Fork 2.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
Cybersource SOAP: Update authentication using p12 keys #5415
base: master
Are you sure you want to change the base?
Conversation
7756c0d
to
40f1900
Compare
40f1900
to
8659932
Compare
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.
Comment, Nice job @sinourain this was a hard one, left some comments for your consideration.
@@ -43,6 +45,18 @@ class CyberSourceGateway < Gateway | |||
} | |||
DEFAULT_COLLECTION_INDICATOR = 2 | |||
|
|||
NS_DS = 'http://www.w3.org/2000/09/xmldsig#' |
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.
💬 Comment:
The first thing to say is that maybe all these signature code perhaps should live in a separate module, There is too much code to throw directly into the main adapter.
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.
That's what I thought, but then I saw the cybersource rest code, which by the way is similar (made by you), so I decided to leave it here, in the end it was much simpler.
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.
Mmmm yes and no @sinourain , Yes CybersourceRest has the signature there, but is a ~15 lines method and that's it, here we have a ton of functionality not directly related to the main purpose of the adapter.
Is a judgment call at the end, but it looks bloated to me.
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 agree on this, it would be nice if those lived in another place. But again it's not code breaking and it works
def signature_element(xml, body) | ||
signed_info = sign_info(body) | ||
rsa_private_key = OpenSSL::PKey::RSA.new(@options[:private_key]) | ||
signature = rsa_private_key.sign(OpenSSL::Digest.new('SHA256'), canonicalization(signed_info)) |
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.
💬 Comment:
if you do the canonicalization first then you could just past that value to the sing_info
method and with that just call the canonicalization
method only once.
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.
Done!
end | ||
|
||
# Where we actually build the full SOAP request using builder | ||
def build_request(request, options) |
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.
💬 Comment - ❓ Question:
Are you sure is a good idea to rewrite the entire build_request
method ?, is going to be end-up being used for the two authentication methods.
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 need the information from the request and in order not to repeat requesting this twice I decided to modify it just a little, I tested it with both forms of authentication and it works well.
def build_request(request, options) | ||
xsd_version = test? ? TEST_XSD_VERSION : PRODUCTION_XSD_VERSION | ||
|
||
body = set_body(request, options, xsd_version) |
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.
💬 Comment:
set_body
isn't actually "setting" body on anything, the method is more "building" a body than "setting" a body.
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.
💬 Style / Suggestion:
What about re-organizing the code so it has a more logical order like this:
def build_request(request, options)
xsd_version = test? ? TEST_XSD_VERSION : PRODUCTION_XSD_VERSION
xml = Builder::XmlMarkup.new indent: 2
xml.instruct!
xml.tag! 's:Envelope', { 'xmlns:s' => ENVELOPE, 'xmlns' => "urn:schemas-cybersource-com:transaction-data-#{xsd_version}" } do
set_headers(xml, body)
xml << build_body(request, options, xsd_version)
end
xml.target!
end
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.
If I remove the body assignation I get an error in the set_headers call, so I prefer the current code
end.check_request do |_endpoint, data, _headers| | ||
assert_match(/<ccAuthService run="true"\/>/, data) | ||
end.respond_with(successful_authorization_certificate_response) | ||
end |
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.
💬 Comment:
This test isn't asserting any behavior related to your change, it is just asserting a tag that was already there
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.
Done!
end.respond_with(successful_authorization_certificate_response) | ||
end | ||
|
||
def test_successful_certificate_purchase |
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.
💬 Comment;
Same case here and the next test
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.
Done!
end.respond_with(successful_verify_certificate_response) | ||
assert_success response | ||
end | ||
|
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.
💬 Comment:
There is a lack of coverage for all the code written to generate the signature sign_info
, signature_element
, set_headers
, set_body
and canonicalization
doesn't have any related tests.
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.
Done!
add6527
to
177b892
Compare
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.
Make sure you're scrubbing public_key and private_key in the scrub method
177b892
to
6db85a9
Compare
I added the |
|
||
xml.tag! 'ds:Signature', { 'xmlns:ds' => NS_DS } do | ||
xml << signed_info | ||
xml.tag! 'ds:SignatureValue', { 'xmlns:ds' => NS_DS } do |
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.
Is it possible for someone to extract the private key from here?
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.
No, it is not possible to extract the private key from the signature. The private key is used to generate the signature, but the signature itself does not contain the private key. The signature is a cryptographic hash that can be verified using the corresponding public key, but it cannot be reversed to obtain the private key. However, it is possible to add the digest value and signature to the scrub method to prevent any information in the transcript from being exposed.
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.
If it won't expose any sensitive info I think it's fine. I will approve but can you just get double confirmation from Cybersource on this?
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.
Sure! While we wait for confirmation from Cybersource, I added the digest value and the signature to the scrub method.
requires!(options, :login, :password) | ||
if options[:public_key] | ||
options[:merchant_id] ||= options[:login] | ||
requires!(options, :merchant_id, :public_key, :private_key) |
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.
Is there a reason you decided to change it from login to merchant_id?
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.
Only that merchant_id is more descriptive to the value it corresponds to, however, I'm going to remove it to only have login
51f4bca
to
95fa6fe
Compare
…public_key and private_key from the p12 file provided/generated in the Cybersource dashboard. fixtures.yml ``` cyber_source_cypher: merchant_id: merchant_id, public_key: SOMECREDENTIAL, private_key: SOMECREDENTIAL ``` (OPPS-348)[https://spreedly.atlassian.net/browse/OPPS-348] Finished in 33.396675 seconds. 6218 tests, 81341 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed 186.19 tests/s, 2435.60 assertions/s Finished in 176.54427 seconds. 149 tests, 762 assertions, 3 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 97.973% passed 0.84 tests/s, 4.32 assertions/s 808 files inspected, no offenses detected
95fa6fe
to
95006fb
Compare
What is the regression risk of such a large refactor , is it going to be feature flagged before landing? |
Yes, these changes are for when the merchant adds the certificates (private and public keys), if they do not add them, they would use the current code (login/password) and it will continue to work the same as now. |
Summary
This changes include the Cybersource p12 authentication. Extract the public_key and private_key from the p12 file provided/generated in the Cybersource dashboard.
fixtures.yml
Spreedly reference
OPPS-348
Tests
Unit Tests
Finished in 33.396675 seconds.
6218 tests, 81341 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed
186.19 tests/s, 2435.60 assertions/s
Remote Tests
Finished in 176.54427 seconds.
148 tests, 762 assertions, 3 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
97.973% passed
0.84 tests/s, 4.32 assertions/s
Rubocop
808 files inspected, no offenses detected