[Feature] Add support for client certificate password and test with real services that require client certificate auth #32

phillee007 · 2017-06-10T09:05:30Z

Add support for client certificate password and test with real services that require client certificates. Also update so when proxying, the path and query will be passed to the proxied api.

Please have a look and let me know what you think. I ran a test for it locally, but it's a bit hard to checkin a runnable unit test as it would require a real service (which I have in my environment) plus a valid pfx with password

…ervices that require client certificates. Also update so when proxying, the path and query will be passed to the proxied api.

codecov · 2017-06-10T10:57:50Z

Codecov Report

Merging #32 into master will decrease coverage by 0.76%.
The diff coverage is 31.57%.

@@            Coverage Diff             @@
##           master      #32      +/-   ##
==========================================
- Coverage    50.4%   49.63%   -0.77%     
==========================================
  Files          58       59       +1     
  Lines        1867     1904      +37     
  Branches      252      258       +6     
==========================================
+ Hits          941      945       +4     
- Misses        858      891      +33     
  Partials       68       68

Impacted Files	Coverage Δ
...rc/WireMock.Net/Settings/ProxyAndRecordSettings.cs	`0% <0%> (ø)`	⬆️
src/WireMock.Net/Admin/Mappings/ResponseModel.cs	`63.63% <0%> (-6.37%)`	⬇️
src/WireMock.Net/Http/CertificateUtil.cs	`0% <0%> (ø)`
src/WireMock.Net/Server/FluentMockServer.Admin.cs	`23.31% <11.11%> (-0.44%)`	⬇️
src/WireMock.Net/Http/HttpClientHelper.cs	`57.77% <63.15%> (-6.33%)`	⬇️
src/WireMock.Net/ResponseBuilders/Response.cs	`75.49% <71.42%> (+0.74%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c29b88e...af2a035. Read the comment docs.

StefH

I've added some remarks and questions.

StefH · 2017-06-11T14:52:35Z

src/WireMock.Net/ResponseBuilders/Response.cs

+ /// <returns>A <see cref="IResponseBuilder"/>.</returns>
+ public IResponseBuilder WithProxy(string proxyUrl, string clientX509Certificate2Filename, string clientX509Certificate2Password)
+ {
+ Check.NotEmpty(clientX509Certificate2Filename, nameof(clientX509Certificate2Password));


Fix this Check --> you check clientX509Certificate2Filename but use nameof(clientX509Certificate2Password)

And add a second check.

StefH · 2017-06-11T14:52:44Z

src/WireMock.Net/ResponseBuilders/Response.cs

+ /// </summary>
+ /// <param name="proxyUrl">The proxy url.</param>
+ /// <param name="clientX509Certificate2Filename">The X509Certificate2 file to use for client authentication.</param>
+ /// /// <param name="clientX509Certificate2Password">The X509Certificate2 password.</param>


StefH · 2017-06-11T14:54:11Z

src/WireMock.Net/ResponseBuilders/Response.cs

@@ -285,7 +305,10 @@ public IResponseBuilder WithProxy(string proxyUrl, string clientX509Certificate2

 if (ProxyUrl != null)
 {
- return await HttpClientHelper.SendAsync(requestMessage, ProxyUrl, X509Certificate2Filename);
+ var requestUri = new Uri(requestMessage.Url);


Why is this needed?

Why not just add the new parameter (X509Certificate2Password) to the method?

StefH · 2017-06-11T14:54:38Z

src/WireMock.Net/Server/FluentMockServer.Admin.cs

@@ -129,7 +129,11 @@ private void InitProxyAndRecord(ProxyAndRecordSettings settings)

 private async Task<ResponseMessage> ProxyAndRecordAsync(RequestMessage requestMessage, ProxyAndRecordSettings settings)
 {
- var responseMessage = await HttpClientHelper.SendAsync(requestMessage, settings.Url);
+ var requestUri = new Uri(requestMessage.Url);


Same as above, please explain why this needed.

StefH · 2017-06-11T14:55:12Z

src/WireMock.Net/Admin/Mappings/ResponseModel.cs

+ /// <summary>
+ /// The X509Certificate2 password.
+ /// </summary>
+ public string X509Certificate2Password { get; set; }


Is it safe to return the password?

StefH · 2017-06-11T14:57:06Z

I've added some remarks and questions. Can you please take a look?

phillee007 · 2017-06-16T05:31:54Z

Hi Stef,

Thanks for all your comments, they're very helpful and I see I missed a few things (inc. updating all tests!)
With regard to client certificates and passwords, the two main options I've seen in the past for reading a client cert to send with a request are:

Read it from a pfx file, and have to specify the password (as generally .pfx are stored with a password, especially if they're production certs we want to use when proxying a live service)
Pros:

Can read from a NAS share or somewhere else, so tests can be run on any machine and read from a common location without having to do any importing of certs into their store
Cons:
Have to send password for reading cert, and store in each request stub mapping and proxyandrecordsettings

Read it from the certificate store on the local machine
Pros:

No need to store passwords in tests or test configuration
Cons:
Have to configure machines on which tests will be run so that certificate is in the store

In this PR, I've done the code for option (1). However, I could update it to do option (2) instead if you think that's preferable? Thoughts?

Thanks,
Phil

StefH · 2017-06-16T07:02:29Z

I'll do the review, thanks for taking this up. And provide feedback.
And I will have to read some more on certificates, to see what is possible.
You said you have implemented option 1, is is also possible to ALSO build option 2?

…ervices that require client certificates. Also update so when proxying, the path and query will be passed to the proxied api.

StefH

Some small comments.

StefH · 2017-06-16T08:28:34Z

src/WireMock.Net/Http/CertificateUtil.cs

+ if (matchingCertificates.Count == 0)
+ {
+ // No certificates matched the search criteria.
+ throw new Exception("no cert fount");


throw new Exception("No certificate found with Thumbprint or SubjectName '{0}'", thumbprintOrSubjectName);

StefH · 2017-06-16T08:28:46Z

src/WireMock.Net/Http/CertificateUtil.cs

+ // Use the first matching certificate.
+ return matchingCertificates[0];
+ }
+


remove empty line

StefH · 2017-06-16T08:30:39Z

src/WireMock.Net/ResponseBuilders/Response.cs

 /// <returns>A <see cref="IResponseBuilder"/>.</returns>
- public IResponseBuilder WithProxy(string proxyUrl, string clientX509Certificate2Filename)
+ public IResponseBuilder WithProxy(string proxyUrl, string clientX509Certificate2ThumbprintOrSubjectName)


You could add:
[NotNull] string proxyUrl
and
[NotNull] clientX509Certificate2ThumbprintOrSubjectName

StefH · 2017-06-16T08:31:36Z

I've added some small comments, can you take a look?

…ervices that require client certificates. Also update so when proxying, the path and query will be passed to the proxied api.

phillee007 · 2017-06-16T09:08:51Z

Yep all good. Having a bit of a nightmare with rebasing and trying to revert some changes... I don't think it's a good idea to include both options for file AND store - at least not in the use cases I'm thinking of. Might not be able to resume this for a few days, but will do my best. Phil

…

On Fri, Jun 16, 2017 at 8:31 PM, Stef Heyenrath ***@***.***> wrote: I've added some small comments, can you take a look? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#32 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEEFo6OaMjf29M_Xsn5Pd6jf7Ybia1M8ks5sEj1ogaJpZM4N2DvN> .

StefH · 2017-06-16T09:21:08Z

Yes rebasing and merging can be difficult to understand sometimes.

Maybe I can create an organization and add you as a member.

If we then work only from separate branches and review the code and only merge to master, that could also work...

phillee007 · 2017-06-16T09:24:00Z

If that works better for you I'm happy to do that. I could then create a feature branch and reapply my changes to it instead of the fork. Sorry I haven't worked with github forking and pull-requests a great deal so am taking a while to get the hang of it...

…

On Fri, Jun 16, 2017 at 9:21 PM, Stef Heyenrath ***@***.***> wrote: Yes rebasing and merging can be difficult to understand sometimes. Maybe I can create an organization and add you as a member. If we then work only from separate branches and review the code and only merge to master, that could also work... — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#32 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEEFo7nZpSK5Jp5satsaHQ5s06K76q_Wks5sEkkFgaJpZM4N2DvN> .

phillee007 · 2017-06-16T09:39:42Z

Cool. I think some of the code in there is dev stuff that needs removing. eg in the console app for the recorder example. Will check next time & do another pr to fix

…

On 16/06/2017 9:36 PM, "Stef Heyenrath" ***@***.***> wrote: Merged #32 <#32>. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#32 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEEFowAsFCRLjfoIX1cNgGt7m9yPq-ASks5sEkyKgaJpZM4N2DvN> .

StefH · 2017-06-16T09:46:27Z

I've just created an Github organization, and I will move the code from here to that organization.
After this is done, I will invite you.

StefH · 2017-06-17T07:36:10Z

I sent invite to you for joining the organization.

Can you also tell me your full name ? So I can include this in the csproj/nuget.

As discussed ; can you please create a separate branch for new functionality, then we can review and if it's correct, merge back to master.

phillee007 · 2017-06-17T07:42:10Z

Cool, thanks! My full name is Phil Lee. Will have a go at some of the unit tests for the code I changed next week, plus do some more testing with proxying/record replay. Am thinking adding an option to name files with url (minus nasty chars) + guid when in record mode might be something i could add. Cheers, Phil

…

On 17/06/2017 7:36 PM, "Stef Heyenrath" ***@***.***> wrote: I sent invite to you for joining the organization. Can you also tell me your full name ? So I can include this in the csproj/nuget. As discussed ; can you please create a separate branch for new functionality, then we can review and if it's correct, merge back to master. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#32 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEEFo9Mo3r7ww_ExZZi3nKwg0ayOzAFpks5sE4HqgaJpZM4N2DvN> .

Add support for client certificate password and test with some real s…

98eed7f

…ervices that require client certificates. Also update so when proxying, the path and query will be passed to the proxied api.

Add correct param

3c26f44

StefH requested changes Jun 11, 2017

View reviewed changes

StefH changed the title ~~For your review: Add support for client certificate password and test with real services that require client certificate auth~~ [Feature] Add support for client certificate password and test with real services that require client certificate auth Jun 13, 2017

StefH added the feature label Jun 13, 2017

Phil Lee added 4 commits June 16, 2017 19:04

Update to read certificate from store instead of file

f3a11fa

Merge branch 'master' of https://github.com/StefH/WireMock.Net

4d9e928

Add support for client certificate password and test with some real s…

3358e8e

…ervices that require client certificates. Also update so when proxying, the path and query will be passed to the proxied api.

Add correct param

70668b2

StefH reviewed Jun 16, 2017

View reviewed changes

Merge branch 'master' of https://github.com/phillee007/WireMock.Net

7dc46a1

Phil Lee added 6 commits June 16, 2017 20:45

Fixup PR issues

699156a

Merge branch 'master' of https://github.com/phillee007/WireMock.Net

753522f

Add support for client certificate password and test with some real s…

1094af7

…ervices that require client certificates. Also update so when proxying, the path and query will be passed to the proxied api.

Add correct param

2648034

Fixup PR issues

a248991

Merge branch 'master' of https://github.com/phillee007/WireMock.Net

af2a035

StefH approved these changes Jun 16, 2017

View reviewed changes

StefH merged commit 49ce2f0 into WireMock-Net:master Jun 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] Add support for client certificate password and test with real services that require client certificate auth #32

[Feature] Add support for client certificate password and test with real services that require client certificate auth #32

phillee007 commented Jun 10, 2017

codecov bot commented Jun 10, 2017 •

edited

Loading

StefH left a comment

StefH Jun 11, 2017

StefH Jun 11, 2017

StefH Jun 11, 2017

StefH Jun 11, 2017

StefH Jun 11, 2017

StefH commented Jun 11, 2017

phillee007 commented Jun 16, 2017

StefH commented Jun 16, 2017

StefH left a comment

StefH Jun 16, 2017

StefH Jun 16, 2017

StefH Jun 16, 2017

StefH commented Jun 16, 2017

phillee007 commented Jun 16, 2017 via email

StefH commented Jun 16, 2017

phillee007 commented Jun 16, 2017 via email

phillee007 commented Jun 16, 2017 via email

StefH commented Jun 16, 2017

StefH commented Jun 17, 2017

phillee007 commented Jun 17, 2017 via email

[Feature] Add support for client certificate password and test with real services that require client certificate auth #32

[Feature] Add support for client certificate password and test with real services that require client certificate auth #32

Conversation

phillee007 commented Jun 10, 2017

codecov bot commented Jun 10, 2017 • edited Loading

Codecov Report

StefH left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StefH commented Jun 11, 2017

phillee007 commented Jun 16, 2017

StefH commented Jun 16, 2017

StefH left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StefH commented Jun 16, 2017

phillee007 commented Jun 16, 2017 via email

StefH commented Jun 16, 2017

phillee007 commented Jun 16, 2017 via email

phillee007 commented Jun 16, 2017 via email

StefH commented Jun 16, 2017

StefH commented Jun 17, 2017

phillee007 commented Jun 17, 2017 via email

codecov bot commented Jun 10, 2017 •

edited

Loading