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

Client side certificates #171

Merged
merged 11 commits into from
Oct 26, 2017
Merged

Client side certificates #171

merged 11 commits into from
Oct 26, 2017

Conversation

aculclasure
Copy link
Contributor

@aculclasure aculclasure commented May 27, 2017

At my workplace, I discovered a need to test HTTPS requests against a server requiring client side certificates as described in the 'Advanced Usage' documentation for the Python Requests library (http://docs.python-requests.org/en/master/user/advanced/). This pull request adds:

  1. 'Create Client Cert Session' keyword that accepts a client key and client certificate (in PEM format)
  2. Updated documentation to describe the new keyword.
  3. Updated tests/testcase.txt to include a test for an HTTPS requests requiring client side certificates.

@aculclasure
Copy link
Contributor Author

Hi,

First, many apologies for the repeated opening/closing of pull requests! I had some errant automation in place that was causing this behavior instead of simply updating my forked repo! Please feel free to let me know if you need any more information from me or need any use cases for this pull request. Thank you!

~ Andrew

@aculclasure aculclasure mentioned this pull request Jul 6, 2017
@vkosuri
Copy link
Contributor

vkosuri commented Jul 7, 2017

@bulkan Any comments on this?

@vkosuri vkosuri mentioned this pull request Jul 11, 2017
@vkosuri
Copy link
Contributor

vkosuri commented Jul 11, 2017

@bulkan Any suggestions/comments?

@tvainika
Copy link
Contributor

There seems to be some extra effort to still support py2.6 here. Official Python 2.6 is already past of its lifetime since 2013-10-29 ( https://docs.python.org/devguide/#status-of-python-branches ). The next version of Pip no longer supports 2.6 (already merged pypa/pip#4343 ) and underlying Requests library is also dropping support for 2.6 (discussion psf/requests#3928 ).

So instead of just flagging py2.6 travis tests as non-critical, what about just dropping those?

@aculclasure
Copy link
Contributor Author

Hi @tvainika ,

Is dropping the py2.6 travis tests something that would be done on the Github side or is it something that I can do within my forked repo?

Andrew

@tvainika
Copy link
Contributor

In this PR there is a modification in .travis.yml to change 2.6 tests as non-critical. IMHO it would make sense to just drop 2.6 env totally from travis configs. But I don't know if this PR was actually correct forum for that discussion anyway.

.travis.yml Outdated
script:
- pybot -P src tests
script:
- if [[ $TRAVIS_PYTHON_VERSION == 2.6 ]];
Copy link
Contributor

Choose a reason for hiding this comment

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

I think i agree with @tvainika comments, @elskohder could you please drop python 2.6 from travis?

@vkosuri
Copy link
Contributor

vkosuri commented Jul 29, 2017

Due to network issue some tests are failed, i restarted the build now it should pass.

@szet
Copy link

szet commented Aug 24, 2017

Hi, I need this functionality as well but it's still not merged with the main branch right? I'm still a bit new to git so can anyone teach me how I can merge this with my local copy of Requests Library?

@vkosuri
Copy link
Contributor

vkosuri commented Aug 25, 2017

@szet
Copy link

szet commented Aug 30, 2017

Thanks! This new keyword helped me a lot in my tests, hopefully it gets merged with the main branch soon :)

@sudhagarc
Copy link

This keyword is really helpful and helped me as well. Could we get this merged into main branch, please? Thanks.

Copy link
Contributor

@vkosuri vkosuri left a comment

Choose a reason for hiding this comment

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

LGTM, @elskohder Greak work, @bulkan any comments/suggestions? can i merge this into master?

@bulkan bulkan merged commit f5ea831 into MarketSquare:master Oct 26, 2017
@surfgirl
Copy link

surfgirl commented Apr 5, 2018

@bulkan Hi, we are trying to implement Request Library and one of the test cases (Get HTTPS & Verify Cert with a CA bundle) to our test suite, and we keep on getting fail with the comment:
"[ ERROR ] Certificate did not match expected hostname: ...". Do you have any idea what can be a problem>

Thank you,

@oleduc
Copy link
Contributor

oleduc commented Apr 5, 2018

@surfgirl The certificate served by the server you are querying does not match the domain you are using to access it. The server might be serving a certificate for domainA.xyz but you are accessing it with domainB.xyz. It could also be that you are accessing it using a sub-domain not covered by your certificate.

@surfgirl
Copy link

surfgirl commented Apr 6, 2018

@oleduc I am not sure if this is a case, because when we perform this test in SOAP UI it works...

@bulkan
Copy link
Collaborator

bulkan commented Apr 8, 2018

@surfgirl can you share your failing test case ?

@surfgirl
Copy link

surfgirl commented Apr 9, 2018

@bulkan

Create UVP session
    [Tags]  Keyword that just creates session, it is important for TC to even run
                    ...  ${SESSION NAME} is alias for session which will be used to call it.
    Create session  ${SESSION NAME}  ${SESSION ADRESS}  verify=${CURDIR}${/}b2csso.pem
    ${resp}=  Get Request  UserPassport  /get
    Should Be Equal As Strings  ${resp.status_code}  200`

${SESSION ADRESS}  https://sodp-uvp-4t.skoda-auto.com/uvp
${SESSION NAME}  UserPassport

@surfgirl
Copy link

surfgirl commented Apr 9, 2018

@bulkan also what is important we originally had p12 certificate and it was converted to pem, do you think that might be a case?

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.

8 participants